From 1503ca5bddbfb12a826a388db92ddacac49e4b94 Mon Sep 17 00:00:00 2001 From: HarikaBishai Date: Wed, 26 Aug 2026 19:14:57 -0400 Subject: [PATCH 1/8] raising 404 exeption on lookup error --- src/nsls2api/api/v1/user_api.py | 79 +++++++++++++++------------------ 1 file changed, 37 insertions(+), 42 deletions(-) diff --git a/src/nsls2api/api/v1/user_api.py b/src/nsls2api/api/v1/user_api.py index 39f54085..8d278038 100644 --- a/src/nsls2api/api/v1/user_api.py +++ b/src/nsls2api/api/v1/user_api.py @@ -16,52 +16,47 @@ @router.get("/person/username/{username}", response_model=Person) async def get_person_from_username(username: str): - bnl_person = await bnlpeople_service.get_person_by_username(username) - print(bnl_person) - if bnl_person: - person = Person( - firstname=bnl_person.FirstName, - lastname=bnl_person.LastName, - email=bnl_person.BNLEmail, - bnl_id=bnl_person.EmployeeNumber, - institution=bnl_person.Institution, - username=bnl_person.ActiveDirectoryName, - cyber_agreement_signed=bnl_person.CyberAgreementSigned, - ) - # If the person is an Employee then set their institution to BNL - if ( - bnl_person.EmployeeStatus == "Active" - and bnl_person.EmployeeType == "Employee" - ): - person.bnl_employee = True - person.institution = "Brookhaven National Laboratory" - return person - else: - return fastapi.responses.JSONResponse( - {"error": f"No people with username {username} found."}, - status_code=404, - ) + try: + bnl_person = await bnlpeople_service.get_person_by_username(username) + except LookupError as e: + raise HTTPException(status_code=404, detail=str(e)) + + person = Person( + firstname=bnl_person.FirstName, + lastname=bnl_person.LastName, + email=bnl_person.BNLEmail, + bnl_id=bnl_person.EmployeeNumber, + institution=bnl_person.Institution, + username=bnl_person.ActiveDirectoryName, + cyber_agreement_signed=bnl_person.CyberAgreementSigned, + ) + # If the person is an Employee then set their institution to BNL + if ( + bnl_person.EmployeeStatus == "Active" + and bnl_person.EmployeeType == "Employee" + ): + person.bnl_employee = True + person.institution = "Brookhaven National Laboratory" + return person @router.get("/person/email/{email}") async def get_person_from_email(email: str): - bnl_person = await bnlpeople_service.get_person_by_email(email) - if bnl_person: - person = Person( - firstname=bnl_person.FirstName, - lastname=bnl_person.LastName, - email=bnl_person.BNLEmail, - bnl_id=bnl_person.EmployeeNumber, - institution=bnl_person.Institution, - username=bnl_person.ActiveDirectoryName, - cyber_agreement_signed=bnl_person.CyberAgreementSigned, - ) - return person - else: - return fastapi.responses.JSONResponse( - {"error": f"No people with username {email} found."}, - status_code=404, - ) + try: + bnl_person = await bnlpeople_service.get_person_by_email(email) + except LookupError as e: + raise HTTPException(status_code=404, detail=str(e)) + + person = Person( + firstname=bnl_person.FirstName, + lastname=bnl_person.LastName, + email=bnl_person.BNLEmail, + bnl_id=bnl_person.EmployeeNumber, + institution=bnl_person.Institution, + username=bnl_person.ActiveDirectoryName, + cyber_agreement_signed=bnl_person.CyberAgreementSigned, + ) + return person # TODO: Add back into schema if we decide to use this endpoint. From b8fa2fb11adf0145e79c6494109af9f070afb097 Mon Sep 17 00:00:00 2001 From: HarikaBishai Date: Wed, 26 Aug 2026 19:22:38 -0400 Subject: [PATCH 2/8] change to custom error messages like earlier --- src/nsls2api/api/v1/user_api.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/nsls2api/api/v1/user_api.py b/src/nsls2api/api/v1/user_api.py index 8d278038..e389e8d3 100644 --- a/src/nsls2api/api/v1/user_api.py +++ b/src/nsls2api/api/v1/user_api.py @@ -18,8 +18,8 @@ async def get_person_from_username(username: str): try: bnl_person = await bnlpeople_service.get_person_by_username(username) - except LookupError as e: - raise HTTPException(status_code=404, detail=str(e)) + except LookupError: + raise HTTPException(status_code=404, detail=f"No people with username {username} found.") person = Person( firstname=bnl_person.FirstName, @@ -44,8 +44,8 @@ async def get_person_from_username(username: str): async def get_person_from_email(email: str): try: bnl_person = await bnlpeople_service.get_person_by_email(email) - except LookupError as e: - raise HTTPException(status_code=404, detail=str(e)) + except LookupError: + raise HTTPException(status_code=404, detail=f"No people with email {email} found.") person = Person( firstname=bnl_person.FirstName, From c253c945c55e9603b08c74b72ccbad1121afcd6f Mon Sep 17 00:00:00 2001 From: HarikaBishai Date: Thu, 27 Aug 2026 12:01:35 -0400 Subject: [PATCH 3/8] test cases added and valueerror added --- src/nsls2api/api/v1/user_api.py | 19 +- src/nsls2api/services/bnlpeople_service.py | 30 ++- src/nsls2api/services/person_service.py | 13 +- src/nsls2api/services/proposal_service.py | 3 +- src/nsls2api/tests/api/test_user_api.py | 224 +++++++++++++++++++++ 5 files changed, 262 insertions(+), 27 deletions(-) create mode 100644 src/nsls2api/tests/api/test_user_api.py diff --git a/src/nsls2api/api/v1/user_api.py b/src/nsls2api/api/v1/user_api.py index e389e8d3..fb023720 100644 --- a/src/nsls2api/api/v1/user_api.py +++ b/src/nsls2api/api/v1/user_api.py @@ -18,8 +18,10 @@ async def get_person_from_username(username: str): try: bnl_person = await bnlpeople_service.get_person_by_username(username) - except LookupError: - raise HTTPException(status_code=404, detail=f"No people with username {username} found.") + except LookupError as e: + raise HTTPException(status_code=404, detail=str(e)) + except ValueError as e: + raise HTTPException(status_code=400, detail=str(e)) person = Person( firstname=bnl_person.FirstName, @@ -44,8 +46,10 @@ async def get_person_from_username(username: str): async def get_person_from_email(email: str): try: bnl_person = await bnlpeople_service.get_person_by_email(email) - except LookupError: - raise HTTPException(status_code=404, detail=f"No people with email {email} found.") + except LookupError as e: + raise HTTPException(status_code=404, detail=str(e)) + except ValueError as e: + raise HTTPException(status_code=400, detail=str(e)) person = Person( firstname=bnl_person.FirstName, @@ -56,6 +60,13 @@ async def get_person_from_email(email: str): username=bnl_person.ActiveDirectoryName, cyber_agreement_signed=bnl_person.CyberAgreementSigned, ) + # If the person is an Employee then set their institution to BNL + if ( + bnl_person.EmployeeStatus == "Active" + and bnl_person.EmployeeType == "Employee" + ): + person.bnl_employee = True + person.institution = "Brookhaven National Laboratory" return person diff --git a/src/nsls2api/services/bnlpeople_service.py b/src/nsls2api/services/bnlpeople_service.py index 20b7a71f..d4e43a88 100644 --- a/src/nsls2api/services/bnlpeople_service.py +++ b/src/nsls2api/services/bnlpeople_service.py @@ -22,10 +22,16 @@ async def get_all_people(): async def get_person_by_username(username: str) -> BNLPerson | None: url = f"{base_url}/api/BNLPeople?accountName={username}" person = await _call_bnlpeople_webservice(url) - if len(person) == 0 or len(person) > 1: - raise LookupError( - f"BNL People could not find a person with a username of '{username}'" + if len(person) == 0: + logger.warning( + f"BNL People API could not find a person with a username of '{username}'" + ) + raise LookupError(f"No person with username {username} found.") + if len(person) > 1: + logger.error( + f"BNL People API returned {len(person)} people for username '{username}' - ambiguous result" ) + raise ValueError(f"Multiple people found with username {username}.") return BNLPerson(**person[0]) @@ -44,7 +50,7 @@ async def get_username_by_id(lifenumber: str) -> str | None: # logger.debug(person) if len(person) == 0 or len(person) > 1: logger.warning( - f"BNL People could not find a person with an employee/life number of '{lifenumber}'" + f"BNL People API could not find a person with an employee/life number of '{lifenumber}'" ) return None @@ -67,7 +73,7 @@ async def get_person_by_id(lifenumber: str) -> BNLPerson | None: if len(person) == 0 or len(person) > 1: raise LookupError( - f"BNL People could not find a person with an employee/life number of '{lifenumber}'" + f"BNL People API could not find a person with an employee/life number of '{lifenumber}'" ) return BNLPerson(**person[0]) @@ -75,10 +81,16 @@ async def get_person_by_id(lifenumber: str) -> BNLPerson | None: async def get_person_by_email(email: str) -> BNLPerson | None: url = f"{base_url}/api/BNLPeople?email={email}" person = await _call_bnlpeople_webservice(url) - if len(person) == 0 or len(person) > 1: - raise LookupError( - f"BNL People could not find a person with an email of '{email}'" + if len(person) == 0: + logger.warning( + f"BNL People API could not find a person with an email of '{email}'" + ) + raise LookupError(f"No person with email {email} found.") + if len(person) > 1: + logger.error( + f"BNL People API returned {len(person)} people for email '{email}' - ambiguous result" ) + raise ValueError(f"Multiple people found with email {email}. Query is ambiguous.") return BNLPerson(**person[0]) @@ -89,7 +101,7 @@ async def get_people_by_department( people = await _call_bnlpeople_webservice(url) if len(people) == 0: raise LookupError( - f"BNL People could not find a person with the department code of '{department_code}'" + f"BNL People API could not find a person with the department code of '{department_code}'" ) people_in_department = [BNLPerson(**p) for p in people] return people_in_department diff --git a/src/nsls2api/services/person_service.py b/src/nsls2api/services/person_service.py index ee6c4f5a..60258b6b 100644 --- a/src/nsls2api/services/person_service.py +++ b/src/nsls2api/services/person_service.py @@ -37,22 +37,11 @@ async def diagnostic_details_by_username(username: str) -> Person | None: ) ad_groups = await n2sn_service.get_groups_by_username(username) proposals = await get_proposals_by_person(bnl_person.EmployeeNumber) - except LookupError as error: + except (LookupError, ValueError) as error: raise LookupError( f"Error obtaining diagnostic details for username of {username}" ) from error - print(bnl_person) - print("-------") - - print(ad_person) - print("-------") - - print(ad_groups) - print("-------") - - print(proposals) - print("-------") person = Person( firstname=bnl_person.FirstName, diff --git a/src/nsls2api/services/proposal_service.py b/src/nsls2api/services/proposal_service.py index 17fa544b..daddd57a 100644 --- a/src/nsls2api/services/proposal_service.py +++ b/src/nsls2api/services/proposal_service.py @@ -810,8 +810,7 @@ async def generate_fake_test_proposal( is_pi=True, ) user_list.append(user) - except LookupError: - logger.error(f"Could not find user {add_specific_user} in BNLPeople.") + except (LookupError, ValueError): return None fake_proposal_id = await generate_fake_proposal_id() diff --git a/src/nsls2api/tests/api/test_user_api.py b/src/nsls2api/tests/api/test_user_api.py new file mode 100644 index 00000000..78df11e6 --- /dev/null +++ b/src/nsls2api/tests/api/test_user_api.py @@ -0,0 +1,224 @@ +import pytest +from httpx import ASGITransport, AsyncClient +from unittest.mock import AsyncMock, patch + +from nsls2api.main import app + +@pytest.mark.anyio +async def test_get_person_by_username_not_found(): + """Test that requesting a non-existent username returns 404.""" + with patch( + "nsls2api.services.bnlpeople_service._call_bnlpeople_webservice", + new_callable=AsyncMock, + return_value=[], + ): + async with AsyncClient( + transport=ASGITransport(app=app), base_url="http://test" + ) as ac: + response = await ac.get("/v1/person/username/nonexistent_user_xyz123") + + assert response.status_code == 404 + response_json = response.json() + assert "detail" in response_json + assert "No person with username nonexistent_user_xyz123 found." in response_json["detail"] + + +@pytest.mark.anyio +async def test_get_person_by_username_multiple_found(): + """Test that multiple people with same username returns 400.""" + # Mock API response with 2 people + mock_api_response = [ + { + "FirstName": "John", + "LastName": "Doe", + "BNLEmail": "john.doe@bnl.gov", + "EmployeeNumber": "123456", + "Institution": "Brookhaven National Laboratory", + "ActiveDirectoryName": "jdoe", + "CyberAgreementSigned": None, + "EmployeeStatus": "Active", + "EmployeeType": "Employee", + "FacilityCode": None, + "Facility": None, + }, + { + "FirstName": "John", + "LastName": "Doe", + "BNLEmail": "john.doe@bnl.gov", + "EmployeeNumber": "789012", + "Institution": "Brookhaven National Laboratory", + "ActiveDirectoryName": "jdoe", + "CyberAgreementSigned": None, + "EmployeeStatus": "Active", + "EmployeeType": "Employee", + "FacilityCode": None, + "Facility": None, + }, + ] + + with patch( + "nsls2api.services.bnlpeople_service._call_bnlpeople_webservice", + new_callable=AsyncMock, + return_value=mock_api_response, + ): + async with AsyncClient( + transport=ASGITransport(app=app), base_url="http://test" + ) as ac: + response = await ac.get("/v1/person/username/jdoe") + + assert response.status_code == 400 + response_json = response.json() + assert "detail" in response_json + assert "Multiple people found with username jdoe" in response_json["detail"] + + +@pytest.mark.anyio +async def test_get_person_by_username_success(): + """Test that valid username returns 200 with person data.""" + mock_api_response = [ + { + "FirstName": "John", + "LastName": "Doe", + "BNLEmail": "john.doe@bnl.gov", + "EmployeeNumber": "123456", + "Institution": "Brookhaven National Laboratory", + "ActiveDirectoryName": "jdoe", + "CyberAgreementSigned": None, + "EmployeeStatus": "Active", + "EmployeeType": "Employee", + "FacilityCode": None, + "Facility": None, + } + ] + + with patch( + "nsls2api.services.bnlpeople_service._call_bnlpeople_webservice", + new_callable=AsyncMock, + return_value=mock_api_response, + ): + async with AsyncClient( + transport=ASGITransport(app=app), base_url="http://test" + ) as ac: + response = await ac.get("/v1/person/username/jdoe") + + assert response.status_code == 200 + response_json = response.json() + assert response_json["firstname"] == "John" + assert response_json["lastname"] == "Doe" + assert response_json["email"] == "john.doe@bnl.gov" + assert response_json["username"] == "jdoe" + assert response_json["bnl_employee"] is True + + +# ============================================================================ +# EMAIL TESTS +# ============================================================================ + + +@pytest.mark.anyio +async def test_get_person_by_email_not_found(): + """Test that requesting a non-existent email returns 404.""" + with patch( + "nsls2api.services.bnlpeople_service._call_bnlpeople_webservice", + new_callable=AsyncMock, + return_value=[], + ): + async with AsyncClient( + transport=ASGITransport(app=app), base_url="http://test" + ) as ac: + response = await ac.get("/v1/person/email/nonexistent@example.com") + + assert response.status_code == 404 + response_json = response.json() + assert "detail" in response_json + assert "No person with email nonexistent@example.com found." in response_json["detail"] + + +@pytest.mark.anyio +async def test_get_person_by_email_multiple_found(): + """Test that multiple people with same email returns 400.""" + # Mock API response with 2 people + mock_api_response = [ + { + "FirstName": "Jane", + "LastName": "Smith", + "BNLEmail": "jane@example.com", + "EmployeeNumber": "123456", + "Institution": "Brookhaven National Laboratory", + "ActiveDirectoryName": "jsmith", + "CyberAgreementSigned": None, + "EmployeeStatus": "Active", + "EmployeeType": "Employee", + "FacilityCode": None, + "Facility": None, + }, + { + "FirstName": "Jane", + "LastName": "Smith", + "BNLEmail": "jane@example.com", + "EmployeeNumber": "654321", + "Institution": "Brookhaven National Laboratory", + "ActiveDirectoryName": "jsmith2", + "CyberAgreementSigned": None, + "EmployeeStatus": "Active", + "EmployeeType": "Employee", + "FacilityCode": None, + "Facility": None, + }, + ] + + with patch( + "nsls2api.services.bnlpeople_service._call_bnlpeople_webservice", + new_callable=AsyncMock, + return_value=mock_api_response, + ): + async with AsyncClient( + transport=ASGITransport(app=app), base_url="http://test" + ) as ac: + response = await ac.get("/v1/person/email/jane@example.com") + + assert response.status_code == 400 + response_json = response.json() + assert "detail" in response_json + assert "Multiple people found with email jane@example.com" in response_json["detail"] + + +@pytest.mark.anyio +async def test_get_person_by_email_success(): + """Test that valid email returns 200 with person data.""" + mock_api_response = [ + { + "FirstName": "Jane", + "LastName": "Smith", + "BNLEmail": "jane.smith@bnl.gov", + "EmployeeNumber": "654321", + "Institution": "Brookhaven National Laboratory", + "ActiveDirectoryName": "jsmith", + "CyberAgreementSigned": None, + "EmployeeStatus": "Active", + "EmployeeType": "Employee", + "FacilityCode": None, + "Facility": None, + } + ] + + with patch( + "nsls2api.services.bnlpeople_service._call_bnlpeople_webservice", + new_callable=AsyncMock, + return_value=mock_api_response, + ): + async with AsyncClient( + transport=ASGITransport(app=app), base_url="http://test" + ) as ac: + response = await ac.get("/v1/person/email/jane.smith@bnl.gov") + + assert response.status_code == 200 + response_json = response.json() + assert response_json["firstname"] == "Jane" + assert response_json["lastname"] == "Smith" + assert response_json["email"] == "jane.smith@bnl.gov" + assert response_json["username"] == "jsmith" + assert response_json["bnl_employee"] is True + + + From ae32163a1b047719706078a5d1876055a0a06bf3 Mon Sep 17 00:00:00 2001 From: HarikaBishai Date: Thu, 27 Aug 2026 12:12:10 -0400 Subject: [PATCH 4/8] review comment changes --- src/nsls2api/api/v1/user_api.py | 2 +- src/nsls2api/services/bnlpeople_service.py | 4 ++-- src/nsls2api/services/proposal_service.py | 3 ++- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/nsls2api/api/v1/user_api.py b/src/nsls2api/api/v1/user_api.py index fb023720..292460ed 100644 --- a/src/nsls2api/api/v1/user_api.py +++ b/src/nsls2api/api/v1/user_api.py @@ -42,7 +42,7 @@ async def get_person_from_username(username: str): return person -@router.get("/person/email/{email}") +@router.get("/person/email/{email}", response_model=Person) async def get_person_from_email(email: str): try: bnl_person = await bnlpeople_service.get_person_by_email(email) diff --git a/src/nsls2api/services/bnlpeople_service.py b/src/nsls2api/services/bnlpeople_service.py index d4e43a88..55297596 100644 --- a/src/nsls2api/services/bnlpeople_service.py +++ b/src/nsls2api/services/bnlpeople_service.py @@ -19,7 +19,7 @@ async def get_all_people(): return people -async def get_person_by_username(username: str) -> BNLPerson | None: +async def get_person_by_username(username: str) -> BNLPerson: url = f"{base_url}/api/BNLPeople?accountName={username}" person = await _call_bnlpeople_webservice(url) if len(person) == 0: @@ -78,7 +78,7 @@ async def get_person_by_id(lifenumber: str) -> BNLPerson | None: return BNLPerson(**person[0]) -async def get_person_by_email(email: str) -> BNLPerson | None: +async def get_person_by_email(email: str) -> BNLPerson: url = f"{base_url}/api/BNLPeople?email={email}" person = await _call_bnlpeople_webservice(url) if len(person) == 0: diff --git a/src/nsls2api/services/proposal_service.py b/src/nsls2api/services/proposal_service.py index daddd57a..8ef0b47d 100644 --- a/src/nsls2api/services/proposal_service.py +++ b/src/nsls2api/services/proposal_service.py @@ -810,7 +810,8 @@ async def generate_fake_test_proposal( is_pi=True, ) user_list.append(user) - except (LookupError, ValueError): + except (LookupError, ValueError) as e: + logger.error(f"Could not find user {add_specific_user} in BNLPeople: {e}") return None fake_proposal_id = await generate_fake_proposal_id() From c63be9b5abad35bb03fb965e632276056baed70b Mon Sep 17 00:00:00 2001 From: HarikaBishai Date: Thu, 27 Aug 2026 12:19:15 -0400 Subject: [PATCH 5/8] added blank line --- src/nsls2api/tests/api/test_user_api.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/nsls2api/tests/api/test_user_api.py b/src/nsls2api/tests/api/test_user_api.py index 78df11e6..90297b03 100644 --- a/src/nsls2api/tests/api/test_user_api.py +++ b/src/nsls2api/tests/api/test_user_api.py @@ -4,6 +4,7 @@ from nsls2api.main import app + @pytest.mark.anyio async def test_get_person_by_username_not_found(): """Test that requesting a non-existent username returns 404.""" From dce41d289e9eae2099a7f1e8cc5e7b38dc0e1165 Mon Sep 17 00:00:00 2001 From: HarikaBishai Date: Fri, 28 Aug 2026 14:02:49 -0400 Subject: [PATCH 6/8] changed 404 to 500 for too many matches --- src/nsls2api/api/v1/user_api.py | 25 ++++++++++++++++------ src/nsls2api/services/bnlpeople_service.py | 13 +++++++++-- src/nsls2api/services/person_service.py | 3 ++- src/nsls2api/services/proposal_service.py | 4 ++-- src/nsls2api/tests/api/test_user_api.py | 12 +++++------ 5 files changed, 40 insertions(+), 17 deletions(-) diff --git a/src/nsls2api/api/v1/user_api.py b/src/nsls2api/api/v1/user_api.py index 292460ed..82dc3d1f 100644 --- a/src/nsls2api/api/v1/user_api.py +++ b/src/nsls2api/api/v1/user_api.py @@ -9,6 +9,7 @@ bnlpeople_service, person_service, ) +from nsls2api.services.bnlpeople_service import AmbiguousPersonLookupError from nsls2api.services.ldap_service import get_user_info, shape_ldap_response router = fastapi.APIRouter() @@ -19,9 +20,15 @@ async def get_person_from_username(username: str): try: bnl_person = await bnlpeople_service.get_person_by_username(username) except LookupError as e: - raise HTTPException(status_code=404, detail=str(e)) - except ValueError as e: - raise HTTPException(status_code=400, detail=str(e)) + raise HTTPException( + status_code=404, + detail=str(e), + ) from e + except AmbiguousPersonLookupError as e: + raise HTTPException( + status_code=500, + detail=str(e), + ) from e person = Person( firstname=bnl_person.FirstName, @@ -47,9 +54,15 @@ async def get_person_from_email(email: str): try: bnl_person = await bnlpeople_service.get_person_by_email(email) except LookupError as e: - raise HTTPException(status_code=404, detail=str(e)) - except ValueError as e: - raise HTTPException(status_code=400, detail=str(e)) + raise HTTPException( + status_code=404, + detail=str(e), + ) from e + except AmbiguousPersonLookupError as e: + raise HTTPException( + status_code=500, + detail=str(e), + ) from e person = Person( firstname=bnl_person.FirstName, diff --git a/src/nsls2api/services/bnlpeople_service.py b/src/nsls2api/services/bnlpeople_service.py index 55297596..0ac66ed9 100644 --- a/src/nsls2api/services/bnlpeople_service.py +++ b/src/nsls2api/services/bnlpeople_service.py @@ -9,6 +9,11 @@ base_url = "https://api.bnl.gov/BNLPeople" +class AmbiguousPersonLookupError(Exception): + """Raised when a person lookup returns multiple results (data integrity issue).""" + pass + + async def _call_bnlpeople_webservice(url: str): return await _call_async_webservice_with_client(url, client=httpx_client_wrapper()) @@ -31,7 +36,9 @@ async def get_person_by_username(username: str) -> BNLPerson: logger.error( f"BNL People API returned {len(person)} people for username '{username}' - ambiguous result" ) - raise ValueError(f"Multiple people found with username {username}.") + raise AmbiguousPersonLookupError( + "Internal server error: ambiguous person lookup" + ) return BNLPerson(**person[0]) @@ -90,7 +97,9 @@ async def get_person_by_email(email: str) -> BNLPerson: logger.error( f"BNL People API returned {len(person)} people for email '{email}' - ambiguous result" ) - raise ValueError(f"Multiple people found with email {email}. Query is ambiguous.") + raise AmbiguousPersonLookupError( + "Internal server error: ambiguous person lookup" + ) return BNLPerson(**person[0]) diff --git a/src/nsls2api/services/person_service.py b/src/nsls2api/services/person_service.py index 60258b6b..aa3cae15 100644 --- a/src/nsls2api/services/person_service.py +++ b/src/nsls2api/services/person_service.py @@ -13,6 +13,7 @@ n2sn_service, proposal_service, ) +from nsls2api.services.bnlpeople_service import AmbiguousPersonLookupError from nsls2api.services.pass_service import get_proposals_by_person @@ -37,7 +38,7 @@ async def diagnostic_details_by_username(username: str) -> Person | None: ) ad_groups = await n2sn_service.get_groups_by_username(username) proposals = await get_proposals_by_person(bnl_person.EmployeeNumber) - except (LookupError, ValueError) as error: + except (LookupError, AmbiguousPersonLookupError) as error: raise LookupError( f"Error obtaining diagnostic details for username of {username}" ) from error diff --git a/src/nsls2api/services/proposal_service.py b/src/nsls2api/services/proposal_service.py index 8ef0b47d..83ee428d 100644 --- a/src/nsls2api/services/proposal_service.py +++ b/src/nsls2api/services/proposal_service.py @@ -28,6 +28,7 @@ facility_service, pass_service, ) +from nsls2api.services.bnlpeople_service import AmbiguousPersonLookupError async def get_locked_proposals( @@ -810,8 +811,7 @@ async def generate_fake_test_proposal( is_pi=True, ) user_list.append(user) - except (LookupError, ValueError) as e: - logger.error(f"Could not find user {add_specific_user} in BNLPeople: {e}") + except (LookupError, AmbiguousPersonLookupError): return None fake_proposal_id = await generate_fake_proposal_id() diff --git a/src/nsls2api/tests/api/test_user_api.py b/src/nsls2api/tests/api/test_user_api.py index 90297b03..ae1ab793 100644 --- a/src/nsls2api/tests/api/test_user_api.py +++ b/src/nsls2api/tests/api/test_user_api.py @@ -26,7 +26,7 @@ async def test_get_person_by_username_not_found(): @pytest.mark.anyio async def test_get_person_by_username_multiple_found(): - """Test that multiple people with same username returns 400.""" + """Test that multiple people with same username returns 500.""" # Mock API response with 2 people mock_api_response = [ { @@ -67,10 +67,10 @@ async def test_get_person_by_username_multiple_found(): ) as ac: response = await ac.get("/v1/person/username/jdoe") - assert response.status_code == 400 + assert response.status_code == 500 response_json = response.json() assert "detail" in response_json - assert "Multiple people found with username jdoe" in response_json["detail"] + assert "Internal server error: ambiguous person lookup" in response_json["detail"] @pytest.mark.anyio @@ -137,7 +137,7 @@ async def test_get_person_by_email_not_found(): @pytest.mark.anyio async def test_get_person_by_email_multiple_found(): - """Test that multiple people with same email returns 400.""" + """Test that multiple people with same email returns 500.""" # Mock API response with 2 people mock_api_response = [ { @@ -178,10 +178,10 @@ async def test_get_person_by_email_multiple_found(): ) as ac: response = await ac.get("/v1/person/email/jane@example.com") - assert response.status_code == 400 + assert response.status_code == 500 response_json = response.json() assert "detail" in response_json - assert "Multiple people found with email jane@example.com" in response_json["detail"] + assert "Internal server error: ambiguous person lookup" in response_json["detail"] @pytest.mark.anyio From fa9a76c4e4fcac5ccbbf1ab441e5d5eaab76260b Mon Sep 17 00:00:00 2001 From: HarikaBishai Date: Mon, 31 Aug 2026 11:54:33 -0400 Subject: [PATCH 7/8] review comment changws --- src/nsls2api/api/v1/user_api.py | 18 ++++-------------- src/nsls2api/services/bnlpeople_service.py | 8 ++++---- src/nsls2api/tests/api/test_user_api.py | 22 ++++++++++------------ 3 files changed, 18 insertions(+), 30 deletions(-) diff --git a/src/nsls2api/api/v1/user_api.py b/src/nsls2api/api/v1/user_api.py index 82dc3d1f..7a28c32f 100644 --- a/src/nsls2api/api/v1/user_api.py +++ b/src/nsls2api/api/v1/user_api.py @@ -22,13 +22,8 @@ async def get_person_from_username(username: str): except LookupError as e: raise HTTPException( status_code=404, - detail=str(e), - ) from e - except AmbiguousPersonLookupError as e: - raise HTTPException( - status_code=500, - detail=str(e), - ) from e + detail=f"No person with username {username} was found.", + ) from None person = Person( firstname=bnl_person.FirstName, @@ -56,13 +51,8 @@ async def get_person_from_email(email: str): except LookupError as e: raise HTTPException( status_code=404, - detail=str(e), - ) from e - except AmbiguousPersonLookupError as e: - raise HTTPException( - status_code=500, - detail=str(e), - ) from e + detail=f"No person with email {email} was found.", + ) from None person = Person( firstname=bnl_person.FirstName, diff --git a/src/nsls2api/services/bnlpeople_service.py b/src/nsls2api/services/bnlpeople_service.py index 0ac66ed9..f5ee4e59 100644 --- a/src/nsls2api/services/bnlpeople_service.py +++ b/src/nsls2api/services/bnlpeople_service.py @@ -31,13 +31,13 @@ async def get_person_by_username(username: str) -> BNLPerson: logger.warning( f"BNL People API could not find a person with a username of '{username}'" ) - raise LookupError(f"No person with username {username} found.") + raise LookupError(f"BNL People could not find a person with a username of '{username}'") if len(person) > 1: logger.error( f"BNL People API returned {len(person)} people for username '{username}' - ambiguous result" ) raise AmbiguousPersonLookupError( - "Internal server error: ambiguous person lookup" + f"BNL People API returned {len(person)} people for username '{username}' - ambiguous result" ) return BNLPerson(**person[0]) @@ -92,13 +92,13 @@ async def get_person_by_email(email: str) -> BNLPerson: logger.warning( f"BNL People API could not find a person with an email of '{email}'" ) - raise LookupError(f"No person with email {email} found.") + raise LookupError(f"BNL People could not find a person with an email of '{email}'") if len(person) > 1: logger.error( f"BNL People API returned {len(person)} people for email '{email}' - ambiguous result" ) raise AmbiguousPersonLookupError( - "Internal server error: ambiguous person lookup" + f"BNL People API returned {len(person)} people for email '{email}' - ambiguous result" ) return BNLPerson(**person[0]) diff --git a/src/nsls2api/tests/api/test_user_api.py b/src/nsls2api/tests/api/test_user_api.py index ae1ab793..02c0ab58 100644 --- a/src/nsls2api/tests/api/test_user_api.py +++ b/src/nsls2api/tests/api/test_user_api.py @@ -21,12 +21,12 @@ async def test_get_person_by_username_not_found(): assert response.status_code == 404 response_json = response.json() assert "detail" in response_json - assert "No person with username nonexistent_user_xyz123 found." in response_json["detail"] + assert "No person with username nonexistent_user_xyz123 was found." in response_json["detail"] @pytest.mark.anyio async def test_get_person_by_username_multiple_found(): - """Test that multiple people with same username returns 500.""" + """Test that multiple people with same username returns 500 via exception handler.""" # Mock API response with 2 people mock_api_response = [ { @@ -63,14 +63,13 @@ async def test_get_person_by_username_multiple_found(): return_value=mock_api_response, ): async with AsyncClient( - transport=ASGITransport(app=app), base_url="http://test" + transport=ASGITransport(app=app, raise_app_exceptions=False), + base_url="http://test", ) as ac: response = await ac.get("/v1/person/username/jdoe") assert response.status_code == 500 - response_json = response.json() - assert "detail" in response_json - assert "Internal server error: ambiguous person lookup" in response_json["detail"] + assert response.text == "Internal Server Error" @pytest.mark.anyio @@ -132,12 +131,12 @@ async def test_get_person_by_email_not_found(): assert response.status_code == 404 response_json = response.json() assert "detail" in response_json - assert "No person with email nonexistent@example.com found." in response_json["detail"] + assert "No person with email nonexistent@example.com was found." in response_json["detail"] @pytest.mark.anyio async def test_get_person_by_email_multiple_found(): - """Test that multiple people with same email returns 500.""" + """Test that multiple people with same email returns 500 via exception handler.""" # Mock API response with 2 people mock_api_response = [ { @@ -174,14 +173,13 @@ async def test_get_person_by_email_multiple_found(): return_value=mock_api_response, ): async with AsyncClient( - transport=ASGITransport(app=app), base_url="http://test" + transport=ASGITransport(app=app, raise_app_exceptions=False), + base_url="http://test", ) as ac: response = await ac.get("/v1/person/email/jane@example.com") assert response.status_code == 500 - response_json = response.json() - assert "detail" in response_json - assert "Internal server error: ambiguous person lookup" in response_json["detail"] + assert response.text == "Internal Server Error" @pytest.mark.anyio From eae75a03ab3d3d9d513ee25c50bb39be13880b02 Mon Sep 17 00:00:00 2001 From: HarikaBishai Date: Mon, 31 Aug 2026 12:02:32 -0400 Subject: [PATCH 8/8] message changes --- src/nsls2api/services/bnlpeople_service.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/nsls2api/services/bnlpeople_service.py b/src/nsls2api/services/bnlpeople_service.py index f5ee4e59..a4d0ae6a 100644 --- a/src/nsls2api/services/bnlpeople_service.py +++ b/src/nsls2api/services/bnlpeople_service.py @@ -31,7 +31,7 @@ async def get_person_by_username(username: str) -> BNLPerson: logger.warning( f"BNL People API could not find a person with a username of '{username}'" ) - raise LookupError(f"BNL People could not find a person with a username of '{username}'") + raise LookupError(f"BNL People API could not find a person with a username of '{username}'") if len(person) > 1: logger.error( f"BNL People API returned {len(person)} people for username '{username}' - ambiguous result" @@ -92,7 +92,7 @@ async def get_person_by_email(email: str) -> BNLPerson: logger.warning( f"BNL People API could not find a person with an email of '{email}'" ) - raise LookupError(f"BNL People could not find a person with an email of '{email}'") + raise LookupError(f"BNL People API could not find a person with an email of '{email}'") if len(person) > 1: logger.error( f"BNL People API returned {len(person)} people for email '{email}' - ambiguous result"