IHS-193 Add support for file upload/download for CoreFileObject#792
IHS-193 Add support for file upload/download for CoreFileObject#792gmazoyer wants to merge 19 commits intoinfrahub-developfrom
CoreFileObject#792Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughAdds file upload/download support across async and sync SDK paths. Introduces MultipartBuilder and multipart payload helpers, FileHandler/FileHandlerSync and PreparedFile, ProxyConfig/ProxyConfigSync types, multipart POST/request and streaming GET helpers, and client methods execute_graphql_with_file. Node classes gain file state, upload_from_path/upload_from_bytes/clear_file, and download_file with validation. GraphQL variable types and constants updated to support Upload, and unit tests were added. 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Comment |
Deploying infrahub-sdk-python with
|
| Latest commit: |
72047a7
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://fec4665a.infrahub-sdk-python.pages.dev |
| Branch Preview URL: | https://gma-20260202-ihs193.infrahub-sdk-python.pages.dev |
Codecov Report❌ Patch coverage is @@ Coverage Diff @@
## infrahub-develop #792 +/- ##
====================================================
+ Coverage 80.36% 80.44% +0.08%
====================================================
Files 115 117 +2
Lines 9865 10225 +360
Branches 1504 1547 +43
====================================================
+ Hits 7928 8226 +298
- Misses 1415 1463 +48
- Partials 522 536 +14
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 2 files with indirect coverage changes 🚀 New features to boost your workflow:
|
Use two distinct methods instead of one to clearly show intent: * select_content_for_upload * select_file_for_upload
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@infrahub_sdk/file_handler.py`:
- Around line 98-118: The handler handle_error_response should guard against
non-JSON responses by wrapping each exc.response.json() call in a try/except
(catching json.JSONDecodeError or ValueError) and falling back to
exc.response.text; for 401/403, if json parsing fails treat the body text as a
single message (or empty) when building the AuthenticationError message, and for
404 use the fallback text as the detail when constructing NodeNotFoundError;
preserve the original HTTPStatusError as the cause (raise ... from exc).
In `@infrahub_sdk/node/node.py`:
- Around line 1160-1163: The error raised when self._file_object_support is true
and self._file_content is None references the wrong helper method name; update
the ValueError text in the block that checks self._file_object_support /
self._file_content (near usage of self._schema.kind) to instruct callers to use
the correct methods select_file_for_upload() or select_content_for_upload()
instead of set_file().
- Around line 2049-2052: The error message inside the check for
self._file_object_support and self._file_content should reference the actual
synchronous setter method name instead of the non-existent set_file(); update
the message to instruct callers to use the real sync setter (e.g.,
set_file_content()) — modify the ValueError string that currently mentions
set_file() so it references set_file_content() (or the actual sync method used
in this class) and keep the rest of the message intact; target the block that
checks _file_object_support and _file_content and update the message formatting
around _schema.kind accordingly.
🧹 Nitpick comments (1)
infrahub_sdk/client.py (1)
997-1035: Avoid mutating caller-providedvariablesin file-upload helpers.Both async and sync paths mutate the incoming dict. Prefer a new dict to avoid side effects.
Suggested refactor
- variables = variables or {} - variables["file"] = None + variables = {**(variables or {}), "file": None} @@ - variables = variables or {} - variables["file"] = None + variables = {**(variables or {}), "file": None}Also applies to: 2055-2093
|
I would prefer the |
gmazoyer
force-pushed
the
gma-20260202-ihs193
branch
2 times, most recently
from
e9e0f9e to
1b304a6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@infrahub_sdk/protocols.py`:
- Around line 110-115: The CoreFileObject class (and the other affected
generated blocks referenced at lines 234-237, 675-680, 799-802) were edited
manually but this file is generated; revert any hand edits to these definitions
and re-run the generation task or update the source schema so the generator
emits the correct definitions (e.g., restore CoreFileObject and the other
generated classes/fields to match the generator output), do not make direct
changes in infrahub_sdk/protocols.py, and commit the regenerated file instead.
🧹 Nitpick comments (1)
infrahub_sdk/node/node.py (1)
224-288: Reuse_validate_file_object_supportto avoid duplicated checks.
upload_from_pathandupload_from_bytesrepeat the same FeatureNotSupportedError logic; reusing the helper keeps error messaging consistent and centralized.♻️ Suggested refactor
- if not self._file_object_support: - raise FeatureNotSupportedError( - f"File upload is not supported for {self._schema.kind}. Only nodes inheriting from CoreFileObject support file uploads." - ) + self._validate_file_object_support( + message=( + f"File upload is not supported for {self._schema.kind}. " + "Only nodes inheriting from CoreFileObject support file uploads." + ) + ) @@ - if not self._file_object_support: - raise FeatureNotSupportedError( - f"File upload is not supported for {self._schema.kind}. Only nodes inheriting from CoreFileObject support file uploads." - ) + self._validate_file_object_support( + message=( + f"File upload is not supported for {self._schema.kind}. " + "Only nodes inheriting from CoreFileObject support file uploads." + ) + )Also applies to: 509-512
It was agreed to use `upload_from_path(path)` and `upload_from_bytes(content, name)`. Also update protocols which raised, it seems a valid error.
gmazoyer
force-pushed
the
gma-20260202-ihs193
branch
from
1b304a6 to
72047a7
Compare
2 participants
This change adds support to create, update and retrieve nodes which schemas implement
CoreFileObject.The proposed user exposed API follows these key points:
node.upload_from_path(path)to select a file from disk for upload (streams during upload)node.upload_from_bytes(content, name)to set content for upload (supports bytes or BinaryIO)node.download_file(dest)to download a file to memory or stream to diskBeing able to stream a file to disk or from a disk is important in order to support large files and to avoid them being loaded completely into memory (which would be problematic for +1GB files in general).
The choice of using
upload_from_pathandupload_from_bytesis to prevent a collision with a potential attribute or relationship calledfilein the schema. That is also the reason why thefileGraphQL parameter is outside thedataone instead of inside it.Here we introduce a couple of components to try to make our code SOLID (it's not much for now, but it's honest work):
FileHandler/FileHandlerSyncdedicated classes for file I/O operationsMultipartBuilderGraphQL Multipart Request Spec payload buildingIt sure won't make our code SOLID but it won't add to the burden for now.
So given the user who loaded a schema, using our SDK will look like:
Upload a file when creating a node
Download a file from a node
Summary by CodeRabbit
New Features
Tests