jazzband / django-constance

Dynamic Django settings.
https://django-constance.readthedocs.io
BSD 3-Clause "New" or "Revised" License
1.71k stars 310 forks source link

Add support for collection types (list/tuple/dict/set/etc.) #574

Closed sebastianmanger closed 1 month ago

sebastianmanger commented 2 months ago

Describe the problem

Using the DatabaseBackend, I store lists of recipients for emails and dicts for small configy-things. The (awesome) changes in https://github.com/jazzband/django-constance/pull/564 do not cover these types. The stored value in the database is not in the expected format

'{"__type__": "default", "__value__": "{"test": "test"}"}'

but

'{"test": "test"}'

This then leads to an exception when trying to access the value, as the keys do not match the expected structure (https://github.com/jazzband/django-constance/blob/master/constance/codecs.py#L58).

Proposed discussion

I could help out with any of the following tasks:

Steps to reproduce

This simply uses the default encoding defined in the JSONEncoder, but should display the issue clearly 😄 See https://github.com/jazzband/django-constance/compare/master...sebastianmanger:django-constance:json-missing-types

System configuration

ivan-klass commented 2 months ago

According to natively supported field types, lists and dicts are not there.

Mogost commented 2 months ago

This is a known limitation. Also, it already declared into https://github.com/jazzband/django-constance/blob/c690d5ef0ffa74e5b09064a3bc5b0e5b81f2dfce/docs/index.rst#custom-fields

As a temporary solution, I would suggest you make your own custom type for the task. Like in this test: https://github.com/jazzband/django-constance/blob/c690d5ef0ffa74e5b09064a3bc5b0e5b81f2dfce/tests/test_codecs.py#L70-L81

As for a long term solution, that needs to be thought about more strongly.

sebastianmanger commented 2 months ago

Thanks to both of you - feel free to close!

Mogost commented 1 month ago

Thanks to both of you - feel free to close!

Your ticket makes sense. It's feature requests. I do not want to close this issue because it's reasonable.

lpomfrey commented 1 month ago

This seems actually more a regression than a new feature. We quite heavy add django.forms.fields.JSONField to the CONSTANCE_ADDITIONAL_FIELDS to be able to have JSON blobs as values. This use case used to work fine, but is completely broken in 4.x now as there's no obvious way to serialise dicts (register_type doesn't work for a dict because of the object_hook implementation in constance.codecs).

Arguably the standard json.loads/json.dumps would be better than the implementation in constance.codecs because they would handle this use case perfectly fine. I'm not entirely sure of the reasoning behind the implementation there but it seems slightly convoluted when perhaps just being able to specify a custom JSONEncoder class in place of register_type etc would work better (and also not break the standard JSON serialisation)?

Also, the whole codecs and register_type API is not documented, only CONSTANCE_ADDITIONAL_FIELDS which seems to suggest that the django.forms.fields.JSONField use case would work perfectly fine still, and the changelog for 4.0.0 only mentions changing the database backend to use json with no mention of the redis backend (or that the JSON implementation isn't the standard one).

As a resolution we've been playing with monkey patching constance.codecs.dumps and constance.codecs.loads, but a better solution may be one of (in a vaguely most-preferable-first ordering):

I'd be happy to implement any of those in a PR if one is agreed on.

Mogost commented 1 month ago

JFI I am working on this problem but cannot provide ETA. If collection is your case please wait until implementation.