Provide a way to pass kwargs when initializing the storage class

[Aymeric Augustin]

Hello,

Currently the DEFAULT_FILE_STORAGE and STATICFILES_STORAGE settings contain a dotted Python path to the storage class. The class is instantiated without any arguments.

** Problem **

This leads to three annoyances.

1) Third-party libraries like django-storages(-redux) need to provide a setting for every value that could be required to configure the storage. This additional level of indirection is annoying.

If you’re skeptical, look at https://github.com/jschneier/django-storages/blob/f2880b16b57a36b241a54932255e1a852c0e0ac7/storages/backends/s3boto.py#L204.

2) Compounding this problem, third-party libraries that want to use a Django storage backend, but likely not the default one, have no convenient way for the user to configure the storage.

Having settings such as `MY_THIRD_PARTY_APP__AWS_ACCESS_KEY_ID` etc. for every possible storage-related setting sounds unreasonable.

(This is the problem I’m facing right now with one of my libraries.)

3) The standard pattern for working around these problems is boilerplate-ish:

my_config = {foo: bar}

class ConfiguredStorageClass(GenericStorageClass):

    def __init__(self, *args, **kwargs):

        kwargs.update(my_config)

        super().__init__(*args, **kwargs)

DEFAULT_FILE_STORAGE = 'path.to.ConfiguredStorageClass'

** Proposed solution **

To solve this problem, I would like the DEFAULT_FILE_STORAGE and STATICFILES_STORAGE settings to accept an additional format: a 2-uple of (dotted Python path to class, init_kwargs).

This would allow simplifying the example above to:

DEFAULT_FILE_STORAGE = ‘path.to.GenericStorageClass’, {foo: bar}

** Variants **

We could go a bit further support 2-uples with args, 2-uples with kwargs, and 3-uples with args and kwargs. I didn’t propose it because I’m not sure this possibility adds much value. Arguments can be passed as kwargs even if the init method expects them positionally.

We could support setting DEFAULT_FILE_STORAGE and STATICFILES_STORAGE to an already initialized instance of a storage class. I’m against this idea because settings should remain primitive Python values whenever possible.

We could introduce DEFAULT_FILE_STORAGE_INIT_KWARGS and STATICFILES_STORAGE_INIT_KWARGS. Well. Perhaps not :-)

What do you think?

-- 

Aymeric.

[Collin Anderson]

Ooooh. I think I like it. The syntax could be a bit confusing, but I think I like it. :)

[charettes]

I think the proposed approach makes sense.

I don't think we need to support any variant to pass the configuration values as args. All the existing storage backends have to make their argument optionals anyway now to support how they are currently initialized.

Requiring a dict makes it also more explicit about what values actually mean and support both cases.

Simon

[charettes]

I think the proposed approach makes sense.

I don't think we need to support any variant to pass the configuration values as args. All the existing storage backends have to make their argument optionals anyway now to support how they are currently initialized.

Requiring a dict makes it also more explicit about what values actually mean and support both cases.

Simon

Le samedi 7 novembre 2015 12:16:05 UTC+1, Aymeric Augustin a écrit :

[James Aylett]

I'm +1 on this solution; it solves the problem in an efficient way without becoming unreadable. I'd shy away from the variants unless/until there's a definite need. kwargs are more explicit, which I think is helpful in readability :)

J

On Saturday, November 7, 2015 at 12:16:05 PM UTC+1, Aymeric Augustin wrote:

[Raphaël Barrois]

Hello,


The core of the proposed solution seems quite interesting; however, it also introduces a new configuration format for backends.

Caches and databases use a dict with a "BACKEND" key and an "OPTIONS" dict for kwargs to pass to the backend.
Likewise, entries in the TEMPLATES list are dicts with a "BACKEND" key and various options.


Do you think that the new setting should match these options instead of the proposed two-tuples?


--
Raphaël

On Sat, 7 Nov 2015 12:15:49 +0100
Aymeric Augustin <aymeric....@polytechnique.org> wrote:

