Fix Error when cloning MultiPart File #1903
Conversation
Signed-off-by: Gabriel Araujo <[email protected]>
Signed-off-by: Peter Leibiger <[email protected]>
|
@kuhnroyal thanks for the tips, PR is ready for review |
dio/lib/src/multipart_file.dart
Outdated
| headers = caseInsensitiveKeyMap(headers), | ||
| contentType = contentType ?? MediaType('application', 'octet-stream'); | ||
| contentType = contentType ?? MediaType('application', 'octet-stream'), | ||
| _stream = data.call(); |
There was a problem hiding this comment.
We don't need the internal _stream field anymore I believe.
There was a problem hiding this comment.
This field is returned in finalize(), should I change it to a call to _data?
There was a problem hiding this comment.
Yea I think this is fine, do you think this could be a problem?
There was a problem hiding this comment.
Nope, all good for me
| /// Creates a new [MultipartFile] from a chunked [Stream] of bytes. The length | ||
| /// of the file in bytes must be known in advance. If it's not, read the data | ||
| /// from the stream and use [MultipartFile.fromBytes] instead. | ||
| /// | ||
| /// [contentType] currently defaults to `application/octet-stream`, but in the | ||
| /// future may be inferred from [filename]. | ||
| MultipartFile( | ||
| Stream<List<int>> stream, | ||
| Stream<List<int>> Function() data, |
There was a problem hiding this comment.
We can not remove this, as it will break current users. We need to keep the stream parameter here but add a @Deprecated annotation on the whole constructor with a message that clone() will not work and that it will be removed in 6.0.0.
Then add a new factory MultipartFile.fromStream with the new data parameter.
There was a problem hiding this comment.
Okay, will do. If it's a new factory, it can't be called by other factories, right? Should I remove the other factories and make dataa named parameter, so that the old constructor can still work? I'm sorry I'm a bit confused with this
There was a problem hiding this comment.
Also, I believe what makes this solution work is that we're storing the stream generating method in _data, not the stream itself
There was a problem hiding this comment.
Your solution looks great, thanks for the help!
| this.length, { | ||
| this.filename, | ||
| MediaType? contentType, | ||
| Map<String, List<String>>? headers, | ||
| }) : _stream = stream, | ||
| }) : _data = data, |
There was a problem hiding this comment.
I don't understand this as well, sorry. Shouldn't data be a method reference that will be stored in that internal parameter _data to be called when cloning? If I just make it a method that will deliver the old stream, the stream would have already been listened to when I try to clone it, resulting in the same error that was happening before. Could you please help me understand that?
There was a problem hiding this comment.
To clarify, this is what I get when I make this change:
There was a problem hiding this comment.
Yea this was odd, not sure what Dart is doing there. I wrapped it in another pair of braces (() => stream) to please the compiler.
|
I pushed some changes, can you check if that works for you? |
|
It does! Thanks a lot @kuhnroyal, I was trying to make the new constructor with as a
|
Signed-off-by: Alex Li <[email protected]>
Signed-off-by: Alex Li <[email protected]>
clone()method in FormDataNew Pull Request Checklist
mainbranch to avoid conflicts (via merge from master or rebase)CHANGELOG.mdin the corresponding package