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

sort_by_fields() fails when model is ordered through a foreign key #45

Closed
juyrjola opened this issue Nov 4, 2015 · 6 comments
Closed

Comments

@juyrjola
Copy link
Contributor

juyrjola commented Nov 4, 2015

If the ordering field of a model is configured to order the objects over a foreign key relationship (using the __ syntax), sort_by_fields() fails.

E.g.

   class Car(models.Model):
   ...
   class Meta:
       ordering = ['brand__name']
@roadsideseb
Copy link

I am running into the same issue with using a ParentalKey in the ordering setting:

class Vineyard(Model):
    name = CharField(max_length=200)


class Dinner(ClusterableModel):
    vineyard = ParentalKey(Vineyard)
    class Meta:
        ordering = ('vineyard',)

This raises an exception with the following message:

  File "$HOME/lib/python3.5/site-packages/modelcluster/utils.py", line 19, in sort_by_fields
    items.sort(key=lambda x: (getattr(x, key) is not None, getattr(x, key)), reverse=reverse)
TypeError: unorderable types: Vineyard() < Vineyard()

One way to make this work is to implement __gt__ and __lt__ on the Vineyard model to make them comparable. Something like this works for me:

class Vineyard(Model):
    name = CharField(max_length=200)

    def __lt__(self, other):
        return self.name < other.name

    def __gt__(self, other):
        return self.name > other.name

I'm not quite sure if that would be the suggested solution or more of a workaround. I would think an implementation that works with Django model out of the box would be best but implicitly comparing model without adding additional DB queries might not be straight forward.

Are there any alternative suggestions?

@thenewguy
Copy link

I've encountered this as well attempting to order based on a field of the foreign key relation as well. My use case is attempting to use the InlinePanel on a "through" model to mimic a m2m relation. My intentions are to order based on a tag:

class TagFilter(models.Model):
    class Meta:
        ordering = ['tag__slug']

    tag = models.ForeignKey(Tag, related_name="+")
    page = ParentalKey(TagFilterPage, related_name='tag_filters')

@thenewguy
Copy link

Btw, my workaround for the meantime is just to duplicate the data. Like this:

class Meta:
        ordering = ['_duplicate_tag_slug']

_duplicate_tag_slug = models.SlugField(max_length=100, editable=False)

And then set the data when tags are saved or the tagfilter is saved. Works easily enough with signals for simple ordering.

willbarton added a commit to cfpb/consumerfinance.gov that referenced this issue Jan 15, 2020
This is a fix for job listings that have multiple USAJobs links in them. With Python 3's stricter comparisons, attempting to order the `USAJobsApplicationLink` objects using `applicant_type` was failing with `ApplicantType` not supporting `<` operations.

This fix adds `__gt__` and `__lt__` to `ApplicantType` to support `>` and `<` operations.

Generally this appears to come from django-modelcluster and behavior that differs from the Django expectation, possibly related to wagtail/django-modelcluster#45.
@przemub
Copy link

przemub commented Mar 8, 2022

The problem is still there. My workaround without duplicating the data is:

class Category(models.Model):
    name = models.CharField(unique=True, blank=False, max_length=50)
    type = models.ForeignKey(
        CategoryType, null=False, on_delete=models.CASCADE
    )

    @property
    def type__name(self):
        """
        Circumvents https://github.com/wagtail/django-modelcluster/issues/45
        """
        return self.type.name

    class Meta:
        ordering = ["type__name", "name"]

@gasman
Copy link
Contributor

gasman commented Mar 8, 2022

This has now been implemented in #148 (but isn't in a release yet, so you'll need to install directly from git).

@gasman gasman closed this as completed Mar 8, 2022
@przemub
Copy link

przemub commented Mar 8, 2022

@gasman Awesome, thank you a lot for letting me know!

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

Successfully merging a pull request may close this issue.

5 participants