> Hello,
>
> Currently the DEFAULT_FILE_STORAGE and STATICFILES_STORAGE settings
> contain a dotted Python path to the storage class. The class is
> instantiated without any arguments.
>
>
> ** Problem **
>
> This leads to three annoyances.
>
> 1) Third-party libraries like django-storages(-redux) need to provide
> a setting for every value that could be required to configure the
> storage. This additional level of indirection is annoying.
>
> If you’re skeptical, look at
> https://github.com/jschneier/django-storages/blob/f2880b16b57a36b241a54932255e1a852c0e0ac7/storages/backends/s3boto.py#L204

> <https://github.com/jschneier/django-storages/blob/f2880b16b57a36b241a54932255e1a852c0e0ac7/storages/backends/s3boto.py#L204>.

[Tom Evans]

On Sat, Nov 7, 2015 at 11:58 AM, Raphaël Barrois
<raphael...@gmail.com> wrote:
> Hello,
>
>
> The core of the proposed solution seems quite interesting; however, it also introduces a new configuration format for backends.
>
> Caches and databases use a dict with a "BACKEND" key and an "OPTIONS" dict for kwargs to pass to the backend.
> Likewise, entries in the TEMPLATES list are dicts with a "BACKEND" key and various options.
>
>
> Do you think that the new setting should match these options instead of the proposed two-tuples?

I like explicit:

STORAGE = {
'default': {
'ENGINE': 'path.to.foo.Storage',
'OPTIONS': { ..}
},
}

Cheers

Tom

[Aymeric Augustin]

Hello,

Indeed, if we took a big step further and provided an API to configure multiple file storage backends, that would make sense.

Currently we have two hardcoded ones: the default, which is used for media files, and the static, which is used for static files.

Essentially your proposal means reformatting the current file-related settings to this structure:

FILE_STORAGES = {
‘media’: {
‘BACKEND’: settings.DEFAULT_FILE_STORAGE,
‘OPTIONS’: {
‘location’: settings.MEDIA_ROOT,
‘base_url’: settings.MEDIA_URL,
# possible override of settings.FILE_CHARSET
},
},
‘static’: {
‘BACKEND’: settings.STATICFILES_STORAGE,
‘OPTIONS’: {
‘location’: settings.STATIC_ROOT,
‘base_url’: settings.STATIC_URL,
# replacement for STATICFILES_FINDERS and STATICFILES_DIRS that would look a lot like template loaders
# possible override of settings.FILE_CHARSET
},

}
}

The FILE_UPLOAD_* settings wouldn’t change because they don’t affect file storage.

It isn’t entirely clear to me what the best way to reconcile “default” and “media” would be. Currently they’re effectively the same but settings are named inconsistently for historical reasons. In addition this would require an API for getting a file storage backend.

This is a much more invasive change as it would require everyone to update their settings. However it would meet some use cases much better. Pluggable apps that need to store files could document that they’ll use the backend with a given name it if exists and fall back to the default backend otherwise. That’s exactly what django.contrib.sessions currently does with cache.

How do people feel about this alternative proposal?

--
Aymeric.

> --
> You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
> To post to this group, send email to django-d...@googlegroups.com.
> Visit this group at http://groups.google.com/group/django-developers.
> To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/20151107125832.440abc17%40corvis.
> For more options, visit https://groups.google.com/d/optout.

[Shai Berger]

On Saturday 07 November 2015 14:55:20 Aymeric Augustin wrote:
>
> Essentially your proposal means reformatting the current file-related
> settings to this structure:
>
> FILE_STORAGES = {
> ‘media’: {
> ‘BACKEND’: settings.DEFAULT_FILE_STORAGE,
> ‘OPTIONS’: {
> ‘location’: settings.MEDIA_ROOT,
> ‘base_url’: settings.MEDIA_URL,
> # possible override of settings.FILE_CHARSET
> },
> },
> ‘static’: {
> ‘BACKEND’: settings.STATICFILES_STORAGE,
> ‘OPTIONS’: {
> ‘location’: settings.STATIC_ROOT,
> ‘base_url’: settings.STATIC_URL,
> # replacement for STATICFILES_FINDERS and STATICFILES_DIRS
> that would look a lot like template loaders # possible override of
> settings.FILE_CHARSET
> },
>
> }
> }
>
>

