Fix marking falsy default values as required (#2186)

* Add test to verify failure of falsy defaults * Add default value from reflection This change provides a means to add the default value from the reflection of a property should the default value not have been set previously via a `default` parameter of an attribute or annotation. Also we check before setting anything to nullable whether a default has been set either via the reflection of via the default property of the schema. In those cases we do not add the property to the required properties as it does have a default * Set defaults only on PHP8 or above The used ReflectionProperty methods are not available before PHP8, therefore we can only execute them there. Affected tests will be skipped on PHP7 * Make class usable in PHP8 and PHP7 This makes sure that the class does not have typehinted properties as those do not work unless PHP7.4 * move upstream changes to required generation to RequiredPropertyDescriber * move upstream changes to required generation to RequiredPropertyDescriber * fix baseline * move reflection logic to ReflectionReader * use isDefault method * use isDefault method * Revert "use isDefault method" This reverts commit 69c5e04. * use PHP_VERSION_ID * use simpler retrieval of default property values * update falsy defaults test * remove test skip --------- Co-authored-by: DjordyKoert <djordy.koert@yoursurprise.com>
nelmio · Jan 11, 2024 · f43a4ba · f43a4ba
1 parent b200bc1
commit f43a4ba
Show file tree

Hide file tree

Showing 10 changed files with 150 additions and 11 deletions.
diff --git a/ModelDescriber/JMSModelDescriber.php b/ModelDescriber/JMSModelDescriber.php
@@ -164,6 +164,11 @@ public function describe(Model $model, OA\Schema $schema)
 
                 continue;
             }
