From f46437ae2c3a7ce1f0b60809d058ebbc1f81cb9b Mon Sep 17 00:00:00 2001 From: Djordy Koert Date: Thu, 17 Oct 2024 17:05:17 +0200 Subject: [PATCH] refactor: determining property required logic (#2359) | Q | A | |---------------|---------------------------------------------------------------------------------------------------------------------------| | Bug fix? | yes | | New feature? | no | | Deprecations? |no | Move the behaviour for determining required property. --- config/services.xml | 4 --- phpstan-baseline.neon | 5 --- src/ModelDescriber/ObjectModelDescriber.php | 32 +++++++++++++++++++ .../PropertyDescriberInterface.php | 3 +- .../RequiredPropertyDescriber.php | 2 ++ .../MapQueryStringCleanupComponents.json | 4 +-- .../Fixtures/MapQueryStringController.json | 11 ++++--- .../Fixtures/MapRequestPayloadController.json | 1 + tests/Functional/FunctionalTest.php | 3 ++ .../ValidationGroupsFunctionalTest.php | 1 + 10 files changed, 49 insertions(+), 17 deletions(-) diff --git a/config/services.xml b/config/services.xml index 3382906f2..be9c3cc56 100644 --- a/config/services.xml +++ b/config/services.xml @@ -139,10 +139,6 @@ - - - - diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon index 6c228bec1..a7dd3f502 100644 --- a/phpstan-baseline.neon +++ b/phpstan-baseline.neon @@ -35,11 +35,6 @@ parameters: count: 1 path: src/PropertyDescriber/PropertyDescriberInterface.php - - - message: "#^PHPDoc tag @param references unknown parameter\\: \\$schema$#" - count: 1 - path: src/PropertyDescriber/PropertyDescriberInterface.php - - message: "#^Method Nelmio\\\\ApiDocBundle\\\\PropertyDescriber\\\\PropertyDescriberInterface\\:\\:describe\\(\\) invoked with 5 parameters, 2\\-3 required\\.$#" count: 1 diff --git a/src/ModelDescriber/ObjectModelDescriber.php b/src/ModelDescriber/ObjectModelDescriber.php index 752bf12c9..f511629b7 100644 --- a/src/ModelDescriber/ObjectModelDescriber.php +++ b/src/ModelDescriber/ObjectModelDescriber.php @@ -182,6 +182,8 @@ public function describe(Model $model, OA\Schema $schema) $this->describeProperty($types, $model, $property, $propertyName, $schema); } + + $this->markRequiredProperties($schema); } /** @@ -233,6 +235,36 @@ private function describeProperty(array $types, Model $model, OA\Schema $propert throw new \Exception(sprintf('Type "%s" is not supported in %s::$%s. You may use the `@OA\Property(type="")` annotation to specify it manually.', $types[0]->getBuiltinType(), $model->getType()->getClassName(), $propertyName)); } + /** + * Mark properties as required while ordering them in the same order as the properties of the schema. + * Then append the original required properties. + */ + private function markRequiredProperties(OA\Schema $schema): void + { + if (Generator::isDefault($properties = $schema->properties)) { + return; + } + + $newRequired = []; + foreach ($properties as $property) { + if (is_array($schema->required) && \in_array($property->property, $schema->required, true)) { + $newRequired[] = $property->property; + continue; + } + + if (true === $property->nullable || !Generator::isDefault($property->default)) { + continue; + } + $newRequired[] = $property->property; + } + + if ([] !== $newRequired) { + $originalRequired = Generator::isDefault($schema->required) ? [] : $schema->required; + + $schema->required = array_values(array_unique(array_merge($newRequired, $originalRequired))); + } + } + public function supports(Model $model): bool { return Type::BUILTIN_TYPE_OBJECT === $model->getType()->getBuiltinType() diff --git a/src/PropertyDescriber/PropertyDescriberInterface.php b/src/PropertyDescriber/PropertyDescriberInterface.php index feac04bb3..ce2e25c52 100644 --- a/src/PropertyDescriber/PropertyDescriberInterface.php +++ b/src/PropertyDescriber/PropertyDescriberInterface.php @@ -19,12 +19,11 @@ interface PropertyDescriberInterface /** * @param Type[] $types * @param string[]|null $groups Deprecated use $context['groups'] instead - * @param Schema $schema Allows to make changes inside of the schema (e.g. adding required fields) * @param array $context Context options for describing the property * * @return void */ - public function describe(array $types, Schema $property, ?array $groups = null /* , ?Schema $schema = null */ /* , array $context = [] */); + public function describe(array $types, Schema $property, ?array $groups = null /* , array $context = [] */); /** * @param Type[] $types diff --git a/src/PropertyDescriber/RequiredPropertyDescriber.php b/src/PropertyDescriber/RequiredPropertyDescriber.php index 1b8b70e68..9fe16aa9e 100644 --- a/src/PropertyDescriber/RequiredPropertyDescriber.php +++ b/src/PropertyDescriber/RequiredPropertyDescriber.php @@ -16,6 +16,8 @@ /** * Mark a property as required if it is not nullable. + * + * @deprecated {@see ObjectModelDescriber::markRequiredProperties()} */ final class RequiredPropertyDescriber implements PropertyDescriberInterface, PropertyDescriberAwareInterface { diff --git a/tests/Functional/Fixtures/MapQueryStringCleanupComponents.json b/tests/Functional/Fixtures/MapQueryStringCleanupComponents.json index 999263bcf..e34e9aa3b 100644 --- a/tests/Functional/Fixtures/MapQueryStringCleanupComponents.json +++ b/tests/Functional/Fixtures/MapQueryStringCleanupComponents.json @@ -148,7 +148,7 @@ { "name": "propertyArray[]", "in": "query", - "required": false, + "required": true, "schema": { "type": "array", "items": { @@ -304,7 +304,7 @@ "name": "productIds[]", "in": "query", "description": "List of product ids", - "required": false, + "required": true, "schema": { "description": "List of product ids", "type": "array", diff --git a/tests/Functional/Fixtures/MapQueryStringController.json b/tests/Functional/Fixtures/MapQueryStringController.json index d29e83361..0b53a3eb2 100644 --- a/tests/Functional/Fixtures/MapQueryStringController.json +++ b/tests/Functional/Fixtures/MapQueryStringController.json @@ -148,7 +148,7 @@ { "name": "propertyArray[]", "in": "query", - "required": false, + "required": true, "schema": { "type": "array", "items": { @@ -304,7 +304,7 @@ "name": "productIds[]", "in": "query", "description": "List of product ids", - "required": false, + "required": true, "schema": { "description": "List of product ids", "type": "array", @@ -1104,6 +1104,7 @@ "required": [ "property", "propertyInDefaultGroup", + "propertyArray", "propertyNotNullOnSpecificGroup" ], "properties": { @@ -1180,7 +1181,8 @@ }, "ArrayQueryModel": { "required": [ - "ids" + "ids", + "productIds" ], "properties": { "ids": { @@ -1248,7 +1250,8 @@ }, "ArrayQueryModel2": { "required": [ - "ids" + "ids", + "productIds" ], "properties": { "ids": { diff --git a/tests/Functional/Fixtures/MapRequestPayloadController.json b/tests/Functional/Fixtures/MapRequestPayloadController.json index 7fff78337..4e39dd338 100644 --- a/tests/Functional/Fixtures/MapRequestPayloadController.json +++ b/tests/Functional/Fixtures/MapRequestPayloadController.json @@ -148,6 +148,7 @@ "required": [ "property", "propertyInDefaultGroup", + "propertyArray", "propertyNotNullOnSpecificGroup" ], "properties": { diff --git a/tests/Functional/FunctionalTest.php b/tests/Functional/FunctionalTest.php index 6a534e4b1..0d4e89972 100644 --- a/tests/Functional/FunctionalTest.php +++ b/tests/Functional/FunctionalTest.php @@ -252,6 +252,9 @@ public function testUserModel(): void ], 'schema' => 'User', 'required' => [ + 'email', + 'location', + 'friendsNumber', 'creationDate', 'users', 'status', diff --git a/tests/Functional/ValidationGroupsFunctionalTest.php b/tests/Functional/ValidationGroupsFunctionalTest.php index 3e43eee2b..2cc94b84c 100644 --- a/tests/Functional/ValidationGroupsFunctionalTest.php +++ b/tests/Functional/ValidationGroupsFunctionalTest.php @@ -89,6 +89,7 @@ public function testConstraintDefaultGroupsAreRespectedWhenReadingAnnotations(): 'required' => [ 'property', 'propertyInDefaultGroup', + 'propertyArray', ], ];