Issue #756: honor inlined entity references on schema create-by-upload

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<Child>" for a plain reference, or "<relationship>Ref<Child>" 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<Something>" 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": "<UniqueName>") 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<Child>" / "<relationship>Ref<Child>", 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": "<UniqueName>" — 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<Child>") 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()

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
+"<relationship>Ref<Child>" 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"