fix(routes): return 500 on DB failure instead of 404 during route lookup

[3rabiii]

Description

Previously, the routeHandler treated all errors returned by GetRoute as a 404 Not Found. This was actively swallowing underlying database or connection errors and disguising them as missing records, making it difficult to debug actual system failures in production.

This PR updates the handler to properly distinguish between a missing record and a genuine internal server error.

Changes Made:

• Error Handling (route_handler.go): * Refactored the error checking logic using errors.Is(err, sql.ErrNoRows).
  • sql.ErrNoRows continues to correctly return a 404 Not Found.
  • Any other database or connection error now properly triggers a 500 Internal Server Error.
  • Preserved the route.ID == "" fallback check as an additional safety net.

Tests:

• Added TestRouteHandler_DatabaseError: Created a new integration test to verify the 500 response behavior.

fixes: #1009

Summary by CodeRabbit

• Bug Fixes

  • Improved error handling for route lookups to correctly distinguish missing routes from database errors, ensuring appropriate HTTP status codes are returned.

• Tests

  • Added tests simulating database failures for route retrieval to verify the API returns proper error responses and status codes.

[coderabbitai]

📝 Walkthrough

Walkthrough

The handler now distinguishes sql.ErrNoRows (returns 404) from other DB errors (returns 500) and separately checks for empty route.ID; a new test forces a DB failure and asserts an HTTP 500 response.

Changes

Route Lookup Error Handling

Layer / File(s)	Summary
Error handling and request path internal/restapi/route_handler.go	Refactor GetRoute handling: on error, map sql.ErrNoRows → 404, other errors → 500 and return early; then independently check route.ID == "" → 404.
Test: database failure scenario internal/restapi/route_handler_test.go	Add TestRouteHandler_DatabaseError: initialize GTFS manager and app, close GtfsDB to simulate DB failure, call route endpoint, and assert HTTP 500 and model.Code == http.StatusInternalServerError.

Sequence Diagram

sequenceDiagram
  participant Client
  participant routeHandler
  participant GetRoute
  participant Database
  Client->>routeHandler: GET /route
  routeHandler->>GetRoute: fetch route
  GetRoute->>Database: query
  alt sql.ErrNoRows
    Database-->>GetRoute: sql.ErrNoRows
    GetRoute-->>routeHandler: error
    routeHandler-->>Client: 404 Not Found
  else other DB error
    Database-->>GetRoute: other error
    GetRoute-->>routeHandler: error
    routeHandler-->>Client: 500 Internal Server Error
  else empty route.ID
    GetRoute-->>routeHandler: route.ID == ""
    routeHandler-->>Client: 404 Not Found
  else success
    Database-->>GetRoute: route found
    GetRoute-->>routeHandler: route object
    routeHandler-->>Client: 200 OK with route data
  end

Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Suggested reviewers

• burma-shave
• aaronbrethorst

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main fix: distinguishing between database failures (500) and missing routes (404) in the route lookup handler.
Linked Issues check	✅ Passed	The PR fully addresses issue #1009 requirements: implements error distinction using sql.ErrNoRows for 404 responses, handles other errors as 500, preserves empty ID check, and adds TestRouteHandler_DatabaseError integration test.
Out of Scope Changes check	✅ Passed	All changes directly support the stated objectives: route handler error handling logic fixes and test coverage for database failure scenarios. No extraneous modifications detected.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[coderabbitai]

🧹 Nitpick comments (1)

internal/restapi/route_handler_test.go (1)

116-146: ⚡ Quick win

Consider adding test coverage for GetAgency database errors.

TestRouteHandler_DatabaseError correctly verifies that database failures during GetRoute return HTTP 500. However, the handler now also calls GetAgency when includeReferences=true (the default), and that call has the same error handling pattern.

For completeness, consider adding a similar test that verifies the GetAgency error path also returns 500 on database failures (e.g., close the DB after the route is loaded but before the agency lookup, or use a route with a valid ID but request includeReferences=true with a closed DB).

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/restapi/route_handler_test.go` around lines 116 - 146, Add a new
unit test similar to TestRouteHandler_DatabaseError that specifically exercises
the GetAgency database error path when includeReferences=true: create the same
setup (internalgtfs.InitGTFSManager, NewRestAPI), ensure a valid route ID is
used, then close manager.GtfsDB after the route is available but before the
agency lookup (or call the handler with includeReferences=true and a closed DB)
and use callAPIHandler[RouteEntryResponse] (and routeURL) to invoke the
endpoint; assert resp.StatusCode and model.Code are
http.StatusInternalServerError to verify GetAgency errors propagate a 500.
Ensure the test name and assertions mirror TestRouteHandler_DatabaseError (e.g.,
TestRouteHandler_AgencyDatabaseError) so it covers the agency error path invoked
by GetAgency when includeReferences=true.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@internal/restapi/route_handler_test.go`:
- Around line 116-146: Add a new unit test similar to
TestRouteHandler_DatabaseError that specifically exercises the GetAgency
database error path when includeReferences=true: create the same setup
(internalgtfs.InitGTFSManager, NewRestAPI), ensure a valid route ID is used,
then close manager.GtfsDB after the route is available but before the agency
lookup (or call the handler with includeReferences=true and a closed DB) and use
callAPIHandler[RouteEntryResponse] (and routeURL) to invoke the endpoint; assert
resp.StatusCode and model.Code are http.StatusInternalServerError to verify
GetAgency errors propagate a 500. Ensure the test name and assertions mirror
TestRouteHandler_DatabaseError (e.g., TestRouteHandler_AgencyDatabaseError) so
it covers the agency error path invoked by GetAgency when
includeReferences=true.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 4e365ad6-f594-49e1-87fb-43c4b86b8ac6

📥 Commits

Reviewing files that changed from the base of the PR and between ecf059f and 800188c.

📒 Files selected for processing (2)

• internal/restapi/route_handler.go
• internal/restapi/route_handler_test.go