Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@
)
@app.route("/<path:path_params>", methods=["POST", "GET"])
def forward_request(path_params):
x_correlation_id = request.headers.get("X-Correlation-ID")
x_correlation_id = request.headers.get("X-Correlation-ID", "")
forwarded_headers = {k.lower(): v for k, v in request.headers.items()}
forwarded_headers["nhsd-correlation-id"] = x_correlation_id

Expand Down Expand Up @@ -75,6 +75,11 @@ def forward_request(path_params):
app.logger.info("response: %s", response.text)
response_data = response.json()

# proxygen adds x correlation id to the response headers if one is sent,
# so we can mimic that here, as we currently dont manually return it
if x_correlation_id:
response_data["headers"]["X-Correlation-ID"] = x_correlation_id

output = (
(
response_data["body"],
Expand Down
29 changes: 24 additions & 5 deletions mocks/src/pdm_mock/handler.py
Original file line number Diff line number Diff line change
Expand Up @@ -73,21 +73,40 @@ def _raise_error(status_code: int, error_text: str, error_code: str) -> RequestH
}


def _get_patient_id_from_resource(resource: dict[str, Any]) -> Any:
resource_type = resource.get("resourceType")
if resource_type != "Composition" and resource_type != "Patient":
return None

if resource_type == "Composition":
resource = resource.get("subject", {})

if "identifier" not in resource or len(resource["identifier"]) == 0:
return None

patient = (
resource["identifier"][0].get("value")
if type(resource["identifier"]) is list
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would we ever expect the patient resource's identifier value to not be a list here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what the exact structure of the document we get from middleware will be but I've seen an example where a patient identifier could be supplied in both a patient resource and as part of the subject in the composition resource. when supplied as part of the composition subject, identifier is not a list.

else resource["identifier"].get("value")
)
if not patient:
return None

return patient


def _fetch_patient_from_payload(payload: dict[str, Any]) -> str | None:
patient_values = [
str(patient)
for entry in payload.get("entry", [])
if (resource := entry.get("resource"))
and resource.get("resourceType") == "Patient"
and "identifier" in resource
and len(resource["identifier"]) > 0
and (patient := resource.get("identifier")[0].get("value"))
and (patient := _get_patient_id_from_resource(resource))
]

if not patient_values:
return None

if len(patient_values) > 1:
if len(set(patient_values)) > 1:
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you create a set from patient_values here to account for the same value being provided multiple times?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

adding onto the above point, if a patient identifier is supplied as part of both composition subject and patient resource then its fine if they are the same value, pdm allows for that.
it does still return an error if the nhs numbers supplied in each resource is different.

raise ValueError("Multiple patients referenced within the same bundle")

return str(patient_values[0])
Expand Down
69 changes: 69 additions & 0 deletions mocks/src/pdm_mock/test_handler.py
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,75 @@ def test_handle_post_request_multiple_patients(

assert not boto3_resource_mock.called

@patch("common.storage_helper.StorageHelper.put_item")
@patch("pdm_mock.handler.uuid4")
@patch("pdm_mock.handler.datetime")
@pytest.mark.parametrize(
("payload_entry", "expected_status_code"),
[
pytest.param({"resource": {"resourceType": "Observation"}}, 201),
pytest.param(
{
"resource": {
"resourceType": "Composition",
"subject": {"identifier": {"value": "test_number"}},
}
},
201,
),
pytest.param({"resource": {"resourceType": "Patient"}}, 201),
pytest.param(
{
"resource": {
"resourceType": "Patient",
"identifier": [{"system": "https://fhir.nhs.uk/Id/nhs-number"}],
}
},
201,
),
pytest.param(
{
"resource": {
"resourceType": "Composition",
"subject": {"identifier": {"value": "PDM_VALIDATION_ERROR"}},
}
},
422,
),
pytest.param(
{
"resource": {
"resourceType": "Patient",
"identifier": [{"value": "PDM_SERVER_ERROR"}],
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there some other scenarios here covered by CDAPI-72 that need to be included here? I think the BAD_GATEWAY and GATEWAY_TIMEOUT scenarios should be included for example as well as perhaps the authentication errors.

}
},
500,
),
],
)
def test_magic_patient_id_in_payload(
self,
mock_datetime: MagicMock,
mock_uuid: MagicMock,
mock_storage_helper_put_item: MagicMock,
basic_document_payload: dict[str, Any],
payload_entry: dict[str, Any],
expected_status_code: int,
handler: ModuleType,
) -> None:

mock_datetime.now.return_value = datetime.datetime(
2024, 6, 1, 0, 0, 0, tzinfo=datetime.timezone.utc
)
mock_uuid.return_value = "uuid4"
payload = {**basic_document_payload, "entry": [payload_entry]}

response = handler.handle_post_request(payload)

assert response["status_code"] == expected_status_code
if expected_status_code == 201:
assert mock_storage_helper_put_item.called

@patch("boto3.resource")
@patch("pdm_mock.handler.uuid4")
@patch("pdm_mock.handler.datetime")
Expand Down
2 changes: 1 addition & 1 deletion pathology-api/src/pathology_api/pdm.py
Original file line number Diff line number Diff line change
Expand Up @@ -59,4 +59,4 @@ def post_document(document: Bundle) -> PdmResponse:
# all other responses including 5xx and 4xx return same format for now
else:
pdm_error = response.text
raise PdmException(f"Failed to send document: {pdm_error}")
raise PdmException(f"Failed to store document: {pdm_error}")
4 changes: 2 additions & 2 deletions pathology-api/src/pathology_api/test_pdm.py
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ def test_post_document_4xx(self) -> None:
)

with pytest.raises(
PdmException, match="Failed to send document: error message"
PdmException, match="Failed to store document: error message"
):
post_document(bundle)

Expand All @@ -195,6 +195,6 @@ def test_post_document_5xx(self) -> None:
)

