diff --git a/components/lif/mdr_services/schema_upload_service.py b/components/lif/mdr_services/schema_upload_service.py index 8409907..2678ffe 100644 --- a/components/lif/mdr_services/schema_upload_service.py +++ b/components/lif/mdr_services/schema_upload_service.py @@ -40,12 +40,57 @@ def parse_dt(val): raise TypeError(f"Unsupported date value: {val!r}") +# --- Entity-reference detection (#756) --------------------------------------------------- +# MDR's schema generator (schema_generation_service.add_ref) encodes a reference to another +# entity as an INLINED object — NOT an OpenAPI "$ref" — stored under a property key carrying a +# "Ref" infix: "Ref" for a plain reference, or "Ref" when the +# association has a relationship (see schema_generation_service.py:802-805). The inlined object +# is a deep copy of the referenced entity's schema narrowed to its required fields, so when the +# schema was generated with entity metadata it still carries the child's UniqueName/DataModelId. +# +# Before #756 this reader only looked for "$ref" and silently dropped every reference in an +# MDR-exported schema (and, worse, the create pass then materialized each reference as a bogus +# child entity — see create_entity_and_children_if_needed). +# +# NOTE — preferred long-term fix: keying off the "Ref" infix is inherently ambiguous (an entity +# legitimately named "Ref" would mis-parse). The robust fix is two-sided: have add_ref +# stamp an explicit marker on the inlined object (e.g. "x-lif-reference-to": "") and +# key off that here instead of string-parsing the property name. +REFERENCE_KEY_MARKER = "Ref" + + +def parse_reference_key(prop_name: str) -> tuple[Optional[str], str]: + """Split an inlined-reference key into (relationship, child_entity_name). + + "RefOrganization" -> (None, "Organization") + "issuedByRefOrganization" -> ("issuedBy", "Organization") + "isReferencedByRefOrganization" -> ("isReferencedBy", "Organization") + + Split on the LAST "Ref" (``rpartition``), not the first: a relationship name can itself + contain "Ref" (e.g. ``isReferencedBy``, ``refersTo``). Splitting on the first occurrence + would mis-parse ``isReferencedByRefOrganization`` as ("is", "erencedByRefOrganization"), + whose lowercase child then fails the PascalCase guard in ``is_inlined_reference`` and the + reference gets silently dropped. Splitting on the last "Ref" keeps the child entity name + (the trailing PascalCase token) intact. (A child entity legitimately named ``Ref...`` is the + residual ambiguity the module NOTE's two-sided marker fix would resolve.) + """ + relationship, _, child_name = prop_name.rpartition(REFERENCE_KEY_MARKER) + return (relationship or None), child_name + + +def is_inlined_reference(prop_name: str, prop) -> bool: + """True if `prop` is an MDR inlined entity reference rather than an embedded child or attribute.""" + if not isinstance(prop, dict) or "$ref" in prop or "ValueSetId" in prop: + return False + _, child_name = parse_reference_key(prop_name) + # The marker must be followed by a PascalCase entity name; this keeps an embedded child whose + # own name merely contains "Ref" (e.g. "Reference") from being misread as a reference. + return bool(child_name) and child_name[0].isupper() + + async def create_reference_entity_association_if_needed( - session: AsyncSession, ref_name, referenced_entity, parent_entity_id, data_model_id + session: AsyncSession, referenced_entity, parent_entity_id, relationship, data_model_id ): - # relationship = the text prepended on ref_name that is not part of the referenced entity name - # e.g. for "issuedByOrganization" if referenced_entity.Name is "Organization", relationship is "issuedBy" - relationship = ref_name[: ref_name.index(referenced_entity.Name)] # Check if an EntityAssociation already exists entity_association = await get_entity_association_by_parent_child_relationship( session, parent_entity_id, referenced_entity.Id, relationship, data_model_id @@ -69,52 +114,64 @@ async def create_reference_entity_association_if_needed( async def create_reference_associations_for_children( - session: AsyncSession, entity_md: Dict, data_model_id: int, openapi_schema: Dict, data_model_type: str + session: AsyncSession, + entity_name: str, + entity_md: Dict, + data_model_id: int, + openapi_schema: Dict, + data_model_type: str, ): entity_properties = entity_md.get("properties", {}) for prop_name, prop in entity_properties.items(): - # KNOWN BUG (#756): this only detects genuine OpenAPI "$ref" properties, but MDR's own - # schema generator (schema_generation_service.add_ref) INLINES references rather than - # emitting "$ref", and marks them with a "Ref" infix in the property key - # ("Ref" / "Ref", see schema_generation_service.py:802-805). - # So re-uploading an MDR-exported schema silently drops every reference here. - # - # Tactical fix: also treat a "Ref"-keyed inlined object as a reference (the inlined object - # IS the referenced schema, so read its UniqueName/DataModelId directly) and parse the - # relationship as prop_name.split("Ref", 1)[0]. - # Preferred fix (more robust, two-sided): have the generator stamp an explicit marker on - # the inlined object on export — e.g. "x-reference-to": "" — and key off that - # here instead of string-sniffing "Ref" out of property names (which mis-parses any entity - # legitimately named with "Ref" in it). - if "$ref" in prop: # This is a reference to another entity - ref_path = prop[ - "$ref" - ] # Assume path is always in format like #/components/schemas/EntityName or #/components/schemas/ParentEntityName/properties/ChildEntityName (this could continue for deeper nesting) - # 1. Find where it is in the openapi_schema - referenced_entity_md = resolve_ref(openapi_schema, ref_path) - # 2. Determine its Entity Id + if not isinstance(prop, dict): + continue + + referenced_entity_md = None + relationship = None + child_name = None + if "$ref" in prop: # externally-authored OpenAPI reference + # Path is in a format like #/components/schemas/EntityName (possibly nested deeper). + referenced_entity_md = resolve_ref(openapi_schema, prop["$ref"]) + elif is_inlined_reference(prop_name, prop): # MDR-exported inlined reference (#756) + referenced_entity_md = prop + relationship, child_name = parse_reference_key(prop_name) + + if referenced_entity_md is not None: + # Resolve the referenced entity. Prefer UniqueName from embedded metadata; otherwise + # fall back to the child name parsed from the key (entities created in the first pass + # default their UniqueName to their schema-key name). + referenced_unique_name = referenced_entity_md.get("UniqueName") or child_name referenced_entity = await get_unique_entity( + session, referenced_unique_name, data_model_id, referenced_entity_md.get("DataModelId"), data_model_type + ) + parent_entity = await get_unique_entity( session, - referenced_entity_md.get("UniqueName"), + entity_md.get("UniqueName") or entity_name, data_model_id, - referenced_entity_md.get("DataModelId"), + entity_md.get("DataModelId"), data_model_type, ) - logger.info(f"Referenced entity unique name: {referenced_entity_md.get('UniqueName')}") - # Determine parent entity id - logger.info(f"Parent entity: {entity_md}") - parent_entity = await get_unique_entity( - session, entity_md.get("UniqueName"), data_model_id, entity_md.get("DataModelId"), data_model_type - ) - parent_entity_id = parent_entity.Id - # 3. Create an EntityAssociation if needed - await create_reference_entity_association_if_needed( - session, prop_name, referenced_entity, parent_entity_id, data_model_id - ) - # Go through this process recursively + if referenced_entity and parent_entity: + if child_name is None: + # $ref path: derive the relationship from the property-name prefix, e.g. + # "issuedByOrganization" -> "issuedBy" for referenced entity "Organization". + # idx > 0 means there is a non-empty prefix before the entity name; idx == 0 + # (no prefix) or idx == -1 (name not in the key) both mean no relationship. + idx = prop_name.find(referenced_entity.Name) + relationship = prop_name[:idx] if idx > 0 else None + logger.info(f"Reference {prop_name!r} -> entity {referenced_unique_name!r} (rel={relationship!r})") + await create_reference_entity_association_if_needed( + session, referenced_entity, parent_entity.Id, relationship, data_model_id + ) + else: + logger.warning(f"Could not resolve reference {prop_name!r} on entity {entity_name!r}") + # An inlined reference is a leaf (narrowed required fields); do not recurse into it. + continue + + # Recurse into genuine child entities. if "properties" in prop: await create_reference_associations_for_children( - session, prop, data_model_id, openapi_schema, data_model_type + session, prop_name, prop, data_model_id, openapi_schema, data_model_type ) @@ -288,8 +345,9 @@ async def create_attribute_if_needed( ) session.add(inclusion) - # If needed, create EntityAttributeAssociation - if parent_entity_id: + # If needed, create EntityAttributeAssociation. `is not None`, not truthiness: a parent + # entity id of 0 is a valid PK, not "no parent" (same class as the #1006 ElementId fix). + if parent_entity_id is not None: # Check if an EntityAttributeAssociation already exists association = await check_existing_association(session, parent_entity_id, attribute.Id, data_model_id) if not association: # If the EntityAttributeAssociation does not exist, create it @@ -470,8 +528,11 @@ async def create_entity_and_children_if_needed( # Process child entities if any entity_properties = entity_md.get("properties", {}) for prop_name, prop in entity_properties.items(): - if "$ref" in prop: - continue # Skip $ref properties for now; we need to make sure all entities are created to reference to first. + if "$ref" in prop or is_inlined_reference(prop_name, prop): + # References (OpenAPI "$ref" or MDR inlined "Ref") point at entities created + # elsewhere; they are wired up in the post-pass (create_reference_associations_for_children), + # not materialized as embedded child entities here. (#756) + continue if "ValueSetId" not in prop: # It's a child entity child_entity = await create_entity_and_children_if_needed( session, prop_name, prop, data_model_id, contributor, contributor_organization, data_model_type @@ -607,7 +668,7 @@ async def create_data_model_from_openapi_schema( ## After everything has been created, process references for schema_name, schema in schemas.items(): await create_reference_associations_for_children( - session, schema, new_data_model.Id, openapi_schema, data_model_type + session, schema_name, schema, new_data_model.Id, openapi_schema, data_model_type ) await session.commit() diff --git a/test/bases/lif/mdr_restapi/test_schema_roundtrip.py b/test/bases/lif/mdr_restapi/test_schema_roundtrip.py new file mode 100644 index 0000000..f9a73c4 --- /dev/null +++ b/test/bases/lif/mdr_restapi/test_schema_roundtrip.py @@ -0,0 +1,113 @@ +"""End-to-end round-trip test for entity references (Issue #756). + +Drives both halves of the real pipeline against a live Postgres database: + + seed a source model with a Reference association + -> generate_openapi_schema (the export the UI/API hands back) + -> create_data_model_from_openapi_schema (the create-by-upload import) + +and asserts the reference survives the trip — i.e. the generator's inlined +"Ref" property is understood by the upload reader, recreated +as a Reference-placement EntityAssociation, and NOT materialized as a bogus child +entity. Before #756 the reference was silently dropped on upload. +""" + +from sqlmodel import select + +from lif.datatypes.mdr_sql_model import DataModel, DataModelType, Entity, EntityAssociation, EntityPlacementType +from lif.mdr_services.schema_generation_service import generate_openapi_schema +from lif.mdr_services.schema_upload_service import create_data_model_from_openapi_schema + + +async def _seed_source_model(session): + """A minimal source model: Person --issuedBy(Reference)--> Organization.""" + dm = DataModel( + Name="RoundTripSource", + Type=DataModelType.SourceSchema, + DataModelVersion="1.0", + ContributorOrganization="UniconQA", + Deleted=False, + ) + session.add(dm) + await session.commit() + await session.refresh(dm) + + person = Entity(Name="Person", UniqueName="Person", DataModelId=dm.Id, Array="No", Required="No", Deleted=False) + org = Entity( + Name="Organization", UniqueName="Organization", DataModelId=dm.Id, Array="No", Required="No", Deleted=False + ) + session.add(person) + session.add(org) + await session.commit() + await session.refresh(person) + await session.refresh(org) + + assoc = EntityAssociation( + ParentEntityId=person.Id, + ChildEntityId=org.Id, + Relationship="issuedBy", + Placement=EntityPlacementType.Reference, + Deleted=False, + ) + session.add(assoc) + await session.commit() + return dm + + +async def test_reference_survives_generate_then_upload(test_db_session): + session = test_db_session + source_dm = await _seed_source_model(session) + + # --- export: the schema the generator produces for the source model --- + schema = await generate_openapi_schema(session, source_dm.Id, include_attr_md=True, include_entity_md=True) + person_props = schema["components"]["schemas"]["Person"]["properties"] + # The reference is inlined under a "Ref"-infix key, not emitted as a "$ref". + assert "issuedByRefOrganization" in person_props + assert "$ref" not in person_props["issuedByRefOrganization"] + + # --- import: round-trip that schema back in through create-by-upload --- + target = await create_data_model_from_openapi_schema( + session=session, + openapi_schema=schema, + data_model_name="RoundTripTarget", + data_model_version="1.0", + data_model_type="SourceSchema", + data_model_description=None, + base_data_model_id=None, + use_considerations=None, + notes=None, + activation_date=None, + deprecation_date=None, + contributor=None, + contributor_organization="UniconQA", + ) + + # The reference must NOT have been materialized as a child entity. + entities = ( + (await session.execute(select(Entity).where(Entity.DataModelId == target.Id, Entity.Deleted == False))) + .scalars() + .all() + ) + names = sorted(e.Name for e in entities) + assert names == ["Organization", "Person"], names + assert not any("Ref" in n for n in names) + + # The reference association must have been recreated, parent->child, with its relationship. + by_name = {e.Name: e.Id for e in entities} + assocs = ( + ( + await session.execute( + select(EntityAssociation) + .join(Entity, Entity.Id == EntityAssociation.ParentEntityId) + .where(Entity.DataModelId == target.Id, EntityAssociation.Deleted == False) + ) + ) + .scalars() + .all() + ) + references = [a for a in assocs if a.Placement == EntityPlacementType.Reference] + assert len(references) == 1, [(a.Placement, a.Relationship) for a in assocs] + ref = references[0] + assert ref.ParentEntityId == by_name["Person"] + assert ref.ChildEntityId == by_name["Organization"] + assert ref.Relationship == "issuedBy" diff --git a/test/components/lif/mdr_services/test_schema_upload_service.py b/test/components/lif/mdr_services/test_schema_upload_service.py new file mode 100644 index 0000000..b1a00e6 --- /dev/null +++ b/test/components/lif/mdr_services/test_schema_upload_service.py @@ -0,0 +1,143 @@ +"""Tests for create-by-upload reference handling (Issue #756). + +MDR's schema generator inlines an entity reference as an object stored under a "Ref"-infix +property key ("Ref" / "Ref") rather than an OpenAPI "$ref". The +upload reader used to look only for "$ref", so references in an MDR-exported schema were +silently dropped (and materialized as bogus child entities). These tests cover the key parsing, +the reference/child discrimination, and the reference-association post-pass. +""" + +import types +import pytest +from unittest.mock import AsyncMock, MagicMock + +# asyncio_mode = auto (see pyproject) runs the async tests; sync helper tests stay sync. + +svc = pytest.importorskip("lif.mdr_services.schema_upload_service") + + +# --- pure helpers --------------------------------------------------------------------------- + + +def test_parse_reference_key_plain(): + assert svc.parse_reference_key("RefOrganization") == (None, "Organization") + + +def test_parse_reference_key_with_relationship(): + assert svc.parse_reference_key("issuedByRefOrganization") == ("issuedBy", "Organization") + + +def test_parse_reference_key_relationship_containing_ref(): + # A relationship name can itself contain "Ref" (isReferencedBy, refersTo). rpartition splits + # on the LAST "Ref", so the child entity name stays intact; splitting on the first would yield + # ("is", "erencedByRefOrganization") whose lowercase child fails the PascalCase guard and the + # reference would be silently dropped (cbeach47 #1007 review). + assert svc.parse_reference_key("isReferencedByRefOrganization") == ("isReferencedBy", "Organization") + assert svc.is_inlined_reference("isReferencedByRefOrganization", {"type": "object"}) is True + + +def test_parse_reference_key_has_relevant_relationship_name_is_lost(): + # KNOWN, generator-side loss (tracked in #1062): the generator encodes has*/relevant* + # relationships WITHOUT the name (schema_generation_service.py:802 -> "Ref" + child), so + # e.g. hasManager exports as "RefEmployee". The reader can't recover a name that was never + # emitted, so it round-trips to relationship=None. Asserted here so the loss is documented, + # not hidden (the round-trip test only uses issuedBy, which survives). + assert svc.parse_reference_key("RefEmployee") == (None, "Employee") + + +@pytest.mark.parametrize( + "prop_name,prop,expected", + [ + ("RefOrganization", {"type": "object", "properties": {}}, True), + ("issuedByRefOrganization", {"type": "object", "properties": {}}, True), + # embedded child whose own name merely contains "Ref" must NOT be read as a reference + ("Reference", {"type": "object", "properties": {}}, False), + # a real $ref property is handled by its own branch, not as an inlined reference + ("RefOrganization", {"$ref": "#/components/schemas/Organization"}, False), + # attributes carry ValueSetId + ("someAttr", {"ValueSetId": 1}, False), + # no marker at all + ("hasOrganization", {"type": "object", "properties": {}}, False), + ], +) +def test_is_inlined_reference(prop_name, prop, expected): + assert svc.is_inlined_reference(prop_name, prop) is expected + + +# --- reference post-pass -------------------------------------------------------------------- + + +@pytest.fixture +def patched_lookups(monkeypatch): + """Resolve entities by UniqueName and report no pre-existing association.""" + entities = { + "Person": types.SimpleNamespace(Id=1, Name="Person"), + "Organization": types.SimpleNamespace(Id=2, Name="Organization"), + } + + async def fake_get_unique_entity(session, unique_name, data_model_id, base_data_model_id, data_model_type): + return entities.get(unique_name) + + monkeypatch.setattr(svc, "get_unique_entity", AsyncMock(side_effect=fake_get_unique_entity)) + monkeypatch.setattr(svc, "get_entity_association_by_parent_child_relationship", AsyncMock(return_value=None)) + return entities + + +def _added_associations(session): + return [c.args[0] for c in session.add.call_args_list if isinstance(c.args[0], svc.EntityAssociation)] + + +async def test_inlined_reference_creates_reference_association(patched_lookups): + session = MagicMock() + session.add = MagicMock() + entity_md = { + "UniqueName": "Person", + "DataModelId": 7, + "properties": { + "issuedByRefOrganization": { + "type": "object", + "UniqueName": "Organization", + "DataModelId": 7, + "properties": {"identifier": {"type": "string"}}, + } + }, + } + + await svc.create_reference_associations_for_children(session, "Person", entity_md, 7, {}, "SourceSchema") + + associations = _added_associations(session) + assert len(associations) == 1 + assoc = associations[0] + assert assoc.ParentEntityId == 1 + assert assoc.ChildEntityId == 2 + assert assoc.Relationship == "issuedBy" + assert assoc.Placement == "Reference" + + +async def test_plain_reference_has_no_relationship(patched_lookups): + session = MagicMock() + session.add = MagicMock() + entity_md = { + "UniqueName": "Person", + "properties": {"RefOrganization": {"type": "object", "UniqueName": "Organization", "properties": {}}}, + } + + await svc.create_reference_associations_for_children(session, "Person", entity_md, 7, {}, "SourceSchema") + + associations = _added_associations(session) + assert len(associations) == 1 + assert associations[0].Relationship is None + + +async def test_embedded_child_does_not_create_reference_association(patched_lookups): + """A non-reference child (no "Ref" marker) is recursed into, not turned into a reference.""" + session = MagicMock() + session.add = MagicMock() + entity_md = { + "UniqueName": "Person", + "properties": {"hasOrganization": {"type": "object", "UniqueName": "Organization", "properties": {}}}, + } + + await svc.create_reference_associations_for_children(session, "Person", entity_md, 7, {}, "SourceSchema") + + assert _added_associations(session) == []