strawberry-graphql / strawberry-django

Strawberry GraphQL Django extension
https://strawberry.rocks/docs/django
MIT License
404 stars 117 forks source link

Ordering order being ignored #150

Closed Kitefiko closed 10 months ago

Kitefiko commented 2 years ago

Hi, when ordering with 2 and more values order of values is bound to the definition of Order object thus negating user defined order.

Am I mistaken in assuming these two should not be equivalent?

query {
  milestoneList(
    order: {
      project: {
        name: ASC
      }
      name: ASC
    }
  ) {
    name
    project {
      name
    }
  }
}
query {
  milestoneList(
    order: {
      name: ASC
      project: {
        name: ASC
      }
    }
  ) {
    name
    project {
      name
    }
  }
}

The result of both of these queries is the same, seems like the actual order is determined via schema definition and not input.

Upvote & Fund

Fund with Polar

Kitefiko commented 2 years ago

According to spec arguments are correctly given unordered.

So sould then be the implementation of ordering in this lib changed? Take current object implementation, put it in list and allow only one leaf value per object in said list?

bellini666 commented 2 years ago

@Kitefiko yeah, we need a new ordering implementation.

I'm actually wanting to implement this for some time (will probably try this weekend) to implement a new ordering which receives a list of objects, which can maintain the order they are given.

With that we can deprecate the order, which is also not a good name since it usually clashes with the name order when used in a different context (e.g. order from an ecommerce project)

g-as commented 1 year ago

@bellini666 is this on your radar? have you given it more thought?

bellini666 commented 11 months ago

Hi @g-as ,

Yes it is! I'm planning to work on this as soon as I can, but I'm a little busy those last weeks.

I would say that I'll be able to take a look at this by the end of october, probably.

g-as commented 11 months ago

Great! Just wanted to make sure it was still in the works. Keep up the good work!

bellini666 commented 10 months ago

I found a way to fix this in #409 . Maybe with this we don't need to refactor the ordering code anymore?

The refactoring discussion is going to continue on #399

TWeidi commented 10 months ago

Hi,

i just tried to use the adjusted ordering interface from the GraphiQL interface and it works very well, thank you for that! Unfortunately, if I want to use a JS GraphQL client like the Apollo Client, the ordering cannot be employed in the way it was ment to be, since Apollo Client or JS itself, I am not yet sure about that, seems to alphabetically order the object keys before invoking the query, so querying for instance { someQuery(ordering: {b: 'ASC', a: 'DESC'}) { ... }

will always be sent as

{ someQuery(ordering: {a: 'DESC', b: 'ASC'}) { ... }

due to alphabetical sorting when passing 'ordering' as variable. Can you confirm this behavior or is it actually me doing something wrong?

bellini666 commented 10 months ago

due to alphabetical sorting when passing 'ordering' as variable. Can you confirm this behavior or is it actually me doing something wrong?

I'm not sure on how apollo itself behaves, but you can try to inspect your network call to see if it is indeed apollo that is modifying the order

he0119 commented 9 months ago

Hi, I found that f.name.value at L110 is camelCase value while at L63 f.name is underscore value which makes sort_key function always return 0. Is there something wrong with my code?

https://github.com/strawberry-graphql/strawberry-graphql-django/blob/c90a7907913188cb5b70ee74e3fbe4a1ead7c8a2/strawberry_django/ordering.py#L110 https://github.com/strawberry-graphql/strawberry-graphql-django/blob/c90a7907913188cb5b70ee74e3fbe4a1ead7c8a2/strawberry_django/ordering.py#L62-L65

I added two queries to test, and second one has failed as expected. https://github.com/he0119/strawberry-graphql-django/commit/d4f2cee36c51e8e8909e953a1f17ac2bb478f171