fix(server): server error-registry sweep — route graphile middleware errors through the helper - #1565
Merged
Conversation
Contributor
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #1540, which introduced the canonical
respondWithGraphQLError(res, ConstructiveError)helper (over the@constructive-io/errorsregistry) and converted the auth/captcha middleware. That fixed the "blankGraphQL Error:" class of bug where a hand-built body carried no top-levelmessagefor clients that rendererrors.map(e => e.message).join('; ').This sweeps the remaining hand-rolled error responses on the GraphQL path — in
middleware/graphile.ts, the PostGraphile handler middleware. Its short-circuit failures answered with a bare string or a REST-shaped{ error: { code, message } }object, both of which a GraphQL client renders as an empty error. All three now go through the same helper:INTERNAL_FAILUREisinternal-class, and the catch branch keeps raw error text out of production responses (dev-only detail) — matching #1540's auth catch exactly. Per the GraphQL-over-HTTP convention the helper responds200(thehttp: 500hint travels inextensions), so these responses change from500to200with a proper{ errors: [{ message, extensions: { code, class, http } }] }body.Scope / call sites reviewed but intentionally left
The rest of the server's error responses are a deliberate REST/HTML transport layer, not GraphQL, so routing them through the GraphQL helper would change their externally-observable contract:
errors/api-errors.ts+middleware/error-handler.ts— the typedApiErrorhierarchy and top-level Express handler; emit{ error: { code, message } }JSON or HTML pages (404/50x) for REST clients.middleware/api.ts— HTML error pages for domain/API resolution failures.middleware/fn.ts— REST function-invocation API ({ error: '...' },{ invocationId }).middleware/observability/*,middleware/favicon.ts,middleware/flush.ts— plain-text / REST utility endpoints.No new registry codes were needed;
INTERNAL_FAILUREalready covers internal server errors.Testing
graphql/server:tsc --noEmitclean; full suite 123/123 (incl. existingerrors/__tests__/graphql-response.test.ts). The graphile middleware can't be imported under the repo's ts-jest config (grafast/graphqltype resolution — same reason the existinggraphile-device-tokentest reimplements its logic), so no new middleware unit test was added; the helper itself is covered by fix(server): route middleware GraphQL errors through the error registry #1540's tests.@constructive-io/graphql-serverdependency subtree successfully. Note: a fullpnpm buildcurrently fails in an unrelated package (pgpm/transform, missing@pgsql/scriptstypes) — pre-existing onmain, independent of this diff. A stalepnpm installwas required first to link recently-added workspace deps.Link to Devin session: https://app.devin.ai/sessions/e05316293f64436d8c9113443c1c7b76
Requested by: @pyramation