Issue #1028: create attribute + association atomically (fix orphaned attributes) (#1030)

Fixes #1028.

## Problem
Adding an attribute in the MDR UI was **two separate,
independently-committed requests** (`POST /attributes/` then `POST
/entity_attribute_associations/`). If the first response was lost
(network/idle-timeout), the client caught the error and never issued the
second — leaving the attribute persisted but **unassociated**: invisible
in the entity view (the list joins through `EntityAttributeAssociation`)
yet blocking re-creation with a uniqueness error. A teammate hit this
twice (`AchievementSubject.id` + `.achievement` on DataModel 3,
demo/lif-team). Confirmed root cause + verified the backend happy path
via a direct two-endpoint test; the failure is specifically the
dropped-response-between-the-two-requests window.

## Fix
**Backend — atomic create**
(`components/lif/mdr_services/attribute_service.py`,
`mdr_dto/attribute_dto.py`):
- `CreateAttributeDTO` gains an optional `EntityId`. When set,
`create_attribute` flushes the attribute then adds the entity
association and **commits once**. A dropped response now leaves either
nothing or a fully-associated (visible) attribute — never an orphan.
- Sets the association's `ExtendedByDataModelId` for extension models
(OrgLIF/PartnerLIF), mirroring the frontend template, so the single call
is correct for extension data models too.
- Backward compatible: no `EntityId` → attribute only, unchanged (import
path etc. unaffected).

**Frontend** (`frontends/mdr-frontend`):
- `ModelExplorer.handleCreateAttribute` passes `EntityId` and drops the
separate `createEntityAttributeAssociation` call — one request instead
of two. (The separate-association dialog for *existing* attributes is
unchanged.)
- `api.ts`: add a **30s request timeout** so a stalled request fails
fast/clearly instead of hanging into the ambiguous "Network error" that
triggered the bug.

## Tests
- Integration tests (live Postgres) in
`test/bases/lif/mdr_restapi/test_attribute_atomic_create.py`: `EntityId`
set → association created atomically; omitted → no association
(backward-compat). Both green.
- Backend pre-commit gate green; frontend `tsc` type-check clean.

## Notes
- `npm run lint` currently crashes repo-wide on a pre-existing
eslint-config typo (`@typescript-eslint/no-explicite-any`) — unrelated
to this change; flagging separately.
- Recovery for already-orphaned attributes: `GET
/attributes/by_data_model_id/{id}?pagination=false` to find, `DELETE
/attributes/{id}` to soft-delete (frees the name). Already cleaned up
the reported two.

##### Type of Change
- [x] Bug fix (non-breaking)
- [x] API endpoints
- [x] frontends/

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Author: bjagg

components/lif/mdr_dto/attribute_dto.py

@@ -53,7 +53,9 @@ class CreateAttributeDTO(BaseModel):
     ExtensionNotes: Optional[str] = None
     Example: Optional[str] = None
     Common: Optional[bool] = None
-    # EntityId: int
+    # When set, the attribute and its association to this entity are created in a single
+    # transaction (see create_attribute) so a dropped response can't orphan the attribute (#1028).
+    EntityId: Optional[int] = None
 
 
 class UpdateAttributeDTO(BaseModel):

components/lif/mdr_services/attribute_service.py

@@ -16,7 +16,7 @@
     CreateAttributeDTO,
     UpdateAttributeDTO,
 )
-from lif.mdr_services.helper_service import check_datamodel_by_id
+from lif.mdr_services.helper_service import check_datamodel_by_id, check_entity_by_id
 from lif.mdr_services.value_set_values_service import check_value_set_exists_by_id
 from lif.mdr_utils.logger_config import get_logger
 from sqlalchemy.ext.asyncio import AsyncSession
@@ -96,9 +96,40 @@ async def create_attribute(session: AsyncSession, data: CreateAttributeDTO):
     if data.ValueSetId:
         await check_value_set_exists_by_id(session=session, id=data.ValueSetId)
 
-    attribute = Attribute(**data.dict())
+    # When an EntityId is supplied, create the attribute AND its entity association in a single
+    # transaction. Previously the UI created these via two separate, independently-committed
+    # requests; if the first response was lost the client never issued the second, leaving the
+    # attribute persisted but unassociated — invisible in the entity view yet blocking re-create
+    # on the unique name (#1028). One commit means a dropped response leaves either nothing or a
+    # fully-associated (visible) attribute, never an orphan.
+    if data.EntityId is not None:
+        await check_entity_by_id(session=session, id=data.EntityId)
+
+    attribute = Attribute(**data.dict(exclude={"EntityId"}))
     session.add(attribute)
