JSONField with string default raises fields.E010 warning.

[Tobias Krönke]

Hi! I have a model with this field:

message = models.JSONField(editable=False, default='')

When running

manage.py check

I receive a warning, that IMO should not be triggered:

message: (fields.E010) JSONField default should be a callable instead of an instance so that it's not shared between all field instances.
	HINT: Use a callable instead, e.g., use `dict` instead of `{}`.

django should check, if the default is mutable and only if so issue that warning.

[Mariusz Felisiak]

Thanks for the report. Agreed, we should fix this, especially since it is clearly ​documented as correct:

"If you give the field a default, ensure it’s an immutable object, such as a str, or a callable object that returns ..."

[stimver]

what changes should be done , if someone could suggest.

As the warning suggests- that the default value for the JSONField should be a callable function, rather than an instance, to avoid sharing the default value between all instances of the field. In the provided code, the default value is set to an empty string , which is an instance of a string and therefore mutable.

To fix the warning, you can change the default value to a callable function that returns an empty dictionary, like this:

message = models.JSONField(editable=False, default=dict) .This ensures that a new empty dictionary is created each time a new instance of the JSONField is created.

[Mariusz Felisiak]

Replying to stimver:

As the warning suggests- that the default value for the JSONField should be a callable function, rather than an instance, to avoid sharing the default value between all instances of the field. In the provided code, the default value is set to an empty string , which is an instance of a string and therefore mutable.

That's not true, strings are ​immutable and should be allowed as a default value.

[stimver]

Replying to Mariusz Felisiak:

Replying to stimver:

As the warning suggests- that the default value for the JSONField should be a callable function, rather than an instance, to avoid sharing the default value between all instances of the field. In the provided code, the default value is set to an empty string , which is an instance of a string and therefore mutable.

That's not true, strings are ​immutable and should be allowed as a default value.

Can you suggest what changes should be done?

[Tobias Krönke]

Replying to stimver:

Can you suggest what changes should be done?

That's a good question, because python offers no simple check a la ismutable(x). It would be easy to just do an additional check like isinstance(x, (bool, int, float, tuple, str, frozenset, decimal, complex, bytes)), if you can live with the "danger" of users sub-classing those types and making them mutable. But I guess those can't be helped with this check.

[Sage Abdullah]

Replying to Tobias Krönke:

That's a good question, because python offers no simple check a la ismutable(x). It would be easy to just do an additional check like isinstance(x, (bool, int, float, tuple, str, frozenset, decimal, complex, bytes)), if you can live with the "danger" of users sub-classing those types and making them mutable. But I guess those can't be helped with this check.

Strict (without subclasses) isinstance checking can be done using type(x) in (bool, int, float, tuple, str, ...).

Anyway, I'm convinced this is more of a documentation than an implementation issue. I'll explain below. Buckle up, it's story time!

JSONField, HStoreField, and ArrayField never supported non-callable defaults as of ​https://github.com/django/django/pull/8930, when JSONField was still django.contrib.postgres.fields.JSONField.

When I started working on the new django.db.models.JSONField, I tried changing this behaviour to support using non-callable defaults because the base ​Field.default documentation (which in theory is applicable to all fields in django.db.models), says "The default value for the field. This can be a value or a callable object.".

Thus, I initially implemented an isinstance() check for this (lost in time, but remnants can be seen at: ​https://github.com/django/django/pull/11452#discussion_r295674504).

During the time where this behaviour change was still in the PR, we added ​the paragraph (quoted in the first comment) to the documentation for JSONField's default, based on a suggestion at: ​https://github.com/django/django/pull/11452#discussion_r306186695

However, based on the discussion at ​https://github.com/django/django/pull/11452#discussion_r335854274, we explicitly decided to keep not supporting non-callable defaults for JSONField. This leads to ​https://github.com/django/django/pull/11932, where CheckFieldDefaultMixin is moved to django.db.models.fields.mixins without any behaviour change (i.e. the default still has to be a callable).

The problem is, after that decision was made, we forgot to remove the paragraph in the documentation, which is no longer correct. I believe the fields.E010 check message is the correct behaviour here.

Thus, I propose that we change the documentation instead, from

If you give the field a default, ensure it's an immutable object, such as a str, or a callable object that returns a fresh mutable object each time, such as dict or a function. Providing a mutable default object like default={} or default=[] shares the one object between all model instances.

to something like

If you give the field a default, ensure it is a callable object that returns a fresh object each time, such as dict or a function. Providing a non-callable default object is not supported because a mutable object would be shared between all model instances. There is a system check to ensure that the default is a callable.

Maybe wrap it in a note or something so that it's more prominent, because this deviates from the base Field.default behaviour.

Hope this helps!

[Mariusz Felisiak]

Thanks for the great summary! and for reminding my own words :) Agreed, we should just remove "it's an immutable object, such as a str". Sage, would you like to prepare a patch?

[Sage Abdullah]

​PR.

Sorry, @stimver. Hope you could find other tickets to work on!

[Mariusz Felisiak <felisiak.mariusz@…>]

In 01ae9d4c:

Fixed #34435 -- Doc'd that JSONField.default must be a callable.

[Mariusz Felisiak <felisiak.mariusz@…>]

In f80dbcf:

[4.2.x] Fixed #34435 -- Doc'd that JSONField.default must be a callable.

Backport of 01ae9d4ca9afdaf30a247e10e8333261a7d8224c from main