Skip to content
This repository has been archived by the owner on Jan 16, 2023. It is now read-only.

Properly handle non-S3 storage backends #124

Closed
wants to merge 8 commits into from
Closed

Properly handle non-S3 storage backends #124

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
855b12a
Update gitignore
johananl Jan 9, 2018
96d5b7e
Test disabled mode with local storage configured
johananl Jan 28, 2018
7b55b97
Don't check preload_metadata when disabled
johananl Jan 28, 2018
b2c4803
Test enabled with non-S3 storage
johananl Jan 28, 2018
9998365
Error when enabled with non-S3 storage
johananl Jan 29, 2018
9244b6b
Store valid backends in a constant
johananl Jan 29, 2018
9dddbcb
Reformat long lines
johananl Jan 29, 2018
7dd3ca5
Refactor conditionals
johananl Jan 29, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,4 @@
*.egg-info
/static/
/static_root/
.idea
28 changes: 21 additions & 7 deletions collectfast/management/commands/collectstatic.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,20 @@
from multiprocessing.dummy import Pool
import warnings

from django.conf import settings as django_settings
from django.contrib.staticfiles.management.commands import collectstatic
from django.utils.encoding import smart_str

from collectfast.etag import should_copy_file
from collectfast.boto import reset_connection
from collectfast import settings

# Backends with which Collectfast should work properly.
VALID_BACKENDS = [
'storages.backends.s3boto3.S3Boto3Storage',
'storages.backends.s3boto.S3BotoStorage',
]


class Command(collectstatic.Command):
def add_arguments(self, parser):
Expand All @@ -32,13 +39,20 @@ def __init__(self, *args, **kwargs):
self.tasks = []
self.etags = {}
self.collectfast_enabled = settings.enabled
if self.storage.preload_metadata is not True:
self.storage.preload_metadata = True
warnings.warn(
"Collectfast does not work properly without "
"`preload_metadata` set to `True` on the storage class. Try "
"setting `AWS_PRELOAD_METADATA` to `True`. Overriding "
"`storage.preload_metadata` and continuing.")
if settings.enabled:
if django_settings.STATICFILES_STORAGE not in VALID_BACKENDS:
Copy link

Choose a reason for hiding this comment

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

one problem with this is that it would cause a RuntimeError if you're using a subclass of S3BotoStorage/S3Boto3Storage. I see two other options:

(1) import the classes and use if isinstance(self.storage, S3BotoStorage) or isinstance(self.storage, S3Boto3Storage) (which could be refactored into a helper: if is_s3_backend(self.storage))
(2) duck typing: if not hasattr(self.storage, 'preload_metadata'): raise --> but I'm wondering if we'd just want to set preload_metadata defensively in that case:

if not getattr(self.storage, 'preload_metadata', False):
    self.storage.preload_metadata = True
    warnings.warn(...)

Copy link
Author

Choose a reason for hiding this comment

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

Yes, that's a good point @pachewise.

Actually I wanted to implement it as in your first suggestion but encountered problems with mocked objects in some existing tests.

In any case, I think both of your suggestions could work, however looks like we have bigger questions now following @antonagestam's comment below. Let's clear the desired behavior first, and then I'll solve the problems in the implementation.

raise RuntimeError(
"Collectfast is intended to work with an S3 storage "
"backend only."
)
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think this is the desired behaviour. In case a storage backend is configured that isn't S3 we want to handle it gracefully. I've had a plan (in my head) for some time to introduce an abstraction layer for what strategy to use for different backends, but I think that's a much larger scope than what's needed here.

I believe all we want to do is skip the md5/etag comparison if the backend isn't s3. If it's feasible I think it'd be cool if we could still have threading enabled.

Copy link
Author

@johananl johananl Feb 5, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for your comment @antonagestam.

Yes, I understand the desire to handle non-S3 backends gracefully. Just to verify I understood you correctly, you're saying that when settings.enabled == True we should handle things gracefully even if the storage backend isn't S3, right?
If that's the case, we must check the backend type before referencing self.storage.preload_metadata, otherwise we get an AttributeError with non-S3 storages. As a reminder, in #120 you've suggested to do:

if settings.enabled and self.storage.preload_metadata is not True:
    ...

instead of checking the storage type.

So, now I'm confused :-) If we want to allow settings.enabled == True + non-S3 backend, we must check the storage type, unless I'm missing something.

Could you please clarify the right approach, the way you see it? I'd love to solve this this way or the other, without increasing the scope of this bugfix too much.

Please let me know your thoughts.

Thanks!

Copy link

Choose a reason for hiding this comment

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

@antonagestam - are you suggesting something like:

self.collectfast_enabled = settings.enabled
if not hasattr(self.storage, 'preload_metadata'):  # or S3 checks
    self.collectfast_enabled = False
    warnings.warn('Your current storage does not have preload_metadata!'
                  'Disabling collectfast and continuing...'
    )
if self.collectfast_enabled and self.storage.preload_metadata is not True:
    self.storage.preload_metadata = True
    warnings.warn(
        # ...
    )

if self.storage.preload_metadata is not True:
self.storage.preload_metadata = True
warnings.warn(
"Collectfast does not work properly without "
"`preload_metadata` set to `True` on the storage "
"class. Try setting `AWS_PRELOAD_METADATA` to `True`. "
"Overriding `storage.preload_metadata` and continuing."
)

def set_options(self, **options):
"""
Expand Down
29 changes: 29 additions & 0 deletions collectfast/tests/test_command.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

from django.core.management import call_command
from django.utils.six import StringIO
from django.test import override_settings as override_django_setting
from mock import patch

from .utils import test, clean_static_dir, create_static_file, override_setting
Expand Down Expand Up @@ -62,6 +63,34 @@ def test_collectfast_disabled(case):
call_collectstatic()


@test
@override_django_setting(
STATICFILES_STORAGE='django.contrib.staticfiles.storage.StaticFilesStorage'
)
@override_setting("enabled", False)
@with_bucket
def test_non_s3_storage(case):
clean_static_dir()
create_static_file()
call_collectstatic()


@test
@override_django_setting(
STATICFILES_STORAGE='django.contrib.staticfiles.storage.StaticFilesStorage'
)
@with_bucket
def test_enabled_with_non_s3_storage(case):
"""
Running collectfast in enabled mode with a storage type other than S3 should
exit with an error.
"""
clean_static_dir()
create_static_file()
with case.assertRaises(RuntimeError):
call_collectstatic()


@test
@with_bucket
def test_disable_collectfast(case):
Expand Down