From 5781c46614ac2d40d025c71837f3dbc090361e53 Mon Sep 17 00:00:00 2001 From: Sabina Talipova Date: Thu, 23 Nov 2023 09:52:47 +1300 Subject: [PATCH] FIX QuerySort::sort method should support sorting for multi arguments --- composer.json | 2 +- src/Schema/DataObject/Plugin/QuerySort.php | 73 ++++- tests/Schema/IntegrationTest.php | 280 ++++++++++++------ .../models.yml | 12 + 4 files changed, 266 insertions(+), 101 deletions(-) create mode 100644 tests/Schema/_testFilterAndSortOnlyRead_QuerySort/models.yml diff --git a/composer.json b/composer.json index 6d1a4452..5f3f6745 100755 --- a/composer.json +++ b/composer.json @@ -7,7 +7,7 @@ "php": "^7.4 || ^8.0", "silverstripe/framework": "^4.11", "silverstripe/vendor-plugin": "^1.0", - "webonyx/graphql-php": "^14.0", + "webonyx/graphql-php": "^14.11", "silverstripe/event-dispatcher": "^0.1.2", "guzzlehttp/guzzle": "^7.3", "guzzlehttp/psr7": "^2", diff --git a/src/Schema/DataObject/Plugin/QuerySort.php b/src/Schema/DataObject/Plugin/QuerySort.php index 1647bc34..4085e2af 100644 --- a/src/Schema/DataObject/Plugin/QuerySort.php +++ b/src/Schema/DataObject/Plugin/QuerySort.php @@ -18,6 +18,7 @@ use Closure; use SilverStripe\ORM\Sortable; use Exception; +use GraphQL\Type\Definition\ResolveInfo; /** * Adds a sort parameter to a DataObject query @@ -87,7 +88,6 @@ protected function buildAllFieldsConfig(ModelType $modelType, Schema $schema, in return $filters; } - /** * @param array $context * @return Closure @@ -96,12 +96,21 @@ public static function sort(array $context): closure { $fieldName = $context['fieldName']; $rootType = $context['rootType']; - return function (?Sortable $list, array $args, array $context) use ($fieldName, $rootType) { + return function (?Sortable $list, array $args, array $context, ResolveInfo $info) use ($fieldName, $rootType) { if ($list === null) { return null; } - $filterArgs = $args[$fieldName] ?? []; - $paths = NestedInputBuilder::buildPathsFromArgs($filterArgs); + + if (!isset($args[$fieldName])) { + return $list; + } + + $sortArgs = static::getSortArgs($info, $args, $rootType, $fieldName); + $paths = NestedInputBuilder::buildPathsFromArgs($sortArgs); + if (empty($paths)) { + return $list; + } + $schemaContext = SchemaConfigProvider::get($context); if (!$schemaContext) { throw new Exception(sprintf( @@ -111,6 +120,7 @@ public static function sort(array $context): closure )); } + $normalisedPaths = []; foreach ($paths as $path => $value) { $normalised = $schemaContext->mapPath($rootType, $path); Schema::invariant( @@ -120,13 +130,64 @@ public static function sort(array $context): closure $path, $rootType ); - $list = $list->sort($normalised, $value); + + $normalisedPaths[$normalised] = $value; } - return $list; + return $list->sort($normalisedPaths); }; } + private static function getSortArgs(ResolveInfo $info, array $args, string $rootType, string $fieldName): array + { + $sortArgs = []; + $sortOrder = self::getSortOrder($info, $fieldName); + + foreach ($sortOrder as $orderName) { + if (!isset($args[$fieldName][$orderName])) { + continue; + } + $sortArgs[$orderName] = $args[$fieldName][$orderName]; + unset($args[$fieldName][$orderName]); + } + + return array_merge($sortArgs, $args[$fieldName]); + } + + /** + * Gets the original order of fields to be sorted based on the query args order. + * + * This is necessary because the underlying GraphQL implementation we're using ignores the + * order of query args, and uses the order that fields are defined in the schema instead. + */ + private static function getSortOrder(ResolveInfo $info, string $fieldName) + { + $relevantNode = $info->fieldDefinition->getName(); + + // Find the query field node that matches the schema + foreach ($info->fieldNodes as $node) { + if ($node->name->value !== $relevantNode) { + continue; + } + + // Find the sort arg + foreach ($node->arguments as $arg) { + if ($arg->name->value !== $fieldName) { + continue; + } + + // Get the sort order from the query + $sortOrder = []; + foreach ($arg->value->fields as $field) { + $sortOrder[] = $field->name->value; + } + return $sortOrder; + } + } + + return []; + } + /** * @param NestedInputBuilder $builder */ diff --git a/tests/Schema/IntegrationTest.php b/tests/Schema/IntegrationTest.php index 372835cf..2afacf9c 100644 --- a/tests/Schema/IntegrationTest.php +++ b/tests/Schema/IntegrationTest.php @@ -415,7 +415,100 @@ public function testNestedFieldDefinitions() $this->assertMissingField($result, 'title'); } - public function testFilterAndSort() + public function provideFilterAndSort(): array + { + return [ + [ + 'query' => << 'id', + 'placeholderRecord' => 'fake1', + 'expected' => 'fake1', + ], + [ + 'query' => << 'id', + 'placeholderRecord' => 'fake1', + 'expected' => 'fake2', + ], + [ + 'query' => << 'myField', + 'placeholderRecord' => '', + 'expected' => 'test1', + ], + [ + 'query' => << 'myField', + 'placeholderRecord' => '', + 'expected' => 'test2', + ], + [ + 'query' => << 'myField', + 'placeholderRecord' => '', + 'expected' => 'test1', + ], + [ + 'query' => << 'myField', + 'placeholderRecord' => '', + 'expected' => 'test4', + ], + [ + 'query' => << 'myField', + 'placeholderRecord' => 'fake3', + 'expected' => 'test4', + ], + ]; + } + + /** + * @dataProvider provideFilterAndSort + */ + public function testFilterAndSort(string $query, string $testAgainst, string $placeholderRecord, string $expected): void { $dir = '_' . __FUNCTION__; @@ -434,6 +527,9 @@ public function testFilterAndSort() $dataObject3 = DataObjectFake::create(['MyField' => 'test3', 'AuthorID' => $author2->ID]); $dataObject3->write(); + $dataObject4 = DataObjectFake::create(['MyField' => 'test4', 'AuthorID' => $author1->ID]); + $dataObject4->write(); + $file1 = File::create(['Title' => 'file1']); $file1->write(); @@ -446,104 +542,45 @@ public function testFilterAndSort() $id1 = $dataObject1->ID; $id2 = $dataObject2->ID; $id3 = $dataObject3->ID; + $id4 = $dataObject4->ID; + + if ($testAgainst === 'id') { + switch ($expected) { + case 'fake1': + $expected = $id1; + break; + case 'fake2': + $expected = $id2; + break; + case 'fake3': + $expected = $id3; + break; + default: + throw new LogicException("No ID known for '$expected'"); + } + } - $schema = $this->createSchema(new TestSchemaBuilder([$dir])); - - $query = <<querySchema($schema, $query); - $this->assertSuccess($result); - $this->assertResult('readOneDataObjectFake.id', $id1, $result); - - $query = <<querySchema($schema, $query); - $this->assertSuccess($result); - $this->assertResult('readOneDataObjectFake.id', $id2, $result); - - $query = <<querySchema($schema, $query); - $this->assertSuccess($result); - $this->assertResult('readOneDataObjectFake.myField', 'test1', $result); - - $query = <<querySchema($schema, $query); - $this->assertSuccess($result); - $this->assertResult('readOneDataObjectFake.myField', 'test2', $result); + $placeholderID = null; + switch ($placeholderRecord) { + case 'fake1': + $placeholderID = $id1; + break; + case 'fake2': + $placeholderID = $id2; + break; + case 'fake3': + $placeholderID = $id3; + break; + } + if ($placeholderID) { + $query = str_replace('_ID_PLACEHOLDER_', (string) $placeholderID, $query); + } - $query = <<querySchema($schema, $query); - $this->assertSuccess($result); - $this->assertResult('readOneDataObjectFake.myField', 'test3', $result); + $schema = $this->createSchema(new TestSchemaBuilder([$dir])); - $query = <<querySchema($schema, $query); $this->assertSuccess($result); - $this->assertResult('readOneDataObjectFake.myField', 'test2', $result); - - $query = <<querySchema($schema, $query); - // Nested fields aren't working. Needs refactoring. -// $this->assertSuccess($result); -// $this->assertResult('readOneDataObjectFake.author.firstName', 'tester1', $result); - - $query = <<querySchema($schema, $query); - -// $this->assertSuccess($result); -// $this->assertNull($result['data']['readOneDataObjectFake']); + $this->assertResult("readOneDataObjectFake.{$testAgainst}", $expected, $result); } @@ -631,6 +668,61 @@ public function testFieldAliases() $this->assertResult('readOneDataObjectFake.author', null, $result); } + public function provideFilterAndSortOnlyRead(): array + { + return [ + 'read with sort' => [ + 'fixture' => '_QuerySort', + 'query' => << '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(); + + + $factory = new TestSchemaBuilder(['_' . __FUNCTION__ . $fixture]); + $schema = $this->createSchema($factory); + + $result = $this->querySchema($schema, $query); + $this->assertSuccess($result); + $records = $result['data']['readDataObjectFakes']['nodes'] ?? []; + $this->assertResults([ + ["myField" => "test2", "author" => ["firstName" => "tester2"]], + ["myField" => "test3", "author" => ["firstName" => "tester2"]], + ["myField" => "test1", "author" => ["firstName" => "tester1"]], + ], $records); + } + public function testAggregateProperties() { $file1 = File::create(['Title' => '1']); diff --git a/tests/Schema/_testFilterAndSortOnlyRead_QuerySort/models.yml b/tests/Schema/_testFilterAndSortOnlyRead_QuerySort/models.yml new file mode 100644 index 00000000..f50a9d5e --- /dev/null +++ b/tests/Schema/_testFilterAndSortOnlyRead_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