-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
✨ Make MultipartFile recoverable to enable retrying FormData requests #1889
Conversation
dio/lib/src/multipart_file.dart
Outdated
|
||
/// Restore MultipartFile, returning a new instance of the same object. This is useful if your request failed and you wish | ||
/// to sistematically retry it, for example a 401 error that can be solved by refreshing the token. | ||
MultipartFile restoreMultipartFile() => MultipartFile( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure restore
is a good word here because it represents manipulating the original object but here we produce a new one. Any other ideas? Other words like copy
or duplicate
might fit more in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
humm, how about reset
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I renamed it as duplicateMultipartFile, but let me know if you think reset would fit better :)
dio/lib/src/multipart_file.dart
Outdated
/// Restore MultipartFile, returning a new instance of the same object. This is useful if your request failed and you wish | ||
/// to sistematically retry it, for example a 401 error that can be solved by refreshing the token. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update codes to follow the 80-word length format or semantic line breaks.
/// Restore MultipartFile, returning a new instance of the same object. This is useful if your request failed and you wish | |
/// to sistematically retry it, for example a 401 error that can be solved by refreshing the token. | |
/// Restore MultipartFile, returning a new instance of the same object. | |
/// This is useful if your request failed and you wish to retry it, | |
/// such as an unauthorized exception can be solved by refreshing the token. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I applied the line length rules to all the changes I made, thanks
dio/test/formdata_test.dart
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new method seems not tested though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed the tests to comply to this, could you please check it out? a22208d
Signed-off-by: Gabriel Araujo <[email protected]>
Hey @AlexV525, hope you're doing well. Could you please take a look and let me know if the changes are ok? We're currently dealing with this bug in production and would love to fix it with this PR. Thanks a lot! |
Can we not just call the method |
@kuhnroyal awesome, done! @AlexV525 fyi |
Signed-off-by: Peter Leibiger <[email protected]>
@kuhnroyal Thanks a lot for the approval! @AlexV525 do we have any perspective on when we could release this change? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution.
This comment was marked as outdated.
This comment was marked as outdated.
@AlexV525 but MultipartFile.fromFileSync(value.path).clone(); works great! |
I don't see we've deprecated the |
Then your deprecated message is not clear to me at least! But use clones is a nice addition, but dio should not throw this error message in the first place. Thanks again for clearing my confusion! |
Resolves #1889 (comment) --------- Signed-off-by: Alex Li <[email protected]> Co-authored-by: Jonas Uekötter <[email protected]>
Possible next steps: Create method in FormData to make it recoverable/clonable as well.
New Pull Request Checklist
main
branch to avoid conflicts (via merge from master or rebase)CHANGELOG.md
in the corresponding packageAdditional context and info (if any)
Hello there!
We at Revelo use Dio for all of our http requests. We also use MultiPart a lot and have gotten a significant amount of errors when retrying to send a MultiPart File because the stream has already been listened to/finalized.
exception: Bad state: The FormData has already been finalized. This typically means you are using the same FormData in repeated requests.. Error thrown .
This is a common use case for ourselves (and we think for others as well) due to access token's short duration. As it expires, we automatically renew it with the refresh token, and finally do the original request again, that's why we fall into this problem.
I thought of a possible way to make retrying these requests possible and it involves, as a first step, making MultipartFile store the original source path/value and be able to "clone" itself with a new stream generated from the same source path.
Even with this change, to retry the request we will still need to recreate FormData. But this small change already makes it possible to recreate it without the need of a secondary interface that would just be used to store the original source path. This proposed change avoids a lot of boilerplate code and extra state management, since we can "reset" the MultipartFile to it's initial state by simply calling
restoreMultipartFile()
.With this, we could retrieve add a new restored
FormData
in the RequestOptions object, and would not need to have a secondary solution just to get the file paths for each MultipartFile and each request.WDYT? Would this ideia be a good addition to Dio? I plan to make another PR with a FormData.clone() convenience method to avoid even more boilerplate code once I have a little more spare time, but this second PR would require us to first apply the proposed change in this one.
Thanks a lot, best regards!