Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#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 Changed 3 years ago by Igor Jerosimić

Description: modified (diff)
Keywords: KeyTransform ArrayAgg added

comment:2 Changed 3 years ago by Simon Charette

Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted

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 3 years ago by Simon Charette (previous) (diff)

comment:3 Changed 3 years ago by Simon Charette

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

  1. ExpressionWrapper.as_sql
  2. FieldGetDbPrepValueIterableMixin.resolve_expression_parameter
  3. Window.as_sql for self.partition but we'd 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.

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

comment:4 Changed 3 years ago by Mariusz Felisiak

Cc: Sage Abdullah added
Owner: changed from nobody to Mariusz Felisiak
Status: newassigned

comment:5 Changed 3 years ago by Mariusz Felisiak

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

comment:6 Changed 3 years ago by Carlton Gibson

Triage Stage: AcceptedReady for checkin

comment:7 Changed 3 years ago by Mariusz Felisiak <felisiak.mariusz@…>

In 7bfdd3b9:

Refs #32096 -- Added test for window expressions with JSONField key transforms.

comment:8 Changed 3 years ago by Mariusz Felisiak <felisiak.mariusz@…>

In 1d650ad0:

Refs #32096 -- Added test for ArrayAgg over JSONField key transforms.

comment:9 Changed 3 years ago by Mariusz Felisiak <felisiak.mariusz@…>

In 1f31027:

Refs #32096 -- Fixed crash of ArrayAgg/StringAgg/JSONBAgg with ordering over JSONField key transforms.

Regression in 6789ded0a6ab797f0dcdfa6ad5d1cfa46e23abcd.

Thanks Igor Jerosimić for the report.

comment:10 Changed 3 years ago by Mariusz Felisiak <felisiak.mariusz@…>

In 7e1e1984:

Refs #32096 -- Fixed in lookup crash against key transforms for JSONField.

Regression in 6789ded0a6ab797f0dcdfa6ad5d1cfa46e23abcd and
1251772cb83aa4106f526fe00738e51c0eb59122.

Thanks Simon Charette and Igor Jerosimić for the report.

comment:11 Changed 3 years ago by Mariusz Felisiak <felisiak.mariusz@…>

In bbd55e5:

Refs #32096 -- Fixed ExpressionWrapper crash with JSONField key transforms.

Regression in 6789ded0a6ab797f0dcdfa6ad5d1cfa46e23abcd.

Thanks Simon Charette and Igor Jerosimić for the report.

comment:12 Changed 3 years ago by Mariusz Felisiak <felisiak.mariusz@…>

In ee0abac1:

Refs #32096 -- Fixed ExclusionConstraint crash with JSONField key transforms in expressions.

Regression in 6789ded0a6ab797f0dcdfa6ad5d1cfa46e23abcd.

comment:13 Changed 3 years ago by Mariusz Felisiak <felisiak.mariusz@…>

In 735c88fd:

[3.1.x] Refs #32096 -- Added test for ArrayAgg over JSONField key transforms.

Backport of 1d650ad019c1ab8e73d1e5b2587bb232c8ab35b6 from master

comment:14 Changed 3 years ago by Mariusz Felisiak <felisiak.mariusz@…>

In d94e777b:

[3.1.x] Refs #32096 -- Fixed crash of ArrayAgg/StringAgg/JSONBAgg with ordering over JSONField key transforms.

Regression in 6789ded0a6ab797f0dcdfa6ad5d1cfa46e23abcd.

Thanks Igor Jerosimić for the report.

Backport of 1f31027bb3ad460864fbcbbb89eeb328c0a2f184 from master

comment:15 Changed 3 years ago by Mariusz Felisiak <felisiak.mariusz@…>

In 59fe0b85:

[3.1.x] Refs #32096 -- Fixed in lookup crash against key transforms for JSONField.

Regression in 6789ded0a6ab797f0dcdfa6ad5d1cfa46e23abcd and
1251772cb83aa4106f526fe00738e51c0eb59122.

Thanks Simon Charette and Igor Jerosimić for the report.

Backport of 7e1e198494d4fc72cf6e153f9d24fe2493c17dc1 from master

comment:16 Changed 3 years ago by Mariusz Felisiak <felisiak.mariusz@…>

In ae6b240:

[3.1.x] Refs #32096 -- Fixed ExpressionWrapper crash with JSONField key transforms.

Regression in 6789ded0a6ab797f0dcdfa6ad5d1cfa46e23abcd.

Thanks Simon Charette and Igor Jerosimić for the report.

Backport of bbd55e58639c33b4c5adff5f41b78deffc915c11 from master

comment:17 Changed 3 years ago by Mariusz Felisiak <felisiak.mariusz@…>

In 53621327:

[3.1.x] Refs #32096 -- Fixed ExclusionConstraint crash with JSONField key transforms in expressions.

Regression in 6789ded0a6ab797f0dcdfa6ad5d1cfa46e23abcd.

Backport of ee0abac169c2dcc6818d583247903c2a8ef55f7c from master.

comment:18 Changed 3 years ago by Mariusz Felisiak

Resolution: fixed
Status: assignedclosed

comment:19 Changed 3 years ago by GitHub <noreply@…>

In 220c4d5c:

Refs #32096 -- Removed JSONBAgg from 3.1.3 release notes.

JSONBAgg doesn't support ordering in Django 3.1.

Follow up to 1f31027bb3ad460864fbcbbb89eeb328c0a2f184.

comment:20 Changed 3 years ago by GitHub <noreply@…>

In 1fb97fb:

Refs #32096 -- Made JSONField check respect Meta.required_db_vendor.

Thanks Simon Charette for the implementation idea.

comment:21 Changed 3 years ago by Mariusz Felisiak <felisiak.mariusz@…>

In be3ce386:

[3.1.x] Refs #32096 -- Made JSONField check respect Meta.required_db_vendor.

Thanks Simon Charette for the implementation idea.
Backport of 1fb97fb965786a4398a2236e4e619b78a25d5875 from master

Note: See TracTickets for help on using tickets.
Back to Top