Opened 7 months ago
Last modified 6 weeks ago
#35381 assigned Bug
Regression on json null value constraints in django 4.2
Reported by: | Olivier Tabone | Owned by: | devday |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | 5.0 |
Severity: | Normal | Keywords: | |
Cc: | David Sanders, Simon Charette, Mariusz Felisiak, David Wobrock | Triage Stage: | Accepted |
Has patch: | no | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
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.
Attachments (1)
Change History (9)
by , 7 months ago
Attachment: | json_null_tests.patch added |
---|
comment:1 by , 7 months ago
Cc: | added |
---|---|
Triage Stage: | Unreviewed → Accepted |
Type: | Uncategorized → Bug |
comment:3 by , 6 months ago
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
- Introduce a
JSONNull
expression to disambigutate between the two. It would also allow the creation of model instances with a JSONnull
which is convoluated today as it requiresmodels.Value(None, models.JSONField())
to be used. - Deprecate
filter(jsonfield=None)
meaning JSONnull
by requiringJSONNull
to be used instead. Should we only do this at the top level to still allowjsonfield__key=None
to filter againstnull
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.
comment:4 by , 6 months ago
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?
comment:5 by , 6 months ago
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
- Introduce
JSONNull
- Make
filter(jsonfield=None)
raise a deprecation warning pointing at either usingfilter(jsonfield__isnull=True)
orfilter(jsonfield=JSONNull)
- At the end of the deprecation period switch
filter(jsonfield=None)
to meanfilter(jsonfield__isnull=True)
like 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.
follow-up: 7 comment:6 by , 3 months ago
Hello, if you're okay with it, I would like to start working on this ticket and submit a PR to address the issue.
comment:7 by , 3 months ago
Cc: | added |
---|
Replying to 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 :)
comment:8 by , 6 weeks ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
I think this is related to #34754, and I originally thought that this was another consequence of what Simon said there: I think the fundamental problem here is that:
but then I ran the proposed test with
--debug-sql
(and postgres) and noticed that the check code is casting the value in the database to::JSONB
independently of whether the value isNone
or not:So I kept digging and bisected the "bad" commit to 5c23d9f0c32f166c81ecb6f3f01d5077a6084318, and something like this makes the test pass:
django/db/models/fields/json.py
hasattr(value, "as_sql"):it makes the test
model_fields.test_jsonfield.TestSaveLoad.test_json_null_different_from_sql_null
fails for the check on the query:So further investigation is needed to come up with an acceptable solution.