-
Notifications
You must be signed in to change notification settings - Fork 12
docs: add specification for the error catalogue in the SDK #1266
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Draft
ogenstad
wants to merge
5
commits into
infrahub-develop
Choose a base branch
from
pog-error-catalogue-IFC-3034
base: infrahub-develop
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+2,324
−0
Draft
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
6597717
docs: add specification for the error catalogue in the SDK
ogenstad a2d6dcf
docs: pin the identifier contract and multi-error precedence
ogenstad 22a4023
docs: add the implementation plan and design artefacts
ogenstad 7e7b131
docs: address the automated review of the design artefacts
ogenstad e1cd490
docs: correct the rationale defects the previous fixes introduced
ogenstad File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
60 changes: 60 additions & 0 deletions
60
dev/specs/ifc-3034-error-catalogue/checklists/requirements.md
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,60 @@ | ||
| # 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. | ||
|
|
||
| Two items were originally deferred to the plan and have since been pulled back into the spec, both | ||
| prompted by automated review of the pull request: | ||
|
|
||
| - **The `identifier` contract on the unified `NodeNotFoundError`.** Deferring the whole question was | ||
| wrong: *which* attributes a consumer can read is observable API surface and belongs here, even | ||
| though the mechanism does not. FR-016 now pins the contract — every construction shape in use today | ||
| keeps working, the server-reported kind and identifier are reachable, one documented accessor works | ||
| for both cases, and any type widening is called out in release notes. Surveying the code for this | ||
| also turned up that the attribute is *already* heterogeneous: the file handler passes a plain string | ||
| where the declared type is a mapping. | ||
| - **Multi-error precedence.** FR-013 originally required only that a rule exist, which is untestable | ||
| until the rule does. It now specifies that the first error in the response governs, with the | ||
| complete list retained, and records why first-*recognised* was rejected: it would make the raised | ||
| type depend on binding freshness rather than on the response. |
123 changes: 123 additions & 0 deletions
123
dev/specs/ifc-3034-error-catalogue/contracts/exception-hierarchy.md
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,123 @@ | ||
| # Contract: The exception hierarchy | ||
|
|
||
| The SDK's public interface here is the set of names a consumer can import from | ||
| `infrahub_sdk.exceptions`, catch, and read attributes off. This is what the change promises. | ||
|
|
||
| `infrahub_sdk.exceptions` is the supported import path for every exception the SDK raises, generated or | ||
| hand-written. A consumer never needs to know which module inside it defines a given class, and the | ||
| modules beneath it are internal. Every name importable from `infrahub_sdk.exceptions` before this | ||
| change is still importable from it afterwards, pinned by a test against a committed snapshot rather | ||
| than asserted. | ||
|
|
||
| ## Catching | ||
|
|
||
| | Intent | Clause | | ||
| |--------|--------| | ||
| | Anything the server rejected, on either transport | `except ApiError` | | ||
| | Any GraphQL-path failure, including catalogued permission and token failures | `except GraphQLError` | | ||
| | Any authentication or permission failure, either transport | `except AuthenticationError` | | ||
| | One specific catalogued failure | `except UniquenessViolationError` (and so on per code) | | ||
| | Anything the SDK raises | `except Error` | | ||
|
|
||
| The catalogued 401/403 classes deliberately satisfy both `except GraphQLError` and | ||
| `except AuthenticationError`, because they reach the SDK on the GraphQL transport two different ways: | ||
|
|
||
| - **Inside a 200 response's `errors` array**, when the failure was raised from within a resolver. | ||
| `except GraphQLError` catches such a response today; the dual base is what keeps that true while also | ||
| making `except AuthenticationError` catch it. | ||
| - **As a real 401 or 403**, when the failure escapes before query execution. | ||
| `except AuthenticationError` catches this today. `except GraphQLError` does not, because the SDK | ||
| raises before reading the body — under this change it will, since the authentication path now resolves | ||
| the catalogue code and raises the specific class. That is a broadening, listed below. | ||
|
|
||
| Every clause that worked before the change still catches what it caught before (FR-018). Three | ||
| broadenings are deliberate: | ||
|
|
||
| - `except GraphQLError` now also catches node, branch, and schema lookup misses that involved no | ||
| GraphQL request at all — both the client-side ones and the REST 404 the file handler turns into a | ||
| `NodeNotFoundError` — because those classes are re-rooted under it. | ||
| - `except GraphQLError` now also catches a real 401 or 403 whose code **this SDK's bindings recognise**, | ||
| which previously raised a plain `AuthenticationError`. This follows from the dual base: the class is | ||
| chosen by the code, and the same class serves both arrival paths. It does not extend to a 401 or 403 | ||
| carrying a code the bindings do not know — that falls back to the generic `AuthenticationError` for | ||
| the transport, which is not a `GraphQLError`. | ||
| - Code that catches the generic error to inspect its message will now sometimes receive a subclass | ||
| whose message names the code instead of embedding the query. | ||
|
|
||
| ## Reading a caught error | ||
|
|
||
| Available on every `ApiError`: | ||
|
|
||
| | Attribute | Contract | | ||
| |-----------|----------| | ||
| | `code` | The catalogue code string, or `None`. Never an integer. `None` means the SDK resolved no catalogue code — a pre-catalogue server, a REST failure, an error with no `extensions`, or an integer `code` on the wire. An unrecognised string code from a newer server is still readable here. | | ||
| | `http_status` | The code's catalogue-declared status, or `None`. This is metadata about the failure, not the HTTP status of the response — a catalogued data error arrives as HTTP 200. Where the error carried an `extensions` mapping, the status the server actually returned is available as `exc.extensions["http_status"]` — guard on `exc.extensions` first, since it is `None` when the error carried none. The two can legitimately differ: the server replaces a declared 500 with the real HTTP status when it has a more accurate one. | | ||
| | the payload's fields | Not on the base. Each catalogued class carries its payload's fields as directly typed attributes — `UniquenessViolationError.node_kind` is a `str`, `.fields` a `list[str]` — typed exactly as the catalogue declares them, so a required field is never optional and needs no guard. The raw payload dict remains in `extensions["data"]` for anything forwarding it verbatim. | | ||
| | `extensions` | The raw `extensions` mapping of the governing error, or `None`. | | ||
| | `errors` | The complete server error list, unreordered — empty for a client-side raise. | | ||
| | `query`, `variables` | The GraphQL query and variables where there was one, otherwise `None`. | | ||
|
|
||
| `errors`, `query`, and `variables` are readable on every `ApiError`, not only on those built from a | ||
| server response. A purely client-side `NodeNotFoundError` has an empty `errors` and `None` for the rest, | ||
| so code that catches `GraphQLError` and inspects them never has to guard for a missing attribute. | ||
|
|
||
| `UNDEFINED_ERROR` is a code like any other: it means the server explicitly reported a gap in its own | ||
| catalogue, and it is not the same as an error carrying no `extensions`. | ||
|
|
||
| ## Cross-version behaviour | ||
|
|
||
| Any SDK version talks to any server version. Parsing never raises. | ||
|
|
||
| | Situation | Behaviour | | ||
| |-----------|-----------| | ||
| | A code the SDK has never heard of | The generic class for the branch is raised — `GraphQLError` for data failures, `AuthenticationError` for 401/403 — with `code` set to the string the server sent. | | ||
| | A known code whose payload gained a field | The unknown field is ignored; behaviour is unchanged. | | ||
| | A server predating the catalogue, or an error with no `extensions` | Today's behaviour exactly; `code` is `None`. | | ||
| | An integer `code` on `/graphql` from a pre-catalogue server | Not surfaced as a catalogue code; `code` is `None`. | | ||
| | A payload that violates the catalogue's own contract | The generic class for the branch, with the code still readable. The specific class's attributes are typed as the catalogue declares them, so there is nothing to populate a required one with. | | ||
|
|
||
| Every fallback above is logged at debug level with the code involved, so an SDK meeting a newer server | ||
| is diagnosable in the field rather than only in tests. | ||
|
|
||
| Regenerating bindings buys typed handling of newly catalogued codes. It never changes which exception | ||
| a byte-identical response produces for a code the SDK already knows, because the first error in the | ||
| response governs unconditionally — not the first *recognised* one. | ||
|
|
||
| ## Multiple errors in one response | ||
|
|
||
| The first error in the response determines the class raised. The complete list is retained on the | ||
| exception, unreordered, and nothing is discarded. If the first error carries no code and a later one | ||
| does, the generic class for the branch is raised. | ||
|
|
||
| ## Messages | ||
|
|
||
| A catalogued failure's message names the code and the server's message and contains no query text. | ||
| An uncatalogued failure's message is byte-identical to today's, query text included. The query is | ||
| available as an attribute in both cases. | ||
|
|
||
| Where the catalogue provides them, the server's message names the failing action and resource kind, so | ||
| that detail now appears in logs and CLI output in place of the query text that used to be there. | ||
|
|
||
| ## Parity | ||
|
|
||
| The async and sync clients raise the same type with the same attributes for the same failure, for | ||
| every catalogued code. | ||
|
|
||
| ## Stability | ||
|
|
||
| `infrahub_sdk.exceptions` is treated as public and is the one import path a consumer needs. That is a | ||
| stronger promise than the constitution's tiering strictly requires — only `Config`, `InfrahubClient`, | ||
| and `InfrahubClientSync` are exported at top level — and it is made deliberately, because | ||
| `infrahubctl`, the Ansible collection, and external consumers already import from it directly. | ||
|
|
||
| Concretely: | ||
|
|
||
| - No name is removed or renamed, and no constructor loses a signature it has today. | ||
| - Every name importable from `infrahub_sdk.exceptions` before this change remains importable from it, | ||
| which a test pins against a committed snapshot. Restructuring the module into a package must not be | ||
| observable from the outside. | ||
| - Modules beneath `infrahub_sdk.exceptions` are internal. Importing `…exceptions.catalogue` or | ||
| `…exceptions.payloads` directly is not supported, and their layout may change. | ||
| - One annotation widens: `NodeNotFoundError.identifier` becomes `Mapping[str, list[str]] | str`. It is | ||
| called out in a changelog fragment because external consumers read these attributes even though | ||
| nothing in this repository does. | ||
122 changes: 122 additions & 0 deletions
122
dev/specs/ifc-3034-error-catalogue/contracts/generator-contract.md
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,122 @@ | ||
| # Contract: Generating the SDK's error bindings | ||
|
|
||
| One artefact crosses from the Infrahub repository into the SDK. This is the contract between the two | ||
| sides. The SDK holds no copy of the catalogue schema (FR-010). | ||
|
|
||
| ## The artefact | ||
|
|
||
| - **Source**: `schema/error-catalogue.json` in the Infrahub repository. | ||
| - **Output**: `infrahub_sdk/exceptions/catalogue.py` in the SDK submodule. | ||
| - **Owner**: Infrahub. The SDK never regenerates it, exactly as it never regenerates `protocols.py` | ||
| or its schema models. | ||
| - **Committed**: yes. Generation happens in a pull request, not at install time, so catalogue drift is | ||
| visible in the diff. | ||
|
|
||
| The file is generated **in full**. There is never a hand-edited region inside a generated file, nor a | ||
| generated region inside a hand-written one. | ||
|
|
||
| ## What the generated module contains | ||
|
|
||
| It opens with a header marking it generated and not to be edited, naming the source artefact, recording | ||
| the catalogue's `infrahub_catalogue_version`, and giving the regeneration command (FR-009) — the same | ||
| marking style as the repository's other generated files. It declares `__all__`, which is what lets the | ||
| package façade re-export it without a hand-maintained list. | ||
|
|
||
| The body holds three things: | ||
|
|
||
| - One pydantic payload model per catalogue code, including codes with an empty payload and including | ||
| adopted codes. These validate the envelope and supply the promoted attributes' types. | ||
| - One exception class per catalogue code the SDK has not adopted, each promoting its payload's fields | ||
| to directly typed attributes and exposing a `from_payload` classmethod. | ||
| - `CODE_TO_EXCEPTION`, mapping every catalogue code to its class — generated classes for most, | ||
| imported adopted classes for the rest. | ||
|
|
||
| It imports only `infrahub_sdk.exceptions.base`, which imports nothing from inside the package. That | ||
| keeps the package's import graph one-way with no cycle: `base` → `catalogue` → `factory` → the façade. | ||
|
|
||
| Codes are emitted in sorted order so that reordering the catalogue's JSON does not churn the diff. | ||
|
|
||
| ## Derivation rules | ||
|
|
||
| No hand-maintained per-code table exists on either side. Everything is derived from the catalogue | ||
| entry: | ||
|
|
||
| | Output | Derived from | | ||
| |--------|--------------| | ||
| | Exception class name | The code's parts capitalised and joined, with `Error` appended only if it does not already end in `Error`. `UNDEFINED_ERROR` → `UndefinedError`. | | ||
| | Payload model name | `data_schema.title`, verbatim. | | ||
| | Base classes | `GraphQLError` always; `http_status in {401, 403}` additionally adds `AuthenticationError`, emitted as `(GraphQLError, AuthenticationError)`. | | ||
| | `http_status` class attribute | The catalogue's declared `http_status`. | | ||
| | Docstring | The catalogue's `description` and `stability`. | | ||
| | Promoted attribute names | The payload field names, verbatim. | | ||
| | Field and attribute types | The JSON Schema mapping in [data-model.md](../data-model.md); a required field is non-optional, a nullable one carries its declared default. | | ||
|
|
||
| ## Adoption | ||
|
|
||
| Some catalogue codes are represented by a class the SDK already ships. Those classes declare the code | ||
| they represent with a `CODE` class attribute. The generator parses | ||
| `infrahub_sdk/exceptions/base.py` with `ast`, collects every class whose body assigns a `CODE` | ||
| string, and for those codes emits an import and a map entry instead of a class definition. The payload | ||
| model is still generated. | ||
|
|
||
| An adopted class supplies its own `from_payload`, since its attribute names are its existing ones | ||
| rather than the catalogue's. | ||
|
|
||
| Adopting a further code later is a one-line change in the SDK's hand-written module with no generator | ||
| edit. Discovery is by parsing rather than importing, so the generator stays a pure text transform and | ||
| generation never depends on the SDK checkout being importable. The same walk collects every class name | ||
| defined in `base.py`, which is what the collision check below needs. Parsing also sidesteps a trap an | ||
| attribute walk would hit: `NodeInvalidError` inherits `CODE` from `NodeNotFoundError`, so two classes | ||
| would appear to claim the same code. | ||
|
|
||
| ## Failing loudly | ||
|
|
||
| Generation aborts, rather than emitting a guess, when: | ||
|
|
||
| - a catalogue entry has no integer `http_status`; | ||
| - a catalogue entry has no non-empty `data_schema.title`; | ||
| - a `data_schema` uses a construct outside the supported vocabulary, in which case the offending | ||
| fragment appears in the error; | ||
| - `codes` is empty or the root is not an object; | ||
| - a derived class name collides with a class already defined in `base.py` that has not declared that | ||
| code as adopted. | ||
|
|
||
| The first four are the assertions the frontend generator already makes, for the same reason. The last | ||
| is specific to Python's import semantics: the SDK's façade re-exports `base` and then `catalogue`, so an | ||
| undeclared collision would let the generated class silently take the name and change what an existing | ||
| `except` clause catches. The SDK already defines `ValidationError`, `RateLimitError`, | ||
| `InvalidResponseError`, `FileNotValidError`, and `ResourceNotDefinedError` — every one of them the name | ||
| a plausible future code would derive — so this is a live hazard rather than a theoretical one. Failing | ||
| generation forces the choice (adopt the code, or rename) into the pull request that adds the code. | ||
|
|
||
| ## Validation | ||
|
|
||
| `uv run invoke frontend.regenerate-error-bindings` regenerates the catalogue JSON, the frontend | ||
| TypeScript bindings, the docs page, and now the SDK bindings, so all catalogue-derived artefacts move | ||
| together. | ||
|
|
||
| `uv run invoke backend.validate-generated` verifies the SDK artefact with | ||
| `git -C python_sdk diff --exit-code infrahub_sdk/exceptions/catalogue.py`. The diff must run inside the | ||
| submodule, because from the superproject `git diff` only sees the submodule pointer — the same reason | ||
| the existing schema-model and protocol checks are written that way. | ||
|
|
||
| In CI, `backend-validate-generated` hosts this because it is the job that runs | ||
| `backend.validate-generated`. Its trigger condition gains `error_catalogue == 'true'` so a | ||
| catalogue-only change cannot slip past, and `error_catalogue_files` in `.github/file-filters.yml` | ||
| gains the submodule artefact path and the template, so editing the committed bindings or the generator | ||
| also triggers the check. | ||
|
|
||
| Submodule availability is not a factor in that placement. Infrahub declares | ||
| `infrahub-sdk = { path = "python_sdk", editable = true }`, so `uv sync` fails without the submodule and | ||
| every Python job there already requires `submodules: true`. Any job that needs the submodule declares | ||
| it; the check goes where it belongs and the checkout follows. | ||
|
|
||
| A catalogue change that skips regeneration therefore fails the pull request that made it (FR-026). | ||
| There is no release-time gate on either side; pull-request-time validation is the mechanism, matching | ||
| how the existing generated artefacts are treated (FR-027). | ||
|
|
||
| ## What regeneration does and does not buy | ||
|
|
||
| Regenerating adds typed handling for newly catalogued codes. It never changes which exception a | ||
| byte-identical response produces for a code the SDK already knows, and correctness never depends on | ||
| having regenerated — an SDK with stale bindings falls back rather than failing. |
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.