Skip to content

VED-000 Quick test - will close soon #1286

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
dlzhry2nhs wants to merge 3 commits into from
Closed

VED-000 Quick test - will close soon #1286

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3e6190b
Make dynamo query actually filter
dlzhry2nhs Mar 12, 2026
4e48a42
Is that not pretty weird
dlzhry2nhs Mar 12, 2026
a4445d9
Example using FilterExpression
dlzhry2nhs Mar 12, 2026
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
40 changes: 27 additions & 13 deletions lambdas/backend/src/repository/fhir_repository.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
import boto3
import botocore.exceptions
import simplejson as json
from boto3.dynamodb.conditions import Attr, Key
from boto3.dynamodb.conditions import Attr, ConditionBase, Key
from botocore.config import Config
from fhir.resources.R4B.fhirtypes import Id
from fhir.resources.R4B.identifier import Identifier
Expand Down Expand Up @@ -299,39 +299,43 @@
else:
raise error

def find_immunizations(self, patient_identifier: str, vaccine_types: set) -> list[dict]:
condition = Key("PatientPK").eq(_make_patient_pk(patient_identifier))
is_not_deleted = Attr("DeletedAt").not_exists() | Attr("DeletedAt").eq("reinstated")
def search_immunizations(self, patient_identifier: str, vaccine_types: set) -> list[dict]:
"""Searches for immunisations by patient identifier (NHS Number) and the vaccination type"""
# vacc_type_condition = self._build_vacc_type_key_condition(vaccine_types)

Check warning on line 304 in lambdas/backend/src/repository/fhir_repository.py

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Remove this commented out code. 


See more on https://sonarcloud.io/project/issues?id=NHSDigital_immunisation-fhir-api&issues=AZzi_J_jPxgnpP6n9IAg&open=AZzi_J_jPxgnpP6n9IAg&pullRequest=1286
is_not_deleted_condition = Attr("DeletedAt").not_exists() | Attr("DeletedAt").eq("reinstated")
patient_key_condition = Key("PatientPK").eq(_make_patient_pk(patient_identifier))

ieds_resources = self.get_all_items(condition, is_not_deleted)
ieds_resources = []

if not ieds_resources:
return []
for vacc_type in vaccine_types:
# Should make these DB keys constants
key_condition = patient_key_condition & Key("PatientSK").begins_with(vacc_type)
Copy link
Copy Markdown
Contributor Author

@dlzhry2nhs dlzhry2nhs Mar 12, 2026

Choose a reason for hiding this comment

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

Really interestingly, Key condition (which would be the most efficient way to run the query using the KeyCondition which uses the Partition and sort key). However, when testing (see first commit) I found that DynamoDB does not support PK equals this AND (vacc type key begins with A OR B OR C). Or conditions can only be used in the Filter Expression.

It is not explicitly called out in the docs, but sort of implied when they say that a sort key value and comparison can be provided (but not multiple): https://docs.aws.amazon.com/boto3/latest/reference/services/dynamodb/table/query.html

My approach here might ultimately be worse, because we will have to do a query per vacc type, although the query itself would be so much more efficient than what we currently are doing whereby we are retrieving all vaccinations.

Another option, besides leaving this alone and filtering client side, would be to add all the vacc type OR conditions to the FilterExpression. In truth, it's not that much more efficient as Dynamo will still retrieve all items before applying the filter, but at least it will be done server side. Will leave it up to you guys.

ieds_resources.extend(self.get_all_items(key_condition, is_not_deleted_condition))

# Filter the response to contain only the requested vaccine types
filtered_ieds_resources = [x for x in ieds_resources if self._vaccine_type(x["PatientSK"]) in vaccine_types]
if len(ieds_resources) == 0:
return []

# Return a list of the FHIR immunization resource JSON items
final_resources = [
{
**json.loads(item["Resource"]),
"meta": {"versionId": int(item.get("Version", 1))},
}
for item in filtered_ieds_resources
for item in ieds_resources
]

return final_resources

def get_all_items(self, condition, is_not_deleted):
def get_all_items(self, key_condition: ConditionBase, filter_condition: ConditionBase):
"""Query DynamoDB and paginate through all results."""
all_items = []
last_evaluated_key = None

while True:
query_args = {
"IndexName": "PatientGSI",
"KeyConditionExpression": condition,
"FilterExpression": is_not_deleted,
"KeyConditionExpression": key_condition,
"FilterExpression": filter_condition,
}
if last_evaluated_key:
query_args["ExclusiveStartKey"] = last_evaluated_key
Expand All @@ -349,6 +353,16 @@

return all_items

# @staticmethod

Check warning on line 356 in lambdas/backend/src/repository/fhir_repository.py

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Remove this commented out code. 


See more on https://sonarcloud.io/project/issues?id=NHSDigital_immunisation-fhir-api&issues=AZzi_J_jPxgnpP6n9IAh&open=AZzi_J_jPxgnpP6n9IAh&pullRequest=1286
# def _build_vacc_type_key_condition(vacc_types: set) -> ConditionBase:
# vacc_type_condition = None
#
# for vacc_type in vacc_types:
# key_cond = Key("PatientSK").begins_with(vacc_type)
# vacc_type_condition = key_cond if vacc_type_condition is None else vacc_type_condition | key_cond
#
# return vacc_type_condition

@staticmethod
def _vaccine_type(patient_sk: str) -> str:
parsed = [str.strip(s) for s in patient_sk.split("#")]
Expand Down
2 changes: 1 addition & 1 deletion lambdas/backend/src/service/fhir_service.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,7 +213,7 @@ def search_immunizations(
raise UnauthorizedVaxError()

# Have to retrieve first and then inspect resource to filter by date
all_resources = self.immunization_repo.find_immunizations(nhs_number, permitted_vacc_types)
all_resources = self.immunization_repo.search_immunizations(nhs_number, permitted_vacc_types)
filtered_resources = self._filter_search_results_by_date_and_status(
immunizations=all_resources, date_from=date_from, date_to=date_to, status=Constants.COMPLETED_STATUS
)
Expand Down
26 changes: 15 additions & 11 deletions lambdas/backend/tests/repository/test_fhir_repository.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -596,12 +596,13 @@ def test_find_immunizations_returns_empty_list_when_not_found(self):
self.table.query = MagicMock(return_value=dynamo_response)

# When
result = self.repository.find_immunizations(nhs_number, vaccine_types={"COVID"})
result = self.repository.search_immunizations(nhs_number, vaccine_types={"COVID"})

# Then
self.table.query.assert_called_once_with(
IndexName="PatientGSI",
KeyConditionExpression=Key("PatientPK").eq(_make_patient_pk(nhs_number)),
KeyConditionExpression=Key("PatientPK").eq(_make_patient_pk(nhs_number))
& Key("PatientSK").begins_with("COVID"),
FilterExpression=Attr("DeletedAt").not_exists() | Attr("DeletedAt").eq("reinstated"),
)
self.assertEqual(result, [])
Expand All @@ -627,7 +628,7 @@ def test_find_immunizations_returns_resources_including_meta(self):
self.table.query = MagicMock(return_value=dynamo_response)

# When
results = self.repository.find_immunizations("an-id", {"COVID"})
results = self.repository.search_immunizations("an-id", {"COVID"})

# Then
self.assertListEqual(results, [imms1, imms2])
Expand All @@ -637,32 +638,35 @@ def test_find_immunizations_filters_any_vacc_types_not_in_the_request(self):
imms1 = {"id": 1, "meta": {"versionId": 1}}
imms2 = {"id": 2, "meta": {"versionId": 2}}
imms3 = {"id": 3, "meta": {"versionId": 4}}
items = [
covid_items = [
{
"Resource": json.dumps(imms1),
"PatientSK": "COVID#some_other_text",
"Version": "1",
},
}
]
flu_items = [
{
"Resource": json.dumps(imms2),
"PatientSK": "FLU#some_other_text",
"Version": "2",
},
{
"Resource": json.dumps(imms3),
"PatientSK": "MMR#some_other_text",
"PatientSK": "FLU#some_other_text",
"Version": "4",
},
]

dynamo_response = {"ResponseMetadata": {"HTTPStatusCode": 200}, "Items": items}
self.table.query = MagicMock(return_value=dynamo_response)
covid_dynamo_response = {"ResponseMetadata": {"HTTPStatusCode": 200}, "Items": covid_items}
flu_dynamo_response = {"ResponseMetadata": {"HTTPStatusCode": 200}, "Items": flu_items}
self.table.query = MagicMock(side_effect=[covid_dynamo_response, flu_dynamo_response])

# When
results = self.repository.find_immunizations("an-id", {"COVID", "FLU"})
results = self.repository.search_immunizations("an-id", {"COVID", "FLU"})

# Then
self.assertListEqual(results, [imms1, imms2])
self.assertListEqual(results, [imms1, imms2, imms3])

def test_bad_response_from_dynamo(self):
"""it should throw UnhandledResponse when the response from dynamodb can't be handled"""
Expand All @@ -672,7 +676,7 @@ def test_bad_response_from_dynamo(self):

with self.assertRaises(UnhandledResponseError) as e:
# When
self.repository.find_immunizations("an-id", {"COVID"})
self.repository.search_immunizations("an-id", {"COVID"})

# Then
self.assertDictEqual(e.exception.response, response)
Expand Down
24 changes: 12 additions & 12 deletions lambdas/backend/tests/service/test_fhir_service.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -752,15 +752,15 @@ def test_search_immunizations_returns_results_as_a_search_bundle(self, mock_uuid
mock_resource = create_covid_immunization_dict("1234-some-id")
vaccine_type = "COVID"
self.authoriser.filter_permitted_vacc_types.return_value = {vaccine_type}
self.imms_repo.find_immunizations.return_value = [mock_resource]
self.imms_repo.search_immunizations.return_value = [mock_resource]

# When
result = self.fhir_service.search_immunizations(
VALID_NHS_NUMBER, {vaccine_type}, self.MOCK_SUPPLIER_SYSTEM_NAME, None, None, None
)

# Then
self.imms_repo.find_immunizations.assert_called_once_with(VALID_NHS_NUMBER, {vaccine_type})
self.imms_repo.search_immunizations.assert_called_once_with(VALID_NHS_NUMBER, {vaccine_type})
mock_uuid.assert_called_once()
self.authoriser.filter_permitted_vacc_types.assert_called_once_with(
self.MOCK_SUPPLIER_SYSTEM_NAME, ApiOperationCode.SEARCH, {"COVID"}
Expand Down Expand Up @@ -797,7 +797,7 @@ def test_search_immunizations_filters_by_date_and_status(self, mock_uuid):

vaccine_type = "COVID"
self.authoriser.filter_permitted_vacc_types.return_value = {vaccine_type}
self.imms_repo.find_immunizations.return_value = [
self.imms_repo.search_immunizations.return_value = [
mock_resource_filtered_date_from,
mock_resource,
mock_resource_filtered_date_to,
Expand All @@ -815,7 +815,7 @@ def test_search_immunizations_filters_by_date_and_status(self, mock_uuid):
)

# Then
self.imms_repo.find_immunizations.assert_called_once_with(VALID_NHS_NUMBER, {vaccine_type})
self.imms_repo.search_immunizations.assert_called_once_with(VALID_NHS_NUMBER, {vaccine_type})
mock_uuid.assert_called_once()
self.authoriser.filter_permitted_vacc_types.assert_called_once_with(
self.MOCK_SUPPLIER_SYSTEM_NAME, ApiOperationCode.SEARCH, {"COVID"}
Expand Down Expand Up @@ -848,15 +848,15 @@ def test_search_immunizations_adds_include_to_searched_url(self, mock_uuid):
mock_resource = create_covid_immunization_dict("1234-some-id")
vaccine_type = "COVID"
self.authoriser.filter_permitted_vacc_types.return_value = {vaccine_type}
self.imms_repo.find_immunizations.return_value = [mock_resource]
self.imms_repo.search_immunizations.return_value = [mock_resource]

# When
result = self.fhir_service.search_immunizations(
VALID_NHS_NUMBER, {vaccine_type}, self.MOCK_SUPPLIER_SYSTEM_NAME, None, None, "Patient.identifier"
)

# Then
self.imms_repo.find_immunizations.assert_called_once_with(VALID_NHS_NUMBER, {vaccine_type})
self.imms_repo.search_immunizations.assert_called_once_with(VALID_NHS_NUMBER, {vaccine_type})
mock_uuid.assert_called_once()
self.authoriser.filter_permitted_vacc_types.assert_called_once_with(
self.MOCK_SUPPLIER_SYSTEM_NAME, ApiOperationCode.SEARCH, {"COVID"}
Expand Down Expand Up @@ -884,15 +884,15 @@ def test_search_immunizations_returns_empty_bundle_when_no_results_found(self):
"""it should return an empty search bundle when no results are found"""
vaccine_type = "FLU"
self.authoriser.filter_permitted_vacc_types.return_value = {vaccine_type}
self.imms_repo.find_immunizations.return_value = []
self.imms_repo.search_immunizations.return_value = []

# When
result = self.fhir_service.search_immunizations(
VALID_NHS_NUMBER, {vaccine_type}, self.MOCK_SUPPLIER_SYSTEM_NAME, None, None, None
)

# Then
self.imms_repo.find_immunizations.assert_called_once_with(VALID_NHS_NUMBER, {vaccine_type})
self.imms_repo.search_immunizations.assert_called_once_with(VALID_NHS_NUMBER, {vaccine_type})
self.authoriser.filter_permitted_vacc_types.assert_called_once_with(
self.MOCK_SUPPLIER_SYSTEM_NAME, ApiOperationCode.SEARCH, {vaccine_type}
)
Expand Down Expand Up @@ -920,7 +920,7 @@ def test_search_immunizations_includes_an_error_outcome_within_results_if_client
mock_resource = create_covid_immunization_dict("1234-some-id")
vaccine_type = "COVID"
self.authoriser.filter_permitted_vacc_types.return_value = {vaccine_type}
self.imms_repo.find_immunizations.return_value = [mock_resource]
self.imms_repo.search_immunizations.return_value = [mock_resource]

# When
result = self.fhir_service.search_immunizations(
Expand All @@ -929,7 +929,7 @@ def test_search_immunizations_includes_an_error_outcome_within_results_if_client

# Then
# Does not pass FLU in as client is only permitted to retrieve COVID vaccinations
self.imms_repo.find_immunizations.assert_called_once_with(VALID_NHS_NUMBER, {vaccine_type})
self.imms_repo.search_immunizations.assert_called_once_with(VALID_NHS_NUMBER, {vaccine_type})
mock_uuid.assert_called_once()
self.authoriser.filter_permitted_vacc_types.assert_called_once_with(
self.MOCK_SUPPLIER_SYSTEM_NAME, ApiOperationCode.SEARCH, {"COVID", "FLU"}
Expand Down Expand Up @@ -969,14 +969,14 @@ def test_search_raises_unauthorised_error_if_no_permissions(self):
self.authoriser.filter_permitted_vacc_types.assert_called_once_with(
self.MOCK_SUPPLIER_SYSTEM_NAME, ApiOperationCode.SEARCH, {vaccine_type}
)
self.imms_repo.find_immunizations.assert_not_called()
self.imms_repo.search_immunizations.assert_not_called()

def test_search_immunizations_includes_operation_outcome_when_invalid_immunization_targets_provided(self):
"""it should include an OperationOutcome in the bundle when invalid -immunization.target values were provided"""
mock_resource = create_covid_immunization_dict("1234-some-id")
vaccine_type = "COVID"
self.authoriser.filter_permitted_vacc_types.return_value = {vaccine_type}
self.imms_repo.find_immunizations.return_value = [mock_resource]
self.imms_repo.search_immunizations.return_value = [mock_resource]

result = self.fhir_service.search_immunizations(
VALID_NHS_NUMBER,
Expand Down
4 changes: 2 additions & 2 deletions lambdas/backend/tests/service/test_fhir_service_target_disease.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ def test_make_empty_search_bundle_with_target_disease_not_in_mapping_returns_bun
def test_search_immunizations_with_target_disease_codes_for_url_echoes_target_disease_in_bundle_link(self):
"""it should include target-disease param in bundle self link when target_disease_codes_for_url is set"""
self.authoriser.filter_permitted_vacc_types.return_value = {"MMR"}
self.imms_repo.find_immunizations.return_value = []
self.imms_repo.search_immunizations.return_value = []

result = self.fhir_service.search_immunizations(
"9000000009",
Expand All @@ -70,7 +70,7 @@ def test_search_immunizations_with_target_disease_codes_for_url_echoes_target_di
def test_search_immunizations_with_invalid_target_diseases_adds_operation_outcomes(self):
"""it should add one OperationOutcome entry per invalid target disease diagnostic"""
self.authoriser.filter_permitted_vacc_types.return_value = {"MMR"}
self.imms_repo.find_immunizations.return_value = []
self.imms_repo.search_immunizations.return_value = []

result = self.fhir_service.search_immunizations(
"9000000009",
Expand Down
Loading