Merge pull request #18 from sitegeist/task/useDeepObjectForAllNonTriv…

…ialParameters TASK: Use `deepObject` for all non trivial parameters and remove `[]` from collection names
sitegeist · Jun 17, 2024 · f7d2033 · f7d2033
2 parents 6dc3da4 + 90becc0
commit f7d2033
Show file tree

Hide file tree

Showing 16 changed files with 472 additions and 26 deletions.
diff --git a/Classes/Domain/Metadata/Parameter.php b/Classes/Domain/Metadata/Parameter.php
@@ -30,13 +30,16 @@ public static function fromReflectionParameter(\ReflectionParameter $reflectionP
         $parameterAttributes = $reflectionParameter->getAttributes(self::class);
         switch (count($parameterAttributes)) {
             case 0:
-                return new self(ParameterLocation::LOCATION_QUERY);
+                return new self(
+                    in: ParameterLocation::LOCATION_QUERY,
+                    style: ParameterStyle::createDefaultForParameterLocationAndReflection(ParameterLocation::LOCATION_QUERY, $reflectionParameter)
+                );
             case 1:
                 $arguments = $parameterAttributes[0]->getArguments();
 
                 return new self(
                     $arguments['in'] ?? $arguments[0],
-                    $arguments['style'] ?? $arguments[1] ?? null,
+                    $arguments['style'] ?? $arguments[1] ?? ParameterStyle::createDefaultForParameterLocationAndReflection($arguments[0], $reflectionParameter),
                     $arguments['description'] ?? $arguments[2] ?? null,
                 );
             default:

diff --git a/Classes/Domain/Path/OpenApiParameter.php b/Classes/Domain/Path/OpenApiParameter.php
@@ -69,7 +69,7 @@ public static function fromReflectionParameter(\ReflectionParameter $reflectionP
         $parameterSchema = OpenApiSchema::fromReflectionClass($reflectionClass);
 
         return new self(
-            name: $reflectionParameter->name . (($parameterAttribute->in === ParameterLocation::LOCATION_QUERY && $parameterSchema->type === 'array') ? '[]' : ''),
+            name: $reflectionParameter->name,
             in: $parameterAttribute->in,
             description: $parameterAttribute->description ?: $schemaAttribute->description,
             required: !$reflectionParameter->isDefaultValueAvailable(),

diff --git a/Classes/Domain/Path/ParameterStyle.php b/Classes/Domain/Path/ParameterStyle.php
@@ -4,6 +4,9 @@
 
 namespace Sitegeist\SchemeOnYou\Domain\Path;
 
+use Sitegeist\SchemeOnYou\Domain\Schema\IsSingleValueDataTransferObject;
+use Sitegeist\SchemeOnYou\Domain\Schema\OpenApiSchema;
+
 /**
  * @see https://swagger.io/specification/#style-values
  */
@@ -30,6 +33,48 @@ public static function createDefaultForParameterLocation(ParameterLocation $loca
         };
     }
 
+    /**
+     * @see https://swagger.io/specification/#fixed-fields-10
+     */
+    public static function createDefaultForParameterLocationAndReflection(ParameterLocation $location, \ReflectionParameter $reflectionParameter): self
+    {
+        return match ($location) {
+            ParameterLocation::LOCATION_QUERY => self::createDefaultForQueryLocationAndReflection($reflectionParameter),
+            ParameterLocation::LOCATION_PATH => ParameterStyle::STYLE_SIMPLE,
+            ParameterLocation::LOCATION_HEADER => ParameterStyle::STYLE_SIMPLE,
+            ParameterLocation::LOCATION_COOKIE => ParameterStyle::STYLE_FORM
+        };
+    }
+
+    public static function createDefaultForQueryLocationAndReflection(\ReflectionParameter $reflectionParameter): self
+    {
+        $reflectionType = $reflectionParameter->getType();
+        if (!$reflectionType instanceof \ReflectionNamedType) {
+            throw new \DomainException(
+                'Query Parameters can only be resolved from named parameters',
+                1710067045
+            );
+        }
+        $type = $reflectionType->getName();
+        if (in_array($type, ['int', 'bool', 'string', 'float', \DateTimeImmutable::class, \DateTime::class, \DateInterval::class])) {
+            return ParameterStyle::STYLE_FORM;
+        }
+        if (!class_exists($type)) {
+            throw new \DomainException(
+                'Query parameters can only be resolved from class parameters, ' . $type . ' given for parameter '
+                . $reflectionParameter->getDeclaringClass()?->name
+                . '::' . $reflectionParameter->getDeclaringFunction()->name
+                . '::' . $reflectionParameter->name,
+                1709592649
+            );
+        }
+        if (IsSingleValueDataTransferObject::isSatisfiedByClassName($type)) {
+            return ParameterStyle::STYLE_FORM;
+        }
+
+        return  ParameterStyle::STYLE_DEEP_OBJECT;
+    }
+
     /**
      * @todo really?
      * @param array<mixed>|int|bool|string|float|null $parameterValue

diff --git a/Classes/Domain/Schema/IsDataTransferObject.php b/Classes/Domain/Schema/IsDataTransferObject.php
@@ -43,8 +43,9 @@ public static function isSatisfiedByReflectionClass(\ReflectionClass $reflection
      */
     private static function evaluateReflectionClass(\ReflectionClass $reflectionClass): bool
     {
-        if ($reflectionClass instanceof \ReflectionEnum) {
-            return true;
+        if ($reflectionClass->isEnum()) {
+            /** @phpstan-ignore-next-line */
+            return (new \ReflectionEnum($reflectionClass->getName()))->isBacked();
         }
         if ($reflectionClass->isReadOnly() === false) {
             return false;

diff --git a/Classes/Domain/Schema/IsDataTransferObjectCollection.php b/Classes/Domain/Schema/IsDataTransferObjectCollection.php
@@ -44,11 +44,13 @@ public static function isSatisfiedByReflectionClass(\ReflectionClass $reflection
      */
     private static function evaluateReflectionClass(\ReflectionClass $reflectionClass): bool
     {
-        $parameters = $reflectionClass->getConstructor()?->getParameters() ?: [];
-        if (count($parameters) !== 1) {
+        // readonly
+        if ($reflectionClass->isReadOnly() === false) {
             return false;
         }
-        if ($reflectionClass->isReadOnly() === false) {
+        // a single variadic constructor parameter of a supported type
+        $parameters = $reflectionClass->getConstructor()?->getParameters() ?: [];
+        if (count($parameters) !== 1) {
             return false;
         }
         $collectionParameter = $parameters[0];
@@ -57,11 +59,15 @@ private static function evaluateReflectionClass(\ReflectionClass $reflectionClas
         }
         $collectionParameterType = $collectionParameter->getType();
         if ($collectionParameterType instanceof \ReflectionNamedType) {
-            if (IsSupportedInSchema::isSatisfiedByReflectionType($collectionParameterType)) {
-                return true;
+            if (!IsSupportedInSchema::isSatisfiedByReflectionType($collectionParameterType)) {
+                return false;
             }
         }
-
+        // a single property of type array
+        $properties = $reflectionClass->getProperties();
+        if (count($properties) === 1 && $properties[0]->getType() instanceof \ReflectionNamedType && $properties[0]->getType()->getName() === 'array') {
+            return true;
+        }
         return false;
     }
 }
diff --git a/Classes/Domain/Schema/IsSingleValueDataTransferObject.php b/Classes/Domain/Schema/IsSingleValueDataTransferObject.php
@@ -0,0 +1,58 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Sitegeist\SchemeOnYou\Domain\Schema;
+
+use Neos\Flow\Annotations as Flow;
+
+#[Flow\Proxy(false)]
+final class IsSingleValueDataTransferObject
+{
+    /**
+     * @var array<class-string,bool>
+     */
+    private static array $evaluationRuntimeCache = [];
+
+    /**
+     * @param class-string $className
+     */
+    public static function isSatisfiedByClassName(string $className): bool
+    {
+        if (!array_key_exists($className, self::$evaluationRuntimeCache)) {
+            self::$evaluationRuntimeCache[$className] = self::evaluateReflectionClass(new \ReflectionClass($className));
+        }
+
+        return self::$evaluationRuntimeCache[$className];
+    }
+
+    /**
+     * @param \ReflectionClass<object> $reflectionClass
+     */
+    public static function isSatisfiedByReflectionClass(\ReflectionClass $reflectionClass): bool
+    {
+        if (!array_key_exists($reflectionClass->name, self::$evaluationRuntimeCache)) {
+            self::$evaluationRuntimeCache[$reflectionClass->name] = self::evaluateReflectionClass($reflectionClass);
+        }
+
+        return self::$evaluationRuntimeCache[$reflectionClass->name];
+    }
+
+    /**
+     * @param \ReflectionClass<object> $reflectionClass
+     */
+    private static function evaluateReflectionClass(\ReflectionClass $reflectionClass): bool
+    {
+        if ($reflectionClass->isEnum()) {
+            /** @phpstan-ignore-next-line */
+            return (new \ReflectionEnum($reflectionClass->getName()))->isBacked();
+        }
+        if (IsDataTransferObject::isSatisfiedByReflectionClass($reflectionClass)) {
+            $parameters = $reflectionClass->getConstructor()?->getParameters() ?: [];
+            if (count($parameters) === 1 && $parameters[0]->name === 'value') {
+                return true;
+            }
+        }
+        return false;
+    }
+}
diff --git a/Tests/Fixtures/InvalidObjects/CollectionThatIsNotReadonly.php b/Tests/Fixtures/InvalidObjects/CollectionThatIsNotReadonly.php
@@ -0,0 +1,22 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Sitegeist\SchemeOnYou\Tests\Fixtures\InvalidObjects;
+
+use Neos\Flow\Annotations as Flow;
+use Sitegeist\SchemeOnYou\Tests\Fixtures;
+
+#[Flow\Proxy(false)]
+final class CollectionThatIsNotReadonly
+{
+    /**
+     * @var Fixtures\Identifier[]
+     */
+    public array $items;
+    public function __construct(
+        Fixtures\Identifier ...$items
+    ) {
+        $this->items = $items;
+    }
+}
diff --git a/Tests/Fixtures/InvalidObjects/CollectionWithMoreThanOneProperty.php b/Tests/Fixtures/InvalidObjects/CollectionWithMoreThanOneProperty.php
@@ -0,0 +1,26 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Sitegeist\SchemeOnYou\Tests\Fixtures\InvalidObjects;
+
+use Neos\Flow\Annotations as Flow;
+use Sitegeist\SchemeOnYou\Tests\Fixtures;
+
+#[Flow\Proxy(false)]
+final readonly class CollectionWithMoreThanOneProperty
+{
+    /**
+     * @var Fixtures\Identifier[]
+     */
+    public array $items;
+
+    public int $count;
+
+    public function __construct(
+        Fixtures\Identifier ...$items
+    ) {
+        $this->items = $items;
+        $this->count = count($items);
+    }
+}
diff --git a/Tests/Fixtures/InvalidObjects/CollectionWithTooManyConstructorArguments.php b/Tests/Fixtures/InvalidObjects/CollectionWithTooManyConstructorArguments.php
@@ -0,0 +1,27 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Sitegeist\SchemeOnYou\Tests\Fixtures\InvalidObjects;
+
+use Neos\Flow\Annotations as Flow;
+use Sitegeist\SchemeOnYou\Tests\Fixtures;
+
+#[Flow\Proxy(false)]
+final readonly class CollectionWithTooManyConstructorArguments
+{
+    /**
+     * @var Fixtures\Identifier[]
+     */
+    public array $items;
+    public function __construct(
+        string $other,
+        Fixtures\Identifier ...$items
+    ) {
+        if ($other === 'whatever') {
+            $this->items = [...$items];
+        } else {
+            $this->items = $items;
+        }
+    }
+}
diff --git a/Tests/Fixtures/InvalidObjects/NonBackedEnum.php b/Tests/Fixtures/InvalidObjects/NonBackedEnum.php
@@ -0,0 +1,14 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Sitegeist\SchemeOnYou\Tests\Fixtures\InvalidObjects;
+
+use Neos\Flow\Annotations as Flow;
+
+#[Flow\Proxy(false)]
+enum NonBackedEnum
+{
+    case Foo;
+    case Bar;
+}
diff --git a/Tests/Fixtures/InvalidObjects/ObjectThatHasNonPromotedConstructorArguments.php b/Tests/Fixtures/InvalidObjects/ObjectThatHasNonPromotedConstructorArguments.php
@@ -0,0 +1,27 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Sitegeist\SchemeOnYou\Tests\Fixtures\InvalidObjects;
+
+use Neos\Flow\Annotations as Flow;
+use Sitegeist\SchemeOnYou\Domain\Metadata as OpenApi;
+
+#[Flow\Proxy(false)]
+final readonly class ObjectThatHasNonPromotedConstructorArguments
+{
+    public string $text;
+    public int $num;
+    public bool $switch;
+
+    public function __construct(
+        string $text,
+        int $num,
+        bool $switch
+    ) {
+
+        $this->text = $text;
+        $this->num = $num;
+        $this->switch = $switch;
+    }
+}
diff --git a/Tests/Fixtures/InvalidObjects/ObjectThatHasNotSupportedProperties.php b/Tests/Fixtures/InvalidObjects/ObjectThatHasNotSupportedProperties.php
@@ -0,0 +1,16 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Sitegeist\SchemeOnYou\Tests\Fixtures\InvalidObjects;
+
+use Neos\Flow\Annotations as Flow;
+
+#[Flow\Proxy(false)]
+final readonly class ObjectThatHasNotSupportedProperties
+{
+    public function __construct(
+        public \Psr\Http\Message\RequestInterface $text
+    ) {
+    }
+}
diff --git a/Tests/Fixtures/InvalidObjects/ObjectThatIsNotReadonly.php b/Tests/Fixtures/InvalidObjects/ObjectThatIsNotReadonly.php
@@ -0,0 +1,18 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Sitegeist\SchemeOnYou\Tests\Fixtures\InvalidObjects;
+
+use Neos\Flow\Annotations as Flow;
+
+#[Flow\Proxy(false)]
+final class ObjectThatIsNotReadonly
+{
+    public function __construct(
+        public string $text,
+        public int $num,
+        public bool $switch
+    ) {
+    }
+}
diff --git a/Tests/Unit/Application/ParameterFactoryTest.php b/Tests/Unit/Application/ParameterFactoryTest.php
@@ -57,12 +57,12 @@ public static function parameterProvider(): iterable
             'request' => ActionRequest::fromHttpRequest(
                 (new ServerRequest(
                     HttpMethod::METHOD_GET->value,
-                    new Uri('https://acme.site/?endpointQuery=de')
-                ))->withQueryParams([
-                    'endpointQuery' => [
-                        'language' => 'de'
+                    new Uri('https://acme.site/')
+                ))->withQueryParams(
+                    [
+                        'endpointQuery' => '{"language":"de"}'
                     ]
-                ])
+                )
             ),
             'className' => PathController::class,
             'methodName' => 'singleParameterAndResponseEndpointAction',
@@ -97,10 +97,12 @@ public static function parameterProvider(): iterable
         $multipleParametersRequest = ActionRequest::fromHttpRequest(
             (new ServerRequest(
                 HttpMethod::METHOD_GET->value,
-                new Uri('https://acme.site/?endpointQuery=de')
-            ))->withQueryParams([
-                'anotherEndpointQuery' => '{"pleaseFail": true}'
-            ])
+                new Uri('https://acme.site/de/')
+            ))->withQueryParams(
+                [
+                    'anotherEndpointQuery' => '{"pleaseFail":"true"}'
+                ]
+            )
         );
         $multipleParametersRequest->setArgument('endpointQuery', ['language' => 'de']);
 
@@ -117,9 +119,9 @@ public static function parameterProvider(): iterable
         $collectionParametersRequest = ActionRequest::fromHttpRequest(
             (new ServerRequest(
                 HttpMethod::METHOD_GET->value,
-                new Uri('https://acme.site/?identifierCollection[]=foo&identifierCollection[]=bar&identifier=baz')
+                new Uri('https://acme.site/?identifierCollection=foo&identifierCollection=bar&identifier=baz')
             ))->withQueryParams([
-                'identifierCollection' => ['foo','bar'],
+                'identifierCollection' => '["foo","bar"]',
                 'identifier' => 'baz'
             ])
         );