-
Notifications
You must be signed in to change notification settings - Fork 13
feat: raise typed per-code exceptions from the error catalogue #1373
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: pog-error-catalogue-IFC-3034
Are you sure you want to change the base?
Changes from all commits
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,20 @@ | ||
| Every code in Infrahub's error catalogue now has an exception class of its 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. | ||
|
|
||
| `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. | ||
|
|
||
| `docs/python-sdk/topics/error_handling` covers the hierarchy, the attributes readable on a caught error, and the cross-version guarantees. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -450,39 +450,39 @@ the raised type and the typed attributes, reading no message (quickstart scenari | |
|
|
||
| ### Tests for User Story 1 | ||
|
|
||
| - [ ] T066 [P] [US1] Add one response-envelope fixture per catalogue code under | ||
| - [X] T066 [P] [US1] Add one response-envelope fixture per catalogue code under | ||
|
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: This change marks Phase 8 tasks done but leaves T063/T064 unchecked, even though this PR performs both and the file's dependency section says Phase 8 depends on T064. Check T063/T064 in this same update, and reword T064's Prompt for AI agents |
||
| `tests/fixtures/error_catalogue/`, each a verbatim server response rather than a hand-shaped dict. | ||
| - [ ] T067 [US1] Add the exhaustive factory cases to `tests/unit/sdk/test_error_catalogue.py`: one per | ||
| - [X] T067 [US1] Add the exhaustive factory cases to `tests/unit/sdk/test_error_catalogue.py`: one per | ||
| code, asserting the raised class, every promoted attribute's concrete value, and that `exc.code` and | ||
| `exc.http_status` match the catalogue entry. No case reads a payload object, because there is none. | ||
| - [ ] T068 [P] [US1] Add the adopted-class cases to `tests/unit/sdk/test_error_catalogue.py`: a | ||
| - [X] T068 [P] [US1] Add the adopted-class cases to `tests/unit/sdk/test_error_catalogue.py`: a | ||
| server-reported `NODE_NOT_FOUND` populates `node_type` and `identifier`, `BRANCH_NOT_FOUND` and | ||
| `SCHEMA_NOT_FOUND` populate `identifier`, and `exc.code is not None` distinguishes a server-reported | ||
| raise from a client-side one. | ||
| - [ ] T069 [P] [US1] Add the representative parity set to `tests/unit/sdk/test_client.py`, parametrized | ||
| - [X] T069 [P] [US1] Add the representative parity set to `tests/unit/sdk/test_client.py`, parametrized | ||
| over `["standard", "sync"]` via the `BothClients` fixture, covering both branches, both transports, | ||
| and the file-upload variant, asserting the same class and the same attributes on each. | ||
| - [ ] T070 [P] [US1] Add a `catalogue`-marked case to `tests/integration/test_infrahub_client.py`: saving a | ||
| - [X] T070 [P] [US1] Add a `catalogue`-marked case to `tests/integration/test_infrahub_client.py`: saving a | ||
| node that collides on a unique attribute raises `UniquenessViolationError` with the node kind and | ||
| colliding fields from the real payload, and deleting a missing node raises `NodeNotFoundError` with | ||
| its kind and identifier. | ||
| - [ ] T071 [P] [US1] Add the same two `catalogue`-marked cases to | ||
| - [X] T071 [P] [US1] Add the same two `catalogue`-marked cases to | ||
| `tests/integration/test_infrahub_client_sync.py`. | ||
|
|
||
| ### Implementation for User Story 1 | ||
|
|
||
| - [ ] T072 [US1] Extend `graphql_error_from_response` in `infrahub_sdk/exceptions/factory.py` to look the | ||
| - [X] T072 [US1] Extend `graphql_error_from_response` in `infrahub_sdk/exceptions/factory.py` to look the | ||
| first error's code up in `CODE_TO_EXCEPTION`, validate `extensions.data` with the class's | ||
| `DATA_MODEL`, and raise via `cls.from_payload(...)`. The factory never assembles attributes itself. | ||
| - [ ] T073 [US1] Implement the validation-failure fallback in `infrahub_sdk/exceptions/factory.py`: an | ||
| - [X] T073 [US1] Implement the validation-failure fallback in `infrahub_sdk/exceptions/factory.py`: an | ||
| invalid payload falls back to the generic class for the observed transport with `exc.code` still | ||
| readable, the raw `extensions` retained, and a debug log. A pydantic `ValidationError` never escapes | ||
| a raise path. | ||
| - [ ] T074 [US1] Confirm in `infrahub_sdk/exceptions/factory.py` that the fallback follows **the transport | ||
| - [X] T074 [US1] Confirm in `infrahub_sdk/exceptions/factory.py` that the fallback follows **the transport | ||
|
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: T074 is marked complete with an overly narrow transport condition: the authentication factory also handles non-401 statuses from failed refresh attempts. Reword the checklist to distinguish the normal 401/403 response path from the refresh-failure exception. Prompt for AI agents |
||
| the SDK observed** and never the code's declared status: the GraphQL branch for anything read from | ||
| an `errors` array, the authentication branch only for a response the SDK saw as HTTP 401 or 403. | ||
| This is the only rule under which the three authentication codes reach the right class at all. | ||
| - [ ] T075 [US1] Add a test to `tests/unit/sdk/test_error_catalogue.py` asserting the first error governs | ||
| - [X] T075 [US1] Add a test to `tests/unit/sdk/test_error_catalogue.py` asserting the first error governs | ||
| even when it carries no code and a later one does, and that the complete list is retained unreordered. | ||
|
|
||
| **Checkpoint**: Every catalogue code is identifiable without reading a message, on both clients. | ||
|
|
@@ -491,12 +491,12 @@ the raised type and the typed attributes, reading no message (quickstart scenari | |
|
|
||
| ## Phase 9: Polish & Cross-Cutting Concerns | ||
|
|
||
| - [ ] T076 [P] Write `docs/docs/python-sdk/topics/error_handling.mdx` covering the hierarchy, catching by | ||
| - [X] T076 [P] Write `docs/docs/python-sdk/topics/error_handling.mdx` covering the hierarchy, catching by | ||
| branch versus by code, the cross-version guarantees, the two accepted broadenings, and the note that | ||
| `infrahub_sdk.exceptions` is the supported import path. Link to Infrahub's published catalogue for | ||
| the code list rather than restating it, and note that a catalogued message now names the failing | ||
| action and resource kind where the catalogue provides them. | ||
| - [ ] T077 [P] Add a towncrier fragment for the typed errors in `changelog/`. | ||
| - [X] T077 [P] Add a towncrier fragment for the typed errors in `changelog/`. | ||
| - [X] T078 [P] Add a towncrier fragment for the `NodeNotFoundError.identifier` widening in `changelog/`. | ||
| **Landed in issue 1**, alongside the widening itself (T036). | ||
| - [X] T079 [P] Add a towncrier fragment for the `except GraphQLError` broadening in `changelog/`. | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,184 @@ | ||||||
| --- | ||||||
| title: Understanding error handling in the Python SDK | ||||||
| --- | ||||||
|
|
||||||
| import Tabs from '@theme/Tabs'; | ||||||
| import TabItem from '@theme/TabItem'; | ||||||
|
|
||||||
| # Understanding error handling in the Python SDK | ||||||
|
|
||||||
| ## Introduction | ||||||
|
|
||||||
| Every exception the SDK raises is importable from `infrahub_sdk.exceptions`. That is the supported | ||||||
|
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: The page incorrectly says every SDK-raised exception is importable from Prompt for AI agents |
||||||
| import path, and the only one: the modules beneath it are internal and their layout may change. | ||||||
|
|
||||||
| ```python | ||||||
| from infrahub_sdk.exceptions import ApiError, GraphQLError, UniquenessViolationError | ||||||
| ``` | ||||||
|
|
||||||
| When Infrahub rejects a request, it describes the failure with a stable code from its | ||||||
| [error catalogue](https://docs.infrahub.app/reference/error-catalogue), an HTTP status, and a typed | ||||||
| payload. The SDK turns that description into an exception class of its own, with the payload's fields | ||||||
| as directly typed attributes, so branching on a specific failure never means matching words in a | ||||||
| message. | ||||||
|
|
||||||
| ## The hierarchy | ||||||
|
|
||||||
| ```text | ||||||
| Error every exception the SDK raises | ||||||
| └── ApiError the server rejected the request | ||||||
| ├── AuthenticationError the SDK observed HTTP 401 or 403 | ||||||
| └── GraphQLError a failure read from an `errors` array | ||||||
| ├── NodeNotFoundError | ||||||
| ├── BranchNotFoundError | ||||||
| ├── SchemaNotFoundError | ||||||
| ├── UniquenessViolationError | ||||||
| └── ... one class per catalogued code | ||||||
| ``` | ||||||
|
|
||||||
| The tree is plain: no class has more than one parent, and `AuthenticationError` and `GraphQLError` | ||||||
| are siblings. Anything the SDK raises without a server behind it - a timeout, an unreadable file, a | ||||||
| malformed query - stays under `Error` and outside `ApiError`. | ||||||
|
|
||||||
| ## Catching by branch, or by code | ||||||
|
|
||||||
| | Intent | Clause | | ||||||
| |--------|--------| | ||||||
| | Anything the server rejected, on either transport | `except ApiError` | | ||||||
| | Any GraphQL-path failure | `except GraphQLError` | | ||||||
| | Any failure the SDK observed as HTTP 401 or 403 | `except AuthenticationError` | | ||||||
| | One specific catalogued failure | `except UniquenessViolationError`, and so on per code | | ||||||
| | Anything the SDK raises | `except Error` | | ||||||
|
|
||||||
| Catching the specific class is the shortest route to the payload, because its attributes are typed | ||||||
| exactly as the catalogue declares them and a required field needs no guard: | ||||||
|
|
||||||
| <Tabs> | ||||||
| <TabItem value="async" label="Async" default> | ||||||
|
|
||||||
| ```python | ||||||
| from infrahub_sdk.exceptions import ApiError, UniquenessViolationError | ||||||
|
|
||||||
| try: | ||||||
| await node.save() | ||||||
| except UniquenessViolationError as exc: | ||||||
| print(exc.node_kind, exc.fields) | ||||||
| except ApiError as exc: | ||||||
| print("some other failure:", exc.code) | ||||||
| ``` | ||||||
|
|
||||||
| </TabItem> | ||||||
| <TabItem value="sync" label="Sync"> | ||||||
|
|
||||||
| ```python | ||||||
| from infrahub_sdk.exceptions import ApiError, UniquenessViolationError | ||||||
|
|
||||||
| try: | ||||||
| node.save() | ||||||
| except UniquenessViolationError as exc: | ||||||
| print(exc.node_kind, exc.fields) | ||||||
| except ApiError as exc: | ||||||
| print("some other failure:", exc.code) | ||||||
| ``` | ||||||
|
|
||||||
| </TabItem> | ||||||
| </Tabs> | ||||||
|
|
||||||
| Both clients raise the same class with the same attributes for the same failure. | ||||||
|
|
||||||
| ### The three authentication codes | ||||||
|
|
||||||
| `AUTHENTICATION_REQUIRED`, `TOKEN_EXPIRED`, and `PERMISSION_DENIED` have no class of their own. They | ||||||
| are the only codes that reach the SDK on two different transports, and each transport already has a | ||||||
|
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 says the authentication codes are the only codes that can arrive on both transports, but any catalogue code can appear in a real 401/403 response. Describe them as the only codes without dedicated classes instead. Prompt for AI agents
Suggested change
|
||||||
| class that existing code depends on: | ||||||
|
|
||||||
| | Arrival | Class raised | `exc.code` | | ||||||
| |---------|--------------|------------| | ||||||
| | A real 401 or 403, when the failure escapes before the query runs | `AuthenticationError` | the catalogue code | | ||||||
| | Inside an HTTP 200 `errors` array, when a resolver raised it | `GraphQLError` | the catalogue code | | ||||||
|
|
||||||
| Any of the three can arrive either way, so the arrival path is a property of how the server happened | ||||||
| to fail rather than of the code. To handle one of them whichever way it arrived, catch `ApiError` and | ||||||
| test the code: | ||||||
|
|
||||||
| ```python | ||||||
| except ApiError as exc: | ||||||
| if exc.code == "TOKEN_EXPIRED": | ||||||
| ... | ||||||
| ``` | ||||||
|
|
||||||
| `AuthenticationError` descends from `ApiError`, so an `except ApiError` clause placed first makes any | ||||||
| later `except AuthenticationError` unreachable. | ||||||
|
|
||||||
| ## Reading a caught error | ||||||
|
|
||||||
| These are readable on every `ApiError`, including one raised with no server response behind it, so | ||||||
| inspecting them never needs a guard for a missing attribute: | ||||||
|
|
||||||
| | Attribute | Contract | | ||||||
| |-----------|----------| | ||||||
| | `code` | The catalogue code string, or `None`. Never an integer. `None` means no code was resolved: a server predating the catalogue, a REST failure, or an error carrying no `extensions`. | | ||||||
| | `http_status` | The status the failure declares, or `None`. This is metadata about the failure, not the status the transport observed: a catalogued data error arrives as HTTP 200. | | ||||||
| | `extensions` | The raw `extensions` mapping of the governing error, or `None`. | | ||||||
| | `errors` | The complete server error list, in the order the server sent it. Empty for a raise the SDK decided on its own. | | ||||||
| | `query`, `variables` | The GraphQL query and variables where there was one, otherwise `None`. | | ||||||
|
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: The Prompt for AI agents |
||||||
|
|
||||||
| The payload's fields are not on the base class. Each catalogued class carries its own, typed as the | ||||||
| catalogue declares them. `NodeNotFoundError`, `BranchNotFoundError`, and `SchemaNotFoundError` are the | ||||||
| exception: the SDK also raises those three on its own, for a lookup that returned nothing and for the | ||||||
| REST 404 behind a missing file, so their attributes may be unpopulated. Test `exc.code is not None` to | ||||||
| tell a server-reported raise from an SDK one. | ||||||
|
|
||||||
| The raw payload stays in `exc.extensions["data"]` for anything that forwards a failure verbatim. | ||||||
|
|
||||||
| ## Messages | ||||||
|
|
||||||
| A failure the catalogue describes carries a message naming the code and the server's own words, with | ||||||
| no query text: | ||||||
|
|
||||||
| ```text | ||||||
| UNIQUENESS_VIOLATION: Node of kind TestPerson already has name 'John' | ||||||
| ``` | ||||||
|
|
||||||
| Where the catalogue provides them, those words name the failing action and the resource kind, so that | ||||||
| detail now appears in logs and CLI output in place of the query text that used to be there. The query | ||||||
| itself stays readable on `exc.query`. | ||||||
|
|
||||||
| A failure the catalogue does **not** describe keeps the message it has always had, query text and full | ||||||
| error list included. Since a current server codes every error it reports, falling back to | ||||||
| `UNDEFINED_ERROR` where its catalogue has no entry, `exc.code is not None` is not the test for whether | ||||||
| the server described a failure. `code_names_the_failure(exc.code)` is, and it is importable from | ||||||
| `infrahub_sdk.exceptions`. | ||||||
|
|
||||||
| Where a response carries several errors, the first one determines the class raised and is the only one | ||||||
| named beside the code. The complete list stays on `exc.errors`, in the order the server sent it. | ||||||
|
|
||||||
| ## Talking to any server version | ||||||
|
|
||||||
| Any SDK version talks to any server version, and parsing a response never raises. | ||||||
|
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: Malformed or non-JSON responses still raise Prompt for AI agents |
||||||
|
|
||||||
| | Situation | Behaviour | | ||||||
| |-----------|-----------| | ||||||
| | A code this SDK has a class for | That class, built from the payload the response carried | | ||||||
| | A code this SDK has never heard of | `GraphQLError`, or `AuthenticationError` on a 401 or 403, with `exc.code` set to the string the server sent | | ||||||
| | A known code whose payload gained a field | The unknown field is ignored | | ||||||
| | A server predating the catalogue, or an error with no `extensions` | `exc.code` is `None`, and the message is the one that version of the SDK has always produced | | ||||||
| | A payload that does not match what the catalogue declares | The generic class for the transport, with the code still readable | | ||||||
|
|
||||||
| Every fallback is logged at debug level on the `infrahub_sdk` logger with the code involved, so an SDK | ||||||
|
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: Unknown-code fallbacks are not logged as documented: the factory logs only when no string code resolves, not when a string code is absent from the catalogue. Log unresolved known-shape codes too, or narrow this guarantee to missing/non-string codes. Prompt for AI agents |
||||||
| meeting a newer server is diagnosable without a debugger. | ||||||
|
|
||||||
| Which generic class a fallback lands on follows the transport the SDK observed, never the status the | ||||||
| code declares. A code read from an `errors` array raises `GraphQLError` even when it declares 401. | ||||||
|
|
||||||
| ## Two clauses that now catch more | ||||||
|
|
||||||
| Existing `except` clauses keep catching everything they caught before. Two of them now catch more. | ||||||
|
|
||||||
| `except GraphQLError` also catches node, branch, and schema lookup misses that involved no GraphQL | ||||||
| request at all, because those three classes are re-rooted under it. Code that relied on them escaping | ||||||
| such a clause should catch the specific class ahead of it, as an ordered `except` ladder already must. | ||||||
|
|
||||||
| A ladder that handles one of those three specifically now sees server-reported failures arrive there | ||||||
| as well as the ones the SDK decides on its own. That is the point of binding a code to a class, and `exc.code is not | ||||||
| None` separates the two. | ||||||
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: The opening sentence overstates the feature: three catalogue codes deliberately have no dedicated exception class, as this changelog later explains. Reword it to say that catalogue failures now have dedicated classes where applicable, so the release note is not self-contradictory.
Prompt for AI agents