JSONField get_db_prep_value being called with `Cast` types

[Samantha Hughes]

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?

[Simon Charette]

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

[Simon Charette]

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

django/db/models/fields/json.py

@@ -99 +99 @@
     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.

[Samantha Hughes]

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?

[Simon Charette]

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.

[HeejunShin]

Could i try this ticket?

[bcail]

@HeejunShin, nobody owns this ticket, so you're welcome to work on this ticket. You can assign it to yourself and/or add comments as you work on it.

[Almaz]

Is this a relevant problem?

Because, after some period of exploration, I see that most of the code were updated.

[HeejunShin]

Is this ticket solved?

[Simon Charette]

Replying to HeejunShin:

Is this ticket solved?

No. While it has an inline tentative patch no one ​has opened a PR on Github with the proposed changes including a regression test yet.

[HeejunShin]

Replying to Simon Charette:

thank you for your response.

[HeejunShin]

Sorry for the slow progress. I am currently analyzing the code looking at the customizing doc of the Field class.

[Natalia Bidart]

Ticket #35381 is related to this.

[HeejunShin]

Replying to Natalia Bidart:

Ticket #35381 is related to this.

Is this a problem that needs to be resolved as quickly as possible?
Should I also refer to the ticket you mentioned to solve the problem?
Any advice would be greatly appreciated.

[Natalia Bidart]

Hello HeejunShin, thank you for asking! You can take the time you need to work on this ticket, in general no ticket is urgent unless is a release blocker, which is not the case for this one.
Also, if you can't work on this anymore, that is also fine and you can just de-assign yourself from the ticket and eventually someone else would pick it up.

Django is built and maintained by its community and everyone contributes their time and energy when they can.

[HeejunShin]

Replying to Natalia Bidart:

Hello HeejunShin, thank you for asking! You can take the time you need to work on this ticket, in general no ticket is urgent unless is a release blocker, which is not the case for this one.
Also, if you can't work on this anymore, that is also fine and you can just de-assign yourself from the ticket and eventually someone else would pick it up.

Django is built and maintained by its community and everyone contributes their time and energy when they can.

thank you for your response.
I'll try my best.

[HeejunShin]

It's really late though
I was just analyzing the code and reproduced the bug.

[HeejunShin]

i have a question.
maybe The bug reproduction seems to be incorrect.

i tested using 2 ways.

tests[1].json = {"test": Cast('good', output_field=CharField())}
Test.objects.bulk_update(tests_will_be_updated, fields=["json"])

tests[1].json = Cast("good", output_field=CharField())
Test.objects.bulk_update(tests_will_be_updated, fields=["json"])

first one, i got "raise TypeError(f'Object of type {o.class.name} 'TypeError: Object of type Cast is not JSON serializable" error.
but value argument type in get_db_prep_value is dict.

second one, i could not get error.
value argument type in get_db_prep_value is <class 'django.db.models.expressions.Case'>. (CASE WHEN <WhereNode: (AND: Exact(Col(polls_test, polls.Test.id), 2))> THEN Cast(Col(polls_test, polls.Test.json)), ELSE Value(None)).

i thought first one is type dict, so this is wrong.
is second one wrong way that reproduce bug case?