Opened 4 years ago

Closed 4 years ago

Last modified 4 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 by Igor Jerosimić, 4 years ago

Description: modified (diff)
Keywords: KeyTransform ArrayAgg added

comment:2 by Simon Charette, 4 years ago

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

comment:3 by Simon Charette, 4 years ago

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

comment:4 by Mariusz Felisiak, 4 years ago

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

comment:5 by Mariusz Felisiak, 4 years ago

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

comment:6 by Carlton Gibson, 4 years ago

Triage Stage: AcceptedReady for checkin

comment:7 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

In 7bfdd3b9:

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

comment:8 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

In 1d650ad0:

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

comment:9 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

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 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

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 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

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 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

In ee0abac1:

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

Regression in 6789ded0a6ab797f0dcdfa6ad5d1cfa46e23abcd.

comment:13 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

In 735c88fd:

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

Backport of 1d650ad019c1ab8e73d1e5b2587bb232c8ab35b6 from master

comment:14 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

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 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

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 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

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 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

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 by Mariusz Felisiak, 4 years ago

Resolution: fixed
Status: assignedclosed

comment:19 by GitHub <noreply@…>, 4 years ago

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 by GitHub <noreply@…>, 4 years ago

In 1fb97fb:

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

Thanks Simon Charette for the implementation idea.

comment:21 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

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