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

fix(storage): Download to existing file #2116

Merged
merged 3 commits into from
Sep 16, 2022

Conversation

dnys1
Copy link
Contributor

@dnys1 dnys1 commented Sep 13, 2022

Fixes #2105.

Fixes an issue where downloading to an existing file will not work on iOS. For whatever reason, it will report progress and success, but will ultimately not overwrite the contents of the file.

@dnys1 dnys1 requested a review from a team as a code owner September 13, 2022 00:51
@dnys1 dnys1 force-pushed the chore/storage/integ-tests branch from 50b02d6 to 48f743c Compare September 13, 2022 00:52
Comment on lines 155 to 160
// Delete the file if it already exists since iOS will only write to
// a non-existent file; it will not overwrite the contents of an existing
// one.
if (Platform.isIOS && await request.local.exists()) {
await request.local.delete();
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the actual fix. The rest is updating tests and Xcode files.

Copy link
Member

Choose a reason for hiding this comment

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

If the download fails this will result in the original file being deleted without the new file being downloaded. Is that expected? Should this instead write to a temp directory and then copy it over?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since the intention is to overwrite whatever is passed in, then I think this is probably fine, although to be safe I can update it 👍

@dnys1 dnys1 enabled auto-merge (rebase) September 13, 2022 00:54
@HuiSF
Copy link
Member

HuiSF commented Sep 13, 2022

Thanks for fixing this issue!

amplify-android implements the logic if file exists (1.x, 2.x), it deletes the original file before write the new file. IMO, we should also file an issue to amplify-ios to implement the same logic instead of fixing in amplify-flutter (we may merge this PR as a temporary fix?). This behavior should be consistent cross Amplify platform libraries.

@dnys1 dnys1 force-pushed the chore/storage/integ-tests branch from 93dca63 to 81c1acd Compare September 14, 2022 18:04
Dillon Nys added 3 commits September 15, 2022 11:47
Fixes aws-amplify#2105.

Fixes an issue where downloading to an existing file will not work on iOS. For whatever reason, it will report progress and success, but will ultimately not overwrite the contents of the file.

commit-id:157d4ea3
First downloads to a non-existent file, then copies the file to the location of the original in order to avoid iOS restrictions on overwriting data and potential failures which would result in deletion of the original file.

commit-id:7fbfb559
@dnys1 dnys1 force-pushed the chore/storage/integ-tests branch from 81c1acd to 355dc65 Compare September 15, 2022 18:48
@dnys1 dnys1 merged commit 8c3c256 into aws-amplify:main Sep 16, 2022
@dnys1 dnys1 deleted the chore/storage/integ-tests branch September 16, 2022 22:50
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.

Downloaded file is empty in iOS
3 participants