From 4ac28a35d97172c28c63a164bf37db34cf908b5d Mon Sep 17 00:00:00 2001 From: eesa456 Date: Thu, 22 May 2025 02:35:24 +0100 Subject: [PATCH 1/3] NRL-1205 dont ignore category if type is given for search --- ...test_search_document_reference_consumer.py | 54 +++++++++++++++++++ layer/nrlf/core/dynamodb/repository.py | 43 ++++++--------- 2 files changed, 69 insertions(+), 28 deletions(-) diff --git a/api/consumer/searchDocumentReference/tests/test_search_document_reference_consumer.py b/api/consumer/searchDocumentReference/tests/test_search_document_reference_consumer.py index 9643dad18..7448ffffb 100644 --- a/api/consumer/searchDocumentReference/tests/test_search_document_reference_consumer.py +++ b/api/consumer/searchDocumentReference/tests/test_search_document_reference_consumer.py @@ -247,6 +247,60 @@ 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={ + "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_multiple_categories( diff --git a/layer/nrlf/core/dynamodb/repository.py b/layer/nrlf/core/dynamodb/repository.py index a65931ccb..313dc9885 100644 --- a/layer/nrlf/core/dynamodb/repository.py +++ b/layer/nrlf/core/dynamodb/repository.py @@ -226,6 +226,7 @@ def search( nhs_number=nhs_number, custodian=custodian, pointer_types=pointer_types, + categories=categories, ) key_conditions = ["patient_key = :patient_key"] @@ -233,34 +234,8 @@ def search( 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}" - 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)) - ] - category_filter_values = { - f":category_{i}": categories[i] for i in range(len(categories)) - } - filter_expressions.append(f"({' OR '.join(category_filters)})") - expression_values.update(category_filter_values) - + # Add pointer_types filter if provided + if pointer_types: expression_names["#pointer_type"] = "type" types_filters = [ f"#pointer_type = :type_{i}" for i in range(len(pointer_types)) @@ -271,6 +246,18 @@ def search( 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)) + ] + category_filter_values = { + f":category_{i}": categories[i] for i in range(len(categories)) + } + filter_expressions.append(f"({' OR '.join(category_filters)})") + expression_values.update(category_filter_values) + if custodian: logger.log( LogReference.REPOSITORY016, From cb69b2c7d1759df23229bc4c3e033e52bbf56177 Mon Sep 17 00:00:00 2001 From: eesa456 Date: Fri, 23 May 2025 01:52:52 +0100 Subject: [PATCH 2/3] NRL-1205 add optimisation back and extra test scenarios --- ...test_search_document_reference_consumer.py | 108 ++++++++++++++++++ layer/nrlf/core/dynamodb/repository.py | 25 ++-- .../searchDocumentReference-success.feature | 41 +++++++ 3 files changed, 165 insertions(+), 9 deletions(-) diff --git a/api/consumer/searchDocumentReference/tests/test_search_document_reference_consumer.py b/api/consumer/searchDocumentReference/tests/test_search_document_reference_consumer.py index 7448ffffb..9cb911bf2 100644 --- a/api/consumer/searchDocumentReference/tests/test_search_document_reference_consumer.py +++ b/api/consumer/searchDocumentReference/tests/test_search_document_reference_consumer.py @@ -301,6 +301,114 @@ def test_search_document_reference_happy_path_with_category_and_type( } +@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( diff --git a/layer/nrlf/core/dynamodb/repository.py b/layer/nrlf/core/dynamodb/repository.py index 313dc9885..9f1158fe4 100644 --- a/layer/nrlf/core/dynamodb/repository.py +++ b/layer/nrlf/core/dynamodb/repository.py @@ -236,15 +236,22 @@ def search( # Add pointer_types filter if provided if pointer_types: - expression_names["#pointer_type"] = "type" - types_filters = [ - f"#pointer_type = :type_{i}" for i in range(len(pointer_types)) - ] - types_filter_values = { - f":type_{i}": pointer_types[i] for i in range(len(pointer_types)) - } - filter_expressions.append(f"({' OR '.join(types_filters)})") - expression_values.update(types_filter_values) + 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: + expression_names["#pointer_type"] = "type" + types_filters = [ + f"#pointer_type = :type_{i}" for i in range(len(pointer_types)) + ] + types_filter_values = { + f":type_{i}": pointer_types[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: diff --git a/tests/features/consumer/searchDocumentReference-success.feature b/tests/features/consumer/searchDocumentReference-success.feature index 1fd296127..b72b00a61 100644 --- a/tests/features/consumer/searchDocumentReference-success.feature +++ b/tests/features/consumer/searchDocumentReference-success.feature @@ -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: From aba621417a7b24fc799ce9f8d97a9149525509cd Mon Sep 17 00:00:00 2001 From: Kate Bobyn Date: Tue, 3 Jun 2025 09:03:14 +0100 Subject: [PATCH 3/3] NRL-1205 add negative tests for category search without correct perms --- .../searchDocumentReference-failure.feature | 67 +++++++++++++------ 1 file changed, 48 insertions(+), 19 deletions(-) diff --git a/tests/features/consumer/searchDocumentReference-failure.feature b/tests/features/consumer/searchDocumentReference-failure.feature index 161e12bcd..97855b808 100644 --- a/tests/features/consumer/searchDocumentReference-failure.feature +++ b/tests/features/consumer/searchDocumentReference-failure.feature @@ -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: @@ -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: @@ -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." } """