Fix: Return 404 instead of 500 when agency does not exist#774
Fix: Return 404 instead of 500 when agency does not exist#774andyosyndoh wants to merge 1 commit intoOneBusAway:mainfrom
Conversation
Handle sql.ErrNoRows explicitly in GetAgency calls across multiple handlers. When an agency is not found, return 404 Not Found instead of 500 Internal Server Error to improve REST API correctness. Changes: - Added sql.ErrNoRows check in arrival_and_departure_for_stop_handler - Added sql.ErrNoRows check in arrivals_and_departure_for_stop - Added sql.ErrNoRows check in trip_for_vehicle_handler - Added sql.ErrNoRows check in trip_details_handler - Added missing imports (database/sql, errors) where needed Impact: - Improves REST API correctness for missing resources - Avoids misleading server error responses - Aligns with existing test expectations
|
andyosyndoh seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
aaronbrethorst
left a comment
There was a problem hiding this comment.
PR #774 Review: Return 404 instead of 500 when agency does not exist
Verdict: Request Changes
Andy, the fix itself is correct and consistent — same sql.ErrNoRows → 404 pattern applied across all four GetAgency call sites. Two things need addressing before this can merge.
Critical Issues (1 found)
1. CLA not signed
The CLA assistant reports the contributor hasn't signed the CLA yet. This is a prerequisite for merging any contribution to the OneBusAway project.
Important Issues (1 found)
1. Conflict with PR #770 on arrival_and_departure_for_stop_handler.go
PR #770 makes the exact same change to arrival_and_departure_for_stop_handler.go:172-180. Whichever PR merges first will cause a merge conflict for the other. Worth coordinating — if #770 merges first, this PR just needs to drop that file's changes and rebase.
Suggestions (1 found)
1. Consider adding tests for the new 404 paths
The fix is mechanical and correct, but there are no tests covering the new behavior. PR #770 includes a test for the arrival_and_departure_for_stop_handler case (TestArrivalAndDepartureForStopHandler_AgencyNotFound). Consider adding similar tests for the other three handlers (arrivals_and_departure_for_stop, trip_details_handler, trip_for_vehicle_handler) to prevent regression. Even one test covering the pattern would be valuable.
Strengths
- Comprehensive: Fixes all four
GetAgencycall sites in one PR, not just one - Consistent pattern: Every fix follows the same structure —
errors.Is(err, sql.ErrNoRows)→sendNotFound, elseserverErrorResponse - Proper imports:
database/sqlanderrorsimports added where needed - Lint clean: 0 issues
Recommended Action
- Sign the CLA
- Coordinate with PR #770 to avoid merge conflict
- Consider adding tests
Handle sql.ErrNoRows explicitly in GetAgency calls across multiple handlers. When an agency is not found, return 404 Not Found instead of 500 Internal Server Error to improve REST API correctness.
Changes:
Impact:
Fixes #769