Skip to content

Fixed #29538 -- Added support for ordering related objects by query expressions.#10146

Closed
alexandrelaplante wants to merge 5 commits intodjango:masterfrom
alexandrelaplante:al/fix-order-by-related-query-expression
Closed

Fixed #29538 -- Added support for ordering related objects by query expressions.#10146
alexandrelaplante wants to merge 5 commits intodjango:masterfrom
alexandrelaplante:al/fix-order-by-related-query-expression

Conversation

@alexandrelaplante
Copy link
Copy Markdown

@alexandrelaplante alexandrelaplante commented Jul 8, 2018

Added support for ordering related objects by query expressions

https://code.djangoproject.com/ticket/29538

Added support for ordering related objects by query expressions
attrgetter('name')
)

@skip('What should the behavior be?')
Copy link
Copy Markdown
Author

@alexandrelaplante alexandrelaplante Jul 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what the expected behavior here is.

class Album:
    class Meta:
        ordering = [models.F('artist').asc(nulls_last=True)]
class Album:
    class Meta:
        ordering = ['artist']

Should both of these work the same way? Or does
the first mean "order by artist_id"
and the second mean "order by the default order of Musician"?

Copy link
Copy Markdown
Member

@carltongibson carltongibson Sep 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty sureordering = ['artist'] will mean order by artist (rather than the default order of the related model). (In the compiler, field names get converted to column references, rather than resolved via related models' Meta...)

Possible to check that though...

Copy link
Copy Markdown
Member

@charettes charettes Sep 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@carltongibson no, you should useartist_id if you want to achieve that. There's a documented difference between order_by('artist') and order_by('artist_id') as it was not even possible to disable related ordering before 24ec953.

Given Artist.Meta.ordering = ['foo', F('bar').asc(nulls_last=False)]

then Album.Meta.ordering = [models.F('artist').asc(nulls_last=True)]should be the equivalent of

[F('artist__foo').asc(nulls_last=True), F('artist__bar').asc(nulls_last=True)]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good. Thanks Simon.

order, already_seen))

def _join_strings(*strings):
return LOOKUP_SEP.join(filter(None, strings))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this for? Is the filter() necessary? Couldn't this just be inlined below?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is the alternative

if prefix is None:
    new_prefix = name
else:
    new_prefix = prefix + LOOKUP_SEP + name

prefix is None at the top of the recursive call.

Which is better?

The old name 'find_ordering_name' was no longer
describing what this function was doing 9 years
since it was named. convert_to_expression seems
to fit better.
@timgraham timgraham changed the title WIP: Fixed #29538 -- Query Expression in ordering of a related object fails Fixed #29538 -- Added support for ordering related objects by query expressions. Aug 23, 2018
Copy link
Copy Markdown
Member

@carltongibson carltongibson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @alexandrelaplante.

Thanks for the input here. Sorry for the slowish pick-up.

One initial point on the placement of the tests:

Can we include these in the existing ordering app?

  • We try not to add new models if we can avoid it. (They slow everything down.) Can we re-use or adjust the ordering.models models?
  • Can we move order_with_expression.tests into ordering too? (Either in a test case in tests.py or a separate test_....py file if your prefer.)

@alexandrelaplante
Copy link
Copy Markdown
Author

@carltongibson I tried reusing the models at first but they weren't quite structured in the right way for my tests. I'll give it another try, possibly restructuring the existing tests to accommodate.

@carltongibson
Copy link
Copy Markdown
Member

@alexandrelaplante Thanks! (Perhaps if you move the tests all into ordering first, we can see if we can remove the new models second.)

Copy link
Copy Markdown
Contributor

@auvipy auvipy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please rebase and address carls comments

@felixxm
Copy link
Copy Markdown
Member

felixxm commented Jun 24, 2020

Closing due to inactivity.

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 this pull request may close these issues.

6 participants