Merge pull request #577 from kinglozzer/571-sort-warning

FIX: Don't attempt to correct sort when passed as an argument (closes #571)

_config/logging.yml

@@ -4,5 +4,11 @@ after: '#logging'
 ---
 
 SilverStripe\Core\Injector\Injector:
+  # Omits the HTTPOutputHandler from the logger so errors won't appear in output
+  Psr\Log\LoggerInterface.graphql-quiet:
+    type: singleton
+    class: Monolog\Logger
+    constructor:
+      - 'graphql'
   # This will occasionally be overridden with SilverStripe\GraphQL\Schema\Logger to provide a nicer output on schema build task
   Psr\Log\LoggerInterface.graphql-build: '%$Psr\Log\LoggerInterface.errorhandler'

src/Schema/Traits/SortTrait.php

@@ -2,7 +2,10 @@
 
 namespace SilverStripe\GraphQL\Schema\Traits;
 
+use GraphQL\Language\AST\ObjectValueNode;
 use GraphQL\Type\Definition\ResolveInfo;
+use Psr\Log\LoggerInterface;
+use SilverStripe\Core\Injector\Injector;
 
 trait SortTrait
 {
@@ -44,6 +47,14 @@ private static function getSortOrder(ResolveInfo $info, string $fieldName)
                     continue;
                 }
 
+                // If the sort has been passed as a variable, we can't attempt to fix it
+                // See https://github.com/silverstripe/silverstripe-graphql/issues/573
+                if (!$arg->value instanceof ObjectValueNode) {
+                    $logger = Injector::inst()->get(LoggerInterface::class . '.graphql-quiet');
+                    $logger->warning('Unable to adjust sort order, sort fields may be applied in incorrect order.');
+                    continue;
+                }
+
                 // Get the sort order from the query
                 $sortOrder = [];
                 foreach ($arg->value->fields as $field) {

tests/Schema/IntegrationTest.php

@@ -847,6 +847,112 @@ public function testFilterAndSortOnlyRead(string $fixture, string $query, array
         $this->assertResults($expected, $records);
     }
 
+    public function provideFilterAndSortWithArgsOnlyRead(): array
+    {
+        return [
+            'read with sort - with sort arg' => [
+                'fixture' => '_QuerySort',
+                'query' => <<<GRAPHQL
+query (\$sort: DataObjectFakeSortFields) {
+  readDataObjectFakes(sort: \$sort) {
+    nodes {
+      myField
+      author {
+        firstName
+      }
+    }
+  }
+}
+GRAPHQL,
+                'args' => [
+                    'sort' => ['author' => ['id' => 'DESC'], 'myField' => 'ASC']
+                ],
+                'expected' => [
+                    ["myField" => "test2", "author" => ["firstName" => "tester2"]],
+                    ["myField" => "test3", "author" => ["firstName" => "tester2"]],
+                    ["myField" => "test1", "author" => ["firstName" => "tester1"]],
+                ],
+            ],
+            'read with sorter files ParentID DESC, name ASC - with sort args' => [
+                'fixture' => '_SortPlugin',
+                'query' => <<<GRAPHQL
+query (\$sort: DataObjectFakeSortFields, \$fileSort: FilesSimpleSortFields) {
+  readDataObjectFakes(sort: \$sort) {
+    nodes {
+      myField
+      files(sort: \$fileSort) {
+        title
+      }
+    }
+  }
+}
+GRAPHQL,
+                'args' => [
+                    'sort' => ['myField' => 'ASC'],
+                    'fileSort' => ['ParentID' => 'DESC', 'name' => 'ASC'],
+                ],
+                'expected' => [
+                    ["myField" => "test1", "files" => [["title" => "file2"], ["title" => "file4"], ["title" => "file1"], ["title" => "file3"]]],
+                    ["myField" => "test2", "files" => []],
+                    ["myField" => "test3", "files" => []],
+                ],
+            ],
+        ];
+    }
+
+    /**
+     * @dataProvider provideFilterAndSortWithArgsOnlyRead
+     */
+    public function testFilterAndSortWithArgsOnlyRead(string $fixture, string $query, array $args, array $expected)
+    {
+        $author = Member::create(['FirstName' => 'tester1']);
+        $author->write();
+
+        $author2 = Member::create(['FirstName' => 'tester2']);
+        $author2->write();
+
+        $dataObject1 = DataObjectFake::create(['MyField' => 'test1', 'AuthorID' => $author->ID]);
+        $dataObject1->write();
+
+        $dataObject2 = DataObjectFake::create(['MyField' => 'test2', 'AuthorID' => $author2->ID]);
+        $dataObject2->write();
+
+        $dataObject3 = DataObjectFake::create(['MyField' => 'test3', 'AuthorID' => $author2->ID]);
+        $dataObject3->write();
+
+        $file1 = File::create(['Title' => 'file1', 'Name' => 'asc_name']);
+        $file1->ParentID = 1;
+        $file1->write();
+
+        $file2 = File::create(['Title' => 'file2', 'Name' => 'desc_name']);
+        $file2->ParentID = 1;
+        $file2->write();
+
+        $file3 = File::create(['Title' => 'file3', 'Name' => 'asc_name']);
+        $file3->ParentID = 2;
+        $file3->write();
+
+        $file4 = File::create(['Title' => 'file4', 'Name' => 'desc_name']);
+        $file4->ParentID = 2;
+        $file4->write();
+
+        $dataObject1->Files()->add($file1);
+        $dataObject1->Files()->add($file2);
+        $dataObject1->Files()->add($file3);
+        $dataObject1->Files()->add($file4);
+
+        $factory = new TestSchemaBuilder(['_' . __FUNCTION__ . $fixture]);
+        $schema = $this->createSchema($factory);
+
+        $result = $this->querySchema($schema, $query, $args);
+        $this->assertSuccess($result);
+        $records = $result['data']['readDataObjectFakes']['nodes'] ?? [];
+
+        // We intentionally don't check the sort order, as it's expected it may not match:
+        // See https://github.com/silverstripe/silverstripe-graphql/issues/573
+        $this->assertEqualsCanonicalizing($expected, $records);
+    }
+
     public function testAggregateProperties()
     {
         $file1 = File::create(['Title' => '1']);

tests/Schema/_testFilterAndSortWithArgsOnlyRead_QuerySort/models.yml

@@ -0,0 +1,12 @@
+SilverStripe\GraphQL\Tests\Fake\DataObjectFake:
+  operations:
+    read:
+      plugins:
+        sort:
+          before: paginateList
+  fields:
+    myField: true
+    author:
+      fields:
+        id: true
+        firstName: true

tests/Schema/_testFilterAndSortWithArgsOnlyRead_SortPlugin/models.yml

@@ -0,0 +1,22 @@
+SilverStripe\GraphQL\Tests\Fake\DataObjectFake:
+  operations:
+    read:
+      plugins:
+        sort:
+          before: paginateList
+  fields:
+    myField: true
+    AuthorID: true
+    author:
+      fields:
+        firstName: true
+    files:
+      fields:
+        title: true
+      plugins:
+        sorter:
+          fields:
+            id: true
+            title: true
+            name: true
+            ParentID: true