#34435 closed Bug (fixed)
JSONField with string default raises fields.E010 warning.
Reported by: | Tobias Krönke | Owned by: | Sage Abdullah |
---|---|---|---|
Component: | Documentation | Version: | 4.1 |
Severity: | Normal | Keywords: | |
Cc: | Sage Abdullah | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
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.
Change History (12)
comment:1 by , 20 months ago
Cc: | added |
---|---|
Component: | Uncategorized → Core (System checks) |
Summary: | JSONField with default='' raises warning "JSONField default should be a callable" → JSONField with string default raises fields.E010 warning. |
Triage Stage: | Unreviewed → Accepted |
Type: | Uncategorized → Bug |
comment:2 by , 20 months ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
follow-up: 4 comment:3 by , 19 months ago
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.
follow-up: 5 comment:4 by , 19 months ago
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.
follow-up: 6 comment:5 by , 19 months ago
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?
follow-up: 7 comment:6 by , 19 months ago
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 a check like isinstance(x, (bool, int, float, tuple, str, frozenset))
, 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.
comment:7 by , 19 months ago
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 likeisinstance(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 astr
, or a callable object that returns a fresh mutable object each time, such asdict
or a function. Providing a mutable default object likedefault={}
ordefault=[]
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 asdict
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 thedefault
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!
comment:8 by , 19 months ago
Component: | Core (System checks) → Documentation |
---|
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?
comment:9 by , 19 months ago
Has patch: | set |
---|---|
Owner: | changed from | to
PR.
Sorry, @stimver. Hope you could find other tickets to work on!
comment:10 by , 19 months ago
Triage Stage: | Accepted → Ready for checkin |
---|
Thanks for the report. Agreed, we should fix this, especially since it is clearly documented as correct: