Skip to content

[Serializer] Add ability to collect denormalization errors #38968

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

Closed
wants to merge 1 commit into from

Conversation

greedyivan
Copy link
Contributor

Q A
Branch? 5.x
Bug fix? no
New feature? yes
Deprecations? no
Tickets Fix #37419
License MIT
Doc PR

This PR is a similar with #38472 (for unknown reason that is a draft) but there is a slightly different approach.

This implementation is more to show how the exceptions that are thrown during denormalization can be collected.

It has two part.

In the first part it is collected NotNormalizableValueExceptions in AbstractObjectNormalizer.

The second part is a collect exceptions from AbstractNormalizer::instantiateObject

@@ -633,6 +638,56 @@ public function testDeserializeAndUnwrap()
$serializer->deserialize($jsonData, __NAMESPACE__.'\Model', 'json', [UnwrappingDenormalizer::UNWRAP_PATH => '[baz][inner]'])
);
}

public function testCollectDenormalizationErrors()
Copy link
Contributor

@camilledejoye camilledejoye Nov 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe use a data provider to able to add new cases like for variadic argument, UnwrappingDenormalizer, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The data provider is suitable for cases where the assertion code is always the same for different data. Perhaps it would be better to write a test for each case here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either way, I just think this one is getting a bit hard to read.
Maybe add a new test for the UnwrappingDenormlizer because I think you don't handle it already and the key should be added to error to match the input paylod.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you provide a test case for UnwrappingDenormlizer?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is already a few tests that uses it.
The idea is to wrap the data into container: {"container:" {"foo": "string", ...}}
And check that the property path for each errors starts with container.

@greedyivan greedyivan force-pushed the rfc_37419 branch 3 times, most recently from 98b5fd3 to 8264a3b Compare November 22, 2020 23:09
Copy link
Member

@dunglas dunglas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great!

try {
$this->setAttributeValue($object, $attribute, $value, $format, $context);
} catch (InvalidArgumentException $e) {
throw new NotNormalizableValueException(sprintf('Failed to denormalize attribute "%s" value for class "%s": '.$e->getMessage(), $attribute, $type), $e->getCode(), $e);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we try to avoid the extra try / catch. I'm nitpicking but catching and throwing is very costly in PHP so let's avoid it when not strictly necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The inner try / catch was here for some reason. It was wrapped around to handle all calls that throw exceptions.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think he suggested to remove the inner try/catch, add another catch clause to the outer one when you deal with this exception by throwing or aggregating it depending on the context

@greedyivan
Copy link
Contributor Author

Added exception handling from ArrayDenormalizer.

I have some thoughts about adding AggregableExceptionInterface to all Serializer exceptions. But this can be done in an iterative style as new cases are identified.

@camilledejoye
Copy link
Contributor

I have some thoughts about adding AggregableExceptionInterface to all Serializer exceptions. But this can be done in an iterative style as new cases are identified.

I don't think it should be added to all of them, RuntimeException for instance is not something we would like to collect.
From my point of view we only want to collect errors due to a data that can not be denormalized, if we are missing a denormalizer for instance we still want it to fail without going any further.

@@ -16,6 +16,6 @@
*
* @author Johannes M. Schmitt <schmittjoh@gmail.com>
*/
class InvalidArgumentException extends \InvalidArgumentException implements ExceptionInterface
class InvalidArgumentException extends \InvalidArgumentException implements ExceptionInterface, AggregableExceptionInterface
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After a quick look I think there is two spots that should not be handled:

throw new InvalidArgumentException(sprintf('Cannot denormalize to a "%s" without the HttpFoundation component installed. Try running "composer require symfony/http-foundation".', File::class));

throw new InvalidArgumentException('Unsupported class: '.$type);

Those cases are because of an invalid type or missing class, I think they should stop the denormalization process as usual.

try {
$this->setAttributeValue($object, $attribute, $value, $format, $context);
} catch (InvalidArgumentException $e) {
throw new NotNormalizableValueException(sprintf('Failed to denormalize attribute "%s" value for class "%s": '.$e->getMessage(), $attribute, $type), $e->getCode(), $e);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think he suggested to remove the inner try/catch, add another catch clause to the outer one when you deal with this exception by throwing or aggregating it depending on the context

@@ -633,6 +638,56 @@ public function testDeserializeAndUnwrap()
$serializer->deserialize($jsonData, __NAMESPACE__.'\Model', 'json', [UnwrappingDenormalizer::UNWRAP_PATH => '[baz][inner]'])
);
}

public function testCollectDenormalizationErrors()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either way, I just think this one is getting a bit hard to read.
Maybe add a new test for the UnwrappingDenormlizer because I think you don't handle it already and the key should be added to error to match the input paylod.

@fabpot
Copy link
Member

fabpot commented Sep 10, 2021

see see #42502

hultberg pushed a commit to hultberg/symfony that referenced this pull request Sep 17, 2021
…ror during denormalization (lyrixx)

This PR was merged into the 5.4 branch.

Discussion
----------

[Serializer] Add support for collecting type error during denormalization

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Tickets       | Fix symfony#27824, Fix symfony#42236, Fix symfony#38472, Fix symfony#37419 Fix symfony#38968
| License       | MIT
| Doc PR        |

---

There is something that I don't like about the (de)Serializer. It's about the way it deals with typed properties.
As soon as you add a type to a property, the API can return 500.

Let's consider the following code:
```php
class MyDto
{
    public string $string;
    public int $int;
    public float $float;
    public bool $bool;
    public \DateTime $dateTime;
    public \DateTimeImmutable $dateTimeImmutable;
    public \DateTimeZone $dateTimeZone;
    public \SplFileInfo $splFileInfo;
    public Uuid $uuid;
    public array $array;
    /** `@var` MyDto[] */
    public array $collection;
}
```

and the following JSON:

```json
{
	"string": null,
	"int": null,
	"float": null,
	"bool": null,
	"dateTime": null,
	"dateTimeImmutable": null,
	"dateTimeZone": null,
	"splFileInfo": null,
	"uuid": null,
	"array": null,
	"collection": [
		{
			"string": "string"
		},
		{
			"string": null
		}
	]
}
```

**By default**, I got a 500:
![image](https://user-images.githubusercontent.com/408368/129211588-0ce9064e-171d-42f2-89ac-b126fc3f9eab.png)

It's the same with the prod environment. This is far from perfect when you try to make a public API :/
ATM, the only solution, is to remove all typehints and add assertions (validator component). With that, the public API is nice, but the internal PHP is not so good (PHP 7.4+ FTW!)

In APIP, they have support for transforming to [something](https://github.com/api-platform/core/blob/53837eee3ebdea861ffc1c9c7f052eecca114757/src/Core/Serializer/AbstractItemNormalizer.php#L233-L237) they can handle gracefully. But the deserialization stop on the first error (so the end user must fix the error, try again, fix the second error, try again etc.). And the raw exception message is leaked to the end user. So the API can return something like  `The type of the "string" attribute for class "App\Dto\MyDto" must be one of "string" ("null" given).`. Really not cool :/

So ATM, building a nice public API is not cool.

That's why I propose this PR that address all issues reported
* be able to collect all error
* with their property path associated
* don't leak anymore internal

In order to not break the BC, I had to use some fancy code to make it work 🐒

With the following code, I'm able to collect all errors, transform them in `ConstraintViolationList` and render them properly, as expected.

![image](https://user-images.githubusercontent.com/408368/129215560-b0254a4e-fec7-4422-bee0-95cf9f9eda6c.png)

```php
    #[Route('/api', methods:['POST'])]
    public function apiPost(SerializerInterface $serializer, Request $request): Response
    {
        $context = ['not_normalizable_value_exceptions' => []];
        $exceptions = &$context['not_normalizable_value_exceptions'];

        $dto = $serializer->deserialize($request->getContent(), MyDto::class, 'json', $context);

        if ($exceptions) {
            $violations = new ConstraintViolationList();
            /** `@var` NotNormalizableValueException */
            foreach ($exceptions as $exception) {
                $message = sprintf('The type must be one of "%s" ("%s" given).', implode(', ', $exception->getExpectedTypes()), $exception->getCurrentType());
                $parameters = [];
                if ($exception->canUseMessageForUser()) {
                    $parameters['hint'] = $exception->getMessage();
                }
                $violations->add(new ConstraintViolation($message, '', $parameters, null, $exception->getPath(), null));
            };

            return $this->json($violations, 400);
        }

        return $this->json($dto);
    }
```

If this PR got accepted, the above code could be transferred to APIP to handle correctly the deserialization

Commits
-------

ebe6551 [Serializer] Add support for collecting type error during denormalization
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[RFC][Serializer] Ability to collect denormalization failures
7 participants