[Django] #31639: Incorrect result when using __year lookup in aggregation filter

[Django]

#31639: Incorrect result when using __year lookup in aggregation filter
-------------------------------------+-------------------------------------
Reporter: Baptiste | Owner: nobody
Mispelon |
Type: Bug | Status: new
Component: Database | Version: master
layer (models, ORM) |
Severity: Normal | Keywords:
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
Consider two models `Client` and `Bill` defined like so:
{{{#!python
from django.db import models

class Client(models.Model):
name = models.CharField(max_length=50)

class Bill(models.Model):
client = models.ForeignKey('Client', on_delete=models.CASCADE,
related_name='bills')
issued_on = models.DateField()
}}}

I was trying to get a `Bill` queryset where each bill is annotated with
the year of the previous bill (same client) and came up with the following
code which gave me incorrect results:
{{{#!python
from django.db.models import F, Max, Q
from django.db.models.functions import ExtractYear

filter_Q = Q(client__bills__issued_on__year__lt=F('issued_on__year'))
annotation = Max(ExtractYear('client__bills__issued_on'), filter=filter_Q)

# Returns wrong annotations (as if the filter=... was not there)
Bill.objects.annotate(previous_year=annotation)
}}}

However if I use `ExtractYear` instead of the `__year` lookup then I get
the correct results:
{{{#!python
filter_Q = Q(client__bills__issued_on__year__lt=ExtractYear('issued_on'))
}}}

I would assume that `F('issued_on__year')` should be strictly equivalent
to `ExtractYear('issued_on')` but somehow it's not the case here. I
believe that's a bug.

Here's a small testcase that summarizes the issue:
{{{#!python
from django.db.models import F, Max, Q
from django.db.models.functions import ExtractYear
from django.test import TestCase

from .models import Bill, Client

class ReproductionTestCase(TestCase):
@classmethod
def setUpTestData(cls):
c = Client.objects.create(name="Baptiste")
c.bills.create(issued_on='2020-01-01')
c.bills.create(issued_on='2019-01-01')
c.bills.create(issued_on='2019-07-01')
c.bills.create(issued_on='2018-01-01')

def test_extractyear(self):
filter_Q =
Q(client__bills__issued_on__year__lt=ExtractYear('issued_on'))
annotation = Max(ExtractYear('client__bills__issued_on'),
filter=filter_Q)

queryset =
Bill.objects.annotate(previous_year=annotation).order_by('issued_on')
expected = [None, 2018, 2018, 2019]
self.assertQuerysetEqual(queryset, expected, transform=lambda
bill: bill.previous_year)

def test_f_object_and_lookup(self):
filter_Q =
Q(client__bills__issued_on__year__lt=F('issued_on__year'))
annotation = Max(ExtractYear('client__bills__issued_on'),
filter=filter_Q)

queryset =
Bill.objects.annotate(previous_year=annotation).order_by('issued_on')
expected = [None, 2018, 2018, 2019]
self.assertQuerysetEqual(queryset, expected, transform=lambda
bill: bill.previous_year)
}}}

Not sure if it's related but while debugging this I came across #29396.
Before that change, evaluating the queryset raised an `IndexError` (both
when using `ExtractYear` and with the `__year` lookup).

--
Ticket URL: <https://code.djangoproject.com/ticket/31639>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#31639: F objects do not error when referencing transforms.
-------------------------------------+-------------------------------------
Reporter: Baptiste Mispelon | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Simon Charette):

* stage: Unreviewed => Accepted

Comment:

I think the underlying issue here is that `F` objects simply don't support
transforms but silently drops them when resolving to `Col`.

For example, given the models you've provided

{{{#!python
Bill.objects.filter(issued_on__year=F('issued_on__year'))
}}}

Results in the following on SQLite

{{{#!sql
SELECT "db_functions_bill"."id", "db_functions_bill"."client_id",
"db_functions_bill"."issued_on" FROM "db_functions_bill" WHERE
django_date_extract('year', "db_functions_bill"."issued_on") =
"db_functions_bill"."issued_on"
}}}

Notice how the right hand side of the filter `__year` transform doesn't
result in wrapping what comes after the `=`.

It's not an issue with `filter`'s compilation, `.annotate` exhibit the
same behaviour

{{{#!python
Bill.objects.annotate(year=F('issued_on__year'))
}}}

{{{#!sql
SELECT "db_functions_bill"."id", "db_functions_bill"."client_id",
"db_functions_bill"."issued_on", "db_functions_bill"."issued_on" AS "year"
FROM "db_functions_bill"
}}}

In the end, references to transforms via `F` is not supported and whether
or not it should be discussed on the mailing list. Accepting the ticket on
the basis that we should at least error out in this case.

--
Ticket URL: <https://code.djangoproject.com/ticket/31639#comment:1>

