fix: Change data type for form data chunks#6173
Conversation
Dav1dde
left a comment
Dav1dde
left a comment
There was a problem hiding this comment.
I think this is the wrong approach.
We can limit the parts to some upper bound, but that should happen in the ChunkedFormAggregator. This isn't necessary if we switch the backing data structure and how the parts are assembled from a vec to a different data structure.
elramen
changed the title
Smart! Changed it now :) |
| /// Returns the string consisting of all parts. | ||
| pub fn join(&self) -> String { | ||
| self.parts.join("") | ||
| self.parts.values().copied().collect() |
There was a problem hiding this comment.
So the contract here is "we'll assemble your parts in ascending order", gaps are allowed?
There was a problem hiding this comment.
This is what it was before, but we could also error here when there are gaps, I think. Seems like that's a case of invalid input.
Either way seems fine for a first fix.
| /// This writer is used to serialize multiple plain fields from a multipart form data request into a | ||
| /// single envelope item. Use `FormDataIter` to iterate all entries. | ||
| struct FormDataWriter { | ||
| pub struct FormDataWriter { |
There was a problem hiding this comment.
Is this just public for tests in a different module? Can the tests instead be in this module?
There was a problem hiding this comment.
I like to put tests in proximity to business logic - it helps to understand the logic and reduces clutter.
If you think that I am misprioritizing here, we can change it.
| /// Returns the string consisting of all parts. | ||
| pub fn join(&self) -> String { | ||
| self.parts.join("") | ||
| self.parts.values().copied().collect() |
There was a problem hiding this comment.
This is what it was before, but we could also error here when there are gaps, I think. Seems like that's a case of invalid input.
Either way seems fine for a first fix.
Dav1dde
left a comment
Dav1dde
left a comment
There was a problem hiding this comment.
Sorry, I thought I approved this already this morning - forgot to press the button.
3 participants
Fixes https://linear.app/getsentry/issue/INGEST-991/