[Django] #35381: Regression on json null value constraints in django 4.2

[Django]

#35381: Regression on json null value constraints in django 4.2
-------------------------------------+-------------------------------------
Reporter: Olivier | Owner: nobody
Tabone |
Type: | Status: new
Uncategorized |
Component: Database | Version: 5.0
layer (models, ORM) |
Severity: Normal | Keywords:
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
Regression is still present in 5.0 and main branch

given a model defined as follow

{{{
from django.db import models


class JSONFieldModel(models.Model):
data = models.JSONField(null=True, blank=True, default=None)

class Meta:
required_db_features = {"supports_json_field"}
constraints = [
models.CheckConstraint(
check=~models.Q(data=models.Value(None,
models.JSONField())),
name="json_data_cant_be_json_null",
),
]

}}}

the following test works in django 4.1.13, fails in later version

{{{
from django.test import TestCase, skipUnlessDBFeature

from .models import JSONFieldModel


class CheckConstraintTests(TestCase):
@skipUnlessDBFeature("supports_json_field")
def test_full_clean_on_null_value(self):
instance = JSONFieldModel.objects.create(data=None) # data = SQL
Null value
instance.full_clean()

}}}

{{{
tests/runtests.py json_null_tests
======================================================================
ERROR: test_full_clean_on_null_value
(json_null_tests.tests.CheckConstraintTests.test_full_clean_on_null_value)
----------------------------------------------------------------------
Traceback (most recent call last):
File
"/Users/olivier/dev/django.dev/olibook/django/django/test/testcases.py",
line 1428, in skip_wrapper
return test_func(*args, **kwargs)
^^^^^^^^^^^^^^^^^^^^^^^^^^
File
"/Users/olivier/dev/django.dev/olibook/django/tests/json_null_tests/tests.py",
line 13, in test_full_clean_on_null_value
instance.full_clean()
File
"/Users/olivier/dev/django.dev/olibook/django/django/db/models/base.py",
line 1504, in full_clean
raise ValidationError(errors)
django.core.exceptions.ValidationError: {'__all__': ['Constraint
“json_data_cant_be_json_null” is violated.']}

----------------------------------------------------------------------
}}}

Attached a patch that will create run json_null_tests folder in django's
`tests` directories. Test can be run with

{{{
tests/runtests.py json_null_tests
}}}

NOTE : in django 5.1 the `CheckConstraint.check` parameter as been renamed
to `CheckConstraint.condition`.

I ran a git bisect on this test case:

8fcb9f1f106cf60d953d88aeaa412cc625c60029 is bad
e14d08cd894e9d91cb5d9f44ba7532c1a223f458 is good

{{{
git bisect run tests/runtests.py json_null_tests
}}}

5c23d9f0c32f166c81ecb6f3f01d5077a6084318 is identified as first bad commit

the issue appears with both sqlite and postgres backends

A few words on what we I'm trying to achieve (some context on the
regression):

The `json_data_cant_be_json_null` aims to prevent having `JsonModel.data =
None` (SQL Null) and `JSONModel.data = Value(None, JSONField())` (JSON
null) values in the database. These leads to unwanted behaviors when
filtering on the null value, and I settled on representing the absence of
value with SQL Null and not JSON null.

The app was running in django 4.1 and I'm upgrading it to later django
version. That's how the issue appeared.
--
Ticket URL: <https://code.djangoproject.com/ticket/35381>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#35381: Regression on json null value constraints in django 4.2
-------------------------------------+-------------------------------------

Reporter: Olivier Tabone | Owner: nobody
Type: Uncategorized | Status: new
Component: Database layer | Version: 5.0
(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 Olivier Tabone):

* Attachment "json_null_tests.patch" added.

[Django]

#35381: Regression on json null value constraints in django 4.2
-------------------------------------+-------------------------------------
Reporter: Olivier Tabone | Owner: nobody

Type: Bug | Status: new

Component: Database layer | Version: 5.0
(models, ORM) |
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 Natalia Bidart):

* cc: David Sanders, Simon Charette, Mariusz Felisiak (added)
* stage: Unreviewed => Accepted
* type: Uncategorized => Bug

Comment:

