Skip to content
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

Merged
merged 9 commits into from
Jul 14, 2023

Conversation

gabrielaraujoz
Copy link
Contributor

  • Add restoration capability to MultipartFile
  • Add tests to ensure MultipartFile is not changed during cloning, it just gets a new instance that can be consumed by FormData

Possible next steps: Create method in FormData to make it recoverable/clonable as well.

New Pull Request Checklist

  • I have read the Documentation
  • I have searched for a similar pull request in the project and found none
  • I have updated this branch with the latest main branch to avoid conflicts (via merge from master or rebase)
  • I have added the required tests to prove the fix/feature I'm adding
  • I have updated the documentation (if necessary)
  • I have run the tests without failures
  • I have updated the CHANGELOG.md in the corresponding package

Additional 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!

@gabrielaraujoz gabrielaraujoz requested a review from a team as a code owner July 6, 2023 11:50

/// 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(
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

humm, how about reset?

Copy link
Contributor Author

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 :)

Comment on lines 144 to 145
/// 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.
Copy link
Member

@AlexV525 AlexV525 Jul 10, 2023

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.

Suggested change
/// 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.

Copy link
Contributor Author

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

Copy link
Member

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.

Copy link
Contributor Author

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

@gabrielaraujoz
Copy link
Contributor Author

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!

@kuhnroyal
Copy link
Member

Can we not just call the method clone()?

@gabrielaraujoz
Copy link
Contributor Author

@kuhnroyal awesome, done! @AlexV525 fyi

dio/CHANGELOG.md Outdated Show resolved Hide resolved
Signed-off-by: Peter Leibiger <[email protected]>
@gabrielaraujoz
Copy link
Contributor Author

@kuhnroyal Thanks a lot for the approval! @AlexV525 do we have any perspective on when we could release this change?

Copy link
Member

@AlexV525 AlexV525 left a 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.

@yuriymalyts

This comment was marked as outdated.

@dipu0
Copy link

dipu0 commented Sep 20, 2024

@AlexV525
why MultipartFile.clone() will Deprecated? i faced this when i use fromStrem or fromFile in the time of retry. Unhandled Exception: Bad state: The MultipartFile has already been finalized. This typically means you are using the same MultipartFile in repeated requests.

but MultipartFile.fromFileSync(value.path).clone(); works great!

@AlexV525
Copy link
Member

@AlexV525 why MultipartFile.clone() will Deprecated? i faced this when i use fromStrem or fromFile in the time of retry. Unhandled Exception: Bad state: The MultipartFile has already been finalized. This typically means you are using the same MultipartFile in repeated requests.

I don't see we've deprecated the MultipartFile.clone(), only we deprecated the unnamed constructor, and its deprecated message means: MultipartFile() is not cloneable when the stream is consumed, use the MultipartFile.fromStream instead.

@dipu0
Copy link

dipu0 commented Sep 20, 2024

Then your deprecated message is not clear to me at least!
But thanks for the quick reply, that clone method is a life saver for me.

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!

AlexV525 added a commit that referenced this pull request Oct 7, 2024
Resolves #1889 (comment)

---------

Signed-off-by: Alex Li <[email protected]>
Co-authored-by: Jonas Uekötter <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants