From 33aace3b0d2d76ebfd83863fd22bbf32e63c050d Mon Sep 17 00:00:00 2001 From: Sandy Forrester Date: Tue, 17 Mar 2026 13:18:57 +0000 Subject: [PATCH 1/8] NRL-1995 Access control for allow_supersede_with_delete_failure --- .../create_document_reference.py | 5 +- .../tests/test_create_document_reference.py | 141 ++++++++++++++++- .../tests/test_upsert_document_reference.py | 143 +++++++++++++++++- .../upsert_document_reference.py | 5 +- layer/nrlf/core/constants.py | 1 + layer/nrlf/core/dynamodb/repository.py | 8 + 6 files changed, 294 insertions(+), 9 deletions(-) diff --git a/api/producer/createDocumentReference/create_document_reference.py b/api/producer/createDocumentReference/create_document_reference.py index 4e0deba1c..1fe2aeadf 100644 --- a/api/producer/createDocumentReference/create_document_reference.py +++ b/api/producer/createDocumentReference/create_document_reference.py @@ -254,7 +254,10 @@ def handler( return error_response can_ignore_delete_fail = ( - PERMISSION_SUPERSEDE_IGNORE_DELETE_FAIL in metadata.nrl_permissions + AccessControls.ALLOW_SUPERSEDE_WITH_DELETE_FAILURE.value + in metadata.nrl_permissions_policy.access_controls + if metadata.nrl_permissions_policy + else PERMISSION_SUPERSEDE_IGNORE_DELETE_FAIL in metadata.nrl_permissions ) if ids_to_delete := _get_document_ids_to_supersede( diff --git a/api/producer/createDocumentReference/tests/test_create_document_reference.py b/api/producer/createDocumentReference/tests/test_create_document_reference.py index 079069f97..b92cd4922 100644 --- a/api/producer/createDocumentReference/tests/test_create_document_reference.py +++ b/api/producer/createDocumentReference/tests/test_create_document_reference.py @@ -1435,7 +1435,7 @@ def test_create_document_reference_supersede_deletes_old_pointers_replace( @mock_aws @mock_repository @freeze_uuid("00000000-0000-0000-0000-000000000001") -def test_create_document_reference_supersede_succeeds_with_toggle( +def test_supersede_non_existent_pointer_succeeds_with_v1_ignore_delete_fail( repository: DocumentPointerRepository, ): doc_ref = load_document_reference("Y05868-736253002-Valid") @@ -1493,7 +1493,7 @@ def test_create_document_reference_supersede_succeeds_with_toggle( @mock_aws @mock_repository -def test_create_document_reference_supersede_fails_without_toggle( +def test_supersede_non_existent_pointer_fails_without_v1_ignore_delete_fail( repository: DocumentPointerRepository, ): doc_ref = load_document_reference("Y05868-736253002-Valid") @@ -1544,6 +1544,143 @@ def test_create_document_reference_supersede_fails_without_toggle( } +@mock_aws +@mock_repository +@freeze_uuid("00000000-0000-0000-0000-000000000001") +@patch("nrlf.core.decorators.get_pointer_permissions_v2") +def test_supersede_non_existent_pointer_succeeds_with_v2_access_control( + get_pointer_permissions_mock, repository: DocumentPointerRepository +): + doc_ref = load_document_reference("Y05868-736253002-Valid") + + # Add reference to a non-existing pointer + doc_ref.relatesTo = [ + DocumentReferenceRelatesTo( + code="replaces", + target=Reference(identifier=Identifier(value="Y05868-99999-99999-000000")), + ) + ] + + v2_headers = create_headers( + additional_headers={ + V2Headers.NHSD_END_USER_ORGANISATION_ODS: "Y05868", + V2Headers.NHSD_NRL_APP_ID: "Y05868-TestApp-12345678", + } + ) + v2_headers.pop(CLIENT_RP_DETAILS) + + get_pointer_permissions_mock.return_value = { + "access_controls": [AccessControls.ALLOW_SUPERSEDE_WITH_DELETE_FAILURE.value], + "types": ["http://snomed.info/sct|736253002"], + } + + event = create_test_api_gateway_event( + headers=v2_headers, + body=doc_ref.model_dump_json(exclude_none=True), + ) + + result = handler(event, create_mock_context()) + body = result.pop("body") + + assert result == { + "statusCode": "201", + "headers": { + "Location": "/DocumentReference/Y05868-00000000-0000-0000-0000-000000000001", + **default_response_headers(), + }, + "isBase64Encoded": False, + } + + parsed_body = json.loads(body) + + assert parsed_body == { + "resourceType": "OperationOutcome", + "issue": [ + { + "severity": "information", + "code": "informational", + "details": { + "coding": [ + { + "code": "RESOURCE_SUPERSEDED", + "display": "Resource created and resource(s) deleted", + "system": "https://fhir.nhs.uk/ValueSet/NRL-ResponseCode", + } + ] + }, + "diagnostics": "The document has been superseded by a new version", + } + ], + } + + +@mock_aws +@mock_repository +@patch("nrlf.core.decorators.get_pointer_permissions_v2") +def test_supersede_fails_without_v2_access_control( + get_pointer_permissions_mock, repository: DocumentPointerRepository +): + doc_ref = load_document_reference("Y05868-736253002-Valid") + + # Add reference to a non-existing pointer + doc_ref.relatesTo = [ + DocumentReferenceRelatesTo( + code="replaces", + target=Reference(identifier=Identifier(value="Y05868-99999-99999-000000")), + ) + ] + + v2_headers = create_headers( + additional_headers={ + V2Headers.NHSD_END_USER_ORGANISATION_ODS: "Y05868", + V2Headers.NHSD_NRL_APP_ID: "Y05868-TestApp-12345678", + } + ) + v2_headers.pop(CLIENT_RP_DETAILS) + + get_pointer_permissions_mock.return_value = { + "access_controls": [], + "types": ["http://snomed.info/sct|736253002"], + } + + event = create_test_api_gateway_event( + headers=v2_headers, + body=doc_ref.model_dump_json(exclude_none=True), + ) + + result = handler(event, create_mock_context()) + body = result.pop("body") + + assert result == { + "statusCode": "422", + "headers": default_response_headers(), + "isBase64Encoded": False, + } + + parsed_body = json.loads(body) + + assert parsed_body == { + "resourceType": "OperationOutcome", + "issue": [ + { + "severity": "error", + "code": "business-rule", + "details": { + "coding": [ + { + "code": "UNPROCESSABLE_ENTITY", + "display": "Unprocessable Entity", + "system": "https://fhir.nhs.uk/CodeSystem/Spine-ErrorOrWarningCode", + } + ] + }, + "diagnostics": "The relatesTo target document does not exist", + "expression": ["relatesTo[0].target.identifier.value"], + } + ], + } + + @mock_aws @mock_repository @freeze_uuid("00000000-0000-0000-0000-000000000001") diff --git a/api/producer/upsertDocumentReference/tests/test_upsert_document_reference.py b/api/producer/upsertDocumentReference/tests/test_upsert_document_reference.py index 10f971db4..f359a5f4b 100644 --- a/api/producer/upsertDocumentReference/tests/test_upsert_document_reference.py +++ b/api/producer/upsertDocumentReference/tests/test_upsert_document_reference.py @@ -1411,7 +1411,7 @@ def test_upsert_document_reference_supersede_deletes_old_pointers_replace( @mock_aws @mock_repository -def test_upsert_document_reference_supersede_succeeds_with_toggle( +def test_supersede_non_existent_pointer_succeeds_with_v1_ignore_delete_fail( repository: DocumentPointerRepository, ): doc_ref = load_document_reference("Y05868-736253002-Valid") @@ -1463,13 +1463,10 @@ def test_upsert_document_reference_supersede_succeeds_with_toggle( ], } - non_existent_pointer = repository.get_by_id("Y05868-99999-99999-000000") - assert non_existent_pointer is None - @mock_aws @mock_repository -def test_upsert_document_reference_supersede_fails_without_toggle( +def test_supersede_non_existent_pointer_fails_without_v1_ignore_delete_fail( repository: DocumentPointerRepository, ): doc_ref = load_document_reference("Y05868-736253002-Valid") @@ -1520,6 +1517,142 @@ def test_upsert_document_reference_supersede_fails_without_toggle( } +@mock_aws +@mock_repository +@patch("nrlf.core.decorators.get_pointer_permissions_v2") +def test_supersede_non_existent_pointer_succeeds_with_v2_access_control( + get_pointer_permissions_mock, repository: DocumentPointerRepository +): + doc_ref = load_document_reference("Y05868-736253002-Valid") + + # Add reference to a non-existing pointer + doc_ref.relatesTo = [ + DocumentReferenceRelatesTo( + code="replaces", + target=Reference(identifier=Identifier(value="Y05868-99999-99999-000000")), + ) + ] + + v2_headers = create_headers( + additional_headers={ + V2Headers.NHSD_END_USER_ORGANISATION_ODS: "Y05868", + V2Headers.NHSD_NRL_APP_ID: "Y05868-TestApp-12345678", + } + ) + v2_headers.pop(CLIENT_RP_DETAILS) + + get_pointer_permissions_mock.return_value = { + "access_controls": [AccessControls.ALLOW_SUPERSEDE_WITH_DELETE_FAILURE.value], + "types": ["http://snomed.info/sct|736253002"], + } + + event = create_test_api_gateway_event( + headers=v2_headers, + body=doc_ref.model_dump_json(exclude_none=True), + ) + + result = handler(event, create_mock_context()) + body = result.pop("body") + + assert result == { + "statusCode": "201", + "headers": { + "Location": "/DocumentReference/Y05868-99999-99999-999999", + **default_response_headers(), + }, + "isBase64Encoded": False, + } + + parsed_body = json.loads(body) + + assert parsed_body == { + "resourceType": "OperationOutcome", + "issue": [ + { + "severity": "information", + "code": "informational", + "details": { + "coding": [ + { + "code": "RESOURCE_SUPERSEDED", + "display": "Resource created and resource(s) deleted", + "system": "https://fhir.nhs.uk/ValueSet/NRL-ResponseCode", + } + ] + }, + "diagnostics": "The document has been superseded by a new version", + } + ], + } + + +@mock_aws +@mock_repository +@patch("nrlf.core.decorators.get_pointer_permissions_v2") +def test_supersede_fails_without_v2_access_control( + get_pointer_permissions_mock, repository: DocumentPointerRepository +): + doc_ref = load_document_reference("Y05868-736253002-Valid") + + # Add reference to a non-existing pointer + doc_ref.relatesTo = [ + DocumentReferenceRelatesTo( + code="replaces", + target=Reference(identifier=Identifier(value="Y05868-99999-99999-000000")), + ) + ] + + v2_headers = create_headers( + additional_headers={ + V2Headers.NHSD_END_USER_ORGANISATION_ODS: "Y05868", + V2Headers.NHSD_NRL_APP_ID: "Y05868-TestApp-12345678", + } + ) + v2_headers.pop(CLIENT_RP_DETAILS) + + get_pointer_permissions_mock.return_value = { + "access_controls": [], + "types": ["http://snomed.info/sct|736253002"], + } + + event = create_test_api_gateway_event( + headers=v2_headers, + body=doc_ref.model_dump_json(exclude_none=True), + ) + + result = handler(event, create_mock_context()) + body = result.pop("body") + + assert result == { + "statusCode": "422", + "headers": default_response_headers(), + "isBase64Encoded": False, + } + + parsed_body = json.loads(body) + + assert parsed_body == { + "resourceType": "OperationOutcome", + "issue": [ + { + "severity": "error", + "code": "business-rule", + "details": { + "coding": [ + { + "code": "UNPROCESSABLE_ENTITY", + "display": "Unprocessable Entity", + "system": "https://fhir.nhs.uk/CodeSystem/Spine-ErrorOrWarningCode", + } + ] + }, + "diagnostics": "The relatesTo target document does not exist", + "expression": ["relatesTo[0].target.identifier.value"], + } + ], + } + + @mock_aws @mock_repository def test_upsert_document_reference_create_relatesto_not_replaces( diff --git a/api/producer/upsertDocumentReference/upsert_document_reference.py b/api/producer/upsertDocumentReference/upsert_document_reference.py index c8b5a122c..dd9e87c6c 100644 --- a/api/producer/upsertDocumentReference/upsert_document_reference.py +++ b/api/producer/upsertDocumentReference/upsert_document_reference.py @@ -258,7 +258,10 @@ def handler( return error_response can_ignore_delete_fail = ( - PERMISSION_SUPERSEDE_IGNORE_DELETE_FAIL in metadata.nrl_permissions + AccessControls.ALLOW_SUPERSEDE_WITH_DELETE_FAILURE.value + in metadata.nrl_permissions_policy.access_controls + if metadata.nrl_permissions_policy + else PERMISSION_SUPERSEDE_IGNORE_DELETE_FAIL in metadata.nrl_permissions ) if ids_to_delete := _get_document_ids_to_supersede( diff --git a/layer/nrlf/core/constants.py b/layer/nrlf/core/constants.py index 9ef41b69c..2fa26e088 100644 --- a/layer/nrlf/core/constants.py +++ b/layer/nrlf/core/constants.py @@ -58,6 +58,7 @@ class AccessControls(Enum): ALLOW_PRODUCE_FOR_ANY_AUTHOR = "allow_produce_for_any_author" ALLOW_PRODUCE_FOR_ANY_CUSTODIAN = "allow_produce_for_any_custodian" ALLOW_OVERRIDE_CREATION_DATETIME = "allow_override_creation_datetime" + ALLOW_SUPERSEDE_WITH_DELETE_FAILURE = "allow_supersede_with_delete_failure" @staticmethod def list(): diff --git a/layer/nrlf/core/dynamodb/repository.py b/layer/nrlf/core/dynamodb/repository.py index 26a36deef..f984edbbe 100644 --- a/layer/nrlf/core/dynamodb/repository.py +++ b/layer/nrlf/core/dynamodb/repository.py @@ -352,6 +352,14 @@ def delete_by_id(self, id_: str, can_ignore_delete_fail: bool = False): stacklevel=5, error=str(exc), ) + # Is this what it should be doing, do we want to change the behaviour here? + else: + raise OperationOutcomeError( + status_code="500", + severity="error", + code="exception", + details=SpineErrorConcept.from_code("INTERNAL_SERVER_ERROR"), + ) from exc def _query(self, **kwargs) -> Iterator[DocumentPointer]: """ From 8a0937fc79cc7ddf98687cb5a8ef64fd040bdb5d Mon Sep 17 00:00:00 2001 From: Sandy Forrester Date: Tue, 17 Mar 2026 17:27:11 +0000 Subject: [PATCH 2/8] NRL-1995 Add feature tests --- layer/nrlf/core/dynamodb/repository.py | 14 ++-- scripts/get_s3_permissions.py | 1 + .../v2-permissions-access-controls.feature | 67 +++++++++++++++++++ 3 files changed, 75 insertions(+), 7 deletions(-) diff --git a/layer/nrlf/core/dynamodb/repository.py b/layer/nrlf/core/dynamodb/repository.py index f984edbbe..0700b9752 100644 --- a/layer/nrlf/core/dynamodb/repository.py +++ b/layer/nrlf/core/dynamodb/repository.py @@ -353,13 +353,13 @@ def delete_by_id(self, id_: str, can_ignore_delete_fail: bool = False): error=str(exc), ) # Is this what it should be doing, do we want to change the behaviour here? - else: - raise OperationOutcomeError( - status_code="500", - severity="error", - code="exception", - details=SpineErrorConcept.from_code("INTERNAL_SERVER_ERROR"), - ) from exc + # else: + # raise OperationOutcomeError( + # status_code="500", + # severity="error", + # code="exception", + # details=SpineErrorConcept.from_code("INTERNAL_SERVER_ERROR"), + # ) from exc def _query(self, **kwargs) -> Iterator[DocumentPointer]: """ diff --git a/scripts/get_s3_permissions.py b/scripts/get_s3_permissions.py index dbf2f2538..66f39c287 100644 --- a/scripts/get_s3_permissions.py +++ b/scripts/get_s3_permissions.py @@ -103,6 +103,7 @@ def add_feature_test_files(local_path): [ AccessControls.ALLOW_ALL_TYPES.value, AccessControls.ALLOW_OVERRIDE_CREATION_DATETIME.value, + AccessControls.ALLOW_SUPERSEDE_WITH_DELETE_FAILURE.value, ], ), ], diff --git a/tests/features/producer/v2-permissions-access-controls.feature b/tests/features/producer/v2-permissions-access-controls.feature index 716bc0966..dad872e43 100644 --- a/tests/features/producer/v2-permissions-access-controls.feature +++ b/tests/features/producer/v2-permissions-access-controls.feature @@ -91,3 +91,70 @@ Feature: Producer v2 access_control permissions - Success and Failure Scenarios | url | https://example.org/my-doc.pdf | | practiceSetting | 788002001 | And the date of the resource in the Location header is not '2024-06-01T12:00:00Z' + + Scenario: Successfully supersede a DocumentReference with ALLOW_SUPERSEDE_WITH_DELETE_FAILURE - createDocumentReference + Given the application 'DataShare' (ID 'z00z-y11y-x22x') is registered to access the API + When producer v2 '4LLTYP35P' creates a DocumentReference with values: + | property | value | + | subject | 9278693472 | + | status | current | + | type | 736253002 | + | category | 734163000 | + | custodian | 4LLTYP35P | + | author | 4LLTYP35P | + | url | https://example.org/newdoc.pdf | + | supercedes | 4LLTYP35P-000-ThisRefDoesNotExistSupersedeTest | + Then the response status code is 201 + And the response is an OperationOutcome with 1 issue + And the OperationOutcome contains the issue: + """ + { + "severity": "information", + "code": "informational", + "details": { + "coding": [ + { + "system": "https://fhir.nhs.uk/ValueSet/NRL-ResponseCode", + "code": "RESOURCE_SUPERSEDED", + "display": "Resource created and resource(s) deleted" + } + ] + }, + "diagnostics": "The document has been superseded by a new version" + } + """ + + Scenario: Supersede a DocumentReference fails without ALLOW_SUPERSEDE_WITH_DELETE_FAILURE - createDocumentReference + Given the application 'DataShare' (ID 'z00z-y11y-x22x') is registered to access the API + When producer v2 'RX898' creates a DocumentReference with values: + | property | value | + | subject | 9278693472 | + | status | current | + | type | 736373009 | + | category | 734163000 | + | custodian | RX898 | + | author | RX898 | + | url | https://example.org/newdoc.pdf | + | supercedes | RX898-000-ThisRefDoesNotExistSupersedeTest | + Then the response status code is 422 + And the response is an OperationOutcome with 1 issue + And the OperationOutcome contains the issue: + """ + { + "severity": "error", + "code": "business-rule", + "details": { + "coding": [ + { + "system": "https://fhir.nhs.uk/CodeSystem/Spine-ErrorOrWarningCode", + "code": "UNPROCESSABLE_ENTITY", + "display": "Unprocessable Entity" + } + ] + }, + "diagnostics": "The relatesTo target document does not exist", + "expression": [ + "relatesTo[0].target.identifier.value" + ] + } + """ From dc2d433b0bf83ca9a7cfbabd5cb7b725c252d591 Mon Sep 17 00:00:00 2001 From: Sandy Forrester Date: Wed, 18 Mar 2026 09:04:04 +0000 Subject: [PATCH 3/8] NRL-1995 Remove comment --- layer/nrlf/core/dynamodb/repository.py | 8 -------- 1 file changed, 8 deletions(-) diff --git a/layer/nrlf/core/dynamodb/repository.py b/layer/nrlf/core/dynamodb/repository.py index 0700b9752..26a36deef 100644 --- a/layer/nrlf/core/dynamodb/repository.py +++ b/layer/nrlf/core/dynamodb/repository.py @@ -352,14 +352,6 @@ def delete_by_id(self, id_: str, can_ignore_delete_fail: bool = False): stacklevel=5, error=str(exc), ) - # Is this what it should be doing, do we want to change the behaviour here? - # else: - # raise OperationOutcomeError( - # status_code="500", - # severity="error", - # code="exception", - # details=SpineErrorConcept.from_code("INTERNAL_SERVER_ERROR"), - # ) from exc def _query(self, **kwargs) -> Iterator[DocumentPointer]: """ From 9fa2c1ee088f04a2fbd2059c3ac9d0172d75c91b Mon Sep 17 00:00:00 2001 From: Sandy Forrester Date: Wed, 18 Mar 2026 13:00:19 +0000 Subject: [PATCH 4/8] NRL-1995 Add upsert feature test and cleanup table before scenarios --- tests/features/environment.py | 33 +++++++++++++++++++ .../upsertDocumentReference-failure.feature | 14 ++++---- .../upsertDocumentReference-success.feature | 6 ++-- .../v2-permissions-access-controls.feature | 5 +-- tests/features/steps/2_request.py | 18 +++++----- 5 files changed, 55 insertions(+), 21 deletions(-) diff --git a/tests/features/environment.py b/tests/features/environment.py index 66249009b..c64a19eaf 100644 --- a/tests/features/environment.py +++ b/tests/features/environment.py @@ -10,6 +10,39 @@ from nrlf.core.dynamodb.repository import DocumentPointerRepository +def before_scenario(context: Context, scenario): + """ + Called before each scenario. Deletes every document in the table so that + dirty data from any failed previous run cannot affect this scenario. + Documents set up in 'Given' steps are re-created fresh after this cleanup. + """ + try: + scan_kwargs = {"ProjectionExpression": "pk, sk"} + keys_to_delete = [] + while True: + response = context.repository.table.scan(**scan_kwargs) + keys_to_delete.extend( + {"pk": item["pk"], "sk": item["sk"]} + for item in response.get("Items", []) + ) + last_key = response.get("LastEvaluatedKey") + if not last_key: + break + scan_kwargs["ExclusiveStartKey"] = last_key + + for i in range(0, len(keys_to_delete), 25): + context.repository.table.meta.client.batch_write_item( + RequestItems={ + context.repository.table_name: [ + {"DeleteRequest": {"Key": key}} + for key in keys_to_delete[i : i + 25] + ] + } + ) + except Exception: + pass + + def before_all(context: Context): """ This function is called before all the tests are executed diff --git a/tests/features/producer/upsertDocumentReference-failure.feature b/tests/features/producer/upsertDocumentReference-failure.feature index 192b6439a..fec9bd07e 100644 --- a/tests/features/producer/upsertDocumentReference-failure.feature +++ b/tests/features/producer/upsertDocumentReference-failure.feature @@ -6,7 +6,7 @@ Feature: Producer - upsertDocumentReference - Failure Scenarios | system | value | | http://snomed.info/sct | 1363501000000100 | | http://snomed.info/sct | 736253002 | - When producer 'X26' upserts a DocumentReference with values: + When producer v1 'X26' upserts a DocumentReference with values: | property | value | | id | X26-testid-upsert-0001-0001 | | subject | 9999999999 | @@ -45,7 +45,7 @@ Feature: Producer - upsertDocumentReference - Failure Scenarios | system | value | | http://snomed.info/sct | 1363501000000100 | | http://snomed.info/sct | 736253002 | - When producer 'X26' upserts a DocumentReference with values: + When producer v1 'X26' upserts a DocumentReference with values: | property | value | | subject | 9999999999 | | type | 736253002 | @@ -82,7 +82,7 @@ Feature: Producer - upsertDocumentReference - Failure Scenarios And the organisation 'ANGY1' is authorised to access pointer types: | system | value | | http://snomed.info/sct | 736253002 | - When producer 'ANGY1' upserts a DocumentReference with values: + When producer v1 'ANGY1' upserts a DocumentReference with values: | property | value | | id | X26-testid-upsert-0001-0001 | | subject | 9278693472 | @@ -120,7 +120,7 @@ Feature: Producer - upsertDocumentReference - Failure Scenarios And the organisation 'ANGY1' is authorised to access pointer types: | system | value | | http://snomed.info/sct | 736253002 | - When producer 'ANGY1' upserts a DocumentReference with values: + When producer v1 'ANGY1' upserts a DocumentReference with values: | property | value | | id | X26-testid-upsert-0001-0001 | | subject | 9999999999 | @@ -161,7 +161,7 @@ Feature: Producer - upsertDocumentReference - Failure Scenarios | system | value | | http://snomed.info/sct | 1363501000000100 | | http://snomed.info/sct | 736253002 | - When producer 'X26' upserts a DocumentReference with values: + When producer v1 'X26' upserts a DocumentReference with values: | property | value | | id | X26-testid-upsert-0001-0001 | | subject | 9999999999 | @@ -200,7 +200,7 @@ Feature: Producer - upsertDocumentReference - Failure Scenarios | system | value | | https://nicip.nhs.uk | MAULR | | https://nicip.nhs.uk | MAXIB | - When producer 'ANGY1' upserts a DocumentReference with values: + When producer v1 'ANGY1' upserts a DocumentReference with values: | property | value | | id | ANGY1-testid-upsert-0001-0001 | | subject | 9999999999 | @@ -335,7 +335,7 @@ Feature: Producer - upsertDocumentReference - Failure Scenarios And the organisation 'ANGY1' is authorised to access pointer types: | system | value | | http://snomed.info/sct | 736253002 | - When producer 'ANGY1' upserts a DocumentReference with values: + When producer v1 'ANGY1' upserts a DocumentReference with values: | property | value | | id | TSTCUS-sample-id-00003 | | subject | 9999999999 | diff --git a/tests/features/producer/upsertDocumentReference-success.feature b/tests/features/producer/upsertDocumentReference-success.feature index 9fefb7e66..2492ec602 100644 --- a/tests/features/producer/upsertDocumentReference-success.feature +++ b/tests/features/producer/upsertDocumentReference-success.feature @@ -8,7 +8,7 @@ Feature: Producer - upsertDocumentReference - Success Scenarios And the organisation 'ANGY1' is authorised to access pointer types: | system | value | | http://snomed.info/sct | 736253002 | - When producer 'ANGY1' upserts a DocumentReference with values: + When producer v1 'ANGY1' upserts a DocumentReference with values: | property | value | | id | ANGY1-testid-upsert-0001-0001 | | subject | 9278693472 | @@ -54,7 +54,7 @@ Feature: Producer - upsertDocumentReference - Success Scenarios | system | value | | https://nicip.nhs.uk | MAULR | | https://nicip.nhs.uk | MAXIB | - When producer 'ANGY1' upserts a DocumentReference with values: + When producer v1 'ANGY1' upserts a DocumentReference with values: | property | value | | id | ANGY1-testid-upsert-0001-0001 | | subject | 9278693472 | @@ -95,7 +95,7 @@ Feature: Producer - upsertDocumentReference - Success Scenarios | custodian | ANGY1 | | author | HAR1 | | url | https://example.org/my-doc.pdf | - When producer 'ANGY1' upserts a DocumentReference with values: + When producer v1 'ANGY1' upserts a DocumentReference with values: | property | value | | id | ANGY1-testid-upsert-0001-0002 | | subject | 9278693472 | diff --git a/tests/features/producer/v2-permissions-access-controls.feature b/tests/features/producer/v2-permissions-access-controls.feature index dad872e43..3ca5246a1 100644 --- a/tests/features/producer/v2-permissions-access-controls.feature +++ b/tests/features/producer/v2-permissions-access-controls.feature @@ -92,10 +92,11 @@ Feature: Producer v2 access_control permissions - Success and Failure Scenarios | practiceSetting | 788002001 | And the date of the resource in the Location header is not '2024-06-01T12:00:00Z' - Scenario: Successfully supersede a DocumentReference with ALLOW_SUPERSEDE_WITH_DELETE_FAILURE - createDocumentReference + Scenario: Successfully supersede a DocumentReference with ALLOW_SUPERSEDE_WITH_DELETE_FAILURE - upsertDocumentReference Given the application 'DataShare' (ID 'z00z-y11y-x22x') is registered to access the API - When producer v2 '4LLTYP35P' creates a DocumentReference with values: + When producer v2 '4LLTYP35P' upserts a DocumentReference with values: | property | value | + | id | 4LLTYP35P-testid-upsert-0001-0002 | | subject | 9278693472 | | status | current | | type | 736253002 | diff --git a/tests/features/steps/2_request.py b/tests/features/steps/2_request.py index 733988d83..50f1b9848 100644 --- a/tests/features/steps/2_request.py +++ b/tests/features/steps/2_request.py @@ -96,10 +96,10 @@ def create_post_document_reference_step(context: Context, version: str, ods_code if context.response.status_code == 201: doc_ref_id = context.response.headers["Location"].split("/")[-1] - doc_ref_id.replace( + doc_ref_id = doc_ref_id.replace( "|", "." ) # NRL-766 define and resolve custodian suffix behaviour - context.add_cleanup(lambda: context.repository.delete_by_id(doc_ref_id)) + context.add_cleanup(lambda id=doc_ref_id: context.repository.delete_by_id(id)) def _create_or_upsert_body_step( @@ -120,10 +120,10 @@ def _create_or_upsert_body_step( if context.response.status_code == 201: doc_ref_id = context.response.headers["Location"].split("/")[-1] - doc_ref_id.replace( + doc_ref_id = doc_ref_id.replace( "|", "." ) # NRL-766 define and resolve custodian suffix behaviour - context.add_cleanup(lambda: context.repository.delete_by_id(doc_ref_id)) + context.add_cleanup(lambda id=doc_ref_id: context.repository.delete_by_id(id)) @when( @@ -182,9 +182,9 @@ def update_post_body_step(context: Context, pointer_id: str): context.response = producer_client.update(doc_ref, pointer_id) -@when("producer '{ods_code}' upserts a DocumentReference with values") -def create_put_document_reference_step(context: Context, ods_code: str): - client = producer_client_from_context(context, ods_code) +@when("producer {version} '{ods_code}' upserts a DocumentReference with values") +def create_put_document_reference_step(context: Context, ods_code: str, version: str): + client = producer_client_from_context(context, ods_code, v2=(version == "v2")) if not context.table: raise ValueError("No document reference data table provided") @@ -196,7 +196,7 @@ def create_put_document_reference_step(context: Context, ods_code: str): context.response = client.upsert(doc_ref.model_dump(exclude_none=True)) if context.response.status_code == 201: - context.add_cleanup(lambda: context.repository.delete_by_id(doc_ref_id)) + context.add_cleanup(lambda id=doc_ref_id: context.repository.delete_by_id(id)) @when("producer '{ods_code}' updates a DocumentReference '{doc_ref_id}' with values") @@ -217,7 +217,7 @@ def update_put_document_reference_step( context.response = client.update(doc_ref, doc_ref_id) if context.response.status_code == 200: - context.add_cleanup(lambda: context.repository.delete_by_id(doc_ref_id)) + context.add_cleanup(lambda id=doc_ref_id: context.repository.delete_by_id(id)) @when( From 62e880bc1e918946bf89ba86d399644cf258c770 Mon Sep 17 00:00:00 2001 From: Sandy Forrester Date: Wed, 18 Mar 2026 14:32:56 +0000 Subject: [PATCH 5/8] NRL-1995 Skip db cleanup if using shared resources --- tests/features/environment.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/tests/features/environment.py b/tests/features/environment.py index c64a19eaf..ad7c4fc3d 100644 --- a/tests/features/environment.py +++ b/tests/features/environment.py @@ -12,10 +12,13 @@ def before_scenario(context: Context, scenario): """ - Called before each scenario. Deletes every document in the table so that - dirty data from any failed previous run cannot affect this scenario. - Documents set up in 'Given' steps are re-created fresh after this cleanup. + Called before each scenario unless the the resources (i.e. the DynamoDB tables) are shared for the stack. + Deletes every item in the table so that leftover data created by previous runs doesn't affect the current + scenario. Documents set up in 'Given' steps are re-created fresh after this cleanup. """ + if context.is_shared_resources: + return + try: scan_kwargs = {"ProjectionExpression": "pk, sk"} keys_to_delete = [] From 0402f796e5513bcf3d9b9dbfa1929072c73fe93f Mon Sep 17 00:00:00 2001 From: Sandy Forrester Date: Thu, 19 Mar 2026 15:16:04 +0000 Subject: [PATCH 6/8] NRL-2089 Raise error when delete fails --- layer/nrlf/core/dynamodb/repository.py | 8 ++ .../core/dynamodb/tests/test_repository.py | 119 +++++++++++++++++- 2 files changed, 125 insertions(+), 2 deletions(-) diff --git a/layer/nrlf/core/dynamodb/repository.py b/layer/nrlf/core/dynamodb/repository.py index 26a36deef..7c2ef6954 100644 --- a/layer/nrlf/core/dynamodb/repository.py +++ b/layer/nrlf/core/dynamodb/repository.py @@ -344,6 +344,7 @@ def delete_by_id(self, id_: str, can_ignore_delete_fail: bool = False): key = f"D#{id_}" try: self.table.delete_item(Key={"pk": key, "sk": key}) + logger.log(LogReference.REPOSITORY027, id=id_) except Exception as exc: if can_ignore_delete_fail: logger.log( @@ -352,6 +353,13 @@ def delete_by_id(self, id_: str, can_ignore_delete_fail: bool = False): stacklevel=5, error=str(exc), ) + else: + raise OperationOutcomeError( + status_code="500", + severity="error", + code="exception", + details=SpineErrorConcept.from_code("INTERNAL_SERVER_ERROR"), + ) from exc def _query(self, **kwargs) -> Iterator[DocumentPointer]: """ diff --git a/layer/nrlf/core/dynamodb/tests/test_repository.py b/layer/nrlf/core/dynamodb/tests/test_repository.py index 75a8b605d..7c7588a6e 100644 --- a/layer/nrlf/core/dynamodb/tests/test_repository.py +++ b/layer/nrlf/core/dynamodb/tests/test_repository.py @@ -1,7 +1,33 @@ +from unittest.mock import patch + import pytest +from moto import mock_aws from nrlf.core.constants import PointerTypes -from nrlf.core.dynamodb.repository import _get_sk_ids_for_type +from nrlf.core.dynamodb.model import DocumentPointer +from nrlf.core.dynamodb.repository import ( + DocumentPointerRepository, + _get_sk_ids_for_type, +) +from nrlf.core.errors import OperationOutcomeError +from nrlf.core.log_references import LogReference +from nrlf.tests.data import load_document_reference +from nrlf.tests.dynamodb import mock_repository + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + + +def make_document_pointer(id: str = "Y05868-99999-99999-999999") -> DocumentPointer: + doc_ref = load_document_reference("Y05868-736253002-Valid") + doc_ref.id = id + return DocumentPointer.from_document_reference(doc_ref) + + +# --------------------------------------------------------------------------- +# _get_sk_ids_for_type +# --------------------------------------------------------------------------- def test_get_sk_ids_for_type_exception_thrown_for_invalid_type(): @@ -30,4 +56,93 @@ def test_get_sk_ids_for_type_exception_thrown_if_new_type_has_no_category(): ) -# TODO: Add unit tests for Repository Class +# --------------------------------------------------------------------------- +# DocumentPointerRepository.supersede +# --------------------------------------------------------------------------- + + +@mock_aws +@mock_repository +def test_supersede_creates_new_and_deletes_old(repository: DocumentPointerRepository): + old_doc = make_document_pointer(id="Y05868-OLD0001") + repository.create(old_doc) + assert repository.get_by_id("Y05868-OLD0001") is not None + + new_doc = make_document_pointer(id="Y05868-NEW0001") + result = repository.supersede(new_doc, ids_to_delete=["Y05868-OLD0001"]) + + assert result.id == "Y05868-NEW0001" + assert repository.get_by_id("Y05868-NEW0001") is not None + assert repository.get_by_id("Y05868-OLD0001") is None + + +@mock_aws +@mock_repository +@patch("nrlf.core.dynamodb.repository.logger") +def test_supersede_with_can_ignore_delete_fail( + mock_logger, + repository: DocumentPointerRepository, +): + new_doc = make_document_pointer(id="Y05868-NEW0003") + + # id_to_delete does not exist – with can_ignore_delete_fail=True this should not raise + result = repository.supersede( + new_doc, + ids_to_delete=["Y05868-NONEXISTENT"], + can_ignore_delete_fail=True, + ) + + assert result.id == "Y05868-NEW0003" + log_codes = [c.args[0] for c in mock_logger.log.call_args_list] + assert LogReference.REPOSITORY026a in log_codes + + +# --------------------------------------------------------------------------- +# DocumentPointerRepository.delete_by_id +# --------------------------------------------------------------------------- + + +@mock_aws +@mock_repository +def test_delete_by_id_removes_document_pointer(repository: DocumentPointerRepository): + doc = make_document_pointer() + repository.create(doc) + assert repository.get_by_id(doc.id) is not None + + repository.delete_by_id(doc.id) + + assert repository.get_by_id(doc.id) is None + + +@mock_aws +@mock_repository +def test_delete_by_id_raises_error_when_can_ignore_delete_fail_is_false( + repository: DocumentPointerRepository, +): + with patch.object( + repository.table, + "delete_item", + side_effect=Exception("simulated delete failure"), + ): + with pytest.raises(OperationOutcomeError) as exc_info: + repository.delete_by_id("X26-000000001", can_ignore_delete_fail=False) + + assert exc_info.value.status_code == "500" + + +@mock_aws +@mock_repository +@patch("nrlf.core.dynamodb.repository.logger") +def test_delete_by_id_ignores_error_when_can_ignore_delete_fail_is_true( + mock_logger, + repository: DocumentPointerRepository, +): + with patch.object( + repository.table, + "delete_item", + side_effect=Exception("simulated delete failure"), + ): + repository.delete_by_id("X26-000000001", can_ignore_delete_fail=True) + + log_codes = [c.args[0] for c in mock_logger.log.call_args_list] + assert LogReference.REPOSITORY026a in log_codes From 7cf17f3d216d103d701eb4e3f5e48d22c3bc82dd Mon Sep 17 00:00:00 2001 From: Sandy Forrester Date: Thu, 19 Mar 2026 17:04:58 +0000 Subject: [PATCH 7/8] NRL-2089 Fix unit test --- .../nrlf/core/dynamodb/tests/test_repository.py | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/layer/nrlf/core/dynamodb/tests/test_repository.py b/layer/nrlf/core/dynamodb/tests/test_repository.py index 7c7588a6e..4c7c894ea 100644 --- a/layer/nrlf/core/dynamodb/tests/test_repository.py +++ b/layer/nrlf/core/dynamodb/tests/test_repository.py @@ -85,12 +85,16 @@ def test_supersede_with_can_ignore_delete_fail( ): new_doc = make_document_pointer(id="Y05868-NEW0003") - # id_to_delete does not exist – with can_ignore_delete_fail=True this should not raise - result = repository.supersede( - new_doc, - ids_to_delete=["Y05868-NONEXISTENT"], - can_ignore_delete_fail=True, - ) + with patch.object( + repository.table, + "delete_item", + side_effect=Exception("simulated delete failure"), + ): + result = repository.supersede( + new_doc, + ids_to_delete=["Y05868-NONEXISTENT"], + can_ignore_delete_fail=True, + ) assert result.id == "Y05868-NEW0003" log_codes = [c.args[0] for c in mock_logger.log.call_args_list] From cf198b0af56be8d94de470683c46ce17b4ba7b56 Mon Sep 17 00:00:00 2001 From: Sandy Forrester Date: Thu, 19 Mar 2026 17:11:07 +0000 Subject: [PATCH 8/8] NRL-2089 Amend unit test --- layer/nrlf/core/dynamodb/tests/test_repository.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/layer/nrlf/core/dynamodb/tests/test_repository.py b/layer/nrlf/core/dynamodb/tests/test_repository.py index 4c7c894ea..387df8d0d 100644 --- a/layer/nrlf/core/dynamodb/tests/test_repository.py +++ b/layer/nrlf/core/dynamodb/tests/test_repository.py @@ -129,7 +129,7 @@ def test_delete_by_id_raises_error_when_can_ignore_delete_fail_is_false( side_effect=Exception("simulated delete failure"), ): with pytest.raises(OperationOutcomeError) as exc_info: - repository.delete_by_id("X26-000000001", can_ignore_delete_fail=False) + repository.delete_by_id("Y05868-NONEXISTENT", can_ignore_delete_fail=False) assert exc_info.value.status_code == "500" @@ -146,7 +146,7 @@ def test_delete_by_id_ignores_error_when_can_ignore_delete_fail_is_true( "delete_item", side_effect=Exception("simulated delete failure"), ): - repository.delete_by_id("X26-000000001", can_ignore_delete_fail=True) + repository.delete_by_id("Y05868-NONEXISTENT", can_ignore_delete_fail=True) log_codes = [c.args[0] for c in mock_logger.log.call_args_list] assert LogReference.REPOSITORY026a in log_codes