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

order_by() has no effect on values()/values_list() in models registered for translation #655

Closed
mick88 opened this issue Nov 1, 2022 · 5 comments

Comments

@mick88
Copy link
Contributor

mick88 commented Nov 1, 2022

I have encounteded a bug with a model that has default ordering. I am making a query with order_by() without parameters to avoid ordering and then use values() or values_list() to extract a subset of fields. The end result is an ordered queryset.

It appears that values() restores model's default ordering if called after order_by(), except if queryset is already ordered by cystom ordering (e.g. order_by('name')), in which case order_by is maintained.

Django 3.2.16
Python 3.10
django-modeltranslation 0.18.5

The model:

class Dashboard(models.Model):
    name = models.CharField(max_length=100)
    order = models.PositiveIntegerField(default=1)

    class Meta:
        ordering = 'order',

@register(models.Dashboard)
class DashboardTranslation(TranslationOptions):
    fields = 'name',

Note: it is important that the model defines ordering

To reproduce issue:

# Queryset is ordered by default field as expected ✔
Dashboard.objects.all().values('name').ordered
Out[44]: True

# Queryset is not ordered after adding order_by() as expected ✔
Dashboard.objects.all().order_by().ordered
Out[54]: False

# adding values() call somehow restores default ordering. Expected behaviour is for QS to remain unordered ✖
Dashboard.objects.all().order_by().values('name').ordered
Out[42]: True

# placing order_by() after values() works as expected ✔
Dashboard.objects.all().values('name').order_by().ordered
Out[48]: False

I originally raised the issue in Django bugtracker, but when it wan't reproduced, I debugged it locally and realized that the model uses translations and modeltranslations overrides values() and does not call super().

@mick88
Copy link
Contributor Author

mick88 commented Nov 1, 2022

Added MR #656 that demonstrates the bug

@last-partizan
Copy link
Collaborator

Great work reproducing issue.

I currently don't have time to debug this myself, but i have time to review a fix if you can figure it out.

@mick88
Copy link
Contributor Author

mick88 commented Nov 5, 2022

It looks like the culprit is the _post_init() method in queryset. It sets order from model if not set:

    def _post_init(self):
        self._rewrite = True
        self._populate = None
        if self.model and (not self.query.order_by):
            if self.model._meta.ordering:
                # If we have default ordering specified on the model, set it now so that
                # it can be rewritten. Otherwise sql.compiler will grab it directly from _meta
                ordering = []
                for key in self.model._meta.ordering:
                    ordering.append(rewrite_order_lookup_key(self.model, key))
                self.query.add_ordering(*ordering)

@mick88
Copy link
Contributor Author

mick88 commented Nov 5, 2022

Updated my MR with a proposed patch. This passes my test and the other tests in the suite. Please review #656

last-partizan pushed a commit that referenced this issue Nov 7, 2022
@last-partizan
Copy link
Collaborator

Looks good. Merged, thanks.

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

No branches or pull requests

2 participants