docs: add specification for the error catalogue in the SDK - #1266
Draft
ogenstad wants to merge 5 commits into
Draft
docs: add specification for the error catalogue in the SDK#1266ogenstad wants to merge 5 commits into
ogenstad wants to merge 5 commits into
Conversation
Specifies how the SDK consumes Infrahub's GraphQL error catalogue so that ordinary operations raise the specific error for the failure, with GraphQLError remaining the fallback and the common base class. Key decisions settled while drafting: - A new ApiError base sits above both AuthenticationError and GraphQLError, since authentication failures reach consumers from the REST path as well as GraphQL. Its code attribute is a catalogue string or None; the REST envelope's integer code is not surfaced through it. - Generated exception classes derive their parent from the code's declared HTTP status (401/403 under the authentication branch, everything else under GraphQLError) rather than a hand-maintained mapping. - Infrahub generates the bindings into this repo as its python_sdk submodule, matching how protocols.py and the generated schema models already arrive. No copy of the catalogue schema is vendored here, so there is one freshness invariant instead of two, policed by extending Infrahub's existing validate-generated check. No release-time gate is added. - The query text is dropped from the message for catalogued errors only; uncatalogued errors keep today's message verbatim. - NodeNotFoundError, BranchNotFoundError and SchemaNotFoundError are unified with their catalogue counterparts and re-rooted under GraphQLError, accepting that except GraphQLError now also catches client-side lookup misses. Ref: IFC-3034
Deploying infrahub-sdk-python with
|
| Latest commit: |
e1cd490
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://0e1233d1.infrahub-sdk-python.pages.dev |
| Branch Preview URL: | https://pog-error-catalogue-ifc-3034.infrahub-sdk-python.pages.dev |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 2 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Both points were deferred to the plan in the first draft; both are observable API surface and belong in the spec. FR-013 now names the rule instead of requiring that one exist: the first error in a multi-error response determines the raised class, with the complete list retained. Selecting the first recognised code was rejected because it would make the raised type depend on how fresh the generated bindings are rather than on the response itself. FR-016 now pins the observable contract for the unified NodeNotFoundError - every construction shape in use today keeps working, the server-reported kind and identifier stay reachable, one documented accessor covers both cases, and any type widening is called out in release notes. The mechanism is still left to the plan. Surveying for this also found that identifier is already heterogeneous: the file handler passes a plain string where the declared type is a mapping of filters. The edge case no longer presents this as introduced by unification.
Plan, research decisions, data model, interface contracts, and a validation quickstart for the error catalogue in the SDK, plus the spec amendments the dual-lens critique produced. Design decisions worth reading first: - A payload's fields are promoted to directly typed attributes on the exception rather than exposed as a payload object. This is what the user story asks for, it matches how the three adopted classes already work, and it removes the unsound-override problem that a base-level payload attribute creates. Nothing in the design is typed Any beyond raw decoded JSON, and no suppression is anticipated anywhere. - exceptions.py becomes a strictly layered package: hand-written base, generated catalogue, factory, façade. Imports only point downward and a test enforces it, so the cycle the layout was designed around cannot creep back in. - The 401/403 codes descend from both the GraphQL and the authentication branch. The server returns HTTP 200 for resolver-raised errors, so a permission failure arrives on the data path in a response `except GraphQLError` catches today. - infrahub_sdk.exceptions is the one supported import path, and no name importable from it may disappear. Pinned by a snapshot test, since the module is being restructured. Spec amendments: the base carries no payload attribute (FR-001); an invalid payload falls back to the generic class (FR-004, FR-012); payload models are the parsing mechanism, not the access path (FR-005); an undeclared name collision fails generation (FR-006); 401/403 codes additionally descend from the authentication branch (FR-008). The HTTP 200 edge case and the stability assumption are corrected to match.
Contributor
There was a problem hiding this comment.
1 issue found across 8 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="dev/specs/ifc-3034-error-catalogue/quickstart.md">
<violation number="1" location="dev/specs/ifc-3034-error-catalogue/quickstart.md:1">
P3: This is a pure documentation/specification change (dev/specs/...), so it can't affect a running product and should ship on the stable release vehicle rather than the develop train. Per the release-vehicle guideline, pure docs changes belong on stable; target develop instead.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| @@ -0,0 +1,196 @@ | |||
| # Quickstart: validating the error catalogue in the SDK | |||
Contributor
There was a problem hiding this comment.
P3: This is a pure documentation/specification change (dev/specs/...), so it can't affect a running product and should ship on the stable release vehicle rather than the develop train. Per the release-vehicle guideline, pure docs changes belong on stable; target develop instead.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At dev/specs/ifc-3034-error-catalogue/quickstart.md, line 1:
<comment>This is a pure documentation/specification change (dev/specs/...), so it can't affect a running product and should ship on the stable release vehicle rather than the develop train. Per the release-vehicle guideline, pure docs changes belong on stable; target develop instead.</comment>
<file context>
@@ -0,0 +1,196 @@
+# Quickstart: validating the error catalogue in the SDK
+
+Runnable checks that prove the feature works end to end. Each scenario names what it proves and the
</file context>
Six findings accepted as filed, one accepted with a corrected diagnosis, one rejected. The consequential one: R6 and R7 disagreed on whether `code` stays readable when a recognised code's payload fails validation. R7 had folded that case into its `code is None` list, which would have made a payload-invalid catalogued error render as an uncatalogued one, since both the CLI branch and the server-reported test key on `exc.code is not None`. R7 now separates which class is raised from what `code` reports. FR-012's "the branch it is already on" meant the transport, but admitted a reading where the fallback follows the code's declared HTTP status — which would send an unrecognised 401/403 code arriving in a 200 body out of `except GraphQLError`. It now says transport explicitly, and records that for an unrecognised code the SDK holds no binding and so cannot know the declared status at all. Reviewing the dual base against the real-401/403 path surfaced a third accepted broadening: the authentication path now resolves catalogue codes, so `except GraphQLError` will begin catching a real 401/403 carrying one. The adopted classes now call `super().__init__(errors=[], ...)` explicitly rather than leaning on a class-level default, which removes the tuple/list divergence in `exc.errors` instead of documenting it. Also: guard the `extensions` read in the hierarchy contract, correct the raise-site count to 11, and put the critique's passes in chronological order. Rejected: the release-vehicle finding. Its remedy contradicts itself, the guideline it cites is not in this repository, and `develop` and `infrahub-develop` are different branches here.
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 6 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
All three findings valid, two of them self-inflicted last round. The third broadening was stated as "a real 401/403 carrying a catalogue code", which is overbroad: the dual base only applies to codes the bindings recognise, and an unrecognised code on a real 401/403 falls back to the generic AuthenticationError, which is not a GraphQLError. Both the contract and the spec now qualify it and say what happens to the rest. The data-model rationale claimed the adopted classes' explicit super().__init__ call is what prevents an AttributeError, but the previous round kept ApiError's class-level defaults, so the attributes exist regardless. Two fixes were applied and the old justification was left attached to the new mechanism. The two mechanisms are now described separately: defaults guarantee the attributes exist, the explicit call guarantees `errors` is a list. FR-012's rationale said the declared status is unknowable for an unrecognised code and then used an unrecognised code as the example of declared-status routing. It also credited FR-008's dual inheritance with preserving GraphQLError coverage, but the dual base shapes the per-code classes while a fallback raises the generic one, which has a single parent. The example is now a recognised 401/403 code whose payload fails validation, and the coverage is attributed to the transport rule. research.md R7 carried both errors and is fixed too.
Contributor
There was a problem hiding this comment.
1 existing issue remains and 1 new issue found across 5 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="dev/specs/ifc-3034-error-catalogue/spec.md">
<violation number="1" location="dev/specs/ifc-3034-error-catalogue/spec.md:450">
P3: The broadening claim that `except GraphQLError` "will also catch a real 401 or 403 whose code the SDK's bindings recognise" is only true when the recognised code's payload validates. Per FR-012, when a recognised 401/403 code's payload fails to validate, the fallback raises the generic class for the observed transport (here `AuthenticationError`), which `except GraphQLError` does not catch. The same PR's edit to FR-012 emphasizes exactly this payload-validation case, so the two statements are inconsistent: qualify that the catch holds only for a recognised code whose payload validates.</violation>
</file>
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| - **Three broadenings are accepted deliberately**: `except GraphQLError` will additionally catch node, | ||
| branch, and schema lookup misses that never involved a GraphQL request at all — both the client-side | ||
| ones and the REST 404 the file handler turns into a `NodeNotFoundError`; it will also catch a real | ||
| 401 or 403 whose code the SDK's bindings recognise, which previously raised a plain |
Contributor
There was a problem hiding this comment.
P3: The broadening claim that except GraphQLError "will also catch a real 401 or 403 whose code the SDK's bindings recognise" is only true when the recognised code's payload validates. Per FR-012, when a recognised 401/403 code's payload fails to validate, the fallback raises the generic class for the observed transport (here AuthenticationError), which except GraphQLError does not catch. The same PR's edit to FR-012 emphasizes exactly this payload-validation case, so the two statements are inconsistent: qualify that the catch holds only for a recognised code whose payload validates.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At dev/specs/ifc-3034-error-catalogue/spec.md, line 450:
<comment>The broadening claim that `except GraphQLError` "will also catch a real 401 or 403 whose code the SDK's bindings recognise" is only true when the recognised code's payload validates. Per FR-012, when a recognised 401/403 code's payload fails to validate, the fallback raises the generic class for the observed transport (here `AuthenticationError`), which `except GraphQLError` does not catch. The same PR's edit to FR-012 emphasizes exactly this payload-validation case, so the two statements are inconsistent: qualify that the catch holds only for a recognised code whose payload validates.</comment>
<file context>
@@ -445,10 +447,11 @@ These are specific hazards found while surveying the current code, not hypotheti
- class is chosen by the code and one class serves both arrival paths; and code that catches the generic
- error to inspect its message will now sometimes receive a subclass with a different message. All three
- follow from answered decisions rather than oversight.
+ 401 or 403 whose code the SDK's bindings recognise, which previously raised a plain
+ `AuthenticationError`, since the class is chosen by the code and one class serves both arrival paths —
+ but not one carrying an unrecognised code, which still falls back to `AuthenticationError`; and code
</file context>
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
1 participant
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.
Specification only — no behaviour change. Adds
dev/specs/ifc-3034-error-catalogue/covering how the SDK consumes Infrahub's GraphQL error catalogue, so that ordinary operations raise the specific error for the failure.GraphQLErrorremains the fallback for failures the catalogue does not cover and stays the common base class, so existingexcept GraphQLErrorcode keeps working.Ref: IFC-3034. Related: IFC-2279 (spike), INFP-468 (backend catalogue), GitHub #7498 (out of scope).
Decisions settled while drafting
ApiErrorbase above bothAuthenticationErrorandGraphQLError. Authentication failures reach consumers from the REST path as well as GraphQL, so they cannot simply be re-rooted underGraphQLError. Verified that a 401/403 on a GraphQL call is already handled as anhttpx.HTTPStatusErrorand raisesAuthenticationErrorbefore the body is parsed for GraphQL errors — soexcept GraphQLErrornever caught auth failures, and no dual inheritance is needed to preserve compatibility..codeis a catalogue string orNone. The/api/...envelope'sextensions.codeis an integer mirroring the HTTP status, a different thing with a different type; it is not surfaced through.code. The catalogue is GraphQL-only today.GraphQLError— rather than a hand-maintained per-code mapping.python_sdksubmodule, matching howprotocols.pyand the generated schema models already arrive. No copy of the catalogue schema is vendored here, so there is one freshness invariant instead of two, policed by extending Infrahub's existingvalidate-generatedcheck. No release-time gate is added on either side.NodeNotFoundError,BranchNotFoundErrorandSchemaNotFoundErrorare unified with their catalogue counterparts and re-rooted underGraphQLError, accepting thatexcept GraphQLErrornow also catches client-side lookup misses.Findings from the code survey worth a reviewer's eye
These are in the spec's Edge Cases section as specific hazards, not hypotheticals:
isinstanceladder gets shadowed.infrahub_sdk/ctl/utils.py:58-72testsGraphQLErrorat line 67 before(SchemaNotFoundError, NodeNotFoundError, ...)at line 70. Re-rooting those classes makes the later branch unreachable, silently changing CLI output for exactly the errors this feature makes specific. FR-018 requires the correction.GraphQLErrorbranch rendersexc.errors, a list of server error dicts. A unifiedNodeNotFoundErrorraised purely client-side has no server response behind it, so the list is empty.identifiercarries two types. The existing client-sideNodeNotFoundErrorhasidentifieras a mapping of filters; the catalogue payload has it as a single string. FR-016 mandates the unification; the reconciliation mechanism is left to the plan.NodeInvalidErrorsilently inheriting the re-rooting, a pre-existing call site passing a string whereGraphQLErrorexpects a list of error dicts,UNDEFINED_ERRORbeing a real code rather than the absence of one, and GraphQL data errors arriving as HTTP 200 while auth failures arrive as real 401/403 on a separate code path.Scope
Six prioritised user stories, 28 functional requirements. FR-025 to FR-027 land in the Infrahub repository (generation plus the extended drift check) and are tagged as such; everything else lands here.
Checks
rumdlclean across 131 files; Vale flags nothing in the new files. Requirements checklist atdev/specs/ifc-3034-error-catalogue/checklists/requirements.mdpasses 16/16 with no[NEEDS CLARIFICATION]markers remaining.Summary by cubic
Documents how the SDK consumes Infrahub's GraphQL error catalogue so ordinary operations raise specific, typed errors. Adds the specification plus the implementation plan and design artefacts (data model, contracts, research, validation quickstart), with the draft's rationale corrected after review. Documentation only; no behavior change.
ApiErroraboveAuthenticationErrorandGraphQLErrorto unify server-reported failures across transports..codeis a catalogue string or None; the REST integer code is never surfaced as.code.Anybeyond raw JSON.exceptions.pybecomes a strictly layered package with imports pointing downward only, enforced by a test;infrahub_sdk.exceptionsstays the single supported import path with no name disappearing, pinned by a snapshot test.NodeNotFoundError,BranchNotFoundError, andSchemaNotFoundErrorwith catalogue counterparts underGraphQLError.code; only the raised class falls back to the generic one.AuthenticationError.NodeNotFoundErroridentifier contract: existing construction shapes keep working; server-reported kind and identifier are accessible; any type widening is documented.Reviewer focus
Written for commit e1cd490. Summary will update on new commits.