-
Notifications
You must be signed in to change notification settings - Fork 13
feat: typed exceptions for the server's error catalogue #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
base: infrahub-develop
Are you sure you want to change the base?
Changes from all commits
dddbdcf
1ec4e05
ed7524c
7418f48
7890f75
1707d04
e3f1236
55c5a56
4d29afc
4107885
c5ccbd0
cc26ca3
3735c7e
2af53e7
b02d2fc
beb37f1
a040b89
a2abeca
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| A lookup miss the server reports - `NODE_NOT_FOUND`, `BRANCH_NOT_FOUND` or `SCHEMA_NOT_FOUND` - now raises `NodeNotFoundError`, `BranchNotFoundError` or `SchemaNotFoundError` built from the payload the server sent, where it previously raised a generic `GraphQLError`: | ||
|
|
||
| ```python | ||
| try: | ||
| await client.delete(kind="NetworkDevice", id=device_id) | ||
| except NodeNotFoundError: | ||
| ... # already gone | ||
| ``` | ||
|
|
||
| These three are the only catalogued classes the SDK also raises on its own, for a lookup that returned nothing and for the REST 404 behind a missing file. A ladder that handles one of them specifically will now see server-reported failures arrive there alongside the SDK's own, and `exc.code is not None` tells the two apart. |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,12 @@ | ||||||
| `query` and `variables` are now readable on every `ApiError`, not only on the GraphQL branch. Reading either off an `AuthenticationError` previously raised `AttributeError`: | ||||||
|
|
||||||
| ```python | ||||||
| try: | ||||||
| await client.execute_graphql(query=query) | ||||||
| except ApiError as exc: | ||||||
| log.error("request failed", code=exc.code, query=exc.query) # no longer raises on a 401 | ||||||
| ``` | ||||||
|
|
||||||
| That clause is the one the SDK recommends for the three authentication codes, since each of them can arrive either on a real 401 or 403 or inside a GraphQL `errors` array, so it is exactly where the attributes had to exist. They join `code`, `http_status`, `extensions` and `errors`, which were already declared there for the same reason. | ||||||
|
|
||||||
| `None` means the failed request was not recorded on the exception rather than that there was no query: an authentication failure is observed at the transport, which has neither to hand. Nothing that reads these attributes today changes - `GraphQLError` still populates both from the request it was raised for. | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P3: The GraphQL transport has Prompt for AI agents
Suggested change
|
||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1 @@ | ||||||
| A client authenticating with an API token no longer replays a request after a 401 that reports an expired token. Only a client configured with a username and password can obtain a new token, so on the request paths that retry automatically the retry sent the same rejected token a second time, doubling the cost of the failure. Streaming downloads never retried and are unaffected. | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P3: Prompt for AI agents
Suggested change
|
||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,5 @@ | ||||||
| A failure the server's error catalogue describes now carries a message naming that code and the server's own words, in place of the query text. `GraphQLError` previously rendered as `An error occurred while executing the GraphQL Query <query>, <errors>` and now reads `UNIQUENESS_VIOLATION: Node of kind TestPerson already has name 'John'`; `AuthenticationError` reads `AUTHENTICATION_REQUIRED: <reason>`. On both transports the code names the first error only, which is the one that determines the exception raised; the complete list stays on `exc.errors`, and the query and variables stay on `exc.query` and `exc.variables`. | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P3: "On both transports ... the one that determines the exception raised" is not true for the authentication transport: Prompt for AI agents
Suggested change
|
||||||
|
|
||||||
| A failure the catalogue does **not** describe keeps today's message exactly, query text and full error list included, as does one of the lookup-miss classes raised without a server behind it. That covers a server predating the catalogue, an error carrying no `extensions`, and, since a current server codes every error it reports, anything the server coded `UNDEFINED_ERROR`. `exc.code` is readable in every case, so it is not the test for which message form you are holding: `code_names_the_failure(exc.code)`, importable from `infrahub_sdk.exceptions`, is. | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: This fallback description applies the GraphQL query-and-error-list format to every failure, but an auth failure without extensions keeps its auth-specific message (joined messages, REST detail, or HTTP status) and has no query attached. Limit the query/full-list claim to GraphQL failures and state separately that auth fallbacks remain auth-specific. Prompt for AI agents
Suggested change
|
||||||
|
|
||||||
| In `infrahubctl`, a described failure is now reported by its code and message rather than prefixed with `Authentication failure:` or rendered as a bare error list. An undescribed one renders exactly as before, so a GraphQL validation error still shows the line and column it failed on. Code matching on any of these strings should branch on `exc.code` instead. | ||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| `infrahubctl` no longer deletes bracketed text from an error message. A message such as `The requested branch was not found on the server [main]` was printed without the branch name, because rich read the brackets as a style tag. Everything the shared error handler prints is now escaped, including the traceback the fallback branch emits. Affects branch, schema, and node lookup misses, authentication failures, HTTP transport failures, and the fallback handler. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| `infrahubctl` no longer exits non-zero with no output when a GraphQL failure arrives in a shape the SDK cannot read as an error envelope. `infrahubctl run`, `infrahubctl validate graphql-query`, and every command wrapped by the shared error handler now fall back to printing the exception's message, which keeps the server's payload verbatim. |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,5 @@ | ||||||
| Authentication failures now surface the server's error envelope. `AuthenticationError` and `GraphQLError` share a new `ApiError` base carrying `code`, `http_status`, `extensions`, and `errors`, so a caller can branch on the server's catalogue code instead of matching on message text. | ||||||
|
|
||||||
| A 401 or 403 whose body the SDK cannot read as an error envelope now raises `AuthenticationError` carrying the best reason available: the REST API's bare `detail` string where the body has one, and otherwise the plain status. Previously the same responses raised `JsonDecodeError` (or, from the object store and file handler, a raw `json.JSONDecodeError`) when the body was not JSON, and `TypeError` when the body carried an `errors` array whose entries had no `message`. A body that was JSON but carried no `errors` key raised `AuthenticationError` with its generic default message, dropping the status the server sent. Code that catches those types around `object_store`, `file_handler`, or a client request should catch `AuthenticationError` instead. | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P3: The final sentence of this paragraph inaccurately describes the old behavior for a JSON body with no Prompt for AI agents
Suggested change
|
||||||
|
|
||||||
| `from infrahub_sdk.exceptions import *` now yields exactly the exception classes. It previously also carried whatever the module imported for its own annotations, such as `Mapping` and `Any`. Every exception class keeps its name and its import path. | ||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| `NodeNotFoundError`, `BranchNotFoundError`, and `SchemaNotFoundError` now descend from `GraphQLError`, so that one class covers a lookup miss however it arose: reported by the server, decided by the SDK, or turned from a REST 404. | ||
|
|
||
| An `except GraphQLError` clause therefore also catches lookup misses that involved no GraphQL request at all. Code that relied on those escaping such a clause should catch the specific class ahead of it, as an ordered `except` ladder already must. Each class keeps its name, its constructor, and the message it produces when no server reported the failure, and `errors`, `query`, and `variables` are now readable on every one of them rather than missing on a client-side raise. | ||
|
|
||
| `NodeInvalidError` inherits the re-rooting but not the adopted code: it means a node of the wrong kind rather than a lookup miss, so `NodeInvalidError.CODE` is `None` where `NodeNotFoundError.CODE` is `NODE_NOT_FOUND`. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| A file download answered with HTTP 404 now raises `NodeNotFoundError` whatever the response body contains. Previously a body that was not a JSON object - an HTML error page from an intermediary, an empty body, or a JSON array - escaped as a raw `json.JSONDecodeError` or `AttributeError` instead. |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,7 @@ | ||||||
| `GraphQLError` and every exception under it now survive `pickle` and `copy.deepcopy` intact. An exception that crosses a process boundary - a task queue, a parallel test runner, a process pool - previously came back wrong in one of two ways, because Python rebuilds an exception by calling its class with the message as a lone positional argument. | ||||||
|
|
||||||
| `GraphQLError` takes the server's error list first, so that call filed the message under `errors` and the message a caller read came back as the placeholder built from it: `An error occurred while executing the GraphQL Query None, UNIQUENESS_VIOLATION: ...` in place of the real one. The per-code classes take their payload fields as required keyword arguments, so the same call raised `TypeError: __init__() takes 1 positional argument but 2 were given` and the original failure was lost entirely. | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P3: The quoted Prompt for AI agents
Suggested change
|
||||||
|
|
||||||
| Both are fixed at the root of the GraphQL branch, so the type, the message, the payload attributes, `code`, `http_status`, `extensions`, `errors`, `query` and `variables` all come back as they went in, and an `except` clause still catches a restored exception by its own class. | ||||||
|
|
||||||
| `NodeNotFoundError`, `BranchNotFoundError` and `SchemaNotFoundError` were affected more quietly: their attributes and `str()` came back correct, but `exc.args` was rebuilt as the class's own default sentence rather than the message the exception was raised with. `AuthenticationError`, whose constructor takes the message first, was the only one unaffected. | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P3: The default pickle round-trip does not preserve Prompt for AI agents
Suggested change
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| `NodeNotFoundError.identifier` is now annotated `Mapping[str, list[str]] | str`. The SDK already raised it with a plain string to name a missing file, so this documents behaviour that was always there; no runtime behaviour changes and no existing caller needs updating. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| Fixed an `AttributeError` escaping the client when a 401 response carried a body that was valid JSON but not an object, such as the bare array or string a proxy or gateway may return. The silent token refresh now treats any body it cannot read as an envelope as carrying no refresh signal, and the request surfaces `AuthenticationError` as it does for every other unreadable 401. |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,22 @@ | ||||||
| Codes in Infrahub's error catalogue now raise an exception class of their own, importable from `infrahub_sdk.exceptions`, carrying the failure's payload as directly typed attributes. Identifying a specific failure no longer means matching words in a message: | ||||||
|
|
||||||
| ```python | ||||||
| from infrahub_sdk.exceptions import ApiError, UniquenessViolationError | ||||||
|
|
||||||
| try: | ||||||
| await node.save() | ||||||
| except UniquenessViolationError as exc: | ||||||
| print(exc.node_kind, exc.fields) # "TestPerson", ["name"] | ||||||
| except ApiError as exc: | ||||||
| print("some other failure:", exc.code) | ||||||
| ``` | ||||||
|
|
||||||
| The new classes are `AttributeConstraintViolationError`, `AttributeInvalidTypeError`, `AttributeRequiredError`, `BranchAlreadyMergedError`, `BranchNeedsRebaseError`, `MergeInProgressError`, `MergeRecoveryRequiredError`, `UndefinedError`, and `UniquenessViolationError`. Each is typed exactly as the catalogue declares the payload, so a required field is never optional and needs no guard. They all descend from `GraphQLError`, so no `except` clause stops catching what it catches today. `NODE_NOT_FOUND`, `BRANCH_NOT_FOUND` and `SCHEMA_NOT_FOUND` reach the classes the SDK already had for them, listed under Changed. | ||||||
|
|
||||||
| `exc.http_status` reads the status the envelope declares. Where the envelope omits one, a generated class supplies the status the catalogue gives its code; the three lookup-miss classes declare none, since they are also raised with no server behind them, and stay `None` there. | ||||||
|
|
||||||
| `AUTHENTICATION_REQUIRED`, `TOKEN_EXPIRED`, and `PERMISSION_DENIED` deliberately have no class of their own: each of them reaches the SDK on two transports, and which generic class it raises follows the transport the SDK observed rather than the status the code declares. Catch `ApiError` and test `exc.code` to handle one of the three whichever way it arrived. | ||||||
|
|
||||||
| A server predating a code, or one whose payload does not match what the catalogue declares for it, still raises the generic class for the transport with `exc.code` readable, so an SDK of any version keeps working against a server of any version. | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: Pre-catalogue GraphQL envelopes can carry numeric (Based on your team's feedback about GraphQL error-code shapes.) Prompt for AI agents |
||||||
|
|
||||||
| `docs/python-sdk/topics/error_handling` covers the hierarchy, the attributes readable on a caught error, and the cross-version guarantees. | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P3: The docs reference Prompt for AI agents
Suggested change
|
||||||
| 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P3:
ApiErroris introduced in this PR, so these fields were not previously declared on it. Describe them as declared alongsidequeryandvariableson the new base.Prompt for AI agents