From edde08e44ea507723fa595b1a1d97b1ed5b5a225 Mon Sep 17 00:00:00 2001 From: Jacob Thomason Date: Sun, 15 Oct 2023 18:23:08 -0400 Subject: [PATCH 1/6] When using property promotion and needing to add an @param annotation for a property, use the constructor docblock as fallback when an @var isn't available --- src/FieldsBuilder.php | 8 ++++++-- src/Mappers/Parameters/TypeHandler.php | 14 ++++++++++++++ 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/src/FieldsBuilder.php b/src/FieldsBuilder.php index 9aa781069a..b99a25e028 100644 --- a/src/FieldsBuilder.php +++ b/src/FieldsBuilder.php @@ -9,7 +9,6 @@ use GraphQL\Type\Definition\InputType; use GraphQL\Type\Definition\NonNull; use GraphQL\Type\Definition\OutputType; -use GraphQL\Type\Definition\Type; use phpDocumentor\Reflection\DocBlock; use phpDocumentor\Reflection\DocBlock\Tags\Var_; use Psr\SimpleCache\InvalidArgumentException; @@ -89,7 +88,12 @@ public function __construct( private readonly InputFieldMiddlewareInterface $inputFieldMiddleware, ) { - $this->typeMapper = new TypeHandler($this->argumentResolver, $this->rootTypeMapper, $this->typeResolver); + $this->typeMapper = new TypeHandler( + $this->argumentResolver, + $this->rootTypeMapper, + $this->typeResolver, + $this->cachedDocBlockFactory, + ); } /** diff --git a/src/Mappers/Parameters/TypeHandler.php b/src/Mappers/Parameters/TypeHandler.php index d660a52151..ee31e10226 100644 --- a/src/Mappers/Parameters/TypeHandler.php +++ b/src/Mappers/Parameters/TypeHandler.php @@ -41,6 +41,7 @@ use TheCodingMachine\GraphQLite\Parameters\InputTypeParameter; use TheCodingMachine\GraphQLite\Parameters\InputTypeProperty; use TheCodingMachine\GraphQLite\Parameters\ParameterInterface; +use TheCodingMachine\GraphQLite\Reflection\CachedDocBlockFactory; use TheCodingMachine\GraphQLite\Types\ArgumentResolver; use TheCodingMachine\GraphQLite\Types\TypeResolver; @@ -66,6 +67,7 @@ public function __construct( private readonly ArgumentResolver $argumentResolver, private readonly RootTypeMapperInterface $rootTypeMapper, private readonly TypeResolver $typeResolver, + private readonly CachedDocBlockFactory $cachedDocBlockFactory, ) { $this->phpDocumentorTypeResolver = new PhpDocumentorTypeResolver(); @@ -128,6 +130,18 @@ private function getDocBlockPropertyType(DocBlock $docBlock, ReflectionProperty } if (count($varTags) > 1) { + // If we don't have any @var tags, was this property promoted, and if so, do we have an + // @param tag on the constructor docblock? If so, use that for the type. + if ($refProperty->isPromoted()) { + $docBlock = $this->cachedDocBlockFactory->getDocBlock($refProperty->getDeclaringClass()->getConstructor()); + $paramTags = $docBlock->getTagsByName('param'); + foreach ($paramTags as $paramTag) { + if ($paramTag->getVariableName() === $refProperty->getName()) { + return $paramTag->getType(); + } + } + } + throw InvalidDocBlockRuntimeException::tooManyVarTags($refProperty); } From 9b96aca4d5b9aa7b389d5e850ce58568c56e5a9b Mon Sep 17 00:00:00 2001 From: Jacob Thomason Date: Sun, 15 Oct 2023 18:39:00 -0400 Subject: [PATCH 2/6] Fixed tests --- src/FieldsBuilder.php | 1 + tests/AbstractQueryProviderTest.php | 11 ++++- tests/Mappers/Parameters/TypeMapperTest.php | 54 ++++++++++++++++----- 3 files changed, 53 insertions(+), 13 deletions(-) diff --git a/src/FieldsBuilder.php b/src/FieldsBuilder.php index b99a25e028..5cd1d75709 100644 --- a/src/FieldsBuilder.php +++ b/src/FieldsBuilder.php @@ -9,6 +9,7 @@ use GraphQL\Type\Definition\InputType; use GraphQL\Type\Definition\NonNull; use GraphQL\Type\Definition\OutputType; +use GraphQL\Type\Definition\Type; use phpDocumentor\Reflection\DocBlock; use phpDocumentor\Reflection\DocBlock\Tags\Var_; use Psr\SimpleCache\InvalidArgumentException; diff --git a/tests/AbstractQueryProviderTest.php b/tests/AbstractQueryProviderTest.php index 0c888343f8..2f59ff768b 100644 --- a/tests/AbstractQueryProviderTest.php +++ b/tests/AbstractQueryProviderTest.php @@ -277,6 +277,15 @@ protected function getParameterMiddlewarePipe(): ParameterMiddlewarePipe return $this->parameterMiddlewarePipe; } + protected function getCachedDocBlockFactory(): CachedDocBlockFactory + { + $arrayAdapter = new ArrayAdapter(); + $arrayAdapter->setLogger(new ExceptionLogger()); + $psr16Cache = new Psr16Cache($arrayAdapter); + + return new CachedDocBlockFactory($psr16Cache); + } + protected function buildFieldsBuilder(): FieldsBuilder { $arrayAdapter = new ArrayAdapter(); @@ -308,7 +317,7 @@ protected function buildFieldsBuilder(): FieldsBuilder $this->getTypeMapper(), $this->getArgumentResolver(), $this->getTypeResolver(), - new CachedDocBlockFactory($psr16Cache), + $this->getCachedDocBlockFactory(), new NamingStrategy(), $this->buildRootTypeMapper(), $parameterMiddlewarePipe, diff --git a/tests/Mappers/Parameters/TypeMapperTest.php b/tests/Mappers/Parameters/TypeMapperTest.php index 537038865d..6a1a148f45 100644 --- a/tests/Mappers/Parameters/TypeMapperTest.php +++ b/tests/Mappers/Parameters/TypeMapperTest.php @@ -22,7 +22,12 @@ class TypeMapperTest extends AbstractQueryProviderTest public function testMapScalarUnionException(): void { - $typeMapper = new TypeHandler($this->getArgumentResolver(), $this->getRootTypeMapper(), $this->getTypeResolver()); + $typeMapper = new TypeHandler( + $this->getArgumentResolver(), + $this->getRootTypeMapper(), + $this->getTypeResolver(), + $this->getCachedDocBlockFactory(), + ); $cachedDocBlockFactory = new CachedDocBlockFactory(new Psr16Cache(new ArrayAdapter())); @@ -39,9 +44,14 @@ public function testMapScalarUnionException(): void */ public function testMapObjectUnionWorks(): void { - $typeMapper = new TypeHandler($this->getArgumentResolver(), $this->getRootTypeMapper(), $this->getTypeResolver()); + $cachedDocBlockFactory = $this->getCachedDocBlockFactory(); - $cachedDocBlockFactory = new CachedDocBlockFactory(new Psr16Cache(new ArrayAdapter())); + $typeMapper = new TypeHandler( + $this->getArgumentResolver(), + $this->getRootTypeMapper(), + $this->getTypeResolver(), + $cachedDocBlockFactory, + ); $refMethod = new ReflectionMethod(UnionOutputType::class, 'objectUnion'); $docBlockObj = $cachedDocBlockFactory->getDocBlock($refMethod); @@ -61,9 +71,14 @@ public function testMapObjectUnionWorks(): void */ public function testMapObjectNullableUnionWorks(): void { - $typeMapper = new TypeHandler($this->getArgumentResolver(), $this->getRootTypeMapper(), $this->getTypeResolver()); + $cachedDocBlockFactory = $this->getCachedDocBlockFactory(); - $cachedDocBlockFactory = new CachedDocBlockFactory(new Psr16Cache(new ArrayAdapter())); + $typeMapper = new TypeHandler( + $this->getArgumentResolver(), + $this->getRootTypeMapper(), + $this->getTypeResolver(), + $cachedDocBlockFactory, + ); $refMethod = new ReflectionMethod(UnionOutputType::class, 'nullableObjectUnion'); $docBlockObj = $cachedDocBlockFactory->getDocBlock($refMethod); @@ -82,9 +97,14 @@ public function testMapObjectNullableUnionWorks(): void public function testHideParameter(): void { - $typeMapper = new TypeHandler($this->getArgumentResolver(), $this->getRootTypeMapper(), $this->getTypeResolver()); + $cachedDocBlockFactory = $this->getCachedDocBlockFactory(); - $cachedDocBlockFactory = new CachedDocBlockFactory(new Psr16Cache(new ArrayAdapter())); + $typeMapper = new TypeHandler( + $this->getArgumentResolver(), + $this->getRootTypeMapper(), + $this->getTypeResolver(), + $cachedDocBlockFactory, + ); $refMethod = new ReflectionMethod($this, 'withDefaultValue'); $refParameter = $refMethod->getParameters()[0]; @@ -101,9 +121,14 @@ public function testHideParameter(): void public function testParameterWithDescription(): void { - $typeMapper = new TypeHandler($this->getArgumentResolver(), $this->getRootTypeMapper(), $this->getTypeResolver()); + $cachedDocBlockFactory = $this->getCachedDocBlockFactory(); - $cachedDocBlockFactory = new CachedDocBlockFactory(new Psr16Cache(new ArrayAdapter())); + $typeMapper = new TypeHandler( + $this->getArgumentResolver(), + $this->getRootTypeMapper(), + $this->getTypeResolver(), + $cachedDocBlockFactory, + ); $refMethod = new ReflectionMethod($this, 'withParamDescription'); $docBlockObj = $cachedDocBlockFactory->getDocBlock($refMethod); @@ -117,9 +142,14 @@ public function testParameterWithDescription(): void public function testHideParameterException(): void { - $typeMapper = new TypeHandler($this->getArgumentResolver(), $this->getRootTypeMapper(), $this->getTypeResolver()); - - $cachedDocBlockFactory = new CachedDocBlockFactory(new Psr16Cache(new ArrayAdapter())); + $cachedDocBlockFactory = $this->getCachedDocBlockFactory(); + + $typeMapper = new TypeHandler( + $this->getArgumentResolver(), + $this->getRootTypeMapper(), + $this->getTypeResolver(), + $cachedDocBlockFactory, + ); $refMethod = new ReflectionMethod($this, 'withoutDefaultValue'); $refParameter = $refMethod->getParameters()[0]; From f2d8895bc68cff3a01b38c5f8c47fe2eef078d92 Mon Sep 17 00:00:00 2001 From: Jacob Thomason Date: Sun, 15 Oct 2023 19:43:20 -0400 Subject: [PATCH 3/6] Corrected some logic and improved for durability --- src/Mappers/Parameters/TypeHandler.php | 25 +++++++++++++++++++------ 1 file changed, 19 insertions(+), 6 deletions(-) diff --git a/src/Mappers/Parameters/TypeHandler.php b/src/Mappers/Parameters/TypeHandler.php index ee31e10226..84282b186b 100644 --- a/src/Mappers/Parameters/TypeHandler.php +++ b/src/Mappers/Parameters/TypeHandler.php @@ -126,22 +126,35 @@ private function getDocBlockPropertyType(DocBlock $docBlock, ReflectionProperty $varTags = $docBlock->getTagsByName('var'); if (! $varTags) { - return null; - } - - if (count($varTags) > 1) { // If we don't have any @var tags, was this property promoted, and if so, do we have an // @param tag on the constructor docblock? If so, use that for the type. if ($refProperty->isPromoted()) { - $docBlock = $this->cachedDocBlockFactory->getDocBlock($refProperty->getDeclaringClass()->getConstructor()); + $refConstructor = $refProperty->getDeclaringClass()->getConstructor(); + if (! $refConstructor) { + return null; + } + + $docBlock = $this->cachedDocBlockFactory->getDocBlock($refConstructor); $paramTags = $docBlock->getTagsByName('param'); foreach ($paramTags as $paramTag) { + if (! method_exists($paramTag, 'getVariableName')) { + continue; + } + + if (! method_exists($paramTag, 'getType')) { + continue; + } + if ($paramTag->getVariableName() === $refProperty->getName()) { return $paramTag->getType(); } } } - + + return null; + } + + if (count($varTags) > 1) { throw InvalidDocBlockRuntimeException::tooManyVarTags($refProperty); } From a788beb809826e57375ffbc4b47f5eb708a93224 Mon Sep 17 00:00:00 2001 From: Jacob Thomason Date: Sun, 15 Oct 2023 19:43:25 -0400 Subject: [PATCH 4/6] Added tests --- tests/FieldsBuilderTest.php | 45 +++++++++++++++++++ .../Fixtures80/PropertyPromotionInputType.php | 22 +++++++++ ...rtyPromotionInputTypeWithoutGenericDoc.php | 20 +++++++++ 3 files changed, 87 insertions(+) create mode 100644 tests/Fixtures80/PropertyPromotionInputType.php create mode 100644 tests/Fixtures80/PropertyPromotionInputTypeWithoutGenericDoc.php diff --git a/tests/FieldsBuilderTest.php b/tests/FieldsBuilderTest.php index 108f87ccd6..02a30fa540 100644 --- a/tests/FieldsBuilderTest.php +++ b/tests/FieldsBuilderTest.php @@ -14,6 +14,7 @@ use GraphQL\Type\Definition\StringType; use GraphQL\Type\Definition\ObjectType; use GraphQL\Type\Definition\UnionType; +use PhpParser\Builder\Property; use ReflectionMethod; use stdClass; use Symfony\Component\Cache\Adapter\ArrayAdapter; @@ -69,6 +70,8 @@ use TheCodingMachine\GraphQLite\Security\VoidAuthenticationService; use TheCodingMachine\GraphQLite\Security\VoidAuthorizationService; use TheCodingMachine\GraphQLite\Annotations\Query; +use TheCodingMachine\GraphQLite\Fixtures80\PropertyPromotionInputType; +use TheCodingMachine\GraphQLite\Fixtures80\PropertyPromotionInputTypeWithoutGenericDoc; use TheCodingMachine\GraphQLite\Types\DateTimeType; use TheCodingMachine\GraphQLite\Types\VoidType; @@ -197,6 +200,48 @@ public function test($typeHintInDocBlock) $this->assertInstanceOf(StringType::class, $query->getType()->getWrappedType()); } + /** + * Tests that the fields builder will fail when a parameter is missing it's generic docblock + * definition, when required - an array, for instance, or could be a collection (List types) + * + * @requires PHP >= 8.0 + */ + public function testTypeMissingForPropertyPromotionWithoutGenericDoc(): void + { + $fieldsBuilder = $this->buildFieldsBuilder(); + + $this->expectException(CannotMapTypeException::class); + + // Techncially at this point, we already know it's working, since an exception would have been + // thrown otherwise, requiring the generic type to be specified. + $fieldsBuilder->getInputFields( + PropertyPromotionInputTypeWithoutGenericDoc::class, + 'PropertyPromotionInputTypeWithoutGenericDocInput', + ); + } + + /** + * Tests that the fields builder will properly build an input type using property promotion + * with the generic docblock defined on the constructor and not the property directly. + * + * @requires PHP >= 8.0 + */ + public function testTypeInDocBlockWithPropertyPromotion(): void + { + $fieldsBuilder = $this->buildFieldsBuilder(); + + // Techncially at this point, we already know it's working, since an exception would have been + // thrown otherwise, requiring the generic type to be specified. + // @see self::testTypeMissingForPropertyPromotionWithoutGenericDoc + $inputFields = $fieldsBuilder->getInputFields( + PropertyPromotionInputType::class, + 'PropertyPromotionInputTypeInput', + ); + + $this->assertCount(1, $inputFields); + $this->assertEquals('amounts', reset($inputFields)->name); + } + public function testQueryProviderWithFixedReturnType(): void { $controller = new TestController(); diff --git a/tests/Fixtures80/PropertyPromotionInputType.php b/tests/Fixtures80/PropertyPromotionInputType.php new file mode 100644 index 0000000000..d9850f52a1 --- /dev/null +++ b/tests/Fixtures80/PropertyPromotionInputType.php @@ -0,0 +1,22 @@ + $amounts + */ + public function __construct( + #[GraphQLite\Field] + public array $amounts, + ) {} +} diff --git a/tests/Fixtures80/PropertyPromotionInputTypeWithoutGenericDoc.php b/tests/Fixtures80/PropertyPromotionInputTypeWithoutGenericDoc.php new file mode 100644 index 0000000000..9b1391f7b8 --- /dev/null +++ b/tests/Fixtures80/PropertyPromotionInputTypeWithoutGenericDoc.php @@ -0,0 +1,20 @@ +` + */ + public function __construct( + #[GraphQLite\Field] + public array $amounts, + ) {} +} From 5257786b587be1956f99b0a8319ef92786557356 Mon Sep 17 00:00:00 2001 From: Jacob Thomason Date: Sun, 15 Oct 2023 19:45:20 -0400 Subject: [PATCH 5/6] Removed incorrect comment --- tests/FieldsBuilderTest.php | 2 -- 1 file changed, 2 deletions(-) diff --git a/tests/FieldsBuilderTest.php b/tests/FieldsBuilderTest.php index 02a30fa540..137a02e72e 100644 --- a/tests/FieldsBuilderTest.php +++ b/tests/FieldsBuilderTest.php @@ -212,8 +212,6 @@ public function testTypeMissingForPropertyPromotionWithoutGenericDoc(): void $this->expectException(CannotMapTypeException::class); - // Techncially at this point, we already know it's working, since an exception would have been - // thrown otherwise, requiring the generic type to be specified. $fieldsBuilder->getInputFields( PropertyPromotionInputTypeWithoutGenericDoc::class, 'PropertyPromotionInputTypeWithoutGenericDocInput', From 5fbbd1ee5eadf66c69d379f222457fcfdbc77370 Mon Sep 17 00:00:00 2001 From: Jacob Thomason Date: Mon, 16 Oct 2023 13:10:46 -0400 Subject: [PATCH 6/6] Check for instanceof Param --- src/Mappers/Parameters/TypeHandler.php | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/Mappers/Parameters/TypeHandler.php b/src/Mappers/Parameters/TypeHandler.php index 84282b186b..8a8eadfe1d 100644 --- a/src/Mappers/Parameters/TypeHandler.php +++ b/src/Mappers/Parameters/TypeHandler.php @@ -9,6 +9,7 @@ use GraphQL\Type\Definition\Type as GraphQLType; use InvalidArgumentException; use phpDocumentor\Reflection\DocBlock; +use phpDocumentor\Reflection\DocBlock\Tags\Param; use phpDocumentor\Reflection\DocBlock\Tags\Return_; use phpDocumentor\Reflection\DocBlock\Tags\Var_; use phpDocumentor\Reflection\Fqsen; @@ -137,11 +138,7 @@ private function getDocBlockPropertyType(DocBlock $docBlock, ReflectionProperty $docBlock = $this->cachedDocBlockFactory->getDocBlock($refConstructor); $paramTags = $docBlock->getTagsByName('param'); foreach ($paramTags as $paramTag) { - if (! method_exists($paramTag, 'getVariableName')) { - continue; - } - - if (! method_exists($paramTag, 'getType')) { + if (! $paramTag instanceof Param) { continue; }