Django ManyToMany fields dont order results by through model's Model.Meta.ordering

[Archmonger]

The example seen in my other issues models.py example is broken.

The top-level ordering of SpotlightCategory works, but the ordering of SpotlightApp is broken.

[shuckc]

Any chance of a failing test case? Otherwise there's not enough information here to reproduce.

[Archmonger]

On the example below, ordering features work properly on SpotlightCategory works, but SpotlightApp ordering is broken.

Example

my_example.py

Running this snippet will receive a completely unordered list of SpotlightApps.

# These are properly ordered
categories: Iterable[SpotlightCategory] = SpotlightCategory.objects.all()

# This is unordered, despite being a OrderedModel
apps: Iterable[SpotlightApp] = category.apps.all()

admin.py

class SpotlightAppTabularInline(OrderedTabularInline):
    model = models.SpotlightApp
    readonly_fields = (
        "order",
        "move_up_down_links",
    )
    ordering = ("order",)
    extra = 1


@admin.register(models.SpotlightCategory)
class SpotlightCategories(OrderedInlineModelAdminMixin, OrderedModelAdmin):
    model = models.SpotlightCategory
    list_display = ("name", "order", "move_up_down_links")
    ordering = ("order",)
    inlines = (SpotlightAppTabularInline,)

models.py

class AppPackage(models.Model):
    def __str__(self):
        return str(self.name)

    uuid = models.UUIDField(
        primary_key=True, default=uuid.uuid4, editable=False, unique=True
    )

    # General Info
    name = models.CharField(max_length=100)

class SpotlightCategory(OrderedModel):
    def __str__(self):
        return str(self.name)

    class Meta:
        verbose_name = "Spotlight category"
        verbose_name_plural = "Spotlight categories"

    uuid = models.UUIDField(
        primary_key=True, default=uuid.uuid4, editable=False, unique=True
    )

    name = models.CharField(max_length=50, unique=True)
    description = models.TextField(blank=True)
    apps = models.ManyToManyField(AppPackage, through="SpotlightApp")


class SpotlightApp(OrderedModel):
    """This model is used to create a many-to-many relationship between SpotlightCategory and AppPackage.
    This allows for the ordering of apps within a spotlight category."""

    category = models.ForeignKey(SpotlightCategory, on_delete=models.CASCADE)
    app = models.ForeignKey(AppPackage, on_delete=models.CASCADE)
    order_with_respect_to = "category"

    class Meta:
        ordering = ("category", "order")

[shuckc]

TLDR: it's a limitation of ManyToMany fields, they don't automatically pick up the ordering from the Meta of the specified through=... model. It's not explicitly documented in Django one way or the other, so perhaps they would accept a patch. Certainly for our usage it seems more correct.

From my point of view, ordered_model provides the admin side of things, we rely on Django to apply the ordering by following the model Meta. If there was a reasonably clean way to hook this our side then I would certainly entertain providing it, especially if Django declined to ship it.

The alternative for now is to manually re-apply the ordering explicitly, e.g. in your case this gives the expected output:

from django.shortcuts import render
from .models import SpotlightCategory

def dumpordering(request):
  for cat in SpotlightCategory.objects.all():
    for app in cat.apps.all().order_by('spotlightapp__order'):
      print(app)
  ...

However its not easy to pass around the sorted output to a template, or call that using standard Django template features.

Looks like it's worked this way for at least 11 years: https://stackoverflow.com/questions/6003330/django-displaying-many-to-many-in-template-ordering-ignored

[shuckc]

Example of a workaround https://django-ajax-selects.readthedocs.io/en/latest/Ordered-ManyToMany.html which subclasses the ManyToManyField out for one that fixes the ordering (but not generically).

There's also https://github.com/jazzband/django-sortedm2m project which overlaps a lot with ours (provides admin, automatic ordering field etc.) when used with a M2M, however it probably works better for this case.

[shuckc]

Hi @Archmonger if you get a chance to run #285 in a dev environment, switch the field over and give it a try. It creates a migration, but I don't think there are any actual database changes when applying it.

[Archmonger]

Sure I'll give it a shot tonight or tomorrow.

I just had an idea that could lead to a much more convenient user experience. What if the OrderedModel class mutates all ManyToMany fields into OrderedManyToMany (if that ManyToMany field is using a OrderedModel).

I haven't done a deep dive on Django's ORM so I don't know if this type of inference is even possible, especially given the use of through=... models.

If you like the idea I could try PRing it into #285

EDIT: On second thought, this would be a bad approach, since OrderedManyToMany can potentially exist on non-ordered parents.

[Archmonger]

This work week has been way more hectic than I expected, will have to get to this over the weekend.

[Archmonger]

I tested #285 in a dev environment. It seems to work for basic use cases.

However, further testing showed that all Django prefetching logic silently fails.

For example, field_name = OrderedManyToMany(...) doesn't work with MyQuerySet.prefetch_related('field_name') and prefetch_related_objects([MyQuerySet], 'field_name').

[shuckc]

What is MyQuerySet here, is it returned from category.apps.all()? Can you give an example I can run.

Might also be worth running the new checks in 3.7.2 to see if they find anything odd with your QuerySet subclasses?

[Archmonger]

Yes. MyQuerySet is any query that contains models with the new OrderedManyToMany field.

I will test out the new version, and create an example when possible.

[Archmonger]

The issues were detected due to Django-IDOM's use_query hook relying heavily on prefetch logic. Database queries are performed async, so we are forced to use Django prefetch to prevent SynchronousOnlyOperation.

I wrote the following example as a similar imitation of use_query.

categories = SpotlightCategory.objects.all()

for category in categories:
   # This should label the 'apps' field as a value that should be opportunistically prefetched
   # note: 'apps' is an `OrderedManyToManyField`
   prefetch_related_objects([category], 'apps')

   # Call any attribute to force the query to execute
   print(category.name)

   # This unexpectedly causes a new query to be performed. However, it should have been prefetched.
   print(category.apps.get_queryset().all())

[shuckc]

I'm no expert on this, but I would have expected the additional query when you are calling get_queryset() explicitly.

Does this version hit the cache (and crucially, returned ordered results!)?

for category in categories:
   prefetch_related_objects([category], 'apps')
   print(category.name)

   print(category.apps.all())

[Archmonger]

Apologies. I'm realizing now the example given wasn't 1:1.

Here's a more accurate example with better comments.

Note that category.apps is a OrderedManyToMany field, as suggested above.

from django.db.models import prefetch_related_objects

categories = SpotlightCategory.objects.all()

# Initial run forces DB query execution to allow results to be stored in cache
for category in categories:
   # This should label the 'apps' field as a value that should be cached
   prefetch_related_objects([category], 'apps')

   print(category.name)  # Call attribute to force `Model` to execute DB queries
   print(category.apps.get_queryset().all())  # Call `QuerySet` to execute DB queries

# Second run, everything should already be stored in cache
for category in categories:
   print(category.name)  # This works as expected (the result is cached)
   print(category.apps.get_queryset().all())  # This causes a new query to be performed. It should have been cached.

[Archmonger]

You can consider merging #285 and I'll open a new issue for OrderedManyToMany caching, since that PR is otherwise functional.