From f2af7e09925c72f45e59f0ef56d146506967b5ba Mon Sep 17 00:00:00 2001 From: Loz Calver 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 | 316 +++++++++++------- .../models.yml | 12 + .../models.yml | 22 ++ 4 files changed, 242 insertions(+), 115 deletions(-) create mode 100644 tests/Schema/_testFilterAndSortWithArgsOnlyRead_QuerySort/models.yml create mode 100644 tests/Schema/_testFilterAndSortWithArgsOnlyRead_SortPlugin/models.yml 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..5c81b1c9 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' => << [ - ["myField" => "test2", "author" => ["firstName" => "tester2"]], - ["myField" => "test3", "author" => ["firstName" => "tester2"]], - ["myField" => "test1", "author" => ["firstName" => "tester1"]], - ], - ], - 'read with sorter files title DESC' => [ - 'fixture' => '_SortPlugin', - 'query' => << [ - ["myField" => "test1", "files" => [["title" => "file4"], ["title" => "file3"], ["title" => "file2"], ["title" => "file1"]]], - ["myField" => "test2", "files" => []], - ["myField" => "test3", "files" => []], + 'read with sort' => [ + 'fixture' => '_QuerySort', + 'query' => << [ + ["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' => << [ - ["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' => << [ + ["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' => << [ - ["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' => << [ + ["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' => << [ - ["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' => << [ + ["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' => << [ - ["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' => << [ + ["myField" => "test1", "files" => [["title" => "file3"], ["title" => "file1"], ["title" => "file4"], ["title" => "file2"]]], + ["myField" => "test2", "files" => []], + ["myField" => "test3", "files" => []], + ], ], - ], ]; } @@ -847,6 +827,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' => << [ + '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' => << [ + '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']); diff --git a/tests/Schema/_testFilterAndSortWithArgsOnlyRead_QuerySort/models.yml b/tests/Schema/_testFilterAndSortWithArgsOnlyRead_QuerySort/models.yml new file mode 100644 index 00000000..f50a9d5e --- /dev/null +++ b/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 diff --git a/tests/Schema/_testFilterAndSortWithArgsOnlyRead_SortPlugin/models.yml b/tests/Schema/_testFilterAndSortWithArgsOnlyRead_SortPlugin/models.yml new file mode 100644 index 00000000..8a6cb339 --- /dev/null +++ b/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