django-sortedm2m
django-sortedm2m copied to clipboard
Broke in Django 5.0?
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.
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 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..
I'm sorry but I haven't been investigating it, and I don't think I will.
TLDR : Avoid using prefetch_related
on SortedManyToManyField
s 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 SortedManyToManyField
s solves the problem, with an obvious performance penalty.
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.
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 :)
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
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
inSortedRelatedManager
. @clintonb @AlanCoding @relrod I see that you are listed asdjango-sortedm2m
maintainers on Jazzband's website. I'd be happy to help if you give a sign of life :)
@Pamoi sorry for not responding sooner. In the future, feel free to open a pull request.