django-sortedm2m icon indicating copy to clipboard operation
django-sortedm2m copied to clipboard

Broke in Django 5.0?

Open brianwawok opened this issue 5 months ago • 7 comments

Trying to deep dive into why, but it broke a bunch of unit tests when I upgrade to django 5.

The symptoms I see, are

print(list(some_model.the_sorted_m2m_field.all()))

Shows the items in a random order (sometimes the right, sometimes not). But if I look at

some_model.the_sorted_m2m_field.all().query I see ORDER BY ("the_join_table".sort_value) ASC

So something that is showing up .query is not actually showing up in the results? Very confused..

Update 1: My some_model has an order_by. That is making the sql sent change from ordering by the sortedm2m to the generic table sorting..

Update 2: In my debugger, I see SortedRelatedManager try to _apply_rel_ordering, but the queryset is already ordered and it gets ignored..

Update 3: If I shove in my code .extra(order_by=[XX]) where XX is the exact code the plugin is generating, all is well and it works.

brianwawok avatar Jan 08 '24 20:01 brianwawok

Tests fail too:

Found 50 test(s).
Creating test database for alias 'default'...
System check identified no issues (0 silenced).
.....F..................F.......F.................
======================================================================
FAIL: test_prefetch_related_sorting (sortedm2m_tests.test_field.TestCustomBaseClass.test_prefetch_related_sorting)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/tmp/django-sortedm2m/sortedm2m_tests/test_field.py", line 185, in test_prefetch_related_sorting
    self.assertEqual(get_ids(shelf.books.all()), get_ids(books))
AssertionError: Lists differ: [1, 2, 3] != [1, 3, 2]

First differing element 1:
2
3

- [1, 2, 3]
+ [1, 3, 2]

======================================================================
FAIL: test_prefetch_related_sorting (sortedm2m_tests.test_field.TestSortedManyToManyField.test_prefetch_related_sorting)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/tmp/django-sortedm2m/sortedm2m_tests/test_field.py", line 185, in test_prefetch_related_sorting
    self.assertEqual(get_ids(shelf.books.all()), get_ids(books))
AssertionError: Lists differ: [1, 2, 3] != [1, 3, 2]

First differing element 1:
2
3

- [1, 2, 3]
+ [1, 3, 2]

======================================================================
FAIL: test_prefetch_related_sorting (sortedm2m_tests.test_field.TestStringReference.test_prefetch_related_sorting)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/tmp/django-sortedm2m/sortedm2m_tests/test_field.py", line 185, in test_prefetch_related_sorting
    self.assertEqual(get_ids(shelf.books.all()), get_ids(books))
AssertionError: Lists differ: [1, 2, 3] != [1, 3, 2]

First differing element 1:
2
3

- [1, 2, 3]
+ [1, 3, 2]

----------------------------------------------------------------------
Ran 50 tests in 1.079s

FAILED (failures=3)
Destroying test database for alias 'default'...

mgorny avatar Jan 27 '24 11:01 mgorny

@mgorny Any leads yet? I spent a solid half day trying to reverse engineer it, but not seeing the change. This is blocking my django 5 upgrade, so something I will have to get sorted eventually..

brianwawok avatar Jan 30 '24 16:01 brianwawok

I'm sorry but I haven't been investigating it, and I don't think I will.

mgorny avatar Jan 30 '24 17:01 mgorny

TLDR : Avoid using prefetch_related on SortedManyToManyFields until the problem is fixed.

I ran into the same problem and came up with a potential fix. Replacing the SortedRelatedManager.get_queryset() implementation with the following seems to solve the problem:

def get_queryset(self):
    try:
        queryset = self.instance._prefetched_objects_cache[self.prefetch_cache_name]
    except (AttributeError, KeyError):
        queryset = super().get_queryset()

    if len(queryset.query.order_by) == 0:
        queryset = self._apply_rel_ordering(queryset)

    return queryset

instead of

def get_queryset(self):
    try:
        return self.instance._prefetched_objects_cache[self.prefetch_cache_name]
    except (AttributeError, KeyError):
        queryset = super().get_queryset()
        return self._apply_rel_ordering(queryset)

Basically we make sure that the ordering is set with _apply_rel_ordering except if the query was already explicitly sorted with order_by. The early return for prefetched objects is no longer ordered correctly for some reason that I could not precisely point out.

If a maintainer could confirm that this is a valid approach I'd be happy to open a pull request. In the meantime avoinding to prefetch_related on SortedManyToManyFields solves the problem, with an obvious performance penalty.

Pamoi avatar Mar 04 '24 15:03 Pamoi

I think, that the problem is changing method get_prefetch_queryset to get_prefetch_querysets in related_descriptors.py in Django 5.0.

So _apply_rel_ordering is not called anymore and there is not a part ORDER BY ("table1_table2_field".sort_value) ASC in generated SQL.

The workaround is to add extra(order_by=[...]) to the queryset in prefetch_related call.

Pseudo code example:

Django 4.2 version (standard and simple):

MyModel.objects.filter(...).prefetch_related('m2mfield')

Django 5.0 solution:

from django.db.models.query import Prefetch

prefetch_qs = M2MModel.objects.all().extra(order_by=['app_mymodel_m2mfield.sort_value'])
MyModel.objects.filter(...).prefetch_related(Prefetch('m2mfield', queryset=prefetch_qs)))

I don't like it so much. It is so low-level, hardcoded tables and sort_value field name. But it works.

mokys avatar Mar 11 '24 20:03 mokys

Thank you @mokys, that seems indeed to be the source of the problem. The definitive solution for Django 5 would then be to implement get_prefetch_querysets in SortedRelatedManager. @clintonb @AlanCoding @relrod I see that you are listed as django-sortedm2m maintainers on Jazzband's website. I'd be happy to help if you give a sign of life :)

Pamoi avatar Mar 12 '24 14:03 Pamoi

I did a very dirty copy paste

https://github.com/listing-mirror/django-sortedm2m/commit/fd9f87a49fb714b1adcfe50fb2b9e2d5dc7029a1

per @mokys

and it works great / passes all 5,000 unit & UI tests

brianwawok avatar Apr 08 '24 21:04 brianwawok