Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug: Using Aggregations in Django >= 5.1.1 fails #616

Open
suspectpart opened this issue Sep 16, 2024 · 2 comments
Open

Bug: Using Aggregations in Django >= 5.1.1 fails #616

suspectpart opened this issue Sep 16, 2024 · 2 comments

Comments

@suspectpart
Copy link

suspectpart commented Sep 16, 2024

After updating to Django 5.1.1, I ran into errors using Aggregations:

  File "/home/me/PycharmProjects/my-project/src/accounting/models.py", line 1440, in period
    return tuple(self.aggregate(Min("period_start"), Max("period_end")).values())
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/me/PycharmProjects/my-project/.venv/lib/python3.12/site-packages/polymorphic/query.py", line 316, in aggregate
    self._process_aggregate_args(args, kwargs)
  File "/home/me/PycharmProjects/my-project/.venv/lib/python3.12/site-packages/polymorphic/query.py", line 303, in _process_aggregate_args
    test___lookup(a)
  File "/home/me/PycharmProjects/my-project/.venv/lib/python3.12/site-packages/polymorphic/query.py", line 298, in test___lookup
    test___lookup(source_expression)
  File "/home/me/PycharmProjects/my-project/.venv/lib/python3.12/site-packages/polymorphic/query.py", line 300, in test___lookup
    assert "___" not in a.name, ___lookup_assert_msg
                        ^^^^^^

The line that breaks is:

for source_expression in a.get_source_expressions():

In Django 5.1.1 a change was made that consistently includes the filter arg of an expression in any case, even if it is empty - so if no filter is used, an additional None will be include in the list returned by get_source_expressions(). See here:

django/django@42b567a#diff-c7d87b6e6b973bf87e5685442a6c1da9531c24bcaa86fe158c40ea0276986f32R53

It seems like we need an additional None check here, just like we have in patch_lookup:

if source_expression is not None:

(This is also a quick workaround: If you run into this issue, transforming all aggregate expressions passed into aggregate() / annotate() into keyword args solves the problem).

Should I submit a quick PR to do that? Or is supporting Django 5 a broader issue?

@msmitherdc
Copy link

another user had a fork here that also fixes this issue master...bengosney:django-polymorphic:master

@aprams
Copy link

aprams commented Dec 19, 2024

@suspectpart thanks a lot for the named keyword args workaround 🙏 life saver

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants