Feature/cdapi-78 by MohammadPatelNHS · Pull Request #36 · NHSDigital/clinical-data-pathology-api

MohammadPatelNHS · 2026-02-19T12:57:18Z

Description

Context

Type of changes

Refactoring (non-breaking change)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would change existing functionality)
Bug fix (non-breaking change which fixes an issue)

Checklist

I have followed the code style of the project
I have added tests to cover my changes
I have updated the documentation accordingly
This PR is a result of pair or mob programming
Exceptions/Exclusions to coding standards (e.g. #noqa or #NOSONAR) are included within this Pull Request.

Sensitive Information Declaration

To ensure the utmost confidentiality and protect your and others privacy, we kindly ask you to NOT including PII (Personal Identifiable Information) / PID (Personal Identifiable Data) or any other sensitive data in this PR (Pull Request) and the codebase changes. We will remove any PR that do contain any sensitive information. We really appreciate your cooperation in this matter.

I confirm that neither PII/PID nor sensitive data are included in this PR and the codebase changes.

github-actions · 2026-02-19T12:59:49Z

✅ Trivy gate: no Critical/High vulnerabilities.

Trivy Filesystem Scan Summary

Filesystem: /tmp/artifact

Severity	Count
CRITICAL	0
HIGH	0
MEDIUM	0
LOW	0
UNKNOWN	0

✅ No vulnerabilities found.

mocks/src/apim_mock/handler.py

github-actions · 2026-03-06T12:22:55Z

Deployment Complete

Preview URL: https://feature-cdapi-78-w-infra.dev.endpoints.pathology-laboratory-reporting.national.nhs.uk — Status
- Smoke Test: ✅ Passed (HTTP 200)
Mock URL: https://feature-cdapi-78-w-infra.m.dev.endpoints.pathology-laboratory-reporting.national.nhs.uk — Status
- Smoke Mock Test: ✅ Passed (HTTP 200)
Proxy URL: https://internal-dev.api.service.nhs.uk/pathology-laboratory-reporting-pr-36
Lambda Function: pr-feature-cdapi-78-w-infra
Mock Lambda Function: mock-feature-cdapi-78-w-infra

nhsd-jack-wainwright

LGTM 👍, just some minor comments

nhsd-jack-wainwright · 2026-03-06T17:33:37Z

infrastructure/images/api-gateway-mock/resources/server.py

@@ -37,9 +43,15 @@
 def forward_request(path_params):
    app.logger.info("received request with data: %s", request.get_data(as_text=True))


Not to do with your changes. But given your updating this file as part of your changes would you mind removing this log line here, as this leads to a sonar issue due to us logging user provided text.

removed this now

nhsd-jack-wainwright · 2026-03-06T17:35:14Z

infrastructure/images/api-gateway-mock/resources/server.py

    app.logger.info("received request with data: %s", request.get_data(as_text=True))

+    if TARGET_CONTAINER == "MOCKS":
+        base_url = "http://mocks:8080"  # NOSONAR python:S5332


I think it would be cleaner here if the URL to forward traffic was provided as an environment variable rather than being dependent on the type of TARGET_CONTAINER.

I've updated this now

nhsd-jack-wainwright · 2026-03-06T17:36:39Z

infrastructure/images/api-gateway-mock/resources/server.py


+    if TARGET_CONTAINER == "MOCKS":
+        base_url = "http://mocks:8080"  # NOSONAR python:S5332
+        content_type = "application/x-www-form-urlencoded"


Should the content type be different here locally for the mocks? I think the request provided to the lambda will always be JSON from API gateway even if the request from the client is different?

I was testing to see if it mattered for how the data is received, Ive just set it to be JSON now

nhsd-jack-wainwright · 2026-03-06T17:37:55Z

mocks/src/apim_mock/__init__.py

@@ -0,0 +1,20 @@
+import logging.config
+
+logging.config.dictConfig(


Rather than using the standard python logging library, does it make more sense to use the AWS lambda powertools logger like we are with the main pathology lambda?

nhsd-jack-wainwright · 2026-03-06T17:39:43Z

mocks/src/apim_mock/auth_check.py

+
+
+def check_authenticated(token: str) -> bool:
+    dynamodb = boto3.resource("dynamodb")


Very minor, but could this dynamodb client be moved out of this method just into the file to allow for the client to be reused across multiple authentication checks?

I was planning a larger change to how we interact with aws resources in the PDM mock, ive left this as is for now but can be changed before merging if we dont want to wait Improvements in the PDM mock

nhsd-jack-wainwright · 2026-03-06T18:06:43Z

mocks/src/apim_mock/test_handler.py

+            ),
+        ],
+    )
+    def test_invalid_payload(self, payload: dict[str, Any], error_message: str) -> None:


I wonder if it would be cleaner if these tests were executed against the public handle_request method? Mainly incase the private methods underneath the handle_request method are updated in the future.

nhsd-jack-wainwright · 2026-03-09T12:07:11Z

Makefile

 	@mkdir -p infrastructure/images/pathology-api/resources/build
 	@cp -r pathology-api/target/pathology-api infrastructure/images/pathology-api/resources/build

