Fixed #25534, Fixed #31639 -- Added support for transform references in expressions.

[LilyFoote]

By updating Query.resolve_ref to return the transform instead of the field, we enable F objects to include transforms.

This resolves ticket-25534 and ticket-31639.

[charettes]

I have a feeling this PR does more than solve ticket-25534 for the aggregation case.

This was a larger problem regarding all transform references as detailed in ticket-31639 which makes be believe this PR should be renamed Added support for transform references in expressions..

@Ian-Foote did you test this out on anything but the simple aggregation case? I think we should also test F and OuterRef support for transform resolution as detailed in ticket-31639 because these also go through resolve_ref and setup_joins.

[LilyFoote]

You're right, I think this does address ticket-31639 too. I actually saw that ticket first via twitter, which inspired me to see if this could be fixed instead of just raising an error as that ticket suggests, especially since I've had this itch myself before too.

[felixxm]

@Ian-Foote Thanks 👍 This is brilliant 💎 I left some comments.

We should update docs and add versionchanged annotations to:

• OuterRef(),
• F(), and
• Aggregation functions docs.

Can we add tests for index and slice transforms for ArrayField() and for key transforms for HStoreField()?

[felixxm]

Can we also add tests for other expressions and for chaining transforms?, e.g.

    def test_chaining_transforms(self):
        Author.objects.create(name=' John  ')
        Author.objects.create(name='Rhonda')
        with register_lookup(CharField, Trim), register_lookup(CharField, Length):
            for expr in [Length('name__trim'), F('name__trim__length')]:
                self.assertCountEqual(
                    Author.objects.annotate(length=expr).values('name', 'length'),
                    [{'name': ' John  ', 'length': 4}, {'name': 'Rhonda', 'length': 6}],
                )

[LilyFoote]

I don't entirely like this example - it's a bit more complicated than would be ideal.

I considered doing just:

    >>> Entry.objects.annotate(
            top_rating=Subquery(
                Entry.objects.filter(
                    pub_date__year=OuterRef('pub_date__year')
                ).order_by('-rating').values('rating')[:1]
            ),
        )

But I don't like that this leads to duplicate rows in the resulting queryset - we get one for each Entry sharing pub_date__year. The example with Sum consolidates these into just one row.

I'm not sure explaining these nuances is in scope here - I'd prefer an example that completely avoids them.

[LilyFoote]

Thinking about this a bit more - I think all my tests for this have been using transforms and I'm not sure the idea makes sense for lookups. Can someone think of an example where a lookup would be used, or should I drop mentions of lookups from the documentation?

[felixxm]

Thinking about this a bit more - I think all my tests for this have been using transforms and I'm not sure the idea makes sense for lookups. Can someone think of an example where a lookup would be used, or should I drop mentions of lookups from the documentation?

True, good catch 🎯, let's chop "lookups". transform_function() should even raise a FieldError for a lookup, see

django/django/db/models/sql/query.py

Lines 1750 to 1752 in c70cd2a

	# Verify that the last lookup in name is a field or a transform:
	# transform_function() raises FieldError if not.
	join_info.transform_function(targets[0], final_alias)

[felixxm]

@Ian-Foote Thanks for updates 👍 I pushed small edits to tests. I'm going to check docs later today.

[LilyFoote]

I've tweaked the first commit message to stop mentioning lookups and squashed the commits while I was at it.

[felixxm]

@Ian-Foote Thanks 👍 I added some tests for key transforms for JSONField() and one of them doesn't work, can you take a look? model_fields.test_jsonfield.TestQuerying.test_key_transform_annotation_expression.

[LilyFoote]

@felixxm That was a tricky one! It turns out that filtering with an F object with a transform based from an annotated field didn't work. The change I've pushed resolves the problem in this case, but there may be other scenarios we need to test.

I also tweaked the test so the filter actually served a purpose.

[felixxm]

@Ian-Foote Thanks for updates 👍 I found one more case that doesn't work:

        self.assertSequenceEqual(
            RelatedJSONModel.objects.annotate(
                key=F('value__d'),
                related=F('json_model'),
                chain=F('key__1'),
                expr=Cast('key', models.JSONField()),
            ).filter(chain=F('related__value__d__0')),
            [related_obj],
        )

it raises "django.core.exceptions.FieldError: Unsupported lookup 'value' for AutoField or join on the field not permitted.". It's probably unrelated because even without this patch a simple related annotation fails, e.g.:

RelatedJSONModel.objects.annotate(
    related=F('json_model'),
).annotate(chain=F('related__value')

raises "django.core.exceptions.FieldError: Cannot resolve keyword 'value' into field. Join on 'related' not permitted."

[LilyFoote]

I agree - I think fixing this one is a fair bit of work and I'd prefer to handle it separately.

[felixxm]

I agree - I think fixing this one is a fair bit of work and I'd prefer to handle it separately.

My mistake, this behavior is well documented: "When referencing relational fields such as ForeignKey, F() returns the primary key value rather than a model instance:" ☺️

I'm going to work on final edits.

[LilyFoote]

I think it's something that could be made to work, but I'm definitely happy to leave it out of this ticket/PR.

[felixxm]

@Ian-Foote I pushed edits to docs, and added tests for windows expressions, QuerySet.alias(), QuerySet.update(), F().bitor(), etc.

Unfortunately QuerySet.update() raises django.core.exceptions.FieldError: Joined field references are not permitted in this query, IMO that's the last 🐛 🤞

[felixxm]

I will try to remove:

django/django/db/models/sql/query.py

Lines 1717 to 1718 in e46ca51

	if not allow_joins and LOOKUP_SEP in name:
	raise FieldError("Joined field references are not permitted in this query")

it looks redundant and unnecessary (\cc @charettes).

[charettes]

Unfortunately QuerySet.update() raises django.core.exceptions.FieldError: Joined field references are not permitted in this query, IMO that's the last 🐛 🤞

Does this happen when resolving transforms as well? I know there's a check in place to prevent JOIN references in UPDATE because that's requires a subquery pushdown (ticket-25643, ticket-14104) but I guess this would need to be adjusted.

it looks redundant and unnecessary (\cc @charettes).

Yeah it looks redundant to me as well since we already check it when trying to resolve a non-annotation. Was it addressing the update issue?

[felixxm]

Unfortunately QuerySet.update() raises django.core.exceptions.FieldError: Joined field references are not permitted in this query, IMO that's the last 🐛 🤞

Does this happen when resolving transforms as well? I know there's a check in place to prevent JOIN references in UPDATE because that's requires a subquery pushdown (ticket-25643, ticket-14104) but I guess this would need to be adjusted.

it looks redundant and unnecessary (\cc @charettes).

Yeah it looks redundant to me as well since we already check it when trying to resolve a non-annotation. Was it addressing the update issue?

Yes, without causing any failures.