Repository navigation
fix(django-google-spanner): preserve JsonObject in JSONField adaptation and deserialization - #18989
sakthivelmanii wants to merge 3 commits into
Conversation
…on and deserialization
There was a problem hiding this comment.
Code Review
This pull request introduces support for JsonObject in Django Spanner's JSONField handling by implementing adapt_json_value in DatabaseOperations and updating from_db_value in JSONField. However, several issues were identified in the review: adapt_json_value bypasses custom encoders and causes type mismatches for primitive JSON values, which can be resolved by serializing and deserializing the value before wrapping it in JsonObject. Additionally, a missing import of JsonObject in __init__.py will cause a runtime NameError, and the unit tests should be expanded to cover primitive values and custom encoder serialization.
| def adapt_json_value(self, value, encoder): | ||
| if value is None: | ||
| return None | ||
| if isinstance(value, JsonObject): | ||
| return value | ||
| if isinstance(value, (dict, list)): | ||
| return JsonObject(value) | ||
| return super().adapt_json_value(value, encoder) |
There was a problem hiding this comment.
The current implementation of adapt_json_value has two critical issues:
- Custom Encoder Bypass: Directly wrapping
dictorlistvalues inJsonObjectbypasses Django's customencoder(e.g.,DjangoJSONEncoder). If the dictionary or list contains custom types likeUUID,Decimal, ordatetime, the Spanner client library's internal serializer will fail with aTypeErrorbecause it does not know how to serialize them. - Primitive Values Type Mismatch: If
valueis a primitive JSON value (e.g.,str,int,float,bool), it bypasses thedict/listcheck and falls back tosuper().adapt_json_value, which returns a serialized JSON string. Because this is a Pythonstr, the Spanner DB-API driver binds it asTypeCode.STRINGinstead ofTypeCode.JSON, leading to aProgrammingError: 400 Value has type STRING which cannot be inserted into column <col>, which has type JSON.
To resolve both issues, serialize the value using the custom encoder first, deserialize it back to standard Python types, and then wrap it in JsonObject.
| def adapt_json_value(self, value, encoder): | |
| if value is None: | |
| return None | |
| if isinstance(value, JsonObject): | |
| return value | |
| if isinstance(value, (dict, list)): | |
| return JsonObject(value) | |
| return super().adapt_json_value(value, encoder) | |
| def adapt_json_value(self, value, encoder): | |
| if value is None: | |
| return None | |
| if isinstance(value, JsonObject): | |
| return value | |
| serialized = json.dumps(value, cls=encoder) | |
| return JsonObject(json.loads(serialized)) |
References
- Do not import modules or classes inside functions if they are already imported at the module level.
There was a problem hiding this comment.
Updated adapt_json_value to serialize via super().adapt_json_value(value, encoder) and construct JsonObject.from_str(...) so custom encoders and primitive JSON scalars are handled properly.
| def jsonfield_from_db_value(self, value, expression, connection): | ||
| if isinstance(value, (dict, list, JsonObject)): | ||
| return value |
There was a problem hiding this comment.
The JsonObject class is used in the isinstance check but is not imported in this file, which will raise a NameError at runtime when from_db_value is called. Import JsonObject from google.cloud.spanner_v1 inside the function to avoid circular dependencies and ensure runtime correctness.
| def jsonfield_from_db_value(self, value, expression, connection): | |
| if isinstance(value, (dict, list, JsonObject)): | |
| return value | |
| def jsonfield_from_db_value(self, value, expression, connection): | |
| from google.cloud.spanner_v1 import JsonObject | |
| if isinstance(value, (dict, list, JsonObject)): | |
| return value |
There was a problem hiding this comment.
JsonObject is already imported at the module level in django_spanner/__init__.py (line 30). Also updated jsonfield_from_db_value to unwrap JsonObject null, array, and scalar values into their native Python equivalents.
| def test_adapt_json_value(self): | ||
| from django.db.models import JSONField | ||
| from google.cloud.spanner_v1 import JsonObject | ||
|
|
||
| self.assertIsNone(self.db_operations.adapt_json_value(None, None)) | ||
| obj = JsonObject({"a": 1}) | ||
| self.assertIs(self.db_operations.adapt_json_value(obj, None), obj) | ||
| adapted_dict = self.db_operations.adapt_json_value({"a": 1}, None) | ||
| self.assertIsInstance(adapted_dict, JsonObject) | ||
| adapted_list = self.db_operations.adapt_json_value([1, 2], None) | ||
| self.assertIsInstance(adapted_list, JsonObject) | ||
| self.assertEqual(self.db_operations.adapt_json_value("str", None), '"str"') | ||
|
|
||
| field = JSONField() | ||
| self.assertIs(field.from_db_value(obj, None, None), obj) | ||
| self.assertEqual(field.from_db_value({"a": 1}, None, None), {"a": 1}) | ||
| self.assertEqual(field.from_db_value('{"a": 1}', None, None), {"a": 1}) | ||
| self.assertIsNone(field.from_db_value(None, None, None)) |
There was a problem hiding this comment.
Update the unit tests to verify that primitive JSON values are correctly wrapped in JsonObject and to cover custom encoder serialization (e.g., using uuid.UUID and DjangoJSONEncoder).
def test_adapt_json_value(self):
import uuid
from django.core.serializers.json import DjangoJSONEncoder
from django.db.models import JSONField
from google.cloud.spanner_v1 import JsonObject
self.assertIsNone(self.db_operations.adapt_json_value(None, None))
obj = JsonObject({"a": 1})
self.assertIs(self.db_operations.adapt_json_value(obj, None), obj)
adapted_dict = self.db_operations.adapt_json_value({"a": 1}, None)
self.assertIsInstance(adapted_dict, JsonObject)
self.assertEqual(adapted_dict.value, {"a": 1})
adapted_list = self.db_operations.adapt_json_value([1, 2], None)
self.assertIsInstance(adapted_list, JsonObject)
self.assertEqual(adapted_list.value, [1, 2])
adapted_str = self.db_operations.adapt_json_value("str", None)
self.assertIsInstance(adapted_str, JsonObject)
self.assertEqual(adapted_str.value, "str")
# Test custom encoder serialization (e.g., UUID)
u = uuid.UUID("12345678-1234-5678-1234-567812345678")
adapted_uuid = self.db_operations.adapt_json_value({"u": u}, DjangoJSONEncoder)
self.assertIsInstance(adapted_uuid, JsonObject)
self.assertEqual(adapted_uuid.value, {"u": "12345678-1234-5678-1234-567812345678"})
field = JSONField()
self.assertIs(field.from_db_value(obj, None, None), obj)
self.assertEqual(field.from_db_value({"a": 1}, None, None), {"a": 1})
self.assertEqual(field.from_db_value('{"a": 1}', None, None), {"a": 1})
self.assertIsNone(field.from_db_value(None, None, None))There was a problem hiding this comment.
Updated test_adapt_json_value to cover primitive JSON values, custom encoder serialization (UUID with DjangoJSONEncoder), and JsonObject unwrapping in from_db_value.
Problem
JSONFieldagainst live Cloud Spanner fails withProgrammingError: 400 Value has type STRING which cannot be inserted into column <col>, which has type JSON.django_spannerregistersjson_field_get_prep_valueto wrap values inJsonObject, Django'sJSONField.get_db_prep_valuesubsequently passes that value toconnection.ops.adapt_json_value(value, self.encoder).BaseDatabaseOperations.adapt_json_valuecallsjson.dumps(value), serializingJsonObjectback into a Pythonstr(TypeCode.STRING).JSONcolumns back from Spanner, the Python Spanner client may already return deserializedJsonObject,dict, orlistinstances, which causesJSONField.from_db_value(json.loads(value)) to raiseTypeError.Solution
DatabaseOperations.adapt_json_valueindjango_spanner/operations.pyto preserveJsonObjectand wrapdict/listvalues inJsonObjectso DB-API binds the parameter with Spanner'sJSONtype.json_field_from_db_valueonJSONField.from_db_valueindjango_spanner/__init__.pyto return already-deserializedJsonObject,dict, orlistvalues directly before falling back tojson.loads.tests/unit/django_spanner/test_operations.pycoveringadapt_json_valueandJSONField.from_db_value.