From 65977175db6e8afb0f4e3dd9677b2c6f1f1bbcdb Mon Sep 17 00:00:00 2001 From: Patrick Ogenstad Date: Fri, 21 Aug 2026 17:28:46 +0200 Subject: [PATCH] docs: add specification for the error catalogue in the SDK 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 --- .../checklists/requirements.md | 53 +++ dev/specs/ifc-3034-error-catalogue/spec.md | 367 ++++++++++++++++++ 2 files changed, 420 insertions(+) create mode 100644 dev/specs/ifc-3034-error-catalogue/checklists/requirements.md create mode 100644 dev/specs/ifc-3034-error-catalogue/spec.md diff --git a/dev/specs/ifc-3034-error-catalogue/checklists/requirements.md b/dev/specs/ifc-3034-error-catalogue/checklists/requirements.md new file mode 100644 index 000000000..2e676df3a --- /dev/null +++ b/dev/specs/ifc-3034-error-catalogue/checklists/requirements.md @@ -0,0 +1,53 @@ +# Specification Quality Checklist: Error Catalogue in the Python SDK + +**Purpose**: Validate specification completeness and quality before proceeding to planning +**Created**: 2026-08-21 +**Feature**: [spec.md](../spec.md) + +## Content Quality + +- [x] No implementation details (languages, frameworks, APIs) +- [x] Focused on user value and business needs +- [x] Written for non-technical stakeholders +- [x] All mandatory sections completed + +## Requirement Completeness + +- [x] No [NEEDS CLARIFICATION] markers remain +- [x] Requirements are testable and unambiguous +- [x] Success criteria are measurable +- [x] Success criteria are technology-agnostic (no implementation details) +- [x] All acceptance scenarios are defined +- [x] Edge cases are identified +- [x] Scope is clearly bounded +- [x] Dependencies and assumptions identified + +## Feature Readiness + +- [x] All functional requirements have clear acceptance criteria +- [x] User scenarios cover primary flows +- [x] Feature meets measurable outcomes defined in Success Criteria +- [x] No implementation details leak into specification + +## Notes + +Two checklist items were resolved by scoping rather than by rewriting, and the reasoning is recorded +here so the plan phase does not relitigate it: + +- **"No implementation details" / "written for non-technical stakeholders"** — for a library, the + exception hierarchy *is* the user-facing product, so class names, catalogue codes, and the + transport split are domain vocabulary rather than implementation leakage. The spec names those and + deliberately withholds module layout, file names, generator implementation, and test mechanics. + Recorded as an explicit assumption in the spec rather than left implicit. +- **"Success criteria are technology-agnostic"** — SC-001 through SC-008 are stated as outcomes a + consumer or reviewer can verify (a failure is handleable without reading a message; no string + matching remains; a stale artefact fails validation) rather than as internal mechanics. They do + reference exceptions and catalogue codes, which is unavoidable and correct for this feature. + +Items deferred to the plan by design, not omission: + +- The reconciliation of the `identifier` attribute on the unified `NodeNotFoundError`, where the + client-side and catalogue meanings differ in type. FR-016 requires the unification; the spec records + the conflict as an edge case and leaves the mechanism to the plan. +- The rule for which error in a multi-error response selects the raised class. FR-013 requires the + rule to be explicit and documented; it does not pick one. diff --git a/dev/specs/ifc-3034-error-catalogue/spec.md b/dev/specs/ifc-3034-error-catalogue/spec.md new file mode 100644 index 000000000..75e89e035 --- /dev/null +++ b/dev/specs/ifc-3034-error-catalogue/spec.md @@ -0,0 +1,367 @@ +# Feature Specification: Error Catalogue in the Python SDK + +**Feature Branch**: `pog-error-catalogue-IFC-3034` + +**Created**: 2026-08-21 + +**Status**: Draft + +**Input**: IFC-3034 — Implement the error catalogue in the Python SDK. Related: IFC-2279 (spike), INFP-468 (backend catalogue), GitHub #7498. + +## Context + +Infrahub's GraphQL error catalogue gives every GraphQL error a stable string `extensions.code`, an +integer `extensions.http_status`, and a typed `extensions.data` payload, published as a +machine-readable schema at `schema/error-catalogue.json` in the Infrahub repository. The frontend +already consumes it through generated TypeScript bindings. + +The SDK consumes none of it. `execute_graphql` raises a generic `GraphQLError` whose message embeds +the entire query text, and consumers that need to branch on a failure still match on message +strings. This feature makes ordinary SDK operations raise the specific error for the failure. + +Two wire shapes matter, and they are not the same: + +- **`/graphql`** carries the catalogue envelope: string `code`, integer `http_status`, typed `data`. +- **`/api/...` (REST)** carries the legacy envelope, where `extensions.code` is an *integer* + mirroring the HTTP status. There is no catalogue code and no `data`. + +The catalogue is therefore GraphQL-only, and a REST `extensions.code` is a different thing with a +different type that must never be mistaken for a catalogue code. + +## User Scenarios & Testing *(mandatory)* + +### User Story 1 - Branch on a specific server failure (Priority: P1) + +A developer writing automation against Infrahub needs to react differently to different failures. A +`.save()` that collides on a uniqueness constraint should be distinguishable from a validation +failure, and the collision detail should be available as attributes rather than parsed out of prose. + +**Why this priority**: This is the feature. Everything else either protects it or maintains it. + +**Independent Test**: Drive each catalogued failure against a server (or a fixture of its response +envelope) and assert the raised type and its typed payload attributes, without reading any message. + +**Acceptance Scenarios**: + +1. **Given** a node whose unique attribute already exists, **When** the developer calls `.save()`, + **Then** `UniquenessViolationError` is raised carrying the node kind and the colliding field names + as typed attributes. +2. **Given** a node that no longer exists, **When** the developer calls `.delete()`, **Then** + `NodeNotFoundError` is raised carrying the node kind and identifier. +3. **Given** any catalogued failure, **When** it is raised, **Then** `exc.code` equals the catalogue + code string and `exc.http_status` equals the catalogue status. +4. **Given** a developer who catches `GraphQLError` today, **When** a catalogued GraphQL failure + occurs, **Then** the specific subclass is caught by that existing clause. + +--- + +### User Story 2 - Keep working against any server version (Priority: P1) + +The SDK and the server are versioned and released independently, so any SDK version may talk to any +server version. Neither direction may break. + +**Why this priority**: The ticket states this is a hard requirement, not a nice-to-have. User Story 1 +is not shippable without it — typed errors that raise on an unrecognised payload would be a +regression, not a feature. + +**Independent Test**: Replay response fixtures representing a newer server, an older server, and a +pre-catalogue server against the parsing layer, asserting no parse failure and correct fallback in +each case. + +**Acceptance Scenarios**: + +1. **Given** a code the SDK has never heard of, emitted by a newer server, **When** the SDK raises, + **Then** it raises the generic fallback for that transport branch with `exc.code` readable as a + plain string, and does not raise on parse. +2. **Given** an existing code whose payload has gained a new attribute in a newer server, **When** + an older SDK parses it, **Then** the unknown attribute is ignored and behaviour is unchanged. +3. **Given** a server that predates the catalogue, or an error carrying no `extensions`, **When** the + SDK raises, **Then** behaviour matches today's and `exc.code` is `None`. +4. **Given** a pre-catalogue server emitting an *integer* `extensions.code` on `/graphql`, **When** + the SDK parses it, **Then** it is not surfaced as a catalogue code and `exc.code` is `None`. +5. **Given** any of the above, **When** the developer regenerates nothing, **Then** correctness is + unaffected — regeneration buys typed handling of newly catalogued errors, never correctness. + +--- + +### User Story 3 - Catch server-reported errors uniformly across transports (Priority: P2) + +Authentication failures reach the developer from both the REST and GraphQL paths. Today they collapse +into a single `AuthenticationError` that cannot distinguish "no credentials" from "token expired" +from "not permitted". The catalogue splits these into three codes, and the SDK needs a hierarchy +where that split is expressible without stranding the REST path. + +**Why this priority**: It restructures the hierarchy every other story hangs off, but User Story 1 +delivers value with the existing flat `AuthenticationError` still in place. + +**Independent Test**: Assert the class hierarchy directly, and assert that each existing `except` +clause in the SDK and CLI still catches what it caught before. + +**Acceptance Scenarios**: + +1. **Given** a GraphQL request with an expired token, **When** it fails, **Then** `TokenExpiredError` + is raised and is caught by an existing `except AuthenticationError` clause. +2. **Given** a GraphQL request the user is not permitted to make, **When** it fails, **Then** + `PermissionDeniedError` is raised, distinguishable from a missing-credentials failure. +3. **Given** a REST request that fails authentication, **When** it fails, **Then** + `AuthenticationError` is raised as it is today, with `exc.code` as `None`. +4. **Given** a developer who wants to catch anything the server rejected regardless of transport, + **When** they catch `ApiError`, **Then** both GraphQL and auth failures are caught. + +--- + +### User Story 4 - Stop the SDK string-matching its own server (Priority: P2) + +The SDK's silent token-refresh path decides whether to re-login by matching the literal string +`"Expired Signature"` in the response body. The catalogue makes that a typed decision. + +**Why this priority**: A correctness improvement to existing behaviour, valuable independently, but +it depends on the envelope parsing from User Story 1. + +**Independent Test**: Drive the relogin path with a catalogue `TOKEN_EXPIRED` envelope, with the +legacy string on a pre-catalogue server, and with an unrelated 401, asserting a refresh is attempted +in the first two cases and not the third. + +**Acceptance Scenarios**: + +1. **Given** a 401 carrying `TOKEN_EXPIRED`, **When** the SDK receives it, **Then** it refreshes the + token and retries, without inspecting any message text. +2. **Given** a 401 from a pre-catalogue server carrying the legacy `"Expired Signature"` message, + **When** the SDK receives it, **Then** it still refreshes and retries. +3. **Given** a 401 that is neither, **When** the SDK receives it, **Then** no refresh is attempted. + +--- + +### User Story 5 - Bindings that cannot silently drift (Priority: P2) + +A catalogue change that is not reflected in the SDK's bindings must surface as a failure, in the +change that caused it, rather than as silence that is noticed months later when a code falls back. + +**Why this priority**: Without it the typed errors decay. It is P2 rather than P1 only because the +first generation can be landed and verified by hand once. + +**Independent Test**: Modify the catalogue without regenerating, and confirm the validation step +fails; regenerate, and confirm it passes. + +**Acceptance Scenarios**: + +1. **Given** a change to the catalogue in the Infrahub repository, **When** the bindings in the SDK + submodule are not regenerated, **Then** Infrahub's generated-artefact validation fails the pull + request that changed the catalogue. +2. **Given** a regenerated set of bindings, **When** validation runs, **Then** it passes and the + generated file is byte-identical to a fresh generation. +3. **Given** the generated bindings file, **When** a developer opens it, **Then** it is marked as + generated and not to be edited, consistent with the repository's other generated artefacts. + +--- + +### User Story 6 - Messages that are about the failure (Priority: P3) + +`GraphQLError`'s message embeds the whole query text, so a one-line failure produces a wall of +output in logs and CLI sessions. + +**Why this priority**: Observable quality-of-life improvement, no functional dependency either way. + +**Independent Test**: Trigger a catalogued failure and an uncatalogued one, and compare their +messages. + +**Acceptance Scenarios**: + +1. **Given** a catalogued failure, **When** its message is rendered, **Then** it names the code and + the server's message and does not contain the query text. +2. **Given** an uncatalogued failure, **When** its message is rendered, **Then** it is unchanged from + today's, query text included. +3. **Given** any GraphQL failure, **When** a developer needs the query, **Then** it is still + available on the exception. + +### Edge Cases + +These are specific hazards found while surveying the current code, not hypotheticals. + +- **Ordered `isinstance` ladder is shadowed.** The CLI's error handler tests + `isinstance(exc, GraphQLError)` *before* it tests + `isinstance(exc, (SchemaNotFoundError, NodeNotFoundError, ...))`. Re-rooting those classes under + `GraphQLError` makes the later branch unreachable, silently changing CLI output for exactly the + errors this feature makes specific. The ladder must be reordered, and the same shadowing hazard + checked wherever else the SDK or CLI tests these classes in sequence. +- **A GraphQL error renderer with no server errors to render.** The CLI's `GraphQLError` branch + renders `exc.errors`, which is a list of server error dicts. A unified `NodeNotFoundError` raised + purely client-side has no server response behind it, so that list is empty. Rendering must degrade + to the message rather than printing nothing. +- **`identifier` means two different things.** The existing client-side `NodeNotFoundError` carries + `identifier` as a mapping of filters, while the catalogue payload carries `identifier` as a single + string. Unifying the class puts two types and two meanings behind one attribute name. The spec + requires the unification (see FR-016); how the attribute is reconciled without breaking existing + readers is a design decision for the plan. +- **A subclass inherits the re-rooting.** `NodeInvalidError` subclasses `NodeNotFoundError`, so it + silently becomes a `GraphQLError` too. Intended, but it must be asserted rather than assumed. +- **A pre-existing constructor misuse.** One call site constructs `GraphQLError` with a plain string + where the constructor expects a list of error dicts, so `errors` holds a string. Any code that now + iterates `errors` to resolve a code will meet it. +- **A message-matching test inside our own suite.** At least one existing test asserts on + `GraphQLError`'s message text. Message changes must be reflected in the suite deliberately, not + worked around. +- **More than one error in one response.** A GraphQL response may carry several errors with different + codes. The rule for which code determines the raised class must be explicit, and no error may be + discarded from the exception. +- **`UNDEFINED_ERROR` is a code, not the absence of one.** A server that explicitly says + `UNDEFINED_ERROR` is reporting a catalogue gap on its side. That is distinct from an error carrying + no `extensions` at all, and the two must not collapse. +- **Codes with no payload.** Several codes declare an empty payload object. These must still produce + a usable class rather than a special case. +- **Silent-refresh runs on both transports.** The relogin wrapper inspects raw responses from REST + *and* GraphQL calls, but only GraphQL carries the catalogue envelope. It must read the code where + one exists and fall back to the legacy check where one does not. +- **GraphQL data errors arrive as HTTP 200.** Catalogued data errors come back with status 200 and an + `errors` array, while auth failures come back as real 401/403 responses handled by a separate code + path. A code's declared `http_status` is metadata about the failure, not the status the SDK saw. + +## Requirements *(mandatory)* + +### Functional Requirements + +#### Envelope parsing + +- **FR-001**: The SDK MUST expose a base class representing "the server reported an error", carrying + the catalogue code, the HTTP status, and the typed payload, from which both the GraphQL and the + authentication branches descend. +- **FR-002**: The SDK MUST parse the error envelope onto the base GraphQL error itself, so the code is + readable against any server version without regenerated bindings. +- **FR-003**: The code attribute MUST be either a catalogue code string or absent. The REST envelope's + integer `code` MUST NOT be surfaced through it; the HTTP status is already available separately. +- **FR-004**: Payload parsing MUST tolerate unknown fields, which is the inverse of the server's + strict emission contract. + +#### Generated bindings + +- **FR-005**: Every catalogue code MUST have one exception class and one typed payload model, all + importable from `infrahub_sdk.exceptions` and rooted at the SDK's base `Error` class. +- **FR-006**: Exception class names MUST derive from the code deterministically, without producing a + doubled `Error` suffix for codes that already end in `_ERROR`. +- **FR-007**: Payload model names MUST come from the catalogue's declared payload title, so SDK and + frontend bindings agree on naming. +- **FR-008**: The parent class MUST be derived from the code's declared HTTP status: 401 and 403 + descend from the authentication branch, everything else from the GraphQL branch. No hand-maintained + per-code mapping. +- **FR-009**: The generated artefact MUST carry the same "generated, do not edit" marking as the + repository's other generated files, and MUST record the catalogue version it was generated from. +- **FR-010**: The SDK MUST NOT contain a copy of the catalogue schema. The generated bindings are the + only artefact that crosses the repository boundary. + +#### Raising the specific error + +- **FR-011**: Every operation that today raises the generic GraphQL error MUST raise the specific + class when the response carries a recognised code. +- **FR-012**: Where no class matches the code — unrecognised, absent, or an integer from a + pre-catalogue server — the operation MUST raise the generic class for the branch it is already on: + the GraphQL error for data failures, the authentication error for 401/403 failures. +- **FR-013**: The rule selecting which of several errors in one response determines the raised class + MUST be explicit and documented, and the exception MUST retain all of them. +- **FR-014**: Async and sync clients MUST behave identically, per the constitution's parity + principle, and both paths MUST be tested. + +#### Reconciling names that already exist + +- **FR-015**: `AuthenticationError` MUST keep its name and constructor and MUST remain the class + raised for REST authentication failures, while gaining the three catalogue subclasses beneath it. +- **FR-016**: `NodeNotFoundError` MUST be unified into a single class covering both the client-side + and the server-reported cases, re-rooted so that an existing `except GraphQLError` clause catches + it. The consequent broadening — that clause now also catches purely client-side lookup misses — is + accepted. +- **FR-017**: `BranchNotFoundError` and `SchemaNotFoundError` MUST be reconciled the same way as + `NodeNotFoundError`. +- **FR-018**: Every existing `except` clause and `isinstance` check in the SDK and CLI MUST still + catch what it caught before the change, with ordered ladders corrected where re-rooting shadows a + later branch. + +#### Removing string matching + +- **FR-019**: The silent token-refresh decision MUST be made from the catalogue code where one is + present, retaining the existing message check only as the fallback for servers that predate the + catalogue. +- **FR-020**: The SDK's remaining message-string checks for catalogued failures MUST be replaced with + typed handling. +- **FR-021**: Checks that detect *uncatalogued* conditions — notably GraphQL schema-validation probing + used for server feature detection — are explicitly out of scope and MUST be left in place. + +#### Messages + +- **FR-022**: A catalogued error's message MUST name the code and the server's message, and MUST NOT + embed the query text. +- **FR-023**: An uncatalogued error's message MUST remain exactly as it is today, query text included. +- **FR-024**: The query and variables MUST remain available as attributes on the exception in both + cases. + +#### Generation and validation (Infrahub repository) + +- **FR-025**: Infrahub MUST generate the SDK's error bindings into the SDK submodule as part of its + existing generation task, alongside the schema models and protocols it already generates there. +- **FR-026**: Infrahub's existing generated-artefact validation MUST be extended to fail when the + submodule's bindings do not match a fresh generation, so a catalogue change that skips regeneration + fails the pull request that made it. +- **FR-027**: No release-time gate is added on either side. Pull-request-time validation is the + mechanism, matching the treatment the existing generated artefacts receive. + +#### Documentation + +- **FR-028**: SDK documentation MUST describe the exception hierarchy, the catalogue codes it covers, + and the cross-version behaviour a consumer can rely on, updated in the same change as the + behaviour. + +### Key Entities + +- **Catalogue code**: A stable string naming one failure mode, with a declared description, stability + level, HTTP status, and payload schema. Owned by Infrahub; the SDK is a consumer. +- **Error envelope**: What the server puts on the wire for one error. Two shapes exist — the catalogue + envelope on GraphQL, and the legacy integer-code envelope on REST. +- **Payload model**: The typed `data` for one code, tolerant of fields it does not recognise. +- **Exception hierarchy**: Rooted at the SDK's `Error`; below it a base for server-reported errors, + splitting into the authentication branch and the GraphQL branch, with one generated class per code. +- **Generated bindings module**: The single artefact crossing from Infrahub into the SDK, holding the + per-code classes, their payload models, and the code-to-class resolution used at raise time. + +## Success Criteria *(mandatory)* + +### Measurable Outcomes + +- **SC-001**: Every code in the catalogue is reachable as its own exception type with its typed + payload; a developer can handle any catalogued failure without reading a message. +- **SC-002**: No message-string matching remains in the SDK for any failure the catalogue covers. +- **SC-003**: The existing test suite passes with no `except` clause losing coverage it had before; + every deliberate behaviour change is pinned by a test that asserts the new behaviour. +- **SC-004**: Every cross-version case — unknown code, unknown payload field, absent envelope, + pre-catalogue integer code — is covered by a test and none of them raises during parsing. +- **SC-005**: A catalogue change that omits regeneration fails validation in the pull request that + introduced it, and a regenerated artefact is byte-identical to a fresh generation. +- **SC-006**: Async and sync clients raise the same type with the same attributes for the same + failure, across all catalogued codes. +- **SC-007**: A catalogued failure's message contains no query text, while an uncatalogued failure's + message is byte-identical to today's. +- **SC-008**: A developer can catch every server-reported error, on either transport, with one + `except` clause. + +## Assumptions + +- **The catalogue is GraphQL-only.** Confirmed against the server: REST responses keep the legacy + integer-code envelope. If REST later adopts the catalogue, the base class introduced here is where + it would attach, but no REST parsing is in scope. +- **Class and code names are the product, not implementation detail.** For an SDK the exception + hierarchy *is* the user-facing contract, so this spec names classes and codes. It deliberately does + not specify module layout, file names, generator implementation, or test framework mechanics. +- **Generation belongs to Infrahub.** The SDK cannot regenerate its own protocols or schema models + today either; those come from Infrahub's generation task writing into the submodule. Error bindings + follow that established pattern rather than introducing a second mechanism, which also removes any + need to keep a vendored catalogue copy in sync. +- **The exception module is not in the SDK's guaranteed-stability tier.** Only `Config`, + `InfrahubClient`, and `InfrahubClientSync` are exported at top level, so by the letter of the + constitution's stability tiers this hierarchy may change in a minor release. It is nonetheless + treated as effectively public — `infrahubctl`, the Ansible collection, and external consumers import + it directly — so all existing names and constructors are preserved. +- **Two broadenings are accepted deliberately**: `except GraphQLError` will additionally catch + client-side node, branch, and schema lookup misses; and code that catches the generic error to + inspect its message will now sometimes receive a subclass with a different message. Both follow from + answered decisions rather than oversight. +- **The repository-import failure handling in the git integrator (GitHub #7498) is out of scope**, as + is any change to what the server emits. +- **Both repositories are in scope for this document.** Requirements FR-025 to FR-027 land in the + Infrahub repository and must be executed from that checkout; everything else lands here.