I think this is related to #34754, and I originally thought that this was
another consequence of what [ticket:34754#comment:4 Simon said there]: ''I
think the fundamental problem here is that:''
{{{#!python
MyModel.objects.create(values=None) # Creates an object with a SQL NULL
MyModel.objects.filter(values=None) # Filters for objects with JSON NULL
}}}

but then I ran the proposed test with `--debug-sql` and noticed that the
check code is casting the value in the database to `::JSONB` independently
of whether the value is `None` or not:

{{{
INSERT INTO "ticket_35381_jsonfieldmodel" ("data")
VALUES (NULL) RETURNING "ticket_35381_jsonfieldmodel"."id";

args=(NONE,);

ALIAS=DEFAULT (0.000)
SELECT 1 AS "_check"
WHERE COALESCE((NOT ('null'::JSONB = 'null'::JSONB)), TRUE);

args=(Int4(1),
Jsonb(NONE),
Jsonb(NONE),
TRUE);

ALIAS=DEFAULT
}}}

So I kept digging and bisected the "bad" commit to
5c23d9f0c32f166c81ecb6f3f01d5077a6084318, and something like this makes
the test pass:

{{{#!diff
diff --git a/django/db/models/fields/json.py
b/django/db/models/fields/json.py
index 1b219e620c..9bd6c4b252 100644
--- a/django/db/models/fields/json.py
+++ b/django/db/models/fields/json.py
@@ -103,7 +103,7 @@ class JSONField(CheckFieldDefaultMixin, Field):
value.output_field, JSONField
):
value = value.value
- elif hasattr(value, "as_sql"):
+ elif value is None or hasattr(value, "as_sql"):
return value
return connection.ops.adapt_json_value(value, self.encoder)
}}}

it makes the test
`model_fields.test_jsonfield.TestSaveLoad.test_json_null_different_from_sql_null`
fails for the check on the query:
{{{
NullableJSONModel.objects.filter(value=Value(None, JSONField())) ==
[json_null]
}}}

So further investigation is needed to come up with an acceptable solution.
--
Ticket URL: <https://code.djangoproject.com/ticket/35381#comment:1>

[Django]

#35381: Regression on json null value constraints in django 4.2
-------------------------------------+-------------------------------------
Reporter: Olivier Tabone | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 5.0
(models, ORM) |
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 Natalia Bidart):

I'm now investigating whether this is a duplicate of #35167 or not.
--
Ticket URL: <https://code.djangoproject.com/ticket/35381#comment:2>

[Django]

#35381: Regression on json null value constraints in django 4.2
-------------------------------------+-------------------------------------
Reporter: Olivier Tabone | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 5.0
(models, ORM) |
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 Simon Charette):

I've looked into it as well and #35167 only provides a partial solution
here IMO as it would require the `Q.check` logic to also pass around
`for_save` to activate the code path it introduces.

I'm starting to believe that in order to finally solve this null ambiguity
problem we should

1. Introduce a `JSONNull` expression to disambigutate between the two. It
would also allow the creation of model instances with a JSON `null` which
is convoluated today as it requires `models.Value(None,
models.JSONField())` to be used.
2. Deprecate `filter(jsonfield=None)` meaning JSON `null` by requiring
`JSONNull` to be used instead. Should we only do this at the top level to
still allow `jsonfield__key=None` to filter against `null` keys? An
alternative would be to introduce a `__jsonnull` lookup.

The ''good news'' is that Django makes it very hard to store JSON null in
the first place since #34754 so this kind of constraints should be rarely
needed.
--
Ticket URL: <https://code.djangoproject.com/ticket/35381#comment:3>

[Django]

#35381: Regression on json null value constraints in django 4.2
-------------------------------------+-------------------------------------
Reporter: Olivier Tabone | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 5.0
(models, ORM) |
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 Natalia Bidart):

Thank you Simon. In my view, and in an ideal situation, we shouldn't allow
"anywhere" to use `None` as the json's `null`. I think we should reserve
`None` only to represent SQL's `NULL`, and have a representation for the
(string) json's `null` (I understand this matches the new `JSONNull`
expression you are proposing).

> Should we only do this at the top level to still allow
`jsonfield__key=None` to filter against null keys?

I would say no, because it then gets very confusing.

> An alternative would be to introduce a `__jsonnull` lookup.

Would this lookup allow further lookups to be chained?
--
Ticket URL: <https://code.djangoproject.com/ticket/35381#comment:4>

[Django]

#35381: Regression on json null value constraints in django 4.2
-------------------------------------+-------------------------------------
Reporter: Olivier Tabone | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 5.0
(models, ORM) |
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 Simon Charette):

> I think we should reserve None only to represent SQL's NULL, and have a

representation for the (string) json's null (I understand this matches the
new JSONNull expression you are proposing).

Yes it matches the `JSONNull` proposal. If we take away the ability to use
`None` for filtering against JSON `null` we should provide an alternative.

The deprecation could look like

