Skip to content

Commit

Permalink
Fixed #31956 -- Fixed crash of ordering by JSONField with a custom de…
Browse files Browse the repository at this point in the history
…coder on PostgreSQL.

Thanks Marc Debureaux for the report.
Thanks Simon Charette, Nick Pope, and Adam Johnson for reviews.
  • Loading branch information
felixxm committed Aug 28, 2020
1 parent 2210539 commit 0be51d2
Show file tree
Hide file tree
Showing 8 changed files with 21 additions and 27 deletions.
2 changes: 1 addition & 1 deletion django/contrib/postgres/aggregates/general.py
Expand Up @@ -48,7 +48,7 @@ class JSONBAgg(OrderableAggMixin, Aggregate):

def convert_value(self, value, expression, connection):
if not value:
return []
return '[]'
return value


Expand Down
7 changes: 0 additions & 7 deletions django/db/backends/base/operations.py
Expand Up @@ -153,13 +153,6 @@ def time_extract_sql(self, lookup_type, field_name):
"""
return self.date_extract_sql(lookup_type, field_name)

def json_cast_text_sql(self, field_name):
"""Return the SQL to cast a JSON value to text value."""
raise NotImplementedError(
'subclasses of BaseDatabaseOperations may require a '
'json_cast_text_sql() method'
)

def deferrable_sql(self):
"""
Return the SQL to make a constraint "initially deferred" during a
Expand Down
5 changes: 4 additions & 1 deletion django/db/backends/postgresql/base.py
Expand Up @@ -200,7 +200,10 @@ def get_new_connection(self, conn_params):
# Set the isolation level to the value from OPTIONS.
if self.isolation_level != connection.isolation_level:
connection.set_session(isolation_level=self.isolation_level)

# Register dummy loads() to avoid a round trip from psycopg2's decode
# to json.dumps() to json.loads(), when using a custom decoder in
# JSONField.
psycopg2.extras.register_default_jsonb(conn_or_curs=connection, loads=lambda x: x)
return connection

def ensure_timezone(self):
Expand Down
3 changes: 0 additions & 3 deletions django/db/backends/postgresql/operations.py
Expand Up @@ -74,9 +74,6 @@ def datetime_trunc_sql(self, lookup_type, field_name, tzname):
def time_trunc_sql(self, lookup_type, field_name):
return "DATE_TRUNC('%s', %s)::time" % (lookup_type, field_name)

def json_cast_text_sql(self, field_name):
return '(%s)::text' % field_name

def deferrable_sql(self):
return " DEFERRABLE INITIALLY DEFERRED"

Expand Down
10 changes: 0 additions & 10 deletions django/db/models/fields/json.py
Expand Up @@ -70,8 +70,6 @@ def deconstruct(self):
def from_db_value(self, value, expression, connection):
if value is None:
return value
if connection.features.has_native_json_field and self.decoder is None:

This comment has been minimized.

Copy link
@adamalton

adamalton Sep 1, 2020

Contributor

@felixxm just notifying you that I think this has caused a bug in 3.1.1. https://code.djangoproject.com/ticket/31973 Letting you know, as your prior knowledge of this code will probably be helpful in working out the fix! Adam

This comment has been minimized.

Copy link
@felixxm

felixxm Sep 1, 2020

Author Member

Thanks, let's discuss this in the ticket.

This comment has been minimized.

Copy link
@gdoermann

gdoermann Jan 27, 2023

Still a problem...

return value
try:
return json.loads(value, cls=self.decoder)
except json.JSONDecodeError:
Expand All @@ -91,14 +89,6 @@ def get_transform(self, name):
return transform
return KeyTransformFactory(name)

def select_format(self, compiler, sql, params):
if (
compiler.connection.features.has_native_json_field and
self.decoder is not None
):
return compiler.connection.ops.json_cast_text_sql(sql), params
return super().select_format(compiler, sql, params)

def validate(self, value, model_instance):
super().validate(value, model_instance)
try:
Expand Down
4 changes: 4 additions & 0 deletions docs/releases/3.1.1.txt
Expand Up @@ -51,3 +51,7 @@ Bugfixes

* Fixed detecting an async ``get_response`` callable in various builtin
middlewares (:ticket:`31928`).

* Fixed a ``QuerySet.order_by()`` crash on PostgreSQL when ordering and
grouping by :class:`~django.db.models.JSONField` with a custom
:attr:`~django.db.models.JSONField.decoder` (:ticket:`31956`).
5 changes: 0 additions & 5 deletions tests/backends/base/test_operations.py
Expand Up @@ -117,11 +117,6 @@ def test_datetime_extract_sql(self):
with self.assertRaisesMessage(NotImplementedError, self.may_require_msg % 'datetime_extract_sql'):
self.ops.datetime_extract_sql(None, None, None)

def test_json_cast_text_sql(self):
msg = self.may_require_msg % 'json_cast_text_sql'
with self.assertRaisesMessage(NotImplementedError, msg):
self.ops.json_cast_text_sql(None)


class DatabaseOperationTests(TestCase):
def setUp(self):
Expand Down
12 changes: 12 additions & 0 deletions tests/model_fields/test_jsonfield.py
Expand Up @@ -359,6 +359,18 @@ def test_ordering_grouping_by_count(self):
).values('value__d__0').annotate(count=Count('value__d__0')).order_by('count')
self.assertQuerysetEqual(qs, [1, 11], operator.itemgetter('count'))

def test_order_grouping_custom_decoder(self):
NullableJSONModel.objects.create(value_custom={'a': 'b'})
qs = NullableJSONModel.objects.filter(value_custom__isnull=False)
self.assertSequenceEqual(
qs.values(
'value_custom__a',
).annotate(
count=Count('id'),
).order_by('value_custom__a'),
[{'value_custom__a': 'b', 'count': 1}],
)

def test_key_transform_raw_expression(self):
expr = RawSQL(self.raw_sql, ['{"x": "bar"}'])
self.assertSequenceEqual(
Expand Down

0 comments on commit 0be51d2

Please sign in to comment.