Skip to content

Conversation

@mkrasnitski
Copy link
Collaborator

Implements #2690, as a second attempt at #2774. Defers reading from paths/files until the data is actually needed, which improves the performance of multipart file uploads by streaming directly from disk instead of copying into memory first. Cases where the file is included as a base64 string in the json payload are unaffected since the data needs to be encoded and written into the payload.

@github-actions github-actions bot added model Related to the `model` module. builder Related to the `builder` module. http Related to the `http` module. examples Related to Serenity's examples. labels Mar 15, 2025
Copy link
Member

@GnomedDev GnomedDev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this, but I have some concerns. Alongside the review comment, I dislike how a bunch of methods have been made async and falliable, instead of just taking the data.

Maybe we need to stop using CreateAttachment for these methods, taking a newtype around String if we want to enforce the base64 encoding with the type system? If so, please do this as a separate commit.

@GnomedDev GnomedDev added the needs-testing A PR that should work, but needs a functional test label Mar 26, 2025
@mkrasnitski
Copy link
Collaborator Author

We could do that, unfortunately the data needs to be encoded as a Data URL, not just raw base64, and therefore needs to have a valid prefix like data:image/png;base64,, so there's a tradeoff. We could certainly validate the string that gets passed in, but that would keep those methods fallible, though it would make them sync again.

@GnomedDev
Copy link
Member

The newtype can enforce the prefix as well, I'm fine with that, that keeps the builders infallible.

@mkrasnitski mkrasnitski requested a review from GnomedDev March 28, 2025 06:06
Copy link
Member

@GnomedDev GnomedDev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking much better, especially like the ImageData changes. Just a few more concerns though.

@mkrasnitski mkrasnitski requested a review from GnomedDev April 1, 2025 13:22
Copy link
Member

@GnomedDev GnomedDev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good, once someone does some testing on this I'm good with merging.

@mkrasnitski
Copy link
Collaborator Author

Tested this with message attachments and modifying a webhook's avatar, works fine in both cases.

@GnomedDev GnomedDev removed the needs-testing A PR that should work, but needs a functional test label Apr 20, 2025
@GnomedDev GnomedDev merged commit f5e6c77 into serenity-rs:next Apr 20, 2025
24 checks passed
@mkrasnitski mkrasnitski deleted the stream branch April 27, 2025 13:06
mkrasnitski added a commit to mkrasnitski/serenity that referenced this pull request Jun 30, 2025
mkrasnitski added a commit to mkrasnitski/serenity that referenced this pull request Jun 30, 2025
mkrasnitski added a commit to mkrasnitski/serenity that referenced this pull request Jun 30, 2025
mkrasnitski added a commit to mkrasnitski/serenity that referenced this pull request Jul 28, 2025
mkrasnitski added a commit to mkrasnitski/serenity that referenced this pull request Jul 28, 2025
mkrasnitski added a commit to mkrasnitski/serenity that referenced this pull request Oct 7, 2025
mkrasnitski added a commit to mkrasnitski/serenity that referenced this pull request Oct 7, 2025
mkrasnitski added a commit to mkrasnitski/serenity that referenced this pull request Oct 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

builder Related to the `builder` module. examples Related to Serenity's examples. http Related to the `http` module. model Related to the `model` module.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants