diff --git a/omop_alchemy/cdm/base/column_mixins.py b/omop_alchemy/cdm/base/column_mixins.py index b2ba3cc..eccb9a6 100644 --- a/omop_alchemy/cdm/base/column_mixins.py +++ b/omop_alchemy/cdm/base/column_mixins.py @@ -46,49 +46,22 @@ class ValueMixin: """ `ValueMixin` - Encodes the OMOP pattern where a value may be represented *either numerically or categorically*. + Encodes the OMOP pattern where a value may be represented *either numerically or categorically*, + via `value_as_number` and/or `value_as_concept_id`. - Structural guarantees: - - - at least one value must be present - - enforced via a check constraint - - validated at assignment time + This mixin only supplies the two columns. It does NOT enforce that either is + populated: OMOP CDM v5.4 does not mandate a value on every row for every + table this mixin is used by (e.g. `Measurement`, `Metadata`), and some + tables represent a third value form (`Observation.value_as_string`) that + this mixin has no way to know about. Subclasses that need a "some value + must be present" guarantee should declare their own check constraint and + `@validates` covering exactly the value columns they actually have. This helps when building generic tooling that needs to handle values flexibly but then normalise for analysis. - - Examples - -------- - >>> from omop_alchemy.cdm.model.clinical.measurement import Measurement - >>> m = Measurement() - >>> m.value_as_number = 42.0 - >>> m.value_as_concept_id = None # OK - >>> m.value_as_number = None # Raises ValueError - """ value_as_number: so.Mapped[Optional[float]] = so.mapped_column(sa.Float, nullable=True) value_as_concept_id: so.Mapped[Optional[int]] = so.mapped_column(sa.ForeignKey("concept.concept_id"), nullable=True) - __table_args__ = ( - sa.CheckConstraint( - "value_as_number IS NOT NULL OR value_as_concept_id IS NOT NULL", - name="ck_value_present", - ), - ) - - @so.validates("value_as_number", "value_as_concept_id") - def _validate_value(self, key, value): - other = ( - self.value_as_concept_id - if key == "value_as_number" - else self.value_as_number - ) - - if value is None and other is None: - raise ValueError( - "At least one of value_as_number or value_as_concept_id must be set" - ) - return value - class DatedEvent: """ `DatedEvent` diff --git a/omop_alchemy/cdm/model/clinical/measurement.py b/omop_alchemy/cdm/model/clinical/measurement.py index ceeb771..aa94954 100644 --- a/omop_alchemy/cdm/model/clinical/measurement.py +++ b/omop_alchemy/cdm/model/clinical/measurement.py @@ -18,7 +18,6 @@ class Measurement(Base, CDMTableBase, ValueMixin): __tablename__ = "measurement" __table_args__ = merge_table_args( - ValueMixin.__table_args__, omop_index(__tablename__, "person_id", cluster=True), omop_index(__tablename__, "measurement_concept_id"), omop_index(__tablename__, "visit_occurrence_id"), diff --git a/omop_alchemy/cdm/model/clinical/observation.py b/omop_alchemy/cdm/model/clinical/observation.py index 0484e22..971eaea 100644 --- a/omop_alchemy/cdm/model/clinical/observation.py +++ b/omop_alchemy/cdm/model/clinical/observation.py @@ -18,7 +18,10 @@ class Observation(Base, CDMTableBase, ValueMixin): __tablename__ = "observation" __table_args__ = merge_table_args( - ValueMixin.__table_args__, + sa.CheckConstraint( + "value_as_number IS NOT NULL OR value_as_concept_id IS NOT NULL OR value_as_string IS NOT NULL", + name="ck_value_present", + ), omop_index(__tablename__, "person_id", cluster=True), omop_index(__tablename__, "observation_concept_id"), omop_index(__tablename__, "visit_occurrence_id"), @@ -54,3 +57,17 @@ def modifier_of_event_id(self) -> Optional[int]: @hybrid_property def modifier_of_field_concept_id(self) -> Optional[int]: return self.obs_event_field_concept_id + + @so.validates("value_as_number", "value_as_concept_id", "value_as_string") + def _validate_value(self, key, value): + others = [ + getattr(self, other_key) + for other_key in ("value_as_number", "value_as_concept_id", "value_as_string") + if other_key != key + ] + if value is None and all(other is None for other in others): + raise ValueError( + "At least one of value_as_number, value_as_concept_id, or " + "value_as_string must be set" + ) + return value diff --git a/omop_alchemy/cdm/model/metadata/metadata.py b/omop_alchemy/cdm/model/metadata/metadata.py index 79501e9..14e052c 100644 --- a/omop_alchemy/cdm/model/metadata/metadata.py +++ b/omop_alchemy/cdm/model/metadata/metadata.py @@ -17,7 +17,6 @@ class Metadata(CDMTableBase, Base, ValueMixin): __tablename__ = "metadata" __table_args__ = merge_table_args( - ValueMixin.__table_args__, omop_index(__tablename__, "metadata_concept_id", cluster=True), omop_index(__tablename__, "metadata_type_concept_id"), ) diff --git a/tests/test_value_mixin.py b/tests/test_value_mixin.py new file mode 100644 index 0000000..b6070f4 --- /dev/null +++ b/tests/test_value_mixin.py @@ -0,0 +1,142 @@ +"""Tests for the relaxed ValueMixin: per-model value requirements. + +Observation requires one of value_as_number/value_as_concept_id/value_as_string. +Measurement and Metadata require no value column to be populated at all. +""" + +from datetime import date + +import pytest +import sqlalchemy.exc as sa_exc + +from omop_alchemy.cdm.model.clinical.measurement import Measurement +from omop_alchemy.cdm.model.clinical.observation import Observation +from omop_alchemy.cdm.model.metadata.metadata import Metadata + +from .conftest import _concept_id + + +@pytest.fixture +def type_concept_id(session): + return _concept_id(session, domain_id="Type Concept") + + +@pytest.fixture +def some_concept_id(session): + return _concept_id(session, domain_id="Condition") + + +def _make_observation(*, observation_id, type_concept_id, some_concept_id, **value_kwargs): + return Observation( + observation_id=observation_id, + person_id=1, + observation_concept_id=some_concept_id, + observation_date=date(2020, 6, 1), + observation_type_concept_id=type_concept_id, + **value_kwargs, + ) + + +def _make_measurement(*, measurement_id, type_concept_id, some_concept_id, **value_kwargs): + return Measurement( + measurement_id=measurement_id, + person_id=1, + measurement_concept_id=some_concept_id, + measurement_date=date(2020, 6, 1), + measurement_type_concept_id=type_concept_id, + **value_kwargs, + ) + + +class TestObservationValueRequirement: + def test_value_as_string_alone_is_sufficient(self, session, type_concept_id, some_concept_id): + """OMOP allows Observation's value via value_as_string alone.""" + obs = _make_observation( + observation_id=9001, + type_concept_id=type_concept_id, + some_concept_id=some_concept_id, + value_as_string="elevated", + ) + session.add(obs) + session.commit() + assert obs.observation_id == 9001 + + def test_value_as_number_alone_is_sufficient(self, session, type_concept_id, some_concept_id): + obs = _make_observation( + observation_id=9002, + type_concept_id=type_concept_id, + some_concept_id=some_concept_id, + value_as_number=42.0, + ) + session.add(obs) + session.commit() + assert obs.observation_id == 9002 + + def test_value_as_concept_id_alone_is_sufficient( + self, session, type_concept_id, some_concept_id + ): + obs = _make_observation( + observation_id=9003, + type_concept_id=type_concept_id, + some_concept_id=some_concept_id, + value_as_concept_id=some_concept_id, + ) + session.add(obs) + session.commit() + assert obs.observation_id == 9003 + + def test_assigning_all_three_none_raises(self, session, type_concept_id, some_concept_id): + """Assignment-time validator still rejects an all-NULL value on Observation.""" + obs = _make_observation( + observation_id=9004, type_concept_id=type_concept_id, some_concept_id=some_concept_id + ) + with pytest.raises(ValueError, match="At least one of"): + obs.value_as_number = None + + def test_no_value_at_all_fails_at_flush(self, session, type_concept_id, some_concept_id): + """Never assigning any value column also fails, at DB flush time.""" + obs = _make_observation( + observation_id=9005, type_concept_id=type_concept_id, some_concept_id=some_concept_id + ) + session.add(obs) + with pytest.raises(sa_exc.IntegrityError): + session.commit() + + +class TestMeasurementNoValueRequirement: + def test_no_value_columns_set_succeeds(self, session, type_concept_id, some_concept_id): + """Measurement has no CDM-mandated value; a bare result row is valid.""" + meas = _make_measurement( + measurement_id=9001, type_concept_id=type_concept_id, some_concept_id=some_concept_id + ) + session.add(meas) + session.commit() + assert meas.measurement_id == 9001 + assert meas.value_as_number is None + assert meas.value_as_concept_id is None + + def test_value_as_number_still_settable(self, session, type_concept_id, some_concept_id): + meas = _make_measurement( + measurement_id=9002, + type_concept_id=type_concept_id, + some_concept_id=some_concept_id, + value_as_number=98.6, + ) + session.add(meas) + session.commit() + assert meas.value_as_number == 98.6 + + +class TestMetadataNoValueRequirement: + def test_no_value_columns_set_succeeds(self, session): + concept_id = _concept_id(session, domain_id="Metadata") + meta = Metadata( + metadata_id=9001, + metadata_concept_id=concept_id, + metadata_type_concept_id=concept_id, + name="fixture-metadata", + ) + session.add(meta) + session.commit() + assert meta.metadata_id == 9001 + assert meta.value_as_string is None