jazzband / django-polymorphic

Improved Django model inheritance with automatic downcasting
https://django-polymorphic.readthedocs.io
Other
1.66k stars 280 forks source link

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

Open suspectpart opened 2 months ago

suspectpart commented 2 months ago

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:

https://github.com/jazzband/django-polymorphic/blob/cfd49b26d580d99b00dcd43a02409ce439a2c78f/polymorphic/query.py#L279

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:

https://github.com/django/django/commit/42b567ab4c5bfb1bbd3e629b1079271c5ae44ea0#diff-c7d87b6e6b973bf87e5685442a6c1da9531c24bcaa86fe158c40ea0276986f32R53

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

https://github.com/jazzband/django-polymorphic/blob/cfd49b26d580d99b00dcd43a02409ce439a2c78f/polymorphic/query.py#L255

(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 commented 1 month ago

another user had a fork here that also fixes this issue https://github.com/jazzband/django-polymorphic/compare/master...bengosney:django-polymorphic:master