feat(state): constraint-aware 422 for denormalization errors#8211
feat(state): constraint-aware 422 for denormalization errors#8211soyuka wants to merge 1 commit into
Conversation
afe00da to
eca83f8
Compare
| { | ||
| public const IS_BLANK_ERROR = 'c1051bb4-d103-4f74-8988-acbcafc7fdc3'; | ||
| public const IS_NULL_ERROR = 'ad32d13f-c3d4-423b-909a-857b961eb720'; | ||
| public const INVALID_TYPE_ERROR = 'ba785a8c-82cb-4283-967c-3cf342181b40'; |
There was a problem hiding this comment.
why uuids? we could have something more explicit especially if hard-coded
| return null; | ||
| } | ||
|
|
||
| if (str_contains($path, '.') || str_contains($path, '[')) { |
There was a problem hiding this comment.
not liking this we have ways to do this with the property accessor no?
| $hasNullable = \in_array('nullable', $rules, true); | ||
|
|
||
| if ($isNull) { | ||
| if ($hasNullable && !array_intersect(self::REQUIRED_RULES, $rules) && !array_intersect(self::PRESENT_RULES, $rules)) { |
There was a problem hiding this comment.
better use keys to get the proper item of the array, array_intersect is slow
| return null; | ||
| } | ||
|
|
||
| if (array_intersect(self::REQUIRED_RULES, $rules)) { |
There was a problem hiding this comment.
repeats array_intersect... same below
| private readonly SerializerInterface $serializer, | ||
| private readonly SerializerContextBuilderInterface $serializerContextBuilder, | ||
| private ?TranslatorInterface $translator = null, | ||
| private readonly ?DenormalizationErrorHandlerInterface $errorHandler = null, |
There was a problem hiding this comment.
we should cover bc layer, probably we should add a default instance or what
|
|
||
| // v1: top-level paths only. Nested paths (e.g. "address.street") return null | ||
| // so the caller's fallback (collect mode) or rethrow (single mode) kicks in. | ||
| if (str_contains($path, '.') || str_contains($path, '[')) { |
There was a problem hiding this comment.
imo we can handle this using property accessor
| } | ||
| } | ||
|
|
||
| return array_values($constraints); |
There was a problem hiding this comment.
I like the optimization but I dislike the spl_object_hash can we use another key? maybe property.$contraint::class?
| } | ||
|
|
||
| return null; | ||
| } |
There was a problem hiding this comment.
can we merge this function with the one above to reduce complexity?
| null, | ||
| (string) Type::INVALID_TYPE_ERROR, | ||
| ); | ||
| } |
There was a problem hiding this comment.
quite some duplicated logic with function above
| $normalized = []; | ||
| foreach ($expectedTypes ?? [] as $expectedType) { | ||
| if (\is_string($expectedType) && (class_exists($expectedType) || interface_exists($expectedType))) { | ||
| $normalized[] = (new \ReflectionClass($expectedType))->getShortName(); |
There was a problem hiding this comment.
do we really need reflection here?
Translates PHP denormalization type errors (NotNormalizableValueException) into a 422 validation exception when the target property has a matching validation contract; rethrows for an honest 400 otherwise. Closes api-platform#7981. Architecture: - api-platform/state owns DenormalizationErrorHandlerInterface (no symfony/ validator dep). DeserializeProvider becomes a thin delegate: catches the denormalization exception, hands it to the handler, rethrows on miss. - api-platform/validator owns the Symfony impl (DenormalizationErrorHandler) which reads Symfony Validator metadata and throws ApiPlatform\Validator\ Exception\ValidationException. - api-platform/laravel owns ApiPlatform\Laravel\State\DenormalizationError Handler which reads Operation::getRules() and throws Laravel's native ApiPlatform\Laravel\ApiResource\ValidationError directly. The Laravel package does NOT depend on symfony/validator nor api-platform/validator. Symfony rule table: - null + NotBlank → IS_BLANK_ERROR + constraint message - null + NotNull → IS_NULL_ERROR + constraint message - wrong type + Type → INVALID_TYPE_ERROR + constraint message - wrong type + any constraint → generic Type @ 422 - no constraint → no match → rethrow → 400 Laravel rule table: - null + required/filled → IS_BLANK_ERROR - null + present → IS_NULL_ERROR - wrong type + string/integer/int/ → INVALID_TYPE_ERROR numeric/boolean/bool/array/date/json - wrong type + any other rule → INVALID_TYPE_ERROR - null + nullable (no required/present) → no match → rethrow - no rule → no match → rethrow → 400 Replaces the BackedEnum-only special case (commit 44bb18d) with a general constraint-aware rule. Enums with NotNull/Type/Choice still produce 422; enums with zero constraints regress to 400 — opt in via auto_mapping or add an explicit Assert\Type(EnumClass). In collect mode (collect_denormalization_errors=true), unconstrained errors still emit a generic Type/INVALID_TYPE_ERROR entry so the response surface stays consistent with prior behavior. FormRequest-class rules and pure-callable rule sets are intentionally skipped in the Laravel impl (v1): a FormRequest-based contract typically runs in the validation phase against the raw request. Tests: - src/Validator/Tests/DenormalizationErrorHandlerTest.php — 10 unit tests covering each row of the matching table + group filtering + collect mode - src/State/Tests/Provider/DeserializeProviderTest.php — handler-delegation assertions (drops the 3 tests that depended on Validator\Exception\ ValidationException; that coverage moved to the Validator handler test) - tests/Functional/DenormalizationValidationTest.php — 5 Symfony functional tests for null+NotBlank, null+NotNull, wrong+Type, no-constraint→400, collect mode mix - src/Laravel/Tests/DenormalizationValidationTest.php — 3 Laravel functional tests including typed-DTO+rule→422 with ValidationError shape verified Composer: - api-platform/state: drops api-platform/validator dev-dep (cycle fix) - api-platform/validator: requires api-platform/state - api-platform/laravel: no new dependencies Closes api-platform#7981 Refs api-platform#8183
eca83f8 to
cb98da4
Compare
Summary
Closes #7981. Translates PHP denormalization type errors (
NotNormalizableValueException) into 422ValidationExceptionresponses only when the target property has a matching validation constraint. Otherwise rethrows the original exception → honest 400.RFC 9110 §15.5.21: "syntax of the request content is correct, thus 400 is inappropriate" — JSON parses, type-mismatch is 422 territory. This brings API Platform in line with FastAPI/Laravel/DRF behavior for properties the user explicitly opted into validation on, without silently inventing constraints elsewhere.
What changed
New service
ApiPlatform\Validator\DenormalizationViolationFactoryresolves the target property's constraints via Symfony's validator metadata factory and emits a typedConstraintViolationper the rule table:nullNotBlankNotBlank::IS_BLANK_ERROR+ constraint messagenullNotNullNotNull::IS_NULL_ERROR+ constraint messageTypeType::INVALID_TYPE_ERROR+ constraint messagenull→ caller rethrows → 400DeserializeProvideraccepts the factory as a new nullable 5th constructor argument (BC). The single-error catch consults the factory and rethrows on null match; the collect-mode catch falls back to the existing generic Type violation when the factory returns null, preserving prior collect-mode semantics.The
isBackedEnumException()special case (introduced in 4.3.6 / 44bb18d as a BC-breaking workaround for #8183) is removed. The new rule subsumes it: enum properties withAssert\NotNull,Assert\Type(EnumClass), orAssert\Choicestill produce 422.BC impact
'abc'→float, no constraint'abc'→float, w/Assert\Type('numeric')null→ required, w/Assert\NotBlankAssert\NotNull/Assert\Type/Assert\ChoiceThe enum regression is the only one. Mitigation:
Assert\Type(EnumClass)orAssert\Choiceconstraint on the enum propertyframework.validation.auto_mapping.paths—PropertyInfoLoaderauto-addsType(EnumClass)constraintsJustified because the 4.3.6 commit itself was a silent BC break (400 → 422 unconditionally for enum properties); the constraint-aware rule is more principled and respects explicit user opt-in.
Laravel
Intentionally untouched in this PR. Laravel uses
Illuminate\Validationrules, not Symfony validator metadata, so the Symfony-validator-based factory cannot apply. ALaravelDenormalizationViolationFactoryreading Eloquent/request rules is tracked as a follow-up.Tests
src/Validator/Tests/DenormalizationViolationFactoryTest.php— 9 unit tests covering each rule row, group filtering, unknown class/property, and nested-path rejection (top-level only in v1)tests/Functional/DenormalizationValidationTest.php— 5 functional tests covering null+NotBlank, null+NotNull, wrong-type+Type, no-constraint→400, and collect-mode mixtests/Functional/EnumDenormalizationValidationTest.php(introduced by Validation groups are skipped when deserializing invalid PHP enums (BackedEnumNormalizer throws before validation) #8183) passes unchanged —NotNullon the enum prop triggers the "any wrong type + any other constraint" rule → 422Out of scope (follow-ups)
address.street,items[0].brand) — v1 returns null for nested paths, caller rethrowsIlluminate\Validationrulesdocs/symfony/validation.mdupdate +CHANGELOG.mdentryPropertyInfoLoader)Test plan
vendor/bin/phpunit tests/Functional/DenormalizationValidationTest.php— 5/5 greenvendor/bin/phpunit tests/Functional/EnumDenormalizationValidationTest.php— 2/2 greencd src/State && ./vendor/bin/phpunit Tests/Provider/DeserializeProviderTest.php— 13/13 greencd src/Validator && ./vendor/bin/phpunit Tests/DenormalizationViolationFactoryTest.php— 9/9 greenvendor/bin/phpunit tests/Functional/ --filter "Validation |Denormalization|BackedEnum"— 122/124 green (2 unrelatedMcpTestfailures fromvendor/mcp/sdksignature mismatch — not touched by this PR)Refs #8183