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

FIX QuerySort::sort method should support sorting for multi arguments #562

Conversation

sabina-talipova
Copy link
Contributor

@sabina-talipova sabina-talipova commented Nov 14, 2023

Description

Even if a method receives several parameters, then sorting occurs by the field of the arguments that is last specified in the schema, and not in the passed parameters. That is, if SilverStripe\GraphQL\Tests\Fake\DataObjectFake has the following schema

fields:
    myField: true
    AuthorID: true

and sorting is required AuthorID => DESC, myField => ASC, then sorting was done only by AuthorID, since in the schema this field is indicated after myField.
Therefore, the call $list->sort() must be completed after all the necessary processing has been carried out.

Parent issue

@sabina-talipova sabina-talipova force-pushed the pulls/5.1/fix-broken-build branch from 176f29f to a713bd8 Compare November 14, 2023 07:32
@sabina-talipova sabina-talipova marked this pull request as draft November 14, 2023 20:23
@sabina-talipova sabina-talipova force-pushed the pulls/5.1/fix-broken-build branch 2 times, most recently from b3929c0 to ef90d51 Compare November 20, 2023 01:55
@sabina-talipova sabina-talipova marked this pull request as ready for review November 20, 2023 02:15
Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

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

This can't be MNT since it's changing actual code that projects will be using.

If the changes in the QuerySort class are fixing a bug, you should use FIX and the commit message should explain what the bug is that's being fixed.
If they're just an enhancement, then use ENH and the commit message should explain what the enhancement is.

@sabina-talipova sabina-talipova force-pushed the pulls/5.1/fix-broken-build branch from ef90d51 to 2e5ca8f Compare November 20, 2023 07:34
@sabina-talipova sabina-talipova changed the title MNT Fix broken build FIX QuerySort::sort method should support sorting for multi arguments Nov 20, 2023
@GuySartorelli
Copy link
Member

GuySartorelli commented Nov 21, 2023

The reason changing the order of the fields in the schema affected the results is because the underlying GraphQL library we're using ignores the order that fields are entered in the query arguments. It uses the order that the fields were defined in the schema.

I've added a new commit on top of yours to resolve this. It also changes the test to use a query provider so it will run all of the FilterAndSort scenarios even if one fails. I removed the scenarios that had commented out tests because they weren't actually contributing anything to the test coverage.

Normally I would have asked you to make those changes, but I didn't know what changes were necessary or even possible - and to find that out I had to essentially do the full implementation. At that point it was just easier to commit that implementation rather than ask you to redo what I had just done.

Next steps

  1. Have a look over this code and test it locally to make sure you're happy with it. This is still your PR, so make any changes you think are appropriate.
  2. Add another test scenario where we sort first by myField and then by AuthorID to protect us against regressions
  3. Add new test coverage to check this works correctly with a normal read (not read one - but read many) query
  4. See if changes similar to what I've made are necessary in src/Schema/Plugin/SortPlugin.php

@GuySartorelli
Copy link
Member

Please also check if this bug affects graphql 4 - if so, we need to retarget to 4.3

@sabina-talipova
Copy link
Contributor Author

@GuySartorelli GuySartorelli deleted the pulls/5.1/fix-broken-build branch November 22, 2023 21:02
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.

2 participants