From 51471a4282229de1ee1ba321a755ac83bfbe3fa0 Mon Sep 17 00:00:00 2001
From: Loz Calver <kinglozzer@gmail.com>
Date: Thu, 8 Feb 2024 10:21:01 +0000
Subject: [PATCH] FIX: Don't attempt to correct sort when passed as an argument
 (closes #571)

---
 src/Schema/Traits/SortTrait.php  |   7 +
 tests/Schema/IntegrationTest.php | 323 ++++++++++++++++++++-----------
 2 files changed, 215 insertions(+), 115 deletions(-)

diff --git a/src/Schema/Traits/SortTrait.php b/src/Schema/Traits/SortTrait.php
index 95f6d0fb..32fb0d45 100644
--- a/src/Schema/Traits/SortTrait.php
+++ b/src/Schema/Traits/SortTrait.php
@@ -2,6 +2,7 @@
 
 namespace SilverStripe\GraphQL\Schema\Traits;
 
+use GraphQL\Language\AST\ObjectValueNode;
 use GraphQL\Type\Definition\ResolveInfo;
 
 trait SortTrait
@@ -44,6 +45,12 @@ 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) {
+                    continue;
+                }
+
                 // Get the sort order from the query
                 $sortOrder = [];
                 foreach ($arg->value->fields as $field) {
diff --git a/tests/Schema/IntegrationTest.php b/tests/Schema/IntegrationTest.php
index c3cab186..05a264d3 100644
--- a/tests/Schema/IntegrationTest.php
+++ b/tests/Schema/IntegrationTest.php
@@ -674,126 +674,106 @@ public function testFieldAliases()
     public function provideFilterAndSortOnlyRead(): array
     {
         return [
-          'read with sort' => [
-            'fixture' => '_QuerySort',
-            'query' => <<<GRAPHQL
-            query {
-              readDataObjectFakes(sort: { author: { id: DESC }, myField: ASC }) {
-                nodes {
-                  myField
-                  author {
-                    firstName
-                  }
-                }
-              }
-            }
-            GRAPHQL,
-            'expected' => [
-              ["myField" => "test2", "author" => ["firstName" => "tester2"]],
-              ["myField" => "test3", "author" => ["firstName" => "tester2"]],
-              ["myField" => "test1", "author" => ["firstName" => "tester1"]],
-            ],
-          ],
-          'read with sorter files title DESC' => [
-            'fixture' => '_SortPlugin',
-            'query' => <<<GRAPHQL
-            query {
-              readDataObjectFakes(sort: { myField: ASC }) {
-                nodes {
-                  myField
-                  files(sort: { title: DESC }) {
-                    title
-                  }
-                }
-              }
-            }
-            GRAPHQL,
-            'expected' => [
-              ["myField" => "test1", "files" => [["title" => "file4"], ["title" => "file3"], ["title" => "file2"], ["title" => "file1"]]],
-              ["myField" => "test2", "files" => []],
-              ["myField" => "test3", "files" => []],
+            'read with sort' => [
+                'fixture' => '_QuerySort',
+                'query' => <<<GRAPHQL
+query {
+  readDataObjectFakes(sort: { author: { id: DESC }, myField: ASC }) {
+    nodes {
+      myField
+      author {
+        firstName
+      }
+    }
+  }
+}
+GRAPHQL,
+                'expected' => [
+                    ["myField" => "test2", "author" => ["firstName" => "tester2"]],
+                    ["myField" => "test3", "author" => ["firstName" => "tester2"]],
+                    ["myField" => "test1", "author" => ["firstName" => "tester1"]],
+                ],
             ],
-          ],
-          'read with sorter files ParentID ACS, name DESC' => [
-            'fixture' => '_SortPlugin',
-            'query' => <<<GRAPHQL
-            query {
-              readDataObjectFakes(sort: { myField: ASC }) {
-                nodes {
-                  myField
-                  files(sort: { ParentID: ASC, name: DESC }) {
-                    title
-                  }
-                }
-              }
-            }
-            GRAPHQL,
-            'expected' => [
-              ["myField" => "test1", "files" => [["title" => "file2"],["title" => "file1"], ["title" => "file4"],["title" => "file3"]]],
-              ["myField" => "test2", "files" => []],
-              ["myField" => "test3", "files" => []],
+            'read with sorter files title DESC' => [
+                'fixture' => '_SortPlugin',
+                'query' => <<<GRAPHQL
+query {
+  readDataObjectFakes(sort: { myField: ASC }) {
+    nodes {
+      myField
+      files(sort: { title: DESC }) {
+        title
+      }
+    }
+  }
+}
+GRAPHQL,
+                'expected' => [
+                    ["myField" => "test1", "files" => [["title" => "file4"], ["title" => "file3"], ["title" => "file2"], ["title" => "file1"]]],
+                    ["myField" => "test2", "files" => []],
+                    ["myField" => "test3", "files" => []],
+                ],
             ],
-          ],
-          'read with sorter files ParentID DESC, name ASC' => [
-            'fixture' => '_SortPlugin',
-            'query' => <<<GRAPHQL
-            query {
-              readDataObjectFakes(sort: { myField: ASC }) {
-                nodes {
-                  myField
-                  files(sort: { ParentID: DESC, name: ASC }) {
-                    title
-                  }
-                }
-              }
-            }
-            GRAPHQL,
-            'expected' => [
-              ["myField" => "test1", "files" => [["title" => "file3"],["title" => "file4"], ["title" => "file1"],["title" => "file2"]]],
-              ["myField" => "test2", "files" => []],
-              ["myField" => "test3", "files" => []],
+            'read with sorter files ParentID ACS, name DESC' => [
+                'fixture' => '_SortPlugin',
+                'query' => <<<GRAPHQL
+query {
+  readDataObjectFakes(sort: { myField: ASC }) {
+    nodes {
+      myField
+      files(sort: { ParentID: ASC, name: DESC }) {
+        title
+      }
+    }
+  }
+}
+GRAPHQL,
+                'expected' => [
+                    ["myField" => "test1", "files" => [["title" => "file2"], ["title" => "file1"], ["title" => "file4"], ["title" => "file3"]]],
+                    ["myField" => "test2", "files" => []],
+                    ["myField" => "test3", "files" => []],
+                ],
             ],
-          ],
-          'read with sorter files name ASC, ParentID DESC' => [
-            'fixture' => '_SortPlugin',
-            'query' => <<<GRAPHQL
-            query {
-              readDataObjectFakes(sort: { myField: ASC }) {
-                nodes {
-                  myField
-                  files(sort: { name: ASC, ParentID: DESC }) {
-                    title
-                  }
-                }
-              }
-            }
-            GRAPHQL,
-            'expected' => [
-              ["myField" => "test1", "files" => [["title" => "file3"],["title" => "file1"], ["title" => "file4"],["title" => "file2"]]],
-              ["myField" => "test2", "files" => []],
-              ["myField" => "test3", "files" => []],
+            'read with sorter files ParentID DESC, name ASC' => [
+                'fixture' => '_SortPlugin',
+                'query' => <<<GRAPHQL
+query {
+  readDataObjectFakes(sort: { myField: ASC }) {
+    nodes {
+      myField
+      files(sort: { ParentID: DESC, name: ASC }) {
+        title
+      }
+    }
+  }
+}
+GRAPHQL,
+                'expected' => [
+                    ["myField" => "test1", "files" => [["title" => "file3"], ["title" => "file4"], ["title" => "file1"], ["title" => "file2"]]],
+                    ["myField" => "test2", "files" => []],
+                    ["myField" => "test3", "files" => []],
+                ],
             ],
-          ],
-          'read with sorter files name DESC, ParentID ASC' => [
-            'fixture' => '_SortPlugin',
-            'query' => <<<GRAPHQL
-            query {
-              readDataObjectFakes(sort: { myField: ASC }) {
-                nodes {
-                  myField
-                  files(sort: { name: DESC, ParentID: ASC }) {
-                    title
-                  }
-                }
-              }
-            }
-            GRAPHQL,
-            'expected' => [
-              ["myField" => "test1", "files" => [["title" => "file2"],[ "title" => "file4"],["title" => "file1"],["title" => "file3"]]],
-              ["myField" => "test2", "files" => []],
-              ["myField" => "test3", "files" => []],
+            'read with sorter files name ASC, ParentID DESC' => [
+                'fixture' => '_SortPlugin',
+                'query' => <<<GRAPHQL
+query {
+  readDataObjectFakes(sort: { myField: ASC }) {
+    nodes {
+      myField
+      files(sort: { name: ASC, ParentID: DESC }) {
+        title
+      }
+    }
+  }
+}
+GRAPHQL,
+                'expected' => [
+                    ["myField" => "test1", "files" => [["title" => "file3"], ["title" => "file1"], ["title" => "file4"], ["title" => "file2"]]],
+                    ["myField" => "test2", "files" => []],
+                    ["myField" => "test3", "files" => []],
+                ],
             ],
-          ],
         ];
     }
 
@@ -847,6 +827,119 @@ 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
+        usort($expected, function($a, $b) {
+            return strcmp($a['myField'], $b['myField']);
+        });
+        usort($records, function($a, $b) {
+            return strcmp($a['myField'], $b['myField']);
+        });
+
+        $this->assertResults($expected, $records);
+    }
+
     public function testAggregateProperties()
     {
         $file1 = File::create(['Title' => '1']);