Generate non-nullable properties on model as required (#2141)

* Add Schema param to PropertyDescriberInterface * Move getSchemaPropertyName to Util * Implement logic to set non-nullable fields to required * Expand tests with required field * Make new describe $schema parameter optional to prevent BC * Use NullablePropertyTrait when describing an object property * Add dummy to required * fix style
nelmio · Jan 2, 2024 · 0dae4c0 · 0dae4c0
1 parent c55d9ef
commit 0dae4c0
Show file tree

Hide file tree

Showing 15 changed files with 97 additions and 40 deletions.
diff --git a/ModelDescriber/Annotations/SymfonyConstraintAnnotationReader.php b/ModelDescriber/Annotations/SymfonyConstraintAnnotationReader.php
@@ -79,7 +79,7 @@ private function processPropertyAnnotations($reflection, OA\Property $property,
                     return;
                 }
 
-                $propertyName = $this->getSchemaPropertyName($property);
+                $propertyName = Util::getSchemaPropertyName($this->schema, $property);
                 if (null === $propertyName) {
                     return;
                 }
@@ -132,23 +132,6 @@ public function setSchema($schema): void
         $this->schema = $schema;
     }
 
-    /**
-     * Get assigned property name for property schema.
-     */
-    private function getSchemaPropertyName(OA\Schema $property): ?string
-    {
-        if (null === $this->schema) {
-            return null;
-        }
-        foreach ($this->schema->properties as $schemaProperty) {
-            if ($schemaProperty === $property) {
-                return Generator::UNDEFINED !== $schemaProperty->property ? $schemaProperty->property : null;
-            }
-        }
-
-        return null;
-    }
-
     /**
      * Append the pattern from the constraint to the existing pattern.
      */

diff --git a/ModelDescriber/ObjectModelDescriber.php b/ModelDescriber/ObjectModelDescriber.php
@@ -147,7 +147,7 @@ public function describe(Model $model, OA\Schema $schema)
                 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));
             }
 
-            $this->describeProperty($types, $model, $property, $propertyName);
+            $this->describeProperty($types, $model, $property, $propertyName, $schema);
         }
     }
 
@@ -182,15 +182,15 @@ private function camelize(string $string): string
     /**
      * @param Type[] $types
      */
-    private function describeProperty(array $types, Model $model, OA\Schema $property, string $propertyName)
+    private function describeProperty(array $types, Model $model, OA\Schema $property, string $propertyName, OA\Schema $schema)
     {
         foreach ($this->propertyDescribers as $propertyDescriber) {
             if ($propertyDescriber instanceof ModelRegistryAwareInterface) {
                 $propertyDescriber->setModelRegistry($this->modelRegistry);
             }
             if ($propertyDescriber->supports($types)) {
                 try {
-                    $propertyDescriber->describe($types, $property, $model->getGroups());
+                    $propertyDescriber->describe($types, $property, $model->getGroups(), $schema);
                 } catch (UndocumentedArrayItemsException $e) {
                     if (null !== $e->getClass()) {
                         throw $e; // This exception is already complete

diff --git a/OpenApiPhp/Util.php b/OpenApiPhp/Util.php
@@ -370,6 +370,24 @@ public static function merge(OA\AbstractAnnotation $annotation, $from, bool $ove
         }
     }
 
+    /**
+     * Get assigned property name for property schema.
+     */
+    public static function getSchemaPropertyName(OA\Schema $schema, OA\Schema $property): ?string
+    {
+        if (Generator::UNDEFINED === $schema->properties) {
+            return null;
+        }
+
+        foreach ($schema->properties as $schemaProperty) {
+            if ($schemaProperty === $property) {
+                return Generator::UNDEFINED !== $schemaProperty->property ? $schemaProperty->property : null;
+            }
+        }
+
+        return null;
+    }
+
     private static function mergeFromArray(OA\AbstractAnnotation $annotation, array $properties, bool $overwrite)
     {
         $done = [];

diff --git a/PropertyDescriber/ArrayPropertyDescriber.php b/PropertyDescriber/ArrayPropertyDescriber.php
@@ -30,7 +30,7 @@ public function __construct(iterable $propertyDescribers = [])
         $this->propertyDescribers = $propertyDescribers;
     }
 
-    public function describe(array $types, OA\Schema $property, array $groups = null)
+    public function describe(array $types, OA\Schema $property, array $groups = null, ?OA\Schema $schema = null)
     {
         // BC layer for symfony < 5.3
         $type = method_exists($types[0], 'getCollectionValueTypes') ?
@@ -41,7 +41,7 @@ public function describe(array $types, OA\Schema $property, array $groups = null
         }
 
         $property->type = 'array';
-        $this->setNullableProperty($types[0], $property);
+        $this->setNullableProperty($types[0], $property, $schema);
         $property = Util::getChild($property, OA\Items::class);
 
         foreach ($this->propertyDescribers as $propertyDescriber) {
@@ -50,7 +50,7 @@ public function describe(array $types, OA\Schema $property, array $groups = null
             }
             if ($propertyDescriber->supports([$type])) {
                 try {
-                    $propertyDescriber->describe([$type], $property, $groups);
+                    $propertyDescriber->describe([$type], $property, $groups, $schema);
                 } catch (UndocumentedArrayItemsException $e) {
                     if (null !== $e->getClass()) {
                         throw $e; // This exception is already complete

diff --git a/PropertyDescriber/BooleanPropertyDescriber.php b/PropertyDescriber/BooleanPropertyDescriber.php
@@ -18,10 +18,10 @@ class BooleanPropertyDescriber implements PropertyDescriberInterface
 {
     use NullablePropertyTrait;
 
-    public function describe(array $types, OA\Schema $property, array $groups = null)
+    public function describe(array $types, OA\Schema $property, array $groups = null, ?OA\Schema $schema = null)
     {
         $property->type = 'boolean';
-        $this->setNullableProperty($types[0], $property);
+        $this->setNullableProperty($types[0], $property, $schema);
     }
 
     public function supports(array $types): bool

diff --git a/PropertyDescriber/CompoundPropertyDescriber.php b/PropertyDescriber/CompoundPropertyDescriber.php
@@ -29,7 +29,7 @@ public function __construct(iterable $propertyDescribers)
         $this->propertyDescribers = $propertyDescribers;
     }
 
-    public function describe(array $types, OA\Schema $property, array $groups = null)
+    public function describe(array $types, OA\Schema $property, array $groups = null, ?OA\Schema $schema = null)
     {
         $property->oneOf = Generator::UNDEFINED !== $property->oneOf ? $property->oneOf : [];
 
@@ -40,7 +40,7 @@ public function describe(array $types, OA\Schema $property, array $groups = null
                     $propertyDescriber->setModelRegistry($this->modelRegistry);
                 }
                 if ($propertyDescriber->supports([$type])) {
-                    $propertyDescriber->describe([$type], $schema, $groups);
+                    $propertyDescriber->describe([$type], $schema, $groups, $schema);
 
                     break;
                 }

diff --git a/PropertyDescriber/DateTimePropertyDescriber.php b/PropertyDescriber/DateTimePropertyDescriber.php
@@ -18,11 +18,11 @@ class DateTimePropertyDescriber implements PropertyDescriberInterface
 {
     use NullablePropertyTrait;
 
-    public function describe(array $types, OA\Schema $property, array $groups = null)
+    public function describe(array $types, OA\Schema $property, array $groups = null, ?OA\Schema $schema = null)
     {
         $property->type = 'string';
         $property->format = 'date-time';
-        $this->setNullableProperty($types[0], $property);
+        $this->setNullableProperty($types[0], $property, $schema);
     }
 
     public function supports(array $types): bool

diff --git a/PropertyDescriber/FloatPropertyDescriber.php b/PropertyDescriber/FloatPropertyDescriber.php
@@ -18,11 +18,11 @@ class FloatPropertyDescriber implements PropertyDescriberInterface
 {
     use NullablePropertyTrait;
 
-    public function describe(array $types, OA\Schema $property, array $groups = null)
+    public function describe(array $types, OA\Schema $property, array $groups = null, ?OA\Schema $schema = null)
     {
         $property->type = 'number';
         $property->format = 'float';
-        $this->setNullableProperty($types[0], $property);
+        $this->setNullableProperty($types[0], $property, $schema);
     }
 
     public function supports(array $types): bool

diff --git a/PropertyDescriber/IntegerPropertyDescriber.php b/PropertyDescriber/IntegerPropertyDescriber.php
@@ -18,10 +18,10 @@ class IntegerPropertyDescriber implements PropertyDescriberInterface
 {
     use NullablePropertyTrait;
 
-    public function describe(array $types, OA\Schema $property, array $groups = null)
+    public function describe(array $types, OA\Schema $property, array $groups = null, ?OA\Schema $schema = null)
     {
         $property->type = 'integer';
-        $this->setNullableProperty($types[0], $property);
+        $this->setNullableProperty($types[0], $property, $schema);
     }
 
     public function supports(array $types): bool

diff --git a/PropertyDescriber/NullablePropertyTrait.php b/PropertyDescriber/NullablePropertyTrait.php
@@ -11,21 +11,38 @@
 
 namespace Nelmio\ApiDocBundle\PropertyDescriber;
 
+use Nelmio\ApiDocBundle\OpenApiPhp\Util;
 use OpenApi\Annotations as OA;
 use OpenApi\Generator;
 use Symfony\Component\PropertyInfo\Type;
 
 trait NullablePropertyTrait
 {
-    protected function setNullableProperty(Type $type, OA\Schema $property): void
+    protected function setNullableProperty(Type $type, OA\Schema $property, ?OA\Schema $schema): void
     {
         if (Generator::UNDEFINED !== $property->nullable) {
             if (!$property->nullable) {
                 // if already false mark it as undefined (so it does not show up as `nullable: false`)
                 $property->nullable = Generator::UNDEFINED;
             }
-        } elseif ($type->isNullable()) {
+
+            return;
+        }
+
+        if ($type->isNullable()) {
             $property->nullable = true;
         }
+
+        if (!$type->isNullable() && null !== $schema) {
+            $propertyName = Util::getSchemaPropertyName($schema, $property);
+            if (null === $propertyName) {
+                return;
+            }
+
+            $existingRequiredFields = Generator::UNDEFINED !== $schema->required ? $schema->required : [];
+            $existingRequiredFields[] = $propertyName;
+
+            $schema->required = array_values(array_unique($existingRequiredFields));
+        }
     }
 }
diff --git a/PropertyDescriber/ObjectPropertyDescriber.php b/PropertyDescriber/ObjectPropertyDescriber.php
@@ -16,13 +16,14 @@
 use Nelmio\ApiDocBundle\Model\Model;
 use Nelmio\ApiDocBundle\OpenApiPhp\Util;
 use OpenApi\Annotations as OA;
+use OpenApi\Generator;
 use Symfony\Component\PropertyInfo\Type;
 
 class ObjectPropertyDescriber implements PropertyDescriberInterface, ModelRegistryAwareInterface
 {
     use ModelRegistryAwareTrait;
 
-    public function describe(array $types, OA\Schema $property, array $groups = null)
+    public function describe(array $types, OA\Schema $property, array $groups = null, ?OA\Schema $schema = null)
     {
         $type = new Type(
             $types[0]->getBuiltinType(),
@@ -45,6 +46,18 @@ public function describe(array $types, OA\Schema $property, array $groups = null
         }
 
         $property->ref = $this->modelRegistry->register(new Model($type, $groups));
+
+        if (!$type->isNullable() && null !== $schema) {
+            $propertyName = Util::getSchemaPropertyName($schema, $property);
+            if (null === $propertyName) {
+                return;
+            }
+
+            $existingRequiredFields = Generator::UNDEFINED !== $schema->required ? $schema->required : [];
+            $existingRequiredFields[] = $propertyName;
+
+            $schema->required = array_values(array_unique($existingRequiredFields));
+        }
     }
 
     public function supports(array $types): bool

diff --git a/PropertyDescriber/PropertyDescriberInterface.php b/PropertyDescriber/PropertyDescriberInterface.php
@@ -18,8 +18,9 @@ interface PropertyDescriberInterface
 {
     /**
      * @param Type[] $types
+     * @param Schema $schema Allows to make changes inside of the schema (e.g. adding required fields)
      */
-    public function describe(array $types, Schema $property, array $groups = null);
+    public function describe(array $types, Schema $property, array $groups = null /* , ?Schema $schema = null */);
 
     /**
      * @param Type[] $types

diff --git a/PropertyDescriber/StringPropertyDescriber.php b/PropertyDescriber/StringPropertyDescriber.php
@@ -18,10 +18,10 @@ class StringPropertyDescriber implements PropertyDescriberInterface
 {
     use NullablePropertyTrait;
 
-    public function describe(array $types, OA\Schema $property, array $groups = null)
+    public function describe(array $types, OA\Schema $property, array $groups = null, ?OA\Schema $schema = null)
     {
         $property->type = 'string';
-        $this->setNullableProperty($types[0], $property);
+        $this->setNullableProperty($types[0], $property, $schema);
     }
 
     public function supports(array $types): bool

diff --git a/Tests/Functional/FunctionalTest.php b/Tests/Functional/FunctionalTest.php
@@ -246,6 +246,16 @@ public function testUserModel()
                     ],
                 ],
                 'schema' => 'User',
+                'required' => [
+                    'id',
+                    'roles',
+                    'money',
+                    'creationDate',
+                    'users',
+                    'status',
+                    'dateAsInterface',
+                    'dummy',
+                ],
             ],
             json_decode($this->getModel('User')->toJson(), true)
         );
@@ -438,6 +448,17 @@ public function testSymfonyConstraintDocumentation()
             'required' => [
                 'propertyNotBlank',
                 'propertyNotNull',
+                'propertyAssertLength',
+                'propertyRegex',
+                'propertyCount',
+                'propertyChoice',
+                'propertyChoiceWithCallback',
+                'propertyChoiceWithCallbackWithoutClass',
+                'propertyChoiceWithMultiple',
+                'propertyExpression',
+                'propertyRange',
+                'propertyLessThan',
+                'propertyLessThanOrEqual',
             ],
             'properties' => [
                 'propertyNotBlank' => [

diff --git a/Tests/Functional/ValidationGroupsFunctionalTest.php b/Tests/Functional/ValidationGroupsFunctionalTest.php
@@ -75,6 +75,10 @@ public function testConstraintDefaultGroupsAreRespectedWhenReadingAnnotations()
             ],
             'type' => 'object',
             'schema' => 'SymfonyConstraintsDefaultGroup',
+            'required' => [
+                'property',
+                'propertyInDefaultGroup',
+            ],
         ];
 
         $this->assertEquals(