> How do people feel about this alternative proposal?
>

This, in general, seems like the right thing to do. The only reservation I
have is that the 'OPTIONS' key seems superfluous -- why not put the options in
the same dictionary as the backend?

On a related point -- I wouldn't put base_url in there. It is related to files,
but not to their storage.

My 2KB,
Shai.

[Sean Brant]

+1 to these ideas. This will make injecting dependancies much cleaner. I’ve pointed my BACKEND settings to factory functions in the past.


def storage_factory():
return SomeStorage(some_de)


DEFAULT_STORAGE_BACKEND = ‘path.to.storage_factory'

[Claude Paroz]

The drawback of complex dictionary settings is that to overwrite only one key in a settings file, you have to copy the entire dictionary, also possibly defeating global settings defaults when they change.

So let's try to have many smaller dictionaries instead of few big ones. The initial proposal was a bit better at this regard.

Claude

[Marc Tamlyn]

I'm definitely in favour of a format allowing multiple storage back ends referred to by name. For larger sites it is not unusual to manage files across multiple locations (eg several S3 buckets). The storage param to FileField would be allowed to be the key, and there would be a get_storage function like get_cache.

This is especially useful when testing locally as you can swap out dependencies on external server storages for local temp storage.

(Side point: a temp dir based storage which could clean itself up between rest runs would be amazing)

--

You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
To post to this group, send email to django-d...@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.

To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/e180cd2b-9601-44cd-9ba4-21e3a7d10157%40googlegroups.com.

[Shai Berger]

On Sunday 08 November 2015 10:31:06 Marc Tamlyn wrote:
>
> (Side point: a temp dir based storage which could clean itself up between
> rest runs would be amazing)
>

Yes, it would. https://code.djangoproject.com/ticket/23251

[James Aylett]

On 8 Nov 2015, at 08:31, Marc Tamlyn <marc....@gmail.com> wrote:

> I'm definitely in favour of a format allowing multiple storage back ends referred to by name. For larger sites it is not unusual to manage files across multiple locations (eg several S3 buckets). The storage param to FileField would be allowed to be the key, and there would be a get_storage function like get_cache.

It would remove the assymetry between the default backends and per-field ones, which does feel a little odd. However I don’t think that’s a strong enough reason to go for more complicated. Ballooning dictionaries can feel overwhelming when looking at modern Django settings (for instance, the new templates configuration is more daunting than it used to be), and as pointed out, overriding is more fiddly.

For testing, you need to be explicit per-field no matter what, so it’s a change from an instance to a symbolic reference. The instance is probably a variable anyway by declaration of the test model, which I suspect is slightly easier to chase.

So I’d be slightly more in favour of the terse, tuple-based syntax.

J

--
James Aylett
I make: devfort.com, spacelog.org
Films: talktorex.co.uk
Everything else: tartarus.org/james/

[Jarosław Wygoda]

 I'd like to introduce a file storage registry similar to BaseConnectionHandler (django/utils/connection.py) and EngineHandler (django/template/utils.py).

Example settings.py snippet:

STORAGES = {  # rename to FILE_STORAGES to make it more explictit?
    'example': {
        'BACKEND': 'django.core.files.storage.FileSystemStorage',
        'OPTIONS': {
            'location': '/example',
            'base_url': '/example/',
        },
    },
}

Changes introduced by this pr are backward compatible. Users can still use existing settings to configure static and media storages.

Currently storages can be retrieved from the following objects:

django/core/files/storage.py:

    get_storage_class
    DefaultStorage
    default_storage

django/contrib/staticfiles/storage.py:

    ConfiguredStorage
    staticfiles_storage

What do you think about deprecating them?

​https://github.com/django/django/pull/15610

FileField can be tackled in a separate pr.

[Carlton Gibson]

Hi Jarosław. Thanks for picking this up. 

There seems to be enough support for the general idea here, so worth pressing on. 

Let's think about any required deprecations on the PR. (It's easier there 😅). 

Kind Regards,

Carlton

[Mariusz Felisiak]

Hi folks,

STORAGES PR is in a really good shape. Can you please test this patch on your projects? Checking pending patches is a great way to contribute 💗

Best,
Mariusz