with pytest.raises(
PdmException, match="Failed to send document: error message"
PdmException, match="Failed to store document: error message"
):
post_document(bundle)
124 changes: 113 additions & 11 deletions pathology-api/tests/integration/test_endpoints.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ def test_bundle_returns_200(
response.headers["X-Correlation-ID"]
== "bb038f9a-dc45-49e1-bcfd-3ab3c3de5e16"
)
assert response.headers["etag"] == 'W/"1"'

response_data = response.json()
response_bundle = Bundle.model_validate(response_data, by_alias=True)
Expand All @@ -46,16 +47,14 @@ def test_bundle_returns_200(
assert response_bundle.meta is not None
response_meta = response_bundle.meta

print(f"Response meta: {response_meta}")

assert response_meta.last_updated is not None
assert response_meta.version_id == "1"

assert response.headers["etag"] == 'W/"1"'

def test_no_payload_returns_error(self, client: Client) -> None:
response = client.send_without_payload(
request_method="POST", path="FHIR/R4/Bundle"
request_method="POST",
path="FHIR/R4/Bundle",
headers={"X-Correlation-ID": "bb038f9a-dc45-49e1-bcfd-3ab3c3de5e16"},
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do these test cases now need to include a correlation ID?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was running the integration tests locally, which doesn't supply a base correlation id like it does on remote because its not using proxygen. I could update the the api gateway mock server.py file to include a correlation id always instead

)
assert response.status_code == 400

Expand All @@ -76,7 +75,12 @@ def test_no_payload_returns_error(self, client: Client) -> None:
assert response.status_code == 400

def test_empty_payload_returns_error(self, client: Client) -> None:
response = client.send(data="", request_method="POST", path="FHIR/R4/Bundle")
response = client.send(
data="",
request_method="POST",
path="FHIR/R4/Bundle",
headers={"X-Correlation-ID": "bb038f9a-dc45-49e1-bcfd-3ab3c3de5e16"},
)
assert response.status_code == 400

response_data = response.json()
Expand Down Expand Up @@ -346,7 +350,10 @@ def test_invalid_payload_returns_error(
self, client: Client, payload: dict[str, Any], expected_diagnostic: str
) -> None:
response = client.send(
data=json.dumps(payload), request_method="POST", path="FHIR/R4/Bundle"
data=json.dumps(payload),
request_method="POST",
path="FHIR/R4/Bundle",
headers={"X-Correlation-ID": "bb038f9a-dc45-49e1-bcfd-3ab3c3de5e16"},
)
assert response.status_code == 400

Expand Down Expand Up @@ -548,7 +555,10 @@ def test_invalid_composition_resource(
}

response = client.send(
data=json.dumps(bundle), request_method="POST", path="FHIR/R4/Bundle"
data=json.dumps(bundle),
request_method="POST",
path="FHIR/R4/Bundle",
headers={"X-Correlation-ID": "bb038f9a-dc45-49e1-bcfd-3ab3c3de5e16"},
)
assert response.status_code == 400

Expand Down Expand Up @@ -653,7 +663,10 @@ def test_invalid_service_request_resource(
}

response = client.send(
data=json.dumps(bundle), request_method="POST", path="FHIR/R4/Bundle"
data=json.dumps(bundle),
request_method="POST",
path="FHIR/R4/Bundle",
headers={"X-Correlation-ID": "bb038f9a-dc45-49e1-bcfd-3ab3c3de5e16"},
)
assert response.status_code == 400

Expand Down Expand Up @@ -759,7 +772,10 @@ def test_invalid_practitioner_role_resource(
}

response = client.send(
data=json.dumps(bundle), request_method="POST", path="FHIR/R4/Bundle"
data=json.dumps(bundle),
request_method="POST",
path="FHIR/R4/Bundle",
headers={"X-Correlation-ID": "bb038f9a-dc45-49e1-bcfd-3ab3c3de5e16"},
)
assert response.status_code == 400

Expand Down Expand Up @@ -870,7 +886,10 @@ def test_invalid_organization_resource(
}

response = client.send(
data=json.dumps(bundle), request_method="POST", path="FHIR/R4/Bundle"
data=json.dumps(bundle),
request_method="POST",
path="FHIR/R4/Bundle",
headers={"X-Correlation-ID": "bb038f9a-dc45-49e1-bcfd-3ab3c3de5e16"},
)
assert response.status_code == 400

Expand All @@ -886,6 +905,89 @@ def test_invalid_organization_resource(
],
}

