PRE / POST hooks

[alexandernst]

The idea of this proposal is to allow developers to hook their own actions into some of the methods (read and write for example).

The way I think this could be achieved without making huge changes to the code is by using django-storages s current OOP design. Currently all backend storages implement Django's Storage` class.

django.core.files.storage.Storage
    |
    |-- storages.backends.s3boto3.S3Bot3Storage

This proposal creates a dummy proxy class that implements the hooks that could be used by developers.

django.core.files.storage.Storage
    |
    |-- storages.backends.base.BaseStorage
        |
        |-- storages.backends.s3boto3.S3Bot3Storage

The code as-it-is now implements only pre_save, which receives name, content and returns name, content without modifying them in any way. This allows the code to work as expected by default if pre_save isn't overwritten.

If a developer chooses to override the behaviour of a particular hook, it would be as simple as overriding only that particular method. As an example, I'll show how to override the pre_save method and convert all uploaded files to base64:

First, create a new class, anywhere in your app/project:

import base64
from storages.backends.s3boto3 import S3Boto3Storage

class S3Mod(S3Boto3Storage):
    def pre_save(self, name, content):
        content.seek(0)
        _content = base64.b64encode(content.read())
        content.seek(0)
        content.truncate(0)
        content.write(_content)
        content.seek(0)
        return name, content

Then set the default storage to that class:

DEFAULT_FILE_STORAGE = 'playground.s3mod.S3Mod'

[jdufresne]

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

[alexandernst]

@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.

[jdufresne]

Is the plan to do this for all backends?

[alexandernst]

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

[jdufresne]

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.

[alexandernst]

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).

[alexandernst]

@jdufresne Are we good on this proposal? Do you have any objections? Should I keep working on this approach?

[jdufresne]

I'm fine with the proposal. I think one could add functionality in this way to a subclass today, but if the stable API helps with documentation and ensures long term compatibility, that makes sense.

[alexandernst]

@jdufresne Great news! So let's move on with this :)

I have a few questions:

1. When are the read, write and close methods of the class *Backend*StorageFile called? I'm asking this because the write method of the class *Backend*Storage doesn't call it, which seems weird. And I can't see any other references from the *Backend*Storage class.

2. What are the methods (of *Backend*Storage) that one would need to implement in order to create a new storage backend? _save, delete, exists, size, url, list* and *time, right? Or am I missing something?

3. Do you want me to patch all backends, or should I submit one MR per backend? I personally feel that the second option would be better.

[alexandernst]

ping @jdufresne

[alexandernst]

ping @jdufresne

[jdufresne]

Do you want me to patch all backends, or should I submit one MR per backend? I personally feel that the second option would be better.

If we're going to add the feature to one backend, might as well make it universal and document it as such. IMO, the backends should behavior similarly to one another when possible.

[alexandernst]

@jdufresne You misunderstood my third question (and didn't reply to the first two!). Can you re-read, please?

[sww314]

@alexandernst

1. Those are public API, they are called by Django. https://docs.djangoproject.com/en/2.1/_modules/django/core/files/storage/#DefaultStorage
2. See the Django Storage base class.
3. I would do it with on 1 storage class, then do the others. Although, I would not argue with other approach either.

[withthelemons]

What's the current status?

[alexandernst]

@withthelemons I have been extremely busy the last couple of months, but I do plan on continuing the work on this shortly.