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

Add md5 extension to md5 file #455

Merged
merged 4 commits into from
May 14, 2019
Merged

Add md5 extension to md5 file #455

merged 4 commits into from
May 14, 2019

Conversation

mtmckenna
Copy link
Contributor

Hello! This issue came up while trying to copy objects.

I think there's an issue with the getMetadata function overwriting the object file instead of the md5 file. Adding the md5 extension as in this PR seemed to do the trick.

Please let me know if I can provide any more info! Thank you!

@kherock
Copy link
Collaborator

kherock commented May 13, 2019

Thanks for the PR! Normally those md5 files are generated upon file upload, and that codepath should only happen if they were somehow deleted by the next time the object is read. Can you think of any reason why those files wouldn't be generated? Just curious.

@mtmckenna
Copy link
Contributor Author

Hello, @kherock ! I saw this behavior when doing a call to s3.copyObject from my code.

Doing the above in the aws-sdk eventually hits copyObject in filesystem.js. Depending on the conditional,copyObject calls getMetadata, which is what triggers the issue I'm seeing (the md5 file overwriting the object file).

Maybe another way to do this would be to make sure the md5 file is always copied during copyObject calls? Not sure how that would impact other things though.

I hope that helps--please let me know if I can provide any more info.

Thank you!

@kherock
Copy link
Collaborator

kherock commented May 13, 2019

That's really helpful - I was just looking for a way to capture this issue in a test case if possible since I was surprised this came up during normal use. It looks like the existing tests don't actually verify that the copied content matches. I'll aim to release a patch tonight!

@mtmckenna
Copy link
Contributor Author

That’s awesome! Your solution is much cleaner. Thank you for your help on this!

@kherock kherock merged commit 0c1f1c1 into jamhall:master May 14, 2019
@kherock
Copy link
Collaborator

kherock commented May 14, 2019

v3.1.0 is published, thanks again for catching this!

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.

None yet

2 participants