Skip to content

NRL-1205 dont ignore category if type is given for search #908

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

Merged
mattdean3-nhs merged 4 commits into from
Jun 6, 2025
Merged

NRL-1205 dont ignore category if type is given for search #908

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4ac28a3
NRL-1205 dont ignore category if type is given for search
eesa456 May 22, 2025
cb69b2c
NRL-1205 add optimisation back and extra test scenarios
eesa456 May 23, 2025
aba6214
NRL-1205 add negative tests for category search without correct perms
katebobyn-nhs Jun 3, 2025
717b2c3
Merge branch 'develop' into feature/eema1-NRL-1205-categoryIsntIgnore…
katebobyn-nhs Jun 6, 2025
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
162 changes: 162 additions & 0 deletions api/consumer/searchDocumentReference/tests/test_search_document_reference_consumer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,168 @@ def test_search_document_reference_happy_path_with_category(
}


@mock_aws
@mock_repository
def test_search_document_reference_happy_path_with_category_and_type(
repository: DocumentPointerRepository,
):
doc_ref = load_document_reference("Y05868-736253002-Valid")
doc_pointer = DocumentPointer.from_document_reference(doc_ref)
repository.create(doc_pointer)

# Second pointer different category
doc_ref2 = load_document_reference("Y05868-736253002-Valid")
doc_ref2.id = "Y05868-736253002-Valid2"
doc_ref2.type.coding[0].code = PointerTypes.NEWS2_CHART.coding_value()
doc_ref2.type.coding[0].display = TYPE_ATTRIBUTES.get(
PointerTypes.NEWS2_CHART.value
).get("display")
doc_ref2.category[0].coding[0].code = Categories.OBSERVATIONS.coding_value()
doc_ref2.category[0].coding[0].display = CATEGORY_ATTRIBUTES.get(
Categories.OBSERVATIONS.value
).get("display")
repository.create(DocumentPointer.from_document_reference(doc_ref2))

event = create_test_api_gateway_event(
headers=create_headers(),
query_string_parameters={
Copy link
Contributor

@katebobyn-nhs katebobyn-nhs May 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

If I'm reading this right, the test is

  1. Creating a pointer of type: MHCP, cat: Care Plan
  2. Creating a pointer of type: NEWS2, cat: Obs
  3. Searching for pointers of type: MHCP, cat: Care Plan
    It then expects the one result (pointer 1) since pointer 2 would be filtered out by both criteria.

I think to prove both filters are being applied there needs to be a test scenario where, e.g., you search for type: MHCP, cat: Obs
Then the expected result is an empty searchset bundle because 1 is filtered out on the basis of failure to match category, and 2 is filtered out on the basis of failure to match type.

Copy link
Contributor Author

@eesa456 eesa456 May 23, 2025

Choose a reason for hiding this comment

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

Yeah I'll add that I'll also add searching with multiple categories and the one type to show that the optimisation for the single type is still used and that it wont get other categories even if they're provided because it isnt for that type

"subject:identifier": "https://fhir.nhs.uk/Id/nhs-number|6700028191",
"category": "http://snomed.info/sct|734163000",
"type": "http://snomed.info/sct|736253002",
},
)

result = handler(event, create_mock_context())
body = result.pop("body")

assert result == {
"statusCode": "200",
"headers": default_response_headers(),
"isBase64Encoded": False,
}
parsed_body = json.loads(body)
assert parsed_body == {
"resourceType": "Bundle",
"type": "searchset",
"link": [
{
"relation": "self",
"url": "https://pytest.api.service.nhs.uk/record-locator/consumer/FHIR/R4/DocumentReference?subject:identifier=https://fhir.nhs.uk/Id/nhs-number|6700028191&type=http://snomed.info/sct|736253002&category=http://snomed.info/sct|734163000",
}
],
"total": 1,
"entry": [{"resource": doc_ref.model_dump(exclude_none=True)}],
}


@mock_aws
@mock_repository
def test_search_document_reference_happy_path_with_category_and_type_no_results(
repository: DocumentPointerRepository,
):
doc_ref = load_document_reference("Y05868-736253002-Valid")
doc_pointer = DocumentPointer.from_document_reference(doc_ref)
repository.create(doc_pointer)

# Second pointer different category
doc_ref2 = load_document_reference("Y05868-736253002-Valid")
doc_ref2.id = "Y05868-736253002-Valid2"
doc_ref2.type.coding[0].code = PointerTypes.NEWS2_CHART.coding_value()
doc_ref2.type.coding[0].display = TYPE_ATTRIBUTES.get(
PointerTypes.NEWS2_CHART.value
).get("display")
doc_ref2.category[0].coding[0].code = Categories.OBSERVATIONS.coding_value()
doc_ref2.category[0].coding[0].display = CATEGORY_ATTRIBUTES.get(
Categories.OBSERVATIONS.value
).get("display")
repository.create(DocumentPointer.from_document_reference(doc_ref2))

event = create_test_api_gateway_event(
headers=create_headers(),
query_string_parameters={
"subject:identifier": "https://fhir.nhs.uk/Id/nhs-number|6700028191",
"category": "http://snomed.info/sct|1102421000000108",
"type": "http://snomed.info/sct|736253002",
},
)

result = handler(event, create_mock_context())
body = result.pop("body")

assert result == {
"statusCode": "200",
"headers": default_response_headers(),
"isBase64Encoded": False,
}
parsed_body = json.loads(body)
assert parsed_body == {
"resourceType": "Bundle",
"type": "searchset",
"link": [
{
"relation": "self",
"url": "https://pytest.api.service.nhs.uk/record-locator/consumer/FHIR/R4/DocumentReference?subject:identifier=https://fhir.nhs.uk/Id/nhs-number|6700028191&type=http://snomed.info/sct|736253002&category=http://snomed.info/sct|1102421000000108",
}
],
"total": 0,
"entry": [],
}


@mock_aws
@mock_repository
def test_search_document_reference_happy_path_with_multiple_categories_and_type(
repository: DocumentPointerRepository,
):
doc_ref = load_document_reference("Y05868-736253002-Valid")
doc_pointer = DocumentPointer.from_document_reference(doc_ref)
repository.create(doc_pointer)

# Second pointer different category
doc_ref2 = load_document_reference("Y05868-736253002-Valid")
doc_ref2.id = "Y05868-736253002-Valid2"
doc_ref2.type.coding[0].code = PointerTypes.NEWS2_CHART.coding_value()
doc_ref2.type.coding[0].display = TYPE_ATTRIBUTES.get(
PointerTypes.NEWS2_CHART.value
).get("display")
doc_ref2.category[0].coding[0].code = Categories.OBSERVATIONS.coding_value()
doc_ref2.category[0].coding[0].display = CATEGORY_ATTRIBUTES.get(
Categories.OBSERVATIONS.value
).get("display")
repository.create(DocumentPointer.from_document_reference(doc_ref2))

event = create_test_api_gateway_event(
headers=create_headers(),
query_string_parameters={
"subject:identifier": "https://fhir.nhs.uk/Id/nhs-number|6700028191",
"category": "http://snomed.info/sct|734163000,http://snomed.info/sct|1102421000000108",
"type": "http://snomed.info/sct|736253002",
},
)

result = handler(event, create_mock_context())
body = result.pop("body")

assert result == {
"statusCode": "200",
"headers": default_response_headers(),
"isBase64Encoded": False,
}
parsed_body = json.loads(body)
assert parsed_body == {
"resourceType": "Bundle",
"type": "searchset",
"link": [
{
"relation": "self",
"url": "https://pytest.api.service.nhs.uk/record-locator/consumer/FHIR/R4/DocumentReference?subject:identifier=https://fhir.nhs.uk/Id/nhs-number|6700028191&type=http://snomed.info/sct|736253002&category=http://snomed.info/sct|734163000,http://snomed.info/sct|1102421000000108",
}
],
"total": 1,
"entry": [{"resource": doc_ref.model_dump(exclude_none=True)}],
}


@mock_aws
@mock_repository
def test_search_document_reference_happy_path_with_multiple_categories(
Expand Down
56 changes: 25 additions & 31 deletions layer/nrlf/core/dynamodb/repository.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,50 +226,44 @@ def search(
nhs_number=nhs_number,
custodian=custodian,
pointer_types=pointer_types,
categories=categories,
)

key_conditions = ["patient_key = :patient_key"]
filter_expressions = []
expression_names = {}
expression_values = {":patient_key": f"P#{nhs_number}"}

if len(pointer_types) == 1:
# Optimisation for single pointer type
category_id, type_id = _get_sk_ids_for_type(pointer_types[0])
patient_sort = f"C#{category_id}#T#{type_id}"
key_conditions.append("begins_with(patient_sort, :patient_sort)")
expression_values[":patient_sort"] = patient_sort
else:
# Handle single/multiple categories and pointer types with filter expressions
if len(categories) == 1:
split_category = categories[0].split("|")
category_id = (
SYSTEM_SHORT_IDS[split_category[0]] + "-" + split_category[1]
)
patient_sort = f"C#{category_id}"
# Add pointer_types filter if provided
if pointer_types:
if len(pointer_types) == 1:
# Optimisation for single pointer type
category_id, type_id = _get_sk_ids_for_type(pointer_types[0])
patient_sort = f"C#{category_id}#T#{type_id}"
key_conditions.append("begins_with(patient_sort, :patient_sort)")
expression_values[":patient_sort"] = patient_sort

if len(categories) > 1:
expression_names["#category"] = "category"
category_filters = [
f"#category = :category_{i}" for i in range(len(categories))
else:
expression_names["#pointer_type"] = "type"
types_filters = [
f"#pointer_type = :type_{i}" for i in range(len(pointer_types))
]
category_filter_values = {
f":category_{i}": categories[i] for i in range(len(categories))
types_filter_values = {
f":type_{i}": pointer_types[i] for i in range(len(pointer_types))
}
filter_expressions.append(f"({' OR '.join(category_filters)})")
expression_values.update(category_filter_values)

expression_names["#pointer_type"] = "type"
types_filters = [
f"#pointer_type = :type_{i}" for i in range(len(pointer_types))
filter_expressions.append(f"({' OR '.join(types_filters)})")
expression_values.update(types_filter_values)

# Add categories filter if provided
if categories:
expression_names["#category"] = "category"
category_filters = [
f"#category = :category_{i}" for i in range(len(categories))
]
types_filter_values = {
f":type_{i}": pointer_types[i] for i in range(len(pointer_types))
category_filter_values = {
f":category_{i}": categories[i] for i in range(len(categories))
}
filter_expressions.append(f"({' OR '.join(types_filters)})")
expression_values.update(types_filter_values)
filter_expressions.append(f"({' OR '.join(category_filters)})")
expression_values.update(category_filter_values)

if custodian:
logger.log(
Expand Down
67 changes: 48 additions & 19 deletions tests/features/consumer/searchDocumentReference-failure.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,23 +162,14 @@ Feature: Consumer - searchDocumentReference - Failure Scenarios
}
"""

Scenario: Search gives 403 if no permission
Scenario: Search rejects request if the organisation has no registered pointer types but uses category filter
Given the application 'DataShare' (ID 'z00z-y11y-x22x') is registered to access the API
And a DocumentReference resource exists with values:
| property | value |
| id | 8FW23-1114567890-SearchDocRefTest |
| subject | 9278693472 |
| status | current |
| type | 736253002 |
| category | 734163000 |
| contentType | application/pdf |
| url | https://example.org/my-doc.pdf |
| custodian | 8FW23 |
| author | 8FW23 |
When consumer 'Z26' searches for DocumentReferences with parameters:
| parameter | value |
| subject | 9278693472 |
| type | 736253002 |
And the organisation 'RX898' is authorised to access pointer types:
| system | value |
When consumer 'RX898' searches for DocumentReferences with parameters:
| parameter | value |
| subject | 9278693472 |
| category | http://snomed.info/sct\|734163000 |
Then the response status code is 403
And the response is an OperationOutcome with 1 issue
And the OperationOutcome contains the issue:
Expand All @@ -193,15 +184,53 @@ Feature: Consumer - searchDocumentReference - Failure Scenarios
"display": "Access has been denied to process this request"
}]
},
"diagnostics": "Your organisation 'Z26' does not have permission to access this resource. Contact the onboarding team."
"diagnostics": "Your organisation 'RX898' does not have permission to access this resource. Contact the onboarding team."
}
"""

Scenario: Search rejects request if the organisation has no registered pointer types
Scenario: Search returns no results if category filter is used without any relevant type permissions
Given the application 'DataShare' (ID 'z00z-y11y-x22x') is registered to access the API
And the organisation 'RX898' is authorised to access pointer types:
| system | value |
| http://snomed.info/sct | 736253002 |
And a DocumentReference resource exists with values:
| property | value |
| id | 8FW23-537854543-SearchDocRefTest |
| subject | 9278693472 |
| status | current |
| type | 1363501000000100 |
| category | 1102421000000108 |
| contentType | application/pdf |
| url | https://example.org/my-doc.pdf |
| custodian | 8FW23 |
| author | 8FW23 |
When consumer 'RX898' searches for DocumentReferences with parameters:
| parameter | value |
| subject | 9278693472 |
| category | http://snomed.info/sct\|1102421000000108 |
Then the response status code is 200
And the response is a searchset Bundle
And the Bundle has a self link matching 'DocumentReference?subject:identifier=https://fhir.nhs.uk/Id/nhs-number|9278693472&category=http://snomed.info/sct|1102421000000108'
And the Bundle has a total of 0
And the Bundle has 0 entries

Scenario: Search gives 403 if no permission
Given the application 'DataShare' (ID 'z00z-y11y-x22x') is registered to access the API
And a DocumentReference resource exists with values:
| property | value |
| id | 8FW23-1114567890-SearchDocRefTest |
| subject | 9278693472 |
| status | current |
| type | 736253002 |
| category | 734163000 |
| contentType | application/pdf |
| url | https://example.org/my-doc.pdf |
| custodian | 8FW23 |
| author | 8FW23 |
When consumer 'Z26' searches for DocumentReferences with parameters:
| parameter | value |
| subject | 9278693472 |
| type | 736253002 |
Then the response status code is 403
And the response is an OperationOutcome with 1 issue
And the OperationOutcome contains the issue:
Expand All @@ -216,7 +245,7 @@ Feature: Consumer - searchDocumentReference - Failure Scenarios
"display": "Access has been denied to process this request"
}]
},
"diagnostics": "Your organisation 'RX898' does not have permission to access this resource. Contact the onboarding team."
"diagnostics": "Your organisation 'Z26' does not have permission to access this resource. Contact the onboarding team."
}
"""

Expand Down
41 changes: 41 additions & 0 deletions tests/features/consumer/searchDocumentReference-success.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -381,6 +381,47 @@ Feature: Consumer - searchDocumentReference - Success Scenarios
| author | 02V |
And the Bundle does not contain a DocumentReference with ID '02V-1111111111-SearchMultipleRefTest3'

Scenario: Search for DocumentReference by NHS number and Category and type
Given the application 'DataShare' (ID 'z00z-y11y-x22x') is registered to access the API
And the organisation 'RX898' is authorised to access pointer types:
| system | value |
| http://snomed.info/sct | 736253002 |
| http://snomed.info/sct | 1363501000000100 |
And a DocumentReference resource exists with values:
| property | value |
| id | 02V-1111111111-SearchMultipleRefTest1 |
| subject | 9278693472 |
| status | current |
| type | 736253002 |
| category | 734163000 |
| contentType | application/pdf |
| url | https://example.org/my-doc-1.pdf |
| custodian | 02V |
| author | 02V |
And a DocumentReference resource exists with values:
| property | value |
| id | 02V-1111111111-SearchMultipleRefTest3 |
| subject | 9278693472 |
| status | current |
| type | 1363501000000100 |
| category | 1102421000000108 |
| contentType | application/pdf |
| url | https://example.org/my-doc-3.pdf |
| custodian | 02V |
| author | 02V |
When consumer 'RX898' searches for DocumentReferences with parameters:
| parameter | value |
| subject | 9278693472 |
| category | http://snomed.info/sct\|1102421000000108 |
| type | http://snomed.info/sct\|736253002 |
Then the response status code is 200
And the response is a searchset Bundle
And the Bundle has a self link matching 'DocumentReference?subject:identifier=https://fhir.nhs.uk/Id/nhs-number|9278693472&type=http://snomed.info/sct|736253002&category=http://snomed.info/sct|1102421000000108'
And the Bundle has a total of 0
And the Bundle has 0 entries
And the Bundle does not contain a DocumentReference with ID '02V-1111111111-SearchMultipleRefTest1'
And the Bundle does not contain a DocumentReference with ID '02V-1111111111-SearchMultipleRefTest3'

Scenario: Search for multiple DocumentReferences by NHS number and Multiple Categories
Given the application 'DataShare' (ID 'z00z-y11y-x22x') is registered to access the API
And the organisation 'RX898' is authorised to access pointer types:
Expand Down
Loading