+
+            if (Generator::UNDEFINED === $property->default && $item->hasDefault) {
+                $property->default = $item->defaultValue;
+            }
+
             if (null === $item->type) {
                 $key = Util::searchIndexedCollectionItem($schema->properties, 'property', $name);
                 unset($schema->properties[$key]);
@@ -248,7 +253,7 @@ public function describeItem(array $type, OA\Schema $property, Context $context)
     {
         $nestedTypeInfo = $this->getNestedTypeInArray($type);
         if (null !== $nestedTypeInfo) {
-            list($nestedType, $isHash) = $nestedTypeInfo;
+            [$nestedType, $isHash] = $nestedTypeInfo;
             if ($isHash) {
                 $property->type = 'object';
                 $property->additionalProperties = Util::createChild($property, OA\Property::class);

diff --git a/ModelDescriber/ObjectModelDescriber.php b/ModelDescriber/ObjectModelDescriber.php
@@ -126,6 +126,10 @@ public function describe(Model $model, OA\Schema $schema)
         // The SerializerExtractor does expose private/protected properties for some reason, so we eliminate them here
         $propertyInfoProperties = array_intersect($propertyInfoProperties, $this->propertyInfo->getProperties($class, []) ?? []);
 
+        $defaultValues = array_filter($reflClass->getDefaultProperties(), static function ($value) {
+            return null !== $value;
+        });
+
         foreach ($propertyInfoProperties as $propertyName) {
             $serializedName = null !== $this->nameConverter ? $this->nameConverter->normalize($propertyName, $class, null, $model->getSerializationContext()) : $propertyName;
 
@@ -152,6 +156,10 @@ public function describe(Model $model, OA\Schema $schema)
                 continue;
             }
 
+            if (Generator::UNDEFINED === $property->default && array_key_exists($propertyName, $defaultValues)) {
+                $property->default = $defaultValues[$propertyName];
+            }
+
             $types = $this->propertyInfo->getTypes($class, $propertyName);
             if (null === $types || 0 === count($types)) {
                 throw new \LogicException(sprintf('The PropertyInfo component was not able to guess the type of %s::$%s. You may need to add a `@var` annotation or use `@OA\Property(type="")` to make its type explicit.', $class, $propertyName));

diff --git a/PropertyDescriber/NullablePropertyTrait.php b/PropertyDescriber/NullablePropertyTrait.php
@@ -36,6 +36,10 @@ protected function setNullableProperty(Type $type, OA\Schema $property, ?OA\Sche
             $property->nullable = true;
         }
 
+        if (!$type->isNullable() && Generator::UNDEFINED !== $property->default) {
+            return;
+        }
+
         if (!$type->isNullable() && null !== $schema) {
             $propertyName = Util::getSchemaPropertyName($schema, $property);
             if (null === $propertyName) {

diff --git a/PropertyDescriber/RequiredPropertyDescriber.php b/PropertyDescriber/RequiredPropertyDescriber.php
@@ -29,12 +29,18 @@ public function describe(array $types, OA\Schema $property, array $groups = null
             return;
         }
 
-        if (true !== $property->nullable) {
-            $existingRequiredFields = Generator::UNDEFINED !== $schema->required ? $schema->required : [];
-            $existingRequiredFields[] = $property->property;
+        if (null === $schema) {
+            return;
+        }
 
-            $schema->required = array_values(array_unique($existingRequiredFields));
+        if (true === $property->nullable || !Generator::isDefault($property->default)) {
+            return;
         }
+
+        $existingRequiredFields = Generator::UNDEFINED !== $schema->required ? $schema->required : [];
+        $existingRequiredFields[] = $property->property;
+
+        $schema->required = array_values(array_unique($existingRequiredFields));
     }
 
     public function supports(array $types): bool

diff --git a/Tests/Functional/Controller/ApiController80.php b/Tests/Functional/Controller/ApiController80.php
@@ -21,6 +21,7 @@
 use Nelmio\ApiDocBundle\Tests\Functional\Entity\CompoundEntity;
 use Nelmio\ApiDocBundle\Tests\Functional\Entity\EntityThroughNameConverter;
 use Nelmio\ApiDocBundle\Tests\Functional\Entity\EntityWithAlternateType80;
+use Nelmio\ApiDocBundle\Tests\Functional\Entity\EntityWithFalsyDefaults;
 use Nelmio\ApiDocBundle\Tests\Functional\Entity\EntityWithNullableSchemaSet;
 use Nelmio\ApiDocBundle\Tests\Functional\Entity\EntityWithObjectType;
 use Nelmio\ApiDocBundle\Tests\Functional\Entity\EntityWithRef;
@@ -457,6 +458,22 @@ public function entityWithNullableSchemaSet()
     {
     }
 
+    /**
+     * @Route("/entity-with-falsy-defaults", methods={"POST"})
+     *
+     * @OA\Response(
+     *     response="204",
+     *     description="Operation automatically detected",
+     *  ),
+     *
+     * @OA\RequestBody(
+     *
+     *     @Model(type=EntityWithFalsyDefaults::class))
+     *  )*/
+    public function entityWithFalsyDefaults()
+    {
+    }
+
     /**
      * @OA\Response(
      *     response="200",

diff --git a/Tests/Functional/Controller/ApiController81.php b/Tests/Functional/Controller/ApiController81.php
@@ -22,6 +22,7 @@
 use Nelmio\ApiDocBundle\Tests\Functional\Entity\CompoundEntity;
 use Nelmio\ApiDocBundle\Tests\Functional\Entity\EntityThroughNameConverter;
 use Nelmio\ApiDocBundle\Tests\Functional\Entity\EntityWithAlternateType81;
+use Nelmio\ApiDocBundle\Tests\Functional\Entity\EntityWithFalsyDefaults;
 use Nelmio\ApiDocBundle\Tests\Functional\Entity\EntityWithNullableSchemaSet;
 use Nelmio\ApiDocBundle\Tests\Functional\Entity\EntityWithObjectType;
 use Nelmio\ApiDocBundle\Tests\Functional\Entity\EntityWithRef;
@@ -368,6 +369,18 @@ public function entityWithNullableSchemaSet()
     {
     }
 
+    #[Route('/entity-with-falsy-defaults', methods: ['GET'])]
+    #[OA\Response(
+        response: 204,
+        description: 'Operation automatically detected',
+    )]
+    #[OA\RequestBody(
+        content: new Model(type: EntityWithFalsyDefaults::class),
+    )]
+    public function entityWithFalsyDefaults()
+    {
+    }
+
     #[OA\Get(responses: [
         new OA\Response(
             response: '200',

diff --git a/Tests/Functional/Entity/EntityWithFalsyDefaults.php b/Tests/Functional/Entity/EntityWithFalsyDefaults.php
@@ -0,0 +1,33 @@
+<?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;
+
+class EntityWithFalsyDefaults
+{
+    /** @var int */
+    public $zero = 0;
+
+    /** @var float */
+    public $float = 0.0;
+
+    /** @var string */
+    public $empty = '';
+
+    /** @var bool */
+    public $false = false;
+
+    /** @var string|null */
+    public $nullString = null;
+
+    /** @var string[] */
+    public $array = [];
+}
diff --git a/Tests/Functional/FunctionalTest.php b/Tests/Functional/FunctionalTest.php
@@ -247,9 +247,6 @@ public function testUserModel()
                 ],
                 'schema' => 'User',
                 'required' => [
-                    'id',
-                    'roles',
-                    'money',
                     'creationDate',
                     'users',
                     'status',
@@ -1176,4 +1173,46 @@ public function testArbitraryArrayModel()
             'type' => 'object',
         ], json_decode($this->getModel('Bar')->toJson(), true));
     }
+
+    public function testEntityWithFalsyDefaults()
+    {
+        $model = $this->getModel('EntityWithFalsyDefaults');
+
+        $this->assertSame(Generator::UNDEFINED, $model->required);
+
+        self::assertEquals([
+            'schema' => 'EntityWithFalsyDefaults',
+            'type' => 'object',
+            'properties' => [
+                'zero' => [
+                    'type' => 'integer',
+                    'default' => 0,
+                ],
+                'float' => [
+                    'type' => 'number',
+                    'format' => 'float',
+                    'default' => 0.0,
+                ],
+                'empty' => [
+                    'type' => 'string',
+                    'default' => '',
+                ],
+                'false' => [
+                    'type' => 'boolean',
+                    'default' => false,
+                ],
+                'nullString' => [
+                    'nullable' => true,
+                    'type' => 'string',
+                ],
+                'array' => [
+                    'type' => 'array',
+                    'items' => [
+                        'type' => 'string',
+                    ],
+                    'default' => [],
+                ],
+            ],
+        ], json_decode($model->toJson(), true));
+    }
 }
diff --git a/Tests/Functional/JMSFunctionalTest.php b/Tests/Functional/JMSFunctionalTest.php
@@ -326,8 +326,8 @@ public function testNamingStrategyWithConstraints()
             'properties' => [
                 'beautifulName' => [
                     'type' => 'string',
-                    'maxLength' => '10',
-                    'minLength' => '3',
+                    'maxLength' => 10,
+                    'minLength' => 3,
                 ],
             ],
             'required' => ['beautifulName'],

diff --git a/phpunit-baseline.json b/phpunit-baseline.json
@@ -6934,7 +6934,6 @@
         "message": "Since sensio/framework-extra-bundle 5.2: The \"sensio_framework_extra.routing.loader.annot_file\" service is deprecated since version 5.2",
         "count": 1
     },
-
     {
         "location": "Nelmio\\ApiDocBundle\\Tests\\Functional\\FunctionalTest::testArbitraryArrayModel",
         "message": "Since sensio/framework-extra-bundle 5.2: The \"sensio_framework_extra.routing.loader.annot_class\" service is deprecated since version 5.2",
@@ -6949,5 +6948,20 @@
         "location": "Nelmio\\ApiDocBundle\\Tests\\Functional\\FunctionalTest::testArbitraryArrayModel",
         "message": "Since sensio/framework-extra-bundle 5.2: The \"sensio_framework_extra.routing.loader.annot_file\" service is deprecated since version 5.2",
         "count": 1
+    },
+    {
+        "location": "Nelmio\\ApiDocBundle\\Tests\\Functional\\FunctionalTest::testEntityWithFalsyDefaults",
+        "message": "Since sensio/framework-extra-bundle 5.2: The \"sensio_framework_extra.routing.loader.annot_class\" service is deprecated since version 5.2",
+        "count": 1
+    },
+    {
+        "location": "Nelmio\\ApiDocBundle\\Tests\\Functional\\FunctionalTest::testEntityWithFalsyDefaults",
+        "message": "Since sensio/framework-extra-bundle 5.2: The \"sensio_framework_extra.routing.loader.annot_dir\" service is deprecated since version 5.2",
+        "count": 1
+    },
+    {
+        "location": "Nelmio\\ApiDocBundle\\Tests\\Functional\\FunctionalTest::testEntityWithFalsyDefaults",
+        "message": "Since sensio/framework-extra-bundle 5.2: The \"sensio_framework_extra.routing.loader.annot_file\" service is deprecated since version 5.2",
+        "count": 1
     }
 ]