1. Introduce `JSONNull`
2. Make `filter(jsonfield=None)` raise a deprecation warning pointing at
either using `filter(jsonfield__isnull=True)` or
`filter(jsonfield=JSONNull)`
3. At the end of the deprecation period switch `filter(jsonfield=None)` to
mean `filter(jsonfield__isnull=True)` link on all other fields

It leaves the problem of having JSON `null` not surviving a round trip to
the database as both SQL `NULL` and `json.loads("null")` are turned into
Python `None` but that's a different issue that can be addressed with a
specialized `decoder` if users require it.

> Would this lookup allow further lookups to be chained?

It could or not, what gets returned from a lookup can be prevent further
chaining if we want.
--
Ticket URL: <https://code.djangoproject.com/ticket/35381#comment:5>

[Django]

#35381: Regression on json null value constraints in django 4.2
-------------------------------------+-------------------------------------
Reporter: Olivier Tabone | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 5.0
(models, ORM) |
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 Mohammad Salehi):

Hello, if you're okay with it, I would like to start working on this
ticket and submit a PR to address the issue.
--
Ticket URL: <https://code.djangoproject.com/ticket/35381#comment:6>

[Django]

#35381: Regression on json null value constraints in django 4.2
-------------------------------------+-------------------------------------
Reporter: Olivier Tabone | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 5.0
(models, ORM) |
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 David Wobrock):

* cc: David Wobrock (added)

Comment:

Replying to [comment:6 Mohammad Salehi]:

> Hello, if you're okay with it, I would like to start working on this
ticket and submit a PR to address the issue.

Of course! Feel free to assign the ticket to you and open a patch.
Bear in mind the suggested approach from the discussions above :)
--
Ticket URL: <https://code.djangoproject.com/ticket/35381#comment:7>

[Django]

#35381: Regression on json null value constraints in django 4.2
-------------------------------------+-------------------------------------

Reporter: Olivier Tabone | Owner: devday
Type: Bug | Status: assigned

Component: Database layer | Version: 5.0
(models, ORM) |
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 devday):

* owner: nobody => devday
* status: new => assigned

--
Ticket URL: <https://code.djangoproject.com/ticket/35381#comment:8>

[Django]

#35381: Regression on json null value constraints in django 4.2
-------------------------------------+-------------------------------------

Reporter: Olivier Tabone | Owner: wookkl

Type: Bug | Status: assigned
Component: Database layer | Version: 5.0
(models, ORM) |
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 wookkl):

* owner: devday => wookkl

--
Ticket URL: <https://code.djangoproject.com/ticket/35381#comment:9>

[Django]

#35381: Regression on json null value constraints in django 4.2
-------------------------------------+-------------------------------------
Reporter: Olivier Tabone | Owner: wookkl
Type: Bug | Status: assigned
Component: Database layer | Version: 5.0
(models, ORM) |
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 Natalia Bidart):

* cc: Sage Abdullah (added)

Comment:

In the context of investigating #36418, I have re-reviewed the different
tickets that we have related to confusing behaviours of SQL's `NULL` vs
JSON's string `null`, and I think this ticket should be edited in summary
and description to match the New Feature described in comment:5 (or
alternatively, close this one as invalid and open a new ticket).
--
Ticket URL: <https://code.djangoproject.com/ticket/35381#comment:10>

[Django]

#35381: Regression on json null value constraints in django 4.2
-------------------------------------+-------------------------------------
Reporter: Olivier Tabone | Owner: wookkl
Type: Bug | Status: assigned
Component: Database layer | Version: 5.0
(models, ORM) |
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 Clifford Gama):

* cc: Clifford Gama (added)

--
Ticket URL: <https://code.djangoproject.com/ticket/35381#comment:11>

[Django]

#35381: Regression on json null value constraints in django 4.2
-------------------------------------+-------------------------------------
Reporter: Olivier Tabone | Owner: wookkl
Type: Bug | Status: assigned
Component: Database layer | Version: 5.0
(models, ORM) |
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 Adam Johnson):

* cc: Adam Johnson (added)

--
Ticket URL: <https://code.djangoproject.com/ticket/35381#comment:12>

[Django]

#35381: Regression on json null value constraints in django 4.2
-------------------------------------+-------------------------------------

Reporter: Olivier Tabone | Owner: (none)
Type: Bug | Status: new

Component: Database layer | Version: 5.0
(models, ORM) |
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 wookkl):

* owner: wookkl => (none)
* status: assigned => new

--
Ticket URL: <https://code.djangoproject.com/ticket/35381#comment:13>