Skip to content

fix: return accurate error messages for API client errors - #141

Open
kawacukennedy wants to merge 2 commits into
FoggedLens:masterfrom
kawacukennedy:fix/api-error-responses
Open

fix: return accurate error messages for API client errors#141
kawacukennedy wants to merge 2 commits into
FoggedLens:masterfrom
kawacukennedy:fix/api-error-responses

Conversation

@kawacukennedy

Copy link
Copy Markdown

📋 Description

The API's global error handler in server.ts returned { error: 'Internal Server Error' } for every error, regardless of status. That means a 400 from request validation (e.g. /geocode called without the required query param) or a thrown client error was reported to the caller as a server fault — misleading, and it hides the real cause from the frontend and from anyone debugging against the API.

This PR:

  1. Fixes the error handler so 4xx responses surface the actual error message, while 5xx responses stay generic (Internal Server Error) to avoid leaking internals.
  2. Extracts app construction into api/app.ts (buildApp()), the idiomatic Fastify pattern, so the server can be exercised via server.inject() without binding a port. server.ts is now a thin entrypoint and behaves identically (verified live).
  3. Adds the API's first route-level tests (api/app.test.ts).
  4. Runs the API test suite in CI before deploy, so the existing tests and these new ones actually gate releases.

🎯 Type of Change

  • 🐛 Bug fix

🧪 Testing

Route-level tests using app.inject():

  • /healthcheck (HEAD) → 200
  • Unknown route → 404
  • /geocode without query → 400 with a useful message (was "Internal Server Error")
  • /geocode?query=90210 → 200 from the local ZIP dataset, no Nominatim call (stubbed fetch asserts it's never hit)
  • /geocode/multi?query=10001 → 200 array
  • Upstream Nominatim failure → 500 Internal Server Error
  • Thrown 400 → message surfaced; thrown 500 → generic body (no internals leaked)
$ bun test
13 pass / 0 fail

Also verified against a live server (bun server.ts): healthcheck, ZIP geocode, 400 validation message, and 404 body all respond correctly.

  • Unit tests added/updated
  • All tests pass
  • Manual testing performed

✅ Checklist

  • I have read the project's README and CONTRIBUTING guidelines
  • My code follows TypeScript best practices
  • I have added tests that prove my fix works
  • PR is focused on a single logical change

🔍 Additional Notes

Behavior is otherwise unchanged — all routes, schemas, CORS, caching headers, telemetry, and the healthcheck route are identical to before, just relocated into buildApp(). This also makes the API easier to test for future contributors (e.g. new routes in #138 can be covered with the same inject() pattern).

The global error handler always sent { error: 'Internal Server Error' },
even for 4xx statuses. A validation failure on /geocode (missing query),
a rejected contact form body, or any thrown 400 was reported to the client
as a server fault, which is misleading and hides the real cause.

- Client errors (4xx) now surface the error message; 5xx stays generic to
  avoid leaking internals.
- Extract app construction into buildApp() in app.ts so the server can be
  exercised via inject() without binding a port.
- Add route-level tests covering healthcheck, 404s, validation errors,
  the local ZIP-code short circuit, upstream failures, and the 4xx/5xx
  error-handler split.
- Run the API test suite in CI before deploying.

@anthony-maio anthony-maio left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review on #143 — happy to return the favor. I verified this locally: fetched the branch, bun install, bun test → 13 pass / 0 fail on bun 1.4.0.

The server.tsapp.ts extraction is faithful — routes, schemas, CORS, hooks, and telemetry all moved verbatim. The only behavior change is the intended one in the error handler.

I specifically checked the leak concern: all four service clients (Nominatim, Zammad, Turnstile, Github) throw plain Errors without a statusCode, so upstream failures still surface as generic 500s. Only Fastify validation errors and explicitly-set 4xx statuses get their message passed through. Good.

One non-blocking suggestion: deflock-api-deploy.yml only triggers on push to master, so these tests gate deploys but never run on PRs. Consider adding a pull_request trigger with the same api/** path filter (or a separate CI workflow) so failures show up during review instead of at deploy time.

The inject() pattern here is a nice template for covering the new routes in #138 too.

Previously the test job only ran on push to master, so failures surfaced
only at deploy time. Run the API test suite on pull_request (path-filtered
to api/**) so regressions show up during review.

The deploy job now depends on test and is gated to push-to-master only,
so fork PRs (which lack deploy secrets) run tests without attempting to
deploy unreviewed code.

Addresses review feedback on FoggedLens#141.
@kawacukennedy

Copy link
Copy Markdown
Author

Good suggestion — you're right that the test job only gating on push meant failures surfaced at deploy time, not review time. I've implemented it in 38bf860.

Changes to deflock-api-deploy.yml:

  • Added a pull_request trigger (path-filtered to api/**), so bun test now runs during review.
  • Split the single job into test and deploy. The deploy job needs: test and is gated with if: github.event_name == 'push' && github.ref == 'refs/heads/master' — so fork PRs (which don't have the deploy secrets) run the test suite but never attempt to deploy unreviewed code.

Also reflected your note about #138 — the inject() pattern in app.test.ts is there precisely to make adding route coverage for /geocode/reverse and /officials straightforward. Happy to help cover those if useful.

Thanks again for the detailed review — the leak-check on the four service clients validating that upstream failures stay generic 500s is exactly the kind of confirmation that's valuable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants