Opened 2 years ago

Closed 19 months ago

Last modified 19 months ago

#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 Daniel Schaffer)

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 Simon Charette, 2 years ago

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.

comment:2 by Daniel Schaffer, 2 years ago

Description: modified (diff)

Sure, I've updated the ticket with a link to a repro repo

comment:3 by Daniel Schaffer, 2 years ago

Description: modified (diff)

comment:4 by Daniel Schaffer, 2 years ago

Description: modified (diff)

comment:5 by Simon Charette, 2 years ago

Summary: "Unknown column" error due to missing JOIN when using Coalesce as part of a FilteredRelation's conditionFilteredRelation doesn't resolve its conditions resulting in unknown alias references at SQL compilation time
Triage Stage: UnreviewedAccepted

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):  
    685685            FilteredRelation("book", condition=Q(book__title="b")), mock.ANY
    686686        )
    687687
     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
    688702
    689703class FilteredRelationAggregationTests(TestCase):
    690704    @classmethod

comment:6 by Simon Charette, 2 years ago

Summary: FilteredRelation doesn't resolve its conditions resulting in unknown alias references at SQL compilation timeFilteredRelation resolves its conditions too late which can result in unknown alias references at SQL compilation time

comment:7 by Daniel Schaffer, 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 Martin Schmidt, 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 Mariusz Felisiak, 2 years ago

#33929 was closed as a duplicate. We should add tests for #33929 when resolving this issue.

comment:10 by Mariusz Felisiak, 23 months ago

#34229 was closed as a duplicate. We should add tests for #34229 when resolving this issue.

comment:11 by zhu, 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 Sarah Boyce, 19 months ago

Cc: Sarah Boyce added

comment:13 by Francesco Panico, 19 months ago

Could this bug be resolved after fixing 34362? In the PR I encountered exactly this error

comment:14 by Francesco Panico, 19 months ago

Cc: Francesco Panico added

comment:15 by Simon Charette, 19 months ago

Has patch: set
Owner: changed from nobody to Simon Charette
Status: newassigned

Francesco, it's the other way around, fixing this bug is required to address #34362.

comment:16 by Mariusz Felisiak, 19 months ago

Triage Stage: AcceptedReady for checkin

comment:17 by Mariusz Felisiak <felisiak.mariusz@…>, 19 months ago

Resolution: fixed
Status: assignedclosed

In d660cee5:

Fixed #33766 -- Resolved FilteredRelation.condition at referencing time.

The previous implementation resolved condition at Join compilation time
which required introducing a specialized expression resolving mode to
alter the join reuse logic solely during that phase.

FilteredRelation.condition is now resolved when the relation is first
referenced which maintains the existing behavior while allowing the
removal of the specialized resolving mode and address an issue where
conditions couldn't spawn new joins.

comment:18 by Mariusz Felisiak <felisiak.mariusz@…>, 19 months ago

In 1a13161:

Refs #33766 -- Removed unused Join.equals().

It's unused now that the specialized FilteredRelation.as_sql logic is
no more.

comment:19 by Mariusz Felisiak <felisiak.mariusz@…>, 19 months ago

In 83c9765:

Refs #33766 -- Removed sql.Query.build_filtered_relation_q().

It was a copy of sql.Query._add_q that avoided JOIN updates.

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