From 1661f99005655b77d0b9c4d93dba12ed9352e3be Mon Sep 17 00:00:00 2001 From: Martin Rademacher Date: Sat, 18 Mar 2023 08:58:25 +1300 Subject: [PATCH] Refactor annotation root comparisons (#1431) --- src/Analysers/AttributeAnnotationFactory.php | 4 +- src/Annotations/AbstractAnnotation.php | 52 ++-- src/Processors/ExpandClasses.php | 3 +- src/Processors/ExpandEnums.php | 10 +- src/Processors/ExpandInterfaces.php | 3 +- src/Processors/ExpandTraits.php | 3 +- src/Processors/MergeIntoComponents.php | 5 +- src/Processors/MergeIntoOpenApi.php | 6 +- tests/Analysers/ReflectionAnalyserTest.php | 3 +- tests/Annotations/AbstractAnnotationTest.php | 3 +- .../Scratch/ComplexCustomAttributes.php | 225 ++++++++++++++++++ .../Scratch/ComplexCustomAttributes.yaml | 63 +++++ tests/Processors/ExpandEnumsTest.php | 3 +- 13 files changed, 335 insertions(+), 48 deletions(-) create mode 100644 tests/Fixtures/Scratch/ComplexCustomAttributes.php create mode 100644 tests/Fixtures/Scratch/ComplexCustomAttributes.yaml diff --git a/src/Analysers/AttributeAnnotationFactory.php b/src/Analysers/AttributeAnnotationFactory.php index 0bbeed5e..179d199e 100644 --- a/src/Analysers/AttributeAnnotationFactory.php +++ b/src/Analysers/AttributeAnnotationFactory.php @@ -109,7 +109,7 @@ public function build(\Reflector $reflector, Context $context): array // merge backwards into parents... $isParent = function (OA\AbstractAnnotation $annotation, OA\AbstractAnnotation $possibleParent): bool { // regular annotation hierarchy - $explicitParent = null !== $possibleParent::matchNested(get_class($annotation)) && !$annotation instanceof OA\Attachable; + $explicitParent = null !== $possibleParent->matchNested($annotation) && !$annotation instanceof OA\Attachable; $isParentAllowed = false; // support Attachable subclasses @@ -126,7 +126,7 @@ public function build(\Reflector $reflector, Context $context): array } // Property can be nested... - return get_class($annotation) != get_class($possibleParent) + return $annotation->getRoot() != $possibleParent->getRoot() && ($explicitParent || ($isAttachable && $isParentAllowed)); }; diff --git a/src/Annotations/AbstractAnnotation.php b/src/Annotations/AbstractAnnotation.php index f888e95c..ccf4a02f 100644 --- a/src/Annotations/AbstractAnnotation.php +++ b/src/Annotations/AbstractAnnotation.php @@ -186,7 +186,7 @@ public function merge(array $annotations, bool $ignore = false): array foreach ($annotations as $annotation) { $mapped = false; - if ($details = static::matchNested(get_class($annotation))) { + if ($details = $this->matchNested($annotation)) { $property = $details->value; if (is_array($property)) { $property = $property[0]; @@ -416,7 +416,7 @@ public function validate(array $stack = [], array $skip = [], string $ref = '', /** @var class-string $class */ $class = get_class($annotation); - if ($details = static::matchNested($class)) { + if ($details = $this->matchNested($annotation)) { $property = $details->value; if (is_array($property)) { $this->_context->logger->warning('Only one ' . Util::shorten(get_class($annotation)) . '() allowed for ' . $this->identity() . ' multiple found, skipped: ' . $annotation->_context); @@ -574,46 +574,50 @@ public function identity(): string } /** - * Find matching nested details. + * Check if `$other` can be nested and if so return details about where/how. * - * @param class-string $class the class to match + * @param AbstractAnnotation $other the other annotation * * @return null|object key/value object or `null` */ - public static function matchNested(string $class) + public function matchNested($other) { - if (array_key_exists($class, static::$_nested)) { - return (object) ['key' => $class, 'value' => static::$_nested[$class]]; - } - - $parent = $class; - // only consider the immediate OpenApi parent - while (0 !== strpos($parent, 'OpenApi\\Annotations\\') && $parent = get_parent_class($parent)) { - if ($kvp = static::matchNested($parent)) { - return $kvp; - } + if ($other instanceof AbstractAnnotation && array_key_exists($root = $other->getRoot(), static::$_nested)) { + return (object) ['key' => $root, 'value' => static::$_nested[$root]]; } return null; } /** - * Match the annotation root. + * Get the root annotation. * - * @param class-string $rootClass the root class to match + * This is used for resolving type equality and nesting rules to allow those rules to also work for custom, + * derived annotation classes. + * + * @return class-string the root annotation class in the `OpenApi\\Annotations` namespace */ - public function isRoot(string $rootClass): bool + public function getRoot(): string { - $parent = get_class($this); + $class = get_class($this); - // only consider the immediate OpenApi parent do { - if ($parent == $rootClass) { - return true; + if (0 === strpos($class, 'OpenApi\\Annotations\\')) { + break; } - } while (0 !== strpos($parent, 'OpenApi\\Annotations\\') && $parent = get_parent_class($parent)); + } while ($class = get_parent_class($class)); - return false; + return $class; + } + + /** + * Match the annotation root. + * + * @param class-string $rootClass the root class to match + */ + public function isRoot(string $rootClass): bool + { + return $this->getRoot() == $rootClass; } /** diff --git a/src/Processors/ExpandClasses.php b/src/Processors/ExpandClasses.php index ac8c2bae..6ede741c 100644 --- a/src/Processors/ExpandClasses.php +++ b/src/Processors/ExpandClasses.php @@ -8,7 +8,6 @@ use OpenApi\Analysis; use OpenApi\Annotations as OA; -use OpenApi\Attributes as OAT; use OpenApi\Generator; /** @@ -25,7 +24,7 @@ class ExpandClasses implements ProcessorInterface public function __invoke(Analysis $analysis) { /** @var OA\Schema[] $schemas */ - $schemas = $analysis->getAnnotationsOfType([OA\Schema::class, OAT\Schema::class], true); + $schemas = $analysis->getAnnotationsOfType(OA\Schema::class, true); foreach ($schemas as $schema) { if ($schema->_context->is('class')) { diff --git a/src/Processors/ExpandEnums.php b/src/Processors/ExpandEnums.php index 0d4ae2b2..6865fb85 100644 --- a/src/Processors/ExpandEnums.php +++ b/src/Processors/ExpandEnums.php @@ -8,7 +8,6 @@ use OpenApi\Analysis; use OpenApi\Annotations as OA; -use OpenApi\Attributes as OAT; use OpenApi\Generator; /** @@ -33,7 +32,7 @@ public function __invoke(Analysis $analysis) protected function expandContextEnum(Analysis $analysis): void { /** @var OA\Schema[] $schemas */ - $schemas = $analysis->getAnnotationsOfType([OA\Schema::class, OAT\Schema::class], true); + $schemas = $analysis->getAnnotationsOfType(OA\Schema::class, true); foreach ($schemas as $schema) { if ($schema->_context->is('enum')) { @@ -63,12 +62,7 @@ protected function expandContextEnum(Analysis $analysis): void protected function expandSchemaEnum(Analysis $analysis): void { /** @var OA\Schema[] $schemas */ - $schemas = $analysis->getAnnotationsOfType([ - OA\Schema::class, - OAT\Schema::class, - OA\ServerVariable::class, - OAT\ServerVariable::class, - ]); + $schemas = $analysis->getAnnotationsOfType([OA\Schema::class, OA\ServerVariable::class]); foreach ($schemas as $schema) { if (Generator::isDefault($schema->enum)) { diff --git a/src/Processors/ExpandInterfaces.php b/src/Processors/ExpandInterfaces.php index a4f5269b..8bcc3c25 100644 --- a/src/Processors/ExpandInterfaces.php +++ b/src/Processors/ExpandInterfaces.php @@ -8,7 +8,6 @@ use OpenApi\Analysis; use OpenApi\Annotations as OA; -use OpenApi\Attributes as OAT; use OpenApi\Generator; /** @@ -23,7 +22,7 @@ class ExpandInterfaces public function __invoke(Analysis $analysis) { /** @var OA\Schema[] $schemas */ - $schemas = $analysis->getAnnotationsOfType([OA\Schema::class, OAT\Schema::class], true); + $schemas = $analysis->getAnnotationsOfType(OA\Schema::class, true); foreach ($schemas as $schema) { if ($schema->_context->is('class')) { diff --git a/src/Processors/ExpandTraits.php b/src/Processors/ExpandTraits.php index be245d2b..cd1b4aca 100644 --- a/src/Processors/ExpandTraits.php +++ b/src/Processors/ExpandTraits.php @@ -8,7 +8,6 @@ use OpenApi\Analysis; use OpenApi\Annotations as OA; -use OpenApi\Attributes as OAT; use OpenApi\Generator; /** @@ -23,7 +22,7 @@ class ExpandTraits implements ProcessorInterface public function __invoke(Analysis $analysis) { /** @var OA\Schema[] $schemas */ - $schemas = $analysis->getAnnotationsOfType([OA\Schema::class, OAT\Schema::class], true); + $schemas = $analysis->getAnnotationsOfType(OA\Schema::class, true); // do regular trait inheritance / merge foreach ($schemas as $schema) { diff --git a/src/Processors/MergeIntoComponents.php b/src/Processors/MergeIntoComponents.php index c344fdc0..8a024f92 100644 --- a/src/Processors/MergeIntoComponents.php +++ b/src/Processors/MergeIntoComponents.php @@ -23,8 +23,11 @@ public function __invoke(Analysis $analysis) $components = new OA\Components(['_context' => new Context(['generated' => true], $analysis->context)]); } + /** @var OA\AbstractAnnotation $annotation */ foreach ($analysis->annotations as $annotation) { - if (OA\Components::matchNested(get_class($annotation)) && $annotation->_context->is('nested') === false) { + if ($annotation instanceof OA\AbstractAnnotation + && in_array(OA\Components::class, $annotation::$_parents) + && false === $annotation->_context->is('nested')) { // A top level annotation. $components->merge([$annotation], true); $analysis->openapi->components = $components; diff --git a/src/Processors/MergeIntoOpenApi.php b/src/Processors/MergeIntoOpenApi.php index 9fe94241..95b7c103 100644 --- a/src/Processors/MergeIntoOpenApi.php +++ b/src/Processors/MergeIntoOpenApi.php @@ -45,7 +45,11 @@ public function __invoke(Analysis $analysis) $openapi->paths[] = $path; } } - } elseif (OA\OpenApi::matchNested(get_class($annotation)) && property_exists($annotation, '_context') && $annotation->_context->is('nested') === false) { + } elseif ( + $annotation instanceof OA\AbstractAnnotation + && in_array(OA\OpenApi::class, $annotation::$_parents) + && property_exists($annotation, '_context') + && false === $annotation->_context->is('nested')) { // A top level annotation. $merge[] = $annotation; } diff --git a/tests/Analysers/ReflectionAnalyserTest.php b/tests/Analysers/ReflectionAnalyserTest.php index f92f8fd5..65d86774 100644 --- a/tests/Analysers/ReflectionAnalyserTest.php +++ b/tests/Analysers/ReflectionAnalyserTest.php @@ -14,7 +14,6 @@ use OpenApi\Analysers\TokenAnalyser; use OpenApi\Analysis; use OpenApi\Annotations as OA; -use OpenApi\Attributes as OAT; use OpenApi\Context; use OpenApi\Generator; use OpenApi\Processors\CleanUnusedComponents; @@ -136,7 +135,7 @@ public function testApiAttributesBasic(AnalyserInterface $analyser): void // check CustomAttachable is only attached to @OA\Get /** @var OA\Get[] $gets */ - $gets = $analysis->getAnnotationsOfType(OAT\Get::class, true); + $gets = $analysis->getAnnotationsOfType(OA\Get::class, true); $this->assertCount(2, $gets); $this->assertTrue(is_array($gets[0]->attachables), 'Attachables not set'); $this->assertCount(1, $gets[0]->attachables); diff --git a/tests/Annotations/AbstractAnnotationTest.php b/tests/Annotations/AbstractAnnotationTest.php index 4ab7da5d..9c468f64 100644 --- a/tests/Annotations/AbstractAnnotationTest.php +++ b/tests/Annotations/AbstractAnnotationTest.php @@ -113,7 +113,6 @@ public function nestedMatches(): iterable $parameterMatch = (object) ['key' => OA\Parameter::class, 'value' => ['parameters']]; return [ - 'unknown' => [self::class, null], 'simple-match' => [OA\Parameter::class, $parameterMatch], 'invalid-annotation' => [OA\Schema::class, null], 'sub-annotation' => [SubParameter::class, $parameterMatch], @@ -127,7 +126,7 @@ public function nestedMatches(): iterable */ public function testMatchNested(string $class, $expected): void { - $this->assertEquals($expected, OA\Get::matchNested($class)); + $this->assertEquals($expected, (new OA\Get([]))->matchNested(new $class([]))); } public function testDuplicateOperationIdValidation(): void diff --git a/tests/Fixtures/Scratch/ComplexCustomAttributes.php b/tests/Fixtures/Scratch/ComplexCustomAttributes.php new file mode 100644 index 00000000..76064de5 --- /dev/null +++ b/tests/Fixtures/Scratch/ComplexCustomAttributes.php @@ -0,0 +1,225 @@ +getProperties(\ReflectionProperty::IS_PUBLIC) as $property) { + if (\in_array($property->getName(), $optional, true)) { + continue; + } + + $required[] = $property->getName(); + } + + $shortName = $class->getShortName(); + + parent::__construct( + schema: $shortName, + title: $shortName, + description: $description, + required: $required, + maxLength: $maxLength, + minLength: $minLength + ); + } +} + +#[\Attribute( + \Attribute::TARGET_METHOD | + \Attribute::TARGET_PROPERTY | + \Attribute::TARGET_PARAMETER | + \Attribute::TARGET_CLASS_CONSTANT | + \Attribute::IS_REPEATABLE +)] +class Collection extends OAT\Property +{ + /** @param class-string $of */ + public function __construct( + string $of, + ?string $description = null + ) { + $shortName = (new \ReflectionClass($of))->getShortName(); + + parent::__construct( + title: $shortName, + description: $description, + items: new OAT\Items(ref: "#/components/schemas/$shortName") + ); + } +} + +#[\Attribute( + \Attribute::TARGET_METHOD | + \Attribute::TARGET_PROPERTY | + \Attribute::TARGET_PARAMETER | + \Attribute::TARGET_CLASS_CONSTANT | + \Attribute::IS_REPEATABLE +)] +class Item extends OAT\Property +{ + /** @param class-string $of */ + public function __construct( + string $of, + ?string $description = null + ) { + $shortName = (new \ReflectionClass($of))->getShortName(); + + parent::__construct( + ref: "#/components/schemas/$shortName", + title: $shortName, + description: $description, + ); + } +} + +#[\Attribute( + \Attribute::TARGET_METHOD | + \Attribute::TARGET_PROPERTY | + \Attribute::TARGET_PARAMETER | + \Attribute::TARGET_CLASS_CONSTANT | + \Attribute::IS_REPEATABLE +)] +class Raw extends OAT\Property +{ + public function __construct( + ?string $title = null, + ?string $description = null + ) { + parent::__construct( + title: $title, + description: $description, + ); + } +} + +#[\Attribute(\Attribute::TARGET_CLASS | \Attribute::TARGET_METHOD | \Attribute::IS_REPEATABLE)] +class Successful extends OAT\Response +{ + /** @param ?class-string $of */ + public function __construct( + ?string $of = null, + ) { + if ($of === null) { + parent::__construct( + response: 200, + description: 'Operation complete' + ); + + return; + } + + $shortName = (new \ReflectionClass($of))->getShortName(); + + parent::__construct( + response: 200, + description: "Successful response of [$shortName]", + content: new OAT\JsonContent( + ref: "#/components/schemas/$shortName" + ) + ); + } +} + +final class TargetGroupController +{ + #[ + OAT\Get(path: '/target_groups', operationId: 'groups', summary: 'List target groups', tags: ['Target groups']), + Successful(of: TargetGroupListDto::class) + ] + public function list(): string + { + } +} + +#[Schema(of: TargetGroupListDto::class)] +final class TargetGroupListDto +{ + public function __construct( + /** @var TargetGroupDto[] */ + #[Collection(of: TargetGroupDto::class)] + public readonly array $targetGroups = [] + ) { + } +} + +#[Schema(of: TargetGroupDto::class)] +final class TargetGroupDto +{ + public function __construct( + #[OAT\Property] # with my custom attribute #[Item] I also had problems + public readonly string $groupId, + #[OAT\Property] # with my custom attribute #[Item] I also had problems + public readonly string $groupName, + + /** @var TargetDto[] */ + #[Collection(of: TargetDto::class)] + /* Same ... + #[OAT\Property( + title: 'TargetDto', + items: new OAT\Items(ref: '#/components/schemas/TargetDto') + )] + */ + public readonly array $targets = [] + ) { + } +} + +#[Schema(of: TargetDto::class)] +final class TargetDto +{ + public function __construct( + #[Item(of: TargetId::class)] + public readonly string $targetId, + #[Item(of: TargetType::class)] + public readonly string $targetType, + // ... + ) { + } +} + +#[Schema(of: TargetId::class)] +class TargetId +{ +} + +#[Schema(of: TargetType::class)] +class TargetType +{ +} diff --git a/tests/Fixtures/Scratch/ComplexCustomAttributes.yaml b/tests/Fixtures/Scratch/ComplexCustomAttributes.yaml new file mode 100644 index 00000000..a2e25b93 --- /dev/null +++ b/tests/Fixtures/Scratch/ComplexCustomAttributes.yaml @@ -0,0 +1,63 @@ +openapi: 3.0.0 +info: + title: API + version: 1.0.0 +paths: + /target_groups: + get: + tags: + - 'Target groups' + summary: 'List target groups' + operationId: groups + responses: + '200': + description: 'Successful response of [TargetGroupListDto]' + content: + application/json: + schema: + $ref: '#/components/schemas/TargetGroupListDto' +components: + schemas: + TargetGroupListDto: + title: TargetGroupListDto + required: + - targetGroups + properties: + targetGroups: + title: TargetGroupDto + type: array + items: + $ref: '#/components/schemas/TargetGroupDto' + type: object + TargetGroupDto: + title: TargetGroupDto + required: + - groupId + - groupName + - targets + properties: + groupId: + type: string + groupName: + type: string + targets: + title: TargetDto + type: array + items: + $ref: '#/components/schemas/TargetDto' + type: object + TargetDto: + title: TargetDto + required: + - targetId + - targetType + properties: + targetId: + $ref: '#/components/schemas/TargetId' + targetType: + $ref: '#/components/schemas/TargetType' + type: object + TargetId: + title: TargetId + TargetType: + title: TargetType diff --git a/tests/Processors/ExpandEnumsTest.php b/tests/Processors/ExpandEnumsTest.php index f5dc1c76..c917e64d 100644 --- a/tests/Processors/ExpandEnumsTest.php +++ b/tests/Processors/ExpandEnumsTest.php @@ -8,7 +8,6 @@ use OpenApi\Analysers\TokenAnalyser; use OpenApi\Annotations as OA; -use OpenApi\Attributes as OAT; use OpenApi\Generator; use OpenApi\Processors\ExpandEnums; use OpenApi\Tests\Fixtures\PHP\Enums\StatusEnum; @@ -149,7 +148,7 @@ public function testExpandEnumClassString(array $files, string $title, mixed $ex { $analysis = $this->analysisFromFixtures($files); $analysis->process([new ExpandEnums()]); - $schemas = $analysis->getAnnotationsOfType([OA\Property::class, OAT\Property::class, OA\Items::class], true); + $schemas = $analysis->getAnnotationsOfType([OA\Property::class, OA\Items::class], true); foreach ($schemas as $schema) { if ($schema instanceof OA\Property || $schema instanceof OA\Items) {