#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:
- Subclassing
django.db.models.sql.Query
and using it in a Queryset mixin - Creating a queryset with an
exclude()
expression which includes a subquery. EGClassroom.objects.exclude(student_set__id=student_A.id)
- Then before calling
super().get_compiler
from our subclassed Query:- Clone the query
- Adding a simple expression via
cloned_query.add_q
egcloned_query.add_q(Q(school_id=1))
- Passing the cloned query into
super().get_compiler
- 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 , 5 weeks ago
Cc: | added |
---|
comment:2 by , 5 weeks ago
Resolution: | → invalid |
---|---|
Status: | new → closed |
comment:3 by , 4 weeks 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 , 4 weeks ago
You might want to use the forums for that as they are more active nowadays.
comment:5 by , 4 weeks ago
Thanks for your detailed answer Simon. We'll followup on the forums as well.
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 aQuerySet
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.