[Django]

#31639: F objects do not error when referencing transforms.
-------------------------------------+-------------------------------------
Reporter: Baptiste Mispelon | Owner: nobody

Type: Bug | Status: new

Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Baptiste Mispelon):

I personally don't have strong feelings on whether `F` objects should
accept transforms or not.

Raising an error instead of silently dropping them seems like a good fix
to me.

I wonder if this relates to #25534 somehow.

--
Ticket URL: <https://code.djangoproject.com/ticket/31639#comment:2>

[Django]

#31639: F objects do not error when referencing transforms.
-------------------------------------+-------------------------------------
Reporter: Baptiste Mispelon | Owner: nobody

Type: Bug | Status: new

Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by felixxm):

Yes it's related. Supporting transforms/lookups in the `F()` expression
will automatically fix #25534 (see
[https://github.com/django/django/blob/f8ef5f2c86683bef3b200fd864efc14f1fbbc23b/django/db/models/expressions.py#L185-L190
BaseExpression._parse_expressions()]).

--
Ticket URL: <https://code.djangoproject.com/ticket/31639#comment:3>

[Django]

#31639: F objects do not error when referencing transforms.
-------------------------------------+-------------------------------------

Reporter: Baptiste Mispelon | Owner: Srinivas
| Reddy Thatiparthy
Type: Bug | Status: assigned

Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Srinivas Reddy Thatiparthy):

* owner: nobody => Srinivas Reddy Thatiparthy
* status: new => assigned

--
Ticket URL: <https://code.djangoproject.com/ticket/31639#comment:4>

[Django]

#31639: F objects do not error when referencing transforms.
-------------------------------------+-------------------------------------
Reporter: Baptiste Mispelon | Owner: Srinivas
| Reddy Thatiparthy
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Carlton Gibson):

#31860 was a duplicate using `__date`.

--
Ticket URL: <https://code.djangoproject.com/ticket/31639#comment:5>

[Django]

#31639: F objects do not error when referencing transforms.
-------------------------------------+-------------------------------------
Reporter: Baptiste Mispelon | Owner: Srinivas
| Reddy Thatiparthy
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Tim Park):

Hey guys - Here is an outline of a brute force solution for raising a
{{{ValueError}}} if a lookup is passed into {{{F()}}}.

**Thought Process**
1. We init the F() object with a parameter {{{name}}}
2. If a lookup value is found in {{{name}}}, raise ValueError
3. Otherwise, continue instantiating the F() object as normal

So the next step is figuring out how to check if a lookup value is
contained in {{{name}}}.

1. Lookup expressions follow the format of {{{<fieldname>__<lookup>}}}
2. First, I wanted to raise the error if {{{name}}} contained two
underscores {{{__}}} but I realized we use double underscores for
referencing foreign keys as shown in our example of
{{{client__bills__issued_on}}}.
2. Per docs linked below, lookups are ALWAYS evaluated last. So if we
split the {{{name}}} string on character {{{__}}}, the last portion of
that split would be a potential lookup expression. Check if that potential
lookup expression exists in Django's actual lookup expressions. I could
follow this process to check for transforms within {{{name}}}

**Conclusion**
So this technically works but it seems hacky because it won't take into
account situations where users define their own custom lookups.

Let me know if I am missing something obvious here. Happy to take
hints/direction to help explore the right places.

Thanks!

**References**
Lookup Positioning Docs [https://docs.djangoproject.com/en/3.0/howto
/custom-lookups/#how-django-determines-the-lookups-and-transforms-which-
are-used]

List of All Django Field Lookups
[https://docs.djangoproject.com/en/3.0/ref/models/querysets/#field-
lookups]

Code for F()
[https://github.com/django/django/blob/58a336a674a658c1cda6707fe1cacb56aaed3008/django/db/models/expressions.py#L560]
{{{
class F(Combinable):

def __init__(self, name):
lookups = {'exact', 'iexact', 'contains', 'icontains', 'in', 'gt',
'gte', 'lt', 'lte', 'startswith', 'istartswith',
'endswith',
'iendswith', 'range', 'date', 'year', 'iso_year',
'month', 'day',
'week', 'week_day', 'quarter', 'time', 'hour',
'minute', 'second',
'isnull', 'regex', 'iregex'}

if name.split("__")[-1] in lookups:
raise ValueError("F() objects do not support lookups")

self.name = name
}}}

--
Ticket URL: <https://code.djangoproject.com/ticket/31639#comment:6>

[Django]

#31639: F() and OuterRef() objects do not error when referencing transforms.

-------------------------------------+-------------------------------------
Reporter: Baptiste Mispelon | Owner: Srinivas
| Reddy Thatiparthy
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by felixxm):

#31977 is a duplicate for `OuterRef()` (which is a subclass of `F`).

