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

Improve support for ordering by multiple fields #678

Open
1 of 3 tasks
diesieben07 opened this issue Dec 19, 2024 · 6 comments · May be fixed by #679
Open
1 of 3 tasks

Improve support for ordering by multiple fields #678

diesieben07 opened this issue Dec 19, 2024 · 6 comments · May be fixed by #679

Comments

@diesieben07
Copy link
Contributor

diesieben07 commented Dec 19, 2024

Support a better API for ordering by multiple fields when using @strawberry_django.order by allowing it to be generated as a list of orderings.

Feature Request Type

  • Core functionality
  • Alteration (enhancement/optimization) of existing feature(s)
  • New behavior

Description

As stated in #677 the way strawberry_django.order distinguishes "order by name then birthday" vs "order by birthday then name" as follows:

order: {
  name: ASC
  birthday: ASC
}

vs.

order: {
  birthday: ASC
  name: ASC
}

This is problematic, because usually object key order is considered to be not relevant. While object property order is now guaranteed in JavaScript and dict order is guaranteed in Python, this is still far from an ideal API, because usually object key order is considered irrelevant. Creating an object/dict with a required key order and then making sure that order is preserved when being serialized out to JSON can be challenging or might even be impossible in some languages, because the implementations might not preserve the order.
The JSON specification also states that (emphasis mine):

An object is an unordered set of name/value pairs.

This is also demonstrated by the hoops that the code needs to jump through to even extract this information from the GraphQL Python implementation.

I propose adding an option to @strawberry_django.order named multi. If set to true, the order parameter would be generated as a list instead, allowing the explicit specification of multiple ordering objects.

Upvote & Fund

  • We're using Polar.sh so you can upvote and help fund this issue.
  • We receive the funding once the issue is completed & confirmed by you.
  • Thank you in advance for helping prioritize & fund our backlog.
Fund with Polar
@jamietdavidson
Copy link

Wow, your timing is perfect - I was just wondering if we could translate it into a list, too.

@diesieben07
Copy link
Contributor Author

I'm now looking into implementing this.

@jamietdavidson
Copy link

jamietdavidson commented Dec 19, 2024 via email

@bellini666
Copy link
Member

Hey @diesieben07 ,

I am glad that you are willing to try to implement this :)

We have discussed this issue before at #615

IMO, instead of multi, I would prefer to have an "ordering v2", which uses lists by default, and move the one we have to "legacy ordering" (even raising a warning when it gets used).

I would even change the strawberry_django.order to strawberry_django.ordering, because order by itself can mean something else (e.g. a purchase "order" in an e-commerce website). So, we could have:

  • @strawberry_django.order and @strawberry_django.type(SomeModel, order=SomeOrder) (legacy)
  • @strawberry_django.ordering and @strawberry_django.type(SomeModel, ordering=SomeOrdering) (v2)

What do you think?

obs. feel free to ping me on Discord if you need any help with the implementation or to discuss any technical decisions in the code :)


Just one more thing:

While object property order is now guaranteed in JavaScript and dict order is guaranteed in Python

Actually, since 3.7, the order of python dicts is guaranteed by the language. It used to be like that for CPython only, but now any other implementation must adhere to that rule as well.

Image

The issue lies mostly in the conversion of variables, which somehow doesn't preserve the order in which it got passed (as you can see in the issue I linked above)

@diesieben07
Copy link
Contributor Author

That sounds like a good idea, @bellini666! I was already experimenting with adding a multi parameter to order() yesterday and the code turned out very messy. I like your idea much better.

The issue lies mostly in the conversion of variables, which somehow doesn't preserve the order in which it got passed (as you can see in the issue I linked above)

Yes, but also the fact that some languages (other than Python/JavaScript) might not even allow you to modify the order in which things get serialized. I am thinking of more "strictly typed" and "compiled" languages like Kotlin, Java, Swift, .NET, etc.

@diesieben07 diesieben07 linked a pull request Dec 22, 2024 that will close this issue
11 tasks
@diesieben07
Copy link
Contributor Author

I've sent my current work as a draft PR at #679.
I won't be able to work on it more for a few days due to the holidays, but any feedback is already welcome.

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.

3 participants