Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
heminei opened this issue May 22, 2024 · 3 comments · May be fixed by #2288
Open

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

heminei opened this issue May 22, 2024 · 3 comments · May be fixed by #2288
Labels

Comments

@heminei
Copy link

heminei commented May 22, 2024

Version

4.26.1

Description

I found issues with '\Symfony\Component\Validator\Constraints\NotBlank'. When merged fix for this bug #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

@heminei heminei added the bug label May 22, 2024
heminei added a commit to heminei/NelmioApiDocBundle that referenced this issue May 22, 2024
@DominicLuidold
Copy link
Contributor

DominicLuidold commented May 30, 2024

I found issues with '\Symfony\Component\Validator\Constraints\NotBlank'. When merged fix for this bug #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
Copy link
Collaborator

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
{
...

@mrazek-honza
Copy link

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
{
...

I think this would be great. There is still a lot of confusion about required and optional parameters in the resulting schema. More so, if the attribute can be nullable. Or when the attribute can be undefined but when it is, it has to be non-nullable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants