Fix/lookup error 404

[Harika Bishai (HarikaBishai)]

This pull request improves the error handling and robustness of the user lookup API endpoints for username and email, ensures consistent API responses, and adds comprehensive tests for these scenarios. The changes also improve logging for ambiguous or failed lookups and clean up some debugging output.

API error handling and response consistency:

• Refactored the /person/username/{username} and /person/email/{email} endpoints in user_api.py to raise appropriate HTTP exceptions (404 for not found, 400 for ambiguous/multiple results) instead of returning custom JSON responses. [1] [2] [3]
• Updated the corresponding service methods in bnlpeople_service.py (get_person_by_username, get_person_by_email) to raise LookupError or ValueError with detailed log messages when no or multiple results are found. [1] [2]

Testing improvements:

• Added a comprehensive test suite (test_user_api.py) for the username and email endpoints, covering not found, multiple results, and successful lookup cases.

Logging and code cleanup:

• Improved log messages for ambiguous or failed lookups in bnlpeople_service.py for easier debugging. [1] [2]
• Removed extraneous debug print statements from person_service.py and improved exception handling to catch both LookupError and ValueError.
• Updated error handling in proposal_service.py to log both LookupError and ValueError when users can't be found.

Pytest Summary:

[Copilot]

Pull request overview

This pull request updates the FastAPI v1 user-identification endpoints to translate BNLPeople lookup failures into consistent HTTP 404 responses, improving client-facing error handling for username- and email-based person lookups.

Changes:

• Refactors /person/username/{username} to handle LookupError via HTTPException(404, detail=...) instead of returning a custom JSONResponse.
• Refactors /person/email/{email} similarly, and corrects the error messaging to be email-specific.
• Removes debug output and simplifies the success-path response construction.

Suppressed comments (1)

src/nsls2api/api/v1/user_api.py:48

• Same issue as the username handler: bnlpeople_service raises LookupError for both 0 and >1 matches, but this always returns “No people … found.” and drops the more specific LookupError detail. Using the exception message avoids misleading clients when multiple matches exist.

    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.")

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Suppressed comments (1)

src/nsls2api/services/bnlpeople_service.py:84

• get_person_by_email now always either returns a BNLPerson or raises (LookupError/ValueError), so the | None return type is misleading for callers and type-checkers.

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:

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

src/nsls2api/tests/api/test_user_api.py:8

• Match existing test-file formatting (and Black/PEP 8) by keeping two blank lines between the import block and the first top-level test function/decorator.

from nsls2api.main import app

@pytest.mark.anyio
async def test_get_person_by_username_not_found():

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (3)

Previously missed (2) — in code that hasn't changed since the last review.

src/nsls2api/services/bnlpeople_service.py:55

• In get_username_by_id, the warning message says the API “could not find” a person even when len(person) > 1 (ambiguous/multiple matches). This makes logs misleading during debugging and hides the ambiguity case.

This issue also appears on line 74 of the same file.

    if len(person) == 0 or len(person) > 1:
        logger.warning(
            f"BNL People API could not find a person with an employee/life number of '{lifenumber}'"
        )
        return None

src/nsls2api/services/person_service.py:43

• diagnostic_details_by_username now catches ValueError (e.g., ambiguous username) but re-raises it as LookupError, which loses the distinction between “not found” and “ambiguous” for any callers that want to map these to different responses. Consider re-raising ValueError as ValueError so upstream code can still produce a 400 when appropriate.

    except (LookupError, ValueError) as error:
        raise LookupError(
            f"Error obtaining diagnostic details for username of {username}"
        ) from error

src/nsls2api/services/bnlpeople_service.py:77

• In get_person_by_id, the raised LookupError message says “could not find” even when len(person) > 1. Since this branch also covers ambiguous results, the message should reflect “not found or ambiguous” (or split the cases) to avoid inaccurate error details being surfaced to callers.

    if len(person) == 0 or len(person) > 1:
        raise LookupError(
            f"BNL People API could not find a person with an employee/life number of '{lifenumber}'"
        )

[Padraic Shafer (padraic-shafer)]

This PR introduces some design changes with implications to consider. I'll move this to draft while we review the PR.

[Padraic Shafer (padraic-shafer)]

I'm not sure that I agree with the decision to use 400 response and python ValueError to indicate a response that we were not hoping for. Both 400 and ValueError indicate problems with the function input, not the output.

Probably 404 and LookupError are the closest appropriate responses here. 502 might apply, but that's arguable and even more ambiguous.

The logging should certainly indicate the reason for the 404 -- not found / too many matches -- but this is info that we may not want propagate in the HTTP response.

[Harika Bishai (HarikaBishai)]

I'm not sure that I agree with the decision to use 400 response and python ValueError to indicate a response that we were not hoping for. Both 400 and ValueError indicate problems with the function input, not the output.

Probably 404 and LookupError are the closest appropriate responses here. 502 might apply, but that's arguable and even more ambiguous.

The logging should certainly indicate the reason for the 404 -- not found / too many matches -- but this is info that we may not want propagate in the HTTP response.

Should I send 404, with message Multiple people found with username {username} for too many matches instead of 400

[Dan Allan (danielballan)]

Under what situation would we expect to the backend to find multiple people for one username?

I would say either:

• This should never happen.
• If it does happen, it's a 500 because there is nothing the client can do. Waiting won't help; reformulating the request won't help. We should log the issue so it can be debugged.

[Padraic Shafer (padraic-shafer)]

Under what situation would we expect to the backend to find multiple people for one username?

I would say either:

• This should never happen.
• If it does happen, it's a 500 because there is nothing the client can do. Waiting won't help; reformulating the request won't help. We should log the issue so it can be debugged.

That seems reasonable to me

[Harika Bishai (HarikaBishai)]

Under what situation would we expect to the backend to find multiple people for one username?

I would say either:

• This should never happen.
• If it does happen, it's a 500 because there is nothing the client can do. Waiting won't help; reformulating the request won't help. We should log the issue so it can be debugged.

I have updated the code , with 500 error on too many matches, take a look.

[Copilot]

🟡 Changes recommended

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (3)

src/nsls2api/api/v1/user_api.py:52

• AmbiguousPersonLookupError from the service (multiple results) is not handled here, so ambiguous email lookups will currently produce a 500 rather than a 400.

    try:
        bnl_person = await bnlpeople_service.get_person_by_email(email)
    except LookupError as e:
        raise HTTPException(

src/nsls2api/tests/api/test_user_api.py:182

• These assertions expect a plain-text 500 response, but ambiguous email lookups should be treated as a 400 with a JSON body (consistent with the intended API behavior).

    assert response.status_code == 500
    assert response.text == "Internal Server Error"

src/nsls2api/services/proposal_service.py:815

• This except block now silently returns None without logging the underlying error. That makes failures to resolve add_specific_user hard to diagnose (and contradicts the PR description that mentions improved logging for failed/ambiguous lookups).

        except (LookupError, AmbiguousPersonLookupError):
            return None

• Files reviewed: 5/5 changed files
• Comments generated: 2
• Review effort level: Lite