Feature/gpcapim 251 #46

DWolfsNHS · 2026-01-15T12:10:35Z

Description

This pull request introduces the GpProviderClient class, which provides a simple client for interacting with the GPProvider FHIR GP System. The client includes methods to fetch structured patient records from a GPProvider FHIR API endpoint. Additionally, unit tests and a stub for the GPProvider FHIR API have been implemented to ensure the correctness of the client.

Context

This change is required to enable integration with the GPProvider FHIR GP System, allowing the retrieval of structured patient records. It addresses the need for a robust and reusable client to interact with the GPProvider API, ensuring compliance with the GPConnect specifications.

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.

Vox-Ben

Looks good, comments mostly on non-critical stuff. :-)

Vox-Ben · 2026-01-16T19:53:10Z

gateway-api/src/gateway_api/provider_request.py

+import requests
+from requests import Response


Do you need to do this? You're basically importing from the same file twice (which AI seems to like doing and I'm not sure why). IMO better to just do from requests import Response, <whatever else you're using>.

yeah, this cleans things up a bit, thanks

Vox-Ben · 2026-01-16T19:54:57Z

gateway-api/src/gateway_api/provider_request.py

+from requests import Response
+
+# definitions
+ars_interactionId = "urn:nhs:names:services:gpconnect:structured:fhir:operation:gpc.getstructuredrecord-1"  # noqa: E501 this is standard InteractionID for accessRecordStructured


You can break this over two lines, it doesn't need to bust the line length limit.

ars_interactionId = ("urn:nhs:names:services:gpconnect:structured" ":fhir:operation:gpc.getstructuredrecord-1")

Vox-Ben · 2026-01-16T19:57:24Z

gateway-api/src/gateway_api/provider_request.py

+    This exception wraps :class:`requests.HTTPError` thrown by
+    ``response.raise_for_status()`` and re-raises it as ``ExternalServiceError``
+    to decouple callers from the underlying ``requests`` library exception types.


I don't understand this comment. The exception as written isn't related to requests.HTTPError and doesn't have any of its own logic. I'd just delete this bit?

The comment is not saying it inherits; it re-raises. Obfuscating the actual error to the end user. Line 137 onwards:

except requests.HTTPError as err: raise ExternalServiceError( f"GPProvider FHIR API request failed:{err.response.reason}" ) from err

If we're likely to move this to a common library then I'll just delete it from here and we can agree wording when we do that.

Vox-Ben · 2026-01-16T19:58:23Z

gateway-api/src/gateway_api/provider_request.py

+timeout = None  # None used for quicker dev, adjust as needed
+
+
+class ExternalServiceError(Exception):


I think we could move this into a common library. I've started one in my current ticket anyway.

Agreed - happy to pattern here, or integrate this as we merge/rebase

Vox-Ben · 2026-01-16T20:03:11Z

gateway-api/src/gateway_api/provider_request.py

+        Args:
+            provider_endpoint (str): The FHIR API endpoint for the provider.
+            provider_asid (str): The ASID for the provider.
+            consumer_asid (str): The ASID for the consumer.