def test_pdm_returns_validation_error(
self, client: Client, build_valid_test_result: Callable[[str, str], Bundle]
) -> None:
bundle = build_valid_test_result("PDM_VALIDATION_ERROR", "ods_code")

response = client.send(
data=bundle.model_dump_json(by_alias=True),
path="FHIR/R4/Bundle",
request_method="POST",
headers={"X-Correlation-ID": "bb038f9a-dc45-49e1-bcfd-3ab3c3de5e16"},
)

assert response.status_code == 500
assert response.headers["Content-Type"] == "application/fhir+json"
assert (
response.headers["X-Correlation-ID"]
== "bb038f9a-dc45-49e1-bcfd-3ab3c3de5e16"
)

response_data = response.json()
operation_outcome = OperationOutcome.model_validate(response_data)

pdm_diagnostic = {
"resourceType": "OperationOutcome",
"issue": [
{
"severity": "error",
"code": "invariant",
"details": {
"text": "Bundle size exceeds maximum allowed size or"
" number of entries."
},
}
],
}
issue: OperationOutcome.Issue = {
"severity": "error",
"code": "invalid",
"diagnostics": f"Failed to store document: {json.dumps(pdm_diagnostic)}",
}

assert operation_outcome.issue == [issue]

def test_pdm_returns_internal_server_error(
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to above, is it worth including some scenarios here for if a BAD_GATEWAY or GATEWAY_TIMEOUT response is returned by PDM?

self, client: Client, build_valid_test_result: Callable[[str, str], Bundle]
) -> None:
bundle = build_valid_test_result("PDM_SERVER_ERROR", "ods_code")

response = client.send(
data=bundle.model_dump_json(by_alias=True),
path="FHIR/R4/Bundle",
request_method="POST",
headers={"X-Correlation-ID": "bb038f9a-dc45-49e1-bcfd-3ab3c3de5e16"},
)

assert response.status_code == 500
assert response.headers["Content-Type"] == "application/fhir+json"
assert (
response.headers["X-Correlation-ID"]
== "bb038f9a-dc45-49e1-bcfd-3ab3c3de5e16"
)

response_data = response.json()
operation_outcome = OperationOutcome.model_validate(response_data)

pdm_diagnostic = {
"resourceType": "OperationOutcome",
"issue": [
{
"severity": "error",
"code": "exception",
"details": {"text": "Internal server error"},
}
],
}
issue: OperationOutcome.Issue = {
"severity": "error",
"code": "invalid",
"diagnostics": f"Failed to store document: {json.dumps(pdm_diagnostic)}",
}

assert operation_outcome.issue == [issue]

@pytest.mark.parametrize(
("subject"),
[
Expand Down
Loading