Opened 10 months ago

Closed 10 months ago

Last modified 10 months ago

#34773 closed Bug (fixed)

settings.STATICFILES_STORAGE does not return correct value when STORAGES are defined

Reported by: Petr Dlouhý Owned by: Mariusz Felisiak
Component: contrib.staticfiles Version: 4.2
Severity: Release blocker Keywords: storages staticfiles
Cc: Jarosław Wygoda, Carlton Gibson Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

I transitioned to the STORAGES settings introduced in Django 4.2 and I got OfflineGenerationError with django-compressor.
I tracked down the issue to be actually bug in django-debug-toolbar's StaticFilesPanel which patches the static files storage and doesn't count with the new STORAGES. The whole issue is described here: https://github.com/jazzband/django-debug-toolbar/issues/1823

The problem itself is, that django-debug-toolbar is using settings.STATICFILES_STORAGE to get the storage class which returns 'django.contrib.staticfiles.storage.StaticFilesStorage' even if I have settings.STORAGES["staticfiles"]["BACKEND"]='whitenoise.storage.CompressedManifestStaticFilesStorage'.
Since Django 4.2 requires STATICFILES_STORAGE to be deleted from settings when STORAGES are present I don't have STATICFILES_STORAGE set to any particular value.

Tracking down this whole issue took me whole week and was very difficult to tackle, because it did appear in completely different place in the system (missing records in the manifest files of django-compressor) than the issue actually was (StaticFilesPanel in django-debug-toolbar).

I think this issue partially goes on behalf of Django itself. If Django would return None value or throw an exception when accessing settings.STATICFILES_STORAGE with STORAGES defined I would see the issue immediately.
The settings.STATICFILES_STORAGE could also return the same value as settings.STORAGES["staticfiles"]["BACKEND"], but I am bigger fun of throwing an exception, because this could lead to other kind of unexpected behavior.

Change History (10)

comment:1 by Mariusz Felisiak, 10 months ago

Cc: Jarosław Wygoda added
Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted

Thanks for the report. IMO, we should make both settings exchangeable during the deprecation period, so settings.STATICFILES_STORAGE/DEFAULT_FILE_STORAGE should be updated when STORAGES is defined.

Bug in 32940d390a00a30a6409282d314d617667892841.

comment:2 by Mariusz Felisiak, 10 months ago

Petr, Does the following patch work for you?

  • django/conf/__init__.py

    diff --git a/django/conf/__init__.py b/django/conf/__init__.py
    index da461ecc02..09ba791f6b 100644
    a b class Settings:  
    217217            os.environ["TZ"] = self.TIME_ZONE
    218218            time.tzset()
    219219
    220         if self.is_overridden("DEFAULT_FILE_STORAGE"):
    221             if self.is_overridden("STORAGES"):
     220        if self.is_overridden("STORAGES"):
     221            if self.is_overridden("DEFAULT_FILE_STORAGE"):
    222222                raise ImproperlyConfigured(
    223223                    "DEFAULT_FILE_STORAGE/STORAGES are mutually exclusive."
    224224                )
    225             warnings.warn(DEFAULT_FILE_STORAGE_DEPRECATED_MSG, RemovedInDjango51Warning)
    226 
    227         if self.is_overridden("STATICFILES_STORAGE"):
    228             if self.is_overridden("STORAGES"):
     225            if self.is_overridden("STATICFILES_STORAGE"):
    229226                raise ImproperlyConfigured(
    230227                    "STATICFILES_STORAGE/STORAGES are mutually exclusive."
    231228                )
     229            setattr(self, 'DEFAULT_FILE_STORAGE', self.STORAGES.get(DEFAULT_STORAGE_ALIAS, {}).get("BACKEND"))
     230            setattr(self, 'STATICFILES_STORAGE', self.STORAGES.get(STATICFILES_STORAGE_ALIAS, {}).get("BACKEND"))
     231        if self.is_overridden("DEFAULT_FILE_STORAGE"):
     232            warnings.warn(DEFAULT_FILE_STORAGE_DEPRECATED_MSG, RemovedInDjango51Warning)
     233        if self.is_overridden("STATICFILES_STORAGE"):
    232234            warnings.warn(STATICFILES_STORAGE_DEPRECATED_MSG, RemovedInDjango51Warning)
    233235
    234236    def is_overridden(self, setting):

comment:3 by Petr Dlouhý, 10 months ago

@Mariusz Felisiak
This code started working only when I re-introduced additional settings for the DEFAULT_STORAGE (which is "storages.backends.s3boto3.S3Boto3Storage" in my case):

