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/exception_handlers.py

@@ -1,19 +1,21 @@
 from asgi_correlation_id import correlation_id
-from fastapi import HTTPException, Request
+from fastapi import FastAPI, HTTPException, Request
 from fastapi.exception_handlers import http_exception_handler
 
-from nsls2api.main import app
 
-
-# This is to make sure we add the request ID to the response headers for the case
-# of unhandled server errors.
-@app.exception_handler(Exception)
 async def unhandled_exception_handler(request: Request, exc: Exception):
+    """Add the correlation ID to responses for unhandled server errors."""
     return await http_exception_handler(
         request,
         HTTPException(
             500,
-            "Internal server error - unhandled exception",
+            "Internal server error",
             headers={"X-Request-ID": correlation_id.get() or ""},
         ),
     )
+
+
+def register_exception_handlers(app: FastAPI):
+    """Register all exception handlers for the app."""
+    # Generic Exception must be listed last.
+    app.add_exception_handler(Exception, unhandled_exception_handler)

src/nsls2api/main.py

@@ -17,6 +17,7 @@
 from nsls2api.api.v1 import proposal_api as proposal_api_v1
 from nsls2api.api.v1 import stats_api as stats_api_v1
 from nsls2api.api.v1 import user_api as user_api_v1
+from nsls2api.exception_handlers import register_exception_handlers
 from nsls2api.infrastructure import app_setup
 from nsls2api.infrastructure.config import get_settings
 from nsls2api.infrastructure.logging import logger
@@ -49,6 +50,7 @@
 app = fastapi.FastAPI(
     title="NSLS-II API", middleware=middleware, lifespan=app_setup.app_lifespan
 )
+register_exception_handlers(app)
 
 # Instrument the app and expose the /metrics endpoint
 # (this is equivalent to calling instrumentator.instrument(app)
@@ -66,7 +68,6 @@
     expose_headers=["X-Request-ID"],
 )
 
-
 def configure_routing():
     app.include_router(proposal_api_v1.router, prefix="/v1", tags=["proposal"])
     app.include_router(stats_api_v1.router, prefix="/v1")

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()