From e3b8407ffbb658162644f7895bface9e8dfc3e1f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20N=C3=A9grier?= Date: Tue, 17 Dec 2019 10:30:12 +0100 Subject: [PATCH 1/5] Adding failing test for duplicate queries --- .../TestControllerWithDuplicateQuery1.php | 17 +++++++++++++ .../TestControllerWithDuplicateQuery2.php | 17 +++++++++++++ tests/SchemaFactoryTest.php | 25 +++++++++++++++++++ 3 files changed, 59 insertions(+) create mode 100644 tests/Fixtures/DuplicateQueriesInTwoControllers/TestControllerWithDuplicateQuery1.php create mode 100644 tests/Fixtures/DuplicateQueriesInTwoControllers/TestControllerWithDuplicateQuery2.php diff --git a/tests/Fixtures/DuplicateQueriesInTwoControllers/TestControllerWithDuplicateQuery1.php b/tests/Fixtures/DuplicateQueriesInTwoControllers/TestControllerWithDuplicateQuery1.php new file mode 100644 index 0000000000..48a826c994 --- /dev/null +++ b/tests/Fixtures/DuplicateQueriesInTwoControllers/TestControllerWithDuplicateQuery1.php @@ -0,0 +1,17 @@ +setAuthenticationService(new VoidAuthenticationService()) + ->setAuthorizationService(new VoidAuthorizationService()) + ->addControllerNamespace('TheCodingMachine\\GraphQLite\\Fixtures\\DuplicateQueriesInTwoControllers') + ->addTypeNamespace('TheCodingMachine\\GraphQLite\\Fixtures\\Integration'); + + $this->expectException(DuplicateMappingException::class); + $schema = $factory->createSchema(); + $queryString = ' + query { + duplicateQuery + } + '; + GraphQL::executeQuery( + $schema, + $queryString + ); + } } From 7d480ba4eb331fe4a15027a78ba3d69f00ed06eb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20N=C3=A9grier?= Date: Tue, 17 Dec 2019 11:28:55 +0100 Subject: [PATCH 2/5] Adding query duplicate detection in multiple classes. --- src/AggregateControllerQueryProvider.php | 45 +++++++++++++++++++++-- src/FieldsBuilder.php | 18 +++------ src/Mappers/DuplicateMappingException.php | 7 +++- tests/FieldsBuilderTest.php | 26 ++++++------- tests/SchemaFactoryTest.php | 2 + 5 files changed, 68 insertions(+), 30 deletions(-) diff --git a/src/AggregateControllerQueryProvider.php b/src/AggregateControllerQueryProvider.php index ff5225b748..0a4f1d2d37 100644 --- a/src/AggregateControllerQueryProvider.php +++ b/src/AggregateControllerQueryProvider.php @@ -6,7 +6,13 @@ use GraphQL\Type\Definition\FieldDefinition; use Psr\Container\ContainerInterface; +use TheCodingMachine\GraphQLite\Mappers\DuplicateMappingException; +use function array_filter; +use function array_keys; use function array_merge; +use function array_sum; +use function array_values; +use function array_walk; /** * A query provider that looks into all controllers of your application to fetch queries. @@ -40,10 +46,10 @@ public function getQueries(): array foreach ($this->controllers as $controllerName) { $controller = $this->controllersContainer->get($controllerName); - $queryList = array_merge($queryList, $this->fieldsBuilder->getQueries($controller)); + $queryList[$controllerName] = $this->fieldsBuilder->getQueries($controller); } - return $queryList; + return $this->flattenList($queryList); } /** @@ -55,9 +61,40 @@ public function getMutations(): array foreach ($this->controllers as $controllerName) { $controller = $this->controllersContainer->get($controllerName); - $mutationList = array_merge($mutationList, $this->fieldsBuilder->getMutations($controller)); + $mutationList[$controllerName] = $this->fieldsBuilder->getMutations($controller); } - return $mutationList; + return $this->flattenList($mutationList); + } + + /** + * @param array> $list + * @return array + */ + private function flattenList(array $list): array + { + if (empty($list)) { + return []; + } + + $flattenedList = array_merge(...array_values($list)); + + // Quick check: are there duplicates? If so, the count of the flattenedList is != from the sum of the count of lists. + if (count($flattenedList) === array_sum(array_map('count', $list))) { + return $flattenedList; + } + + // We have an issue, let's detect the duplicate + $duplicates = array_intersect_key(...array_values($list)); + // Let's display an error from the first one. + $firstDuplicate = reset($duplicates); + + $duplicateName = $firstDuplicate->name; + + $classes = array_keys(array_filter($list, function(array $fields) use ($duplicateName) { + return isset($fields[$duplicateName]); + })); + + throw DuplicateMappingException::createForQueryInTwoControllers($classes[0], $classes[1], $duplicateName); } } diff --git a/src/FieldsBuilder.php b/src/FieldsBuilder.php index a657c923a3..15aec58a9f 100644 --- a/src/FieldsBuilder.php +++ b/src/FieldsBuilder.php @@ -81,7 +81,7 @@ public function __construct( } /** - * @return FieldDefinition[] + * @return array * * @throws ReflectionException */ @@ -91,7 +91,7 @@ public function getQueries(object $controller): array } /** - * @return FieldDefinition[] + * @return array * * @throws ReflectionException */ @@ -118,10 +118,7 @@ public function getFields(object $controller): array $fieldsFromSourceFields = $this->getQueryFieldsFromSourceFields($sourceFields, $refClass); - $fields = []; - foreach ($fieldAnnotations as $field) { - $fields[$field->name] = $field; - } + $fields = $fieldAnnotations; foreach ($fieldsFromSourceFields as $field) { $fields[$field->name] = $field; } @@ -147,10 +144,7 @@ public function getSelfFields(string $className): array $fieldsFromSourceFields = $this->getQueryFieldsFromSourceFields($sourceFields, $refClass); - $fields = []; - foreach ($fieldAnnotations as $field) { - $fields[$field->name] = $field; - } + $fields = $fieldAnnotations; foreach ($fieldsFromSourceFields as $field) { $fields[$field->name] = $field; } @@ -199,7 +193,7 @@ public function getParametersForDecorator(ReflectionMethod $refMethod): array * @param object|class-string $controller The controller instance, or the name of the source class name * @param bool $injectSource Whether to inject the source object or not as the first argument. True for @Field (unless @Type has no class attribute), false for @Query and @Mutation * - * @return FieldDefinition[] + * @return array * * @throws ReflectionException */ @@ -335,7 +329,7 @@ public function handle(QueryFieldDescriptor $fieldDescriptor): ?FieldDefinition }*/ } - return array_values($queryList); + return $queryList; } /** diff --git a/src/Mappers/DuplicateMappingException.php b/src/Mappers/DuplicateMappingException.php index 99270d24a8..714ccd16b1 100644 --- a/src/Mappers/DuplicateMappingException.php +++ b/src/Mappers/DuplicateMappingException.php @@ -26,6 +26,11 @@ public static function createForTypeName(string $type, string $sourceClass1, str public static function createForQuery(string $sourceClass, string $queryName): self { - throw new self(sprintf("The query '%s' is duplicate in class '%s'", $queryName, $sourceClass)); + throw new self(sprintf("The query/mutation '%s' is declared twice in class '%s'", $queryName, $sourceClass)); + } + + public static function createForQueryInTwoControllers(string $sourceClass1, string $sourceClass2, string $queryName): self + { + throw new self(sprintf("The query/mutation '%s' is declared twice: in class '%s' and in class '%s'", $queryName, $sourceClass1, $sourceClass2)); } } diff --git a/tests/FieldsBuilderTest.php b/tests/FieldsBuilderTest.php index b08dfc330e..81976216b1 100644 --- a/tests/FieldsBuilderTest.php +++ b/tests/FieldsBuilderTest.php @@ -86,7 +86,7 @@ public function testQueryProvider(): void $queries = $queryProvider->getQueries($controller); $this->assertCount(7, $queries); - $usersQuery = $queries[0]; + $usersQuery = $queries['test']; $this->assertSame('test', $usersQuery->name); $this->assertCount(10, $usersQuery->args); @@ -141,7 +141,7 @@ public function testMutations(): void $mutations = $queryProvider->getMutations($controller); $this->assertCount(1, $mutations); - $mutation = $mutations[0]; + $mutation = $mutations['mutation']; $this->assertSame('mutation', $mutation->name); $resolve = $mutation->resolveFn; @@ -190,7 +190,7 @@ public function test($typeHintInDocBlock) $queries = $queryProvider->getQueries($controller); $this->assertCount(1, $queries); - $query = $queries[0]; + $query = $queries['test']; $this->assertInstanceOf(NonNull::class, $query->args[0]->getType()); $this->assertInstanceOf(IntType::class, $query->args[0]->getType()->getWrappedType()); @@ -207,7 +207,7 @@ public function testQueryProviderWithFixedReturnType(): void $queries = $queryProvider->getQueries($controller); $this->assertCount(7, $queries); - $fixedQuery = $queries[1]; + $fixedQuery = $queries['testFixReturnType']; $this->assertInstanceOf(IDType::class, $fixedQuery->getType()); } @@ -221,7 +221,7 @@ public function testQueryProviderWithComplexFixedReturnType(): void $queries = $queryProvider->getQueries($controller); $this->assertCount(7, $queries); - $fixedQuery = $queries[6]; + $fixedQuery = $queries['testFixComplexReturnType']; $this->assertInstanceOf(NonNull::class, $fixedQuery->getType()); $this->assertInstanceOf(ListOfType::class, $fixedQuery->getType()->getWrappedType()); @@ -237,7 +237,7 @@ public function testNameFromAnnotation(): void $queries = $queryProvider->getQueries($controller); - $query = $queries[2]; + $query = $queries['nameFromAnnotation']; $this->assertSame('nameFromAnnotation', $query->name); } @@ -410,7 +410,7 @@ public function testQueryProviderWithIterableClass(): void $queries = $queryProvider->getQueries($controller); $this->assertCount(7, $queries); - $iterableQuery = $queries[3]; + $iterableQuery = $queries['arrayObject']; $this->assertSame('arrayObject', $iterableQuery->name); $this->assertInstanceOf(NonNull::class, $iterableQuery->getType()); @@ -427,7 +427,7 @@ public function testQueryProviderWithIterable(): void $queries = $queryProvider->getQueries(new TestController()); $this->assertCount(7, $queries); - $iterableQuery = $queries[4]; + $iterableQuery = $queries['iterable']; $this->assertSame('iterable', $iterableQuery->name); $this->assertInstanceOf(NonNull::class, $iterableQuery->getType()); @@ -453,7 +453,7 @@ public function testQueryProviderWithUnion(): void $queries = $queryProvider->getQueries($controller); $this->assertCount(7, $queries); - $unionQuery = $queries[5]; + $unionQuery = $queries['union']; $this->assertInstanceOf(NonNull::class, $unionQuery->getType()); $this->assertInstanceOf(UnionType::class, $unionQuery->getType()->getWrappedType()); @@ -539,7 +539,7 @@ public function testFailWith(): void $queries = $queryProvider->getQueries($controller); $this->assertCount(1, $queries); - $query = $queries[0]; + $query = $queries['testFailWith']; $this->assertSame('testFailWith', $query->name); $resolve = $query->resolveFn; @@ -621,7 +621,7 @@ public function testMissingArgument(): void $queries = $queryProvider->getQueries($controller); $this->assertCount(7, $queries); - $usersQuery = $queries[0]; + $usersQuery = $queries['test']; $context = []; $resolve = $usersQuery->resolveFn; @@ -686,7 +686,7 @@ public function testSecurityBadQuery(): void $queries = $queryProvider->getQueries($controller); $this->assertCount(1, $queries); - $query = $queries[0]; + $query = $queries['testBadSecurity']; $this->assertSame('testBadSecurity', $query->name); $resolve = $query->resolveFn; @@ -705,7 +705,7 @@ public function testQueryProviderWithNullableArray(): void $queries = $queryProvider->getQueries($controller); $this->assertCount(1, $queries); - $usersQuery = $queries[0]; + $usersQuery = $queries['test']; $this->assertSame('test', $usersQuery->name); $this->assertInstanceOf(NonNull::class, $usersQuery->args[0]->getType()); diff --git a/tests/SchemaFactoryTest.php b/tests/SchemaFactoryTest.php index 6d562c0d7f..c7e014ee94 100644 --- a/tests/SchemaFactoryTest.php +++ b/tests/SchemaFactoryTest.php @@ -194,6 +194,7 @@ public function testDuplicateQueryException(): void ->addTypeNamespace('TheCodingMachine\\GraphQLite\\Fixtures\\Integration'); $this->expectException(DuplicateMappingException::class); + $this->expectErrorMessage("The query/mutation 'duplicateQuery' is declared twice in class 'TheCodingMachine\\GraphQLite\\Fixtures\\DuplicateQueries\\TestControllerWithDuplicateQuery"); $schema = $factory->createSchema(); $queryString = ' query { @@ -220,6 +221,7 @@ public function testDuplicateQueryInTwoControllersException(): void ->addTypeNamespace('TheCodingMachine\\GraphQLite\\Fixtures\\Integration'); $this->expectException(DuplicateMappingException::class); + $this->expectErrorMessage("The query/mutation 'duplicateQuery' is declared twice: in class 'TheCodingMachine\\GraphQLite\\Fixtures\\DuplicateQueriesInTwoControllers\\TestControllerWithDuplicateQuery1' and in class 'TheCodingMachine\\GraphQLite\\Fixtures\\DuplicateQueriesInTwoControllers\\TestControllerWithDuplicateQuery2"); $schema = $factory->createSchema(); $queryString = ' query { From 45e873c1bdce73f840163e67b8c9c93472f99d24 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20N=C3=A9grier?= Date: Tue, 17 Dec 2019 11:37:51 +0100 Subject: [PATCH 3/5] CS + Stan --- src/AggregateControllerQueryProvider.php | 10 ++++++++-- src/FieldsBuilder.php | 1 - 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/src/AggregateControllerQueryProvider.php b/src/AggregateControllerQueryProvider.php index 0a4f1d2d37..ac47cd4226 100644 --- a/src/AggregateControllerQueryProvider.php +++ b/src/AggregateControllerQueryProvider.php @@ -8,11 +8,15 @@ use Psr\Container\ContainerInterface; use TheCodingMachine\GraphQLite\Mappers\DuplicateMappingException; use function array_filter; +use function array_intersect_key; use function array_keys; +use function array_map; use function array_merge; use function array_sum; use function array_values; -use function array_walk; +use function assert; +use function count; +use function reset; /** * A query provider that looks into all controllers of your application to fetch queries. @@ -69,6 +73,7 @@ public function getMutations(): array /** * @param array> $list + * * @return array */ private function flattenList(array $list): array @@ -88,10 +93,11 @@ private function flattenList(array $list): array $duplicates = array_intersect_key(...array_values($list)); // Let's display an error from the first one. $firstDuplicate = reset($duplicates); + assert($firstDuplicate instanceof FieldDefinition); $duplicateName = $firstDuplicate->name; - $classes = array_keys(array_filter($list, function(array $fields) use ($duplicateName) { + $classes = array_keys(array_filter($list, static function (array $fields) use ($duplicateName) { return isset($fields[$duplicateName]); })); diff --git a/src/FieldsBuilder.php b/src/FieldsBuilder.php index 15aec58a9f..7534f1cf44 100644 --- a/src/FieldsBuilder.php +++ b/src/FieldsBuilder.php @@ -32,7 +32,6 @@ use Webmozart\Assert\Assert; use function array_merge; use function array_shift; -use function array_values; use function get_parent_class; use function is_string; use function ucfirst; From e71571b588495b3e61bdbce6c72f11d5fe0f0bba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20N=C3=A9grier?= Date: Tue, 17 Dec 2019 14:03:03 +0100 Subject: [PATCH 4/5] Sorting classes to get consistent error messages --- src/AggregateControllerQueryProvider.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/AggregateControllerQueryProvider.php b/src/AggregateControllerQueryProvider.php index ac47cd4226..32e47091fc 100644 --- a/src/AggregateControllerQueryProvider.php +++ b/src/AggregateControllerQueryProvider.php @@ -17,6 +17,7 @@ use function assert; use function count; use function reset; +use function sort; /** * A query provider that looks into all controllers of your application to fetch queries. @@ -100,6 +101,7 @@ private function flattenList(array $list): array $classes = array_keys(array_filter($list, static function (array $fields) use ($duplicateName) { return isset($fields[$duplicateName]); })); + sort($classes); throw DuplicateMappingException::createForQueryInTwoControllers($classes[0], $classes[1], $duplicateName); } From 712d88ac3be73a1bd5c24ea4abfb52b3efda698d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20N=C3=A9grier?= Date: Tue, 17 Dec 2019 14:14:39 +0100 Subject: [PATCH 5/5] Fixing test for old PHPUnit --- tests/SchemaFactoryTest.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/SchemaFactoryTest.php b/tests/SchemaFactoryTest.php index c7e014ee94..3f10223410 100644 --- a/tests/SchemaFactoryTest.php +++ b/tests/SchemaFactoryTest.php @@ -194,7 +194,7 @@ public function testDuplicateQueryException(): void ->addTypeNamespace('TheCodingMachine\\GraphQLite\\Fixtures\\Integration'); $this->expectException(DuplicateMappingException::class); - $this->expectErrorMessage("The query/mutation 'duplicateQuery' is declared twice in class 'TheCodingMachine\\GraphQLite\\Fixtures\\DuplicateQueries\\TestControllerWithDuplicateQuery"); + $this->expectExceptionMessage("The query/mutation 'duplicateQuery' is declared twice in class 'TheCodingMachine\\GraphQLite\\Fixtures\\DuplicateQueries\\TestControllerWithDuplicateQuery"); $schema = $factory->createSchema(); $queryString = ' query { @@ -221,7 +221,7 @@ public function testDuplicateQueryInTwoControllersException(): void ->addTypeNamespace('TheCodingMachine\\GraphQLite\\Fixtures\\Integration'); $this->expectException(DuplicateMappingException::class); - $this->expectErrorMessage("The query/mutation 'duplicateQuery' is declared twice: in class 'TheCodingMachine\\GraphQLite\\Fixtures\\DuplicateQueriesInTwoControllers\\TestControllerWithDuplicateQuery1' and in class 'TheCodingMachine\\GraphQLite\\Fixtures\\DuplicateQueriesInTwoControllers\\TestControllerWithDuplicateQuery2"); + $this->expectExceptionMessage("The query/mutation 'duplicateQuery' is declared twice: in class 'TheCodingMachine\\GraphQLite\\Fixtures\\DuplicateQueriesInTwoControllers\\TestControllerWithDuplicateQuery1' and in class 'TheCodingMachine\\GraphQLite\\Fixtures\\DuplicateQueriesInTwoControllers\\TestControllerWithDuplicateQuery2"); $schema = $factory->createSchema(); $queryString = ' query {