--
Ticket URL: <https://code.djangoproject.com/ticket/31639#comment:7>

[Django]

#31639: F() and OuterRef() objects do not error when referencing transforms.
-------------------------------------+-------------------------------------
Reporter: Baptiste Mispelon | Owner: Srinivas
| Reddy Thatiparthy
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Gordon Wrigley):

* cc: Gordon Wrigley (added)

--
Ticket URL: <https://code.djangoproject.com/ticket/31639#comment:8>

[Django]

#31639: F() and OuterRef() objects do not error when referencing transforms.
-------------------------------------+-------------------------------------
Reporter: Baptiste Mispelon | Owner: Srinivas
| Reddy Thatiparthy
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Gordon Wrigley):

As a user who has run into this several times in different contexts it
really does seem intuitive and expected that transforms and lookups would
work in F expressions and the work arounds for them not working are quite
clunky.

The current behaviour of silently not working is the worst of all options
and I'd take an error over the current behaviour, but having them work
would be better.

--
Ticket URL: <https://code.djangoproject.com/ticket/31639#comment:9>

[Django]

#31639: F() and OuterRef() objects do not error when referencing transforms.
-------------------------------------+-------------------------------------
Reporter: Baptiste Mispelon | Owner: Srinivas
| Reddy Thatiparthy
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Ian Foote):

* has_patch: 0 => 1

Comment:

While fixing #25534, I've implemented support for transforms in
expressions, fixing this too. https://github.com/django/django/pull/13685

--
Ticket URL: <https://code.djangoproject.com/ticket/31639#comment:10>

[Django]

#31639: Allow using lookup and transforms in F() and OuterRef().

-------------------------------------+-------------------------------------
Reporter: Baptiste Mispelon | Owner: Srinivas
| Reddy Thatiparthy

Type: New feature | Status: assigned

Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Mariusz Felisiak):

* has_patch: 1 => 0
* type: Bug => New feature

--
Ticket URL: <https://code.djangoproject.com/ticket/31639#comment:11>

[Django]

#31639: Allow using lookup and transforms in F() and OuterRef().
-------------------------------------+-------------------------------------
Reporter: Baptiste Mispelon | Owner: Srinivas
| Reddy Thatiparthy
Type: New feature | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Mariusz Felisiak):

* needs_docs: 0 => 1

* has_patch: 0 => 1

* needs_tests: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/31639#comment:12>

[Django]

#31639: Allow using lookup and transforms in F() and OuterRef().
-------------------------------------+-------------------------------------

Reporter: Baptiste Mispelon | Owner: Ian Foote

Type: New feature | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1

Needs tests: 1 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Mariusz Felisiak):

* owner: Srinivas Reddy Thatiparthy => Ian Foote

--
Ticket URL: <https://code.djangoproject.com/ticket/31639#comment:13>

[Django]

#31639: Allow using lookup and transforms in F() and OuterRef().
-------------------------------------+-------------------------------------
Reporter: Baptiste Mispelon | Owner: Ian Foote
Type: New feature | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Ian Foote):

* needs_docs: 1 => 0
* needs_tests: 1 => 0

--
Ticket URL: <https://code.djangoproject.com/ticket/31639#comment:14>

[Django]

#31639: Allow using transforms in F() and OuterRef().

-------------------------------------+-------------------------------------
Reporter: Baptiste Mispelon | Owner: Ian Foote
Type: New feature | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

--
Ticket URL: <https://code.djangoproject.com/ticket/31639#comment:15>

[Django]

#31639: Allow using transforms in F() and OuterRef().
-------------------------------------+-------------------------------------
Reporter: Baptiste Mispelon | Owner: Ian Foote
Type: New feature | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:

Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Mariusz Felisiak):

* stage: Accepted => Ready for checkin

--
Ticket URL: <https://code.djangoproject.com/ticket/31639#comment:16>

[Django]

#31639: Allow using transforms in F() and OuterRef().
-------------------------------------+-------------------------------------
Reporter: Baptiste Mispelon | Owner: Ian Foote

Type: New feature | Status: closed

Component: Database layer | Version: master
(models, ORM) |

Severity: Normal | Resolution: fixed

Keywords: | Triage Stage: Ready for
| checkin

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Mariusz Felisiak <felisiak.mariusz@…>):

* status: assigned => closed
* resolution: => fixed

Comment:

In [changeset:"8b040e3cbbb2e81420e777afc3ca48a1c8f4dd5a" 8b040e3c]:
{{{
#!CommitTicketReference repository=""
revision="8b040e3cbbb2e81420e777afc3ca48a1c8f4dd5a"
Fixed #25534, Fixed #31639 -- Added support for transform references in
expressions.

Thanks Mariusz Felisiak and Simon Charette for reviews.
}}}

--
Ticket URL: <https://code.djangoproject.com/ticket/31639#comment:17>