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

Calculate settings for S3Boto3StorageFile when instantiated, not imported #930

Merged
merged 1 commit into from Nov 16, 2020

Conversation

brianhelba
Copy link
Contributor

This allows the storages.backends.s3boto3 module to be cleanly imported before Django settings are configured. This further supports the changes from #524.

Removing S3Boto3StorageFile.buffer_size as a class variable does not affect the API surface of S3Boto3StorageFile, as buffer_size was always accessible as an instance variable.

…rted

This allows the "storages.backends.s3boto3" module to be cleanly imported
before Django settings are configured. This further supports the changes
from jschneier#524.

Removing "S3Boto3StorageFile.buffer_size" as a class variable does not
affect the API surface of "S3Boto3StorageFile", as "buffer_size" was
always accessible as an instance variable.
@brianhelba
Copy link
Contributor Author

@jschneier @jdufresne I'd welcome your feedback, as this supports #524.

@brianhelba
Copy link
Contributor Author

Ping @jschneier @jdufresne

Is there anything I should do to support review of this PR?

@jdufresne
Copy link
Contributor

The change looks good to me. Could a test be added to demonstrate the effect of the change?

@brianhelba
Copy link
Contributor Author

In the test environment, the Django settings are always implicitly configured on-demand, since the DJANGO_SETTINGS_MODULE environment variable is set.

The proof that this PR works is that from a clean Python shell (where DJANGO_SETTINGS_MODULE is not set),

from storages.backends import s3boto3

raises

django.core.exceptions.ImproperlyConfigured: Requested setting AWS_S3_FILE_BUFFER_SIZE, but settings are not configured. You must either define the environment variable DJANGO_SETTINGS_MODULE or call settings.configure() before accessing settings.

before this change and successfully imports after this change.

I could add an additional Tox test environment where DJANGO_SETTINGS_MODULE is not set, and where the test code attempts to import all of the the modules within storages. However, that seems like a lot of additional test infrastructure to maintain. On the other hand, if everyone agrees that being able to cleanly import is an important feature (since it is a major goal of #524), maybe having a test to prevent a regression is worth it.

I'd be happy to write the extra testing if the project maintainers want it.

@jdufresne
Copy link
Contributor

The following test demonstrates the change, IMO. It fails before the fix and passes aftewards.

    def test_override_file_buffer_size(self):
        f = s3boto3.S3Boto3StorageFile("foo.txt", "rb", mock.MagicMock())
        self.assertEqual(f.buffer_size, 5242880)
        with override_settings(AWS_S3_FILE_BUFFER_SIZE=1024):
            f = s3boto3.S3Boto3StorageFile("foo.txt", "rb", mock.MagicMock())
            self.assertEqual(f.buffer_size, 1024)

@jschneier jschneier merged commit b743396 into jschneier:master Nov 16, 2020
@brianhelba brianhelba deleted the boto3-import branch October 20, 2021 04:29
mlazowik pushed a commit to qedsoftware/django-storages that referenced this pull request Mar 9, 2022
…rted (jschneier#930)

This allows the "storages.backends.s3boto3" module to be cleanly imported
before Django settings are configured. This further supports the changes
from jschneier#524.

Removing "S3Boto3StorageFile.buffer_size" as a class variable does not
affect the API surface of "S3Boto3StorageFile", as "buffer_size" was
always accessible as an instance variable.
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

3 participants