Issue #1028: address review — rollback on failure, generic test data, failure tests

Per cbeach47's review of #1030:
- create_attribute: wrap the flush/association/commit in try/except that rolls back
  on any failure, so a failed association can't leave a half-created attribute (makes
  the atomicity guarantee explicit).
- tests: drop the 'Unicon' name (use 'Test Organization'); derive seed names from the
  test name (request.node.name) to avoid cross-test collisions; add two failure tests —
  attribute-creation failure creates no association, and association failure rolls back
  the attribute (no orphan).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Author: bjagg

components/lif/mdr_services/attribute_service.py

@@ -108,22 +108,28 @@ async def create_attribute(session: AsyncSession, data: CreateAttributeDTO):
     attribute = Attribute(**data.dict(exclude={"EntityId"}))
     session.add(attribute)
 
-    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)
+    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.commit()
     await session.refresh(attribute)
     attribute_dto = AttributeDTO.from_orm(attribute)
     return attribute_dto

test/bases/lif/mdr_restapi/test_attribute_atomic_create.py

@@ -3,25 +3,30 @@
 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, no association).
+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 DataModel, DataModelType, Entity, EntityAttributeAssociation
+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):
-    # Unique names per test — the fixture DB is shared across tests in this module.
     dm = DataModel(
         Name=f"AtomicCreateDM_{tag}",
         Type=DataModelType.SourceSchema,
         DataModelVersion="1.0",
-        ContributorOrganization="UniconQA",
+        ContributorOrganization="Test Organization",
         Deleted=False,
     )
     session.add(dm)
@@ -46,9 +51,14 @@ async def _associations_for(session, attribute_id):
     return result.scalars().all()
 
 
-async def test_create_attribute_with_entity_id_creates_association_atomically(test_db_session):
+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, "assoc")
+    dm, entity = await _seed_model_and_entity(session, request.node.name)
 
     created = await create_attribute(
         session,
@@ -68,17 +78,68 @@ async def test_create_attribute_with_entity_id_creates_association_atomically(te
     assert assocs[0].EntityId == entity.Id
 
 
-async def test_create_attribute_without_entity_id_makes_no_association(test_db_session):
+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, "noassoc")
+    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
+            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) == []