[NDR-423] CORE DB to use ID, all endpoints ignore SNOMED code by jameslinnell · Pull Request #1151 · NHSDigital/national-document-repository

jameslinnell · 2026-03-04T14:00:22Z

Overview

Jira ticket: NDR-423

Description

Changes CORE db calls to use the updated DB design. All FHIR endpoints now longer require snomed code as part of their request

Context

SNOMED is a redundant parameter, IDs were previously not FHIR compliant.

Checklist

Tasks for all changes:

1. I have linked this PR to its Jira ticket.
2. I have run git pre-commits.
3. I have added and/or updated relevant tests.
4. I have updated relevant documentation.
5. I have considered the cross-team impact (and have PR approval from both Core & Demographics if necessary).
6. I have successfully deployed this change to a sandbox and witnessed unit, e2e and smoke tests passing:
- 6a. Deploy - Sandbox
- 6b. SANDBOX Full- Deploy feature branch to sandbox

github-actions · 2026-03-04T14:12:14Z

Code security issues found

View full details here.

sonarqubecloud · 2026-03-04T14:16:36Z

Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
91.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

megan-bower4 · 2026-03-06T16:18:13Z

lambdas/handlers/get_fhir_document_reference_handler.py


-    if not document_id or not snomed_code:
+    if not document_id:
        logger.error("Missing document id or snomed code in request path parameters.")


Remove reference to snomed code here

megan-bower4 · 2026-03-06T16:21:49Z

lambdas/handlers/get_fhir_document_reference_handler.py

+        return SnomedCodes.LLOYD_GEORGE.value
+
+    if common_name not in MtlsCommonNames:
+        logger.error(f"mTLS common name {common_name} - is not supported")


This isn't really to do with this PR, more a note to what Fraser was saying about updating the ssm param - I would expect there to be a fhir lambda error if an mtls request is used, with an invalid common name, but it seemed like access and store were alerted to an error on their stuff, implying it went down the LG route?

I think there's a ticket to look into Mtls errors

megan-bower4 · 2026-03-06T16:27:43Z

lambdas/services/document_service.py


-            document = model_class.model_validate(response["Item"])
-            return document
+            if not return_deleted:


Might need a run through of this - would it be simpler to reverse this and if return_deleted exit earlier?

megan-bower4 · 2026-03-06T16:29:13Z

lambdas/services/document_service.py

    def get_item_agnostic(
        self,
-        partion_key: dict,
+        partition_key: dict,


Ooh surprised this wasn't causing issues before!

megan-bower4 · 2026-03-06T16:30:40Z

lambdas/services/get_fhir_document_reference_service.py

+        document_id: str,
+        table,
+    ) -> DocumentReference | None:
+        documentreference = self.document_service.get_item(


Should this be document_reference to keep with existing syntax patterns

megan-bower4 · 2026-03-06T16:32:24Z

lambdas/services/upload_document_reference_service.py

            object_parts = object_key.split("/")
            document_key = object_parts[-1]
-            nhs_number = None
-            if len(object_parts) > 1:


Is this not a shared service used by access & store too? Would they still need this as their ID's are made up of snomed?

I added this in, now I'm removing it. It's not needed anymore.

megan-bower4 · 2026-03-06T16:33:32Z

lambdas/services/upload_document_reference_service.py

+                if not documents:
                    logger.error(
-                        f"Failed to process object key with ID: {document_key}",
+                        f"No document with the following key found in {self.table_name} table: {document_key}",


What is document_key in this scenario?

document reference ID

megan-bower4 · 2026-03-06T16:35:58Z

lambdas/tests/e2e/api/fhir/test_mtls_api_failure.py

-    assert len(body["issue"]) > 0
-    issue = body["issue"][0]
-    assert issue["severity"] == "error"
+# Commented this out for now as this will no longer retunr a 500 but now a 400


Do we have a ticket for this to be fixed in? And should we remove the test or change it to assert a 400 for now, to keep the coverage?

I have added 400 invalid tests, so this is covered. Yeah Matt added this test in his PR to fix 500 FHIR response. but now it doesn't return a 500. Could be worth talking it through

megan-bower4 · 2026-03-06T16:36:46Z

lambdas/tests/e2e/api/fhir/test_retrieve_document_fhir_api_failure.py

+        (f"{pdm_data_helper.snomed_code}+{str(uuid.uuid4())}", 400, "invalid"),
+        (f"{pdm_data_helper.snomed_code}&{str(uuid.uuid4())}", 400, "invalid"),
+        (f"{pdm_data_helper.snomed_code}{str(uuid.uuid4())}", 400, "invalid"),
+        (f"{str(uuid.uuid4())}~{pdm_data_helper.snomed_code}", 400, "invalid"),


Can you add empty string here?

Ah can see this is on line 113 - can it be combined into this test?

TBH, that test works in a different way. It's easier to leave as separate tests.

megan-bower4 · 2026-03-06T16:43:37Z

lambdas/tests/e2e/api/fhir/test_retrieve_document_fhir_api_failure.py

    _assert_operation_outcome(body=body, code="MISSING_VALUE")


 def test_no_snomed_or_document_id_in_path_param_id():


Is this one redundant now?

no, there is a still a function to look for path params. IT's not used but may be used in the future. I think as it doesn''t affect the current code it can be left in

megan-bower4 · 2026-03-06T16:59:35Z

lambdas/tests/e2e/helpers/data_helper.py

            "pdm_record",
        )

-    def retrieve_document_reference(self, record):


Don't know if I'm missing something here but don't we need these? We reference data_helper.retrieve_document_reference a lot

They are no longer required to be different compared to the same methods on the base class so, we don't need them on the pdm_helper class anymore.

megan-bower4 · 2026-03-06T17:03:22Z

lambdas/tests/unit/handlers/test_get_fhir_document_reference_handler.py

    mock_document_service.handle_get_document_reference_request.assert_not_called()


-def test_lambda_handler_id_malformed(


Why is this one not needed anymore?

Because
MOCK_INVALID_EVENT_ID_MALFORMED["pathParameters"]["id"] = f"~{TEST_UUID}"

This is now a valid ID. the function that gets the ID will split by ~ and take the last item.

Invalid IDs would be non-uuids and a test for that has been added by

def test_get_id_from_path_parameters_raises_error(path_parameter):

megan-bower4 · 2026-03-06T17:10:19Z

lambdas/tests/unit/services/test_pdm_get_fhir_document_reference_by_id_service.py

+    patched_service.document_service.get_item.return_value = None

-    with pytest.raises(GetFhirDocumentReferenceException) as exc_info:
+    with pytest.raises(GetFhirDocumentReferenceException) as excinfo:


Isn't exc_info the standard everywhere else?

jameslinnell requested review from a team as code owners March 4, 2026 14:00

jameslinnell requested a review from AndyFlintAnswerDigital March 4, 2026 14:00

jameslinnell had a problem deploying to development March 4, 2026 14:00 — with GitHub Actions Failure

jameslinnell added 18 commits March 4, 2026 14:01

[NDR-423] FHIR GET and SEARCH by ID

b4fe4ce

[NDR-423] Unit tests

5b828aa

[NDR-423] e2e tests

282a75e

[NDR-423] 404 test

4a309ee

[NDR-423] Don't return deleted items on get_item

509df15

[NDR-423] Search tests

0e7d350

[NDR-423] Docker fix

ab85d1e

[NDR-423] Update unit tests

6b4fd56

[NDR-423] upload service now checks PDM by ID

a06c666

[NDR-423] LG FHIR E2E tests fix

448796e

[NDR-423] Update file update

5da9626

[NDR-423] Fix upload test

437f761

[NDR-423] LG FHIR test

0e300f4

[NDR-423] Removed now broken 500 error

b368279

[NDR-423] Update tests

e776cd6

[NDR-423] Update tests

5101627

[NDR-423] Fix DocStatus tests

7be0a9f

[NDR-423] Update DocStatus

6113c56

jameslinnell force-pushed the NDR-423 branch from 61afa8a to 6113c56 Compare March 4, 2026 14:01

jameslinnell had a problem deploying to development March 4, 2026 14:01 — with GitHub Actions Failure

[NDR-423] Remove print

90d8986

jameslinnell had a problem deploying to development March 4, 2026 14:11 — with GitHub Actions Failure

megan-bower4 reviewed Mar 6, 2026

View reviewed changes

		_assert_operation_outcome(body=body, code="MISSING_VALUE")


		def test_no_snomed_or_document_id_in_path_param_id():

		mock_document_service.handle_get_document_reference_request.assert_not_called()


		def test_lambda_handler_id_malformed(

Conversation

jameslinnell commented Mar 4, 2026

Overview

Description

Context

Checklist

Uh oh!

github-actions bot commented Mar 4, 2026

Code security issues found

Uh oh!

sonarqubecloud bot commented Mar 4, 2026

Quality Gate passed

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants