Collectfast breaks with local filesystem storage even when disabled

[johananl]

Hi,

Thanks for publishing this project. It is very helpful :-)

I've encountered the following issue:

When using Collectfast in disabled mode (for local development without S3), running ./manage.py collectstatic breaks even with --disable-collectfast or COLLECTFAST_ENABLED = False. The reason seems to be the following:

collectfast/collectfast/management/commands/collectstatic.py

Line 35 in ced213e

if self.storage.preload_metadata is not True:

When using filesystem storage, collectstatic crashes with the following:

Traceback (most recent call last):
  File "./manage.py", line 10, in <module>
    execute_from_command_line(sys.argv)
  File "/Users/johananl/tmp/collectfast_test/venv/lib/python3.6/site-packages/django/core/management/__init__.py", line 371, in execute_from_command_line
    utility.execute()
  File "/Users/johananl/tmp/collectfast_test/venv/lib/python3.6/site-packages/django/core/management/__init__.py", line 365, in execute
    self.fetch_command(subcommand).run_from_argv(self.argv)
  File "/Users/johananl/tmp/collectfast_test/venv/lib/python3.6/site-packages/django/core/management/__init__.py", line 216, in fetch_command
    klass = load_command_class(app_name, subcommand)
  File "/Users/johananl/tmp/collectfast_test/venv/lib/python3.6/site-packages/django/core/management/__init__.py", line 37, in load_command_class
    return module.Command()
  File "/Users/johananl/tmp/collectfast_test/venv/lib/python3.6/site-packages/collectfast/management/commands/collectstatic.py", line 36, in __init__
    if self.storage.preload_metadata is not True:
  File "/Users/johananl/tmp/collectfast_test/venv/lib/python3.6/site-packages/django/utils/functional.py", line 216, in inner
    return func(self._wrapped, *args)
AttributeError: 'StaticFilesStorage' object has no attribute 'preload_metadata'

This happens because StaticFilesStorage doesn't have an attribute called preload_metadata (I believe it is S3-specific).

To reproduce this issue, simply run ./manage.py collectstatic with collectfast in INSTALLED_APPS and a default storage configuration.

I believe this can be solved by something similar to the following:

from storages.backends.s3boto3 import S3Boto3Storage
...
if isinstance(self.storage, S3Boto3Storage) and self.storage.preload_metadata is not True:
    ...

The following is also possible to avoid the import:

if (type(self.storage).__name__ == 'storages.backends.s3boto3.S3Boto3Storage'
    and self.storage.preload_metadata is not True):

What do you think?

Thanks,
Johanan

[antonagestam]

This definitely looks like a bug, thanks for reporting it.

I will accept a patch for this as long as a test case is added (that fails with the current implementation). I would prefer checking settings.enabled instead of checking the storage type, e.g:

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

[johananl]

I'd be happy to submit a PR @antonagestam .

One question though: I'm trying to figure out how to change the STATICFILES_STORAGE setting in a unit test. I need to set it to the default (django.contrib.staticfiles.storage.StaticFilesStorage).
I saw the @override_setting decorator, however it seems to operate on collectfast.settings and not the global Django settings module.

Could you please point me at the right direction?

Thanks!

[pachewise]

we also ran into this issue after upgrading collectfast at work, so would like to see this get fixed...

@johananl I'm assuming the override_storage_attr decorator will do what you want for the tests (you could also try importing the override_settings decorator from django).

I think we need two tests: (1) the test discussed: collectfast is disabled and default storage is used; (2) a test where collectfast is enabled, but the staticfiles_storage is set to the django default. I'm not sure what the expected behavior is in that case, but we should be deliberate about it.

[antonagestam]

@johananl https://docs.djangoproject.com/en/2.0/topics/testing/tools/#django.test.override_settings

We could change the name of override_setting to override_collectfast_setting to not conflict with the Django decorator. Or if you have a shorter or better name :)

[johananl]

Good to hear we're not the only ones, @pachewise :-)
Thanks for your assistance @antonagestam. I'm on it and will open a PR as soon as I can.
In case I have further questions, I'll just open a work-in-progress PR to help us discuss things in context.

Thanks!

[johananl]

Perfect @antonagestam - the Django override_settings decorator works.

@pachewise - thanks for your advice regarding the tests. Regarding number 1, this test case already exists:

@test
@override_setting("enabled", False)
@with_bucket
def test_collectfast_disabled(case):
    clean_static_dir()
    create_static_file()
    call_collectstatic()

As for number 2, I think the expected behavior should be exit with an error, since collectfast should only be used with S3 (right @antonagestam?).

In addition, I would also add a test case with collectfast disabled and the storage is Django's default (filesystem storage). This is in fact the trigger for this bug.

[pachewise]

thanks for working on this, Johan! by (1), I meant the test you're describing for the bug: by default storage, I meant the django default (StaticFilesStorage)

[johananl]

Thanks for clarifying @pachewise. Then yes, looks like both your suggested test cases will be included in my PR.

[johananl]

Please see #124. @antonagestam @pachewise - please feel free to comment and suggest changes. I hope I've managed to handle this properly and make Collectfast a little more stable.