[Serializer] Add ability to collect denormalization errors

[julienfalque]

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
Deprecations?	no
Tickets	#37419
License	MIT
Doc PR	-

This PR implements #37419 and replaces #27136 (and #28358 maybe).

With these changes, the feature would require all involved third party denormalizers to check the new collect_invariant_violations context option and transform any relevant domain error into an InvariantViolationException exception that contains information needed to build e.g. a nice 400 Bad Request response.

Maybe I can go further with something similar to ConstraintViolationBuilder to ease creating InvariantViolationException instances with symfony/translation integration. But I'm not sure about the current direction so I'd like to get some feedback first :)

TODO:

• tests
• builder similar to ConstraintViolationBuilder?
• integration with symfony/translation?

[weaverryan]

I don't know the serializer internals well, but I would LOVE this for 5.2 - it would help API Platform return denormalization errors as validation errors instead of one-off 400 errors.

[weaverryan]

I don't know the internals well here. Do we need BOTH the $this->validateAndDenormalize() and $this->setAttributeValue() to be inside the try-catch for InvariantViolationException?

[julienfalque]

Actually if we can denormalize the value (which means it is valid) but can't write it, I don't think this is an invariant violation. I moved that parted out of the new try-catch.

[weaverryan]

So, the idea is that all denormalizers need to be aware of the COLLECT_INVARIANT_VIOLATIONS context key and behave differently based on this? Either throwing a NotNormalizableValueException or a InvariantViolationException? Is that correct?

Could we just catch the existing NotNormalizableValueException? Or is the problem that these error messages are currently a bit "internal" (e.g. Failed to denormalize attribute foo for class Foo\Bar) and so the new exception message allows denormalizers to advertise a safer, "validation" message? Or is there another purpose?

If the only purpose is to advertise a validation-safe message, perhaps we could add a new optional property+method (thinking out loud) to NotNormalizableValueException that contains this - e.g. getValidationMessage()? Or maybe I'm way off understanding the mechanics :)

[julienfalque]

So, the idea is that all denormalizers need to be aware of the COLLECT_INVARIANT_VIOLATIONS context key and behave differently based on this? Either throwing a NotNormalizableValueException or a InvariantViolationException? Is that correct?

Yes, I think collecting denormalization errors should be opt-in. Though throwing InvariantViolationException is the only implemenation I managed to get working for now.

My first attempt was to wrap all denormalizations using an interface (e.g. InvariantViolationCatcherInterface). One implementation would just throw immediatly (current behavior) and another would collect errors instead. But I faced too many issues and gave up with this approach. Maybe I should give it another try?

Could we just catch the existing NotNormalizableValueException?

Here we catch InvariantViolationException because we rely on nested denormalizers calls to denormalize nested values. Those denormalizers will only throw this kind of exception for each invariant violation if COLLECT_INVARIANT_VIOLATIONS is enabled, so if we catch one here, we can assume it's enabled without actually checking the context in this method.

I'd like to introduce an interface for exceptions that would simplify this maybe, but I'll wait until we're sure the current approach is correct.

Or is the problem that these error messages are currently a bit "internal" (e.g. Failed to denormalize attribute foo for class Foo\Bar) and so the new exception message allows denormalizers to advertise a safer, "validation" message? Or is there another purpose?

If the only purpose is to advertise a validation-safe message, perhaps we could add a new optional property+method (thinking out loud) to NotNormalizableValueException that contains this - e.g. getValidationMessage()? Or maybe I'm way off understanding the mechanics :)

Indeed, the purpose is to advertise validation-safe messages (and maybe more validation related data later). Not sure NotNormalizableValueException is suitable for that though: currently, denormalizers might throw other exceptions (e.g. UnexpectedValueException). We could either:

• always throw NotNormalizableValueException, but replacing a thrown exception with another would be a BC break I guess;
• add the methods you suggest to all possible exceptions.

WDYT?

[dkarlovi]

Discussed for a bit in #25729

[fabpot]

We've just moved away from master as the main branch to use 5.x instead. Unfortunately, I cannot reopen the PR and change the target branch to 5.x. Can you open a new PR referencing this one to not loose the discussion? Thank you for your understanding and for your help.

[julienfalque]

Sure, see #38472 😊