Conversation
Generated by `uv run invoke backend.generate` from the Infrahub branch that adds the generator. Nine exception classes, fifteen payload models, and a dispatch that resolves a catalogue code and its payload to a concrete exception. The three codes the server reports as 401 or 403 get a payload model but no class: the SDK routes those by the response it saw. This file is generated and must not be hand-edited. Infrahub owns it, the same way it owns the schema models and the protocols.
infrahub_sdk.exceptions is the supported import path, so the nine classes the catalogue generates belong on it. They are listed by name rather than star-imported: a wildcard would also promote fifteen payload models, both lookup maps and the dispatch helper to the package surface, where a name is a stability promise. Those stay importable from the catalogue module, which is where the factory wants them. The layering test learns that the generated module sits above base and below factory, and the public-names snapshot gains the nine classes, so the surface stays a deliberate choice rather than a side effect.
Deploying infrahub-sdk-python with
|
| Latest commit: |
d1d10c6
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://38ee5503.infrahub-sdk-python.pages.dev |
| Branch Preview URL: | https://pog-raise-per-code-ihs-295.infrahub-sdk-python.pages.dev |
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## pog-error-catalogue-IFC-3034 #1373 +/- ##
================================================================
+ Coverage 86.49% 86.67% +0.17%
================================================================
Files 151 152 +1
Lines 14497 14683 +186
Branches 1987 1981 -6
================================================================
+ Hits 12539 12726 +187
Misses 1388 1388
+ Partials 570 569 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
3 issues found across 5 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="infrahub_sdk/exceptions/catalogue.py">
<violation number="1" location="infrahub_sdk/exceptions/catalogue.py:164">
P2: PermissionDeniedData declares `resource_kind`, but the recorded server payload (tests/fixtures/error_catalogue/graphql_permission_denied.json) sends `kind` under `data`. Because the model sets `extra="ignore"`, validating the observed payload silently discards `kind` and leaves `resource_kind` None, so a caller reading the model loses the resource name. Align the field name with what the server actually sends, or fix the fixture if it misnames the payload.</violation>
<violation number="2" location="infrahub_sdk/exceptions/catalogue.py:640">
P2: When a catalogued code arrives without the payload fields its model declares required, this dispatch raises pydantic.ValidationError instead of returning an exception. That error is not under `Error`, so `except GraphQLError`/`except Error` consumers miss the server's failure entirely. The existing factories (factory.py `_payload_strings`, `_adopted_exception`) deliberately degrade such envelopes to the generic class and never raise from an already-failing call; this entry point, which the follow-up factory will feed with `extensions.get("data")` (absent, or `{}` for codes that carry no data per tests/fixtures/error_catalogue/auth_token_expired.json), breaks that guarantee. Trap ValidationError per code and degrade to a generic GraphQLError carrying the code, or catch it where the dispatch is used.</violation>
</file>
<file name="tests/unit/sdk/test_exceptions_public_names.py">
<violation number="1" location="tests/unit/sdk/test_exceptions_public_names.py:39">
P3: The new union assertion treats catalogue.__all__ as ground truth, but nothing verifies that every exception class catalogue defines actually appears in catalogue.__all__. test_base_declares_every_exception_it_defines exists precisely because such an omission is otherwise invisible; the catalogue side has the same single-source trust but no guard. Add a catalogue analogue of the base test (e.g. assert classes_defined_in(catalogue) is a subset of catalogue.__all__), so a generated class dropped from __all__ and never hand-imported in __init__.py still fails the suite.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| """ | ||
| builder = _CODE_TO_BUILDER.get(code) | ||
| return None if builder is None else builder(data) |
Contributor
There was a problem hiding this comment.
P2: When a catalogued code arrives without the payload fields its model declares required, this dispatch raises pydantic.ValidationError instead of returning an exception. That error is not under Error, so except GraphQLError/except Error consumers miss the server's failure entirely. The existing factories (factory.py _payload_strings, _adopted_exception) deliberately degrade such envelopes to the generic class and never raise from an already-failing call; this entry point, which the follow-up factory will feed with extensions.get("data") (absent, or {} for codes that carry no data per tests/fixtures/error_catalogue/auth_token_expired.json), breaks that guarantee. Trap ValidationError per code and degrade to a generic GraphQLError carrying the code, or catch it where the dispatch is used.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At infrahub_sdk/exceptions/catalogue.py, line 640:
<comment>When a catalogued code arrives without the payload fields its model declares required, this dispatch raises pydantic.ValidationError instead of returning an exception. That error is not under `Error`, so `except GraphQLError`/`except Error` consumers miss the server's failure entirely. The existing factories (factory.py `_payload_strings`, `_adopted_exception`) deliberately degrade such envelopes to the generic class and never raise from an already-failing call; this entry point, which the follow-up factory will feed with `extensions.get("data")` (absent, or `{}` for codes that carry no data per tests/fixtures/error_catalogue/auth_token_expired.json), breaks that guarantee. Trap ValidationError per code and degrade to a generic GraphQLError carrying the code, or catch it where the dispatch is used.</comment>
<file context>
@@ -0,0 +1,640 @@
+
+ """
+ builder = _CODE_TO_BUILDER.get(code)
+ return None if builder is None else builder(data)
</file context>
| model_config = ConfigDict(extra="ignore") | ||
|
|
||
| action: str | None = None | ||
| resource_kind: str | None = None |
Contributor
There was a problem hiding this comment.
P2: PermissionDeniedData declares resource_kind, but the recorded server payload (tests/fixtures/error_catalogue/graphql_permission_denied.json) sends kind under data. Because the model sets extra="ignore", validating the observed payload silently discards kind and leaves resource_kind None, so a caller reading the model loses the resource name. Align the field name with what the server actually sends, or fix the fixture if it misnames the payload.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At infrahub_sdk/exceptions/catalogue.py, line 164:
<comment>PermissionDeniedData declares `resource_kind`, but the recorded server payload (tests/fixtures/error_catalogue/graphql_permission_denied.json) sends `kind` under `data`. Because the model sets `extra="ignore"`, validating the observed payload silently discards `kind` and leaves `resource_kind` None, so a caller reading the model loses the resource name. Align the field name with what the server actually sends, or fix the fixture if it misnames the payload.</comment>
<file context>
@@ -0,0 +1,640 @@
+ model_config = ConfigDict(extra="ignore")
+
+ action: str | None = None
+ resource_kind: str | None = None
+
+
</file context>
Suggested change
| resource_kind: str | None = None | |
| kind: str | None = None |
|
|
||
| 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]: |
Contributor
There was a problem hiding this comment.
P3: The new union assertion treats catalogue.all as ground truth, but nothing verifies that every exception class catalogue defines actually appears in catalogue.all. test_base_declares_every_exception_it_defines exists precisely because such an omission is otherwise invisible; the catalogue side has the same single-source trust but no guard. Add a catalogue analogue of the base test (e.g. assert classes_defined_in(catalogue) is a subset of catalogue.all), so a generated class dropped from all and never hand-imported in init.py still fails the suite.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/unit/sdk/test_exceptions_public_names.py, line 39:
<comment>The new union assertion treats catalogue.__all__ as ground truth, but nothing verifies that every exception class catalogue defines actually appears in catalogue.__all__. test_base_declares_every_exception_it_defines exists precisely because such an omission is otherwise invisible; the catalogue side has the same single-source trust but no guard. Add a catalogue analogue of the base test (e.g. assert classes_defined_in(catalogue) is a subset of catalogue.__all__), so a generated class dropped from __all__ and never hand-imported in __init__.py still fails the suite.</comment>
<file context>
@@ -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 {
</file context>
The GraphQL factory now resolves a catalogue code through the generated bindings' own dispatch, which validates the payload against that code's model and hands it to the class's `from_payload`. The hand-written stand-in payload dataclasses and the three-way branch that read them go away with it, so the factory assembles no attribute itself and every catalogued code - not only the three adopted ones - reaches a class carrying its payload as typed attributes. A payload that violates what the catalogue declares falls back to the generic class for the transport, with the code still readable and the raw extensions retained. Which generic class it falls back to follows the transport the SDK observed, never the status the code declares: the GraphQL branch for anything read from an `errors` array, the authentication branch only for a response the SDK saw as 401 or 403. That is the only rule under which the three authentication codes, which deliberately have no class of their own, reach the class an existing `except` clause expects. A declared HTTP status is now assigned only when the envelope carried one, so a generated class keeps the status the catalogue gave it. A class built from its payload alone carries the GraphQL placeholder message; where the failure was not described, the factory replaces that with the message the call site has always produced rather than leaving a sentence naming no query and no errors. Tests cover every code in the bindings, checked against the bindings' own code list so the set cannot fall behind: the class raised, each promoted attribute's value, and the code and status, with no case reading a message. Both clients are driven over both transports and the file-upload path, and two integration cases prove the envelope against a real server. The uniqueness case skips where the server's catalogue has no entry for that failure yet.
github-actions
Bot
added
the
type/documentation
Contributor
There was a problem hiding this comment.
1 existing issue remains and 10 new issues found across 26 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="docs/docs/python-sdk/topics/error_handling.mdx">
<violation number="1" location="docs/docs/python-sdk/topics/error_handling.mdx:12">
P2: The page incorrectly says every SDK-raised exception is importable from `infrahub_sdk.exceptions`; the SDK also raises built-in exceptions such as `ValueError` and `IndexError`. Limit this claim to SDK-defined exceptions, and update the hierarchy description accordingly.</violation>
<violation number="2" location="docs/docs/python-sdk/topics/error_handling.mdx:92">
P2: This says the authentication codes are the only codes that can arrive on both transports, but any catalogue code can appear in a real 401/403 response. Describe them as the only codes without dedicated classes instead.</violation>
<violation number="3" location="docs/docs/python-sdk/topics/error_handling.mdx:124">
P2: The `query` and `variables` row is not valid for every `ApiError`: `AuthenticationError` does not define either attribute. Scope those attributes to `GraphQLError` or add them to the base contract before telling callers no guard is needed.</violation>
<violation number="4" location="docs/docs/python-sdk/topics/error_handling.mdx:158">
P2: Malformed or non-JSON responses still raise `JsonDecodeError` before the catalogue factory runs. Replace this absolute statement with the narrower guarantee that catalogue payload mismatches do not raise validation errors.</violation>
<violation number="5" location="docs/docs/python-sdk/topics/error_handling.mdx:168">
P2: Unknown-code fallbacks are not logged as documented: the factory logs only when no string code resolves, not when a string code is absent from the catalogue. Log unresolved known-shape codes too, or narrow this guarantee to missing/non-string codes.</violation>
</file>
<file name="dev/specs/ifc-3034-error-catalogue/tasks.md">
<violation number="1" location="dev/specs/ifc-3034-error-catalogue/tasks.md:453">
P3: This change marks Phase 8 tasks done but leaves T063/T064 unchecked, even though this PR performs both and the file's dependency section says Phase 8 depends on T064. Check T063/T064 in this same update, and reword T064's `from .catalogue import *` step to the named re-exports the PR actually implemented, so the tracker matches the code.</violation>
<violation number="2" location="dev/specs/ifc-3034-error-catalogue/tasks.md:481">
P3: T074 is marked complete with an overly narrow transport condition: the authentication factory also handles non-401 statuses from failed refresh attempts. Reword the checklist to distinguish the normal 401/403 response path from the refresh-failure exception.</violation>
</file>
<file name="tests/integration/test_infrahub_client.py">
<violation number="1" location="tests/integration/test_infrahub_client.py:221">
P3: When the server emits UNIQUENESS_VIOLATION but the payload fails catalogue validation, `graphql_error_from_response` falls back to the generic GraphQLError while still assigning `code="UNIQUENESS_VIOLATION"` on the instance. The code-mismatch skip at `if exc_info.value.code != "UNIQUENESS_VIOLATION"` then passes, and the test fails at `assert isinstance(...)` instead of skipping. Guard the tolerance on the class as well as the code so an evolved/unknown payload shape skips like the other server variants.</violation>
</file>
<file name="changelog/+typed-per-code-exceptions.added.md">
<violation number="1" location="changelog/+typed-per-code-exceptions.added.md:1">
P3: The opening sentence overstates the feature: three catalogue codes deliberately have no dedicated exception class, as this changelog later explains. Reword it to say that catalogue failures now have dedicated classes where applicable, so the release note is not self-contradictory.</violation>
</file>
<file name="tests/integration/test_infrahub_client_sync.py">
<violation number="1" location="tests/integration/test_infrahub_client_sync.py:238">
P2: Custom agent: **Flag AI Slop and Fabricated Changes**
This test asserts `obj.delete()` on a deleted node raises `NodeNotFoundError` and that `code == "NODE_NOT_FOUND"`, but the PR description states no behavior changes land here — nothing raises these typed catalogue classes until the factory work (T066–T077). Without the factory, the GraphQL error path raises the base `GraphQLError`, so `pytest.raises(NodeNotFoundError, ...)` fails and the subsequent `.code`/`.node_type`/`.identifier` assertions on the exception also fail. The sibling uniqueness test in the same file guards this exact case with a `pytest.skip` fallback; this deletion test has no such guard. Add the same skip fallback (or move the test to the factory work branch).</violation>
</file>
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
Re-trigger cubic
|
|
||
| ## Talking to any server version | ||
|
|
||
| Any SDK version talks to any server version, and parsing a response never raises. |
Contributor
There was a problem hiding this comment.
P2: Malformed or non-JSON responses still raise JsonDecodeError before the catalogue factory runs. Replace this absolute statement with the narrower guarantee that catalogue payload mismatches do not raise validation errors.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/docs/python-sdk/topics/error_handling.mdx, line 158:
<comment>Malformed or non-JSON responses still raise `JsonDecodeError` before the catalogue factory runs. Replace this absolute statement with the narrower guarantee that catalogue payload mismatches do not raise validation errors.</comment>
<file context>
@@ -0,0 +1,184 @@
+
+## Talking to any server version
+
+Any SDK version talks to any server version, and parsing a response never raises.
+
+| Situation | Behaviour |
</file context>
| ### The three authentication codes | ||
|
|
||
| `AUTHENTICATION_REQUIRED`, `TOKEN_EXPIRED`, and `PERMISSION_DENIED` have no class of their own. They | ||
| are the only codes that reach the SDK on two different transports, and each transport already has a |
Contributor
There was a problem hiding this comment.
P2: This says the authentication codes are the only codes that can arrive on both transports, but any catalogue code can appear in a real 401/403 response. Describe them as the only codes without dedicated classes instead.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/docs/python-sdk/topics/error_handling.mdx, line 92:
<comment>This says the authentication codes are the only codes that can arrive on both transports, but any catalogue code can appear in a real 401/403 response. Describe them as the only codes without dedicated classes instead.</comment>
<file context>
@@ -0,0 +1,184 @@
+### The three authentication codes
+
+`AUTHENTICATION_REQUIRED`, `TOKEN_EXPIRED`, and `PERMISSION_DENIED` have no class of their own. They
+are the only codes that reach the SDK on two different transports, and each transport already has a
+class that existing code depends on:
+
</file context>
Suggested change
| are the only codes that reach the SDK on two different transports, and each transport already has a | |
| are the only catalogue codes without a dedicated class; any catalogue code can still arrive on either transport: |
| | A server predating the catalogue, or an error with no `extensions` | `exc.code` is `None`, and the message is the one that version of the SDK has always produced | | ||
| | A payload that does not match what the catalogue declares | The generic class for the transport, with the code still readable | | ||
|
|
||
| Every fallback is logged at debug level on the `infrahub_sdk` logger with the code involved, so an SDK |
Contributor
There was a problem hiding this comment.
P2: Unknown-code fallbacks are not logged as documented: the factory logs only when no string code resolves, not when a string code is absent from the catalogue. Log unresolved known-shape codes too, or narrow this guarantee to missing/non-string codes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/docs/python-sdk/topics/error_handling.mdx, line 168:
<comment>Unknown-code fallbacks are not logged as documented: the factory logs only when no string code resolves, not when a string code is absent from the catalogue. Log unresolved known-shape codes too, or narrow this guarantee to missing/non-string codes.</comment>
<file context>
@@ -0,0 +1,184 @@
+| A server predating the catalogue, or an error with no `extensions` | `exc.code` is `None`, and the message is the one that version of the SDK has always produced |
+| A payload that does not match what the catalogue declares | The generic class for the transport, with the code still readable |
+
+Every fallback is logged at debug level on the `infrahub_sdk` logger with the code involved, so an SDK
+meeting a newer server is diagnosable without a debugger.
+
</file context>
|
|
||
| ## Introduction | ||
|
|
||
| Every exception the SDK raises is importable from `infrahub_sdk.exceptions`. That is the supported |
Contributor
There was a problem hiding this comment.
P2: The page incorrectly says every SDK-raised exception is importable from infrahub_sdk.exceptions; the SDK also raises built-in exceptions such as ValueError and IndexError. Limit this claim to SDK-defined exceptions, and update the hierarchy description accordingly.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/docs/python-sdk/topics/error_handling.mdx, line 12:
<comment>The page incorrectly says every SDK-raised exception is importable from `infrahub_sdk.exceptions`; the SDK also raises built-in exceptions such as `ValueError` and `IndexError`. Limit this claim to SDK-defined exceptions, and update the hierarchy description accordingly.</comment>
<file context>
@@ -0,0 +1,184 @@
+
+## Introduction
+
+Every exception the SDK raises 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.
+
</file context>
| | `http_status` | The status the failure declares, or `None`. This is metadata about the failure, not the status the transport observed: a catalogued data error arrives as HTTP 200. | | ||
| | `extensions` | The raw `extensions` mapping of the governing error, or `None`. | | ||
| | `errors` | The complete server error list, in the order the server sent it. Empty for a raise the SDK decided on its own. | | ||
| | `query`, `variables` | The GraphQL query and variables where there was one, otherwise `None`. | |
Contributor
There was a problem hiding this comment.
P2: The query and variables row is not valid for every ApiError: AuthenticationError does not define either attribute. Scope those attributes to GraphQLError or add them to the base contract before telling callers no guard is needed.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/docs/python-sdk/topics/error_handling.mdx, line 124:
<comment>The `query` and `variables` row is not valid for every `ApiError`: `AuthenticationError` does not define either attribute. Scope those attributes to `GraphQLError` or add them to the base contract before telling callers no guard is needed.</comment>
<file context>
@@ -0,0 +1,184 @@
+| `http_status` | The status the failure declares, or `None`. This is metadata about the failure, not the status the transport observed: a catalogued data error arrives as HTTP 200. |
+| `extensions` | The raw `extensions` mapping of the governing error, or `None`. |
+| `errors` | The complete server error list, in the order the server sent it. Empty for a raise the SDK decided on its own. |
+| `query`, `variables` | The GraphQL query and variables where there was one, otherwise `None`. |
+
+The payload's fields are not on the base class. Each catalogued class carries its own, typed as the
</file context>
| node_id = obj.id | ||
| obj.delete() | ||
|
|
||
| with pytest.raises(NodeNotFoundError, match="NODE_NOT_FOUND") as exc_info: |
Contributor
There was a problem hiding this comment.
P2: Custom agent: Flag AI Slop and Fabricated Changes
This test asserts obj.delete() on a deleted node raises NodeNotFoundError and that code == "NODE_NOT_FOUND", but the PR description states no behavior changes land here — nothing raises these typed catalogue classes until the factory work (T066–T077). Without the factory, the GraphQL error path raises the base GraphQLError, so pytest.raises(NodeNotFoundError, ...) fails and the subsequent .code/.node_type/.identifier assertions on the exception also fail. The sibling uniqueness test in the same file guards this exact case with a pytest.skip fallback; this deletion test has no such guard. Add the same skip fallback (or move the test to the factory work branch).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/integration/test_infrahub_client_sync.py, line 238:
<comment>This test asserts `obj.delete()` on a deleted node raises `NodeNotFoundError` and that `code == "NODE_NOT_FOUND"`, but the PR description states no behavior changes land here — nothing raises these typed catalogue classes until the factory work (T066–T077). Without the factory, the GraphQL error path raises the base `GraphQLError`, so `pytest.raises(NodeNotFoundError, ...)` fails and the subsequent `.code`/`.node_type`/`.identifier` assertions on the exception also fail. The sibling uniqueness test in the same file guards this exact case with a `pytest.skip` fallback; this deletion test has no such guard. Add the same skip fallback (or move the test to the factory work branch).</comment>
<file context>
@@ -201,6 +207,41 @@ def test_query_unexisting_branch(self, client_sync: InfrahubClientSync) -> None:
+ node_id = obj.id
+ obj.delete()
+
+ with pytest.raises(NodeNotFoundError, match="NODE_NOT_FOUND") as exc_info:
+ obj.delete()
+
</file context>
| ### Tests for User Story 1 | ||
|
|
||
| - [ ] T066 [P] [US1] Add one response-envelope fixture per catalogue code under | ||
| - [X] T066 [P] [US1] Add one response-envelope fixture per catalogue code under |
Contributor
There was a problem hiding this comment.
P3: This change marks Phase 8 tasks done but leaves T063/T064 unchecked, even though this PR performs both and the file's dependency section says Phase 8 depends on T064. Check T063/T064 in this same update, and reword T064's from .catalogue import * step to the named re-exports the PR actually implemented, so the tracker matches the code.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At dev/specs/ifc-3034-error-catalogue/tasks.md, line 453:
<comment>This change marks Phase 8 tasks done but leaves T063/T064 unchecked, even though this PR performs both and the file's dependency section says Phase 8 depends on T064. Check T063/T064 in this same update, and reword T064's `from .catalogue import *` step to the named re-exports the PR actually implemented, so the tracker matches the code.</comment>
<file context>
@@ -450,39 +450,39 @@ the raised type and the typed attributes, reading no message (quickstart scenari
### Tests for User Story 1
-- [ ] T066 [P] [US1] Add one response-envelope fixture per catalogue code under
+- [X] T066 [P] [US1] Add one response-envelope fixture per catalogue code under
`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
</file context>
| with pytest.raises(GraphQLError, match=r"UNIQUENESS_VIOLATION|An error occurred while") as exc_info: | ||
| await duplicate.save() | ||
|
|
||
| if exc_info.value.code != "UNIQUENESS_VIOLATION": |
Contributor
There was a problem hiding this comment.
P3: When the server emits UNIQUENESS_VIOLATION but the payload fails catalogue validation, graphql_error_from_response falls back to the generic GraphQLError while still assigning code="UNIQUENESS_VIOLATION" on the instance. The code-mismatch skip at if exc_info.value.code != "UNIQUENESS_VIOLATION" then passes, and the test fails at assert isinstance(...) instead of skipping. Guard the tolerance on the class as well as the code so an evolved/unknown payload shape skips like the other server variants.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/integration/test_infrahub_client.py, line 221:
<comment>When the server emits UNIQUENESS_VIOLATION but the payload fails catalogue validation, `graphql_error_from_response` falls back to the generic GraphQLError while still assigning `code="UNIQUENESS_VIOLATION"` on the instance. The code-mismatch skip at `if exc_info.value.code != "UNIQUENESS_VIOLATION"` then passes, and the test fails at `assert isinstance(...)` instead of skipping. Guard the tolerance on the class as well as the code so an evolved/unknown payload shape skips like the other server variants.</comment>
<file context>
@@ -200,6 +206,41 @@ async def test_query_unexisting_branch(self, client: InfrahubClient) -> None:
+ with pytest.raises(GraphQLError, match=r"UNIQUENESS_VIOLATION|An error occurred while") as exc_info:
+ await duplicate.save()
+
+ if exc_info.value.code != "UNIQUENESS_VIOLATION":
+ pytest.skip(f"this server reports a uniqueness violation as {exc_info.value.code}")
+
</file context>
Suggested change
| if exc_info.value.code != "UNIQUENESS_VIOLATION": | |
| if exc_info.value.code != "UNIQUENESS_VIOLATION" or not isinstance(exc_info.value, UniquenessViolationError): | |
| pytest.skip(f"this server reports a uniqueness violation as {exc_info.value.code}") |
| readable, the raw `extensions` retained, and a debug log. A pydantic `ValidationError` never escapes | ||
| a raise path. | ||
| - [ ] T074 [US1] Confirm in `infrahub_sdk/exceptions/factory.py` that the fallback follows **the transport | ||
| - [X] T074 [US1] Confirm in `infrahub_sdk/exceptions/factory.py` that the fallback follows **the transport |
Contributor
There was a problem hiding this comment.
P3: T074 is marked complete with an overly narrow transport condition: the authentication factory also handles non-401 statuses from failed refresh attempts. Reword the checklist to distinguish the normal 401/403 response path from the refresh-failure exception.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At dev/specs/ifc-3034-error-catalogue/tasks.md, line 481:
<comment>T074 is marked complete with an overly narrow transport condition: the authentication factory also handles non-401 statuses from failed refresh attempts. Reword the checklist to distinguish the normal 401/403 response path from the refresh-failure exception.</comment>
<file context>
@@ -450,39 +450,39 @@ the raised type and the typed attributes, reading no message (quickstart scenari
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.
</file context>
| @@ -0,0 +1,20 @@ | |||
| Every code in Infrahub's error catalogue now has an exception class of its own, importable from `infrahub_sdk.exceptions`, carrying the failure's payload as directly typed attributes. Identifying a specific failure no longer means matching words in a message: | |||
Contributor
There was a problem hiding this comment.
P3: The opening sentence overstates the feature: three catalogue codes deliberately have no dedicated exception class, as this changelog later explains. Reword it to say that catalogue failures now have dedicated classes where applicable, so the release note is not self-contradictory.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At changelog/+typed-per-code-exceptions.added.md, line 1:
<comment>The opening sentence overstates the feature: three catalogue codes deliberately have no dedicated exception class, as this changelog later explains. Reword it to say that catalogue failures now have dedicated classes where applicable, so the release note is not self-contradictory.</comment>
<file context>
@@ -0,0 +1,20 @@
+Every code in Infrahub's error catalogue now has an exception class of its own, importable from `infrahub_sdk.exceptions`, carrying the failure's payload as directly typed attributes. Identifying a specific failure no longer means matching words in a message:
+
+```python
</file context>
Suggested change
| Every code in Infrahub's error catalogue now has an exception class of its own, importable from `infrahub_sdk.exceptions`, carrying the failure's payload as directly typed attributes. Identifying a specific failure no longer means matching words in a message: | |
| Catalogue failures can now be distinguished with dedicated exception classes where applicable, 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: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
The SDK cannot tell one server failure from another without reading message
strings. Infrahub publishes an error catalogue and already generates TypeScript
bindings for the frontend from it; this is the SDK half.
This PR lands the generated bindings and puts the exception classes on the
supported import path. The factory work that raises them per code (T066-T077)
follows on this branch.
Non-goals: the generator itself, which lives in
opsmill/infrahubPR #10678.IHS-295. Covers T063-T064; T066-T077 to follow.
What changed
infrahub_sdk/exceptions/catalogue.py- generated, committed, not to behand-edited. Nine exception classes, fifteen payload models, and a dispatch
that resolves a catalogue code and its payload to a concrete exception. The
three codes the server reports as 401 or 403 get a payload model but no class:
the SDK routes those by the response it saw.
infrahub_sdk/exceptions/__init__.py- re-exports the nine classes byname. Not
from .catalogue import *: a wildcard would also promote thepayload models, both lookup maps and the dispatch helper onto the package
surface, where a name is a stability promise. They stay importable from the
catalogue module, which is where the factory wants them.
Implementation notes:
tasks.mdT064 says to star-import. That is superseded, for the reason aboveand because
test_star_import_gives_the_exception_classes_and_nothing_elseenforces it.
base -> catalogue -> factory -> __init__, which is where the factory will need it in T072.public_names.jsonsnapshot gains the nine classes, 33 -> 42. That fixtureexists so the public surface stays a deliberate choice, so the addition is
explicit rather than inferred.
What stayed the same: no behavioural change yet. Nothing raises these classes
until the factory work lands; this PR only makes them exist and be importable.
How to review
The generated file is machine output - review the generator in
opsmill/infrahubPR #10678 instead, and readcatalogue.pyonly to confirmits shape: 9 classes, 15 models, no class for
AUTHENTICATION_REQUIRED,PERMISSION_DENIEDorTOKEN_EXPIRED.The reviewable change is
exceptions/__init__.pyand the three test updates.Test Plan
invoke lint-codeclean: ruff, ty and mypy across 163 files.from infrahub_sdk.exceptions import *leaks no payload model, nolookup map and no dispatch helper.
Assisted-by: opsmill-dev-commit 0.1.0
Assisted-by: opsmill-dev-pr 0.2.0
Summary by cubic
Raises a dedicated exception class for every catalogued failure, carrying the payload's fields as typed attributes, so code can branch on the exception type instead of parsing message strings. Nine new classes are importable from
infrahub_sdk.exceptions; unknown codes, older servers, and malformed payloads still fall back to the generic class withexc.codereadable.Notes
catalogue.pyis generated inopsmill/infrahubPR #10678 and must not be hand-edited.AUTHENTICATION_REQUIRED,PERMISSION_DENIED, andTOKEN_EXPIREDget payload models but no class; the SDK routes them by the transport it observed.except GraphQLErrornow also catches server-reported node, branch, and schema lookup failures;exc.code is not Nonedistinguishes those from SDK-side raises.Written for commit d1d10c6. Summary will update on new commits.