feat(graphql): wire enriched error catalogue into GraphQL responses#9342
feat(graphql): wire enriched error catalogue into GraphQL responses#9342ogenstad wants to merge 15 commits into
Conversation
There was a problem hiding this comment.
3 issues found
Confidence score: 4/5
- This PR looks safe to merge overall, but test reliability is weakened by an assertion gap in
backend/tests/functional/api/test_exception_handlers.pywhere anif response.status_code == 401check can let failures pass silently. - The most severe issue is in the exception-handler tests: the expired-JWT case appears non-deterministic and may not actually exercise the
TOKEN_EXPIREDpath, which reduces confidence in regression detection for auth error handling. backend/tests/functional/graphql/test_error_catalogue.pycurrently has a weak assertion pattern that may pass on a generic validation error without proving the intended per-field error catalogue behavior.- Pay close attention to
backend/tests/functional/api/test_exception_handlers.pyandbackend/tests/functional/graphql/test_error_catalogue.py- these tests may provide false confidence and miss real regressions in error handling paths.
Shadow auto-approve: would not auto-approve because issues were found.
Re-trigger cubic
b9a7462 to
7ebbdb4
Compare
There was a problem hiding this comment.
No issues found across 17 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Shadow auto-approve: would require human review. This PR adds a new error catalogue system to GraphQL responses, modifies core error handling logic, updates frontend error handling, and changes the structure of error extensions—a significant architectural change with broad impact that requires human review to ensure correctness and safety.
Re-trigger cubic
There was a problem hiding this comment.
0 issues found across 2 files (changes from recent commits).
Shadow auto-approve: would require human review. This PR introduces a breaking change to GraphQL error response format, touching core exception handling, validation logic, and frontend error handling across 1139 lines; such high-impact changes require human review to ensure correctness and avoid production issues.
Re-trigger cubic
65b23ac to
9d45e31
Compare
9d45e31 to
471b242
Compare
|
|
||
| @classmethod | ||
| async def mutate( | ||
| async def mutate( # noqa: PLR0915 |
There was a problem hiding this comment.
This method was already too complex and catching the new exception pushed it over the top to require the too-many-statements (PLR0915) noqa line. I didn't want to make larger changes then needed for this iteration, and catching ValidationError as we now do below is temporary. But I think we'll have to go back and simplify this method in general later.
There was a problem hiding this comment.
would it be better to add it to the pyproject.toml exclusions instead of inline?
Captures the design for refactoring frontend/app/src/shared/api/graphql/ graphqlClientApollo.tsx to use the new backend error catalogue (INFP-468). Introduces a typed ErrorCode / GraphQLErrorExtensions module, restores a switch-based errorLink with named helpers, and fixes the silent-failure on AUTHENTICATION_REQUIRED that exists on develop and on this branch today. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds dev/specs/infp-468-graphql-error-catalogue/frontend-errorlink-refactor.md covering the deeper consumer-side follow-up to T031: a hand-written catalogue mirror at frontend/app/src/shared/api/graphql/errors.ts, a typed switch-based errorLink, and the fix for the silent-failure on AUTHENTICATION_REQUIRED (shared with develop, finally distinguishable thanks to US1's split). Adds T031a–T031d under Phase 4 (US2) for the implementation tasks, and notes how this file is deleted cleanly when T029's generated bindings land. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…1a-d) Bite-sized TDD plan covering the three commits: hand-written errors.ts module, vitest tests with a compile-time exhaustiveness guard, and the switch-based errorLink refactor with extracted retryWithRefreshedToken / notifyUser helpers. Maps each step to an exact file path, command, and expected output. Source spec at dev/specs/infp-468-graphql-error-catalogue/ frontend-errorlink-refactor.md. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… T031a) Mirrors backend/infrahub/errors/catalogue.py until US2's generated bindings (T029) land. Adds ERROR_CODES, ErrorCode, GraphQLErrorExtensions discriminated union, and parseErrorExtensions narrowing helper that falls back to UNDEFINED_ERROR for unrecognised inputs. See spec at dev/specs/infp-468-graphql-error-catalogue/frontend-errorlink-refactor.md.
Covers per-code narrowing, http_status/data passthrough, UNDEFINED_ERROR fallback for unknown codes and malformed inputs, and a compile-time exhaustiveness guard that fails the build if ErrorCode gains a value without a corresponding switch arm.
…1c, T031d) Replaces the if-chain in errorLink with switch (parsed.code) over the typed GraphQLErrorExtensions returned by parseErrorExtensions. Extracts retryWithRefreshedToken and notifyUser as file-local helpers so the per- code policy is a one-screen read. Fixes a long-standing silent failure (shared with develop) where AUTHENTICATION_REQUIRED was routed through the refresh flow and produced no toast when refresh inevitably failed for bad credentials. With the catalogue split landed in US1, only TOKEN_EXPIRED triggers a refresh attempt; AUTHENTICATION_REQUIRED now surfaces via the default toast / processErrorMessage path. Also adds FetchResult to the @apollo/client import and a trailing return; to satisfy noImplicitReturns — the Parameters<...> helper type compiled without needing internal Apollo module imports. Design: dev/specs/infp-468-graphql-error-catalogue/frontend-errorlink-refactor.md Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
2 issues found across 6 files (changes from recent commits).
Shadow auto-approve: would not auto-approve because issues were found.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
0 issues found across 2 files (changes from recent commits).
Shadow auto-approve: would not auto-approve. Auto-approval blocked by 2 unresolved issues from previous reviews.
Re-trigger cubic
…n (INFP-468) retryWithRefreshedToken wrapped the refresh flow in a new Observable but only completed it on the happy path: if queryClient.fetchQuery resolved without an access_token, the if (newToken?.access_token) branch was skipped silently and observer.next/error/complete were never called, hanging the operation indefinitely. Invert the guard so that branch calls observer.error(...) with a descriptive message — Apollo surfaces this as a network error and the caller unblocks. Also drop the trailing bare forward(operation) at the bottom of the subscriber: Apollo link Observables are lazy and that call had no .subscribe(), so it was dead code from day one (pre-dates the catalogue refactor). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…468) The file-structure preamble claimed no files outside frontend/app/src/shared/api/graphql/ are touched, but Task 4 (steps 4.4-4.5) edits dev/specs/infp-468-graphql-error-catalogue/tasks.md to flip the T031a-T031d checkboxes to [X]. Narrow the claim to source and test files and call out the tasks.md checkbox-flip as the one exception. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
0 issues found across 2 files (changes from recent commits).
Shadow auto-approve: would require human review. This PR introduces a breaking change to GraphQL error responses, modifies core error handling infrastructure, and touches authentication, validation, and frontend integration—areas where a subtle bug could have severe consequences for production reliability and client compatibility.
Re-trigger cubic
| """Flatten a ``ValidationError.input_value`` into a single ``{field: reason}`` dict. | ||
|
|
||
| Returns ``None`` when the input cannot be structurally interpreted as per-field reasons, | ||
| in which case the caller should let the original error propagate as ``UNDEFINED_ERROR``. |
There was a problem hiding this comment.
I'd say that this line can go
|
|
||
| @classmethod | ||
| async def mutate( | ||
| async def mutate( # noqa: PLR0915 |
There was a problem hiding this comment.
would it be better to add it to the pyproject.toml exclusions instead of inline?
There was a problem hiding this comment.
should this be included?
Why
Introduce enriched GraphQL errors by using errors from a catalog.
As an example of how errors will change would be for a mutation that looks like this:
Where the current response would look like this:
{ "data": { "BuiltinTagCreate": null }, "errors": [ { "message": "name is mandatory for BuiltinTag at name", "locations": [ { "line": 3, "column": 3 } ], "path": [ "BuiltinTagCreate" ] } ] }After this change we'll instead have:
{ "data": { "BuiltinTagCreate": null }, "errors": [ { "message": "name is mandatory for BuiltinTag", "locations": [ { "line": 3, "column": 3 } ], "path": [ "BuiltinTagCreate" ], "extensions": { "code": "ATTRIBUTE_REQUIRED", "http_status": 422, "data": { "node_kind": "BuiltinTag", "field_name": "name" } } } ] }The goal is that for each string in the code we'll have an error catalogue that defines what the data payload could look like. I.e. what fields exist and what the type is of those fields. Then we'll create bindings for the frontend along with the ability to generate bindings for the SDK and make the error catalogue available for inclusion in other tools as well.
What changed
ValidationErrorthis exception was very generic and will need to be broken down into smaller parts, but I didn't want to change the internals of the code for this round and that part will come later in this work.Summary by cubic
Adds a catalogue-backed GraphQL error formatter and routes both resolver errors and auth short-circuits on
/graphqlthrough a structured envelope with string codes, HTTP status, and typed payloads for programmatic handling (INFP-468). REST/api/...responses are unchanged.New Features
extensions.code(string),extensions.http_status(int), and typedextensions.data; uncatalogued errors useUNDEFINED_ERRORwith HTTP fromError.HTTP_CODEor 500.pathand codes:ATTRIBUTE_REQUIRED,ATTRIBUTE_INVALID_TYPE,ATTRIBUTE_CONSTRAINT_VIOLATION; legacyValidationErroris auto-classified without refactoring call sites.AUTHENTICATION_REQUIREDvsTOKEN_EXPIRED;/graphqlshort-circuits return the catalogue envelope while/api/...keeps the old integer code shape.PERMISSION_DENIEDpayload exposes onlyactionandresource_kind; structlog logs the emittedcode.Migration
extensions.codeto string codes; useextensions.http_statusfor numeric checks.graphqlClientApollo.tsxmatches onAUTHENTICATION_REQUIRED/TOKEN_EXPIREDand suppresses toasts forPERMISSION_DENIED;login.tsxdisplays string codes./api/...consumers.Written for commit 471b242. Summary will update on new commits. Review in cubic