+	@mkdir infrastructure/images/mocks/resources/build/


I think this logic should be updated to rely on just the mocks target directory, rather than unzipping the artifact.zip file.

changed this to be the same as the pathology-api lambda now

nhsd-jack-wainwright · 2026-03-09T12:08:42Z

Makefile

+	$(docker) run --platform linux/amd64 --name pathology-api -p 5001:8080 --network $(dockerNetwork) -d localhost/pathology-api-image
+	$(docker) run --platform linux/amd64 --name mocks -p 5003:8080 --network $(dockerNetwork) -d localhost/mocks-image
 	$(docker) run --name api-gateway-mock -p 5002:5000 --network $(dockerNetwork) -d localhost/api-gateway-mock-image
+	$(docker) run --name api-gateway-mock-2 -p 5005:5000 -e TARGET_CONTAINER='MOCKS' --network $(dockerNetwork) -d localhost/api-gateway-mock-image


Maybe this mock should be named something like mocks-api-gateway rather than api-gateway-mock-2? If renaming this container it's probably worth renaming the existing api-gateway-mock container to be something like pathology-api-gateway.

I've renamed these now

nhsd-jack-wainwright · 2026-03-09T12:12:48Z

mocks/src/apim_mock/test_handler.py

+        with pytest.raises(ValueError, match=error_message):
+            _get_jwk_key_from_url_by_kid("TEST-1")
+
+    @pytest.mark.parametrize(


Is it worth including a scenario here where the expiry is too far in the future?

nhsd-jack-wainwright · 2026-03-09T12:15:56Z

mocks/lambda_handler.py

+    _logger.debug("temp: %s", app.current_event.body)
+    _logger.debug("temp2: %s", app.current_event.decoded_body)


Are these log lines required? I think these logs will include the client_assertion which does include a secret value (though the secret is only being used in a secret context here).

I was mostly using it for debugging, so its not really necessary to keep.
I think we could do with someway of logging what the body receives without logging any actual secret values if we need some help debugging in the future

sonarqubecloud · 2026-03-10T19:27:39Z

Quality Gate failed

Failed conditions
0.0% Coverage on New Code (required ≥ 95%)
9.0% Duplication on New Code (required ≤ 3%)
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

MohammadPatelNHS changed the title ~~Feature/cdapi 78 w infrastructure changes~~ Feature/cdapi-78 Feb 19, 2026

MohammadPatelNHS force-pushed the feature/CDAPI-78-w-infra branch 3 times, most recently from 7595562 to a9f2f2f Compare March 2, 2026 13:41

github-advanced-security bot found potential problems Mar 2, 2026

View reviewed changes

mocks/src/apim_mock/handler.py Fixed Show fixed Hide fixed

MohammadPatelNHS force-pushed the feature/CDAPI-78-w-infra branch from a9f2f2f to ae7f964 Compare March 3, 2026 10:20

github-advanced-security bot found potential problems Mar 3, 2026

View reviewed changes

mocks/src/apim_mock/handler.py Fixed Show fixed Hide fixed

MohammadPatelNHS force-pushed the feature/CDAPI-78-w-infra branch 13 times, most recently from 9054c38 to 62ba4bc Compare March 4, 2026 12:17

MohammadPatelNHS marked this pull request as ready for review March 4, 2026 12:25

MohammadPatelNHS requested a review from a team as a code owner March 4, 2026 12:25

MohammadPatelNHS force-pushed the feature/CDAPI-78-w-infra branch 3 times, most recently from 1732712 to 6c8de20 Compare March 6, 2026 12:20

nhsd-jack-wainwright reviewed Mar 9, 2026

View reviewed changes

MohammadPatelNHS and others added 2 commits March 10, 2026 15:05

[CDAPI-78]: created mock for apim

c16d7c9

[CDAPI-78]: Added new virtual environment for use by the mocks

dbbce28

MohammadPatelNHS force-pushed the feature/CDAPI-78-w-infra branch from 6c8de20 to dbbce28 Compare March 10, 2026 15:08

MohammadPatelNHS added 2 commits March 10, 2026 18:03

[CDAPI-78]: made changes according to pr comments

301c3df

[CDAPI-78]: github actions changes after rebase

2d50ef0

		@@ -37,9 +43,15 @@
		def forward_request(path_params):
		app.logger.info("received request with data: %s", request.get_data(as_text=True))

		@@ -0,0 +1,20 @@
		import logging.config

		logging.config.dictConfig(



		def check_authenticated(token: str) -> bool:
		dynamodb = boto3.resource("dynamodb")

		_logger.debug("temp: %s", app.current_event.body)
		_logger.debug("temp2: %s", app.current_event.decoded_body)

Conversation

MohammadPatelNHS commented Feb 19, 2026

Description

Context

Type of changes

Checklist

Sensitive Information Declaration

Uh oh!

github-actions bot commented Feb 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Trivy Filesystem Scan Summary

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Mar 6, 2026

Uh oh!

nhsd-jack-wainwright left a comment

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!

sonarqubecloud bot commented Mar 10, 2026

Quality Gate failed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

github-actions bot commented Feb 19, 2026 •

edited

Loading