From 8de79045e5c152f9264e7707ef9f816b8ac54a9a Mon Sep 17 00:00:00 2001 From: Alyssa Dai Date: Mon, 8 Jan 2024 22:33:39 -0500 Subject: [PATCH 01/21] add comment explaining FEDERATION_NODES format --- app/api/utility.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/api/utility.py b/app/api/utility.py index 484e304..dcc39ec 100644 --- a/app/api/utility.py +++ b/app/api/utility.py @@ -10,6 +10,8 @@ from jsonschema import validate LOCAL_NODE_INDEX_PATH = Path(__file__).parents[2] / "local_nb_nodes.json" + +# Stores the names and URLs of all Neurobagel nodes known to the API instance, in the form of {node_url: node_name, ...} FEDERATION_NODES = {} # We use this schema to validate the local_nb_nodes.json file From 780b6157cef69cc1352197a162a4a9e1fabe7138 Mon Sep 17 00:00:00 2001 From: Alyssa Dai Date: Mon, 8 Jan 2024 23:58:01 -0500 Subject: [PATCH 02/21] implement custom HTTP response for partial success federated query - use custom HTTP success status code - return both node-specific errors and combined successful query results in response body - log node errors and query federation successfulness to console --- app/api/crud.py | 44 +++++++++++++++++++++++++++++++++++++++----- 1 file changed, 39 insertions(+), 5 deletions(-) diff --git a/app/api/crud.py b/app/api/crud.py index c926018..6cff568 100644 --- a/app/api/crud.py +++ b/app/api/crud.py @@ -1,5 +1,9 @@ """CRUD functions called by path operations.""" +import warnings + +from fastapi import HTTPException, status + from . import utility as util @@ -13,7 +17,7 @@ async def get( assessment: str, image_modal: str, node_urls: list[str], -): +) -> list[dict] | dict: """ Makes GET requests to one or more Neurobagel node APIs using send_get_request utility function where the parameters are Neurobagel query parameters. @@ -45,8 +49,10 @@ async def get( """ cross_node_results = [] + node_errors = [] node_urls = util.validate_query_node_url_list(node_urls) + total_nodes = len(node_urls) # Node API query parameters params = {} @@ -67,14 +73,42 @@ async def get( if image_modal: params["image_modal"] = image_modal + # TODO: make the requests asynchronous using asyncio and asyncio.gather, see also https://www.python-httpx.org/async/ for node_url in node_urls: node_name = util.FEDERATION_NODES[node_url] - response = util.send_get_request(node_url + "query/", params) - for result in response: - result["node_name"] = node_name + try: + response = util.send_get_request(node_url + "query/", params) + + for result in response: + result["node_name"] = node_name + + cross_node_results += response + except HTTPException as e: + node_errors.append({"NodeName": node_url, "error": e.detail}) + + warnings.warn( + f"Query to node {node_name} ({node_url}) did not succeed: {e.detail}" + ) - cross_node_results += response + if not node_errors: + # TODO: Use logger instead of print, see https://github.com/tiangolo/fastapi/issues/5003 + print( + f"All nodes were queried successfully ({total_nodes/total_nodes})." + ) + elif len(node_errors) < total_nodes: + failed_node_names = [ + node_error["NodeName"] for node_error in node_errors + ] + print( + f"Queries to {len(failed_node_names)}/{total_nodes} nodes failed: {failed_node_names}." + ) + + raise HTTPException( + status_code=status.HTTP_207_MULTI_STATUS, + detail={"errors": node_errors, "responses": cross_node_results}, + ) + # TODO: Handle case when all nodes fail return cross_node_results From 3643130b86f5ef3d3e7c8f07df7b3e25b12ef871 Mon Sep 17 00:00:00 2001 From: Alyssa Dai Date: Tue, 9 Jan 2024 00:02:53 -0500 Subject: [PATCH 03/21] test API path response for partial success federated query --- tests/test_nodes.py | 87 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 87 insertions(+) diff --git a/tests/test_nodes.py b/tests/test_nodes.py index 4459dc8..9e11f47 100644 --- a/tests/test_nodes.py +++ b/tests/test_nodes.py @@ -1,6 +1,8 @@ import httpx import pytest +from fastapi import HTTPException, status +from app.api import crud from app.api import utility as util @@ -165,3 +167,88 @@ def mock_httpx_get(**kwargs): "No local or public Neurobagel nodes available for federation" in str(exc_info.value) ) + + +def test_partial_node_failures_are_handled_gracefully(monkeypatch, test_app): + """ + Test that when queries to some nodes result in errors, the overall API get request still succeeds, + and that the successful responses are returned along with a list of the encountered errors. + """ + monkeypatch.setattr( + util, + "FEDERATION_NODES", + { + "https://firstpublicnode.org/": "First Public Node", + "https://secondpublicnode.org/": "Second Public Node", + }, + ) + + async def mock_partially_successful_get( + min_age, + max_age, + sex, + diagnosis, + is_control, + min_num_sessions, + assessment, + image_modal, + node_urls, + ): + raise HTTPException( + status_code=status.HTTP_207_MULTI_STATUS, + detail={ + "errors": [ + { + "NodeName": "Second Public Node", + "error": "500 Server Error: Internal Server Error", + }, + ], + "responses": [ + { + "dataset_uuid": "http://neurobagel.org/vocab/12345", + "dataset_name": "QPN", + "dataset_portal_uri": "https://rpq-qpn.ca/en/researchers-section/databases/", + "dataset_total_subjects": 200, + "num_matching_subjects": 5, + "records_protected": True, + "subject_data": "protected", + "image_modals": [ + "http://purl.org/nidash/nidm#T1Weighted", + "http://purl.org/nidash/nidm#T2Weighted", + ], + "node_name": "First Public Node", + }, + ], + }, + ) + + monkeypatch.setattr(crud, "get", mock_partially_successful_get) + response = test_app.get("/query/") + + assert response.is_success + assert response.json() == { + "detail": { + "errors": [ + { + "NodeName": "Second Public Node", + "error": "500 Server Error: Internal Server Error", + }, + ], + "responses": [ + { + "dataset_uuid": "http://neurobagel.org/vocab/12345", + "dataset_name": "QPN", + "dataset_portal_uri": "https://rpq-qpn.ca/en/researchers-section/databases/", + "dataset_total_subjects": 200, + "num_matching_subjects": 5, + "records_protected": True, + "subject_data": "protected", + "image_modals": [ + "http://purl.org/nidash/nidm#T1Weighted", + "http://purl.org/nidash/nidm#T2Weighted", + ], + "node_name": "First Public Node", + }, + ], + } + } From 566e7e18b67c2c6a658b4dcc6643517960451739 Mon Sep 17 00:00:00 2001 From: Alyssa Dai Date: Tue, 9 Jan 2024 12:21:45 -0500 Subject: [PATCH 04/21] mock individual node request to assert over combined query response --- app/api/crud.py | 2 +- app/api/utility.py | 1 + tests/test_nodes.py | 79 +++++++++++++++++++++------------------------ 3 files changed, 39 insertions(+), 43 deletions(-) diff --git a/app/api/crud.py b/app/api/crud.py index 6cff568..e1b36e4 100644 --- a/app/api/crud.py +++ b/app/api/crud.py @@ -85,7 +85,7 @@ async def get( cross_node_results += response except HTTPException as e: - node_errors.append({"NodeName": node_url, "error": e.detail}) + node_errors.append({"NodeName": node_name, "error": e.detail}) warnings.warn( f"Query to node {node_name} ({node_url}) did not succeed: {e.detail}" diff --git a/app/api/utility.py b/app/api/utility.py index dcc39ec..7ca77b1 100644 --- a/app/api/utility.py +++ b/app/api/utility.py @@ -220,6 +220,7 @@ def send_get_request(url: str, params: list): HTTPException _description_ """ + # TODO: Handle case when request fails response = httpx.get( url=url, params=params, diff --git a/tests/test_nodes.py b/tests/test_nodes.py index 9e11f47..76b0022 100644 --- a/tests/test_nodes.py +++ b/tests/test_nodes.py @@ -2,7 +2,6 @@ import pytest from fastapi import HTTPException, status -from app.api import crud from app.api import utility as util @@ -169,10 +168,13 @@ def mock_httpx_get(**kwargs): ) -def test_partial_node_failures_are_handled_gracefully(monkeypatch, test_app): +def test_partial_node_failures_are_handled_gracefully( + monkeypatch, test_app, capsys +): """ - Test that when queries to some nodes result in errors, the overall API get request still succeeds, - and that the successful responses are returned along with a list of the encountered errors. + Test that when queries to some nodes return errors, the overall API get request still succeeds, + the successful responses are returned along with a list of the encountered errors, + and the failed nodes are logged to the console. """ monkeypatch.setattr( util, @@ -183,47 +185,37 @@ def test_partial_node_failures_are_handled_gracefully(monkeypatch, test_app): }, ) - async def mock_partially_successful_get( - min_age, - max_age, - sex, - diagnosis, - is_control, - min_num_sessions, - assessment, - image_modal, - node_urls, - ): + def mock_send_get_request(url, params): + if url == "https://firstpublicnode.org/query/": + return [ + { + "dataset_uuid": "http://neurobagel.org/vocab/12345", + "dataset_name": "QPN", + "dataset_portal_uri": "https://rpq-qpn.ca/en/researchers-section/databases/", + "dataset_total_subjects": 200, + "num_matching_subjects": 5, + "records_protected": True, + "subject_data": "protected", + "image_modals": [ + "http://purl.org/nidash/nidm#T1Weighted", + "http://purl.org/nidash/nidm#T2Weighted", + ], + }, + ] + raise HTTPException( - status_code=status.HTTP_207_MULTI_STATUS, - detail={ - "errors": [ - { - "NodeName": "Second Public Node", - "error": "500 Server Error: Internal Server Error", - }, - ], - "responses": [ - { - "dataset_uuid": "http://neurobagel.org/vocab/12345", - "dataset_name": "QPN", - "dataset_portal_uri": "https://rpq-qpn.ca/en/researchers-section/databases/", - "dataset_total_subjects": 200, - "num_matching_subjects": 5, - "records_protected": True, - "subject_data": "protected", - "image_modals": [ - "http://purl.org/nidash/nidm#T1Weighted", - "http://purl.org/nidash/nidm#T2Weighted", - ], - "node_name": "First Public Node", - }, - ], - }, + status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, + detail="500 Server Error: Internal Server Error", ) - monkeypatch.setattr(crud, "get", mock_partially_successful_get) - response = test_app.get("/query/") + monkeypatch.setattr(util, "send_get_request", mock_send_get_request) + + with pytest.warns( + UserWarning, + match=r"Second Public Node \(https://secondpublicnode.org/\) did not succeed", + ): + response = test_app.get("/query/") + captured = capsys.readouterr() assert response.is_success assert response.json() == { @@ -252,3 +244,6 @@ async def mock_partially_successful_get( ], } } + assert ( + "Queries to 1/2 nodes failed: ['Second Public Node']" in captured.out + ) From 5502a4996f2710f2d88f7afe49645f28e8000232 Mon Sep 17 00:00:00 2001 From: Alyssa Dai Date: Wed, 10 Jan 2024 01:30:47 -0500 Subject: [PATCH 05/21] add new response model returning errors and node query results --- app/api/models.py | 8 ++++++++ app/api/routers/query.py | 6 ++---- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/app/api/models.py b/app/api/models.py index b086776..32f70d0 100644 --- a/app/api/models.py +++ b/app/api/models.py @@ -36,3 +36,11 @@ class CohortQueryResponse(BaseModel): num_matching_subjects: int subject_data: Union[list[dict], str] image_modals: list + + +class CombinedQueryResponse(BaseModel): + """Data model for the combined query results of all matching datasets across all queried nodes.""" + + errors: list + responses: list[CohortQueryResponse] + status: str diff --git a/app/api/routers/query.py b/app/api/routers/query.py index cd53245..eed690c 100644 --- a/app/api/routers/query.py +++ b/app/api/routers/query.py @@ -1,16 +1,14 @@ """Router for query path operations.""" -from typing import List - from fastapi import APIRouter, Depends from .. import crud -from ..models import CohortQueryResponse, QueryModel +from ..models import CombinedQueryResponse, QueryModel router = APIRouter(prefix="/query", tags=["query"]) -@router.get("/", response_model=List[CohortQueryResponse]) +@router.get("/", response_model=CombinedQueryResponse) async def get_query(query: QueryModel = Depends(QueryModel)): """When a GET request is sent, return list of dicts corresponding to subject-level metadata aggregated by dataset.""" response = await crud.get( From 16114da8b60843e00f8500944c659848e61e486a Mon Sep 17 00:00:00 2001 From: Alyssa Dai Date: Wed, 10 Jan 2024 01:34:41 -0500 Subject: [PATCH 06/21] do not use exception object for partial success responses --- app/api/crud.py | 11 ++++++++-- tests/test_nodes.py | 49 ++++++++++++++++++++++----------------------- 2 files changed, 33 insertions(+), 27 deletions(-) diff --git a/app/api/crud.py b/app/api/crud.py index e1b36e4..969ec9e 100644 --- a/app/api/crud.py +++ b/app/api/crud.py @@ -3,6 +3,7 @@ import warnings from fastapi import HTTPException, status +from fastapi.responses import JSONResponse from . import utility as util @@ -104,10 +105,16 @@ async def get( f"Queries to {len(failed_node_names)}/{total_nodes} nodes failed: {failed_node_names}." ) - raise HTTPException( + # See https://fastapi.tiangolo.com/advanced/additional-responses/ for more info + return JSONResponse( status_code=status.HTTP_207_MULTI_STATUS, - detail={"errors": node_errors, "responses": cross_node_results}, + content={ + "errors": node_errors, + "responses": cross_node_results, + "status": "partial success", + }, ) + # TODO: Handle case when all nodes fail return cross_node_results diff --git a/tests/test_nodes.py b/tests/test_nodes.py index 76b0022..32b8bc4 100644 --- a/tests/test_nodes.py +++ b/tests/test_nodes.py @@ -217,32 +217,31 @@ def mock_send_get_request(url, params): response = test_app.get("/query/") captured = capsys.readouterr() - assert response.is_success + assert response.status_code == status.HTTP_207_MULTI_STATUS assert response.json() == { - "detail": { - "errors": [ - { - "NodeName": "Second Public Node", - "error": "500 Server Error: Internal Server Error", - }, - ], - "responses": [ - { - "dataset_uuid": "http://neurobagel.org/vocab/12345", - "dataset_name": "QPN", - "dataset_portal_uri": "https://rpq-qpn.ca/en/researchers-section/databases/", - "dataset_total_subjects": 200, - "num_matching_subjects": 5, - "records_protected": True, - "subject_data": "protected", - "image_modals": [ - "http://purl.org/nidash/nidm#T1Weighted", - "http://purl.org/nidash/nidm#T2Weighted", - ], - "node_name": "First Public Node", - }, - ], - } + "errors": [ + { + "NodeName": "Second Public Node", + "error": "500 Server Error: Internal Server Error", + }, + ], + "responses": [ + { + "dataset_uuid": "http://neurobagel.org/vocab/12345", + "dataset_name": "QPN", + "dataset_portal_uri": "https://rpq-qpn.ca/en/researchers-section/databases/", + "dataset_total_subjects": 200, + "num_matching_subjects": 5, + "records_protected": True, + "subject_data": "protected", + "image_modals": [ + "http://purl.org/nidash/nidm#T1Weighted", + "http://purl.org/nidash/nidm#T2Weighted", + ], + "node_name": "First Public Node", + }, + ], + "status": "partial success", } assert ( "Queries to 1/2 nodes failed: ['Second Public Node']" in captured.out From fe9703f1d9cf9f3684b423ef998425414ce7ff0d Mon Sep 17 00:00:00 2001 From: Alyssa Dai Date: Wed, 10 Jan 2024 01:49:57 -0500 Subject: [PATCH 07/21] rename status key in combined query response --- app/api/models.py | 2 +- tests/test_nodes.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/api/models.py b/app/api/models.py index 32f70d0..ac66e67 100644 --- a/app/api/models.py +++ b/app/api/models.py @@ -43,4 +43,4 @@ class CombinedQueryResponse(BaseModel): errors: list responses: list[CohortQueryResponse] - status: str + nodes_response_status: str diff --git a/tests/test_nodes.py b/tests/test_nodes.py index 32b8bc4..72ce33e 100644 --- a/tests/test_nodes.py +++ b/tests/test_nodes.py @@ -241,7 +241,7 @@ def mock_send_get_request(url, params): "node_name": "First Public Node", }, ], - "status": "partial success", + "nodes_response_status": "partial success", } assert ( "Queries to 1/2 nodes failed: ['Second Public Node']" in captured.out From 9fa13ea44bfacc16789ff0ec04478e82641b957f Mon Sep 17 00:00:00 2001 From: Alyssa Dai Date: Wed, 10 Jan 2024 02:00:38 -0500 Subject: [PATCH 08/21] update response returned when all nodes succeed or fail --- app/api/crud.py | 33 +++++++++++++++++++-------------- 1 file changed, 19 insertions(+), 14 deletions(-) diff --git a/app/api/crud.py b/app/api/crud.py index 969ec9e..51b75a8 100644 --- a/app/api/crud.py +++ b/app/api/crud.py @@ -92,32 +92,37 @@ async def get( f"Query to node {node_name} ({node_url}) did not succeed: {e.detail}" ) - if not node_errors: + if node_errors: # TODO: Use logger instead of print, see https://github.com/tiangolo/fastapi/issues/5003 print( - f"All nodes were queried successfully ({total_nodes/total_nodes})." - ) - elif len(node_errors) < total_nodes: - failed_node_names = [ - node_error["NodeName"] for node_error in node_errors - ] - print( - f"Queries to {len(failed_node_names)}/{total_nodes} nodes failed: {failed_node_names}." + f"Queries to {len(node_errors)}/{total_nodes} nodes failed: {[node_error['NodeName'] for node_error in node_errors]}." ) - # See https://fastapi.tiangolo.com/advanced/additional-responses/ for more info + if len(node_errors) == total_nodes: + # See https://fastapi.tiangolo.com/advanced/additional-responses/ for more info + return JSONResponse( + status_code=status.HTTP_207_MULTI_STATUS, + content={ + "errors": node_errors, + "responses": cross_node_results, + "nodes_response_status": "fail", + }, + ) return JSONResponse( status_code=status.HTTP_207_MULTI_STATUS, content={ "errors": node_errors, "responses": cross_node_results, - "status": "partial success", + "nodes_response_status": "partial success", }, ) - # TODO: Handle case when all nodes fail - - return cross_node_results + print(f"All nodes were queried successfully ({total_nodes/total_nodes}).") + return { + "errors": node_errors, + "responses": cross_node_results, + "nodes_response_status": "success", + } async def get_terms(data_element_URI: str): From 05be17fcda13fb52ba26749c463d68414c8fecd3 Mon Sep 17 00:00:00 2001 From: Alyssa Dai Date: Wed, 10 Jan 2024 16:18:17 -0500 Subject: [PATCH 09/21] handle network errors in federated query --- app/api/utility.py | 37 +++++++++++++++++++++---------------- 1 file changed, 21 insertions(+), 16 deletions(-) diff --git a/app/api/utility.py b/app/api/utility.py index 7ca77b1..7381153 100644 --- a/app/api/utility.py +++ b/app/api/utility.py @@ -6,7 +6,7 @@ import httpx import jsonschema -from fastapi import HTTPException +from fastapi import HTTPException, status from jsonschema import validate LOCAL_NODE_INDEX_PATH = Path(__file__).parents[2] / "local_nb_nodes.json" @@ -220,20 +220,25 @@ def send_get_request(url: str, params: list): HTTPException _description_ """ - # TODO: Handle case when request fails - response = httpx.get( - url=url, - params=params, - # TODO: Revisit timeout value when query performance is improved - timeout=30.0, - # Enable redirect following (off by default) so - # APIs behind a proxy can be reached - follow_redirects=True, - ) + try: + response = httpx.get( + url=url, + params=params, + # TODO: Revisit timeout value when query performance is improved + timeout=30.0, + # Enable redirect following (off by default) so + # APIs behind a proxy can be reached + follow_redirects=True, + ) - if not response.is_success: + if not response.is_success: + raise HTTPException( + status_code=response.status_code, + detail=f"{response.reason_phrase}: {response.text}", + ) + return response.json() + except httpx.NetworkError as exc: raise HTTPException( - status_code=response.status_code, - detail=f"{response.reason_phrase}: {response.text}", - ) - return response.json() + status_code=status.HTTP_503_SERVICE_UNAVAILABLE, + detail=f"Request failed due to a network error or because the node API cannot be reached: {exc}", + ) from exc From 1ea377aa1a243c20344c313b521f9c9c57426e0b Mon Sep 17 00:00:00 2001 From: Alyssa Dai Date: Wed, 10 Jan 2024 16:57:25 -0500 Subject: [PATCH 10/21] test federated response given unreachable nodes, create fixture for single matching dataset result --- tests/conftest.py | 18 ++++++++ tests/test_nodes.py | 108 ++++++++++++++++++++++++++++---------------- 2 files changed, 88 insertions(+), 38 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index 3d9516b..c1f0769 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -8,3 +8,21 @@ def test_app(): client = TestClient(app) yield client + + +@pytest.fixture() +def test_single_matching_dataset_result(): + """Valid aggregate query result for a single matching dataset.""" + return { + "dataset_uuid": "http://neurobagel.org/vocab/12345", + "dataset_name": "QPN", + "dataset_portal_uri": "https://rpq-qpn.ca/en/researchers-section/databases/", + "dataset_total_subjects": 200, + "num_matching_subjects": 5, + "records_protected": True, + "subject_data": "protected", + "image_modals": [ + "http://purl.org/nidash/nidm#T1Weighted", + "http://purl.org/nidash/nidm#T2Weighted", + ], + } diff --git a/tests/test_nodes.py b/tests/test_nodes.py index 72ce33e..d2ffc4d 100644 --- a/tests/test_nodes.py +++ b/tests/test_nodes.py @@ -1,6 +1,6 @@ import httpx import pytest -from fastapi import HTTPException, status +from fastapi import status from app.api import utility as util @@ -168,13 +168,12 @@ def mock_httpx_get(**kwargs): ) -def test_partial_node_failures_are_handled_gracefully( - monkeypatch, test_app, capsys +def test_partial_node_failure_responses_are_handled_gracefully( + monkeypatch, test_app, capsys, test_single_matching_dataset_result ): """ Test that when queries to some nodes return errors, the overall API get request still succeeds, - the successful responses are returned along with a list of the encountered errors, - and the failed nodes are logged to the console. + the successful responses are returned along with a list of the encountered errors, and the failed nodes are logged to the console. """ monkeypatch.setattr( util, @@ -185,30 +184,71 @@ def test_partial_node_failures_are_handled_gracefully( }, ) - def mock_send_get_request(url, params): - if url == "https://firstpublicnode.org/query/": - return [ - { - "dataset_uuid": "http://neurobagel.org/vocab/12345", - "dataset_name": "QPN", - "dataset_portal_uri": "https://rpq-qpn.ca/en/researchers-section/databases/", - "dataset_total_subjects": 200, - "num_matching_subjects": 5, - "records_protected": True, - "subject_data": "protected", - "image_modals": [ - "http://purl.org/nidash/nidm#T1Weighted", - "http://purl.org/nidash/nidm#T2Weighted", - ], - }, - ] + def mock_httpx_get(**kwargs): + if kwargs["url"] == "https://firstpublicnode.org/query/": + return httpx.Response( + status_code=200, json=[test_single_matching_dataset_result] + ) - raise HTTPException( - status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, - detail="500 Server Error: Internal Server Error", + return httpx.Response( + status_code=500, json={}, text="Some internal server error" ) - monkeypatch.setattr(util, "send_get_request", mock_send_get_request) + monkeypatch.setattr(httpx, "get", mock_httpx_get) + + with pytest.warns( + UserWarning, + match=r"Second Public Node \(https://secondpublicnode.org/\) did not succeed", + ): + response = test_app.get("/query/") + captured = capsys.readouterr() + + assert response.status_code == status.HTTP_207_MULTI_STATUS + assert response.json() == { + "errors": [ + { + "NodeName": "Second Public Node", + "error": "Internal Server Error: Some internal server error", + }, + ], + "responses": [ + { + **test_single_matching_dataset_result, + "node_name": "First Public Node", + }, + ], + "nodes_response_status": "partial success", + } + assert ( + "Queries to 1/2 nodes failed: ['Second Public Node']" in captured.out + ) + + +def test_partial_node_connection_failures_are_handled_gracefully( + monkeypatch, test_app, capsys, test_single_matching_dataset_result +): + """ + Test that when requests to some nodes fail (e.g., if API is unreachable), the overall API get request still succeeds, + the successful responses are returned along with a list of the encountered errors, and the failed nodes are logged to the console. + """ + monkeypatch.setattr( + util, + "FEDERATION_NODES", + { + "https://firstpublicnode.org/": "First Public Node", + "https://secondpublicnode.org/": "Second Public Node", + }, + ) + + def mock_httpx_get(**kwargs): + if kwargs["url"] == "https://firstpublicnode.org/query/": + return httpx.Response( + status_code=200, json=[test_single_matching_dataset_result] + ) + + raise httpx.ConnectError("Some connection error") + + monkeypatch.setattr(httpx, "get", mock_httpx_get) with pytest.warns( UserWarning, @@ -222,22 +262,12 @@ def mock_send_get_request(url, params): "errors": [ { "NodeName": "Second Public Node", - "error": "500 Server Error: Internal Server Error", + "error": "Request failed due to a network error or because the node API cannot be reached: Some connection error", }, ], "responses": [ { - "dataset_uuid": "http://neurobagel.org/vocab/12345", - "dataset_name": "QPN", - "dataset_portal_uri": "https://rpq-qpn.ca/en/researchers-section/databases/", - "dataset_total_subjects": 200, - "num_matching_subjects": 5, - "records_protected": True, - "subject_data": "protected", - "image_modals": [ - "http://purl.org/nidash/nidm#T1Weighted", - "http://purl.org/nidash/nidm#T2Weighted", - ], + **test_single_matching_dataset_result, "node_name": "First Public Node", }, ], @@ -246,3 +276,5 @@ def mock_send_get_request(url, params): assert ( "Queries to 1/2 nodes failed: ['Second Public Node']" in captured.out ) + + # TODO: test for all nodes failed, or succeeded From e3385991707181cf772281af1630e6b7578ef109 Mon Sep 17 00:00:00 2001 From: Alyssa Dai Date: Wed, 10 Jan 2024 17:01:54 -0500 Subject: [PATCH 11/21] refactor tests of federated query response into separate module --- tests/conftest.py | 18 ------ tests/test_nodes.py | 113 ------------------------------------ tests/test_query.py | 135 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 135 insertions(+), 131 deletions(-) create mode 100644 tests/test_query.py diff --git a/tests/conftest.py b/tests/conftest.py index c1f0769..3d9516b 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -8,21 +8,3 @@ def test_app(): client = TestClient(app) yield client - - -@pytest.fixture() -def test_single_matching_dataset_result(): - """Valid aggregate query result for a single matching dataset.""" - return { - "dataset_uuid": "http://neurobagel.org/vocab/12345", - "dataset_name": "QPN", - "dataset_portal_uri": "https://rpq-qpn.ca/en/researchers-section/databases/", - "dataset_total_subjects": 200, - "num_matching_subjects": 5, - "records_protected": True, - "subject_data": "protected", - "image_modals": [ - "http://purl.org/nidash/nidm#T1Weighted", - "http://purl.org/nidash/nidm#T2Weighted", - ], - } diff --git a/tests/test_nodes.py b/tests/test_nodes.py index d2ffc4d..4459dc8 100644 --- a/tests/test_nodes.py +++ b/tests/test_nodes.py @@ -1,6 +1,5 @@ import httpx import pytest -from fastapi import status from app.api import utility as util @@ -166,115 +165,3 @@ def mock_httpx_get(**kwargs): "No local or public Neurobagel nodes available for federation" in str(exc_info.value) ) - - -def test_partial_node_failure_responses_are_handled_gracefully( - monkeypatch, test_app, capsys, test_single_matching_dataset_result -): - """ - Test that when queries to some nodes return errors, the overall API get request still succeeds, - the successful responses are returned along with a list of the encountered errors, and the failed nodes are logged to the console. - """ - monkeypatch.setattr( - util, - "FEDERATION_NODES", - { - "https://firstpublicnode.org/": "First Public Node", - "https://secondpublicnode.org/": "Second Public Node", - }, - ) - - def mock_httpx_get(**kwargs): - if kwargs["url"] == "https://firstpublicnode.org/query/": - return httpx.Response( - status_code=200, json=[test_single_matching_dataset_result] - ) - - return httpx.Response( - status_code=500, json={}, text="Some internal server error" - ) - - monkeypatch.setattr(httpx, "get", mock_httpx_get) - - with pytest.warns( - UserWarning, - match=r"Second Public Node \(https://secondpublicnode.org/\) did not succeed", - ): - response = test_app.get("/query/") - captured = capsys.readouterr() - - assert response.status_code == status.HTTP_207_MULTI_STATUS - assert response.json() == { - "errors": [ - { - "NodeName": "Second Public Node", - "error": "Internal Server Error: Some internal server error", - }, - ], - "responses": [ - { - **test_single_matching_dataset_result, - "node_name": "First Public Node", - }, - ], - "nodes_response_status": "partial success", - } - assert ( - "Queries to 1/2 nodes failed: ['Second Public Node']" in captured.out - ) - - -def test_partial_node_connection_failures_are_handled_gracefully( - monkeypatch, test_app, capsys, test_single_matching_dataset_result -): - """ - Test that when requests to some nodes fail (e.g., if API is unreachable), the overall API get request still succeeds, - the successful responses are returned along with a list of the encountered errors, and the failed nodes are logged to the console. - """ - monkeypatch.setattr( - util, - "FEDERATION_NODES", - { - "https://firstpublicnode.org/": "First Public Node", - "https://secondpublicnode.org/": "Second Public Node", - }, - ) - - def mock_httpx_get(**kwargs): - if kwargs["url"] == "https://firstpublicnode.org/query/": - return httpx.Response( - status_code=200, json=[test_single_matching_dataset_result] - ) - - raise httpx.ConnectError("Some connection error") - - monkeypatch.setattr(httpx, "get", mock_httpx_get) - - with pytest.warns( - UserWarning, - match=r"Second Public Node \(https://secondpublicnode.org/\) did not succeed", - ): - response = test_app.get("/query/") - captured = capsys.readouterr() - - assert response.status_code == status.HTTP_207_MULTI_STATUS - assert response.json() == { - "errors": [ - { - "NodeName": "Second Public Node", - "error": "Request failed due to a network error or because the node API cannot be reached: Some connection error", - }, - ], - "responses": [ - { - **test_single_matching_dataset_result, - "node_name": "First Public Node", - }, - ], - "nodes_response_status": "partial success", - } - assert ( - "Queries to 1/2 nodes failed: ['Second Public Node']" in captured.out - ) - - # TODO: test for all nodes failed, or succeeded diff --git a/tests/test_query.py b/tests/test_query.py new file mode 100644 index 0000000..83b31f1 --- /dev/null +++ b/tests/test_query.py @@ -0,0 +1,135 @@ +import httpx +import pytest +from fastapi import status + +from app.api import utility as util + + +@pytest.fixture() +def test_single_matching_dataset_result(): + """Valid aggregate query result for a single matching dataset.""" + return { + "dataset_uuid": "http://neurobagel.org/vocab/12345", + "dataset_name": "QPN", + "dataset_portal_uri": "https://rpq-qpn.ca/en/researchers-section/databases/", + "dataset_total_subjects": 200, + "num_matching_subjects": 5, + "records_protected": True, + "subject_data": "protected", + "image_modals": [ + "http://purl.org/nidash/nidm#T1Weighted", + "http://purl.org/nidash/nidm#T2Weighted", + ], + } + + +def test_partial_node_failure_responses_are_handled_gracefully( + monkeypatch, test_app, capsys, test_single_matching_dataset_result +): + """ + Test that when queries to some nodes return errors, the overall API get request still succeeds, + the successful responses are returned along with a list of the encountered errors, and the failed nodes are logged to the console. + """ + monkeypatch.setattr( + util, + "FEDERATION_NODES", + { + "https://firstpublicnode.org/": "First Public Node", + "https://secondpublicnode.org/": "Second Public Node", + }, + ) + + def mock_httpx_get(**kwargs): + if kwargs["url"] == "https://firstpublicnode.org/query/": + return httpx.Response( + status_code=200, json=[test_single_matching_dataset_result] + ) + + return httpx.Response( + status_code=500, json={}, text="Some internal server error" + ) + + monkeypatch.setattr(httpx, "get", mock_httpx_get) + + with pytest.warns( + UserWarning, + match=r"Second Public Node \(https://secondpublicnode.org/\) did not succeed", + ): + response = test_app.get("/query/") + captured = capsys.readouterr() + + assert response.status_code == status.HTTP_207_MULTI_STATUS + assert response.json() == { + "errors": [ + { + "NodeName": "Second Public Node", + "error": "Internal Server Error: Some internal server error", + }, + ], + "responses": [ + { + **test_single_matching_dataset_result, + "node_name": "First Public Node", + }, + ], + "nodes_response_status": "partial success", + } + assert ( + "Queries to 1/2 nodes failed: ['Second Public Node']" in captured.out + ) + + +def test_partial_node_connection_failures_are_handled_gracefully( + monkeypatch, test_app, capsys, test_single_matching_dataset_result +): + """ + Test that when requests to some nodes fail (e.g., if API is unreachable), the overall API get request still succeeds, + the successful responses are returned along with a list of the encountered errors, and the failed nodes are logged to the console. + """ + monkeypatch.setattr( + util, + "FEDERATION_NODES", + { + "https://firstpublicnode.org/": "First Public Node", + "https://secondpublicnode.org/": "Second Public Node", + }, + ) + + def mock_httpx_get(**kwargs): + if kwargs["url"] == "https://firstpublicnode.org/query/": + return httpx.Response( + status_code=200, json=[test_single_matching_dataset_result] + ) + + raise httpx.ConnectError("Some connection error") + + monkeypatch.setattr(httpx, "get", mock_httpx_get) + + with pytest.warns( + UserWarning, + match=r"Second Public Node \(https://secondpublicnode.org/\) did not succeed", + ): + response = test_app.get("/query/") + captured = capsys.readouterr() + + assert response.status_code == status.HTTP_207_MULTI_STATUS + assert response.json() == { + "errors": [ + { + "NodeName": "Second Public Node", + "error": "Request failed due to a network error or because the node API cannot be reached: Some connection error", + }, + ], + "responses": [ + { + **test_single_matching_dataset_result, + "node_name": "First Public Node", + }, + ], + "nodes_response_status": "partial success", + } + assert ( + "Queries to 1/2 nodes failed: ['Second Public Node']" in captured.out + ) + + # TODO: test for all nodes failed, or succeeded From caa3933bfdd0fdd6c82b9dfb64de360afd861639 Mon Sep 17 00:00:00 2001 From: Alyssa Dai Date: Wed, 10 Jan 2024 17:22:24 -0500 Subject: [PATCH 12/21] test response when queries to all nodes fail --- tests/test_query.py | 42 +++++++++++++++++++++++++++++++++++++++--- 1 file changed, 39 insertions(+), 3 deletions(-) diff --git a/tests/test_query.py b/tests/test_query.py index 83b31f1..bed4bc4 100644 --- a/tests/test_query.py +++ b/tests/test_query.py @@ -23,7 +23,7 @@ def test_single_matching_dataset_result(): } -def test_partial_node_failure_responses_are_handled_gracefully( +def test_partial_node_failure_responses_handled_gracefully( monkeypatch, test_app, capsys, test_single_matching_dataset_result ): """ @@ -79,7 +79,7 @@ def mock_httpx_get(**kwargs): ) -def test_partial_node_connection_failures_are_handled_gracefully( +def test_partial_node_connection_failures_handled_gracefully( monkeypatch, test_app, capsys, test_single_matching_dataset_result ): """ @@ -132,4 +132,40 @@ def mock_httpx_get(**kwargs): "Queries to 1/2 nodes failed: ['Second Public Node']" in captured.out ) - # TODO: test for all nodes failed, or succeeded + +def test_all_nodes_failure_handled_gracefully(monkeypatch, test_app, capsys): + """ + Test that when queries sent to all nodes fail, the federation API get request still succeeds, + but includes an overall failure status and all encountered errors in the response. + """ + monkeypatch.setattr( + util, + "FEDERATION_NODES", + { + "https://firstpublicnode.org/": "First Public Node", + "https://secondpublicnode.org/": "Second Public Node", + }, + ) + + def mock_httpx_get(**kwargs): + raise httpx.ConnectError("Some connection error") + + monkeypatch.setattr(httpx, "get", mock_httpx_get) + + with pytest.warns( + UserWarning, + ) as w: + response = test_app.get("/query/") + captured = capsys.readouterr() + + assert len(w) == 2 + assert response.status_code == status.HTTP_207_MULTI_STATUS + + response = response.json() + assert response["nodes_response_status"] == "fail" + assert len(response["errors"]) == 2 + assert response["responses"] == [] + assert ( + "Queries to 2/2 nodes failed: ['First Public Node', 'Second Public Node']" + in captured.out + ) From d0f858b28ffd2b44926971443b94ff6f523737cd Mon Sep 17 00:00:00 2001 From: Alyssa Dai Date: Wed, 10 Jan 2024 17:37:43 -0500 Subject: [PATCH 13/21] test response when queries to all nodes succeed --- tests/test_query.py | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/tests/test_query.py b/tests/test_query.py index bed4bc4..ee27a64 100644 --- a/tests/test_query.py +++ b/tests/test_query.py @@ -169,3 +169,37 @@ def mock_httpx_get(**kwargs): "Queries to 2/2 nodes failed: ['First Public Node', 'Second Public Node']" in captured.out ) + + +def test_all_nodes_success_handled_gracefully( + monkeypatch, test_app, capsys, test_single_matching_dataset_result +): + """ + Test that when queries sent to all nodes succeed, the federation API response includes an overall success status and no errors. + """ + monkeypatch.setattr( + util, + "FEDERATION_NODES", + { + "https://firstpublicnode.org/": "First Public Node", + "https://secondpublicnode.org/": "Second Public Node", + }, + ) + + def mock_httpx_get(**kwargs): + return httpx.Response( + status_code=200, json=[test_single_matching_dataset_result] + ) + + monkeypatch.setattr(httpx, "get", mock_httpx_get) + + response = test_app.get("/query/") + captured = capsys.readouterr() + + assert response.status_code == status.HTTP_200_OK + + response = response.json() + assert response["nodes_response_status"] == "success" + assert response["errors"] == [] + assert len(response["responses"]) == 2 + assert "All nodes queried successfully" in captured.out From 4b9e926c1a3c566416e512f0f68b4dc2c7f8da4c Mon Sep 17 00:00:00 2001 From: Alyssa Dai Date: Wed, 10 Jan 2024 17:39:34 -0500 Subject: [PATCH 14/21] update comments and type hints --- app/api/crud.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/app/api/crud.py b/app/api/crud.py index 51b75a8..62b2b3c 100644 --- a/app/api/crud.py +++ b/app/api/crud.py @@ -18,7 +18,7 @@ async def get( assessment: str, image_modal: str, node_urls: list[str], -) -> list[dict] | dict: +) -> dict: """ Makes GET requests to one or more Neurobagel node APIs using send_get_request utility function where the parameters are Neurobagel query parameters. @@ -117,7 +117,7 @@ async def get( }, ) - print(f"All nodes were queried successfully ({total_nodes/total_nodes}).") + print(f"All nodes queried successfully ({total_nodes/total_nodes}).") return { "errors": node_errors, "responses": cross_node_results, @@ -126,6 +126,7 @@ async def get( async def get_terms(data_element_URI: str): + # TODO: Make this path able to handle partial successes as well """ Makes a GET request to one or more Neurobagel node APIs using send_get_request utility function where the only parameter is a data element URI. From e1eafdc4bb1d07b62c4aed3b9c2f2ba79f259bb1 Mon Sep 17 00:00:00 2001 From: Alyssa Dai Date: Wed, 10 Jan 2024 17:59:42 -0500 Subject: [PATCH 15/21] turn status of node responses into enum --- app/api/models.py | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/app/api/models.py b/app/api/models.py index ac66e67..69567c6 100644 --- a/app/api/models.py +++ b/app/api/models.py @@ -1,4 +1,5 @@ """Data models.""" +from enum import Enum from typing import Optional, Union from fastapi import Query @@ -38,9 +39,17 @@ class CohortQueryResponse(BaseModel): image_modals: list +class NodesResponseStatus(str, Enum): + """Possible values for the status of the responses from the queried nodes.""" + + SUCCESS = "success" + PARTIAL_SUCCESS = "partial success" + FAIL = "fail" + + class CombinedQueryResponse(BaseModel): """Data model for the combined query results of all matching datasets across all queried nodes.""" errors: list responses: list[CohortQueryResponse] - nodes_response_status: str + nodes_response_status: NodesResponseStatus From b7fbb6097790110c0d8747df9d81ea607c9d535d Mon Sep 17 00:00:00 2001 From: Alyssa Dai Date: Wed, 10 Jan 2024 18:38:55 -0500 Subject: [PATCH 16/21] use model for node error in federated query response --- app/api/crud.py | 4 ++-- app/api/models.py | 9 ++++++++- tests/test_query.py | 4 ++-- 3 files changed, 12 insertions(+), 5 deletions(-) diff --git a/app/api/crud.py b/app/api/crud.py index 62b2b3c..438390a 100644 --- a/app/api/crud.py +++ b/app/api/crud.py @@ -86,7 +86,7 @@ async def get( cross_node_results += response except HTTPException as e: - node_errors.append({"NodeName": node_name, "error": e.detail}) + node_errors.append({"node_name": node_name, "error": e.detail}) warnings.warn( f"Query to node {node_name} ({node_url}) did not succeed: {e.detail}" @@ -95,7 +95,7 @@ async def get( if node_errors: # TODO: Use logger instead of print, see https://github.com/tiangolo/fastapi/issues/5003 print( - f"Queries to {len(node_errors)}/{total_nodes} nodes failed: {[node_error['NodeName'] for node_error in node_errors]}." + f"Queries to {len(node_errors)}/{total_nodes} nodes failed: {[node_error['node_name'] for node_error in node_errors]}." ) if len(node_errors) == total_nodes: diff --git a/app/api/models.py b/app/api/models.py index 69567c6..1c0f7b3 100644 --- a/app/api/models.py +++ b/app/api/models.py @@ -47,9 +47,16 @@ class NodesResponseStatus(str, Enum): FAIL = "fail" +class NodeError(BaseModel): + """Data model for an error encountered when querying a node.""" + + node_name: str + error: str + + class CombinedQueryResponse(BaseModel): """Data model for the combined query results of all matching datasets across all queried nodes.""" - errors: list + errors: list[NodeError] responses: list[CohortQueryResponse] nodes_response_status: NodesResponseStatus diff --git a/tests/test_query.py b/tests/test_query.py index ee27a64..d40a7c8 100644 --- a/tests/test_query.py +++ b/tests/test_query.py @@ -62,7 +62,7 @@ def mock_httpx_get(**kwargs): assert response.json() == { "errors": [ { - "NodeName": "Second Public Node", + "node_name": "Second Public Node", "error": "Internal Server Error: Some internal server error", }, ], @@ -116,7 +116,7 @@ def mock_httpx_get(**kwargs): assert response.json() == { "errors": [ { - "NodeName": "Second Public Node", + "node_name": "Second Public Node", "error": "Request failed due to a network error or because the node API cannot be reached: Some connection error", }, ], From cf8d392b87d6d7c2e99cfc85fa2993fd6f3db13c Mon Sep 17 00:00:00 2001 From: Alyssa Dai Date: Thu, 11 Jan 2024 02:33:16 -0500 Subject: [PATCH 17/21] switch to sending requests to nodes asynchronously --- app/api/crud.py | 29 ++++++++++++++++------------- app/api/utility.py | 43 ++++++++++++++++++++++--------------------- 2 files changed, 38 insertions(+), 34 deletions(-) diff --git a/app/api/crud.py b/app/api/crud.py index 438390a..8be3a98 100644 --- a/app/api/crud.py +++ b/app/api/crud.py @@ -1,5 +1,6 @@ """CRUD functions called by path operations.""" +import asyncio import warnings from fastapi import HTTPException, status @@ -74,23 +75,25 @@ async def get( if image_modal: params["image_modal"] = image_modal - # TODO: make the requests asynchronous using asyncio and asyncio.gather, see also https://www.python-httpx.org/async/ - for node_url in node_urls: - node_name = util.FEDERATION_NODES[node_url] - - try: - response = util.send_get_request(node_url + "query/", params) + tasks = [ + util.send_get_request(node_url + "query/", params) + for node_url in node_urls + ] + responses = await asyncio.gather(*tasks, return_exceptions=True) + for node_url, response in zip(node_urls, responses): + node_name = util.FEDERATION_NODES[node_url] + if isinstance(response, HTTPException): + node_errors.append( + {"node_name": node_name, "error": response.detail} + ) + warnings.warn( + f"Query to node {node_name} ({node_url}) did not succeed: {response.detail}" + ) + else: for result in response: result["node_name"] = node_name - cross_node_results += response - except HTTPException as e: - node_errors.append({"node_name": node_name, "error": e.detail}) - - warnings.warn( - f"Query to node {node_name} ({node_url}) did not succeed: {e.detail}" - ) if node_errors: # TODO: Use logger instead of print, see https://github.com/tiangolo/fastapi/issues/5003 diff --git a/app/api/utility.py b/app/api/utility.py index 7381153..792df6f 100644 --- a/app/api/utility.py +++ b/app/api/utility.py @@ -198,7 +198,7 @@ def validate_query_node_url_list(node_urls: list) -> list: return node_urls -def send_get_request(url: str, params: list): +async def send_get_request(url: str, params: list): """ Makes a GET request to one or more Neurobagel nodes. @@ -220,25 +220,26 @@ def send_get_request(url: str, params: list): HTTPException _description_ """ - try: - response = httpx.get( - url=url, - params=params, - # TODO: Revisit timeout value when query performance is improved - timeout=30.0, - # Enable redirect following (off by default) so - # APIs behind a proxy can be reached - follow_redirects=True, - ) + async with httpx.AsyncClient() as client: + try: + response = await client.get( + url=url, + params=params, + # TODO: Revisit timeout value when query performance is improved + timeout=30.0, + # Enable redirect following (off by default) so + # APIs behind a proxy can be reached + follow_redirects=True, + ) - if not response.is_success: + if not response.is_success: + raise HTTPException( + status_code=response.status_code, + detail=f"{response.reason_phrase}: {response.text}", + ) + return response.json() + except httpx.NetworkError as exc: raise HTTPException( - status_code=response.status_code, - detail=f"{response.reason_phrase}: {response.text}", - ) - return response.json() - except httpx.NetworkError as exc: - raise HTTPException( - status_code=status.HTTP_503_SERVICE_UNAVAILABLE, - detail=f"Request failed due to a network error or because the node API cannot be reached: {exc}", - ) from exc + status_code=status.HTTP_503_SERVICE_UNAVAILABLE, + detail=f"Request failed due to a network error or because the node API cannot be reached: {exc}", + ) from exc From 83a50cd98e25ccf464e6b931b9a5631cfe39351c Mon Sep 17 00:00:00 2001 From: Alyssa Dai Date: Thu, 11 Jan 2024 02:33:47 -0500 Subject: [PATCH 18/21] update mocked get function in tests to be async --- tests/test_query.py | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/tests/test_query.py b/tests/test_query.py index d40a7c8..0bc7849 100644 --- a/tests/test_query.py +++ b/tests/test_query.py @@ -39,7 +39,7 @@ def test_partial_node_failure_responses_handled_gracefully( }, ) - def mock_httpx_get(**kwargs): + async def mock_httpx_get(self, **kwargs): if kwargs["url"] == "https://firstpublicnode.org/query/": return httpx.Response( status_code=200, json=[test_single_matching_dataset_result] @@ -49,7 +49,7 @@ def mock_httpx_get(**kwargs): status_code=500, json={}, text="Some internal server error" ) - monkeypatch.setattr(httpx, "get", mock_httpx_get) + monkeypatch.setattr(httpx.AsyncClient, "get", mock_httpx_get) with pytest.warns( UserWarning, @@ -95,7 +95,7 @@ def test_partial_node_connection_failures_handled_gracefully( }, ) - def mock_httpx_get(**kwargs): + async def mock_httpx_get(self, **kwargs): if kwargs["url"] == "https://firstpublicnode.org/query/": return httpx.Response( status_code=200, json=[test_single_matching_dataset_result] @@ -103,7 +103,7 @@ def mock_httpx_get(**kwargs): raise httpx.ConnectError("Some connection error") - monkeypatch.setattr(httpx, "get", mock_httpx_get) + monkeypatch.setattr(httpx.AsyncClient, "get", mock_httpx_get) with pytest.warns( UserWarning, @@ -147,10 +147,10 @@ def test_all_nodes_failure_handled_gracefully(monkeypatch, test_app, capsys): }, ) - def mock_httpx_get(**kwargs): + async def mock_httpx_get(self, **kwargs): raise httpx.ConnectError("Some connection error") - monkeypatch.setattr(httpx, "get", mock_httpx_get) + monkeypatch.setattr(httpx.AsyncClient, "get", mock_httpx_get) with pytest.warns( UserWarning, @@ -186,12 +186,12 @@ def test_all_nodes_success_handled_gracefully( }, ) - def mock_httpx_get(**kwargs): + async def mock_httpx_get(self, **kwargs): return httpx.Response( status_code=200, json=[test_single_matching_dataset_result] ) - monkeypatch.setattr(httpx, "get", mock_httpx_get) + monkeypatch.setattr(httpx.AsyncClient, "get", mock_httpx_get) response = test_app.get("/query/") captured = capsys.readouterr() From ab50b3754981ad7111a8444633fbaefc5f7295a3 Mon Sep 17 00:00:00 2001 From: Alyssa Dai Date: Thu, 11 Jan 2024 03:09:00 -0500 Subject: [PATCH 19/21] update return type hint --- app/api/utility.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/api/utility.py b/app/api/utility.py index 792df6f..6636325 100644 --- a/app/api/utility.py +++ b/app/api/utility.py @@ -198,7 +198,7 @@ def validate_query_node_url_list(node_urls: list) -> list: return node_urls -async def send_get_request(url: str, params: list): +async def send_get_request(url: str, params: list) -> dict: """ Makes a GET request to one or more Neurobagel nodes. From f65aff825ae4f90bb08c885d4573ce89fbc68393 Mon Sep 17 00:00:00 2001 From: Alyssa Dai Date: Sun, 14 Jan 2024 16:34:35 -0500 Subject: [PATCH 20/21] make code a bit cleaner Co-authored-by: Sebastian Urchs --- app/api/crud.py | 2 +- tests/test_query.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/api/crud.py b/app/api/crud.py index 8be3a98..3444bda 100644 --- a/app/api/crud.py +++ b/app/api/crud.py @@ -93,7 +93,7 @@ async def get( else: for result in response: result["node_name"] = node_name - cross_node_results += response + cross_node_results.extend(response) if node_errors: # TODO: Use logger instead of print, see https://github.com/tiangolo/fastapi/issues/5003 diff --git a/tests/test_query.py b/tests/test_query.py index 0bc7849..0c5ae31 100644 --- a/tests/test_query.py +++ b/tests/test_query.py @@ -6,7 +6,7 @@ @pytest.fixture() -def test_single_matching_dataset_result(): +def mocked_single_matching_dataset_result(): """Valid aggregate query result for a single matching dataset.""" return { "dataset_uuid": "http://neurobagel.org/vocab/12345", From f3640e677f29f76b0dc04ac023536f1ec24ed010 Mon Sep 17 00:00:00 2001 From: Alyssa Dai Date: Sun, 14 Jan 2024 16:57:08 -0500 Subject: [PATCH 21/21] rename fixture for mocked data --- tests/test_query.py | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/tests/test_query.py b/tests/test_query.py index 0c5ae31..4cbaf25 100644 --- a/tests/test_query.py +++ b/tests/test_query.py @@ -24,7 +24,7 @@ def mocked_single_matching_dataset_result(): def test_partial_node_failure_responses_handled_gracefully( - monkeypatch, test_app, capsys, test_single_matching_dataset_result + monkeypatch, test_app, capsys, mocked_single_matching_dataset_result ): """ Test that when queries to some nodes return errors, the overall API get request still succeeds, @@ -42,7 +42,7 @@ def test_partial_node_failure_responses_handled_gracefully( async def mock_httpx_get(self, **kwargs): if kwargs["url"] == "https://firstpublicnode.org/query/": return httpx.Response( - status_code=200, json=[test_single_matching_dataset_result] + status_code=200, json=[mocked_single_matching_dataset_result] ) return httpx.Response( @@ -68,7 +68,7 @@ async def mock_httpx_get(self, **kwargs): ], "responses": [ { - **test_single_matching_dataset_result, + **mocked_single_matching_dataset_result, "node_name": "First Public Node", }, ], @@ -80,7 +80,7 @@ async def mock_httpx_get(self, **kwargs): def test_partial_node_connection_failures_handled_gracefully( - monkeypatch, test_app, capsys, test_single_matching_dataset_result + monkeypatch, test_app, capsys, mocked_single_matching_dataset_result ): """ Test that when requests to some nodes fail (e.g., if API is unreachable), the overall API get request still succeeds, @@ -98,7 +98,7 @@ def test_partial_node_connection_failures_handled_gracefully( async def mock_httpx_get(self, **kwargs): if kwargs["url"] == "https://firstpublicnode.org/query/": return httpx.Response( - status_code=200, json=[test_single_matching_dataset_result] + status_code=200, json=[mocked_single_matching_dataset_result] ) raise httpx.ConnectError("Some connection error") @@ -122,7 +122,7 @@ async def mock_httpx_get(self, **kwargs): ], "responses": [ { - **test_single_matching_dataset_result, + **mocked_single_matching_dataset_result, "node_name": "First Public Node", }, ], @@ -172,7 +172,7 @@ async def mock_httpx_get(self, **kwargs): def test_all_nodes_success_handled_gracefully( - monkeypatch, test_app, capsys, test_single_matching_dataset_result + monkeypatch, test_app, capsys, mocked_single_matching_dataset_result ): """ Test that when queries sent to all nodes succeed, the federation API response includes an overall success status and no errors. @@ -188,7 +188,7 @@ def test_all_nodes_success_handled_gracefully( async def mock_httpx_get(self, **kwargs): return httpx.Response( - status_code=200, json=[test_single_matching_dataset_result] + status_code=200, json=[mocked_single_matching_dataset_result] ) monkeypatch.setattr(httpx.AsyncClient, "get", mock_httpx_get)