[Django] #31324: Filter JSONField using `=None`
Django
-------------------------------------------+------------------------
Reporter: Nikolay Tretyak | Owner: nobody
Type: Bug | Status: new
Component: Uncategorized | Version: 2.2
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------------+------------------------
{{{
from django.contrib.postgres.fields import JSONField
from django.db import models
class A(models.Model):
field = JSONField(null=True)
class Meta:
app_label = 'T'
print(A.objects.filter(field=None).query) # SELECT "T_a"."id",
"T_a"."field" FROM "T_a" WHERE "T_a"."field" = 'null'
print(A.objects.filter(field__isnull=True).query) # SELECT "T_a"."id",
"T_a"."field" FROM "T_a" WHERE "T_a"."field" IS NULL
}}}
If I create a new object with {{{A.objects.create(field=None)}}}, it will
be stored in the database as NULL. Therefore, I think it would be better
to use {{{IS NULL}}} query in the first filter from the example. And it
worked like this at least in 1.9
Moreover, with the behavior that we have now I can't even do
{{{A.objects.filter(field=existing_obj.field)}}} if {{{existing_obj.field
is None}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31324>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Nikolay Tretyak | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Nikolay Tretyak):
* component: Uncategorized => Database layer (models, ORM)
--
Ticket URL: <https://code.djangoproject.com/ticket/31324#comment:1>
Django
Reporter: Nikolay Tretyak | Owner: Nikolay
| Tretyak
Type: Bug | Status: assigned
Component: Database layer | Version: 2.2
(models, ORM) |
Severity: Normal | Resolution:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Nikolay Tretyak):
* owner: nobody => Nikolay Tretyak
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/31324#comment:2>
Django
Reporter: Nikolay Tretyak | Owner: Nikolay
| Tretyak
Type: Bug | Status: assigned
Component: Database layer | Version: 2.2
(models, ORM) |
Severity: Normal | Resolution:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Nikolay Tretyak):
Test and possible fix: https://github.com/django/django/pull/12512/files
--
Ticket URL: <https://code.djangoproject.com/ticket/31324#comment:3>
Django
Reporter: Nikolay Tretyak | Owner: Nikolay
| Tretyak
Type: Bug | Status: assigned
Component: Database layer | Version: 2.2
(models, ORM) |
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Simon Charette):
* stage: Unreviewed => Accepted
Comment:
I think that could be a 2.1 regression caused by fixing #25718
(c979c0a2b8abca325a549961fd7a17bdc36bcb1f).
It looks like it could qualify for a data loss bug because
`Model.objects.filter(json_field=None).update(field=value)` would be a
noop starting for `Model.json_field = None` instances.
--
Ticket URL: <https://code.djangoproject.com/ticket/31324#comment:4>
Django
Reporter: Nikolay Tretyak | Owner: Nikolay
| Tretyak
Type: Bug | Status: assigned
Component: Database layer | Version: 2.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Nikolay Tretyak):
* has_patch: 0 => 1
Comment:
[https://github.com/django/django/pull/12512 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/31324#comment:5>
Django
Reporter: Nikolay Tretyak | Owner: Nikolay
| Tretyak
Type: Bug | Status: assigned
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* cc: sage (added)
* version: 2.2 => 3.0
Comment:
I'm not sure if that's an issue, I would say that it's decision made to
support `NULL` (SQL) and `null` (JSON). We support storing primitives in
`JSONField` so also `null` which is not the same as `NULL` in the sense of
SQL. We also decided to keep this behavior in new `JSONField`'s, see
[https://github.com/django/django/pull/12392/files#diff-
8306d3c952976534470d55bf80dff66bR245-R266
test_json_null_different_from_sql_null()] test and
[https://github.com/django/django/pull/11452#discussion_r335375254 a short
discussion]. This could be documented better in
[https://github.com/django/django/pull/12392 PR 12392].
IMO it's invalid ticket.
--
Ticket URL: <https://code.djangoproject.com/ticket/31324#comment:6>
Django
Reporter: Nikolay Tretyak | Owner: Nikolay
| Tretyak
Type: Bug | Status: assigned
Component: Database layer | Version: 3.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Nikolay Tretyak):
But if I have an object `a = A.objects.create()` which is stored in the
database as {{{field = NULL}}}
{{{A.objects.filter(pk=a.pk, field=a.field).exists() is False}}} which is
very strange.
So, to update the Django version in my project I have to migrate all
{{{NULL}}} values to {{{'null'}}}?
--
Ticket URL: <https://code.djangoproject.com/ticket/31324#comment:7>
Django
Reporter: Nikolay Tretyak | Owner: Nikolay
| Tretyak
Type: Bug | Status: assigned
Component: Database layer | Version: 3.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Alexey Boriskin):
* cc: Alexey Boriskin (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/31324#comment:8>
Django
Reporter: Nikolay Tretyak | Owner: Nikolay
| Tretyak
Type: Bug | Status: assigned
Component: Database layer | Version: 3.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Simon Charette):
* cc: Simon Charette (added)
Comment:
I remember a bit of discussion around the fact the `__isnull` lookup
should be used for this purpose but in retrospection it certainly feels
weird that the usual `field=None` and `field__isnull=True` symmetry is not
honoured for `JSONField`.
There's certainly a good reason for it because of the existence of the
JSON's `null` type but given `field=None` insertion and updates result in
SQL `NULL` and not JSON `null` it feels like `filter(field=None)` should
default to SQL `NULL` and `filter(field=Value('null'))` should be used to
target cases where `field=Value('null')` was used for insertions like in
`test_json_null_different_from_sql_null`.
--
Ticket URL: <https://code.djangoproject.com/ticket/31324#comment:9>
Django
Reporter: Nikolay Tretyak | Owner: Nikolay
| Tretyak
Type: Bug | Status: assigned
Component: Database layer | Version: 3.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Carlton Gibson):
There's definitely room for a docs clarification of the exact behaviour
here, but I think the current behaviour is correct/desired.
I think it was a deliberate change in behavior, rather than a regression.
([https://docs.djangoproject.com/en/3.0/releases/2.1/#miscellaneous It was
documented as a backwards incompatible change].)
#25718 came up because people using JSONField are wanting to query for the
(JSON) `null` in the vast majority of cases. (99%? 99.9%? ...)
For most occurrences, checking for (SQL) `NULL` is the wrong behaviour.
We also want(ed) the same behaviour for checking the base field for `null`
(`field=None`) as a nested key (`field__a__b__c=None`).
This is so much more helpful for users of JSONField, that the weirdness is
a cost worth paying. (Better docs, always.)
(The ambiguity around querying for `null` vs `NULL` is something that
previously I've seen come up repeatedly on DRF and django-filter.)
> So, to update the Django version in my project I have to migrate all
NULL values to 'null'?
I'm almost tempted towards "Yes" for that. Much in the same way as the
empty value for `CharField` is `''`, the empty string, rather that `None`
for JSONField, you don't really/often want to be writing `NULL` — the
empty value for JSON is `null`.
I wonder if (without an explicit `null=True`) `JSONModel(field=None)`
should write the JSON value `null`, rather than have to do
`JSONModel(field=Value('null'))`.
In, general, on this last, I think having users need to use `Value` for
the default use-case is not optimal.
--
Ticket URL: <https://code.djangoproject.com/ticket/31324#comment:10>
Django
Reporter: Nikolay Tretyak | Owner: Nikolay
| Tretyak
Type: Bug | Status: assigned
Component: Database layer | Version: 3.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Nikolay Tretyak):
Yes, {{{Charfield}}} uses {{{''}}} as an empty value. But if you use
{{{filter(charfield=None)}}} it will produce {{{IS NULL}}} sql, not {{{=
''}}}. And this behavior is clear. So, why should we transform
{{{filter(jsonfield=None)}}} to {{{= 'null'}}}?
I think that the majority of JSONField users want to check if the
{{{jsonfield__key}}} is {{{'null'}}}. But are there any use cases when
they want to store just {{{'null'}}} in the whole field instead of SQL
{{{NULL}}}?
--
Ticket URL: <https://code.djangoproject.com/ticket/31324#comment:11>
Django
Reporter: Nikolay Tretyak | Owner: Nikolay
| Tretyak
Type: Bug | Status: assigned
Component: Database layer | Version: 3.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Carlton Gibson):
If we're going to conjecture, I think in reality most users set a default,
such as `dict`, so they don't have empty fields... — but I stand by point:
if you're using JSONField you're best off setting `null` as the empty
value, so all fields contain JSON.
For this ticket, the relevant point it that I don't think we should have
the API favour SQL-like usage over JSON-like usage. And I really don't
think `=None` should behave differently for the base field as against a
nested key. JSONFields aren't quite the same as other fields OK—can we doc
that better—but they should be internally consistent so if I'm using one,
I can have reasonable expectations about how they'll behave.
--
Ticket URL: <https://code.djangoproject.com/ticket/31324#comment:12>
Django
Reporter: Nikolay Tretyak | Owner: Nikolay
| Tretyak
Component: Database layer | Version: 3.0
(models, ORM) |
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* status: assigned => closed
* resolution: => wontfix
Comment:
Addressing the comments here, and
[https://github.com/django/django/pull/11452#discussion_r335375254 the
discussion] on PR with new `JSONField` implementation, and
[https://github.com/django/django/pull/8239 the original PR], I'm marking
this as `wontfix`. I believe that the current behavior is expected. You
can start a discussion on DevelopersMailingList if you don't agree.
--
Ticket URL: <https://code.djangoproject.com/ticket/31324#comment:13>
Django
Reporter: Nikolay Tretyak | Owner: Nikolay
| Tretyak
Type: Bug | Status: closed
Component: Database layer | Version: 3.0
(models, ORM) |
Severity: Normal | Resolution: wontfix
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by felixxm):
#31899 is a duplicate for `get_or_create()`.
--
Ticket URL: <https://code.djangoproject.com/ticket/31324#comment:14>