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.

Sign up for GitHub

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

Jump to bottom

Replace get_storage_class with new storages API for Django >= 4.2 #1216

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

Replace get_storage_class with new storages API for Django >= 4.2 #1216

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ffdc8da
Replace get_storage_class with new storages API for Django >= 4.2
adnathanail Jan 6, 2024
54c60b6
Fix failing tests
adnathanail Jan 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 14 additions & 4 deletions compressor/storage.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,22 @@
import time
from urllib.parse import urljoin

from django.core.files.storage import FileSystemStorage, get_storage_class
import django
from django.core.files.storage import FileSystemStorage
from django.utils.functional import LazyObject, SimpleLazyObject

from compressor.conf import settings


def _get_storage_class(backend):
if django.VERSION[0] > 4 or (django.VERSION[0] == 4 and django.VERSION[1] > 1):
from django.core.files.storage import storages
return storages.create_storage({"BACKEND": backend})
else:
from django.core.files.storage import get_storage_class
return get_storage_class(backend)()
Comment on lines +19 to +20
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should be using storages[backend] here (and likely override_settings) 🤔

(Rather than creating a new instance...)

Copy link
Author

Choose a reason for hiding this comment

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

I think currently storages[backend] wouldn't have anything in it?

Are we saying that users should be declaring a backend with compressor.storage.CompressorFileStorage for us to use, so we don't keep creating a new instance?

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, the whole storages thing is new so... 😜

I think if we were designing from scratch, we'd likely say something like, "If you declare a backend in STORAGES we'll use that, otherwise we'll automatically create one for you".

So we'd need a setting for the backend name, and then we could look for that on first access.

That's just a first thought. Open to suggestions.



class CompressorFileStorage(FileSystemStorage):
"""
Standard file system storage for files handled by django-compressor.
Expand Down Expand Up @@ -48,7 +58,7 @@ def save(self, filename, content):


compressor_file_storage = SimpleLazyObject(
lambda: get_storage_class("compressor.storage.CompressorFileStorage")()
lambda: _get_storage_class("compressor.storage.CompressorFileStorage")
)


Expand Down Expand Up @@ -112,7 +122,7 @@ def save(self, filename, content):

class DefaultStorage(LazyObject):
def _setup(self):
self._wrapped = get_storage_class(settings.COMPRESS_STORAGE)()
self._wrapped = _get_storage_class(settings.COMPRESS_STORAGE)


default_storage = DefaultStorage()
Expand All @@ -131,7 +141,7 @@ def __init__(self, location=None, base_url=None, *args, **kwargs):

class DefaultOfflineManifestStorage(LazyObject):
def _setup(self):
self._wrapped = get_storage_class(settings.COMPRESS_OFFLINE_MANIFEST_STORAGE)()
self._wrapped = _get_storage_class(settings.COMPRESS_OFFLINE_MANIFEST_STORAGE)


default_offline_manifest_storage = DefaultOfflineManifestStorage()
10 changes: 4 additions & 6 deletions compressor/tests/test_storages.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,29 +2,27 @@
import brotli

from django.core.files.base import ContentFile
from django.core.files.storage import get_storage_class
from django.test import TestCase
from django.test.utils import override_settings
from django.utils.functional import LazyObject

from compressor import storage
from compressor.conf import settings
from compressor.storage import _get_storage_class
from compressor.tests.test_base import css_tag
from compressor.tests.test_templatetags import render


class GzipStorage(LazyObject):
def _setup(self):
self._wrapped = get_storage_class(
"compressor.storage.GzipCompressorFileStorage"
)()
self._wrapped = _get_storage_class("compressor.storage.GzipCompressorFileStorage")


class BrotliStorage(LazyObject):
def _setup(self):
self._wrapped = get_storage_class(
self._wrapped = _get_storage_class(
"compressor.storage.BrotliCompressorFileStorage"
)()
)


@override_settings(COMPRESS_ENABLED=True)
Expand Down
7 changes: 6 additions & 1 deletion docs/remote-storages.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,8 @@ apps can be integrated.
to use; below is an example of the boto3 S3 storage backend from
django-storages_::

from django.core.files.storage import get_storage_class
from django.core.files.storage import get_storage_class # Django <= 4.1
from django.core.files.storage import storages # Django >= 4.2
from storages.backends.s3boto3 import S3Boto3Storage

class CachedS3Boto3Storage(S3Boto3Storage):
Expand All @@ -62,8 +63,12 @@ apps can be integrated.
"""
def __init__(self, *args, **kwargs):
super().__init__(*args, **kwargs)
# Django <= 4.1
self.local_storage = get_storage_class(
"compressor.storage.CompressorFileStorage")()
# Django >= 4.2
self.local_storage = storages.create_storage(
{"BACKEND": "compressor.storage.CompressorFileStorage"})

def save(self, name, content):
self.local_storage.save(name, content)
Expand Down