#33766 closed Bug (fixed)
FilteredRelation resolves its conditions too late which can result in unknown alias references at SQL compilation time
Reported by: | Daniel Schaffer | Owned by: | Simon Charette |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | 3.2 |
Severity: | Normal | Keywords: | filteredrelation coalesce |
Cc: | Sarah Boyce, Francesco Panico | 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 )
When using the Coalesce
function as part of the condition of a FilteredRelation
, the query fails with an "Unknown column" error if any of the fields referenced by Coalesce
requires a JOIN. This appears to be due to the JOIN not actually getting added to the query.
job_worker_preference=FilteredRelation( relation_name="company__worker_preferences", condition=Q( company__worker_preferences__worker=Coalesce(F("worker"), F("worker_substitutions__worker")), company__worker_preferences__company=F("company"), ) ), is_allowed=Case(When(job_worker_preference__allow_assignments=True, then=1), default=0, output_field=BooleanField())
This can be worked around by creating a separate annotation for the result of the Coalesce
function:
actual_worker=Coalesce(F("worker"), F("worker_substitutions__worker")), job_worker_preference=FilteredRelation( relation_name="company__worker_preferences", condition=Q( company__worker_preferences__worker=F("actual_worker"), company__worker_preferences__company=F("company"), ) ), is_allowed=Case(When(job_worker_preference__allow_assignments=True, then=1), default=0, output_field=BooleanField())
However, I suspect there may be an underlying issue with how JOINs are detected and added to a query when there are nested field references like this.
I've reproduced the issue in this repro: https://github.com/DanielSchaffer/django_filtered_relation_coalesce_repro.
django_filtered_relation_coalesce_repro/test/test_job_manager.py
contains a failing test that reproduces the issue, and a passing test that demonstrates the workaround.
Here's the stringified representation of the query - note the missing JOIN
to django_filtered_relation_coalesce_repro_workersubstitution
, even though it's referenced in the COALESCE
expression:
SELECT `django_filtered_relation_coalesce_repro_job`.`id`, `django_filtered_relation_coalesce_repro_job`.`company_id`, `django_filtered_relation_coalesce_repro_job`.`worker_id`, CASE WHEN job_worker_preference.`allow_assignments` THEN 1 ELSE 0 END AS `is_allowed` FROM `django_filtered_relation_coalesce_repro_job` INNER JOIN `django_filtered_relation_coalesce_repro_company` ON (`django_filtered_relation_coalesce_repro_job`.`company_id` = `django_filtered_relation_coalesce_repro_company`.`id`) LEFT OUTER JOIN `django_filtered_relation_coalesce_repro_workerpreference` job_worker_preference ON (`django_filtered_relation_coalesce_repro_company`.`id` = job_worker_preference.`company_id` AND ((job_worker_preference.`company_id` = `django_filtered_relation_coalesce_repro_job`.`company_id` AND job_worker_preference.`worker_id` = COALESCE(`django_filtered_relation_coalesce_repro_job`.`worker_id`, `django_filtered_relation_coalesce_repro_workersubstitution`.`worker_id`))))
Change History (19)
comment:1 by , 2 years ago
comment:2 by , 2 years ago
Description: | modified (diff) |
---|
Sure, I've updated the ticket with a link to a repro repo
comment:3 by , 2 years ago
Description: | modified (diff) |
---|
comment:4 by , 2 years ago
Description: | modified (diff) |
---|
comment:5 by , 2 years ago
Summary: | "Unknown column" error due to missing JOIN when using Coalesce as part of a FilteredRelation's condition → FilteredRelation doesn't resolve its conditions resulting in unknown alias references at SQL compilation time |
---|---|
Triage Stage: | Unreviewed → Accepted |
FilteredRelation
annotations are implemented in a somewhat awkward way in the sense that they are accepted by Queryset.annotate
even they are not truly resolvable.
From my understanding the reason behind that is to somewhat mimic what Queryset.alias
does for annotations to avoid incurring an extraneous JOIN
if the filtered relation end up not being referenced in the final queryset composition.
This is achieved by having Queryset.annotate
take an entirely different route when provided a FilteredRelation
which defers expression resolving by stashing the instance and its alias in Query._filtered_relations
. This stash is then later used to create an actual filtered join when Query.names_to_path
detects a reference to a previously stashed name in ._filtered_relations
.
What the actual implementation of FilteredRelation
failed to account for when making the latter rely on a special JIT resolving instead of using the usual resolve_expression
path is that .conditions
right-hand-sides are allowed to span joins (as in the reporter's case here with F("worker_substitutions__worker")
) and thus must be resolved before the query is passed to the compiler. This is problematic because the .condition
of FilteredRelation
is resolved via the SQLCompiler.compile -> .get_from_clause -> Join.as_sql -> FilteredRelation.as_sql
chain but at the time FilteredRelation.as_sql
calls compiler.query.build_filtered_relation_q
, which happens to augment query.alias_map
with the proper joins associated with the right-hand-sides, it's already too late because the list of aliases to be considered for compilation in get_from_clause
is already fixed.
I don't have all the details at hand here but I wonder why we chose to go this way instead of immediately creating the alias_map
entry with a corresponding alias_refcount
of 0
which would have resulted in the joins being referenceable but eventually pruned if unused. We have the machinery in place to track dependency chains between joins so it would only have been a matter of incrementing the transitive chain of joins in names_to_path
when the filtered relation was referenced and we should have been set.
I didn't have time to commit in exploring this route, which seems like a good way of cleaning all the filtered relation logic, but here's at least a small patch reproduces the issue in our current test suite.
-
tests/filtered_relation/tests.py
diff --git a/tests/filtered_relation/tests.py b/tests/filtered_relation/tests.py index 7d77e31b51..825d150830 100644
a b def test_eq(self): 685 685 FilteredRelation("book", condition=Q(book__title="b")), mock.ANY 686 686 ) 687 687 688 def test_condition_spans_join(self): 689 self.assertSequenceEqual( 690 Book.objects.annotate( 691 contains_editor_author=FilteredRelation( 692 "author", condition=Q(author__name__contains=F("editor__name")) 693 ) 694 ) 695 .filter( 696 contains_editor_author__isnull=False, 697 ) 698 .order_by("id"), 699 [cls.book1, cls.book4], 700 ) 701 688 702 689 703 class FilteredRelationAggregationTests(TestCase): 690 704 @classmethod
comment:6 by , 2 years ago
Summary: | FilteredRelation doesn't resolve its conditions resulting in unknown alias references at SQL compilation time → FilteredRelation resolves its conditions too late which can result in unknown alias references at SQL compilation time |
---|
comment:7 by , 2 years ago
Thanks for that explanation Simon - I wouldn't have guessed this was quite that complicated an issue, but that all makes sense (and an interesting read to boot!).
comment:8 by , 2 years ago
I have a similar issue with FilteredRelation and joins and traced it back to a django update from 4.0.3 to 4.0.6.
My guess is that this specific issue was introduced in https://code.djangoproject.com/ticket/33598
The provided workaround helped me a lot. Thanks!
comment:9 by , 2 years ago
comment:10 by , 23 months ago
comment:11 by , 21 months ago
The FilteredRelation cannot resolve itself too.
def test_ref_self_in_condition(self): self.assertSequenceEqual( Author.objects.annotate( the_book=FilteredRelation( "book", condition=Q( name=F('book__title'), ), ), ) .filter( the_book__isnull=False, ), [], )
comment:12 by , 19 months ago
Cc: | added |
---|
comment:13 by , 19 months ago
Could this bug be resolved after fixing 34362? In the PR I encountered exactly this error
comment:14 by , 19 months ago
Cc: | added |
---|
comment:15 by , 19 months ago
Has patch: | set |
---|---|
Owner: | changed from | to
Status: | new → assigned |
Francesco, it's the other way around, fixing this bug is required to address #34362.
comment:16 by , 19 months ago
Triage Stage: | Accepted → Ready for checkin |
---|
Thank you for your report, please provide a minimal set of models that reproduce the issue and the resulting traceback and query if possible.
That'll make the job of volunteers trying to validate your report way easier.