AWS_ACCESS_KEY_ID = AWS_DEFAULT.get("AWS_ACCESS_KEY_ID")
AWS_SECRET_ACCESS_KEY = AWS_DEFAULT.get("AWS_SECRET_ACCESS_KEY")
AWS_S3_ENDPOINT_URL = AWS_DEFAULT.get("AWS_S3_ENDPOINT_URL")
AWS_S3_HOST = AWS_DEFAULT.get("AWS_S3_HOST", "s3.amazonaws.com")
AWS_S3_CUSTOM_DOMAIN = AWS_DEFAULT.get("AWS_S3_CUSTOM_DOMAIN", None)
AWS_STORAGE_BUCKET_NAME = AWS_DEFAULT.get("AWS_STORAGE_BUCKET_NAME", "bucket")

I already deleted those values from my settings (used them only for STORAGES), but only now I realized, that I need them for applications like django-avatar that didn't transition to the new STORAGES yet and were appearing to work but in fact were working with different storage than I intended.

I am not sure, if there are not any other consequences of this code, but this seems to enable much smoother transition.

comment:4 by Petr Dlouhý, 10 months ago

@Mariusz Felisiak
Now I realized, that there is another problem with this approach.
If I set STORAGES["staticfiles"]["BACKEND"]="storages.backends.s3boto3.S3Boto3Storage" with some OPTIONS and I set the AWS_* variables at the same time those variables will be taken preferentially.
That means, that I can't set AWS_* variables if I use the new STORAGES, so this approach will not help at all in my case.

I think mixing the old and new settings will lead to many unexpected problems.
I would be OK if I would have to wait until the get_storage_class usage is removed from all applications that I use.
So overall I still think the best approach to this issue would be to solve this issue with:

setattr(self, 'DEFAULT_FILE_STORAGE', None)
setattr(self, 'STATICFILES_STORAGE', None)

in reply to:  4 comment:5 by Mariusz Felisiak, 10 months ago

Cc: Carlton Gibson added

Replying to Petr Dlouhý:

@Mariusz Felisiak
Now I realized, that there is another problem with this approach.
If I set STORAGES["staticfiles"]["BACKEND"]="storages.backends.s3boto3.S3Boto3Storage" with some OPTIONS and I set the AWS_* variables at the same time those variables will be taken preferentially.
That means, that I can't set AWS_* variables if I use the new STORAGES, so this approach will not help at all in my case.

Yes, but this is something that you should report to the 3rd-party storage, it's not an issue in Django itself. I'd not expect that all 3rd-party storage backends will support the new setting immediately. Therefore, it's a pending deprecation, so there is plenty of time to switch to the new STORAGES. I don't see anything unusual in using STORAGES for defining the engine and project-settings for defining options.

I think mixing the old and new settings will lead to many unexpected problems.
I would be OK if I would have to wait until the get_storage_class usage is removed from all applications that I use.
So overall I still think the best approach to this issue would be to solve this issue with:

setattr(self, 'DEFAULT_FILE_STORAGE', None)
setattr(self, 'STATICFILES_STORAGE', None)

I don't agree, this will cause extra work for all 3rd-party packages that use the old settings, and may effectively force them to use STORAGES immediately which is against our deprecation policy.

comment:6 by Mariusz Felisiak, 10 months ago

Owner: changed from nobody to Mariusz Felisiak
Status: newassigned

comment:7 by Mariusz Felisiak, 10 months ago

Has patch: set

comment:8 by Natalia Bidart, 10 months ago

Triage Stage: AcceptedReady for checkin

comment:9 by GitHub <noreply@…>, 10 months ago

Resolution: fixed
Status: assignedclosed

In 6b965c6:

Fixed #34773 -- Fixed syncing DEFAULT_FILE_STORAGE/STATICFILES_STORAGE settings with STORAGES.

Thanks Petr Dlouhý for the report.

Bug in 32940d390a00a30a6409282d314d617667892841.

comment:10 by Mariusz Felisiak <felisiak.mariusz@…>, 10 months ago

In d34db660:

[4.2.x] Fixed #34773 -- Fixed syncing DEFAULT_FILE_STORAGE/STATICFILES_STORAGE settings with STORAGES.

Thanks Petr Dlouhý for the report.

Bug in 32940d390a00a30a6409282d314d617667892841.
Backport of 6b965c600054f970bdf94017ecf2e0e6e0a4326b from main

Note: See TracTickets for help on using tickets.
Back to Top