IHS-154 Deprecate using raise_for_error = False
#508
Conversation
WalkthroughAdded Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## develop #508 +/- ##
===========================================
- Coverage 75.83% 75.75% -0.09%
===========================================
Files 100 100
Lines 9303 8854 -449
Branches 1826 1733 -93
===========================================
- Hits 7055 6707 -348
+ Misses 1756 1671 -85
+ Partials 492 476 -16
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 8 files with indirect coverage changes 🚀 New features to boost your workflow:
|
Deploying infrahub-sdk-python with
|
| Latest commit: |
74dffe1
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://49831d03.infrahub-sdk-python.pages.dev |
| Branch Preview URL: | https://gma-20250825-ihs154.infrahub-sdk-python.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/unit/sdk/test_client.py (1)
805-835: Use realistic GraphQL error shape and assert the message; optionally validate the raised error content.GraphQL errors are typically dict objects with a
messagefield, e.g.{"errors": [{"message": "foo"}]}. Mocking as a bare list of strings works with current code but diverges from spec and from how you extract messages elsewhere (e.g.,error.get("message")patterns).Recommended tweaks in this test:
- Mock
errorsas a list of dicts with a message.- When asserting the non-raising path, assert
response[0]["message"] == "foo".- Optionally, capture the exception and assert its
.errorspayload.Apply within this hunk:
- httpx_mock.add_response(method="POST", json={"errors": ["foo"]}, is_reusable=True) + httpx_mock.add_response(method="POST", json={"errors": [{"message": "foo"}]}, is_reusable=True) @@ - if client_type == "standard": - with pytest.raises(GraphQLError): - await clients.standard.execute_graphql(query=query, raise_for_error=True) + if client_type == "standard": + with pytest.raises(GraphQLError) as excinfo: + await clients.standard.execute_graphql(query=query, raise_for_error=True) @@ - with pytest.raises(GraphQLError): - clients.sync.execute_graphql(query=query, raise_for_error=True) + with pytest.raises(GraphQLError) as excinfo: + clients.sync.execute_graphql(query=query, raise_for_error=True) @@ - assert response - assert response[0] == "foo" + assert response + assert response[0]["message"] == "foo" + # Optional: validate error content on the raised path as well + # assert excinfo.value.errors[0]["message"] == "foo"Nice-to-have: add a sibling test that sets
status_code=400with the same error body to ensure HTTP status isn’t raised whenraise_for_error=False(sinceraise_for_status()is gated by the flag).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
infrahub_sdk/client.py(2 hunks)tests/unit/sdk/test_client.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
When implementing Infrahub checks, subclass InfrahubCheck and override validate(data); do not implement or rely on a check() method
Files:
infrahub_sdk/client.pytests/unit/sdk/test_client.py
infrahub_sdk/client.py
📄 CodeRabbit inference engine (CLAUDE.md)
infrahub_sdk/client.py: Use HTTPX for transport with proxy support (single proxy or HTTP/HTTPS mounts)
Support authentication via API tokens or JWT with automatic refresh
Files:
infrahub_sdk/client.py
tests/unit/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Place and write unit tests under tests/unit/ (isolated component tests)
Files:
tests/unit/sdk/test_client.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use the custom pytest plugin (infrahub_sdk.pytest_plugin) fixtures for clients, configuration, and Infrahub-specific testing
Files:
tests/unit/sdk/test_client.py
🧠 Learnings (2)
📚 Learning: 2025-08-24T13:35:12.974Z
Learnt from: CR
PR: opsmill/infrahub-sdk-python#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-24T13:35:12.974Z
Learning: Applies to infrahub_sdk/client.py : Use HTTPX for transport with proxy support (single proxy or HTTP/HTTPS mounts)
Applied to files:
tests/unit/sdk/test_client.py
📚 Learning: 2025-08-24T13:35:12.974Z
Learnt from: CR
PR: opsmill/infrahub-sdk-python#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-24T13:35:12.974Z
Learning: Applies to tests/**/*.py : Use the custom pytest plugin (infrahub_sdk.pytest_plugin) fixtures for clients, configuration, and Infrahub-specific testing
Applied to files:
tests/unit/sdk/test_client.py
🧬 Code graph analysis (2)
infrahub_sdk/client.py (1)
infrahub_sdk/exceptions.py (1)
GraphQLError(40-46)
tests/unit/sdk/test_client.py (2)
infrahub_sdk/exceptions.py (1)
GraphQLError(40-46)infrahub_sdk/client.py (2)
execute_graphql(877-959)execute_graphql(1623-1705)
🔇 Additional comments (2)
infrahub_sdk/client.py (1)
1699-1702: I’ve added scripts to extract and compare the error-handling blocks for both the async and syncexecute_graphqlmethods. Once results are available, we can confirm whether the sync variant still returns a union type and lacks the updated docstring or guards, and then update the review accordingly.tests/unit/sdk/test_client.py (1)
7-7: LGTM: GraphQLError import is needed for the new tests.
|
We'd want a news fragment for this as well to indicate the bug that was addressed. |
gmazoyer
changed the title
raise_for_error = Falseraise_for_error = False
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
infrahub_sdk/client.py (4)
905-910: Point deprecation warning at the callerUse stacklevel=2 so the warning points to user code.
- warnings.warn( + warnings.warn( "Using `raise_for_error` is deprecated, use `try/except` to handle errors.", DeprecationWarning, - stacklevel=1, + stacklevel=2, )
1656-1661: Point sync deprecation warning at callerSame stacklevel adjustment.
- warnings.warn( + warnings.warn( "Using `raise_for_error` is deprecated, use `try/except` to handle errors.", DeprecationWarning, - stacklevel=1, + stacklevel=2, )
888-892: Minor typo in docstring“reacheable” → “reachable”.
- If retry_on_failure is True, the query will retry until the server becomes reacheable. + If retry_on_failure is True, the query will retry until the server becomes reachable.
1170-1176: DeprecationWarning onraise_for_errorwill surface at call sitesWe’ve confirmed there are multiple call sites where helpers still pass
raise_for_errorthrough, for example:
- infrahub_sdk/client.py:1174, 1474, 2319, 2466, 2615
- infrahub_sdk/ctl/validate.py:89
- infrahub_sdk/ctl/utils.py:125
Each of these will now emit a
DeprecationWarningat runtime. If that’s your intent—to remind consumers thatraise_for_erroris deprecated—you can leave them as-is. Otherwise, I recommend removing theraise_for_errorparameter from these helper calls in a follow-up PR to prevent unexpected warnings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
infrahub_sdk/client.py(7 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
When implementing Infrahub checks, subclass InfrahubCheck and override validate(data); do not implement or rely on a check() method
Files:
infrahub_sdk/client.py
infrahub_sdk/client.py
📄 CodeRabbit inference engine (CLAUDE.md)
infrahub_sdk/client.py: Use HTTPX for transport with proxy support (single proxy or HTTP/HTTPS mounts)
Support authentication via API tokens or JWT with automatic refresh
Files:
infrahub_sdk/client.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: integration-tests-latest-infrahub
- GitHub Check: unit-tests (3.12)
- GitHub Check: unit-tests (3.13)
- GitHub Check: unit-tests (3.11)
- GitHub Check: unit-tests (3.10)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (5)
infrahub_sdk/client.py (5)
7-7: Import for deprecation warnings — OKNecessary and scoped import.
885-887: Signature change to Optional[bool] matches the deprecation directionGood move to detect explicit usage via None default.
933-935: HTTP error raising gate — OK, but note behavior change when FalseWhen False, 401/403/404 won’t be mapped to AuthenticationError/URLNotFoundError via raise_for_status; document this as it differs from the default path.
Would you like me to add a short note to the docstring explaining this edge case?
1636-1638: Sync signature parity — OKMatches async variant and supports the same deprecation strategy.
1684-1686: HTTP error raising gate (sync) — OK, with same noteBehavior matches async; consider documenting the different exception mapping when False.
If desired, I can open a follow-up to align error-mapping behavior or explicitly document it.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
When looking through the SDK for other reasons I realise we have a number of methods that's using this raise_for_error for example .allocate_next_ip_prefix(), I think we need to deprecate those as well and avoid calling this deprecated parameter from those other methods as well. |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (11)
infrahub_sdk/client.py (11)
887-971: raise_for_error=False can lead to silent HTTP errors and KeyError; map auth/404 and guard ‘data’ accessWith raise_for_error=False, HTTPX won’t raise, so 401/403/404 bypass existing handlers and later
response["data"]can KeyError. Map those statuses even when not raising, and guard missing ‘data’.@@ - if not resp: + if not resp: raise Error("Unexpected situation, resp hasn't been initialized.") - response = decode_json(response=resp) + if raise_for_error is False: + if resp.status_code in (401, 403): + response = decode_json(response=resp) + messages = [error.get("message") for error in response.get("errors", [])] + raise AuthenticationError(" | ".join(messages) or "Authentication error") + if resp.status_code == 404: + raise URLNotFoundError(url=url) + + response = decode_json(response=resp) @@ - return response["data"] + data = response.get("data") + if data is None: + raise Error("Unexpected GraphQL response: missing 'data'.") + return data
1157-1167: Mirror minimal HTTP error mapping for query endpoints when raise_for_error=FalseAvoid returning a body that callers assume is OK when it’s a 401/403/404.
resp = await self._post( @@ - if raise_for_error in (None, True): - resp.raise_for_status() + if raise_for_error in (None, True): + resp.raise_for_status() + else: + if resp.status_code in (401, 403): + response = decode_json(response=resp) + messages = [error.get("message") for error in response.get("errors", [])] + raise AuthenticationError(" | ".join(messages) or "Authentication error") + if resp.status_code == 404: + raise URLNotFoundError(url=url) return decode_json(response=resp)
1186-1195: Use .get to avoid KeyError on DiffTreePrevents a crash if upstream response body lacks DiffTree (e.g., non-2xx when raise_for_error=False).
- diff_tree = response["DiffTree"] + diff_tree = response.get("DiffTree")
1336-1339: Harden mutation result accessGuard when mutation key is missing or ok is false; avoids KeyError.
- if response[mutation_name]["ok"]: - resource_details = response[mutation_name]["node"] + result = response.get(mutation_name) or {} + if result.get("ok"): + resource_details = result["node"] return await self.get(kind=resource_details["kind"], id=resource_details["id"], branch=branch) return None
1450-1466: Harden IP prefix allocation result accessSame guard pattern as address allocation to avoid KeyError.
- if response[mutation_name]["ok"]: - resource_details = response[mutation_name]["node"] + result = response.get(mutation_name) or {} + if result.get("ok"): + resource_details = result["node"] return await self.get(kind=resource_details["kind"], id=resource_details["id"], branch=branch) return NoneAlso applies to: 1485-1488
1637-1721: Same HTTP error handling bug in sync execute_graphql when raise_for_error=False@@ - if not resp: + if not resp: raise Error("Unexpected situation, resp hasn't been initialized.") - response = decode_json(response=resp) + if raise_for_error is False: + if resp.status_code in (401, 403): + response = decode_json(response=resp) + messages = [error.get("message") for error in response.get("errors", [])] + raise AuthenticationError(" | ".join(messages) or "Authentication error") + if resp.status_code == 404: + raise URLNotFoundError(url=url) + + response = decode_json(response=resp) @@ - return response["data"] + data = response.get("data") + if data is None: + raise Error("Unexpected GraphQL response: missing 'data'.") + return data
2302-2312: Sync query_gql_query: map 401/403/404 when raise_for_error=False- if raise_for_error in (None, True): - resp.raise_for_status() + if raise_for_error in (None, True): + resp.raise_for_status() + else: + if resp.status_code in (401, 403): + response = decode_json(response=resp) + messages = [error.get("message") for error in response.get("errors", [])] + raise AuthenticationError(" | ".join(messages) or "Authentication error") + if resp.status_code == 404: + raise URLNotFoundError(url=url)
2331-2340: Use .get to avoid KeyError on DiffTree (sync)- diff_tree = response["DiffTree"] + diff_tree = response.get("DiffTree")
2477-2480: Harden mutation result access (sync address allocation)- if response[mutation_name]["ok"]: - resource_details = response[mutation_name]["node"] + result = response.get(mutation_name) or {} + if result.get("ok"): + resource_details = result["node"] return self.get(kind=resource_details["kind"], id=resource_details["id"], branch=branch) return None
2626-2629: Harden mutation result access (sync prefix allocation)- if response[mutation_name]["ok"]: - resource_details = response[mutation_name]["node"] + result = response.get(mutation_name) or {} + if result.get("ok"): + resource_details = result["node"] return self.get(kind=resource_details["kind"], id=resource_details["id"], branch=branch) return None
1-10: Add CHANGELOG entry forraise_for_errordeprecationAdd a news fragment under the Unreleased section:
“Deprecate
raise_for_errorparameter usage; passing any value emits aDeprecationWarning; behavior unchanged for GraphQL errors.”
🧹 Nitpick comments (5)
infrahub_sdk/client.py (5)
109-116: Bump warning stacklevel and clarify deprecation messageMake the warning point to the user callsite (not the helper) and state eventual removal.
def raise_for_error_deprecation_warning(value: bool | None) -> None: if value is not None: warnings.warn( - "Using `raise_for_error` is deprecated, use `try/except` to handle errors.", + "Using `raise_for_error` is deprecated and will be removed in a future release; use try/except to handle errors.", DeprecationWarning, - stacklevel=1, + stacklevel=3, )
1311-1314: Align docstring with execute_graphql semantics for raise_for_error- raise_for_error (bool, optional): Deprecated, raise an error if the HTTP status is not 2XX. + raise_for_error (bool | None, optional): Deprecated. Controls only HTTP status handling. + - None (default) or True: HTTP errors raise via resp.raise_for_status(). + - False: HTTP errors are not automatically raised. + GraphQL errors always raise GraphQLError.
1463-1464: Sync wording across methods for deprecation text- raise_for_error (bool, optional): Deprecated, raise an error if the HTTP status is not 2XX. + raise_for_error (bool | None, optional): Deprecated. Controls only HTTP status handling. + None/True: HTTP errors raise; False: HTTP errors aren’t auto-raised. GraphQL errors always raise.
2454-2457: Fix incorrect docstring text for raise_for_error (sync address allocation)- raise_for_error (bool, optional): The limit for pagination. + raise_for_error (bool | None, optional): Deprecated. Controls only HTTP status handling. + None/True: HTTP errors raise; False: HTTP errors aren’t auto-raised. GraphQL errors always raise.
2603-2606: Fix incorrect docstring text for raise_for_error (sync prefix allocation)- raise_for_error (bool, optional): The limit for pagination. + raise_for_error (bool | None, optional): Deprecated. Controls only HTTP status handling. + None/True: HTTP errors raise; False: HTTP errors aren’t auto-raised. GraphQL errors always raise.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
changelog/493.changed.md(1 hunks)infrahub_sdk/client.py(28 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- changelog/493.changed.md
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
When implementing Infrahub checks, subclass InfrahubCheck and override validate(data); do not implement or rely on a check() method
Files:
infrahub_sdk/client.py
infrahub_sdk/client.py
📄 CodeRabbit inference engine (CLAUDE.md)
infrahub_sdk/client.py: Use HTTPX for transport with proxy support (single proxy or HTTP/HTTPS mounts)
Support authentication via API tokens or JWT with automatic refresh
Files:
infrahub_sdk/client.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: integration-tests-latest-infrahub
- GitHub Check: unit-tests (3.13)
🔇 Additional comments (1)
infrahub_sdk/client.py (1)
966-970: Resolved: execute_graphql no longer returns a list; GraphQL errors now always raiseThe previous union-return concern is addressed; callers can rely on dict or exception.
2 participants
This is a simple change that adds a warning when using a client's method with the named parameter
raise_for_errorset. This parameter must be removed later.Summary by CodeRabbit
Documentation
Chores