Use @param tags from constructor with property promotion (#628)

* 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 * Fixed tests * Corrected some logic and improved for durability * Added tests * Removed incorrect comment * Check for instanceof Param
thecodingmachine · Oct 16, 2023 · 3c4045a · 3c4045a
1 parent 812a830
commit 3c4045a
Show file tree

Hide file tree

Showing 7 changed files with 167 additions and 14 deletions.
diff --git a/src/FieldsBuilder.php b/src/FieldsBuilder.php
@@ -89,7 +89,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
@@ -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;
@@ -41,6 +42,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 +68,7 @@ public function __construct(
         private readonly ArgumentResolver $argumentResolver,
         private readonly RootTypeMapperInterface $rootTypeMapper,
         private readonly TypeResolver $typeResolver,
+        private readonly CachedDocBlockFactory $cachedDocBlockFactory,
     )
     {
         $this->phpDocumentorTypeResolver = new PhpDocumentorTypeResolver();
@@ -124,6 +127,27 @@ private function getDocBlockPropertyType(DocBlock $docBlock, ReflectionProperty
         $varTags = $docBlock->getTagsByName('var');
 
         if (! $varTags) {
+            // 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()) {
+                $refConstructor = $refProperty->getDeclaringClass()->getConstructor();
+                if (! $refConstructor) {
+                    return null;
+                }
+
+                $docBlock = $this->cachedDocBlockFactory->getDocBlock($refConstructor);
+                $paramTags = $docBlock->getTagsByName('param');
+                foreach ($paramTags as $paramTag) {
+                    if (! $paramTag instanceof Param) {
+                        continue;
+                    }
+
+                    if ($paramTag->getVariableName() === $refProperty->getName()) {
+                        return $paramTag->getType();
+                    }
+                }
+            }
+
             return null;
         }
 

diff --git 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/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,46 @@ 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);
+
+        $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
@@ -0,0 +1,22 @@
+<?php
+
+declare(strict_types=1);
+
+namespace TheCodingMachine\GraphQLite\Fixtures80;
+
+use TheCodingMachine\GraphQLite\Annotations as GraphQLite;
+
+#[GraphQLite\Input]
+class PropertyPromotionInputType
+{
+
+    /**
+     * Constructor
+     *
+     * @param array<int> $amounts
+     */
+    public function __construct(
+        #[GraphQLite\Field]
+        public array $amounts,
+    ) {}
+}
diff --git a/tests/Fixtures80/PropertyPromotionInputTypeWithoutGenericDoc.php b/tests/Fixtures80/PropertyPromotionInputTypeWithoutGenericDoc.php
@@ -0,0 +1,20 @@
+<?php
+
+declare(strict_types=1);
+
+namespace TheCodingMachine\GraphQLite\Fixtures80;
+
+use TheCodingMachine\GraphQLite\Annotations as GraphQLite;
+
+#[GraphQLite\Input]
+class PropertyPromotionInputTypeWithoutGenericDoc
+{
+
+    /**
+     * We expect this to fail since the array must have a generic type `array<int>`
+     */
+    public function __construct(
+        #[GraphQLite\Field]
+        public array $amounts,
+    ) {}
+}
diff --git 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];