Re: [Django] #34435: JSONField with string default raises fields.E010 warning.
Django
--------------------------------------+------------------------------------
Reporter: Tobias Krönke | Owner: stimver
Type: Bug | Status: assigned
Component: Core (System checks) | Version: 4.1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Changes (by stimver):
* owner: nobody => stimver
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/34435#comment:2>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
--------------------------------------+------------------------------------
Reporter: Tobias Krönke | Owner: stimver
Type: Bug | Status: assigned
Component: Core (System checks) | Version: 4.1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/34435#comment:3>
Django
--------------------------------------+------------------------------------
Reporter: Tobias Krönke | Owner: stimver
Type: Bug | Status: assigned
Component: Core (System checks) | Version: 4.1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Comment (by Mariusz Felisiak):
Replying to [comment:3 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 [https://docs.python.org/3/glossary.html
#term-immutable immutable] and should be allowed as a default value.
--
Ticket URL: <https://code.djangoproject.com/ticket/34435#comment:4>
Django
--------------------------------------+------------------------------------
Reporter: Tobias Krönke | Owner: stimver
Type: Bug | Status: assigned
Component: Core (System checks) | Version: 4.1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Comment (by stimver):
Replying to [comment:4 Mariusz Felisiak]:
> Replying to [comment:3 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 [https://docs.python.org/3/glossary.html
#term-immutable immutable] and should be allowed as a default value.
Can you suggest what changes should be done?
--
Ticket URL: <https://code.djangoproject.com/ticket/34435#comment:5>
Django
--------------------------------------+------------------------------------
Reporter: Tobias Krönke | Owner: stimver
Type: Bug | Status: assigned
Component: Core (System checks) | Version: 4.1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Comment (by Tobias Krönke):
Replying to [comment:5 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/34435#comment:6>
Django
--------------------------------------+------------------------------------
Reporter: Tobias Krönke | Owner: stimver
Type: Bug | Status: assigned
Component: Core (System checks) | Version: 4.1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Comment (by Sage Abdullah):
Replying to [comment:6 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 [https://docs.djangoproject.com/en/dev/ref/models/fields/#default
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
[https://github.com/django/django/blob/8b1ff0da4b162e87edebd94e61f2cd153e9e159d/docs/ref/models/fields.txt#L1268-L1272
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!
--
Ticket URL: <https://code.djangoproject.com/ticket/34435#comment:7>
Django
-------------------------------+------------------------------------
Reporter: Tobias Krönke | Owner: stimver
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* component: Core (System checks) => Documentation
Comment:
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?
--
Ticket URL: <https://code.djangoproject.com/ticket/34435#comment:8>
Django
Reporter: Tobias Krönke | Owner: Sage Abdullah
Type: Bug | Status: assigned
Component: Documentation | Version: 4.1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Sage Abdullah):
* owner: stimver => Sage Abdullah
* has_patch: 0 => 1
Comment:
[https://github.com/django/django/pull/16783 PR].
Sorry, @stimver. Hope you could find other tickets to work on!
--
Ticket URL: <https://code.djangoproject.com/ticket/34435#comment:9>
Django
Reporter: Tobias Krönke | Owner: Sage
| Abdullah
Type: Bug | Status: assigned
Component: Documentation | Version: 4.1
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/34435#comment:10>
Django
-------------------------------------+-------------------------------------
Reporter: Tobias Krönke | Owner: Sage
| Abdullah
Component: Documentation | Version: 4.1
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"01ae9d4ca9afdaf30a247e10e8333261a7d8224c" 01ae9d4c]:
{{{
#!CommitTicketReference repository=""
revision="01ae9d4ca9afdaf30a247e10e8333261a7d8224c"
Fixed #34435 -- Doc'd that JSONField.default must be a callable.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34435#comment:11>
Django
-------------------------------------+-------------------------------------
Reporter: Tobias Krönke | Owner: Sage
| Abdullah
Type: Bug | Status: closed
Component: Documentation | Version: 4.1
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"f80dbcf7dccc7227eade1a02d413fc65fc6f53ef" f80dbcf]:
{{{
#!CommitTicketReference repository=""
revision="f80dbcf7dccc7227eade1a02d413fc65fc6f53ef"
[4.2.x] Fixed #34435 -- Doc'd that JSONField.default must be a callable.
Backport of 01ae9d4ca9afdaf30a247e10e8333261a7d8224c from main
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34435#comment:12>