Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#31324 closed Bug (wontfix)

Filter JSONField using `=None`

Reported by: Nikolay Tretyak Owned by: Nikolay Tretyak
Component: Database layer (models, ORM) Version: 3.0
Severity: Normal Keywords:
Cc: Sage Abdullah, Alexey Boriskin, Simon Charette Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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

Change History (14)

comment:1 by Nikolay Tretyak, 4 years ago

Component: UncategorizedDatabase layer (models, ORM)

comment:2 by Nikolay Tretyak, 4 years ago

Owner: changed from nobody to Nikolay Tretyak
Status: newassigned

comment:4 by Simon Charette, 4 years ago

Triage Stage: UnreviewedAccepted

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 for Model.json_field = None instances since this commit landed while it wasn't the case before.

Last edited 4 years ago by Simon Charette (previous) (diff)

comment:5 by Nikolay Tretyak, 4 years ago

Has patch: set

comment:6 by Mariusz Felisiak, 4 years ago

Cc: Sage Abdullah added
Version: 2.23.0

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 test_json_null_different_from_sql_null() test and a short discussion. This could be documented better in PR 12392.

IMO it's invalid ticket.

comment:7 by Nikolay Tretyak, 4 years ago

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'?

comment:8 by Alexey Boriskin, 4 years ago

Cc: Alexey Boriskin added

comment:9 by Simon Charette, 4 years ago

Cc: Simon Charette added

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.

comment:10 by Carlton Gibson, 4 years ago

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. (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.

comment:11 by Nikolay Tretyak, 4 years ago

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?

comment:12 by Carlton Gibson, 4 years ago

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 the 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.

Last edited 4 years ago by Carlton Gibson (previous) (diff)

comment:13 by Mariusz Felisiak, 4 years ago

Resolution: wontfix
Status: assignedclosed

Addressing the comments here, and the discussion on PR with new JSONField implementation, and 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.

comment:14 by Mariusz Felisiak, 4 years ago

#31899 is a duplicate for get_or_create().

Note: See TracTickets for help on using tickets.
Back to Top