-    await session.commit()
+
+    try:
+        if data.EntityId is not None:
+            await session.flush()  # assign attribute.Id without committing
+            # For an extension model (OrgLIF/PartnerLIF — data.Extension is derived from the model's
+            # BaseDataModelId above), the association is an extension placement onto this model, matching
+            # the frontend's tmplCreateEntityAttributeAssociation (ExtendedByDataModelId = the model id).
+            association = EntityAttributeAssociation(
+                EntityId=data.EntityId,
+                AttributeId=attribute.Id,
+                Contributor=data.Contributor,
+                ContributorOrganization=data.ContributorOrganization,
+                Deleted=False,
+                ExtendedByDataModelId=data.DataModelId if data.Extension else None,
+            )
+            session.add(association)
+        await session.commit()
+    except Exception:
+        # Keep the create atomic: if the association or commit fails, roll back the attribute too,
+        # so we never persist a half-created (orphaned) attribute (#1028).
+        await session.rollback()
+        raise
+
     await session.refresh(attribute)
     attribute_dto = AttributeDTO.from_orm(attribute)
     return attribute_dto

frontends/mdr-frontend/src/components/ModelExplorer/ModelExplorer.tsx

@@ -407,21 +407,20 @@ const ModelTree: React.FC<ModelTreeProps> = ({
     const useEAA = parentNode?.type === 'entity';
       /** ^ Always want an association, but should also be under an Entity */
     const inclusionParams: any = useInclusion ? tmplCreateInclusion(model.Id, 0, 'Attribute') : {};
-    const eAAParams: any = useEAA ? tmplCreateEntityAttributeAssociation(parentNode.id, model) : {};
 
     const createdIds: any = {};
     try {
-      const newAttribute: any = await createAttribute({ ...params, DataModelId: model.Id });
+      // When adding under an Entity, pass EntityId so the API creates the attribute AND its
+      // entity association in a single transaction (#1028). This replaces the old two-request
+      // flow (create attribute, then create association), which orphaned the attribute if the
+      // first response was lost — persisted-but-unassociated: invisible + blocking re-create.
+      const newAttribute: any = await createAttribute({
+        ...params,
+        DataModelId: model.Id,
+        ...(useEAA ? { EntityId: parentNode.id } : {}),
+      });
       createdIds.Attribute = newAttribute?.Id;
       debugLog(">> Created Attribute:", newAttribute?.Id, newAttribute);
-      if (newAttribute?.Id && useEAA) {
-        eAAParams.AttributeId = newAttribute?.Id;
-        eAAParams.Contributor = params.Contributor;
-        eAAParams.ContributorOrganization = params.ContributorOrganization;
-        const newEAA: any = await createEntityAttributeAssociation(eAAParams);
-        createdIds.EntityAttributeAssociation = newEAA?.Id;
-        debugLog(">> Created Entity Attribute Association:", newEAA?.Id, newEAA);
-      }
       if (newAttribute?.Id && useInclusion) {
         inclusionParams.IncludedElementId = newAttribute?.Id;
         inclusionParams.Contributor = params.Contributor;

frontends/mdr-frontend/src/services/api.ts

@@ -12,6 +12,10 @@ const api = axios.create({
   // config to set Access-Control-Allow-Credentials: true and an explicit
   // origin (not "*"), which the MDR API already does.
   withCredentials: true,
+  // Fail a stalled request instead of hanging indefinitely. Without a timeout, a request
+  // whose response is lost hangs until the browser/OS gives up, surfacing as an ambiguous
+  // "Network error" — the trigger for the orphaned-attribute class of bugs (#1028).
+  timeout: 30000,
 });
 
 // Add a response interceptor to handle token refresh

frontends/mdr-frontend/src/services/attributesService.ts

@@ -25,6 +25,9 @@ export interface AttributeParams {
   ContributorOrganization?: string | null;
   Extension?: boolean;
   ExtensionNotes?: string | null;
+  // When set, the API creates the attribute AND its association to this entity in one
+  // transaction, so a dropped response can't orphan the attribute (#1028).
+  EntityId?: number;
 }
 
 export interface AttributeDTO {

test/bases/lif/mdr_restapi/test_attribute_atomic_create.py

@@ -0,0 +1,145 @@
+"""Atomic create-attribute (Issue #1028).
+
+Passing ``EntityId`` to ``create_attribute`` must create the attribute AND its
+entity association in a single transaction, so a dropped response can never leave
+an orphaned (persisted-but-unassociated) attribute. Without ``EntityId`` the
+behaviour is unchanged (attribute only). If either step fails, the whole create
+rolls back — never a half-created attribute.
+
+Drives the real service against a live Postgres (``test_db_session`` fixture).
+Seed names are derived from the test name (``request.node.name``) since the
+fixture DB is shared across tests in this module.
+"""
+
+import pytest
+from fastapi import HTTPException
+from sqlmodel import select
+
+from lif.datatypes.mdr_sql_model import Attribute, DataModel, DataModelType, Entity, EntityAttributeAssociation
+from lif.mdr_dto.attribute_dto import CreateAttributeDTO
+from lif.mdr_services import attribute_service
+from lif.mdr_services.attribute_service import create_attribute
+
+
+async def _seed_model_and_entity(session, tag):
+    dm = DataModel(
+        Name=f"AtomicCreateDM_{tag}",
+        Type=DataModelType.SourceSchema,
+        DataModelVersion="1.0",
+        ContributorOrganization="Test Organization",
+        Deleted=False,
+    )
+    session.add(dm)
+    await session.commit()
+    await session.refresh(dm)
+
+    name = f"Widget_{tag}"
+    entity = Entity(Name=name, UniqueName=name, DataModelId=dm.Id, Array="No", Required="No", Deleted=False)
+    session.add(entity)
+    await session.commit()
+    await session.refresh(entity)
+    return dm, entity
+
+
+async def _associations_for(session, attribute_id):
+    result = await session.execute(
+        select(EntityAttributeAssociation).where(
+            EntityAttributeAssociation.AttributeId == attribute_id,
+            EntityAttributeAssociation.Deleted == False,  # noqa: E712
+        )
+    )
+    return result.scalars().all()
+
+
+async def _attributes_named(session, unique_name):
+    result = await session.execute(select(Attribute).where(Attribute.UniqueName == unique_name))
+    return result.scalars().all()
+
+
+async def test_create_attribute_with_entity_id_creates_association_atomically(test_db_session, request):
+    session = test_db_session
+    dm, entity = await _seed_model_and_entity(session, request.node.name)
+
+    created = await create_attribute(
+        session,
+        CreateAttributeDTO(
+            Name="color",
+            UniqueName=f"{entity.UniqueName}.color",
+            DataType="xsd:string",
+            DataModelId=dm.Id,
+            EntityId=entity.Id,
+        ),
+    )
+
+    assert created.Id is not None
+    # The association was created in the same call — the attribute is NOT orphaned.
+    assocs = await _associations_for(session, created.Id)
+    assert len(assocs) == 1
+    assert assocs[0].EntityId == entity.Id
+
+
+async def test_create_attribute_without_entity_id_makes_no_association(test_db_session, request):
+    """Backward-compatible: no EntityId -> attribute only, no association (unchanged behaviour)."""
+    session = test_db_session
+    dm, _entity = await _seed_model_and_entity(session, request.node.name)
+
+    created = await create_attribute(
+        session,
+        CreateAttributeDTO(
+            Name="size", UniqueName=f"{_entity.UniqueName}.size", DataType="xsd:string", DataModelId=dm.Id
+        ),
+    )
+
+    assert created.Id is not None
+    assert await _associations_for(session, created.Id) == []
+
+
+async def test_failure_creating_attribute_creates_no_association(test_db_session, request):
+    """If the attribute step fails (duplicate unique name), the failed attempt creates no association."""
+    session = test_db_session
+    dm, entity = await _seed_model_and_entity(session, request.node.name)
+    dto = CreateAttributeDTO(
+        Name="dup", UniqueName=f"{entity.UniqueName}.dup", DataType="xsd:string", DataModelId=dm.Id, EntityId=entity.Id
+    )
+
+    first = await create_attribute(session, dto)  # succeeds (attribute + association)
+
+    # A second create with the same unique name fails the uniqueness check.
+    with pytest.raises(HTTPException) as exc:
+        await create_attribute(session, dto)
+    assert exc.value.status_code == 400
+
+    # Only the first attribute's association exists — the failed retry added nothing.
+    result = await session.execute(
+        select(EntityAttributeAssociation).where(
+            EntityAttributeAssociation.EntityId == entity.Id,
+            EntityAttributeAssociation.Deleted == False,  # noqa: E712
+        )
+    )
+    assocs = result.scalars().all()
+    assert len(assocs) == 1
+    assert assocs[0].AttributeId == first.Id
+
+
+async def test_failure_creating_association_rolls_back_the_attribute(test_db_session, request, monkeypatch):
+    """If the association step fails, the attribute must roll back too — no orphaned attribute."""
+    session = test_db_session
+    dm, entity = await _seed_model_and_entity(session, request.node.name)
+
+    class _BoomAssociation:
+        def __init__(self, *args, **kwargs):
+            raise RuntimeError("simulated association-creation failure")
+
+    monkeypatch.setattr(attribute_service, "EntityAttributeAssociation", _BoomAssociation)
+
+    unique_name = f"{entity.UniqueName}.rollback"
+    with pytest.raises(RuntimeError):
+        await create_attribute(
+            session,
+            CreateAttributeDTO(
+                Name="rollback", UniqueName=unique_name, DataType="xsd:string", DataModelId=dm.Id, EntityId=entity.Id
+            ),
+        )
+
+    # The attribute must NOT have persisted — it was rolled back with the failed association.
+    assert await _attributes_named(session, unique_name) == []