search

silverstripe / silverstripe-graphql

Serves Silverstripe data as GraphQL representations
BSD 3-Clause "New" or "Revised" License
52 stars 61 forks source link

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

Closed sabina-talipova closed 9 months ago

sabina-talipova commented 9 months ago

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

GuySartorelli commented 9 months ago

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 commented 9 months ago

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

sabina-talipova commented 9 months ago

Open new PR for 4.3: