Skip to content

#[MapRequestPayload] does not handle invalid enums gracefully #52705

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

Open
cgrabenstein opened this issue Nov 23, 2023 · 11 comments
Open

#[MapRequestPayload] does not handle invalid enums gracefully #52705

cgrabenstein opened this issue Nov 23, 2023 · 11 comments

Comments

@cgrabenstein
Copy link

cgrabenstein commented Nov 23, 2023

Symfony version(s) affected

6.3.8

Description

The #[MapRequestPayload] attribute handles invalid input (wrong type, missing fields) by returning a 422 Response and a message hinting at the problem(s). However when sending an invalid enum value, the result is a 500 Response.

How to reproduce

Given the following setup

enum Foo: string 
{
    case BAR = 'bar';
}

class Input 
{
    public function __construct(public readonly Foo $foo) {}
}

class TestController
{
    #[Route(...)]
    public function test(#[MapRequestPayload] Input $input)
    {
        // ...
    }
}

If you send a request containing the following Json

{
    "foo": "baz"
}

You get a 500 Response, even though a 422 would have been more sensible. The exception message is

[critical] Uncaught PHP Exception Symfony\Component\Serializer\Exception\InvalidArgumentException: "The data must belong to a backed enumeration of type Foo" at vendor/symfony/serializer/Normalizer/BackedEnumNormalizer.php line 80

Possible Solution

No response

Additional Context

The BackedEnumNormalizer handles failure differently depending on whether the has_constructor flag in the $context is set:

try {
    return $type::from($data);
} catch (\ValueError $e) {
    if (isset($context['has_constructor'])) {
        throw new InvalidArgumentException('The data must belong to a backed enumeration of type '.$type);
    }

    throw NotNormalizableValueException::createForUnexpectedDataType('The data must belong to a backed enumeration of type '.$type, $data, [$type], $context['deserialization_path'] ?? null, true, 0, $e);
}

The NotNormalizableValueException case is handled in the RequestPayloadValueResolver, the InvalidArgumentException case is not.

@SebLevDev
Copy link
Contributor

SebLevDev commented Dec 9, 2023

Hello, I can try a PR during the #SymfonyHackday

@SebLevDev
Copy link
Contributor

SebLevDev commented Dec 9, 2023

The bug is introduce with #47128 and maybe already a fix with #50201
The #50192 don't also solve the issue

In these condition what is the best way to fix that ?

@cgrabenstein
Copy link
Author

Instead of focusing on the Serializer, one could also catch and handle the Serializer's InvalidArgumentException in the RequestPayloadValueResolver. However I don't know if it was a deliberate choice not to catch the InvalidArgumentException already.

Maybe @Koc can chime in about that?

@SebLevDev
Copy link
Contributor

SebLevDev commented Dec 11, 2023

During the SymfonyHackday, after discussing with @dunglas and @nicolas-grekas , we explored two initial approaches to address this without making significant changes to the Serializer but by working at the level of the RequestPayloadValueResolver:

  • Caught the Serializer's InvalidArgumentException at the RequestPayloadValueResolver level to convert the 500 error to a 422 error.
  • Similarly, but replaced InvalidArgumentException with a new Exception class extending it for a more precise catch.

Upon delving deeper into the code, I then came across the concept of PartialDenormalizationException. The first two approaches do not allow for surfacing the error in the same way as for errors on other types of a DTO by returning a collection of violations. Having a consistent solution when COLLECT_DENORMALIZATION_ERRORS = true would be a plus IMO

I don't know what to do after reading this pull request: #50201

@MaximePinot
Copy link
Contributor

Hi!

I'd like to take over this issue!

If we have to choose between the two solutions above, then I'd go for the second one. Throwing a custom exception (that inherits from InvalidArgumentException) would allow us to store useful data so we can create a ConstraintViolation in RequestPayloadValueResolver (e.g. propertyPath, invalidValue, etc.)

However, I agree with @SebLevDev regarding the concept of PartialDenomalizationException... It is inconsistent.

@dunglas @nicolas-grekas : 5 months later, do you still think that catching the InvalidArgumentException on the RequestPayloadValueResolver level is the right solution? If so, I have time to work on it. 😉

@alexgithub98
Copy link

Sorry, is there any final solution to this issue?

@alexandre-le-borgne
Copy link

alexandre-le-borgne commented Oct 7, 2024

The current workaround I use:

class MyInputDTO
{
    /**
     * DO NOT REMOVE THAT, it's a workaround to a bug of the SF denormalizer that is using the type of the getter
     * @var string
     */
    #[Assert\NotBlank]
    #[Assert\Choice(callback: [MyEnum::class, 'values'])]
    public string $type;
    
     public function getType(): MyEnum
    {
        return MyEnum::from($this->type);
    }
}    
    
    
enum MyEnum: string
{
    case A = 'A ';
    case B = 'B';
    case C = 'C';

    public static function values(): array
    {
        return array_column(self::cases(), 'value');
    }
}

@alexgithub98
Copy link

Thanks! Cool solution:)

@dionisvl
Copy link

dionisvl commented Nov 4, 2024

up

@alexandre-le-borgne
Copy link

alexandre-le-borgne commented Nov 5, 2024

From my previous comment

The current workaround I use:

class MyInputDTO
{
    /**
     * DO NOT REMOVE THAT, it's a workaround to a bug of the SF denormalizer that is using the type of the getter
     * @var string
     */
    #[Assert\NotBlank]
    #[Assert\Choice(callback: [MyEnum::class, 'values'])]
    public string $type;
    
     public function getType(): MyEnum
    {
        return MyEnum::from($this->type);
    }
}
[...]

I made some changes in my current usage:

class MyInputDTO
{
    #[Assert\NotBlank]
    #[Assert\Choice(callback: [MyEnum::class, 'values'])]
    public string $type;
    
    public function getTypeEnum(): MyEnum
    {
        return MyEnum::from($this->type);
    }
}

The suffix Enum on the getter allows me to remove the @var string on the property

@motoronik
Copy link

I also encountered this problem, can you tell me if it will be solved by the framework?

I really would not like to remake all the code that uses enum to the option proposed by @alexandre-le-borgne

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

No branches or pull requests

9 participants