From da02f3ad339437b939a6d309e9c98b62afe53c6c Mon Sep 17 00:00:00 2001
From: Christopher Davis <chris@pmg.com>
Date: Sat, 30 Apr 2022 13:28:05 -0500
Subject: [PATCH] Stop Model Property Description When a Schema Type or Ref is
 Already Defined (#1978)

* Return a Result Object from AnnotationsReader::updateDefinition

This is so we can make a decision on whether or not a schema's type or
ref has been manually defined by a user via an `@OA\Schema` annotation
as something other than an object.

If it has been defined, this bundle should not read model properties any
further as it causes errors.

I put this in AnnotationReader as it seemed the most flexible in the
long run. It could have gone in `OpenApiAnnotationsReader`, but then any
additional things added to `updateDefinition` could be left out of the
decision down the road. This is also a convenient place to decide this
once for `ObjectModelDescriber` and `JMSModelDescriber`.

* Stop Model Describer if a Schema Type or Ref Has Been Defined

Via the result object added in the previous commit.

This lets user "short circuit" the model describers by manually defining
the schema type or ref on a plain PHP object or form. For example,
a collection class could be defined like this:

    /**
     * @OA\Schema(type="array", @OA\Items(ref=@Model(type=SomeEntity::class)))
     */
     class SomeCollection implements \IteratorAggregate { }

Previously the model describer would error as it tries to merge the
`array` schema with the already defiend `object` schema. Now it will
prefer the array schema and skip reading all the properties of the
object.

* Add a Documentation Bit on Stopping Property Description

* Mark UpdateClassDefinitionResult as Internal
---
 .../Annotations/AnnotationsReader.php         | 18 ++++-
 .../UpdateClassDefinitionResult.php           | 41 +++++++++++
 ModelDescriber/FormModelDescriber.php         | 10 ++-
 ModelDescriber/JMSModelDescriber.php          |  8 ++-
 ModelDescriber/ObjectModelDescriber.php       | 10 ++-
 Resources/doc/faq.rst                         | 27 ++++++++
 .../Functional/Controller/ApiController80.php | 69 +++++++++++++++++++
 .../Entity/EntityWithAlternateType.php        | 35 ++++++++++
 .../Entity/EntityWithObjectType.php           | 25 +++++++
 Tests/Functional/Entity/EntityWithRef.php     | 25 +++++++
 .../Form/FormWithAlternateSchemaType.php      | 31 +++++++++
 Tests/Functional/Form/FormWithRefType.php     | 31 +++++++++
 Tests/Functional/FunctionalTest.php           | 45 ++++++++++++
 13 files changed, 366 insertions(+), 9 deletions(-)
 create mode 100644 ModelDescriber/Annotations/UpdateClassDefinitionResult.php
 create mode 100644 Tests/Functional/Entity/EntityWithAlternateType.php
 create mode 100644 Tests/Functional/Entity/EntityWithObjectType.php
 create mode 100644 Tests/Functional/Entity/EntityWithRef.php
 create mode 100644 Tests/Functional/Form/FormWithAlternateSchemaType.php
 create mode 100644 Tests/Functional/Form/FormWithRefType.php

diff --git a/ModelDescriber/Annotations/AnnotationsReader.php b/ModelDescriber/Annotations/AnnotationsReader.php
index 62641ab..261bd84 100644
--- a/ModelDescriber/Annotations/AnnotationsReader.php
+++ b/ModelDescriber/Annotations/AnnotationsReader.php
@@ -14,6 +14,7 @@ namespace Nelmio\ApiDocBundle\ModelDescriber\Annotations;
 use Doctrine\Common\Annotations\Reader;
 use Nelmio\ApiDocBundle\Model\ModelRegistry;
 use OpenApi\Annotations as OA;
+use OpenApi\Generator;
 
 /**
  * @internal
@@ -37,10 +38,14 @@ class AnnotationsReader
         $this->symfonyConstraintAnnotationReader = new SymfonyConstraintAnnotationReader($annotationsReader);
     }
 
-    public function updateDefinition(\ReflectionClass $reflectionClass, OA\Schema $schema): void
+    public function updateDefinition(\ReflectionClass $reflectionClass, OA\Schema $schema): UpdateClassDefinitionResult
     {
         $this->openApiAnnotationsReader->updateSchema($reflectionClass, $schema);
         $this->symfonyConstraintAnnotationReader->setSchema($schema);
+
+        return new UpdateClassDefinitionResult(
+            $this->shouldDescribeModelProperties($schema)
+        );
     }
 
     public function getPropertyName($reflection, string $default): string
@@ -54,4 +59,15 @@ class AnnotationsReader
         $this->phpDocReader->updateProperty($reflection, $property);
         $this->symfonyConstraintAnnotationReader->updateProperty($reflection, $property);
     }
+
+    /**
+     * if an objects schema type and ref are undefined OR the object was manually
+     * defined as an object, then we're good to do the normal describe flow of
+     * class properties.
+     */
+    private function shouldDescribeModelProperties(OA\Schema $schema): bool
+    {
+        return (Generator::UNDEFINED === $schema->type || 'object' === $schema->type)
+            && Generator::UNDEFINED === $schema->ref;
+    }
 }
diff --git a/ModelDescriber/Annotations/UpdateClassDefinitionResult.php b/ModelDescriber/Annotations/UpdateClassDefinitionResult.php
new file mode 100644
index 0000000..3a3945c
--- /dev/null
+++ b/ModelDescriber/Annotations/UpdateClassDefinitionResult.php
@@ -0,0 +1,41 @@
+<?php
+
+/*
+ * This file is part of the NelmioApiDocBundle package.
+ *
+ * (c) Nelmio
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Nelmio\ApiDocBundle\ModelDescriber\Annotations;
+
+/**
+ * result object returned from `AnnotationReader::updateDefinition` as a way
+ * to pass back information about manually defined schema elements.
+ *
+ * @internal
+ */
+final class UpdateClassDefinitionResult
+{
+    /**
+     * Whether or not the model describer shoudl continue reading class properties
+     * after updating the open api schema from an `OA\Schema` definition.
+     *
+     * Users may maually define a `type` or `ref` on a schema, and if that's the case
+     * model describers should _probably_ not describe any additional properties or try
+     * to merge in properties.
+     */
+    private $shouldDescribeModelProperties;
+
+    public function __construct(bool $shouldDescribeModelProperties)
+    {
+        $this->shouldDescribeModelProperties = $shouldDescribeModelProperties;
+    }
+
+    public function shouldDescribeModelProperties(): bool
+    {
+        return $this->shouldDescribeModelProperties;
+    }
+}
diff --git a/ModelDescriber/FormModelDescriber.php b/ModelDescriber/FormModelDescriber.php
index 093422b..7a398a8 100644
--- a/ModelDescriber/FormModelDescriber.php
+++ b/ModelDescriber/FormModelDescriber.php
@@ -63,12 +63,16 @@ final class FormModelDescriber implements ModelDescriberInterface, ModelRegistry
             throw new \LogicException('You need to enable forms in your application to use a form as a model.');
         }
 
-        $schema->type = 'object';
-
         $class = $model->getType()->getClassName();
 
         $annotationsReader = new AnnotationsReader($this->doctrineReader, $this->modelRegistry, $this->mediaTypes);
-        $annotationsReader->updateDefinition(new \ReflectionClass($class), $schema);
+        $classResult = $annotationsReader->updateDefinition(new \ReflectionClass($class), $schema);
+
+        if (!$classResult->shouldDescribeModelProperties()) {
+            return;
+        }
+
+        $schema->type = 'object';
 
         $form = $this->formFactory->create($class, null, $model->getOptions() ?? []);
         $this->parseForm($schema, $form);
diff --git a/ModelDescriber/JMSModelDescriber.php b/ModelDescriber/JMSModelDescriber.php
index 60a57ac..0a65c26 100644
--- a/ModelDescriber/JMSModelDescriber.php
+++ b/ModelDescriber/JMSModelDescriber.php
@@ -73,9 +73,13 @@ class JMSModelDescriber implements ModelDescriberInterface, ModelRegistryAwareIn
             throw new \InvalidArgumentException(sprintf('No metadata found for class %s.', $className));
         }
 
-        $schema->type = 'object';
         $annotationsReader = new AnnotationsReader($this->doctrineReader, $this->modelRegistry, $this->mediaTypes);
-        $annotationsReader->updateDefinition(new \ReflectionClass($className), $schema);
+        $classResult = $annotationsReader->updateDefinition(new \ReflectionClass($className), $schema);
+
+        if (!$classResult->shouldDescribeModelProperties()) {
+            return;
+        }
+        $schema->type = 'object';
 
         $isJmsV1 = null !== $this->namingStrategy;
 
diff --git a/ModelDescriber/ObjectModelDescriber.php b/ModelDescriber/ObjectModelDescriber.php
index 21f8d03..e10dba6 100644
--- a/ModelDescriber/ObjectModelDescriber.php
+++ b/ModelDescriber/ObjectModelDescriber.php
@@ -58,8 +58,6 @@ class ObjectModelDescriber implements ModelDescriberInterface, ModelRegistryAwar
 
     public function describe(Model $model, OA\Schema $schema)
     {
-        $schema->type = 'object';
-
         $class = $model->getType()->getClassName();
         $schema->_context->class = $class;
 
@@ -70,7 +68,13 @@ class ObjectModelDescriber implements ModelDescriberInterface, ModelRegistryAwar
 
         $reflClass = new \ReflectionClass($class);
         $annotationsReader = new AnnotationsReader($this->doctrineReader, $this->modelRegistry, $this->mediaTypes);
-        $annotationsReader->updateDefinition($reflClass, $schema);
+        $classResult = $annotationsReader->updateDefinition($reflClass, $schema);
+
+        if (!$classResult->shouldDescribeModelProperties()) {
+            return;
+        }
+
+        $schema->type = 'object';
 
         $discriminatorMap = $this->getAnnotation($reflClass, DiscriminatorMap::class);
         if ($discriminatorMap && Generator::UNDEFINED === $schema->discriminator) {
diff --git a/Resources/doc/faq.rst b/Resources/doc/faq.rst
index 6398a41..439dfb5 100644
--- a/Resources/doc/faq.rst
+++ b/Resources/doc/faq.rst
@@ -210,3 +210,30 @@ A: Use ``disable_default_routes`` config in your area.
         areas:
             default:
                 disable_default_routes: true
+
+Overriding a Form or Plain PHP Object Schema Type
+-------------------------------------------------
+
+Q: I'd like to define a PHP object or form with a type other any ``object``, how
+do I do that?
+
+A: By using the ``@OA\Schema`` annotation or attribute with a ``type`` or ``ref``.
+Note, however, that a ``type="object"`` will still read all a models properties.
+
+.. code-block:: php
+
+    <?php
+    use OpenApi\Annotations as OA;
+    use Nelmio\ApiDocBundle\Annotation\Model;
+
+    /**
+     * @OA\Schema(type="array", @OA\Items(ref=@Model(type=SomeEntity::class)))
+     *
+     * or define a `ref`:
+     * @OA\Schema(ref="#/components/schemas/SomeRef")
+     */
+    class SomeCollection implements \IteratorAggregate
+    {
+        // ...
+    }
+
diff --git a/Tests/Functional/Controller/ApiController80.php b/Tests/Functional/Controller/ApiController80.php
index b292e83..3d74a53 100644
--- a/Tests/Functional/Controller/ApiController80.php
+++ b/Tests/Functional/Controller/ApiController80.php
@@ -17,10 +17,15 @@ use Nelmio\ApiDocBundle\Annotation\Operation;
 use Nelmio\ApiDocBundle\Annotation\Security;
 use Nelmio\ApiDocBundle\Tests\Functional\Entity\Article;
 use Nelmio\ApiDocBundle\Tests\Functional\Entity\CompoundEntity;
+use Nelmio\ApiDocBundle\Tests\Functional\Entity\EntityWithAlternateType;
+use Nelmio\ApiDocBundle\Tests\Functional\Entity\EntityWithObjectType;
+use Nelmio\ApiDocBundle\Tests\Functional\Entity\EntityWithRef;
 use Nelmio\ApiDocBundle\Tests\Functional\Entity\SymfonyConstraints;
 use Nelmio\ApiDocBundle\Tests\Functional\Entity\SymfonyDiscriminator;
 use Nelmio\ApiDocBundle\Tests\Functional\Entity\User;
 use Nelmio\ApiDocBundle\Tests\Functional\Form\DummyType;
+use Nelmio\ApiDocBundle\Tests\Functional\Form\FormWithAlternateSchemaType;
+use Nelmio\ApiDocBundle\Tests\Functional\Form\FormWithRefType;
 use Nelmio\ApiDocBundle\Tests\Functional\Form\UserType;
 use OpenApi\Annotations as OA;
 use Symfony\Component\Routing\Annotation\Route;
@@ -259,4 +264,68 @@ class ApiController80
     public function customOperationIdAction()
     {
     }
+
+    /**
+     * @Route("/alternate-entity-type", methods={"GET", "POST"})
+     *
+     * @OA\Get(operationId="alternate-entity-type")
+     * @OA\Response(response=200, description="success", @OA\JsonContent(
+     *      ref=@Model(type=EntityWithAlternateType::class),
+     * ))
+     */
+    public function alternateEntityType()
+    {
+    }
+
+    /**
+     * @Route("/entity-with-ref", methods={"GET", "POST"})
+     *
+     * @OA\Get(operationId="entity-with-ref")
+     * @OA\Response(response=200, description="success", @OA\JsonContent(
+     *      ref=@Model(type=EntityWithRef::class),
+     * ))
+     */
+    public function entityWithRef()
+    {
+    }
+
+    /**
+     * @Route("/entity-with-object-type", methods={"GET", "POST"})
+     *
+     * @OA\Get(operationId="entity-with-object-type")
+     * @OA\Response(response=200, description="success", @OA\JsonContent(
+     *      ref=@Model(type=EntityWithObjectType::class),
+     * ))
+     */
+    public function entityWithObjectType()
+    {
+    }
+
+    /**
+     * @Route("/form-with-alternate-type", methods={"POST"})
+     * @OA\Response(
+     *    response="204",
+     *    description="Operation automatically detected",
+     * ),
+     * @OA\RequestBody(
+     *    @Model(type=FormWithAlternateSchemaType::class))
+     * )
+     */
+    public function formWithAlternateSchemaType()
+    {
+    }
+
+    /**
+     * @Route("/form-with-ref-type", methods={"POST"})
+     * @OA\Response(
+     *    response="204",
+     *    description="Operation automatically detected",
+     * ),
+     * @OA\RequestBody(
+     *    @Model(type=FormWithRefType::class))
+     * )
+     */
+    public function formWithRefSchemaType()
+    {
+    }
 }
diff --git a/Tests/Functional/Entity/EntityWithAlternateType.php b/Tests/Functional/Entity/EntityWithAlternateType.php
new file mode 100644
index 0000000..1b9538c
--- /dev/null
+++ b/Tests/Functional/Entity/EntityWithAlternateType.php
@@ -0,0 +1,35 @@
+<?php
+
+/*
+ * This file is part of the NelmioApiDocBundle package.
+ *
+ * (c) Nelmio
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Nelmio\ApiDocBundle\Tests\Functional\Entity;
+
+use ArrayIterator;
+use IteratorAggregate;
+use OpenApi\Annotations as OA;
+
+/**
+ * @OA\Schema(type="array", @OA\Items(type="string"))
+ */
+class EntityWithAlternateType implements IteratorAggregate
+{
+    /**
+     * @var string
+     */
+    public $ignored = 'this property should be ignored because of the annotation above';
+
+    public function getIterator(): ArrayIterator
+    {
+        return new ArrayIterator([
+            'abc',
+            'def',
+        ]);
+    }
+}
diff --git a/Tests/Functional/Entity/EntityWithObjectType.php b/Tests/Functional/Entity/EntityWithObjectType.php
new file mode 100644
index 0000000..ba0415c
--- /dev/null
+++ b/Tests/Functional/Entity/EntityWithObjectType.php
@@ -0,0 +1,25 @@
+<?php
+
+/*
+ * This file is part of the NelmioApiDocBundle package.
+ *
+ * (c) Nelmio
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Nelmio\ApiDocBundle\Tests\Functional\Entity;
+
+use OpenApi\Annotations as OA;
+
+/**
+ * @OA\Schema(type="object")
+ */
+class EntityWithObjectType
+{
+    /**
+     * @var string
+     */
+    public $notIgnored = 'this should be read';
+}
diff --git a/Tests/Functional/Entity/EntityWithRef.php b/Tests/Functional/Entity/EntityWithRef.php
new file mode 100644
index 0000000..f149bfb
--- /dev/null
+++ b/Tests/Functional/Entity/EntityWithRef.php
@@ -0,0 +1,25 @@
+<?php
+
+/*
+ * This file is part of the NelmioApiDocBundle package.
+ *
+ * (c) Nelmio
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Nelmio\ApiDocBundle\Tests\Functional\Entity;
+
+use OpenApi\Annotations as OA;
+
+/**
+ * @OA\Schema(ref="#/components/schemas/Test")
+ */
+class EntityWithRef
+{
+    /**
+     * @var string
+     */
+    public $ignored = 'this property should be ignored because of the annotation above';
+}
diff --git a/Tests/Functional/Form/FormWithAlternateSchemaType.php b/Tests/Functional/Form/FormWithAlternateSchemaType.php
new file mode 100644
index 0000000..68f1e5d
--- /dev/null
+++ b/Tests/Functional/Form/FormWithAlternateSchemaType.php
@@ -0,0 +1,31 @@
+<?php
+
+/*
+ * This file is part of the NelmioApiDocBundle package.
+ *
+ * (c) Nelmio
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Nelmio\ApiDocBundle\Tests\Functional\Form;
+
+use OpenApi\Annotations as OA;
+use Symfony\Component\Form\AbstractType;
+use Symfony\Component\Form\Extension\Core\Type\InputType;
+use Symfony\Component\Form\FormBuilderInterface;
+
+/**
+ * @OA\Schema(type="string")
+ */
+class FormWithAlternateSchemaType extends AbstractType
+{
+    public function buildForm(FormBuilderInterface $builder, array $options)
+    {
+        $builder
+            ->add('ignored', InputType::class, [
+                'required' => false,
+            ]);
+    }
+}
diff --git a/Tests/Functional/Form/FormWithRefType.php b/Tests/Functional/Form/FormWithRefType.php
new file mode 100644
index 0000000..7904230
--- /dev/null
+++ b/Tests/Functional/Form/FormWithRefType.php
@@ -0,0 +1,31 @@
+<?php
+
+/*
+ * This file is part of the NelmioApiDocBundle package.
+ *
+ * (c) Nelmio
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Nelmio\ApiDocBundle\Tests\Functional\Form;
+
+use OpenApi\Annotations as OA;
+use Symfony\Component\Form\AbstractType;
+use Symfony\Component\Form\Extension\Core\Type\InputType;
+use Symfony\Component\Form\FormBuilderInterface;
+
+/**
+ * @OA\Schema(ref="#/components/schemas/Test")
+ */
+class FormWithRefType extends AbstractType
+{
+    public function buildForm(FormBuilderInterface $builder, array $options)
+    {
+        $builder
+            ->add('ignored', InputType::class, [
+                'required' => false,
+            ]);
+    }
+}
diff --git a/Tests/Functional/FunctionalTest.php b/Tests/Functional/FunctionalTest.php
index 6ef59df..617a98d 100644
--- a/Tests/Functional/FunctionalTest.php
+++ b/Tests/Functional/FunctionalTest.php
@@ -635,4 +635,49 @@ class FunctionalTest extends WebTestCase
         $this->assertSame('string', $model->type);
         $this->assertCount(2, $model->enum);
     }
+
+    public function testEntitiesWithOverriddenSchemaTypeDoNotReadOtherProperties()
+    {
+        $model = $this->getModel('EntityWithAlternateType');
+
+        $this->assertSame('array', $model->type);
+        $this->assertSame('string', $model->items->type);
+        $this->assertSame(Generator::UNDEFINED, $model->properties);
+    }
+
+    public function testEntitiesWithRefInSchemaDoNoReadOtherProperties()
+    {
+        $model = $this->getModel('EntityWithRef');
+
+        $this->assertSame(Generator::UNDEFINED, $model->type);
+        $this->assertSame('#/components/schemas/Test', $model->ref);
+        $this->assertSame(Generator::UNDEFINED, $model->properties);
+    }
+
+    public function testEntitiesWithObjectTypeStillReadProperties()
+    {
+        $model = $this->getModel('EntityWithObjectType');
+
+        $this->assertSame('object', $model->type);
+        $this->assertCount(1, $model->properties);
+        $property = Util::getProperty($model, 'notIgnored');
+        $this->assertSame('string', $property->type);
+    }
+
+    public function testFormsWithOverriddenSchemaTypeDoNotReadOtherProperties()
+    {
+        $model = $this->getModel('FormWithAlternateSchemaType');
+
+        $this->assertSame('string', $model->type);
+        $this->assertSame(Generator::UNDEFINED, $model->properties);
+    }
+
+    public function testFormWithRefInSchemaDoNoReadOtherProperties()
+    {
+        $model = $this->getModel('FormWithRefType');
+
+        $this->assertSame(Generator::UNDEFINED, $model->type);
+        $this->assertSame('#/components/schemas/Test', $model->ref);
+        $this->assertSame(Generator::UNDEFINED, $model->properties);
+    }
 }