Opened 2 months ago

Closed 2 months ago

Last modified 2 months ago

#35836 closed Bug (invalid)

ORM can produce invalid SQL when subclassing Query

Reported by: Ben Pearman Owned by:
Component: Database layer (models, ORM) Version: 5.0
Severity: Normal Keywords:
Cc: Ben Pearman, Craig de Stigter Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

We have observed a bug in the ORM where invalid SQL can be produced.

A reproduction can be seen here:
https://github.com/benpearman/django-orm-bug
And specifically:
https://github.com/benpearman/django-orm-bug/blob/master/demo/models.py

It is caused with the following steps:

  1. Subclassing django.db.models.sql.Query and using it in a Queryset mixin
  2. Creating a queryset with an exclude() expression which includes a subquery. EG Classroom.objects.exclude(student_set__id=student_A.id)
  3. Then before calling super().get_compiler from our subclassed Query:
    • Clone the query
    • Adding a simple expression via cloned_query.add_q eg cloned_query.add_q(Q(school_id=1))
    • Passing the cloned query into super().get_compiler
  4. Then evaluate the queryset, which produces invalid SQL and raises django.db.utils.OperationalError

Note the issue happens when get_compiler is invoked for the subquery, not on the main query itself.

This was working for us in Django 3.2, but not for Django 4.1

git bisect shows the breakage was introduced by this commit:
https://github.com/django/django/commit/14c8504a37afad96ab93cf82f47b13bcc4d00621

Change History (5)

comment:1 by Craig de Stigter, 2 months ago

Cc: Craig de Stigter added

comment:2 by Simon Charette, 2 months ago

Resolution: invalid
Status: newclosed

Django 3.2 has been EOL for 6 months now and while we accept patch to allow extension of the ORM we don't offer any guarantees towards backward compatibility around sql.Query API changes.

In other words, if you extend undocumented and unstable part of the framework you must be prepared to adapt your code and provide a solution that goes beyond bisecting the origin of the problem you are experiencing.

In the case of 14c8504a37afad96ab93cf82f47b13bcc4d00621 I think it's a sane change in towards extensibility and something we'd want to keep in main. SQLQuery.get_compiler is by no means meant to be used as a hook to augment the where clause of a QuerySet like you did; this should be done at the manager level or at the queryset level at worst.

At this point, even if we were to land a solution that you'd have to provide and build a rationale for it wouldn't make it to a public release until 5.2 which is meant to be released in April 2025. If your solution runs into problems when dealing with subqueries and you want to double down on this approach you should be able to identify the proper attributes to used specialized logic.

Version 2, edited 2 months ago by Simon Charette (previous) (next) (diff)

comment:3 by Craig de Stigter, 2 months ago

Thanks, suspected as much.

We'll follow up with a django-users post about how to implement something to support our use-case. (as far as we can tell, there isn't any way to do it using the supported APIs)

comment:4 by Simon Charette, 2 months ago

You might want to use the forums for that as they are more active nowadays.

comment:5 by Ben Pearman, 2 months ago

Thanks for your detailed answer Simon. We'll followup on the forums as well.

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