I suggest we write docstrings with Sphinx-style argument listing. Then we can auto-generate our documentation (though that said I don't know if the likes of ChatGPT are going to obsolete the likes of Sphinx). Still, it's standard and it gives us the option, and it's easy enough to just ask the AI to do. Don't know if @davidhamill1-nhs and @nhsd-jack-wainwright agree?

As a starter for ten, I'm okay with this.

My preference is to avoid primitives in the parameters, and define ProviderEndpoint, etc. types some other way (rather than saying they can be any string). By doing so, your type hinting is your documentation.

I assume any LLM running over the codebase will correctly pull the type and any validations/restrictions, and thus you can still gain the AI's summary of the code.

But happy to go with the consensus.

Vox-Ben · 2026-01-16T21:19:01Z

gateway-api/src/gateway_api/provider_request.py

+        headers = self._build_headers(trace_id)
+
+        response = requests.post(
+            self.provider_endpoint + ars_fhir_base + "/patient/" + ars_fhir_operation,


I was convinced that Python had a built-in function for constructing URLs from arbitrary components, but it turns out that it doesn't. And you can't use pathlib either because if any component has a leading slash it drops all the previous components. I wonder if it's worth writing one ourselves.

Vox-Ben · 2026-01-16T21:20:11Z

gateway-api/src/gateway_api/test_provider_request.py

+Fixtures:
+    - `stub`: Provides an instance of the `GpProviderStub` for simulating the GPProvider
+    - `mock_request_post`: Patches the `requests.post` method to intercept API calls and
+    route them to the stub provider. Captures request details for verification.


Not sure we really need to list the fixtures in the module docstring. Easy enough to find them by looking through the file, and the header will go out of date because nobody will update it.

Vox-Ben · 2026-01-16T21:20:38Z

gateway-api/src/gateway_api/test_provider_request.py

+
+from gateway_api.provider_request import ExternalServiceError, GpProviderClient
+
+ars_interactionId = "urn:nhs:names:services:gpconnect:structured:fhir:operation:gpc.getstructuredrecord-1"  # noqa: E501 this is standard InteractionID for accessRecordStructured


As above don't need to #noqa this, just break it across two lines.

- Create GPProviderClient class to interact with GPProvider FHIR API - Add initialization method with provider and consumer ASIDs - Implement get_structured_record method to fetch patient records - Update unit tests to verify successful response from GPProviderClient from example.com

- Change return type of get_structured_record method to Response - Update timeout parameter to None for quicker development - Add mock_request_get fixture to unit tests for simulating API responses

- Rename method `get_structured_record` to `access_structured_record` - Change HTTP method from GET to POST for fetching structured patient records - Update request headers to match GP Connect specifications - Enhance mock request handling in tests for POST requests - Add a GPProviderStub class to simulate structured patient record responses

- Correct header casing in GPProviderClient for FHIR API requests - Enhance GPProviderStub to return a Response object - Refactor mock_request_post to utilize GPProviderStub

…ap to https for dummy url

…PI requests - Implement _build_headers method to construct request headers - Update access_structured_record method to utilize new headers - Modify unit tests to verify correct headers are sent in requests

- Modify access_structured_record method to accept a body parameter - Update request handling to include body in POST requests - Add unit test to verify correct body is sent in requests

- Implement test to verify GPProviderClient returns correct response from stub provider - Remove outdated pseudo-code comments from test file - Update stub provider response handling for initial implementation

- Introduce a StubResponse class for simplified response representation - Update access_record_structured method to return StubResponse instead of Response - Enhance test cases to utilize FakeResponse for better testing of HTTP interactions

- Replace FakeResponse with a more generic StubResponse class - Update mock_request_post to return the actual stub response - Adjust content handling in tests to match new response structure

- Update interaction ID and base URL definitions for clarity - Adjust request timeout handling for better development flexibility - Enhance unit tests to verify correct URL formation and response status

- Implement test to verify ExternalServiceError is raised on HTTP error - Simulate a 500 Internal Server Error response from the GPProvider API - Enhance test coverage for GpProviderClient error handling

github-actions · 2026-01-19T15:13:28Z

ALB Target: arn:aws:elasticloadbalancing:eu-west-2:900119715266:targetgroup/gpcapim-251/70f82f0c23a3968d
Preview URL: https://feature-gpcapim-251.dev.endpoints.clinical-data-gateway.national.nhs.uk

davidhamill1-nhs

Happy with functionality. Some minor comments about style

Same comment as I left with @Vox-Ben , which needs a discussion amongst the team: to what extent do we document in code?

davidhamill1-nhs · 2026-01-19T22:17:56Z

gateway-api/src/gateway_api/provider_request.py

+
+        headers = self._build_headers(trace_id)
+
+        endpoint_path = "/".join([ars_fhir_base, fhir_resource, ars_fhir_operation])


Nit: I would say it is easier to parse ...

Suggested change

endpoint_path = "/".join([ars_fhir_base, fhir_resource, ars_fhir_operation])

endpoint_path = f"{ars_fhir_base}/{fhir_resource}/{ars_fhir_operation}"

davidhamill1-nhs · 2026-01-19T22:28:31Z

gateway-api/src/gateway_api/provider_request.py

+ars_interactionId = (
+    "urn:nhs:names:services:gpconnect:structured"
+    ":fhir:operation:gpc.getstructuredrecord-1"
+)
+ars_fhir_base = "FHIR/STU3"
+fhir_resource = "patient"
+ars_fhir_operation = "$gpc.getstructuredrecord"
+timeout = None  # None used for quicker dev, adjust as needed


Given these are constants, I would suggest making them UPPERCASE. It makes it clearer in the code later on that that the timeout is (currently) a constant.

davidhamill1-nhs · 2026-01-19T22:33:06Z

gateway-api/src/gateway_api/test_provider_request.py

+    """
+    provider_asid = "200000001154"
+    consumer_asid = "200000001152"
+    provider_endpoint = "https://invalid.com"


Nit: Given the assigned string, I would assume this to be an invalid url.

Suggested change

provider_endpoint = "https://invalid.com"

provider_endpoint = "https://test.com"

davidhamill1-nhs · 2026-01-20T09:48:42Z

gateway-api/src/gateway_api/test_provider_request.py

+    for key, value in expected_headers.items():
+        assert captured_headers.get(key) == value


Unless you're expecting the expected headers to be a subset of the captured headers, pytest can "intelligently" assert equality of dicts.

Suggested change

for key, value in expected_headers.items():

assert captured_headers.get(key) == value

assert expected_headers == captured_headers

davidhamill1-nhs · 2026-01-20T09:52:42Z

gateway-api/stubs/stubs/stub_provider.py

+from requests.structures import CaseInsensitiveDict
+
+
+@dataclass()


Is there a reason you need this to be a dataclass as well as a Response?

No, this was left-overs so not needed

github-actions · 2026-01-20T11:39:32Z

ALB Target: arn:aws:elasticloadbalancing:eu-west-2:900119715266:targetgroup/gpcapim-251/70f82f0c23a3968d
Preview URL: https://feature-gpcapim-251.dev.endpoints.clinical-data-gateway.national.nhs.uk

github-actions · 2026-01-20T13:18:01Z

ALB Target: arn:aws:elasticloadbalancing:eu-west-2:900119715266:targetgroup/gpcapim-251/70f82f0c23a3968d
Preview URL: https://feature-gpcapim-251.dev.endpoints.clinical-data-gateway.national.nhs.uk

- Introduced GpProviderStub to simulate provider responses - Updated post method to use stub for structured record access - Enhanced StubResponse to set encoding to UTF-8

github-actions · 2026-01-20T13:40:12Z

ALB Target: arn:aws:elasticloadbalancing:eu-west-2:900119715266:targetgroup/gpcapim-251/70f82f0c23a3968d
Preview URL: https://feature-gpcapim-251.dev.endpoints.clinical-data-gateway.national.nhs.uk

sonarqubecloud · 2026-01-20T13:42:16Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

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

See analysis details on SonarQube Cloud

DWolfsNHS force-pushed the feature/GPCAPIM-251 branch from a73f501 to 89fa638 Compare January 16, 2026 15:49

Vox-Ben reviewed Jan 16, 2026

View reviewed changes

DWolfsNHS marked this pull request as ready for review January 19, 2026 11:20

DWolfsNHS requested a review from a team as a code owner January 19, 2026 11:20

DWolfsNHS added 15 commits January 19, 2026 15:11

[GPCAPIM-251] initial commit with basic file structure and descriptions

d6b5fc9

[GPCAPIM-251]: Refactor GPProviderClient and enhance test coverage

296403f

- Change return type of get_structured_record method to Response - Update timeout parameter to None for quicker development - Add mock_request_get fixture to unit tests for simulating API responses

[GPCAPIM-251]: Update GPProviderClient and stub for FHIR API interaction

7908028

- Correct header casing in GPProviderClient for FHIR API requests - Enhance GPProviderStub to return a Response object - Refactor mock_request_post to utilize GPProviderStub

[GPCAPIM-251]: Refactor GPProvider class names for consistency and sw…

f6680ee

…ap to https for dummy url

[GPCAPIM-251]: Enhance GPProviderClient to include headers for FHIR A…

deae9e6

…PI requests - Implement _build_headers method to construct request headers - Update access_structured_record method to utilize new headers - Modify unit tests to verify correct headers are sent in requests

[GPCAPIM-251]: Update GpProviderClient to handle request body

2ea0376

- Modify access_structured_record method to accept a body parameter - Update request handling to include body in POST requests - Add unit test to verify correct body is sent in requests

[GPCAPIM-251]: Add tests for GPProviderClient response handling

74f0db7

- Implement test to verify GPProviderClient returns correct response from stub provider - Remove outdated pseudo-code comments from test file - Update stub provider response handling for initial implementation

[GPCAPIM-251]: Refactor response handling in stub provider and tests

5ecc05c

- Replace FakeResponse with a more generic StubResponse class - Update mock_request_post to return the actual stub response - Adjust content handling in tests to match new response structure

[GPCAPIM-251]: Refactor GPProviderClient to improve request handling

ecb046d

- Update interaction ID and base URL definitions for clarity - Adjust request timeout handling for better development flexibility - Enhance unit tests to verify correct URL formation and response status

[GPCAPIM-251]: cleanup and update documentation

b5448c5

[GPCAPIM-251]: Add error handling test for access_structured_record

7b94e69

- Implement test to verify ExternalServiceError is raised on HTTP error - Simulate a 500 Internal Server Error response from the GPProvider API - Enhance test coverage for GpProviderClient error handling

[GPCAPIM-251]: Address peer feedback

2026888

DWolfsNHS force-pushed the feature/GPCAPIM-251 branch from 2da189a to 2026888 Compare January 19, 2026 15:11

davidhamill1-nhs reviewed Jan 20, 2026

View reviewed changes

[GPCAPIM-251]: PR feedback

a25d463

[GPCAPIM-251]: Update remaining provider endpoint in tests

fc4aa34

DWolfsNHS added 2 commits January 20, 2026 13:36

[GPCAPIM-251]: Fix constant naming for interaction ID

a105270

[GPCAPIM-251]: Implement stub provider for development

e790853

- Introduced GpProviderStub to simulate provider responses - Updated post method to use stub for structured record access - Enhanced StubResponse to set encoding to UTF-8

		timeout = None # None used for quicker dev, adjust as needed


		class ExternalServiceError(Exception):


		from gateway_api.provider_request import ExternalServiceError, GpProviderClient

		ars_interactionId = "urn:nhs:names:services:gpconnect:structured:fhir:operation:gpc.getstructuredrecord-1" # noqa: E501 this is standard InteractionID for accessRecordStructured


		headers = self._build_headers(trace_id)

		endpoint_path = "/".join([ars_fhir_base, fhir_resource, ars_fhir_operation])

	endpoint_path = "/".join([ars_fhir_base, fhir_resource, ars_fhir_operation])
	endpoint_path = f"{ars_fhir_base}/{fhir_resource}/{ars_fhir_operation}"

	provider_endpoint = "https://invalid.com"
	provider_endpoint = "https://test.com"

		for key, value in expected_headers.items():
		assert captured_headers.get(key) == value

	for key, value in expected_headers.items():
	assert captured_headers.get(key) == value
	assert expected_headers == captured_headers

		from requests.structures import CaseInsensitiveDict


		@dataclass()

Feature/gpcapim 251 #46

Are you sure you want to change the base?

Feature/gpcapim 251 #46

Conversation

DWolfsNHS commented Jan 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Context

Type of changes

Checklist

Sensitive Information Declaration

Uh oh!

Vox-Ben 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!

github-actions bot commented Jan 19, 2026

Uh oh!

davidhamill1-nhs 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!

github-actions bot commented Jan 20, 2026

Uh oh!

github-actions bot commented Jan 20, 2026

Uh oh!

github-actions bot commented Jan 20, 2026

Uh oh!

sonarqubecloud bot commented Jan 20, 2026

Quality Gate passed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

DWolfsNHS commented Jan 15, 2026 •

edited

Loading