[CDAPI-72]: implemented pdm client#99
[CDAPI-72]: implemented pdm client#99MohammadPatelNHS wants to merge 1 commit intofeature/CDAPI-113from
Conversation
MohammadPatelNHS
force-pushed
the
feature/CDAPI-72
branch
3 times, most recently
from
fbcdca3 to
923a897
Compare
nhsd-jack-wainwright
force-pushed
the
feature/CDAPI-113
branch
from
f6b38ee to
6a780bf
Compare
MohammadPatelNHS
force-pushed
the
feature/CDAPI-72
branch
from
7944912 to
1ec6131
Compare
nhsd-jack-wainwright
left a comment
nhsd-jack-wainwright
left a comment
There was a problem hiding this comment.
LGTM, just some minor questions/suggestions
| _create_operation_outcome(400, "Missing X-Request-ID header", "required") | ||
| ) | ||
| if not check_valid_uuid4(x_request_id): | ||
| _logger.error("Invalid X-Request-ID header.") |
There was a problem hiding this comment.
Very minor but might be worth including the provided x_request_id here just to aid debugging?
| def set_correlation_id(value: CorrelationID) -> None: | ||
| """Set the correlation ID for the current request context.""" | ||
| _correlation_id.set(value) | ||
| _correlation_id.set({"full_id": value["full_id"], "short_id": value["short_id"]}) |
There was a problem hiding this comment.
Do we need to create a new CorrelationID instance here or could we just set the _correlation_id value to what's supplied?
| def reset_correlation_id() -> None: | ||
| """Reset the correlation ID to the default empty string.""" | ||
| _correlation_id.set("") | ||
| print("resetting", _correlation_id.get()) |
There was a problem hiding this comment.
Is this print required? If so I think this should be included via a logger.
| correlation_id = _get_correlation_id() | ||
| if correlation_id: | ||
| return correlation_id["short_id"] | ||
| else: |
There was a problem hiding this comment.
As discussed I think an exception should be thrown here rather than defaulting to an empty string.
| os.environ["CLIENT_TIMEOUT"] = "3s" | ||
| os.environ["APIM_TOKEN_URL"] = "apim_url" # noqa S105 - dummy value | ||
| os.environ["APIM_PRIVATE_KEY_NAME"] = "apim_private_key_name" | ||
| os.environ["APIM_API_KEY_NAME"] = "apim_api_key_name" | ||
| os.environ["APIM_TOKEN_EXPIRY_THRESHOLD"] = "1s" # noqa S105 - dummy value | ||
| os.environ["APIM_KEY_ID"] = "apim_key" | ||
| os.environ["PDM_BUNDLE_URL"] = "pdm_bundle_url" |
There was a problem hiding this comment.
As you're mocking the apim_authenticator in this file I don't think these environment variables are required? Instead calls to pathology_api.environment.values can be patched to return the required values in the tests themselves.
| context: LambdaContext, | ||
| ) -> None: | ||
| # First request sets a correlation ID | ||
| uuid_mock.return_value = "test_uuid" |
There was a problem hiding this comment.
Is it worth setting a separate return value here in between the first and second handler calls to verify that a new correlation ID is generated per handler call?
nhsd-jack-wainwright
force-pushed
the
feature/CDAPI-113
branch
from
12e38ea to
a7d3e1c
Compare
MohammadPatelNHS
force-pushed
the
feature/CDAPI-72
branch
from
a4e7e3f to
b7f970e
Compare
nhsd-jack-wainwright
left a comment
nhsd-jack-wainwright
left a comment
There was a problem hiding this comment.
LGTM just a couple additional suggestions.
|
|
||
| @_exception_handler(ValueError) | ||
| def handle_value_error(exception: ValueError) -> Response[str]: | ||
| if "Missing required header: nhsd-correlation-id" in str(exception): |
There was a problem hiding this comment.
Rather than requiring this if statement based on exception message here. Maybe before throwing the ValueError when no correlation ID is provided we could set a correlation ID?
| response_meta = response_bundle.meta | ||
| assert response_meta.last_updated is not None | ||
| assert response_meta.version_id is None | ||
| assert response_meta.version_id == "1" |
There was a problem hiding this comment.
Is it worth adding an assertion around the eTag header returned here also?
MohammadPatelNHS
force-pushed
the
feature/CDAPI-72
branch
2 times, most recently
from
40465e6 to
04519be
Compare
nhsd-jack-wainwright
force-pushed
the
feature/CDAPI-113
branch
from
3e0fcc8 to
e2a30bd
Compare
MohammadPatelNHS
force-pushed
the
feature/CDAPI-72
branch
from
04519be to
03f3c01
Compare
|
|
Deployment Complete
|
2 participants
Description
Context
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.