diff --git a/changelog/+api-error-carries-the-request.added.md b/changelog/+api-error-carries-the-request.added.md new file mode 100644 index 000000000..eea138369 --- /dev/null +++ b/changelog/+api-error-carries-the-request.added.md @@ -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. diff --git a/changelog/+graphql-error-survives-serialisation.fixed.md b/changelog/+graphql-error-survives-serialisation.fixed.md new file mode 100644 index 000000000..09b9d0aa6 --- /dev/null +++ b/changelog/+graphql-error-survives-serialisation.fixed.md @@ -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. + +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. diff --git a/changelog/+typed-per-code-exceptions.added.md b/changelog/+typed-per-code-exceptions.added.md new file mode 100644 index 000000000..c9b160a90 --- /dev/null +++ b/changelog/+typed-per-code-exceptions.added.md @@ -0,0 +1,20 @@ +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. + +`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. diff --git a/dev/specs/ifc-3034-error-catalogue/contracts/exception-hierarchy.md b/dev/specs/ifc-3034-error-catalogue/contracts/exception-hierarchy.md index ef7ff6f27..71d3fa231 100644 --- a/dev/specs/ifc-3034-error-catalogue/contracts/exception-hierarchy.md +++ b/dev/specs/ifc-3034-error-catalogue/contracts/exception-hierarchy.md @@ -82,15 +82,17 @@ 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 status the governing error's `extensions` declares, or `None` when it declared none. This is metadata about the failure, not the status the transport observed — a catalogued data error arrives as HTTP 200, and the observed status is not carried on the exception at all. Once the generated classes land, a class's own declared status fills this in for a code whose envelope omitted it. The envelope's value is the catalogue's except where the catalogue could not resolve a status more specific than 500, in which case the server substitutes the HTTP status it is about to return — so a generated class's declared 500 and the envelope's value can differ. | +| `http_status` | The status the governing error's `extensions` declares, or `None` when it declared none. This is metadata about the failure, not the status the transport observed — a catalogued data error arrives as HTTP 200, and the observed status is not carried on the exception at all. Once the generated classes land, a generated class's own declared status fills this in for a code whose envelope omitted it. `NodeNotFoundError`, `BranchNotFoundError` and `SchemaNotFoundError` declare none and so stay `None` there, because they are also raised with no catalogue code behind them. The envelope's value is the catalogue's except where the catalogue could not resolve a status more specific than 500, in which case the server substitutes the HTTP status it is about to return — so a generated class's declared 500 and the envelope's value can differ. | | 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 three exceptions are `NodeNotFoundError`, `BranchNotFoundError`, and `SchemaNotFoundError`, whose attributes are optional because those classes are also raised with no catalogue code behind them — a client-side lookup miss, or the REST 404 that has a response but no code; guard on `exc.code is not None` there. 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`. | +| `query`, `variables` | The GraphQL query and variables the failed request carried, where the exception recorded them. `None` means the request was not recorded, not that there was none: an authentication failure is observed at the transport, which has neither to hand. | -`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. +`errors`, `query`, and `variables` are declared on `ApiError` itself, so they are readable on every +server-reported failure and on a purely client-side raise alike. A client-side `NodeNotFoundError` has +an empty `errors` and `None` for the rest, and so does an `AuthenticationError`, so neither +`except GraphQLError` nor the `except ApiError` clause that spans both transports has to guard for a +missing attribute. `UNDEFINED_ERROR` is readable on `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`. It is the one code diff --git a/dev/specs/ifc-3034-error-catalogue/plan.md b/dev/specs/ifc-3034-error-catalogue/plan.md index 76af08d22..88a7304b7 100644 --- a/dev/specs/ifc-3034-error-catalogue/plan.md +++ b/dev/specs/ifc-3034-error-catalogue/plan.md @@ -96,7 +96,7 @@ documentation topic page. | III. Layered Architecture | **Pass.** All envelope parsing, resolution, and message construction lives in `infrahub_sdk/`. The CLI change is confined to presentation: reordering its `isinstance` ladder and degrading the GraphQL renderer when there are no server errors to render. | | IV. Type Safety & Typed Errors | **Pass, with no suppressions.** This principle is the feature. Generated payload models are pydantic v2; every failure mode gets a specific subclass under `Error`; each catalogued class carries its payload's fields as attributes typed exactly as the catalogue declares them. `Any` appears only where the value genuinely is unknown at the type level — the raw decoded JSON in `extensions` and `errors`, the latter already annotated that way today. No `# type: ignore` is anticipated anywhere; if the R15 spike shows one is needed, that is a signal the shape is wrong rather than a licence to add it. | | V. Test-First Development | **Pass.** Tests ship in the same change: envelope fixtures per code, cross-version fallback cases, ladder assertions, relogin cases, and both-client parity. Deliberate behaviour changes (the message change, the re-rooting) are pinned by tests that assert the new behaviour rather than being worked around. Because every fixture is authored alongside the parser that reads it, the mocked suite alone could pass against an envelope shape the server never sends — so two real catalogued failures are also driven through testcontainers, which is where the constitution puts behaviour that depends on real server responses. | -| VI. Format & Lint Before Commit | **Pass, with one justified silencing.** `uv run invoke format lint-code` and `lint-docs`; the generated modules are `ruff format`ed by the generator, as the other generated artefacts are. The façade re-exports the generated classes with `from .catalogue import *`, which trips `F403` under `select = ["ALL"]`. A wildcard is the only re-export form that keeps the export surface automatic as codes are added *and* stays visible to mypy and `ty`; the alternative is a hand-maintained list edited every time the catalogue grows. Recorded as a commented `per-file-ignores` entry, mirroring the existing entry for `infrahub_sdk/schema/generated/*.py`. | +| VI. Format & Lint Before Commit | **Pass, with no silencing.** `uv run invoke format lint-code` and `lint-docs`; the generated modules are `ruff format`ed by the generator, as the other generated artefacts are. This row first planned to re-export the generated classes with `from .catalogue import *` and to record the resulting `F403` as a commented `per-file-ignores` entry. The façade lists its re-exports by explicit name instead, so no `F403` arises and no entry was added: a wildcard would also have promoted the payload models, the lookup maps and the dispatch helper onto the package surface. The hand-maintained list is held against what the source modules define by a test, which is what the wildcard was meant to buy. | | VII. Documentation Accuracy | **Pass.** A new hand-written topic page describes the hierarchy and the cross-version guarantees, linking to Infrahub's published catalogue for the code list rather than restating it. Converting `exceptions.py` into a package requires categorising it in `tasks.py::get_modules_to_document`; it goes in `packages_to_ignore`, which preserves today's `sdk_ref` output exactly and avoids coupling the SDK's `docs-validate` to an Infrahub-side regeneration. | **Result**: no violations. Complexity Tracking is empty. @@ -160,7 +160,7 @@ docs/docs/python-sdk/topics/ └── error_handling.mdx # New topic page (sidebar globs this directory) changelog/ # towncrier fragments: typed errors, identifier widening, broadening -pyproject.toml # per-file-ignore for the façade's re-export star imports +pyproject.toml # pytest markers: catalogue, crossversion, malformed, message tasks.py # Add `exceptions` to packages_to_ignore for API-doc generation ``` diff --git a/dev/specs/ifc-3034-error-catalogue/research.md b/dev/specs/ifc-3034-error-catalogue/research.md index 7c54faae7..a5b3c8919 100644 --- a/dev/specs/ifc-3034-error-catalogue/research.md +++ b/dev/specs/ifc-3034-error-catalogue/research.md @@ -84,13 +84,14 @@ decay into the cycle it was designed out of. No new dependency. not documented in `sdk_ref` today, so ignoring the package preserves current behaviour exactly and creates no coupling. FR-028 is satisfied by the hand-written topic page, which is the better artefact for a hierarchy anyway. -- The façade re-exports with `from .base import *` and `from .catalogue import *`, each source module - declaring its own `__all__` (generated for `catalogue.py`). That keeps the export - surface automatic as codes are added and stays visible to mypy and `ty`, at the cost of an `F403` - `per-file-ignores` entry with a comment — the same treatment - `infrahub_sdk/schema/generated/*.py` already gets. `infrahub_sdk.exceptions` is the supported import - path for consumers; the submodules beneath it are internal, and the façade is what makes that true - rather than aspirational. +- The façade re-exports by explicit name from both `base` and `catalogue`, rather than the wildcards + this section first proposed. A wildcard would have kept the export surface automatic as codes are + added, at the cost of an `F403` `per-file-ignores` entry; it would also have promoted `catalogue`'s + payload models, lookup maps and dispatch helper onto the package surface, where each name becomes a + stability promise the package never chose to make. Written out by hand, the file says what it + exports, and a test holds it against what the source modules define so the two cannot drift. + `infrahub_sdk.exceptions` is the supported import path for consumers; the submodules beneath it are + internal, and the façade is what makes that true rather than aspirational. - First generation is a bootstrap: the SDK pull request lands `catalogue.py` produced by running the Infrahub generator from the paired branch, verified by hand once, which is what US5 anticipates. diff --git a/dev/specs/ifc-3034-error-catalogue/tasks.md b/dev/specs/ifc-3034-error-catalogue/tasks.md index 2ed8af506..4b296af41 100644 --- a/dev/specs/ifc-3034-error-catalogue/tasks.md +++ b/dev/specs/ifc-3034-error-catalogue/tasks.md @@ -65,14 +65,18 @@ and pin that invisibility before touching anything. so. The captured/not-parser-shaped rule stands for every fixture representing a real response. - [X] T003 Convert `infrahub_sdk/exceptions.py` into `infrahub_sdk/exceptions/base.py` by verbatim move (no behaviour edits in this task), and add `__all__` to it listing every class it defines. -- [X] T004 Create the façade `infrahub_sdk/exceptions/__init__.py` re-exporting with `from .base import *` - and nothing else yet. +- [X] T004 Create the façade `infrahub_sdk/exceptions/__init__.py` re-exporting every class `base` + defines, by explicit name rather than the `from .base import *` this task first said, and nothing + else yet. - [X] T005 [P] Add `"exceptions"` to `packages_to_ignore` in `tasks.py::get_modules_to_document`, so `docs-generate` does not fail with `Uncategorized packages under infrahub_sdk/` and `sdk_ref` output stays byte-identical. -- [X] T006 [P] Add the `per-file-ignores` entry for `infrahub_sdk/exceptions/__init__.py` in +- [X] T006 [P] ~~Add the `per-file-ignores` entry for `infrahub_sdk/exceptions/__init__.py` in `pyproject.toml` silencing `F403`/`F405`, with a comment giving the reason, mirroring the existing - `infrahub_sdk/schema/generated/*.py` entry. + `infrahub_sdk/schema/generated/*.py` entry.~~ **Superseded by T004 and T064**: the façade + re-exports by explicit name, so no `F403`/`F405` arises and no entry was added. What this + feature does add to `pyproject.toml` is its four pytest markers: `catalogue`, `crossversion`, + `malformed` and `message`. - [X] T007 Run `uv run pytest tests/unit/ -q` and `uv run invoke format lint-code docs-generate docs-validate` to confirm the restructure is invisible from outside the package. @@ -427,11 +431,14 @@ and confirm it passes (quickstart scenario 7). - [ ] T062 [US5] Add `error_catalogue == 'true'` to the `backend-validate-generated` job trigger in `[infrahub] .github/workflows/ci.yml`, so a hand-edit of the catalogue JSON alone cannot slip past. This is the only CI edit; no new path filter can match a file inside a submodule. -- [ ] T063 [US5] Run `uv run invoke backend.generate` from the Infrahub checkout and hand-verify the +- [X] T063 [US5] Run `uv run invoke backend.generate` from the Infrahub checkout and hand-verify the resulting `infrahub_sdk/exceptions/catalogue.py` in the SDK: nine generated classes, three adopted imports, fifteen payload models, and no class for the three 401/403 codes. -- [ ] T064 [US5] Commit the generated `infrahub_sdk/exceptions/catalogue.py` in the SDK repository and - extend the façade `infrahub_sdk/exceptions/__init__.py` with `from .catalogue import *`. +- [X] T064 [US5] Commit the generated `infrahub_sdk/exceptions/catalogue.py` in the SDK repository and + re-export its nine exception classes from the façade `infrahub_sdk/exceptions/__init__.py` **by + explicit name**. Not `from .catalogue import *`, as this task first said: a wildcard also promotes + the payload models, the lookup maps and the dispatch helper onto the package surface, where each + name becomes a stability promise the package never chose to make. - [ ] T065 [US5] Prove the negative from the Infrahub checkout: add a code to the backend catalogue, run `uv run invoke backend.export-error-catalogue` alone, confirm `backend.validate-generated` exits non-zero naming the stale artefact, then revert. @@ -450,39 +457,41 @@ 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 `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 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 + an `errors` array, the authentication branch for a response the SDK rejected before the query ran. + That is usually a 401 or 403, and also a token refresh that failed on any other status, which is + why the branch reads the status off the response rather than assuming one. This is the only rule + under which the three authentication codes reach the right class at all. +- [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 +500,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/`. diff --git a/docs/docs/python-sdk/topics/error_handling.mdx b/docs/docs/python-sdk/topics/error_handling.mdx new file mode 100644 index 000000000..a193bf5b4 --- /dev/null +++ b/docs/docs/python-sdk/topics/error_handling.mdx @@ -0,0 +1,201 @@ +--- +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 defines is importable from `infrahub_sdk.exceptions`. That is the supported +import path, and the only one: the modules beneath it are internal and their layout may change. A +call can still raise a built-in such as `ValueError` for an argument the SDK rejects before it builds +a request; what follows covers the exceptions the SDK defines. + +```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 request was rejected before it ran + └── GraphQLError a failure read from an `errors` array + ├── NodeNotFoundError + ├── BranchNotFoundError + ├── SchemaNotFoundError + ├── UniquenessViolationError + ├── UndefinedError + └── ... a class for most catalogued codes +``` + +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`. + +Most catalogued codes have a class; the three authentication codes deliberately do not, for the +reason below. `AuthenticationError` covers the responses the SDK rejected before the query ran, +which is usually an HTTP 401 or 403 but also a token refresh that failed on any other status. + +## Catching by branch, or by code + +| Intent | Clause | +|--------|--------| +| Anything the server rejected, on either transport | `except ApiError` | +| Any GraphQL-path failure | `except GraphQLError` | +| Any request the SDK saw rejected before it ran | `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: + + + + +```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) +``` + + + + +```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) +``` + + + + +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 codes a server reports for the failures it rejects a request on, so they are the ones that +routinely arrive on either transport, and each transport already has a 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, an error carrying no `extensions`, or an integer `code` on the wire, which is what a pre-catalogue server puts there. | +| `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. Where the envelope declares none, a class that declares its own supplies it - which the generated classes do and the three lookup-miss classes below do not, since they are also raised with no server behind them. | +| `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 the failed request carried, where the exception recorded them. `None` means the request was not recorded rather than that there was none - an authentication failure is observed at the transport, which has neither to hand. | + +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`. + +`UNDEFINED_ERROR` has a class like any other code, `UndefinedError`, so a failure the server could not +describe still raises a subclass of `GraphQLError` rather than `GraphQLError` itself. Match on +`except GraphQLError` or on `exc.code`, never on `type(exc) is GraphQLError`. + +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. Once the SDK has a decoded response in hand, reading the +catalogue out of it never raises: an unknown code, a payload that does not match what the catalogue +declares, and an envelope from a server that predates it all degrade to a class the caller can catch. +A body the SDK cannot decode as JSON at all is a separate failure and still raises `JsonDecodeError`, +before any of this runs. + +| Situation | Behaviour | +|-----------|-----------| +| A code this SDK has a class for, read from an `errors` array | That class, built from the payload the response carried | +| Any code at all, on a request the SDK saw rejected before it ran | `AuthenticationError`, with `exc.code` set | +| A code this SDK has never heard of | The generic class for the transport, 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, naming the code where the +response carried one, so an SDK 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. diff --git a/infrahub_sdk/exceptions/__init__.py b/infrahub_sdk/exceptions/__init__.py index 94b565290..369cb312b 100644 --- a/infrahub_sdk/exceptions/__init__.py +++ b/infrahub_sdk/exceptions/__init__.py @@ -1,9 +1,12 @@ """The supported import path for every SDK exception. -The class list below is written out rather than star-imported, so reading this file tells you what -the package exports. It has to be kept in step with `base.__all__` by hand; the tests in -`tests/unit/sdk/test_exceptions_public_names.py` fail if the two drift apart, or if a class defined -in `base` is left out of either. +The class lists below are written out rather than star-imported, so reading this file tells you what +the package exports. They have to be kept in step by hand with `base.__all__` and with the exception +classes `catalogue` generates; the tests in `tests/unit/sdk/test_exceptions_public_names.py` fail if +they drift apart, or if a class defined in `base` is left out of either. + +Only `catalogue`'s exception classes are re-exported. Its payload models, its lookup maps and its +dispatch helper are internal to the package and stay importable from the module itself. `__all__` is what `import *` hands a caller: the exception classes and nothing else. Without it the wildcard also carries the `base` and `factory` submodule names, which are an artefact of the layout. @@ -48,12 +51,28 @@ VersionNotSupportedError, ) from .base import code_names_the_failure as code_names_the_failure +from .catalogue import ( + AttributeConstraintViolationError, + AttributeInvalidTypeError, + AttributeRequiredError, + BranchAlreadyMergedError, + BranchNeedsRebaseError, + MergeInProgressError, + MergeRecoveryRequiredError, + UndefinedError, + UniquenessViolationError, +) from .factory import authentication_error_from_response as authentication_error_from_response from .factory import graphql_error_from_response as graphql_error_from_response __all__ = [ "ApiError", + "AttributeConstraintViolationError", + "AttributeInvalidTypeError", + "AttributeRequiredError", "AuthenticationError", + "BranchAlreadyMergedError", + "BranchNeedsRebaseError", "BranchNotFoundError", "CircularFragmentError", "DuplicateFragmentError", @@ -68,6 +87,8 @@ "InfrahubTransformNotFoundError", "InvalidResponseError", "JsonDecodeError", + "MergeInProgressError", + "MergeRecoveryRequiredError", "ModuleImportError", "NodeInvalidError", "NodeNotFoundError", @@ -82,7 +103,9 @@ "ServerNotResponsiveError", "TimestampFormatError", "URLNotFoundError", + "UndefinedError", "UninitializedError", + "UniquenessViolationError", "ValidationError", "VersionNotSupportedError", ] diff --git a/infrahub_sdk/exceptions/base.py b/infrahub_sdk/exceptions/base.py index 34c220c81..40a69b4f5 100644 --- a/infrahub_sdk/exceptions/base.py +++ b/infrahub_sdk/exceptions/base.py @@ -137,12 +137,33 @@ class ApiError(Error): http_status: int | None = None extensions: dict[str, Any] | None = None errors: Sequence[dict[str, Any]] = () - - -class GraphQLError(ApiError): + # `None` means the request that failed was not recorded on the exception, not that there was no + # query: an authentication failure is observed at the transport, where the class has neither. query: str | None = None variables: dict | None = None + +def _rebuild_graphql_error(cls: type[GraphQLError], args: tuple[Any, ...], state: dict[str, Any]) -> GraphQLError: + """Reconstruct a GraphQL-path exception without replaying its constructor. + + `BaseException.__reduce__` rebuilds by calling `cls(*args)`, where `args` is the message alone. + This class takes the server's error list first, so that call files the message under `errors` and + the message a caller reads comes back as the placeholder built from it. `args` is carried + separately from the rest of the state because it lives on the exception itself rather than in + `__dict__`, and `str()` reads it. + """ + exc = cls.__new__(cls) + exc.args = args + exc.__dict__.update(state) + return exc + + +def graphql_default_message(query: str | None, errors: Any) -> str: + """The message the GraphQL path produces where the server described nothing better.""" + return f"An error occurred while executing the GraphQL Query {query}, {errors}" + + +class GraphQLError(ApiError): def __init__( self, errors: list[dict[str, Any]], @@ -154,11 +175,14 @@ def __init__( self.variables = variables # `is not None` rather than `or`: an empty message is a deliberate one, not a request for # the default. - default = f"An error occurred while executing the GraphQL Query {query}, {errors}" + default = graphql_default_message(query=query, errors=errors) self.message = message if message is not None else default self.errors = as_error_list(errors) super().__init__(self.message) + def __reduce__(self) -> tuple[Any, ...]: + return (_rebuild_graphql_error, (type(self), self.args, self.__dict__)) + class VersionNotSupportedError(Error): """Raised when a feature is used against an Infrahub server version that does not support it.""" diff --git a/infrahub_sdk/exceptions/catalogue.py b/infrahub_sdk/exceptions/catalogue.py new file mode 100644 index 000000000..d2b8f0a73 --- /dev/null +++ b/infrahub_sdk/exceptions/catalogue.py @@ -0,0 +1,640 @@ +# Generated from schema/error-catalogue.json in the opsmill/infrahub repository - DO NOT EDIT. +# Catalogue version: 1 +# Regenerate there with: uv run invoke backend.generate +# +# Stability: a "stable" code keeps its payload shape; an "evolving" code may still gain fields. +from __future__ import annotations + +from collections.abc import Callable, Mapping +from datetime import datetime +from typing import Any, ClassVar + +from pydantic import BaseModel, ConfigDict +from typing_extensions import Self + +from .base import BranchNotFoundError, GraphQLError, NodeNotFoundError, SchemaNotFoundError + +__all__ = [ + "CODE_TO_DATA_MODEL", + "CODE_TO_EXCEPTION", + "AttributeConstraintViolationData", + "AttributeConstraintViolationError", + "AttributeInvalidTypeData", + "AttributeInvalidTypeError", + "AttributeRequiredData", + "AttributeRequiredError", + "AuthenticationRequiredData", + "BranchAlreadyMergedData", + "BranchAlreadyMergedError", + "BranchNeedsRebaseData", + "BranchNeedsRebaseError", + "BranchNotFoundData", + "MergeInProgressData", + "MergeInProgressError", + "MergeRecoveryRequiredData", + "MergeRecoveryRequiredError", + "NodeNotFoundData", + "PermissionDeniedData", + "SchemaNotFoundData", + "TokenExpiredData", + "UndefinedError", + "UndefinedErrorData", + "UniquenessViolationData", + "UniquenessViolationError", + "exception_from_payload", +] + + +class AttributeConstraintViolationData(BaseModel): + """Payload a server-reported ATTRIBUTE_CONSTRAINT_VIOLATION carries.""" + + # A newer server may add fields this SDK has never heard of; ignoring them keeps the code + # resolvable rather than failing validation. + model_config = ConfigDict(extra="ignore") + + node_kind: str + field_name: str + constraint: str + detail: str | None = None + + +class AttributeInvalidTypeData(BaseModel): + """Payload a server-reported ATTRIBUTE_INVALID_TYPE carries.""" + + # A newer server may add fields this SDK has never heard of; ignoring them keeps the code + # resolvable rather than failing validation. + model_config = ConfigDict(extra="ignore") + + node_kind: str + field_name: str + expected_type: str + received_type: str + + +class AttributeRequiredData(BaseModel): + """Payload a server-reported ATTRIBUTE_REQUIRED carries.""" + + # A newer server may add fields this SDK has never heard of; ignoring them keeps the code + # resolvable rather than failing validation. + model_config = ConfigDict(extra="ignore") + + node_kind: str + field_name: str + + +class AuthenticationRequiredData(BaseModel): + """Payload a server-reported AUTHENTICATION_REQUIRED carries.""" + + # A newer server may add fields this SDK has never heard of; ignoring them keeps the code + # resolvable rather than failing validation. + model_config = ConfigDict(extra="ignore") + + +class BranchAlreadyMergedData(BaseModel): + """Payload a server-reported BRANCH_ALREADY_MERGED carries.""" + + # A newer server may add fields this SDK has never heard of; ignoring them keeps the code + # resolvable rather than failing validation. + model_config = ConfigDict(extra="ignore") + + branch_name: str + + +class BranchNeedsRebaseData(BaseModel): + """Payload a server-reported BRANCH_NEEDS_REBASE carries.""" + + # A newer server may add fields this SDK has never heard of; ignoring them keeps the code + # resolvable rather than failing validation. + model_config = ConfigDict(extra="ignore") + + branch_name: str + + +class BranchNotFoundData(BaseModel): + """Payload a server-reported BRANCH_NOT_FOUND carries.""" + + # A newer server may add fields this SDK has never heard of; ignoring them keeps the code + # resolvable rather than failing validation. + model_config = ConfigDict(extra="ignore") + + branch_name: str + + +class MergeInProgressData(BaseModel): + """Payload a server-reported MERGE_IN_PROGRESS carries.""" + + # A newer server may add fields this SDK has never heard of; ignoring them keeps the code + # resolvable rather than failing validation. + model_config = ConfigDict(extra="ignore") + + branch_name: str + merging_branch: str + + +class MergeRecoveryRequiredData(BaseModel): + """Payload a server-reported MERGE_RECOVERY_REQUIRED carries.""" + + # A newer server may add fields this SDK has never heard of; ignoring them keeps the code + # resolvable rather than failing validation. + model_config = ConfigDict(extra="ignore") + + branch_name: str + merging_branch: str + + +class NodeNotFoundData(BaseModel): + """Payload a server-reported NODE_NOT_FOUND carries.""" + + # A newer server may add fields this SDK has never heard of; ignoring them keeps the code + # resolvable rather than failing validation. + model_config = ConfigDict(extra="ignore") + + node_kind: str + identifier: str + + +class PermissionDeniedData(BaseModel): + """Payload a server-reported PERMISSION_DENIED carries.""" + + # A newer server may add fields this SDK has never heard of; ignoring them keeps the code + # resolvable rather than failing validation. + model_config = ConfigDict(extra="ignore") + + action: str | None = None + resource_kind: str | None = None + + +class SchemaNotFoundData(BaseModel): + """Payload a server-reported SCHEMA_NOT_FOUND carries.""" + + # A newer server may add fields this SDK has never heard of; ignoring them keeps the code + # resolvable rather than failing validation. + model_config = ConfigDict(extra="ignore") + + kind: str + + +class TokenExpiredData(BaseModel): + """Payload a server-reported TOKEN_EXPIRED carries.""" + + # A newer server may add fields this SDK has never heard of; ignoring them keeps the code + # resolvable rather than failing validation. + model_config = ConfigDict(extra="ignore") + + expired_at: datetime | None = None + + +class UndefinedErrorData(BaseModel): + """Payload a server-reported UNDEFINED_ERROR carries.""" + + # A newer server may add fields this SDK has never heard of; ignoring them keeps the code + # resolvable rather than failing validation. + model_config = ConfigDict(extra="ignore") + + +class UniquenessViolationData(BaseModel): + """Payload a server-reported UNIQUENESS_VIOLATION carries.""" + + # A newer server may add fields this SDK has never heard of; ignoring them keeps the code + # resolvable rather than failing validation. + model_config = ConfigDict(extra="ignore") + + node_kind: str + fields: list[str] + + +class AttributeConstraintViolationError(GraphQLError): + """Raised when the server reports ATTRIBUTE_CONSTRAINT_VIOLATION. + + A node attribute value failed a schema-defined constraint (e.g. regex, length, range). + + Stability: evolving. + """ + + CODE: ClassVar[str | None] = "ATTRIBUTE_CONSTRAINT_VIOLATION" + DATA_MODEL: ClassVar[type[BaseModel]] = AttributeConstraintViolationData + + code: str | None = "ATTRIBUTE_CONSTRAINT_VIOLATION" + http_status: int | None = 422 + + def __init__( + self, + *, + node_kind: str, + field_name: str, + constraint: str, + detail: str | None = None, + errors: list[dict[str, Any]] | None = None, + query: str | None = None, + variables: dict | None = None, + message: str | None = None, + ) -> None: + self.node_kind = node_kind + self.field_name = field_name + self.constraint = constraint + self.detail = detail + super().__init__(errors=errors or [], query=query, variables=variables, message=message) + + @classmethod + def from_payload(cls, payload: AttributeConstraintViolationData) -> Self: + """Build the exception from the validated payload of a server-reported failure.""" + return cls( + node_kind=payload.node_kind, + field_name=payload.field_name, + constraint=payload.constraint, + detail=payload.detail, + ) + + +class AttributeInvalidTypeError(GraphQLError): + """Raised when the server reports ATTRIBUTE_INVALID_TYPE. + + A node attribute received a value that does not match its declared type. + + Stability: stable. + """ + + CODE: ClassVar[str | None] = "ATTRIBUTE_INVALID_TYPE" + DATA_MODEL: ClassVar[type[BaseModel]] = AttributeInvalidTypeData + + code: str | None = "ATTRIBUTE_INVALID_TYPE" + http_status: int | None = 422 + + def __init__( + self, + *, + node_kind: str, + field_name: str, + expected_type: str, + received_type: str, + errors: list[dict[str, Any]] | None = None, + query: str | None = None, + variables: dict | None = None, + message: str | None = None, + ) -> None: + self.node_kind = node_kind + self.field_name = field_name + self.expected_type = expected_type + self.received_type = received_type + super().__init__(errors=errors or [], query=query, variables=variables, message=message) + + @classmethod + def from_payload(cls, payload: AttributeInvalidTypeData) -> Self: + """Build the exception from the validated payload of a server-reported failure.""" + return cls( + node_kind=payload.node_kind, + field_name=payload.field_name, + expected_type=payload.expected_type, + received_type=payload.received_type, + ) + + +class AttributeRequiredError(GraphQLError): + """Raised when the server reports ATTRIBUTE_REQUIRED. + + A mandatory node attribute was not provided. + + Stability: stable. + """ + + CODE: ClassVar[str | None] = "ATTRIBUTE_REQUIRED" + DATA_MODEL: ClassVar[type[BaseModel]] = AttributeRequiredData + + code: str | None = "ATTRIBUTE_REQUIRED" + http_status: int | None = 422 + + def __init__( + self, + *, + node_kind: str, + field_name: str, + errors: list[dict[str, Any]] | None = None, + query: str | None = None, + variables: dict | None = None, + message: str | None = None, + ) -> None: + self.node_kind = node_kind + self.field_name = field_name + super().__init__(errors=errors or [], query=query, variables=variables, message=message) + + @classmethod + def from_payload(cls, payload: AttributeRequiredData) -> Self: + """Build the exception from the validated payload of a server-reported failure.""" + return cls(node_kind=payload.node_kind, field_name=payload.field_name) + + +class BranchAlreadyMergedError(GraphQLError): + """Raised when the server reports BRANCH_ALREADY_MERGED. + + The target branch has been merged and is permanently read-only. + + Stability: stable. + """ + + CODE: ClassVar[str | None] = "BRANCH_ALREADY_MERGED" + DATA_MODEL: ClassVar[type[BaseModel]] = BranchAlreadyMergedData + + code: str | None = "BRANCH_ALREADY_MERGED" + http_status: int | None = 400 + + def __init__( + self, + *, + branch_name: str, + errors: list[dict[str, Any]] | None = None, + query: str | None = None, + variables: dict | None = None, + message: str | None = None, + ) -> None: + self.branch_name = branch_name + super().__init__(errors=errors or [], query=query, variables=variables, message=message) + + @classmethod + def from_payload(cls, payload: BranchAlreadyMergedData) -> Self: + """Build the exception from the validated payload of a server-reported failure.""" + return cls(branch_name=payload.branch_name) + + +class BranchNeedsRebaseError(GraphQLError): + """Raised when the server reports BRANCH_NEEDS_REBASE. + + The target branch must be rebased before it accepts new changes. + + Stability: stable. + """ + + CODE: ClassVar[str | None] = "BRANCH_NEEDS_REBASE" + DATA_MODEL: ClassVar[type[BaseModel]] = BranchNeedsRebaseData + + code: str | None = "BRANCH_NEEDS_REBASE" + http_status: int | None = 400 + + def __init__( + self, + *, + branch_name: str, + errors: list[dict[str, Any]] | None = None, + query: str | None = None, + variables: dict | None = None, + message: str | None = None, + ) -> None: + self.branch_name = branch_name + super().__init__(errors=errors or [], query=query, variables=variables, message=message) + + @classmethod + def from_payload(cls, payload: BranchNeedsRebaseData) -> Self: + """Build the exception from the validated payload of a server-reported failure.""" + return cls(branch_name=payload.branch_name) + + +class MergeInProgressError(GraphQLError): + """Raised when the server reports MERGE_IN_PROGRESS. + + The write was rejected because a branch merge is in progress. The block is transient; retry once the + merge completes. + + Stability: evolving. + """ + + CODE: ClassVar[str | None] = "MERGE_IN_PROGRESS" + DATA_MODEL: ClassVar[type[BaseModel]] = MergeInProgressData + + code: str | None = "MERGE_IN_PROGRESS" + http_status: int | None = 423 + + def __init__( + self, + *, + branch_name: str, + merging_branch: str, + errors: list[dict[str, Any]] | None = None, + query: str | None = None, + variables: dict | None = None, + message: str | None = None, + ) -> None: + self.branch_name = branch_name + self.merging_branch = merging_branch + super().__init__(errors=errors or [], query=query, variables=variables, message=message) + + @classmethod + def from_payload(cls, payload: MergeInProgressData) -> Self: + """Build the exception from the validated payload of a server-reported failure.""" + return cls(branch_name=payload.branch_name, merging_branch=payload.merging_branch) + + +class MergeRecoveryRequiredError(GraphQLError): + """Raised when the server reports MERGE_RECOVERY_REQUIRED. + + The write was rejected because a previous branch merge failed and left the default branch protected. + Recovery is required: an administrator must run `infrahub recover merge`. Unlike MERGE_IN_PROGRESS + this is not retryable. + + Stability: evolving. + """ + + CODE: ClassVar[str | None] = "MERGE_RECOVERY_REQUIRED" + DATA_MODEL: ClassVar[type[BaseModel]] = MergeRecoveryRequiredData + + code: str | None = "MERGE_RECOVERY_REQUIRED" + http_status: int | None = 423 + + def __init__( + self, + *, + branch_name: str, + merging_branch: str, + errors: list[dict[str, Any]] | None = None, + query: str | None = None, + variables: dict | None = None, + message: str | None = None, + ) -> None: + self.branch_name = branch_name + self.merging_branch = merging_branch + super().__init__(errors=errors or [], query=query, variables=variables, message=message) + + @classmethod + def from_payload(cls, payload: MergeRecoveryRequiredData) -> Self: + """Build the exception from the validated payload of a server-reported failure.""" + return cls(branch_name=payload.branch_name, merging_branch=payload.merging_branch) + + +class UndefinedError(GraphQLError): + """Raised when the server reports UNDEFINED_ERROR. + + An error not yet covered by the catalogue. Its occurrence indicates a catalogue gap and should be + triaged. + + Stability: stable. + """ + + CODE: ClassVar[str | None] = "UNDEFINED_ERROR" + DATA_MODEL: ClassVar[type[BaseModel]] = UndefinedErrorData + + code: str | None = "UNDEFINED_ERROR" + http_status: int | None = 500 + + def __init__( + self, + *, + errors: list[dict[str, Any]] | None = None, + query: str | None = None, + variables: dict | None = None, + message: str | None = None, + ) -> None: + super().__init__(errors=errors or [], query=query, variables=variables, message=message) + + @classmethod + def from_payload(cls, payload: UndefinedErrorData) -> Self: + """Build the exception from the validated payload of a server-reported failure.""" + del payload + return cls() + + +class UniquenessViolationError(GraphQLError): + """Raised when the server reports UNIQUENESS_VIOLATION. + + The submitted values collide with an existing object on a uniqueness constraint. The constraint members + may be relationships as well as attributes. + + Stability: evolving. + """ + + CODE: ClassVar[str | None] = "UNIQUENESS_VIOLATION" + DATA_MODEL: ClassVar[type[BaseModel]] = UniquenessViolationData + + code: str | None = "UNIQUENESS_VIOLATION" + http_status: int | None = 422 + + def __init__( + self, + *, + node_kind: str, + fields: list[str], + errors: list[dict[str, Any]] | None = None, + query: str | None = None, + variables: dict | None = None, + message: str | None = None, + ) -> None: + self.node_kind = node_kind + self.fields = fields + super().__init__(errors=errors or [], query=query, variables=variables, message=message) + + @classmethod + def from_payload(cls, payload: UniquenessViolationData) -> Self: + """Build the exception from the validated payload of a server-reported failure.""" + return cls(node_kind=payload.node_kind, fields=payload.fields) + + +# Codes that map to a dedicated exception class. Authentication and permission codes are absent: the +# SDK raises a generic class for those and carries the code on the instance. +CODE_TO_EXCEPTION: dict[str, type[GraphQLError]] = { + "ATTRIBUTE_CONSTRAINT_VIOLATION": AttributeConstraintViolationError, + "ATTRIBUTE_INVALID_TYPE": AttributeInvalidTypeError, + "ATTRIBUTE_REQUIRED": AttributeRequiredError, + "BRANCH_ALREADY_MERGED": BranchAlreadyMergedError, + "BRANCH_NEEDS_REBASE": BranchNeedsRebaseError, + "BRANCH_NOT_FOUND": BranchNotFoundError, + "MERGE_IN_PROGRESS": MergeInProgressError, + "MERGE_RECOVERY_REQUIRED": MergeRecoveryRequiredError, + "NODE_NOT_FOUND": NodeNotFoundError, + "SCHEMA_NOT_FOUND": SchemaNotFoundError, + "UNDEFINED_ERROR": UndefinedError, + "UNIQUENESS_VIOLATION": UniquenessViolationError, +} + +# Every code's payload model, including the codes that get no class, so a caller that observes one +# can still validate what it carries. +CODE_TO_DATA_MODEL: dict[str, type[BaseModel]] = { + "ATTRIBUTE_CONSTRAINT_VIOLATION": AttributeConstraintViolationData, + "ATTRIBUTE_INVALID_TYPE": AttributeInvalidTypeData, + "ATTRIBUTE_REQUIRED": AttributeRequiredData, + "AUTHENTICATION_REQUIRED": AuthenticationRequiredData, + "BRANCH_ALREADY_MERGED": BranchAlreadyMergedData, + "BRANCH_NEEDS_REBASE": BranchNeedsRebaseData, + "BRANCH_NOT_FOUND": BranchNotFoundData, + "MERGE_IN_PROGRESS": MergeInProgressData, + "MERGE_RECOVERY_REQUIRED": MergeRecoveryRequiredData, + "NODE_NOT_FOUND": NodeNotFoundData, + "PERMISSION_DENIED": PermissionDeniedData, + "SCHEMA_NOT_FOUND": SchemaNotFoundData, + "TOKEN_EXPIRED": TokenExpiredData, + "UNDEFINED_ERROR": UndefinedErrorData, + "UNIQUENESS_VIOLATION": UniquenessViolationData, +} + + +def _build_attribute_constraint_violation(data: Mapping[str, Any]) -> GraphQLError: + return AttributeConstraintViolationError.from_payload(AttributeConstraintViolationData.model_validate(data)) + + +def _build_attribute_invalid_type(data: Mapping[str, Any]) -> GraphQLError: + return AttributeInvalidTypeError.from_payload(AttributeInvalidTypeData.model_validate(data)) + + +def _build_attribute_required(data: Mapping[str, Any]) -> GraphQLError: + return AttributeRequiredError.from_payload(AttributeRequiredData.model_validate(data)) + + +def _build_branch_already_merged(data: Mapping[str, Any]) -> GraphQLError: + return BranchAlreadyMergedError.from_payload(BranchAlreadyMergedData.model_validate(data)) + + +def _build_branch_needs_rebase(data: Mapping[str, Any]) -> GraphQLError: + return BranchNeedsRebaseError.from_payload(BranchNeedsRebaseData.model_validate(data)) + + +def _build_branch_not_found(data: Mapping[str, Any]) -> GraphQLError: + return BranchNotFoundError.from_payload(BranchNotFoundData.model_validate(data)) + + +def _build_merge_in_progress(data: Mapping[str, Any]) -> GraphQLError: + return MergeInProgressError.from_payload(MergeInProgressData.model_validate(data)) + + +def _build_merge_recovery_required(data: Mapping[str, Any]) -> GraphQLError: + return MergeRecoveryRequiredError.from_payload(MergeRecoveryRequiredData.model_validate(data)) + + +def _build_node_not_found(data: Mapping[str, Any]) -> GraphQLError: + return NodeNotFoundError.from_payload(NodeNotFoundData.model_validate(data)) + + +def _build_schema_not_found(data: Mapping[str, Any]) -> GraphQLError: + return SchemaNotFoundError.from_payload(SchemaNotFoundData.model_validate(data)) + + +def _build_undefined_error(data: Mapping[str, Any]) -> GraphQLError: + return UndefinedError.from_payload(UndefinedErrorData.model_validate(data)) + + +def _build_uniqueness_violation(data: Mapping[str, Any]) -> GraphQLError: + return UniquenessViolationError.from_payload(UniquenessViolationData.model_validate(data)) + + +# Each builder validates against its own code's model, so the payload type is never widened on the +# way to the constructor that consumes it. +_CODE_TO_BUILDER: dict[str, Callable[[Mapping[str, Any]], GraphQLError]] = { + "ATTRIBUTE_CONSTRAINT_VIOLATION": _build_attribute_constraint_violation, + "ATTRIBUTE_INVALID_TYPE": _build_attribute_invalid_type, + "ATTRIBUTE_REQUIRED": _build_attribute_required, + "BRANCH_ALREADY_MERGED": _build_branch_already_merged, + "BRANCH_NEEDS_REBASE": _build_branch_needs_rebase, + "BRANCH_NOT_FOUND": _build_branch_not_found, + "MERGE_IN_PROGRESS": _build_merge_in_progress, + "MERGE_RECOVERY_REQUIRED": _build_merge_recovery_required, + "NODE_NOT_FOUND": _build_node_not_found, + "SCHEMA_NOT_FOUND": _build_schema_not_found, + "UNDEFINED_ERROR": _build_undefined_error, + "UNIQUENESS_VIOLATION": _build_uniqueness_violation, +} + + +def exception_from_payload(code: str, data: Mapping[str, Any]) -> GraphQLError | None: + """Build the exception a catalogued code names, or None where the SDK has no class for it. + + Raises: + pydantic.ValidationError: when the payload does not match what the code declares. + + """ + builder = _CODE_TO_BUILDER.get(code) + return None if builder is None else builder(data) diff --git a/infrahub_sdk/exceptions/factory.py b/infrahub_sdk/exceptions/factory.py index ec12901df..183d7aa59 100644 --- a/infrahub_sdk/exceptions/factory.py +++ b/infrahub_sdk/exceptions/factory.py @@ -9,19 +9,20 @@ from __future__ import annotations import logging -from dataclasses import dataclass +from collections.abc import Mapping from typing import TYPE_CHECKING, Any +from pydantic import ValidationError as PayloadValidationError + from .base import ( AuthenticationError, - BranchNotFoundError, Error, GraphQLError, - NodeNotFoundError, - SchemaNotFoundError, as_error_list, code_names_the_failure, + graphql_default_message, ) +from .catalogue import exception_from_payload if TYPE_CHECKING: import httpx @@ -93,14 +94,10 @@ def _governing_message(errors: Any) -> str: def _named_by_code(code: str, message: str) -> str: - """The message for a described failure: the code and the server's message, and no query text. - - A described failure is one the server's catalogue has an entry for, so its own words are what the - reader needs; the query stays on the exception as an attribute. + """The message for a described failure: the code and the server's own words, and no query text. - Callers apply this only when the governing error carried a message. A code with nothing beside it - is a worse headline than whatever that transport would otherwise have produced, and it is no loss - of information: the code is on `exc.code` either way. + Callers apply this only where the governing error carried a message, since a bare code is a + worse headline than what the transport would otherwise produce. The query stays on `exc.query`. """ return f"{code}: {message}" @@ -115,81 +112,60 @@ def _replace_message(exc: Error, message: str) -> None: exc.args = (message,) -@dataclass -class _NodeNotFoundData: - """Stands in for the generated payload model, which the SDK does not carry yet. +def _payload_of(extensions: dict[str, Any] | None) -> Mapping[str, Any]: + """The governing error's payload, or an empty one where the envelope carried none. - Plain rather than frozen, because the payload protocols declare settable attributes: the shape - the generated pydantic models will have. + An absent payload is not the same as a malformed one: a code whose fields are all optional still + resolves to its class, while one with required fields fails the validation below and falls back. """ + data = extensions.get("data") if extensions is not None else None + return data if isinstance(data, Mapping) else {} - node_kind: str - identifier: str - - -@dataclass -class _BranchNotFoundData: - branch_name: str - - -@dataclass -class _SchemaNotFoundData: - kind: str +def _catalogued_exception(code: str | None, extensions: dict[str, Any] | None) -> GraphQLError | None: + """The class the catalogue binds `code` to, built from the payload the envelope carries. -def _payload_strings(data: Any, names: tuple[str, ...]) -> dict[str, str] | None: - """The named payload fields, when every one of them is present as a string. + Resolution and validation both belong to the generated bindings: each code's payload is validated + against its own model and handed to that class's `from_payload`, so the factory never assembles + an attribute itself and never has to widen a payload type to reach a constructor. - A payload that violates the catalogue's own contract yields `None` so the caller falls back to - the generic class, rather than a TypeError raised from inside the SDK while the caller is - already failing. + `None` means no class was resolved - the code has none, or its payload violates what the + catalogue declares for it - and the caller then raises the generic class for the transport it + observed, with the code still readable. """ - if not isinstance(data, dict): + if code is None: return None - values = {name: data.get(name) for name in names} - if any(not isinstance(value, str) for value in values.values()): + try: + exception = exception_from_payload(code=code, data=_payload_of(extensions)) + except PayloadValidationError: + # The caller is already failing, so a validation error from inside the SDK would replace the + # server's reason with one of the SDK's own. + LOGGER.debug("Payload for %s does not match what the catalogue declares: %r", code, extensions, exc_info=True) return None - return {name: value for name, value in values.items() if isinstance(value, str)} + if exception is None: + LOGGER.debug("These bindings have no class for the catalogue code %s: %r", code, extensions) + return exception -def _adopted_exception(code: str | None, extensions: dict[str, Any] | None) -> GraphQLError | None: - """The class that adopted `code`, built from the payload the envelope carries. +def _has_no_sentence_of_its_own(exc: GraphQLError) -> bool: + """Whether the class supplied no message of its own when it built itself from its payload. - Only the three codes the SDK already ships a class for are resolved here, and each class maps the - payload itself through its own `from_payload`. Every other code raises the generic class for the - transport with the code readable on `exc.code`; turning the rest into classes of their own is - what the generated bindings buy. - - `None` means no class was resolved, whether because the code has none or because its payload did - not carry the fields the class needs. + A class that predates the catalogue writes its own sentence; one bound to a code takes the + GraphQL default, which names the query and errors it was given - and `from_payload` gives it + neither. Asking the instance rather than a fixed string keeps that true if a class ever starts + passing them. """ - if code is None: - return None - data = extensions.get("data") if extensions is not None else None - - if code == NodeNotFoundError.CODE: - node = _payload_strings(data=data, names=("node_kind", "identifier")) - if node is not None: - payload = _NodeNotFoundData(node_kind=node["node_kind"], identifier=node["identifier"]) - return NodeNotFoundError.from_payload(payload=payload) - elif code == BranchNotFoundError.CODE: - branch = _payload_strings(data=data, names=("branch_name",)) - if branch is not None: - return BranchNotFoundError.from_payload(payload=_BranchNotFoundData(branch_name=branch["branch_name"])) - elif code == SchemaNotFoundError.CODE: - schema = _payload_strings(data=data, names=("kind",)) - if schema is not None: - return SchemaNotFoundError.from_payload(payload=_SchemaNotFoundData(kind=schema["kind"])) - else: - return None + return exc.message == graphql_default_message(query=exc.query, errors=exc.errors) - LOGGER.debug("Payload for %s does not carry the fields its class needs: %r", code, data) - return None +def _log_unresolved_code(code: str | None, extensions: dict[str, Any] | None, source: str) -> None: + """Record a response the SDK read no catalogue code from, whatever the reason. -def _log_unresolved_code(extensions: dict[str, Any] | None, source: str) -> None: - if extensions is not None and _catalogue_code(extensions) is None: - LOGGER.debug("No catalogue code resolved from %s error extensions: %r", source, extensions.get("code")) + The whole `extensions` mapping is logged rather than its `code` alone, since the commonest shapes + here carry no `code` at all and the rest of the mapping is what identifies the server. + """ + if code is None: + LOGGER.debug("No catalogue code resolved from %s error extensions: %r", source, extensions) def token_expired_in(errors: Any) -> bool: @@ -215,20 +191,26 @@ def graphql_error_from_response( """Build the exception for an `errors` array returned on the GraphQL path. `errors` is raw decoded JSON, so it is read defensively. The complete list is retained - unreordered. A code the SDK has adopted a class for raises that class; every other failure raises + unreordered. A code the catalogue binds to a class raises that class; every other failure raises `GraphQLError`, and one the server's catalogue could not describe keeps the message this call site has always produced, query text included. + + This is the GraphQL branch, so its fallback is `GraphQLError` whatever status the code declares. + A code that reaches an `errors` array was read off this transport, and a declared 401 does not + make it an authentication failure the SDK observed. """ extensions = _first_extensions(errors) code = _catalogue_code(extensions) governing = _governing_message(errors) message = _named_by_code(code, governing) if code_names_the_failure(code) and governing else None - adopted = _adopted_exception(code=code, extensions=extensions) - if adopted is not None: - exc: GraphQLError = adopted - # An adopted class builds itself from its payload alone, so the envelope it came out of is - # attached here. A silent governing error leaves `message` None, and the class its own text. + catalogued = _catalogued_exception(code=code, extensions=extensions) + if catalogued is not None: + exc: GraphQLError = catalogued + # An undescribed failure leaves a class with its own sentence holding it, and one without + # holding a placeholder that names neither the query nor the errors it was never given. + if message is None and _has_no_sentence_of_its_own(exc): + message = graphql_default_message(query=query, errors=errors) if message is not None: _replace_message(exc, message) exc.query = query @@ -238,9 +220,13 @@ def graphql_error_from_response( exc.errors = as_error_list(errors) exc.code = code - exc.http_status = _declared_http_status(extensions) + declared_status = _declared_http_status(extensions) + if declared_status is not None: + # Conditional so a class that declares the catalogue's status keeps it where the envelope + # omitted one. + exc.http_status = declared_status exc.extensions = extensions - _log_unresolved_code(extensions=extensions, source="GraphQL") + _log_unresolved_code(code=code, extensions=extensions, source="GraphQL") return exc @@ -256,6 +242,10 @@ def authentication_error_from_response(response: httpx.Response) -> Authenticati A described failure names its code and the governing error's message instead, on the same rule as the GraphQL path: only the error the code came from may be named beside it, and the complete list stays on `exc.errors`. + + No code is resolved to a class here. Every catalogued class descends from `GraphQLError`, so + raising one for a response the SDK observed as an authentication failure would put it out of + reach of `except AuthenticationError`. """ errors: Any = [] message = f"HTTP {response.status_code}" @@ -288,5 +278,5 @@ def authentication_error_from_response(response: httpx.Response) -> Authenticati exc.http_status = _declared_http_status(extensions) exc.extensions = extensions exc.errors = as_error_list(errors) - _log_unresolved_code(extensions=extensions, source="authentication") + _log_unresolved_code(code=code, extensions=extensions, source="authentication") return exc diff --git a/pyproject.toml b/pyproject.toml index 74978bf82..849072cfe 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -137,6 +137,7 @@ filterwarnings = [ "ignore:Deprecated call to", ] markers = [ + "catalogue: Drives a catalogued failure through a real server, proving the envelope shape", "crossversion: Parses an envelope from a server whose version differs from the SDK's", "malformed: Parses an envelope that violates the shape the SDK expects", "message: Pins an exception's message text, in either the catalogued or the uncatalogued direction", diff --git a/tests/fixtures/error_catalogue/README.md b/tests/fixtures/error_catalogue/README.md index 43c051e08..54f8038d7 100644 --- a/tests/fixtures/error_catalogue/README.md +++ b/tests/fixtures/error_catalogue/README.md @@ -21,6 +21,14 @@ The `malformed_*` files are the exception: they are constructed, because a corre produce them. They stand in for a proxy, a gateway, or a future server that answers in a shape the SDK does not expect, and they exist to pin that such a shape degrades rather than raising. +`codes/` holds one envelope per catalogue code, named after the code in lower case, each carrying the +payload that code declares. They are the exhaustive set: a code without a file here is a code nothing +proves the SDK can resolve, which is why the test that reads them addresses each file by code rather +than listing the directory, and separately pins its case list equal to the bindings' own code map. +All of them are shaped as a GraphQL response, including the three +authentication codes, which reach that transport whenever a resolver rather than the request pipeline +raised them. + `public_names.json` is not an envelope. It is the committed snapshot of every exception class importable from `infrahub_sdk.exceptions`, which pins that restructuring the module into a package stays invisible from outside. It lists exception classes only; incidental typing imports that the diff --git a/tests/fixtures/error_catalogue/codes/attribute_constraint_violation.json b/tests/fixtures/error_catalogue/codes/attribute_constraint_violation.json new file mode 100644 index 000000000..c8eb60a4a --- /dev/null +++ b/tests/fixtures/error_catalogue/codes/attribute_constraint_violation.json @@ -0,0 +1,27 @@ +{ + "data": null, + "errors": [ + { + "message": "Value for TestPerson.name does not conform to the regex '^[A-Z]'", + "locations": [ + { + "line": 2, + "column": 3 + } + ], + "path": [ + "TestPersonCreate" + ], + "extensions": { + "code": "ATTRIBUTE_CONSTRAINT_VIOLATION", + "http_status": 422, + "data": { + "node_kind": "TestPerson", + "field_name": "name", + "constraint": "regex", + "detail": "^[A-Z]" + } + } + } + ] +} diff --git a/tests/fixtures/error_catalogue/codes/attribute_invalid_type.json b/tests/fixtures/error_catalogue/codes/attribute_invalid_type.json new file mode 100644 index 000000000..ffe04011e --- /dev/null +++ b/tests/fixtures/error_catalogue/codes/attribute_invalid_type.json @@ -0,0 +1,27 @@ +{ + "data": null, + "errors": [ + { + "message": "Value for TestPerson.height must be of type Integer, received String", + "locations": [ + { + "line": 2, + "column": 3 + } + ], + "path": [ + "TestPersonCreate" + ], + "extensions": { + "code": "ATTRIBUTE_INVALID_TYPE", + "http_status": 422, + "data": { + "node_kind": "TestPerson", + "field_name": "height", + "expected_type": "Integer", + "received_type": "String" + } + } + } + ] +} diff --git a/tests/fixtures/error_catalogue/codes/attribute_required.json b/tests/fixtures/error_catalogue/codes/attribute_required.json new file mode 100644 index 000000000..20c5e005c --- /dev/null +++ b/tests/fixtures/error_catalogue/codes/attribute_required.json @@ -0,0 +1,25 @@ +{ + "data": null, + "errors": [ + { + "message": "A value is required for TestPerson.name", + "locations": [ + { + "line": 2, + "column": 3 + } + ], + "path": [ + "TestPersonCreate" + ], + "extensions": { + "code": "ATTRIBUTE_REQUIRED", + "http_status": 422, + "data": { + "node_kind": "TestPerson", + "field_name": "name" + } + } + } + ] +} diff --git a/tests/fixtures/error_catalogue/codes/authentication_required.json b/tests/fixtures/error_catalogue/codes/authentication_required.json new file mode 100644 index 000000000..c49b79209 --- /dev/null +++ b/tests/fixtures/error_catalogue/codes/authentication_required.json @@ -0,0 +1,22 @@ +{ + "data": null, + "errors": [ + { + "message": "Authentication is required to perform this operation", + "locations": [ + { + "line": 2, + "column": 3 + } + ], + "path": [ + "TestPersonCreate" + ], + "extensions": { + "code": "AUTHENTICATION_REQUIRED", + "http_status": 401, + "data": {} + } + } + ] +} diff --git a/tests/fixtures/error_catalogue/codes/branch_already_merged.json b/tests/fixtures/error_catalogue/codes/branch_already_merged.json new file mode 100644 index 000000000..11a4ba7fa --- /dev/null +++ b/tests/fixtures/error_catalogue/codes/branch_already_merged.json @@ -0,0 +1,24 @@ +{ + "data": null, + "errors": [ + { + "message": "Branch: feature-a has already been merged and is read-only.", + "locations": [ + { + "line": 2, + "column": 3 + } + ], + "path": [ + "TestPersonCreate" + ], + "extensions": { + "code": "BRANCH_ALREADY_MERGED", + "http_status": 400, + "data": { + "branch_name": "feature-a" + } + } + } + ] +} diff --git a/tests/fixtures/error_catalogue/codes/branch_needs_rebase.json b/tests/fixtures/error_catalogue/codes/branch_needs_rebase.json new file mode 100644 index 000000000..7b1268ba2 --- /dev/null +++ b/tests/fixtures/error_catalogue/codes/branch_needs_rebase.json @@ -0,0 +1,24 @@ +{ + "data": null, + "errors": [ + { + "message": "Branch: feature-a must be rebased before it accepts new changes.", + "locations": [ + { + "line": 2, + "column": 3 + } + ], + "path": [ + "TestPersonCreate" + ], + "extensions": { + "code": "BRANCH_NEEDS_REBASE", + "http_status": 400, + "data": { + "branch_name": "feature-a" + } + } + } + ] +} diff --git a/tests/fixtures/error_catalogue/codes/branch_not_found.json b/tests/fixtures/error_catalogue/codes/branch_not_found.json new file mode 100644 index 000000000..c88dfbab8 --- /dev/null +++ b/tests/fixtures/error_catalogue/codes/branch_not_found.json @@ -0,0 +1,21 @@ +{ + "data": null, + "errors": [ + { + "message": "Branch: does-not-exist not found.", + "locations": [ + { + "line": 2, + "column": 3 + } + ], + "extensions": { + "code": "BRANCH_NOT_FOUND", + "http_status": 400, + "data": { + "branch_name": "does-not-exist" + } + } + } + ] +} diff --git a/tests/fixtures/error_catalogue/codes/merge_in_progress.json b/tests/fixtures/error_catalogue/codes/merge_in_progress.json new file mode 100644 index 000000000..3494578d6 --- /dev/null +++ b/tests/fixtures/error_catalogue/codes/merge_in_progress.json @@ -0,0 +1,25 @@ +{ + "data": null, + "errors": [ + { + "message": "Branch main is locked while feature-a is being merged into it.", + "locations": [ + { + "line": 2, + "column": 3 + } + ], + "path": [ + "TestPersonCreate" + ], + "extensions": { + "code": "MERGE_IN_PROGRESS", + "http_status": 423, + "data": { + "branch_name": "main", + "merging_branch": "feature-a" + } + } + } + ] +} diff --git a/tests/fixtures/error_catalogue/codes/merge_recovery_required.json b/tests/fixtures/error_catalogue/codes/merge_recovery_required.json new file mode 100644 index 000000000..b7bd7e704 --- /dev/null +++ b/tests/fixtures/error_catalogue/codes/merge_recovery_required.json @@ -0,0 +1,25 @@ +{ + "data": null, + "errors": [ + { + "message": "Branch main is protected after a failed merge of feature-a. Run `infrahub recover merge`.", + "locations": [ + { + "line": 2, + "column": 3 + } + ], + "path": [ + "TestPersonCreate" + ], + "extensions": { + "code": "MERGE_RECOVERY_REQUIRED", + "http_status": 423, + "data": { + "branch_name": "main", + "merging_branch": "feature-a" + } + } + } + ] +} diff --git a/tests/fixtures/error_catalogue/codes/node_not_found.json b/tests/fixtures/error_catalogue/codes/node_not_found.json new file mode 100644 index 000000000..b606ed061 --- /dev/null +++ b/tests/fixtures/error_catalogue/codes/node_not_found.json @@ -0,0 +1,25 @@ +{ + "data": null, + "errors": [ + { + "message": "Unable to find the node john / TestPerson in the database.", + "locations": [ + { + "line": 2, + "column": 3 + } + ], + "path": [ + "TestPersonDelete" + ], + "extensions": { + "code": "NODE_NOT_FOUND", + "http_status": 404, + "data": { + "node_kind": "TestPerson", + "identifier": "john" + } + } + } + ] +} diff --git a/tests/fixtures/error_catalogue/codes/permission_denied.json b/tests/fixtures/error_catalogue/codes/permission_denied.json new file mode 100644 index 000000000..b6c1ad7c7 --- /dev/null +++ b/tests/fixtures/error_catalogue/codes/permission_denied.json @@ -0,0 +1,25 @@ +{ + "data": null, + "errors": [ + { + "message": "The requested operation was not authorized", + "locations": [ + { + "line": 2, + "column": 3 + } + ], + "path": [ + "TestPersonUpdate" + ], + "extensions": { + "code": "PERMISSION_DENIED", + "http_status": 403, + "data": { + "action": "update", + "resource_kind": "TestPerson" + } + } + } + ] +} diff --git a/tests/fixtures/error_catalogue/codes/schema_not_found.json b/tests/fixtures/error_catalogue/codes/schema_not_found.json new file mode 100644 index 000000000..9f265b84f --- /dev/null +++ b/tests/fixtures/error_catalogue/codes/schema_not_found.json @@ -0,0 +1,21 @@ +{ + "data": null, + "errors": [ + { + "message": "Unable to find the schema TestWidget in the database.", + "locations": [ + { + "line": 2, + "column": 3 + } + ], + "extensions": { + "code": "SCHEMA_NOT_FOUND", + "http_status": 422, + "data": { + "kind": "TestWidget" + } + } + } + ] +} diff --git a/tests/fixtures/error_catalogue/codes/token_expired.json b/tests/fixtures/error_catalogue/codes/token_expired.json new file mode 100644 index 000000000..7fcddb596 --- /dev/null +++ b/tests/fixtures/error_catalogue/codes/token_expired.json @@ -0,0 +1,21 @@ +{ + "data": null, + "errors": [ + { + "message": "Expired Signature", + "locations": [ + { + "line": 2, + "column": 3 + } + ], + "extensions": { + "code": "TOKEN_EXPIRED", + "http_status": 401, + "data": { + "expired_at": "2026-09-18T09:30:00+00:00" + } + } + } + ] +} diff --git a/tests/fixtures/error_catalogue/codes/undefined_error.json b/tests/fixtures/error_catalogue/codes/undefined_error.json new file mode 100644 index 000000000..930f532fb --- /dev/null +++ b/tests/fixtures/error_catalogue/codes/undefined_error.json @@ -0,0 +1,19 @@ +{ + "data": null, + "errors": [ + { + "message": "Cannot query field 'nope' on type 'Query'.", + "locations": [ + { + "line": 1, + "column": 9 + } + ], + "extensions": { + "code": "UNDEFINED_ERROR", + "http_status": 500, + "data": {} + } + } + ] +} diff --git a/tests/fixtures/error_catalogue/codes/uniqueness_violation.json b/tests/fixtures/error_catalogue/codes/uniqueness_violation.json new file mode 100644 index 000000000..acc470d08 --- /dev/null +++ b/tests/fixtures/error_catalogue/codes/uniqueness_violation.json @@ -0,0 +1,27 @@ +{ + "data": null, + "errors": [ + { + "message": "Node of kind TestPerson already has name 'John'", + "locations": [ + { + "line": 2, + "column": 3 + } + ], + "path": [ + "TestPersonCreate" + ], + "extensions": { + "code": "UNIQUENESS_VIOLATION", + "http_status": 422, + "data": { + "node_kind": "TestPerson", + "fields": [ + "name" + ] + } + } + } + ] +} diff --git a/tests/fixtures/error_catalogue/graphql_permission_denied.json b/tests/fixtures/error_catalogue/graphql_permission_denied.json index fbf64e440..23bd5cfad 100644 --- a/tests/fixtures/error_catalogue/graphql_permission_denied.json +++ b/tests/fixtures/error_catalogue/graphql_permission_denied.json @@ -10,7 +10,7 @@ "http_status": 403, "data": { "action": "update", - "kind": "TestPerson" + "resource_kind": "TestPerson" } } } diff --git a/tests/fixtures/error_catalogue/public_names.json b/tests/fixtures/error_catalogue/public_names.json index 79a809034..172c49f5b 100644 --- a/tests/fixtures/error_catalogue/public_names.json +++ b/tests/fixtures/error_catalogue/public_names.json @@ -1,6 +1,11 @@ [ "ApiError", + "AttributeConstraintViolationError", + "AttributeInvalidTypeError", + "AttributeRequiredError", "AuthenticationError", + "BranchAlreadyMergedError", + "BranchNeedsRebaseError", "BranchNotFoundError", "CircularFragmentError", "DuplicateFragmentError", @@ -15,6 +20,8 @@ "InfrahubTransformNotFoundError", "InvalidResponseError", "JsonDecodeError", + "MergeInProgressError", + "MergeRecoveryRequiredError", "ModuleImportError", "NodeInvalidError", "NodeNotFoundError", @@ -29,7 +36,9 @@ "ServerNotResponsiveError", "TimestampFormatError", "URLNotFoundError", + "UndefinedError", "UninitializedError", + "UniquenessViolationError", "ValidationError", "VersionNotSupportedError" ] diff --git a/tests/integration/test_infrahub_client.py b/tests/integration/test_infrahub_client.py index bc39c2320..ad7c7ea5e 100644 --- a/tests/integration/test_infrahub_client.py +++ b/tests/integration/test_infrahub_client.py @@ -9,7 +9,14 @@ from infrahub_sdk import Config, InfrahubClient from infrahub_sdk.branch import BranchData from infrahub_sdk.constants import InfrahubClientMode -from infrahub_sdk.exceptions import BranchNotFoundError, URLNotFoundError +from infrahub_sdk.exceptions import ( + BranchNotFoundError, + GraphQLError, + NodeNotFoundError, + UniquenessViolationError, + URLNotFoundError, + code_names_the_failure, +) from infrahub_sdk.node import InfrahubNode from infrahub_sdk.playback import JSONPlayback from infrahub_sdk.recorder import JSONRecorder @@ -200,6 +207,45 @@ async def test_query_unexisting_branch(self, client: InfrahubClient) -> None: with pytest.raises(URLNotFoundError, match=r"/graphql/unexisting` not found."): await client.execute_graphql(query="unused", branch_name="unexisting") + @pytest.mark.catalogue + async def test_a_unique_attribute_collision_raises_its_own_class( + self, client: InfrahubClient, base_dataset: None + ) -> None: + """Against a real server, so the payload is the one Infrahub sends rather than a fixture.""" + duplicate = await client.create(kind=TESTING_PERSON, name="Liam Walker", height=180) + + # The two message shapes a supported server can produce here: the code named, or the text + # the call site falls back to when the server described nothing. A server that described the + # failure as some other code matches neither and fails, which is the point. + with pytest.raises(GraphQLError, match=r"UNIQUENESS_VIOLATION|An error occurred while") as exc_info: + await duplicate.save() + + # A server that did not describe the failure is one whose catalogue has no entry for it yet, + # which is the only tolerable reason to stop here. + if not code_names_the_failure(exc_info.value.code): + pytest.skip("this server's catalogue has no entry for a uniqueness violation") + + assert isinstance(exc_info.value, UniquenessViolationError) + assert exc_info.value.code == "UNIQUENESS_VIOLATION" + assert exc_info.value.node_kind == TESTING_PERSON + assert exc_info.value.fields == ["name"] + + @pytest.mark.catalogue + async def test_deleting_a_missing_node_raises_its_own_class( + self, client: InfrahubClient, base_dataset: None + ) -> None: + obj = await client.create(kind=TESTING_PERSON, name="Gone Walker", height=170) + await obj.save() + node_id = obj.id + await obj.delete() + + with pytest.raises(NodeNotFoundError, match="NODE_NOT_FOUND") as exc_info: + await obj.delete() + + assert exc_info.value.code == "NODE_NOT_FOUND" + assert exc_info.value.node_type == TESTING_PERSON + assert exc_info.value.identifier == node_id + async def test_create_generic_rel_with_hfid( self, client: InfrahubClient, diff --git a/tests/integration/test_infrahub_client_sync.py b/tests/integration/test_infrahub_client_sync.py index 57d7c842e..795f3eef4 100644 --- a/tests/integration/test_infrahub_client_sync.py +++ b/tests/integration/test_infrahub_client_sync.py @@ -8,7 +8,14 @@ from infrahub_sdk import Config, InfrahubClientSync from infrahub_sdk.branch import BranchData from infrahub_sdk.constants import InfrahubClientMode -from infrahub_sdk.exceptions import BranchNotFoundError, URLNotFoundError +from infrahub_sdk.exceptions import ( + BranchNotFoundError, + GraphQLError, + NodeNotFoundError, + UniquenessViolationError, + URLNotFoundError, + code_names_the_failure, +) from infrahub_sdk.node import InfrahubNodeSync from infrahub_sdk.playback import JSONPlayback from infrahub_sdk.recorder import JSONRecorder @@ -201,6 +208,45 @@ def test_query_unexisting_branch(self, client_sync: InfrahubClientSync) -> None: with pytest.raises(URLNotFoundError, match=r"/graphql/unexisting` not found."): client_sync.execute_graphql(query="unused", branch_name="unexisting") + @pytest.mark.catalogue + def test_a_unique_attribute_collision_raises_its_own_class( + self, client_sync: InfrahubClientSync, base_dataset: None + ) -> None: + """Against a real server, so the payload is the one Infrahub sends rather than a fixture.""" + duplicate = client_sync.create(kind=TESTING_PERSON, name="Liam Walker", height=180) + + # The two message shapes a supported server can produce here: the code named, or the text + # the call site falls back to when the server described nothing. A server that described the + # failure as some other code matches neither and fails, which is the point. + with pytest.raises(GraphQLError, match=r"UNIQUENESS_VIOLATION|An error occurred while") as exc_info: + duplicate.save() + + # A server that did not describe the failure is one whose catalogue has no entry for it yet, + # which is the only tolerable reason to stop here. + if not code_names_the_failure(exc_info.value.code): + pytest.skip("this server's catalogue has no entry for a uniqueness violation") + + assert isinstance(exc_info.value, UniquenessViolationError) + assert exc_info.value.code == "UNIQUENESS_VIOLATION" + assert exc_info.value.node_kind == TESTING_PERSON + assert exc_info.value.fields == ["name"] + + @pytest.mark.catalogue + def test_deleting_a_missing_node_raises_its_own_class( + self, client_sync: InfrahubClientSync, base_dataset: None + ) -> None: + obj = client_sync.create(kind=TESTING_PERSON, name="Vanished Walker", height=170) + obj.save() + node_id = obj.id + obj.delete() + + with pytest.raises(NodeNotFoundError, match="NODE_NOT_FOUND") as exc_info: + obj.delete() + + assert exc_info.value.code == "NODE_NOT_FOUND" + assert exc_info.value.node_type == TESTING_PERSON + assert exc_info.value.identifier == node_id + def test_create_generic_rel_with_hfid( self, client_sync: InfrahubClientSync, diff --git a/tests/unit/sdk/test_client.py b/tests/unit/sdk/test_client.py index d63c8c564..0280ec2c6 100644 --- a/tests/unit/sdk/test_client.py +++ b/tests/unit/sdk/test_client.py @@ -3,15 +3,20 @@ import inspect import json import ssl -from dataclasses import dataclass +from dataclasses import dataclass, field from pathlib import Path -from typing import TYPE_CHECKING +from typing import TYPE_CHECKING, Any import pytest from infrahub_sdk import Config, InfrahubClient, InfrahubClientSync -from infrahub_sdk.exceptions import NodeNotFoundError +from infrahub_sdk.exceptions import ( + ApiError, + NodeNotFoundError, + UniquenessViolationError, +) from infrahub_sdk.node import InfrahubNode, InfrahubNodeSync +from tests.helpers.fixtures import read_fixture if TYPE_CHECKING: from collections.abc import Callable, Mapping @@ -932,3 +937,56 @@ class GraphQLURLCase: async def test_graphql_url_encodes_branch_name(clients: BothClients, client_type: str, case: GraphQLURLCase) -> None: client = clients.standard if client_type == "standard" else clients.sync assert client._graphql_url(branch_name=case.branch_name) == case.expected_url + + +@dataclass +class CatalogueParityCase: + name: str + code: str + status_code: int + expected_class: type[ApiError] + expected_attributes: dict[str, Any] = field(default_factory=dict) + + +CATALOGUE_PARITY_CASES = [ + CatalogueParityCase( + name="uniqueness-violation-inside-a-200", + code="UNIQUENESS_VIOLATION", + status_code=200, + expected_class=UniquenessViolationError, + expected_attributes={"node_kind": "TestPerson", "fields": ["name"], "http_status": 422}, + ), + CatalogueParityCase( + name="node-not-found-inside-a-200", + code="NODE_NOT_FOUND", + status_code=200, + expected_class=NodeNotFoundError, + expected_attributes={"node_type": "TestPerson", "identifier": "john", "http_status": 404}, + ), +] + + +@pytest.mark.parametrize("client_type", client_types) +@pytest.mark.parametrize("case", [pytest.param(tc, id=tc.name) for tc in CATALOGUE_PARITY_CASES]) +async def test_both_clients_raise_the_same_catalogued_exception( + httpx_mock: HTTPXMock, clients: BothClients, client_type: str, case: CatalogueParityCase +) -> None: + """A promoted payload attribute reaches the caller the same way from either client. + + The exception is built at a different call site per client, so parity is a property of the + client rather than of the factory, and only driving both proves it. + """ + envelope = json.loads(read_fixture(file_name=f"{case.code.lower()}.json", fixture_subdir="error_catalogue/codes")) + httpx_mock.add_response(method="POST", status_code=case.status_code, json=envelope) + client = clients.standard if client_type == "standard" else clients.sync + + with pytest.raises(case.expected_class, match=case.code) as exc_info: + if isinstance(client, InfrahubClient): + await client.execute_graphql(query="query { TestPerson { edges { node { id }}}}") + else: + client.execute_graphql(query="query { TestPerson { edges { node { id }}}}") + + assert type(exc_info.value) is case.expected_class + assert exc_info.value.code == case.code + for attribute, value in case.expected_attributes.items(): + assert getattr(exc_info.value, attribute) == value diff --git a/tests/unit/sdk/test_error_catalogue.py b/tests/unit/sdk/test_error_catalogue.py index b52101a6f..1fa66d542 100644 --- a/tests/unit/sdk/test_error_catalogue.py +++ b/tests/unit/sdk/test_error_catalogue.py @@ -2,7 +2,9 @@ from __future__ import annotations +import copy import json +import pickle # noqa: S403 from dataclasses import dataclass from io import BytesIO from typing import TYPE_CHECKING, Any @@ -12,25 +14,53 @@ from infrahub_sdk import Config, InfrahubClient, InfrahubClientSync from infrahub_sdk.exceptions import ( + AttributeConstraintViolationError, + AttributeInvalidTypeError, + AttributeRequiredError, AuthenticationError, + BranchAlreadyMergedError, + BranchNeedsRebaseError, + BranchNotFoundError, GraphQLError, + MergeInProgressError, + MergeRecoveryRequiredError, + NodeNotFoundError, + SchemaNotFoundError, + UndefinedError, + UniquenessViolationError, authentication_error_from_response, graphql_error_from_response, ) +from infrahub_sdk.exceptions.catalogue import CODE_TO_DATA_MODEL from tests.helpers.fixtures import read_fixture if TYPE_CHECKING: + from collections.abc import Callable + from pytest_httpx import HTTPXMock from tests.unit.sdk.conftest import BothClients FIXTURE_SUBDIR = "error_catalogue" +CODES_FIXTURE_SUBDIR = f"{FIXTURE_SUBDIR}/codes" + +# The three codes that reach the SDK on either transport, so neither transport's class can be theirs. +CODES_WITHOUT_A_CLASS = {"AUTHENTICATION_REQUIRED", "PERMISSION_DENIED", "TOKEN_EXPIRED"} def load_envelope(name: str) -> dict[str, Any]: return json.loads(read_fixture(file_name=name, fixture_subdir=FIXTURE_SUBDIR)) +def load_code_envelope(code: str) -> dict[str, Any]: + """The captured response for one catalogue code. + + Addressed by code rather than by listing the directory, so a code whose fixture is missing fails + here instead of quietly dropping out of the parametrisation that is supposed to be exhaustive. + """ + return json.loads(read_fixture(file_name=f"{code.lower()}.json", fixture_subdir=CODES_FIXTURE_SUBDIR)) + + def auth_response(envelope: dict[str, Any] | str, status_code: int = 401) -> httpx.Response: """Build the httpx response the SDK would have observed for a rejected request. @@ -80,6 +110,326 @@ def test_query_and_variables_are_retained(self) -> None: assert exc.variables == {"a": 1} +@dataclass +class CodeCase: + name: str + expected_class: type[GraphQLError] + expected_http_status: int + expected_attributes: dict[str, Any] + + +CODE_CASES = [ + CodeCase( + name="ATTRIBUTE_CONSTRAINT_VIOLATION", + expected_class=AttributeConstraintViolationError, + expected_http_status=422, + expected_attributes={ + "node_kind": "TestPerson", + "field_name": "name", + "constraint": "regex", + "detail": "^[A-Z]", + }, + ), + CodeCase( + name="ATTRIBUTE_INVALID_TYPE", + expected_class=AttributeInvalidTypeError, + expected_http_status=422, + expected_attributes={ + "node_kind": "TestPerson", + "field_name": "height", + "expected_type": "Integer", + "received_type": "String", + }, + ), + CodeCase( + name="ATTRIBUTE_REQUIRED", + expected_class=AttributeRequiredError, + expected_http_status=422, + expected_attributes={"node_kind": "TestPerson", "field_name": "name"}, + ), + CodeCase( + name="AUTHENTICATION_REQUIRED", + expected_class=GraphQLError, + expected_http_status=401, + expected_attributes={}, + ), + CodeCase( + name="BRANCH_ALREADY_MERGED", + expected_class=BranchAlreadyMergedError, + expected_http_status=400, + expected_attributes={"branch_name": "feature-a"}, + ), + CodeCase( + name="BRANCH_NEEDS_REBASE", + expected_class=BranchNeedsRebaseError, + expected_http_status=400, + expected_attributes={"branch_name": "feature-a"}, + ), + CodeCase( + name="BRANCH_NOT_FOUND", + expected_class=BranchNotFoundError, + expected_http_status=400, + expected_attributes={"identifier": "does-not-exist"}, + ), + CodeCase( + name="MERGE_IN_PROGRESS", + expected_class=MergeInProgressError, + expected_http_status=423, + expected_attributes={"branch_name": "main", "merging_branch": "feature-a"}, + ), + CodeCase( + name="MERGE_RECOVERY_REQUIRED", + expected_class=MergeRecoveryRequiredError, + expected_http_status=423, + expected_attributes={"branch_name": "main", "merging_branch": "feature-a"}, + ), + CodeCase( + name="NODE_NOT_FOUND", + expected_class=NodeNotFoundError, + expected_http_status=404, + expected_attributes={"node_type": "TestPerson", "identifier": "john"}, + ), + CodeCase( + name="PERMISSION_DENIED", + expected_class=GraphQLError, + expected_http_status=403, + expected_attributes={}, + ), + CodeCase( + name="SCHEMA_NOT_FOUND", + expected_class=SchemaNotFoundError, + expected_http_status=422, + expected_attributes={"identifier": "TestWidget"}, + ), + CodeCase( + name="TOKEN_EXPIRED", + expected_class=GraphQLError, + expected_http_status=401, + expected_attributes={}, + ), + CodeCase( + name="UNDEFINED_ERROR", + expected_class=UndefinedError, + expected_http_status=500, + expected_attributes={}, + ), + CodeCase( + name="UNIQUENESS_VIOLATION", + expected_class=UniquenessViolationError, + expected_http_status=422, + expected_attributes={"node_kind": "TestPerson", "fields": ["name"]}, + ), +] + + +class TestEveryCatalogueCode: + """One case per code, reading the raised class and its attributes and never a message.""" + + def test_the_cases_cover_every_code_the_bindings_carry(self) -> None: + """Exhaustive only if it is checked against the bindings rather than maintained by hand.""" + assert {case.name for case in CODE_CASES} == set(CODE_TO_DATA_MODEL) + + def test_the_codes_with_no_class_of_their_own_are_the_authentication_ones(self) -> None: + """The one asymmetry in the hierarchy, and the reason the fallback follows the transport.""" + assert {case.name for case in CODE_CASES if case.expected_class is GraphQLError} == CODES_WITHOUT_A_CLASS + + @pytest.mark.parametrize("case", [pytest.param(tc, id=tc.name) for tc in CODE_CASES]) + def test_the_code_raises_its_class_with_its_payload_promoted(self, case: CodeCase) -> None: + envelope = load_code_envelope(case.name) + + exc = graphql_error_from_response(errors=envelope["errors"], query="query { x }") + + assert type(exc) is case.expected_class + assert exc.code == case.name + assert exc.http_status == case.expected_http_status + for attribute, value in case.expected_attributes.items(): + assert getattr(exc, attribute) == value + + @pytest.mark.parametrize("case", [pytest.param(tc, id=tc.name) for tc in CODE_CASES]) + def test_the_raw_payload_stays_available_for_forwarding(self, case: CodeCase) -> None: + envelope = load_code_envelope(case.name) + + exc = graphql_error_from_response(errors=envelope["errors"]) + + assert exc.extensions is not None + assert exc.extensions["data"] == envelope["errors"][0]["extensions"]["data"] + + +ROUND_TRIPS = { + "pickle": lambda exc: pickle.loads(pickle.dumps(exc)), # noqa: S301 + "deepcopy": copy.deepcopy, +} + + +class TestARaisedExceptionSurvivesSerialisation: + """A raised exception crosses process boundaries: a task queue, a parallel test runner. + + The catalogued classes take their payload fields as required keyword arguments, which is the + shape that lets a caller read one without a guard - and the shape the default reconstruction + cannot replay, since it calls the class with the message as a lone positional argument. + """ + + @pytest.mark.parametrize("round_trip", [pytest.param(fn, id=name) for name, fn in ROUND_TRIPS.items()]) + @pytest.mark.parametrize("case", [pytest.param(tc, id=tc.name) for tc in CODE_CASES]) + def test_the_whole_exception_comes_back(self, case: CodeCase, round_trip: Callable[[Any], Any]) -> None: + envelope = load_code_envelope(case.name) + exc = graphql_error_from_response(errors=envelope["errors"], query="query { x }", variables={"a": 1}) + + restored = round_trip(exc) + + assert type(restored) is case.expected_class + assert str(restored) == str(exc) + assert restored.code == exc.code + assert restored.http_status == exc.http_status + assert restored.errors == exc.errors + assert restored.extensions == exc.extensions + assert restored.query == exc.query + assert restored.variables == exc.variables + for attribute, value in case.expected_attributes.items(): + assert getattr(restored, attribute) == value + + def test_a_restored_exception_is_still_caught_by_its_own_clause(self) -> None: + """Reconstructing the type is only worth anything if the `except` ladder still sees it.""" + envelope = load_code_envelope("UNIQUENESS_VIOLATION") + exc = graphql_error_from_response(errors=envelope["errors"]) + restored = pickle.loads(pickle.dumps(exc)) # noqa: S301 + + with pytest.raises(UniquenessViolationError, match="UNIQUENESS_VIOLATION") as exc_info: + raise restored + + assert exc_info.value.node_kind == "TestPerson" + assert exc_info.value.fields == ["name"] + + +class TestTheAdoptedClasses: + """The three classes that predate the catalogue, now reachable from a server-reported failure. + + That a server-reported one reaches the class with its payload promoted is covered per code by + the table above. What is only true of these three is the other direction: they are also raised + with no code behind them, and `code` is what tells a caller which they are holding. + """ + + def test_a_client_side_raise_of_the_same_class_carries_no_code(self) -> None: + """`exc.code is not None` is the test for which of the two a caller is holding.""" + assert NodeNotFoundError(identifier="john", node_type="TestPerson").code is None + assert BranchNotFoundError(identifier="does-not-exist").code is None + assert SchemaNotFoundError(identifier="TestWidget").code is None + + +class TestTheFirstErrorGoverns: + def test_a_silent_first_error_governs_over_a_later_coded_one(self) -> None: + """Otherwise a response's class would depend on which error the SDK happens to recognise.""" + errors = [ + {"message": "the failure that came first"}, + { + "message": "and one the catalogue describes", + "extensions": { + "code": "UNIQUENESS_VIOLATION", + "http_status": 422, + "data": {"node_kind": "TestPerson", "fields": ["name"]}, + }, + }, + ] + + exc = graphql_error_from_response(errors=errors, query="mutation { TestPersonCreate }") + + assert type(exc) is GraphQLError + assert exc.code is None + assert exc.http_status is None + + def test_the_complete_list_is_retained_unreordered(self) -> None: + errors = [ + {"message": "the failure that came first"}, + { + "message": "and one the catalogue describes", + "extensions": { + "code": "UNIQUENESS_VIOLATION", + "http_status": 422, + "data": {"node_kind": "TestPerson", "fields": ["name"]}, + }, + }, + ] + + exc = graphql_error_from_response(errors=errors) + + assert [error["message"] for error in exc.errors] == [ + "the failure that came first", + "and one the catalogue describes", + ] + + +class TestTheDeclaredStatus: + """Where `http_status` comes from when the envelope and the class disagree, or one is silent. + + Every captured envelope declares the status its code's class already carries, so these drive the + shapes that tell the two sources apart. + """ + + def test_a_generated_class_keeps_its_catalogue_status_when_the_envelope_omits_one(self) -> None: + envelope = load_code_envelope("UNIQUENESS_VIOLATION") + del envelope["errors"][0]["extensions"]["http_status"] + + exc = graphql_error_from_response(errors=envelope["errors"]) + + assert isinstance(exc, UniquenessViolationError) + assert exc.http_status == 422, "the class declares the catalogue's status, so there is one to keep" + + def test_the_envelope_status_wins_over_the_one_the_class_declares(self) -> None: + """The server substitutes its own where its catalogue could not resolve a specific status.""" + envelope = load_code_envelope("UNIQUENESS_VIOLATION") + envelope["errors"][0]["extensions"]["http_status"] = 409 + + exc = graphql_error_from_response(errors=envelope["errors"]) + + assert isinstance(exc, UniquenessViolationError) + assert exc.http_status == 409 + + def test_an_adopted_class_has_no_status_of_its_own_to_fall_back_on(self) -> None: + """The three pre-catalogue classes declare none, because they are also raised without one.""" + envelope = load_code_envelope("NODE_NOT_FOUND") + del envelope["errors"][0]["extensions"]["http_status"] + + exc = graphql_error_from_response(errors=envelope["errors"]) + + assert isinstance(exc, NodeNotFoundError) + assert exc.http_status is None + + def test_the_generic_class_reports_no_status_where_the_envelope_declared_none(self) -> None: + exc = graphql_error_from_response(errors=[{"message": "boom", "extensions": {"code": "NOT_A_KNOWN_CODE"}}]) + + assert type(exc) is GraphQLError + assert exc.http_status is None + + +class TestTheFallbackFollowsTheObservedTransport: + """Which generic class a code falls back to is decided by how the SDK saw it arrive. + + Following the status the catalogue declares instead would send the three authentication codes to + `AuthenticationError` whenever a resolver raised them inside an HTTP 200, out of reach of the + `except GraphQLError` clause that catches them today - and send a 401 carrying a data code to a + class no caller of that path expects. + """ + + @pytest.mark.parametrize("code", sorted(CODES_WITHOUT_A_CLASS)) + def test_an_authentication_code_inside_a_graphql_response_raises_the_graphql_class(self, code: str) -> None: + envelope = load_code_envelope(code) + + exc = graphql_error_from_response(errors=envelope["errors"]) + + assert type(exc) is GraphQLError, "the declared 401 or 403 is metadata, not the transport" + assert not isinstance(exc, AuthenticationError) + assert exc.code == code + + def test_a_data_code_on_a_real_401_raises_the_authentication_class(self) -> None: + response = auth_response(envelope=load_code_envelope("NODE_NOT_FOUND")) + + exc = authentication_error_from_response(response=response) + + assert type(exc) is AuthenticationError, "a class the caller of this path cannot expect is worse than none" + assert exc.code == "NODE_NOT_FOUND" + assert exc.http_status == 404, "the declared status is still readable, it just governs nothing" + + class TestAuthenticationFactory: def test_reads_code_and_status_off_a_real_401(self) -> None: response = auth_response(envelope=load_envelope("auth_token_expired.json")) @@ -250,6 +600,7 @@ class CrossVersionCase: fixture: str expected_code: str | None expected_http_status: int | None = None + expected_class: type[GraphQLError] = GraphQLError CROSS_VERSION_CASES = [ @@ -264,6 +615,7 @@ class CrossVersionCase: fixture="graphql_extra_payload_field.json", expected_code="UNIQUENESS_VIOLATION", expected_http_status=422, + expected_class=UniquenessViolationError, ), CrossVersionCase( name="error-carrying-no-extensions", @@ -280,13 +632,17 @@ class CrossVersionCase: @pytest.mark.crossversion @pytest.mark.parametrize("case", [pytest.param(tc, id=tc.name) for tc in CROSS_VERSION_CASES]) -def test_cross_version_envelope_parses_onto_the_generic_class(case: CrossVersionCase) -> None: - """Any SDK version talks to any server version, and parsing never raises.""" +def test_a_cross_version_envelope_parses_without_raising(case: CrossVersionCase) -> None: + """Any SDK version talks to any server version, and parsing never raises. + + A code these bindings have a class for still reaches it here: gaining a field it has never heard + of changes nothing, which is the forward compatibility the payload models are for. + """ envelope = load_envelope(case.fixture) exc = graphql_error_from_response(errors=envelope["errors"], query="query { x }") - assert type(exc) is GraphQLError + assert type(exc) is case.expected_class assert exc.code == case.expected_code assert exc.http_status == case.expected_http_status @@ -476,6 +832,52 @@ def test_cross_version_fallback_is_logged_at_debug_level(caplog: pytest.LogCaptu assert any("No catalogue code resolved" in record.getMessage() for record in caplog.records) +@dataclass +class FallbackLogCase: + name: str + errors: list[dict[str, Any]] + expected_fragment: str + + +FALLBACK_LOG_CASES = [ + FallbackLogCase( + name="a-code-these-bindings-have-no-class-for", + errors=[{"message": "boom", "extensions": {"code": "SOMETHING_WE_HAVE_NEVER_HEARD_OF", "http_status": 418}}], + expected_fragment="have no class for the catalogue code SOMETHING_WE_HAVE_NEVER_HEARD_OF", + ), + FallbackLogCase( + name="an-error-carrying-no-extensions", + errors=[{"message": "boom"}], + expected_fragment="No catalogue code resolved", + ), + FallbackLogCase( + name="a-payload-the-catalogue-does-not-declare", + errors=[{"message": "boom", "extensions": {"code": "UNIQUENESS_VIOLATION", "data": {"fields": "not-a-list"}}}], + expected_fragment="does not match what the catalogue declares", + ), +] + + +@pytest.mark.crossversion +@pytest.mark.parametrize("case", [pytest.param(tc, id=tc.name) for tc in FALLBACK_LOG_CASES]) +def test_every_fallback_is_logged_at_debug_level(case: FallbackLogCase, caplog: pytest.LogCaptureFixture) -> None: + """An SDK meeting a newer server has to be diagnosable from a log, not only from a debugger.""" + with caplog.at_level("DEBUG", logger="infrahub_sdk"): + graphql_error_from_response(errors=case.errors) + + assert any(case.expected_fragment in record.getMessage() for record in caplog.records) + + +def test_a_resolved_code_logs_nothing(caplog: pytest.LogCaptureFixture) -> None: + """The log marks the paths that degraded, so the ordinary one must stay quiet.""" + envelope = load_code_envelope("UNIQUENESS_VIOLATION") + + with caplog.at_level("DEBUG", logger="infrahub_sdk"): + graphql_error_from_response(errors=envelope["errors"]) + + assert caplog.records == [] + + @pytest.mark.malformed def test_unparseable_authentication_body_is_logged_at_debug_level(caplog: pytest.LogCaptureFixture) -> None: response = auth_response(envelope="502") diff --git a/tests/unit/sdk/test_exceptions.py b/tests/unit/sdk/test_exceptions.py index 2610ea4f2..79a9a2b33 100644 --- a/tests/unit/sdk/test_exceptions.py +++ b/tests/unit/sdk/test_exceptions.py @@ -188,6 +188,23 @@ def test_every_unified_class_sets_its_envelope_through_the_constructor(self, cas assert exc.errors == [] assert isinstance(exc.errors, list) + def test_the_request_attributes_are_readable_on_an_authentication_failure(self) -> None: + """`except ApiError` is the clause that spans both transports, so it may read all of these. + + An authentication failure is observed at the transport, which has no query to record, so the + attributes are empty rather than absent. + """ + exc = AuthenticationError("boom") + + assert exc.query is None + assert exc.variables is None + assert exc.errors == () + + @pytest.mark.parametrize("attribute", ["code", "http_status", "extensions", "errors", "query", "variables"]) + def test_every_envelope_attribute_is_declared_on_the_shared_base(self, attribute: str) -> None: + """Declared on `ApiError` itself, so neither transport's class can drift out of step.""" + assert hasattr(ApiError, attribute) + class TestTheBroadening: def test_except_graphql_error_now_catches_a_client_side_lookup_miss(self) -> None: diff --git a/tests/unit/sdk/test_exceptions_layering.py b/tests/unit/sdk/test_exceptions_layering.py index e6f71bb4d..e76897553 100644 --- a/tests/unit/sdk/test_exceptions_layering.py +++ b/tests/unit/sdk/test_exceptions_layering.py @@ -18,11 +18,13 @@ PACKAGE_DIR = Path(exceptions_package.__file__).parent # base.py sits at the bottom, which is what keeps the hand-written hierarchy independent of anything -# built on top of it. Each layer above may import only from below it. +# built on top of it. Each layer above may import only from below it. The generated catalogue sits +# above base and below factory, so the factory can resolve a code to one of its classes. LAYERS = { "base": 0, - "factory": 1, - "__init__": 2, + "catalogue": 1, + "factory": 2, + "__init__": 3, } diff --git a/tests/unit/sdk/test_exceptions_public_names.py b/tests/unit/sdk/test_exceptions_public_names.py index 5d100e191..f52bc4e8f 100644 --- a/tests/unit/sdk/test_exceptions_public_names.py +++ b/tests/unit/sdk/test_exceptions_public_names.py @@ -3,7 +3,7 @@ import json from infrahub_sdk import exceptions -from infrahub_sdk.exceptions import authentication_error_from_response, base, graphql_error_from_response +from infrahub_sdk.exceptions import authentication_error_from_response, base, catalogue, graphql_error_from_response from tests.helpers.fixtures import read_fixture @@ -36,13 +36,34 @@ def classes_defined_in(module: object) -> set[str]: } -def test_the_facade_lists_every_class_base_declares() -> None: - """The façade writes its exports out by hand, so nothing may drift out of step with `base`. +def catalogue_exception_names() -> set[str]: + """The exception classes the generated module declares, without its payload models or maps.""" + return { + name + for name in catalogue.__all__ + if isinstance(getattr(catalogue, name), type) and issubclass(getattr(catalogue, name), BaseException) + } + - A class added to `base.__all__` has to be added to both lists in - `infrahub_sdk/exceptions/__init__.py`: the import block and `__all__`. +def test_the_facade_lists_every_class_base_and_catalogue_declare() -> None: + """The façade writes its exports out by hand, so nothing may drift out of step with its sources. + + A class added to `base.__all__`, or a new code added to the catalogue, has to be added to both + lists in `infrahub_sdk/exceptions/__init__.py`: the import block and `__all__`. """ - assert set(exceptions.__all__) == set(base.__all__) + assert set(exceptions.__all__) == set(base.__all__) | catalogue_exception_names() + + +def test_the_facade_re_exports_no_catalogue_payload_model_or_lookup() -> None: + """Only the classes a caller catches are promoted to the package surface. + + The payload models, the two lookup maps and the dispatch helper are internal to the package, so + they stay importable from `catalogue` rather than becoming a stability promise of the package. + """ + non_classes = set(catalogue.__all__) - catalogue_exception_names() + leaked = sorted(non_classes & set(exceptions.__all__)) + + assert leaked == [], f"re-exported from catalogue but not an exception class: {leaked}" def test_every_name_the_facade_declares_is_bound() -> None: @@ -63,6 +84,17 @@ def test_base_declares_every_exception_it_defines() -> None: assert undeclared == [], f"defined in base.py but missing from base.__all__: {undeclared}" +def test_the_catalogue_declares_every_exception_it_defines() -> None: + """Every other check here trusts `catalogue.__all__`, so nothing else would catch an omission. + + A generated class left out of it is simply absent from the façade, and the two checks that would + otherwise notice both derive from the same list, so they agree with each other and stay green. + """ + undeclared = sorted(classes_defined_in(catalogue) - set(catalogue.__all__)) + + assert undeclared == [], f"defined in catalogue.py but missing from catalogue.__all__: {undeclared}" + + def test_snapshot_matches_the_exported_exceptions() -> None: """infrahub_sdk.exceptions is the supported import path.