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

Allow a way to configure `allowS3ExistenceOptimization` #175

Closed
batpad opened this Issue Feb 9, 2019 · 3 comments

Comments

Projects
None yet
2 participants
@batpad
Copy link

batpad commented Feb 9, 2019

Let me start off describing the problem we are facing.

We have two separate models in Django that both have s3direct fields, with different destination paths. So, say, model A that saves to dest/a/ and model B that saves to dest/b/ - now a user uploads a file foo.jpg to model A, and that gets saved at /dest/a/ - then user goes to model B and happens to upload the same file. I would expect that file to get uploaded to /dest/b/ but instead, the location of the file in Model B points to the existing file at /dest/a/. This was surprising, and in our case, causes undesirable behaviour as we have parts of code that expect uploads from Model A to always be at /dest/a/ and uploads from Model B to always be at /dest/b/

It seems this behaviour comes from options passed to the underlying Evaporate library that handles the uploading to s3, specifically this line: https://github.com/bradleyg/django-s3direct/blob/master/src/index.js#L169 . It would be great to be able to pass in configuration from the outside to specify whether or not to enable this behaviour. I can imagine that it is useful in lots of cases, but can also be unwanted in others. If this seems reasonable, am happy to submit a PR.

Thank you for a wonderful library!

@bradleyg

This comment has been minimized.

Copy link
Owner

bradleyg commented Feb 9, 2019

So, I didn't really understand how much of an issue this was until your detailed explanation, so thanks for that. Somebody actually created a pull request for this #157, but I wanted to ensure it remained optional. Would love a PR for this!

Thanks

@bradleyg

This comment has been minimized.

Copy link
Owner

bradleyg commented Feb 9, 2019

Guess it should even be disabled by default...

@bradleyg

This comment has been minimized.

Copy link
Owner

bradleyg commented Feb 10, 2019

Fixed in 962a55c, thanks!

@bradleyg bradleyg closed this Feb 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.