nelmio / NelmioApiDocBundle

Generates documentation for your REST API from annotations
MIT License
2.21k stars 828 forks source link

[Bug]: Symfony assertions (NotBlank) not working in DTO classes #2287

Open heminei opened 1 month ago

heminei commented 1 month ago

Version

4.26.1

Description

I found issues with '\Symfony\Component\Validator\Constraints\NotBlank'. When merged fix for this bug https://github.com/nelmio/NelmioApiDocBundle/issues/2222 can't set the property required if it has default value.

Example:

<?php

namespace App\Dto\Api\V1\Response\Contact;

use App\Dto\Api\V1\Response\ResponseDto;
use App\Entity\Contact;

class GetContactsDto
{
    /**
     * @var Contact[]
     */
    protected array $items = [];

    #[\Symfony\Component\Validator\Constraints\NotBlank()]
    protected int $count = 0;

    /**
     * @return Contact[]
     */
    public function getItems(): array
    {
        return $this->items;
    }

    /**
     * Set the value of items.
     *
     * @param Contact[] $items
     *
     * @return self
     */
    public function setItems(array $items)
    {
        $this->items = $items;

        return $this;
    }

    /**
     * Get the value of count.
     */
    public function getCount(): int
    {
        return $this->count;
    }

    /**
     * Set the value of count.
     *
     * @return self
     */
    public function setCount(int $count)
    {
        $this->count = $count;

        return $this;
    }
}

JSON:

"GetContactsDto": {
                "required": [
                ],
                "properties": {
                    "items": {
                        "title": "Set the value of items.",
                        "type": "array",
                        "items": {
                            "$ref": "#/components/schemas/Contact"
                        },
                        "default": []
                    },
                    "count": {
                        "title": "Get the value of count.",
                        "type": "integer",
                        "default": 0
                    }
                },
                "type": "object"
            },

This is important when using the OpenAPI documentation in Angular for example. All properties of response DTO classes should be non-undefinable.

I will make a pull request later.

Additional context

No response

DominicLuidold commented 1 month ago

I found issues with '\Symfony\Component\Validator\Constraints\NotBlank'. When merged fix for this bug https://github.com/nelmio/NelmioApiDocBundle/issues/2222 can't set the property required if it has default value.

In my opinion, the current behavior is correct and works as intended.

Consider a scenario where the DTO is used not for a GET endpoint but for a POST or PUT API endpoint by your Angular application - the proposed changes in #2288 would create an inconsistency (which got caught by a failing test): When your Angular app sends data, it would be correct that $count is not marked as required, as the data and the DTO would still be valid even if the $count property wasn't included (since there is a fallback/default value). This means it is, in fact, an optional value.

This logic applies to your case as well, and I don't believe there is an easy solution within the Nelmio bundle. However, a simple fix for your problem could be to avoid defining the default value at the property level. Instead, you can define it within the getCount() method, as shown below (additionally, see https://3v4l.org/YlcuX):

class GetContactDto
{
    protected int $count;

    public function getCount(): int
    {
        return $this->count ?? 0;
    }
}
DjordyKoert commented 1 week ago

I think a way we could "fix" this is by introducing some kind of new attribute to make it a little bit easier for users to mark their property as required=true|false. This could then be done on the property level instead of on class level.

class GetContactsDto
{
    /**
     * @var Contact[]
     */
    #[Required]
    protected array $items = [];

Using the solution provided by @DominicLuidold seems a little bit hacky for this, but for now you should be able to overwrite the setting it as required with the OA\Schema attribute from swagger-php.

#[OA\Schema(required: ['items'])]
class GetContactsDto
{
...