Context Navigation

← Previous Ticket
Next Ticket →

#32096 closed Bug (fixed)

Using KeyTransform for JSONField produces invalid SQL in various places.

Reported by:	Igor Jerosimić	Owned by:	Mariusz Felisiak
Component:	Database layer (models, ORM)	Version:	3.1
Severity:	Release blocker	Keywords:	KeyTransform, ArrayAgg
Cc:	Sage Abdullah	Triage Stage:	Ready for checkin
Has patch:	yes	Needs documentation:	no
Needs tests:	no	Patch needs improvement:	no
Easy pickings:	no	UI/UX:	no

Description (last modified by Igor Jerosimić)

Using KeyTransform in ordering attribute of ArrayAgg function produces invalid SQL. I don't know if it matters but I'm using Postgres for DB.

# sample model
from django.db import models

class Parent(models.Model):
    name = models.CharField(default='test')

class Child(models.Model):
    parent = models.ForeignKey(
        Parent,
        on_delete=models.SET_NULL,
        related_name='children',
    )
    data = models.JSONField(default=dict)


# sample data
parent = Parent.objects.create()
Child.objects.create(parent=parent, data={'en': 'English', 'fr': 'French'})


# error
Parent.objects.annotate(
        children_array=ArrayAgg(
                KeyTextTransform('en', 'children__data'),
                distinct=True,
                ordering=[KeyTransform('en', 'children__data')],
        ),
).all()

Produces invalid SQL in the ORDER BY section:

ARRAY_AGG(DISTINCT ("children"."data" ->> 'default') ORDER BY None("children"."data"))

NOTE: This was working fine before Django 3.1.

Change History (21)

comment:1 by Igor Jerosimić, 5 years ago

Description:	modified (diff)
Keywords:	KeyTransform ArrayAgg added

comment:2 by Simon Charette, 5 years ago

Severity:	Normal → Release blocker
Triage Stage:	Unreviewed → Accepted

It's due to OrderableAggMixin.as_sql calling its ordering expression as_sql method directly instead of doing compiler.compile(expr) since the latter properly handles the vendor logic.

This wasn't an issue when KeyTransform was only implemented for contrib.postgres as it's implementation was not vendored but now it does.

There might other instances of this problem lying around in django.db.models.expressions and friends since it's the first time I've seen this issue manifest itself and I don't remember this is something we kept an eye for during code reviews.

Last edited 5 years ago by Simon Charette (previous) (diff)

comment:3 by Simon Charette, 5 years ago

From a quick grep I identified the following as_sql calls that suffered from the same class of isssue

ExpressionWrapper.as_sql
FieldGetDbPrepValueIterableMixin.resolve_expression_parameter
Window.as_sql for self.partition but it would need to allow compiler.comple to proxy **kwargs passing or simply create a copy of self.partition to assign it the template at __init__ time instead. The latter seems like the less intrusive method.

I identified these by going through all instances of [^)]\.as_sql so I might have missed a few.

Version 0, edited 5 years ago by Simon Charette (next)

comment:4 by Mariusz Felisiak, 5 years ago

Cc:	Sage Abdullah added
Owner:	changed from nobody to Mariusz Felisiak
Status:	new → assigned

comment:5 by Mariusz Felisiak, 5 years ago

Has patch:	set
Summary:	Using KeyTransform in ArrayAgg function produces invalid SQL → Using KeyTransform for JSONField produces invalid SQL in various places.