[Django] #35167: JSONFIeld get_db_prep_value being called with `Cast` types
Django
-------------------------------------------+---------------------------
Reporter: shughes-uk | Owner: (none)
Type: Uncategorized | Status: new
Component: Error reporting | Version: 4.2
Severity: Normal | Keywords: JSONField
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------------+---------------------------
Running django 4.2.9.
We have a customized version of the standard `JsonField` where we have
overridden `from_db_value` and `get_prep_value`.
The worked in django ~4.1. Migrating to 4.2.9, the `get_prep_value`
function started receiving `Cast` objects as the `value`, exploding json
serializing/encryption , only during `bulk_update`.
We fixed it by overriding `get_db_prep_save to
{{{
def get_db_prep_save(self, value, connection):
if hasattr(value, "as_sql"):
return value
return self.get_db_prep_value(value, connection=connection,
prepared=False)
}}}
The jsonfield implementation does not check for `as_sql`. I'm not sure if
this is intentional or not (unmodified JSONField columns don't seem to
explode), possibly our implementation has messed up the jsonfield?
--
Ticket URL: <https://code.djangoproject.com/ticket/35167>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Samantha Hughes | Owner: (none)
Type: Bug | Status: new
Component: Error reporting | Version: 4.2
Keywords: JSONField | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Simon Charette):
* type: Uncategorized => Bug
* stage: Unreviewed => Accepted
Comment:
This highly likely relates to #34539 which was about making sure
`get_db_prep_value` **always** calls `get_prep_value` as it use to be case
in 4.1.
Given the default implementation of `get_db_prep_save` doesn't delegate to
`get_db_prep_value`
[https://github.com/django/django/blob/9cefdfc43f0bae696b56fa5a0bf22346f85affff/django/db/models/fields/__init__.py#L1012-L1013
when provided a compilable expression] I believe we should also make sure
`JSONField.get_db_prep_save` does something equivalent.
The challenge here is that `Value(output_field=JSONField())`, which
represents wrapped literal JSON values which could also be JSON strings,
must be able to make their way to `connection.ops.adapt_json_value(value,
self.encoder)`. At least they needed during the deprecation period maybe
it's no longer the case since `Value(value).as_sql` will do the right
thing by itself.
--
Ticket URL: <https://code.djangoproject.com/ticket/35167#comment:1>
Django
---------------------------------+------------------------------------
Reporter: Samantha Hughes | Owner: (none)
Type: Bug | Status: new
Component: Error reporting | Version: 4.2
Severity: Normal | Resolution:
Keywords: JSONField | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
---------------------------------+------------------------------------
You mentioned that you are using Django 4.2 so the following patch won't
apply cleanly against your branch but against main it gets the tests
passing
{{{#!diff
diff --git a/django/db/models/fields/json.py
b/django/db/models/fields/json.py
index 571e6e79f3..73b4a4dd50 100644
--- a/django/db/models/fields/json.py
+++ b/django/db/models/fields/json.py
@@ -99,18 +99,23 @@ def get_internal_type(self):
def get_db_prep_value(self, value, connection, prepared=False):
if not prepared:
value = self.get_prep_value(value)
- if isinstance(value, expressions.Value) and isinstance(
- value.output_field, JSONField
- ):
- value = value.value
- elif hasattr(value, "as_sql"):
- return value
return connection.ops.adapt_json_value(value, self.encoder)
def get_db_prep_save(self, value, connection):
+ # This slightly convoluted logic is to allow for `None` to be
used to
+ # store SQL `NULL` while `Value(None, JSONField())` can be used
to
+ # store JSON `null` while preventing compilable `as_sql` values
from
+ # making their way to `get_db_prep_value` which is what the
`super()`
+ # implementation does.
if value is None:
return value
- return self.get_db_prep_value(value, connection)
+ elif (
+ isinstance(value, expressions.Value)
+ and value.value is None
+ and isinstance(value.output_field, JSONField)
+ ):
+ value = None
+ return super().get_db_prep_save(value, connection)
def get_transform(self, name):
transform = super().get_transform(name)
}}}
Basically now that the deprecation shims have been removed the logic can
be slightly simplified to only have to special case the `Value(None,
JSONField())` which is truly the only problematic case in the
implementation of `JSONField` due to ambiguous nature of `None` with
regards to SQL `NULL` and JSON `null`.
--
Ticket URL: <https://code.djangoproject.com/ticket/35167#comment:2>
Django
---------------------------------+------------------------------------
Type: Bug | Status: new
Component: Error reporting | Version: 4.2
Severity: Normal | Resolution:
Keywords: JSONField | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
---------------------------------+------------------------------------
* cc: Simon Charette (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/35167#comment:3>
Django
---------------------------------+------------------------------------
Type: Bug | Status: new
Component: Error reporting | Version: 4.2
Severity: Normal | Resolution:
Keywords: JSONField | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
---------------------------------+------------------------------------
Thanks, the quick response is really appreciated. Any idea if this will
make it to a 4.2 patch or will we need to upgrade to ~5?
--
Ticket URL: <https://code.djangoproject.com/ticket/35167#comment:4>
Django
Reporter: Samantha Hughes | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: JSONField | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Jacob Walls):
* owner: (none) => nobody
* component: Error reporting => Database layer (models, ORM)
--
Ticket URL: <https://code.djangoproject.com/ticket/35167#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: Samantha Hughes | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: JSONField | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
> Any idea if this will make it to a 4.2 patch or will we need to upgrade
to ~5?
Assuming someone prepares a patch it should make the cut for 5.1 meant to
be released in August 2024. It won't be backported to 4.2 or 5.0 at this
point unfortunately.
--
Ticket URL: <https://code.djangoproject.com/ticket/35167#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: Samantha Hughes | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: JSONField | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Krish (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/35167#comment:7>