[GPCAPIM-275]: Harmonise the Controller, Flask entry point and Provider System modules#75
[GPCAPIM-275]: Harmonise the Controller, Flask entry point and Provider System modules#75davidhamill1-nhs wants to merge 42 commits intomainfrom
Conversation
|
✅ Trivy gate: no Critical/High vulnerabilities. Trivy Image Scan SummaryImage: 900119715266.dkr.ecr.eu-west-2.amazonaws.com/whoami:feature-gpcapim-275-harmonise
Findings (top 50)
|
|
Some general suggestions to consider on this PR:
|
davidhamill1-nhs
changed the title
…ns will/should bubble up under the same Error class
…ns will/should bubble up under the same Error class
…t need to handle NHS Session URID
… test, TestGetStructuredRecord.test_happy_path*
|
✅ Trivy gate: no Critical/High issues. Trivy IaC (Terraform) Summary
Findings (top 50)
|
| """ | ||
| # Placeholder implementation | ||
| return "PLACEHOLDER_AUTH_TOKEN" | ||
| return "AUTH_TOKEN123" |
There was a problem hiding this comment.
This is accepted by the PDS sandbox.
…t preventing committing.
…e NoPatientFound error. This will be raised by a raise_from_status call following a 404 from PDS.
…t coverage doesn't capture that.
|
|
Deployment Complete
|
Vox-Ben
left a comment
Vox-Ben
left a comment
There was a problem hiding this comment.
Looks good, nothing major. :-)
| { | ||
| // Use IntelliSense to learn about possible attributes. | ||
| // Hover to view descriptions of existing attributes. | ||
| // For more information, visit: https://go.microsoft.com/fwlink/?linkid=830387 | ||
| "version": "0.2.0", | ||
| "configurations": [ | ||
|
|
||
| { | ||
| "name": "Python Debugger: Flask", | ||
| "type": "debugpy", | ||
| "request": "launch", | ||
| "program": "${workspaceFolder}/infrastructure/images/gateway-api/resources/build/gateway-api/gateway_api/app.py", | ||
| "envFile": "${workspaceFolder}/.env", | ||
| "jinja": true, | ||
| "console": "integratedTerminal" | ||
| } | ||
| ] | ||
| } |
There was a problem hiding this comment.
Does it make sense to add this? As I understand it we're about to be asked to drop Flask.
There was a problem hiding this comment.
This is quite a generic configuration (the name is arbitrary - but will change to make it a more specific to our project/this command); a move away from Flask will not affect this.
|
|
||
| system: str | ||
| value: str | ||
| period: Period |
There was a problem hiding this comment.
Should Period be NotRequired if it's optional?
There was a problem hiding this comment.
AFAIK, period is not optional; however, period.end is for generalPractitioner (and most fields).
| """FHIR Period type.""" | ||
|
|
||
| start: str | ||
| end: NotRequired[str] |
There was a problem hiding this comment.
Are we better to have end be required, but if there isn't an end date then be None? I don't know, this way is conceptually neater and makes type handling simpler, but the other means you can just assume that a Period has an end attribute.
Don't mind, whichever you prefer, just a thought. :-)
There was a problem hiding this comment.
For FHIR, empty objects are not sent. So if an element has no period.end then the Period object will only contain "start". Making NotRequired[str] a more suitable type.
I suspect we will move away from these TypedDicts as pathology have written their own FHIR library using Pydantic which will handle request/response bodies better.
I have a TODO to write up a ticket to make this move.
|
|
||
|
|
||
| @dataclass | ||
| class BaseError(Exception): |
There was a problem hiding this comment.
Is it worth calling this something more specific? CdgError, BaseCdgError, something? BaseError is rather close to BaseException, which obviously you aren't supposed to use directly, and I wouldn't want anyone to mix them up down the line.
| severity: str = "error" | ||
| error_code: ErrorCode = ErrorCode.EXCEPTION | ||
|
|
||
| def __init__(self, **additional_details: str): |
There was a problem hiding this comment.
Just for convention should **additional_details be called **kwargs? I think they're the same thing, you're just collecting any keyword arguments passed to the exception?
There was a problem hiding this comment.
Should the type of these additional details be slightly different here also, would a dict[str,str] make more sense here if we're expecting an arbitrary set of key, value pairs?
| else: | ||
| entries = cast("list[BundleEntry]", body.get("entry", [])) | ||
| if not entries: | ||
| raise RuntimeError("PDS response contains no patient entries") |
There was a problem hiding this comment.
Should this now be a BaseError subclass?
|
|
||
| nhs_number = str(patient.get("id", "")).strip() | ||
| if not nhs_number: | ||
| raise RuntimeError("PDS patient resource missing NHS number") |
There was a problem hiding this comment.
Should also be a BaseError subclass
| """ | ||
| # Placeholder implementation | ||
| return "PLACEHOLDER_AUTH_TOKEN" | ||
| return "AUTH_TOKEN123" |
| class Bundles: | ||
| @staticmethod | ||
| def _wrap_patient_in_bundle(patient: dict[str, Any]) -> dict[str, Any]: | ||
| return { | ||
| "resourceType": "Bundle", | ||
| "type": "collection", | ||
| "meta": { | ||
| "profile": [ | ||
| "https://fhir.nhs.uk/STU3/StructureDefinition/GPConnect-StructuredRecord-Bundle-1" | ||
| ] | ||
| }, | ||
| "entry": [{"resource": patient}], | ||
| } | ||
|
|
||
| ALICE_JONES_9999999999 = _wrap_patient_in_bundle(Patients.ALICE_JONES_9999999999) |
There was a problem hiding this comment.
Should this be in its own file rather than in __init.py__? It does make a certain amount of sense (I'm assuming that the Bundles class itself isn't ever really used elsewhere) but I always think code hidden in __init__.py is hard to find, because you don't necessarily think to look there.
| class Patients: | ||
| @staticmethod | ||
| def load_patient(filename: str) -> dict[str, Any]: | ||
| with open(_path_to_here() / filename, encoding="utf-8") as f: | ||
| patient: dict[str, Any] = json.load(f) | ||
| return patient | ||
|
|
||
| JANE_SMITH_9000000009 = load_patient("jane_smith_9000000009.json") | ||
| NO_SDS_RESULT_9000000010 = load_patient("no_sds_result_9000000010.json") | ||
| BLANK_ASID_SDS_RESULT_9000000011 = load_patient( | ||
| "blank_asid_sds_result_9000000011.json" | ||
| ) | ||
| INDUCE_PROVIDER_ERROR_9000000012 = load_patient( | ||
| "induce_provider_error_9000000012.json" | ||
| ) | ||
| BLANK_ENDPOINT_SDS_RESULT_9000000013 = load_patient( | ||
| "blank_endpoint_sds_result_9000000013.json" | ||
| ) | ||
| ALICE_JONES_9999999999 = load_patient("alice_jones_9999999999.json") |
There was a problem hiding this comment.
Similar comment about whether this is best here or in its own file.
nhsd-jack-wainwright
left a comment
nhsd-jack-wainwright
left a comment
There was a problem hiding this comment.
Have had quite a high level look through these changes now, mostly focusing on the main code rather than the changes to the stubs/tests. Happy to talk through any of the suggestions I've included as a lot of them are more around style rather than requiring specific changes 🙂 .
| "sonarsource.sonarlint-vscode", | ||
| "alexkrechik.cucumberautocomplete" | ||
| "alexkrechik.cucumberautocomplete", | ||
| "streetsidesoftware.code-spell-checker" |
There was a problem hiding this comment.
We're already using vale to provide some spell checking within the repository. Does this extension integrate with this?
| _message = "Internal Server Error" | ||
| status_code: int = INTERNAL_SERVER_ERROR | ||
| severity: str = "error" | ||
| error_code: ErrorCode = ErrorCode.EXCEPTION |
There was a problem hiding this comment.
A minor style point but I'm not sure I like these values being declared by default by the BaseError here. Would it be nicer if these were instead defined as constructor arguments to force subclasses to provide them?
| severity: str = "error" | ||
| error_code: ErrorCode = ErrorCode.EXCEPTION | ||
|
|
||
| def __init__(self, **additional_details: str): |
There was a problem hiding this comment.
Should the type of these additional details be slightly different here also, would a dict[str,str] make more sense here if we're expecting an arbitrary set of key, value pairs?
| return self.message | ||
|
|
||
|
|
||
| class InvalidRequestJSON(BaseError): |
There was a problem hiding this comment.
A very minor style point, but given these classes are exceptions is it worth including Error in the name somewhere? Perhaps this class could be named InvalidRequestError for example?
There was a problem hiding this comment.
I'm also not sure if Python makes a distinction between Exceptions and Errors like some languages do (like Java for example). Are these classes actually more like exceptions, or are errors and exceptions interchangeable in Python?
|
|
||
| class GetStructuredRecordRequest: | ||
| INTERACTION_ID: str = "urn:nhs:names:services:gpconnect:gpc.getstructuredrecord-1" | ||
| INTERACTION_ID: str = ACCESS_RECORD_STRUCTURED_INTERACTION_ID |
There was a problem hiding this comment.
From there name and value I think these are static constants that are associated more to the class than a particular instance. If so maybe it's worth annotating the type with ClassVar?
| INTERACTION_ID: str = ACCESS_RECORD_STRUCTURED_INTERACTION_ID | |
| from typing import ClassVar | |
| .... | |
| INTERACTION_ID: ClassVar[str] = ACCESS_RECORD_STRUCTURED_INTERACTION_ID |
| from stubs.pds.stub import PdsFhirApiStub | ||
|
|
||
| pds = PdsFhirApiStub() | ||
| post = pds.post # type: ignore |
There was a problem hiding this comment.
Do we need to ignore typing here? Could we instead define post as a Callable that both requests.post and pds.post comply with?
| post = pds.post # type: ignore | |
| post: Callable[[...], requests.Response] = requests.post |
| """ | ||
|
|
||
| # URLs for different PDS environments. Requires authentication to use live. | ||
| SANDBOX_URL = "https://sandbox.api.service.nhs.uk/personal-demographics/FHIR/R4" |
There was a problem hiding this comment.
Rather than being included as constants here, I wonder if the URL should be provided as an environment variable that's supplied when starting the container?
| request_id: str | None = None, | ||
| correlation_id: str | None = None, | ||
| timeout: int | None = None, |
There was a problem hiding this comment.
Should these values ever be None?
|
|
||
| # This normally calls requests.get, but if STUB_PDS is set it uses the stub. | ||
| response = post( | ||
| url, # TODO: URL points to sandbox env even when STUB_PDS |
There was a problem hiding this comment.
Is this TODO still valid? I think this method should always post to url, it's just that the base_url may point to a different environment?
| get = sds.get # type: ignore | ||
|
|
||
| # Recursive JSON-like structure typing used for parsed FHIR bodies. | ||
| type ResultStructure = str | dict[str, "ResultStructure"] | list["ResultStructure"] |
There was a problem hiding this comment.
Very minor but there is a bit of duplication here I think, could this union utilise ResultStructureDict?
| type ResultStructure = str | dict[str, "ResultStructure"] | list["ResultStructure"] | |
| type ResultStructureDict = dict[str, "ResultStructure"] | |
| type ResultStructure = str | ResultStructureDict | list["ResultStructure"] |
4 participants
Description
Context
As three pieces of work were complete in parallel by three different developers, there were differences in approaches and code styles that mean some harmonisation is required.
Type of changes
Checklist
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.