From 0ff46591ac3010841c73fd26e0fef93995fedd99 Mon Sep 17 00:00:00 2001 From: Simon Charette Date: Wed, 2 Nov 2022 22:03:05 -0400 Subject: [PATCH] Refs #33308 -- Deprecated support for passing encoded JSON string literals to JSONField & co. JSON should be provided as literal Python objects an not in their encoded string literal forms. --- django/contrib/postgres/aggregates/general.py | 52 +++++++++++++-- django/db/models/aggregates.py | 2 + django/db/models/fields/__init__.py | 4 ++ django/db/models/fields/json.py | 31 ++++++++- django/db/models/sql/compiler.py | 12 +--- django/db/models/sql/query.py | 6 +- docs/internals/deprecation.txt | 3 + docs/releases/4.2.txt | 36 ++++++++++ docs/topics/db/queries.txt | 17 ++++- tests/model_fields/test_jsonfield.py | 31 ++++++++- tests/postgres_tests/test_aggregates.py | 66 +++++++++++++++++-- 11 files changed, 230 insertions(+), 30 deletions(-) diff --git a/django/contrib/postgres/aggregates/general.py b/django/contrib/postgres/aggregates/general.py index f8b40fb709cd..3de59dfcfd11 100644 --- a/django/contrib/postgres/aggregates/general.py +++ b/django/contrib/postgres/aggregates/general.py @@ -1,8 +1,9 @@ +import json import warnings from django.contrib.postgres.fields import ArrayField from django.db.models import Aggregate, BooleanField, JSONField, TextField, Value -from django.utils.deprecation import RemovedInDjango50Warning +from django.utils.deprecation import RemovedInDjango50Warning, RemovedInDjango51Warning from .mixins import OrderableAggMixin @@ -31,6 +32,14 @@ def __init__(self, *expressions, default=NOT_PROVIDED, **extra): self._default_provided = True super().__init__(*expressions, default=default, **extra) + def resolve_expression(self, *args, **kwargs): + resolved = super().resolve_expression(*args, **kwargs) + if not self._default_provided: + resolved.empty_result_set_value = getattr( + self, "deprecation_empty_result_set_value", self.deprecation_value + ) + return resolved + def convert_value(self, value, expression, connection): if value is None and not self._default_provided: warnings.warn(self.deprecation_msg, category=RemovedInDjango50Warning) @@ -48,8 +57,7 @@ class ArrayAgg(DeprecatedConvertValueMixin, OrderableAggMixin, Aggregate): deprecation_msg = ( "In Django 5.0, ArrayAgg() will return None instead of an empty list " "if there are no rows. Pass default=None to opt into the new behavior " - "and silence this warning or default=Value([]) to keep the previous " - "behavior." + "and silence this warning or default=[] to keep the previous behavior." ) @property @@ -87,13 +95,46 @@ class JSONBAgg(DeprecatedConvertValueMixin, OrderableAggMixin, Aggregate): # RemovedInDjango50Warning deprecation_value = "[]" + deprecation_empty_result_set_value = property(lambda self: []) deprecation_msg = ( "In Django 5.0, JSONBAgg() will return None instead of an empty list " "if there are no rows. Pass default=None to opt into the new behavior " - "and silence this warning or default=Value('[]') to keep the previous " + "and silence this warning or default=[] to keep the previous " "behavior." ) + # RemovedInDjango51Warning: When the deprecation ends, remove __init__(). + # + # RemovedInDjango50Warning: When the deprecation ends, replace with: + # def __init__(self, *expressions, default=None, **extra): + def __init__(self, *expressions, default=NOT_PROVIDED, **extra): + super().__init__(*expressions, default=default, **extra) + if ( + isinstance(default, Value) + and isinstance(default.value, str) + and not isinstance(default.output_field, JSONField) + ): + value = default.value + try: + decoded = json.loads(value) + except json.JSONDecodeError: + warnings.warn( + "Passing a Value() with an output_field that isn't a JSONField as " + "JSONBAgg(default) is deprecated. Pass default=" + f"Value({value!r}, output_field=JSONField()) instead.", + stacklevel=2, + category=RemovedInDjango51Warning, + ) + self.default.output_field = self.output_field + else: + self.default = Value(decoded, self.output_field) + warnings.warn( + "Passing an encoded JSON string as JSONBAgg(default) is " + f"deprecated. Pass default={decoded!r} instead.", + stacklevel=2, + category=RemovedInDjango51Warning, + ) + class StringAgg(DeprecatedConvertValueMixin, OrderableAggMixin, Aggregate): function = "STRING_AGG" @@ -106,8 +147,7 @@ class StringAgg(DeprecatedConvertValueMixin, OrderableAggMixin, Aggregate): deprecation_msg = ( "In Django 5.0, StringAgg() will return None instead of an empty " "string if there are no rows. Pass default=None to opt into the new " - "behavior and silence this warning or default=Value('') to keep the " - "previous behavior." + 'behavior and silence this warning or default="" to keep the previous behavior.' ) def __init__(self, expression, delimiter, **extra): diff --git a/django/db/models/aggregates.py b/django/db/models/aggregates.py index e2e0ef762b3c..e672f0aeb09c 100644 --- a/django/db/models/aggregates.py +++ b/django/db/models/aggregates.py @@ -85,6 +85,8 @@ def resolve_expression( return c if hasattr(default, "resolve_expression"): default = default.resolve_expression(query, allow_joins, reuse, summarize) + if default._output_field_or_none is None: + default.output_field = c._output_field_or_none else: default = Value(default, c._output_field_or_none) c.default = None # Reset the default argument before wrapping. diff --git a/django/db/models/fields/__init__.py b/django/db/models/fields/__init__.py index 2a98396cadb6..cd995de80fb6 100644 --- a/django/db/models/fields/__init__.py +++ b/django/db/models/fields/__init__.py @@ -921,6 +921,8 @@ def get_db_prep_value(self, value, connection, prepared=False): def get_db_prep_save(self, value, connection): """Return field's value prepared for saving into a database.""" + if hasattr(value, "as_sql"): + return value return self.get_db_prep_value(value, connection=connection, prepared=False) def has_default(self): @@ -1715,6 +1717,8 @@ def to_python(self, value): def get_db_prep_value(self, value, connection, prepared=False): if not prepared: value = self.get_prep_value(value) + if hasattr(value, "as_sql"): + return value return connection.ops.adapt_decimalfield_value( value, self.max_digits, self.decimal_places ) diff --git a/django/db/models/fields/json.py b/django/db/models/fields/json.py index c0242bd7bee6..eb2d35f10058 100644 --- a/django/db/models/fields/json.py +++ b/django/db/models/fields/json.py @@ -1,9 +1,10 @@ import json +import warnings from django import forms from django.core import checks, exceptions from django.db import NotSupportedError, connections, router -from django.db.models import lookups +from django.db.models import expressions, lookups from django.db.models.constants import LOOKUP_SEP from django.db.models.fields import TextField from django.db.models.lookups import ( @@ -11,6 +12,7 @@ PostgresOperatorLookup, Transform, ) +from django.utils.deprecation import RemovedInDjango51Warning from django.utils.translation import gettext_lazy as _ from . import Field @@ -97,7 +99,32 @@ def get_internal_type(self): return "JSONField" def get_db_prep_value(self, value, connection, prepared=False): - if hasattr(value, "as_sql"): + # RemovedInDjango51Warning: When the deprecation ends, replace with: + # if ( + # isinstance(value, expressions.Value) + # and isinstance(value.output_field, JSONField) + # ): + # value = value.value + # elif hasattr(value, "as_sql"): ... + if isinstance(value, expressions.Value): + if isinstance(value.value, str) and not isinstance( + value.output_field, JSONField + ): + try: + value = json.loads(value.value, cls=self.decoder) + except json.JSONDecodeError: + value = value.value + else: + warnings.warn( + "Providing an encoded JSON string via Value() is deprecated. " + f"Use Value({value!r}, output_field=JSONField()) instead.", + category=RemovedInDjango51Warning, + ) + elif isinstance(value.output_field, JSONField): + value = value.value + else: + return value + elif hasattr(value, "as_sql"): return value return connection.ops.adapt_json_value(value, self.encoder) diff --git a/django/db/models/sql/compiler.py b/django/db/models/sql/compiler.py index caf36382b545..2b66ab12b446 100644 --- a/django/db/models/sql/compiler.py +++ b/django/db/models/sql/compiler.py @@ -1637,9 +1637,7 @@ def prepare_value(self, field, value): "Window expressions are not allowed in this query (%s=%r)." % (field.name, value) ) - else: - value = field.get_db_prep_save(value, connection=self.connection) - return value + return field.get_db_prep_save(value, connection=self.connection) def pre_save_val(self, field, obj): """ @@ -1893,18 +1891,14 @@ def as_sql(self): ) elif hasattr(val, "prepare_database_save"): if field.remote_field: - val = field.get_db_prep_save( - val.prepare_database_save(field), - connection=self.connection, - ) + val = val.prepare_database_save(field) else: raise TypeError( "Tried to update field %s with a model instance, %r. " "Use a value compatible with %s." % (field, val, field.__class__.__name__) ) - else: - val = field.get_db_prep_save(val, connection=self.connection) + val = field.get_db_prep_save(val, connection=self.connection) # Getting the placeholder for the field. if hasattr(field, "get_placeholder"): diff --git a/django/db/models/sql/query.py b/django/db/models/sql/query.py index 521054f69e28..9e70aadca11c 100644 --- a/django/db/models/sql/query.py +++ b/django/db/models/sql/query.py @@ -522,9 +522,9 @@ def get_aggregation(self, using, aggregate_exprs): result = compiler.execute_sql(SINGLE) if result is None: result = empty_set_result - - converters = compiler.get_converters(outer_query.annotation_select.values()) - result = next(compiler.apply_converters((result,), converters)) + else: + converters = compiler.get_converters(outer_query.annotation_select.values()) + result = next(compiler.apply_converters((result,), converters)) return dict(zip(outer_query.annotation_select, result)) diff --git a/docs/internals/deprecation.txt b/docs/internals/deprecation.txt index abd5a363bc40..02a12f08232d 100644 --- a/docs/internals/deprecation.txt +++ b/docs/internals/deprecation.txt @@ -39,6 +39,9 @@ details on these changes. * The ``TransactionTestCase.assertQuerysetEqual()`` method will be removed. +* Support for passing encoded JSON string literals to ``JSONField`` and + associated lookups and expressions will be removed. + .. _deprecation-removed-in-5.0: 5.0 diff --git a/docs/releases/4.2.txt b/docs/releases/4.2.txt index 8153eefebc2d..054f8d66212c 100644 --- a/docs/releases/4.2.txt +++ b/docs/releases/4.2.txt @@ -444,6 +444,42 @@ but it should not be used for new migrations. Use :class:`~django.db.migrations.operations.AddIndex` and :class:`~django.db.migrations.operations.RemoveIndex` operations instead. +Passing encoded JSON string literals to ``JSONField`` is deprecated +------------------------------------------------------------------- + +``JSONField`` and its associated lookups and aggregates use to allow passing +JSON encoded string literals which caused ambiguity on whether string literals +were already encoded from database backend's perspective. + +During the deprecation period string literals will be attempted to be JSON +decoded and a warning will be emitted on success that points at passing +non-encoded forms instead. + +Code that use to pass JSON encoded string literals:: + + Document.objects.bulk_create( + Document(data=Value("null")), + Document(data=Value("[]")), + Document(data=Value('"foo-bar"')), + ) + Document.objects.annotate( + JSONBAgg("field", default=Value('[]')), + ) + +Should become:: + + Document.objects.bulk_create( + Document(data=Value(None, JSONField())), + Document(data=[]), + Document(data="foo-bar"), + ) + Document.objects.annotate( + JSONBAgg("field", default=[]), + ) + +From Django 5.1+ string literals will be implicitly interpreted as JSON string +literals. + Miscellaneous ------------- diff --git a/docs/topics/db/queries.txt b/docs/topics/db/queries.txt index 5114efb57d9c..977e287c533b 100644 --- a/docs/topics/db/queries.txt +++ b/docs/topics/db/queries.txt @@ -971,7 +971,7 @@ Storing and querying for ``None`` As with other fields, storing ``None`` as the field's value will store it as SQL ``NULL``. While not recommended, it is possible to store JSON scalar -``null`` instead of SQL ``NULL`` by using :class:`Value('null') +``null`` instead of SQL ``NULL`` by using :class:`Value(None, JSONField()) `. Whichever of the values is stored, when retrieved from the database, the Python @@ -987,11 +987,13 @@ query for SQL ``NULL``, use :lookup:`isnull`:: >>> Dog.objects.create(name='Max', data=None) # SQL NULL. - >>> Dog.objects.create(name='Archie', data=Value('null')) # JSON null. + >>> Dog.objects.create( + ... name='Archie', data=Value(None, JSONField()) # JSON null. + ... ) >>> Dog.objects.filter(data=None) ]> - >>> Dog.objects.filter(data=Value('null')) + >>> Dog.objects.filter(data=Value(None, JSONField()) ]> >>> Dog.objects.filter(data__isnull=True) ]> @@ -1007,6 +1009,15 @@ Unless you are sure you wish to work with SQL ``NULL`` values, consider setting Storing JSON scalar ``null`` does not violate :attr:`null=False `. +.. versionchanged:: 4.2 + + Support for expressing JSON ``null`` using ``Value(None, JSONField())`` was + added. + +.. deprecated:: 4.2 + + Passing ``Value("null")`` to express JSON ``null`` is deprecated. + .. fieldlookup:: jsonfield.key Key, index, and path transforms diff --git a/tests/model_fields/test_jsonfield.py b/tests/model_fields/test_jsonfield.py index 277e8aaa3c1c..05816817ef6a 100644 --- a/tests/model_fields/test_jsonfield.py +++ b/tests/model_fields/test_jsonfield.py @@ -19,6 +19,7 @@ ExpressionWrapper, F, IntegerField, + JSONField, OuterRef, Q, Subquery, @@ -36,6 +37,7 @@ from django.db.models.functions import Cast from django.test import SimpleTestCase, TestCase, skipIfDBFeature, skipUnlessDBFeature from django.test.utils import CaptureQueriesContext +from django.utils.deprecation import RemovedInDjango51Warning from .models import CustomJSONDecoder, JSONModel, NullableJSONModel, RelatedJSONModel @@ -191,15 +193,40 @@ def test_null(self): obj.refresh_from_db() self.assertIsNone(obj.value) + def test_ambiguous_str_value_deprecation(self): + msg = ( + "Providing an encoded JSON string via Value() is deprecated. Use Value([], " + "output_field=JSONField()) instead." + ) + with self.assertWarnsMessage(RemovedInDjango51Warning, msg): + obj = NullableJSONModel.objects.create(value=Value("[]")) + obj.refresh_from_db() + self.assertEqual(obj.value, []) + + @skipUnlessDBFeature("supports_primitives_in_json_field") + def test_value_str_primitives_deprecation(self): + msg = ( + "Providing an encoded JSON string via Value() is deprecated. Use " + "Value(None, output_field=JSONField()) instead." + ) + with self.assertWarnsMessage(RemovedInDjango51Warning, msg): + obj = NullableJSONModel.objects.create(value=Value("null")) + obj.refresh_from_db() + self.assertIsNone(obj.value) + obj = NullableJSONModel.objects.create(value=Value("invalid-json")) + obj.refresh_from_db() + self.assertEqual(obj.value, "invalid-json") + @skipUnlessDBFeature("supports_primitives_in_json_field") def test_json_null_different_from_sql_null(self): - json_null = NullableJSONModel.objects.create(value=Value("null")) + json_null = NullableJSONModel.objects.create(value=Value(None, JSONField())) + NullableJSONModel.objects.update(value=Value(None, JSONField())) json_null.refresh_from_db() sql_null = NullableJSONModel.objects.create(value=None) sql_null.refresh_from_db() # 'null' is not equal to NULL in the database. self.assertSequenceEqual( - NullableJSONModel.objects.filter(value=Value("null")), + NullableJSONModel.objects.filter(value=Value(None, JSONField())), [json_null], ) self.assertSequenceEqual( diff --git a/tests/postgres_tests/test_aggregates.py b/tests/postgres_tests/test_aggregates.py index 629493b78fe3..183cff10c9f0 100644 --- a/tests/postgres_tests/test_aggregates.py +++ b/tests/postgres_tests/test_aggregates.py @@ -4,6 +4,7 @@ F, Func, IntegerField, + JSONField, OuterRef, Q, Subquery, @@ -15,7 +16,7 @@ from django.test import skipUnlessDBFeature from django.test.utils import Approximate, ignore_warnings from django.utils import timezone -from django.utils.deprecation import RemovedInDjango50Warning +from django.utils.deprecation import RemovedInDjango50Warning, RemovedInDjango51Warning from . import PostgreSQLTestCase from .models import AggregateTestModel, HotelReservation, Room, StatTestModel @@ -124,7 +125,11 @@ def test_default_argument(self): (BitOr("integer_field", default=0), 0), (BoolAnd("boolean_field", default=False), False), (BoolOr("boolean_field", default=False), False), - (JSONBAgg("integer_field", default=Value('[""]')), [""]), + (JSONBAgg("integer_field", default=[""]), [""]), + ( + JSONBAgg("integer_field", default=Value([""], JSONField())), + [""], + ), (StringAgg("char_field", delimiter=";", default=""), ""), ( StringAgg("char_field", delimiter=";", default=Value("")), @@ -189,9 +194,7 @@ def test_convert_value_deprecation(self): {"aggregation": []}, ) self.assertEqual( - queryset.aggregate( - aggregation=JSONBAgg("integer_field", default=Value("[]")) - ), + queryset.aggregate(aggregation=JSONBAgg("integer_field", default=[])), {"aggregation": []}, ) self.assertEqual( @@ -201,6 +204,59 @@ def test_convert_value_deprecation(self): {"aggregation": ""}, ) + @ignore_warnings(category=RemovedInDjango51Warning) + def test_jsonb_agg_default_str_value(self): + AggregateTestModel.objects.all().delete() + queryset = AggregateTestModel.objects.all() + self.assertEqual( + queryset.aggregate( + aggregation=JSONBAgg("integer_field", default=Value("")) + ), + {"aggregation": ""}, + ) + + def test_jsonb_agg_default_str_value_deprecation(self): + queryset = AggregateTestModel.objects.all() + msg = ( + "Passing a Value() with an output_field that isn't a JSONField as " + "JSONBAgg(default) is deprecated. Pass default=Value('', " + "output_field=JSONField()) instead." + ) + with self.assertWarnsMessage(RemovedInDjango51Warning, msg): + queryset.aggregate( + aggregation=JSONBAgg("integer_field", default=Value("")) + ) + with self.assertWarnsMessage(RemovedInDjango51Warning, msg): + queryset.none().aggregate( + aggregation=JSONBAgg("integer_field", default=Value("")) + ), + + @ignore_warnings(category=RemovedInDjango51Warning) + def test_jsonb_agg_default_encoded_json_string(self): + AggregateTestModel.objects.all().delete() + queryset = AggregateTestModel.objects.all() + self.assertEqual( + queryset.aggregate( + aggregation=JSONBAgg("integer_field", default=Value("[]")) + ), + {"aggregation": []}, + ) + + def test_jsonb_agg_default_encoded_json_string_deprecation(self): + queryset = AggregateTestModel.objects.all() + msg = ( + "Passing an encoded JSON string as JSONBAgg(default) is deprecated. Pass " + "default=[] instead." + ) + with self.assertWarnsMessage(RemovedInDjango51Warning, msg): + queryset.aggregate( + aggregation=JSONBAgg("integer_field", default=Value("[]")) + ) + with self.assertWarnsMessage(RemovedInDjango51Warning, msg): + queryset.none().aggregate( + aggregation=JSONBAgg("integer_field", default=Value("[]")) + ) + def test_array_agg_charfield(self): values = AggregateTestModel.objects.aggregate(arrayagg=ArrayAgg("char_field")) self.assertEqual(values, {"arrayagg": ["Foo1", "Foo2", "Foo4", "Foo3"]})