Fix/lookup error 404

src/nsls2api/api/v1/user_api.py

@@ -16,52 +16,60 @@
 
 @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."},
+    try:
+        bnl_person = await bnlpeople_service.get_person_by_username(username)
+    except LookupError:
+        raise HTTPException(
             status_code=404,
-        )
+            detail=f"Person with username '{username}' was not found.",
+        ) from None
+
+    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}")
+@router.get("/person/email/{email}", response_model=Person)
 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."},
+    try:
+        bnl_person = await bnlpeople_service.get_person_by_email(email)
+    except LookupError:
+        raise HTTPException(
             status_code=404,
-        )
+            detail=f"Person with email '{email}' was not found.",
+        ) from None
+
+    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
 
 
 # TODO: Add back into schema if we decide to use this endpoint.

src/nsls2api/services/bnlpeople_service.py

@@ -9,6 +9,20 @@
 base_url = "https://api.bnl.gov/BNLPeople"
 
 
+class AmbiguousPersonLookupError(Exception):
+    """Raised when a person lookup returns multiple results (data integrity issue).
+
+    ``AmbiguousPersonLookupError`` does not derive from ``LookupError``.
+    ``LookupError`` represents an expected condition and is generally converted
+    into a 404 NOT FOUND response. An ambiguous result indicates a data
+    integrity issue with the upstream service and should generally propagate
+    to the global exception handler as a 500 INTERNAL SERVER ERROR or
+    502 BAD GATEWAY response.
+    """
+
+    pass
+
+
 async def _call_bnlpeople_webservice(url: str):
     return await _call_async_webservice_with_client(url, client=httpx_client_wrapper())
 
@@ -19,65 +33,114 @@ 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 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"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"
+        )
+        raise AmbiguousPersonLookupError(
+            f"BNL People API returned {len(person)} people for username '{username}' - ambiguous result"
         )
     return BNLPerson(**person[0])
 
 
-async def get_username_by_id(lifenumber: str) -> str | None:
+async def get_username_by_id(lifenumber: str | None) -> str | None:
     if lifenumber is None:
         return None
 
     url = f"{base_url}/api/BNLPeople?employeeNumber={lifenumber}"
     logger.debug(f"Calling URL: {url}")
+
     try:
         person = await _call_bnlpeople_webservice(url)
     except Exception:
-        message = f"BNL People API query failed for lifenumber {lifenumber}"
-        logger.exception(message)
+        logger.exception(
+            f"BNL People API query failed for lifenumber {lifenumber}"
+        )
         return None
-    # logger.debug(person)
-    if len(person) == 0 or len(person) > 1:
+
+    if len(person) == 0:
         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}'"
+        )
+        raise LookupError(
+            f"BNL People API could not find a person with an employee/life number of '{lifenumber}'"
+        )
+
+    if len(person) > 1:
+        logger.error(
+            f"BNL People API returned {len(person)} people for employee/life number '{lifenumber}' - ambiguous result"
+        )
+        raise AmbiguousPersonLookupError(
+            f"BNL People API returned {len(person)} people for employee/life number '{lifenumber}' - ambiguous result"
         )
-        return None
 
-    # Let's check that the response validates
     bnl_person = BNLPerson(**person[0])
 
     # Guard against the BNLPeople API giving us an empty string.
     if len(bnl_person.ActiveDirectoryName) > 0:
         return bnl_person.ActiveDirectoryName
-    else:
-        return None
+
+    return None
 
 
 async def get_person_by_id(lifenumber: str) -> BNLPerson | None:
     if lifenumber is None:
         return None
 
     url = f"{base_url}/api/BNLPeople?employeeNumber={lifenumber}"
-    person = await _call_bnlpeople_webservice(url)
+    logger.debug(f"Calling URL: {url}")
 
-    if len(person) == 0 or len(person) > 1:
+    try:
+        person = await _call_bnlpeople_webservice(url)
+    except Exception:
+        logger.exception(
+            f"BNL People API query failed for lifenumber {lifenumber}"
+        )
+        return None
+
+    if len(person) == 0:
+        logger.warning(
+            f"BNL People API could not find a person with an employee/life number of '{lifenumber}'"
+        )
         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}'"
+        )
+
+    if len(person) > 1:
+        logger.error(
+            f"BNL People API returned {len(person)} people for employee/life number "
+            f"'{lifenumber}' - ambiguous result"
         )
+        raise AmbiguousPersonLookupError(
+            f"BNL People API returned {len(person)} people for employee/life number "
+            f"'{lifenumber}' - ambiguous result"
+        )
+
     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 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"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"
+        )
+        raise AmbiguousPersonLookupError(
+            f"BNL People API returned {len(person)} people for email '{email}' - ambiguous result"
         )
     return BNLPerson(**person[0])
 
@@ -89,7 +152,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

src/nsls2api/services/person_service.py

@@ -6,13 +6,15 @@
     Person,
     PersonSummary,
 )
+from nsls2api.infrastructure.logging import logger
 from nsls2api.services import (
     beamline_service,
     bnlpeople_service,
     facility_service,
     n2sn_service,
     proposal_service,
 )
+from nsls2api.services.bnlpeople_service import AmbiguousPersonLookupError
 from nsls2api.services.pass_service import get_proposals_by_person
 
 
@@ -37,22 +39,16 @@ 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, AmbiguousPersonLookupError) as error:
         raise LookupError(
-            f"Error obtaining diagnostic details for username of {username}"
+            f"Error obtaining diagnostic details for username '{username}'"
         ) from error
 
-    print(bnl_person)
-    print("-------")
+    logger.debug(f"bnl_person: {bnl_person}")
+    logger.debug(f"ad_person: {ad_person}")
+    logger.debug(f"ad_groups: {ad_groups}")
+    logger.debug(f"proposals: {proposals}")
 
-    print(ad_person)
-    print("-------")
-
-    print(ad_groups)
-    print("-------")
-
-    print(proposals)
-    print("-------")
 
     person = Person(
         firstname=bnl_person.FirstName,

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,10 @@ 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 (AmbiguousPersonLookupError, LookupError):
+            logger.error(
+                f"Could not resolve user '{add_specific_user}' in BNLPeople to add to fake test proposal."
+            )
             return None
 
     fake_proposal_id = await generate_fake_proposal_id()

src/nsls2api/services/sync_service.py

@@ -2,7 +2,6 @@
 
 from beanie import UpdateResponse
 from beanie.operators import AddToSet, Set
-from httpx import HTTPStatusError
 
 from nsls2api.api.models.facility_model import FacilityName
 from nsls2api.api.models.person_model import ActiveDirectoryUser
@@ -26,6 +25,7 @@
     pass_service,
     proposal_service,
 )
+from nsls2api.services.bnlpeople_service import AmbiguousPersonLookupError
 
 
 async def worker_synchronize_dataadmins(skip_beamlines=False) -> None:
@@ -292,10 +292,10 @@ async def synchronize_proposal_from_pass(
             )
             bnl_username = await bnlpeople_service.get_username_by_id(user.BNL_ID)
             logger.debug(f"     ---> {bnl_username}")
-        except HTTPStatusError as error:
-            logger.error(f"Could not find BNL username for BNL ID '{user.BNL_ID}'.")
-            logger.error(f"BNL People API returned: {error}")
-            bnl_username = None
+        except (AmbiguousPersonLookupError, LookupError):
+            logger.error(
+                f"Error obtaining username for BNL ID '{user.BNL_ID}'"
+            )
 
         userinfo = User(
             first_name=user.First_Name,
@@ -311,9 +311,15 @@ async def synchronize_proposal_from_pass(
     # Let's add the PI explicitly anyway as PASS sometimes includes the PI in the
     # Experimenters list and sometimes not.
     if pass_proposal.PI and not pi_found_in_experimenters:
-        bnl_username = await bnlpeople_service.get_username_by_id(
-            pass_proposal.PI.BNL_ID
-        )
+        bnl_username = None
+        try:
+            bnl_username = await bnlpeople_service.get_username_by_id(
+                pass_proposal.PI.BNL_ID
+            )
+        except (AmbiguousPersonLookupError, LookupError):
+            logger.error(
+                f"Error obtaining username for PI with BNL ID '{pass_proposal.PI.BNL_ID}'"
+            )
         pi_info = User(
             first_name=pass_proposal.PI.First_Name,
             last_name=pass_proposal.PI.Last_Name,