Fix ObjectStore.get() and upload() swallowing HTTP errors other than …#959
Merged
gmazoyer merged 2 commits intoinfrahub-developfrom Apr 22, 2026
Merged
Conversation
…401/403 ObjectStore.get() and ObjectStore.upload() (async + sync) caught httpx.HTTPStatusError in an except block that only converted 401/403 to AuthenticationError. For any other status code (404, 500, etc.) the exception was silently dropped and execution fell through to the return statement — `return resp.text` for get() and `return resp.json()` for upload(). Callers received the error body as if it were valid content. The downstream impact surfaced during INFP-504 testing: the new `artifact_content` Jinja2 filter calls `object_store.get()`; when the filter was passed a non-existent storage_id, the backend correctly returned HTTP 404 with a GraphQL-shaped error body, but the SDK silently returned that body as a string, which became the artifact's content. The artifact was marked Ready with a corrupt payload. Fix: add a trailing `raise` at the end of the `except httpx.HTTPStatusError` block in all four places, matching the pattern already used in `_get_file()` (lines 99 and 185). Non-401/403 HTTP errors now propagate to callers. Added unit tests covering: - get() raises httpx.HTTPStatusError on 404 - upload() raises httpx.HTTPStatusError on 500 - get() still converts 401/403 to AuthenticationError (unchanged) Both parametrised for async and sync clients. Fixes #958
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## infrahub-develop #959 +/- ##
====================================================
+ Coverage 81.11% 81.38% +0.27%
====================================================
Files 134 134
Lines 11314 11324 +10
Branches 1693 1693
====================================================
+ Hits 9177 9216 +39
+ Misses 1594 1566 -28
+ Partials 543 542 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 4 files with indirect coverage changes 🚀 New features to boost your workflow:
|
gmazoyer
reviewed
Apr 22, 2026
| @@ -0,0 +1 @@ | |||
| Fixed `ObjectStore.get()` and `ObjectStore.upload()` (async + sync) silently swallowing `httpx.HTTPStatusError` for HTTP status codes other than 401/403. The except block only converted 401/403 to `AuthenticationError` and fell through for all other non-2xx responses, causing `get()` to return the error body as string content and `upload()` to return the error payload as if it were an upload response. Downstream effect on INFP-504 artifact composition: `artifact_content` filter silently stored GraphQL-shaped 404 bodies as artifact content for missing storage IDs — a typo'd storage_id would deploy a broken config with no visible failure. Added a trailing `raise` to re-raise non-401/403 HTTP errors, matching the pattern already used in `_get_file()`. Added unit tests covering 404 on `get()`, 500 on `upload()`, and unchanged 401/403 → AuthenticationError conversion. | |||
Contributor
There was a problem hiding this comment.
Not sure how useful this changelog entry would be to end user. It mentions quite a bit the internals, which is useless IMO, but it also mention a Jira task which is not public and therefore should not be in this.
gmazoyer
reviewed
Apr 22, 2026
|
|
||
| @pytest.mark.parametrize("client_type", client_types) | ||
| async def test_object_store_get_raises_on_404(client_type: str, clients: BothClients, httpx_mock: HTTPXMock) -> None: | ||
| """get() must raise on 404 — otherwise the response body is silently returned as 'content'.""" |
gmazoyer
reviewed
Apr 22, 2026
| async def test_object_store_get_raises_authentication_error( | ||
| client_type: str, status_code: int, clients: BothClients, httpx_mock: HTTPXMock | ||
| ) -> None: | ||
| """get() must still convert 401/403 responses to AuthenticationError (unchanged behaviour).""" |
gmazoyer
reviewed
Apr 22, 2026
|
|
||
| @pytest.mark.parametrize("client_type", client_types) | ||
| async def test_object_store_upload_raises_on_500(client_type: str, clients: BothClients, httpx_mock: HTTPXMock) -> None: | ||
| """upload() must raise on server errors — otherwise resp.json() returns the error payload as if it were a successful upload.""" |
Simplify changelog entry to be user-facing and remove verbose test docstrings on self-explanatory tests.
Deploying infrahub-sdk-python with
|
| Latest commit: |
c265b84
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://1dd28359.infrahub-sdk-python.pages.dev |
| Branch Preview URL: | https://fix-issue-958-object-store-s.infrahub-sdk-python.pages.dev |
0590e20 to
c265b84
Compare
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
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
ObjectStore.get()andObjectStore.upload()(both async and sync) silently swallowhttpx.HTTPStatusErrorfor any non-2xx status code other than 401/403. Theexceptblock was written to convert 401/403 toAuthenticationErrorbut missed the trailingraisethat would re-surface other HTTP errors — so for 404, 500, etc. the exception is caught, nothing happens inside theif, and execution falls through toreturn resp.text/return resp.json(). The caller receives the error response body as if it were the requested content.The concrete downstream impact surfaced during INFP-504 testing: the new
artifact_contentJinja2 filter callsclient.object_store.get(identifier=storage_id). When passed a non-existent storage_id, the backend correctly returned HTTP 404 with a GraphQL-shaped error body, but the SDK silently returned that body as a string, which became the artifact's content. The artifact was markedReadywith a corrupt payload (checksum matched the MD5 of the 404 error JSON). A typo'd storage_id in a transform would deploy a broken config to a device with no visible failure upstream.Goal: make
get()andupload()raise on non-2xx non-auth responses, matching the pattern_get_file()already uses.Non-goals: not adding new typed exceptions, not changing the 401/403 →
AuthenticationErrorbehaviour, not touching any other call sites.Closes #958
What changed
httpx.HTTPStatusErroras expected. Callers that have been relying on the silent-return behaviour (storing error bodies as content) will now correctly fail fast. No legitimate success path changes.raiseat the end of theexcept httpx.HTTPStatusErrorblock in all four methods (ObjectStore.get,ObjectStore.upload,ObjectStoreSync.get,ObjectStoreSync.upload). The pre-existing_get_file()/ sync_get_filealready had this pattern — the fix brings the other four into line.test_object_store_get_raises_on_404,test_object_store_upload_raises_on_500,test_object_store_get_raises_authentication_error(parametrised over 401/403 to lock in the unchanged behaviour). All parametrised across async and sync clients.AuthenticationError, 2xx still returns content,ServerNotReachableErrorstill propagates, sync/async parity unchanged, public API unchanged.How to review
infrahub_sdk/object_store.py: four blocks, same one-line change (raiseat the end ofexcept httpx.HTTPStatusError).git diffshows six matches of the pattern"raise AuthenticationError(...) from exc\n raise"— four new, two pre-existing in_get_fileand sync_get_filethat I left untouched.tests/unit/sdk/test_object_store.py: three new parametrised test cases. The 401/403 test locks in the existing auth-conversion behaviour; the 404 and 500 tests would have FAILED on the pre-fix code (silent return of error body).changelog/958.fixed.md: towncrier fragment.How to test
Unit tests + linters (fast)
All three new tests should pass. To prove they catch the bug, revert one of the four
raisestatements inobject_store.pyand re-run: the corresponding test must fail.End-to-end reproduction (against a live Infrahub 1.9 stack)
Start an Infrahub 1.9 instance (
invoke demo.start). Export your token:Direct SDK reproduction:
End-to-end verification via the
artifact_contentfilter. This is optional if the direct SDK repro above is sufficient, but it exercises the full downstream code path including the worker.1. Load a minimal schema — a
TestingPersonnode that can be the target of an artifact:2. Create a person and add it to a group targeted by the artifact definition:
3. Prepare a small Git repo that uses
artifact_contentwith a storage_id that doesn't exist. Inside a worker container (or any directory Infrahub can reach as a git remote):4. Register the repo with Infrahub:
Wait ~15 seconds for the artifact pipeline to run.
5. Check the resulting artifact:
Then fetch the stored content:
Before this PR:
Artifact
status.value==Ready.checksum.value==7f6284048c7bcc3d6acc1697467f382e— the MD5 of the GraphQL error body.Fetched content is the 404 error JSON verbatim:
{"data":null,"errors":[{"message":"Unable to find the node ffffffff-ffff-ffff-ffff-ffffffffffff / StorageObject in the database.","extensions":{"code":404}}]}No errors in worker logs — the failure is completely silent. A consumer of this artifact would receive the error JSON as its config content with no warning.
After this PR:
Artifact
status.value==Error.storage_id.valueisNone.Worker logs (from
infp504-task-worker-*or equivalent) contain:The fix-behaviour contract is: anything that was going to produce corrupt content now produces a visible failure instead.
Impact & rollout
get()to always return a string (even for 404) would now need to catchhttpx.HTTPStatusError. Release notes should call this out.Checklist