From d6e7735562538d1432e968cf3ba1491419a2ed4a Mon Sep 17 00:00:00 2001 From: Loz Calver Date: Wed, 8 May 2024 09:21:01 +0100 Subject: [PATCH] FIX: Don't attempt to correct sort when passed as an argument (closes #571) --- _config/logging.yml | 6 + src/Schema/Traits/SortTrait.php | 11 ++ tests/Schema/IntegrationTest.php | 106 ++++++++++++++++++ .../models.yml | 12 ++ .../models.yml | 22 ++++ 5 files changed, 157 insertions(+) create mode 100644 tests/Schema/_testFilterAndSortWithArgsOnlyRead_QuerySort/models.yml create mode 100644 tests/Schema/_testFilterAndSortWithArgsOnlyRead_SortPlugin/models.yml diff --git a/_config/logging.yml b/_config/logging.yml index 097fc839..e19a72a8 100644 --- a/_config/logging.yml +++ b/_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' diff --git a/src/Schema/Traits/SortTrait.php b/src/Schema/Traits/SortTrait.php index 95f6d0fb..5fba7503 100644 --- a/src/Schema/Traits/SortTrait.php +++ b/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) { diff --git a/tests/Schema/IntegrationTest.php b/tests/Schema/IntegrationTest.php index c3cab186..c563abe4 100644 --- a/tests/Schema/IntegrationTest.php +++ b/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' => << [ + '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