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

PRE / POST hooks #627

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
9 changes: 9 additions & 0 deletions storages/backends/base.py
@@ -0,0 +1,9 @@
from django.core.files.base import File
from django.core.files.storage import Storage

class BaseFile(File):
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the purpose of BaseFile if it provides no hooks?

Copy link
Author

Choose a reason for hiding this comment

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

@jdufresne It doesn't provide hooks at this moment as this is just a skeleton proposal, but it will certainly have hooks if we agree that this is a good approach.

pass

class BaseStorage(Storage):
def pre_save(self, name, content):
return name, content
7 changes: 4 additions & 3 deletions storages/backends/s3boto3.py
Expand Up @@ -10,14 +10,14 @@
from django.conf import settings as django_settings
from django.core.exceptions import ImproperlyConfigured, SuspiciousOperation
from django.core.files.base import File
from django.core.files.storage import Storage
from django.utils.deconstruct import deconstructible
from django.utils.encoding import (
filepath_to_uri, force_bytes, force_text, smart_text,
)
from django.utils.six.moves.urllib import parse as urlparse
from django.utils.timezone import is_naive, localtime

from storages.backends.base import BaseStorage, BaseFile
from storages.utils import (
check_location, get_available_overwrite_name, lookup_env, safe_join,
setting,
Expand All @@ -37,7 +37,7 @@


@deconstructible
class S3Boto3StorageFile(File):
class S3Boto3StorageFile(BaseFile):
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the plan to do this for all backends?

Copy link
Author

Choose a reason for hiding this comment

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

Yes. This should be transparent and shouldn't create any bugs, as the Base* classes are just proxies.


"""
The default file object used by the S3Boto3Storage backend.
Expand Down Expand Up @@ -168,7 +168,7 @@ def close(self):


@deconstructible
class S3Boto3Storage(Storage):
class S3Boto3Storage(BaseStorage):
"""
Amazon Simple Storage Service using Boto3

Expand Down Expand Up @@ -469,6 +469,7 @@ def _open(self, name, mode='rb'):
return f

def _save(self, name, content):
name, content = self.pre_save(name, content)
Copy link
Contributor

Choose a reason for hiding this comment

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

As pre_save is the very first line of the method, I don't see the need for this at all. Couldn't a backend do the following?

class MyS3Boto3Storage(S3Boto3Storage):
    def _save(self, name, content):
        name, content = self.do_my_special_thing()
        return super()._save(name, conent)

This is just as flexible, requires no special hooks and is compatible with recent releases.

Copy link
Author

Choose a reason for hiding this comment

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

The entire idea of a hook in a library is to provide a way in which developers can run their own code in a guaranteed-to-be-stable way. This means that your proposal of directly subclassing the backends is not feasible, as there is no guarantee that the code in that method won't change (and break the subclass-patch).

cleaned_name = self._clean_name(name)
name = self._normalize_name(cleaned_name)
parameters = self.object_parameters.copy()
Expand Down