I didn't found an existing Ticket and I think the patch is small enough to avoid one. Please let me know otherwise.

All bugfixes require an accepted ticket. Is it a regression in Django 5.0?

Looks like a regression in 59f4754.

I didn't found an existing Ticket and I think the patch is small enough to avoid one. Please let me know otherwise.

All bugfixes require an accepted ticket. Is it a regression in Django 5.0?

All good, I will create one. Yes, it is a regression introduced by DJango 5.0, probably introduced by d660cee and subsequent improvements in afc8805 and 59f4754

I haven't checked the branch locally but I'm curious what the original crash was given QuerySet is also resolvable to sql.Query?

Without this patch, it crashes with:

  File "/django/django/db/models/sql/query.py", line 108, in get_child_with_renamed_prefix
    child = child.copy()
AttributeError: 'QuerySet' object has no attribute 'copy'

I haven't checked the branch locally but I'm curious what the original crash was given QuerySet is also resolvable to sql.Query?

My understanding is that the QuerySet is interpreted as an Expression because it has a resolve_expression method.

...
elif hasattr(child, "resolve_expression"):
...

but the nature of the QuerySet doesn't allow it to be treated like an Expression. Hence the elif isintance(child, QuerySet) is added, to leave the QuerySet as it is (as it was, in previous versions of Django).

My understanding is that the QuerySet is interpreted as an Expression because it has a resolve_expression method.

The presence of resolve_expression is exactly what defines the expression protocol. There are other objects that are not BaseExpression subclass that implement this protocol.

but the nature of the QuerySet doesn't allow it to be treated like an Expression.

It's treated as an expression in any other context so I don't think your assessment is right. I think that the usage of copy is problematic here.

To me the problem is more than QuerySet doesn't have a copy method but _clone should do.

Do the tests pass with

elif hasattr(child, "resolve_expression"):
    if isinstance(child, QuerySet):
        child = child._clone()
    else:
        child = child.copy()
    ...

My understanding is that the QuerySet is interpreted as an Expression because it has a resolve_expression method.

The presence of resolve_expression is exactly what defines the expression protocol. There are other objects that are not BaseExpression subclass that implement this protocol.

but the nature of the QuerySet doesn't allow it to be treated like an Expression.

It's treated as an expression in any other context so I don't think your assessment is right. I think that the usage of copy is problematic here.

To me the problem is more than QuerySet doesn't have a copy method but _clone should do.

Do the tests pass with

elif hasattr(child, "resolve_expression"):
    if isinstance(child, QuerySet):
        child = child._clone()
    else:
        child = child.copy()
    ...

We end up with another missing method expected by the Expression's API.

AttributeError: 'QuerySet' object has no attribute 'set_source_expressions'. Did you mean: 'resolve_expression'?

The problem is that QuerySet has very specialized expression resolving logic as its source expressions are stored in multiple different attributes (annotations, where) and can be composed of other querysets as well that themselves would need to have their references repointed (union, intersection).

That brings the question what exactly should be done here.

1. Don't support querysets
2. Allow support for queryets but don't support OuterRef or reference to the filtered expression itself (as that requires complex replacement logic)
3. Add full support for querysets

1. is the current state of things albeit with an ugly error, 2. is what's proposed here with the pass approach without any warning about the lack of support for OuterRef and references to filtered relations (which would be very hard to implement right), 3. is a large project that has likely minimal benefit given how rare joining using a subquery condition is.

My take is that we should either go with 1. and raise a more appropriate error (it can always be worked around by materializing the queryset by users by doing list(qs.values("pk"))) or 2. and emit a warning of some sort.

I have to admit I have a slight preference for 1. because of the can of worms 2. opens with regards to supporting some querysets and not others as whether a subquery is supported or not is not something that can be easily determined at condition replacement time.

Maybe we can try testing out what the message emitted when making use of OuterRef is and see if we believe it is appropriate?

Thanks for the clarifications. I agree with your strategy. I'll give it a try to option 2 first, though. As it's already not supported, I don't expect many complains will come out of this option.

For the record this is what the orm would produce if we would let QuerySet passed as it is.

qs = Author.objects.annotate(
    poem_book=FilteredRelation(
        "book",
        condition=Q(
            book__in=Book.objects.filter(title=OuterRef("book__title"))
        ),
    ),      
).filter(poem_book__isnull=False)

Outputs

SELECT "filtered_relation_author"."id", "filtered_relation_author"."name", "filtered_relation_author"."content_type_id", "filtered_relation_author"."object_id"
FROM "filtered_relation_author
LEFT OUTER JOIN "filtered_relation_book" T3 ON ("filtered_relation_author"."id" = T3."author_id")
INNER JOIN "filtered_relation_book" poem_book ON ("filtered_relation_author"."id" = poem_book."author_id" AND (poem_book."id" IN (SELECT U0."id" FROM "filtered_relation_book" U0 WHERE U0."title" = (T3."title"))))
WHERE poem_book."id" IS NOT NULL

Which is clearly not what we want, and letting it passed, silently, would be damaging to the users.
Good call @charettes

I'm still continuing to make progress on option 2. to see if it seems applicable. stay tuned.

I'd prefer not to support querysets at all. Especially since it's intended for backporting.

I changed the direction of the PR to prevent passing a QuerySet within a FilteredRelation.

@ticosax Thanks 👍

I pushed edits to the release note and comment.

@charettes Does it work for you?

@felixxm it does, thanks for the patch @ticosax!

I wonder if we should suggest that users materialize the queryset to workaround the problem in the error message. e.g. list(Book.objects.filter(title__istartswith="a").values_list("id", flat=True))). Maybe that's something the rare users who use subquery in this case will be able to figure out on their own?

@felixxm it does, thanks for the patch @ticosax!

I wonder if we should suggest that users materialize the queryset to workaround the problem in the error message. e.g. list(Book.objects.filter(title__istartswith="a").values_list("id", flat=True))). Maybe that's something the rare users who use subquery in this case will be able to figure out on their own?

Thanks for checking 👍 I think it's fine without a hint.