Skip to content

Relax value-presence constraint per model (Observation/Measurement/Metadata)#33

Open
nicoloesch wants to merge 1 commit into
mainfrom
25-value-mixin
Open

Relax value-presence constraint per model (Observation/Measurement/Metadata)#33
nicoloesch wants to merge 1 commit into
mainfrom
25-value-mixin

Conversation

@nicoloesch

Copy link
Copy Markdown
Collaborator

Summary

Breaking Change

The old shared constraint had a latent bug (single CheckConstraint object reused across three declarative classes) that meant only one table ever actually got it at the DB level. Verify which, if any, apply to your database rather than assume.

IMPORTANT: I cannot test any of this as I don't have the clinical tables locally present. Run this commands with care and verify if they are correct. See additional context below for information.

PostgreSQL

-- PostgreSQL: find the actual constraint name and owning table (don't
--    assume. It depends on this database's import history).
SELECT conname, conrelid::regclass FROM pg_constraint WHERE conname LIKE '%value_present%';

-- If step 2 returns a row (commonly `ck_measurement_ck_value_present` on
--    the `metadata` table, per this repo's canonical import order):
ALTER TABLE metadata DROP CONSTRAINT ck_measurement_ck_value_present;

SQlite changes (probably not super relevant)

-- SQLite equivalent of above (no pg_constraint catalog):
SELECT name, sql FROM sqlite_master WHERE type='table' AND sql LIKE '%value_present%';

-- SQLite has no DROP CONSTRAINT at all. If step above finds a match, the
-- standard workaround is a table rebuild:
--   1. CREATE TABLE metadata_new (... same columns, no CHECK ...);
--   2. INSERT INTO metadata_new SELECT * FROM metadata;
--   3. DROP TABLE metadata;
--   4. ALTER TABLE metadata_new RENAME TO metadata;
--   5. Recreate metadata's indexes as they are dropped with the old table.

Additional context

No migration framework exists in this repo. Verified directly that the old shared ValueMixin.__table_args__ constraint has a latent bug: Measurement, Observation, and Metadata all referenced the same CheckConstraint Python object rather than three independent ones. SQLAlchemy can only bind a constraint object to one table, so it silently re-parents to whichever of the three is constructed last during import.

In this repo's canonical import order (omop_alchemy/cdm/model/__init__.py: clinical, which loads measurement then observation , before several other packages, then metadata last), only metadata ever physically got the DB-level constraint, under the name ck_measurement_ck_value_present (frozen from its first binding, to measurement).

measurement and observation never had a DB-level constraint at all; only the Python-side @validates hook (a decorated method, not a shared object, so it genuinely was active on all three) enforced anything for them. This is import-order-dependent, not guaranteed identical across every deployment/entry point. This requires verification against the actual target database before dropping anything (see above).

@nicoloesch
nicoloesch requested a review from gkennos July 10, 2026 05:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

omop-alchemy currently enforces a shared ValueMixin rule requiring at least one of value_as_number or value_as_concept_id to be set

1 participant