Improve handling Var's when passing them as data to rx.download(...)#5961
Improve handling Var's when passing them as data to rx.download(...)#5961riebecj wants to merge 4 commits intoreflex-dev:mainfrom
Var's when passing them as data to rx.download(...)#5961Conversation
CodSpeed Performance ReportMerging #5961 will not alter performanceComparing Summary
|
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR enhances the rx.download() function to properly handle different types of Var objects when downloading files. The changes address critical formatting issues that occurred when downloading files with string or byte data from state variables. The solution introduces new JavaScript function variables (BASE64_ENCODE and CREATE_OBJECT_URL) and adds a to_blob() method to ArrayVar for converting byte arrays to JavaScript Blob objects.
The implementation distinguishes between three data types: Var[bytes] creates Blob objects using Uint8Array for proper binary downloads, Var[str] preserves string formatting using base64 encoding instead of URL encoding, and Var[Any] stringifies other data types. This approach works within Reflex's existing variable ecosystem rather than modifying embedded JavaScript files, maintaining architectural consistency while solving fundamental data handling problems in file downloads.
Important Files Changed
| Filename | Score | Overview |
|---|---|---|
| reflex/vars/sequence.py | 3/5 | Adds to_blob() method to ArrayVar class for converting arrays to JavaScript Blob objects |
| reflex/event.py | 4/5 | Enhances rx.download() function with improved Var handling logic for different data types |
| reflex/vars/function.py | 4/5 | Introduces BASE64_ENCODE and CREATE_OBJECT_URL JavaScript function variables |
Confidence score: 3/5
- This PR addresses a legitimate bug but introduces potential security vulnerabilities that require careful review
- Score reflects concerns about JavaScript injection risks in the
to_blob()method wheremime_typeparameter is directly interpolated into JavaScript expressions without validation - Pay close attention to reflex/vars/sequence.py for the security vulnerability in the
to_blob()method's f-string interpolation
Sequence Diagram
sequenceDiagram
participant User
participant Frontend
participant Server
participant VarOperations
participant FileSystem
User->>Frontend: "Call rx.download with data parameter"
Frontend->>VarOperations: "Check data type (bytes/string/Var)"
alt data is bytes
VarOperations->>VarOperations: "Base64 encode bytes"
VarOperations->>VarOperations: "Create data URI with mime_type"
else data is string
VarOperations->>VarOperations: "Base64 encode UTF-8 string"
VarOperations->>VarOperations: "Create data URI with mime_type"
else data is Var[bytes] (ArrayVar)
VarOperations->>VarOperations: "Convert to Uint8Array"
VarOperations->>VarOperations: "Create Blob with mime_type"
VarOperations->>VarOperations: "Generate object URL from Blob"
else data is Var[str] (StringVar)
VarOperations->>VarOperations: "Base64 encode string data"
VarOperations->>VarOperations: "Create data URI"
else data is Var[Any]
VarOperations->>VarOperations: "Convert to string representation"
VarOperations->>VarOperations: "Base64 encode"
VarOperations->>VarOperations: "Create data URI"
end
VarOperations->>Server: "Create EventSpec with processed URL"
Server->>Frontend: "Return download event with URL and filename"
Frontend->>FileSystem: "Trigger browser download"
FileSystem-->>User: "File downloaded with correct format"
3 files reviewed, 2 comments
|
I don't think the failed test has anything to do with my code changes, but I can update if necessary. |
masenf
left a comment
There was a problem hiding this comment.
i like the direction here and tightening up the types, but i think we need to make Blob, object url, and base64 encoding more first class.
I'd like to see a BlobVar implementation, that can be created from the to_blob method on appropriate var types. And then create_object_url could be a method on the BlobVar to get a StringVar with the temporary object path.
I think it would also be cool if the StringVar had a to_base64 and from_base64 methods that properly handled unicode strings (the current BASE64_ENCODE helper would blow up on unicode characters.
With these in place, the behavior of rx.download would be: pass a StringVar, either we use it directly as a data: or blob: url OR we base64 encode it as a data uri with the given mime_type. if you pass a BlobVar, then we give you an object url to download, and any other kind of var would result in JSON (how it currently is handled today). If users need other type conversions, those should be implemented as Var operation methods the user is expected to translate with before calling rx.download.
At the end of the day, we need very consistent and understandable behavior with this API. It should strive to not make implicit conversions; so I think the behavior mentioned above would give everyone what they need.
|
@masenf, I can work on that. What about handling of Python |
|
Converting to draft to use as a work space for This will be abandoned and a new one created when the architecture and implementation of the referenced vars is agreed upon. |
|
@masenf , Take a look at I ran it through multiple example tests using direct State vars, computed vars, and even creating them directly. All seem to function correctly, download with proper formatting, and supports Unicode. |
|
All the type hinting seems to be there too. I was a little wary about using |
masenf
left a comment
There was a problem hiding this comment.
overall, this looks quite good. i left a couple of comments.
hopefully this work makes it easier to deal with blobby things, like media streams, in reflex
|
@masenf Made the changes you suggested. |
|
@masenf Any more suggestions? |
Related to #5954 and #5957.
When
datais provided as aVar, strings could sometimes have formatting issues and bytes wereArrayVar's that were really not handled at all, as there is no such thing as byte-strings in JavaScript. They were correctly converted to byte arrays, but the resultant downloaded file was just a string representation of the byte array.Var[bytes]: The byte array is coerced into aUint8Arrayand converted to aBlobusing the specifiedmime_type. The object URL is then created from the Blob.Var[str]: The string data is left intact to preserve formatting.Var[Any]: Any other supplied state or non-state variable is stringify-ed.All of this is done within the variable ecosystem, as there seems to be some aversion to changing any of the Reflex embedded JS (not a gripe, just an observation).
All Submissions:
Type of change
Please delete options that are not relevant.
New Feature Submission:
Changes To Core Features: