Skip to content

Fix #104 –– Add support for custom AWS_LOCATION or S3Boto3Storage.location #105

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

Merged
merged 1 commit into from
Jul 3, 2019

Conversation

aaronenberg
Copy link
Contributor

@aaronenberg aaronenberg commented Jun 17, 2019

This fix is for users who have set S3Boto3Storage.location to anything other than the default value of empty string. We just have to prepend default_storage.location to upload_path in forms.py so the input-tag gets it client-side then strip it back out before passing to default_storage.open(path) in middleware.py. We have to strip it out before this because S3Boto3Storage._normalize_name() will prepend the location to the path (as it should).

@codingjoe
Copy link
Owner

@aaronenberg thanks for the contribution.

I see you point. Your patch should work, but the tests to verify it, are missing.
Please also make sure to fix the existing tests.

@codingjoe codingjoe changed the title fix #104, upload path assumes S3Boto3Storage.location default value of empty string Fix #104 –– Add support for custom AWS_LOCATION or S3Boto3Storage.location Jun 18, 2019
@codecov
Copy link

codecov bot commented Jun 18, 2019

Codecov Report

Merging #105 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #105   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           4      4           
  Lines          69     76    +7     
=====================================
+ Hits           69     76    +7
Impacted Files Coverage Δ
s3file/storages.py 100% <100%> (ø) ⬆️
s3file/middleware.py 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bd51d3e...dc81c6b. Read the comment docs.

@codingjoe codingjoe force-pushed the master branch 4 times, most recently from 3c78ad9 to c2e0ae3 Compare June 30, 2019 15:49
@codingjoe
Copy link
Owner

@aaronenberg I did some changes to your pull-request. I would be ok merging this, but can you please tests this in your environment, to make sure everything is working fine? Thanks!

Copy link
Owner

@codingjoe codingjoe left a comment

Choose a reason for hiding this comment

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

LGTM, just waiting for confirmation from @aaronenberg

@aaronenberg
Copy link
Contributor Author

aaronenberg commented Jul 2, 2019

Tested and had to swap storages.utils.safe_join for django.utils._os.safe_join which returns absolute paths unlike django-storages version which returns a relative path to avoid accessing anything outside of the base location. Before the swap, when submitting the forms, files were saved under FileSystemStorage.base_location: /tmp/tmpasdfq234 but when calling FileSystemStorage.save() files were saved with a relative path tmp/tmpasdfq234. It all has to do with wherever FileSystemStorage.path() points, which should be an absolute path, hence the need for django.utils._os.safe_join. This way no matter how files are sent to storage, they are always submitted to the same place somewhere under the base location.

@codingjoe codingjoe merged commit bdebc3b into codingjoe:master Jul 3, 2019
@codingjoe
Copy link
Owner

🎉

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.

2 participants