Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 3fc8d0f

Browse filesBrowse files
fabpotfabpot
committed
bug #59134 [HttpKernel] Denormalize request data using the csv format when using "#[MapQueryString]" or "#[MapRequestPayload]" (except for content data) (ovidiuenache)
This PR was squashed before being merged into the 6.4 branch. Discussion ---------- [HttpKernel] Denormalize request data using the csv format when using "#[MapQueryString]" or "#[MapRequestPayload]" (except for content data) | Q | A | ------------- | --- | Branch? | 6.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Issues | #59104 #50904 | License | MIT This fixes the scenario mentioned above where using `#[MapQueryString]` or `#[MapRequestPayload]` (except request content) with at least one readonly property with a type mismatch error leads to getting an object with uninitialized properties. The only properties that are set are the ones that are assigned a value via a request parameter and are NOT readonly. Moreover, if the corresponding query parameter is not provided for non-readonly property A and there is at least one other readonly property B that has a type mismatch then A will still be returned as uninitialized (even if it has a default value). Shortly put, the instantiation fails and the values of the properties cannot be set later on. Examples ``` class FilterDto { public function __construct( public readonly ?int $id = null, public readonly ?string $name = null, public ?string $description = null, ) { } } GET https://127.0.0.1:8000/test?id=x&name=test id: ? ?int name: ? ?string description: ? ?string GET https://127.0.0.1:8000/test?id=x&name=test&description=desc id: ? ?int name: ? ?string description: "desc" ``` The complete list of steps to reproduce this is provided in #59104. The reason why this happens is because we are disabling the type enforcement of the denormalizer in the `Symfony\Component\HttpKernel\Controller\ArgumentResolver\RequestPayloadValueResolver` class and when we eventually end up in the `validateAndDenormalize` method of the `Symfony\Component\Serializer\Normalizer\AbstractObjectNormalizer` class we ignore the type mismatch because of: ``` if ($context[self::DISABLE_TYPE_ENFORCEMENT] ?? $this->defaultContext[self::DISABLE_TYPE_ENFORCEMENT] ?? false) { return $data; } ``` Thus, we get a type error when trying to create the object and we fall back to `$reflectionClass->newInstanceWithoutConstructor();` Then, based on the provided request data, we attempt to set the values of the properties but this process fails for the readonly properties so they stay uninitialized. As discussed with `@nicolas`-grekas during [the hackathon at SymfonyCon Vienna 2024](https://live.symfony.com/2024-vienna-con/) the solution here is to stop disabling the type enforcement of the denormalizer. However, this alone is not enough because then we won't be able to use anything but string since this is the type that comes in the request so we also need to set the denormalization format to either `csv` or `xml`. This comment from `the Symfony\Component\Serializer\Normalizer\AbstractObjectNormalizer` sheds some light on why: ``` // In XML and CSV all basic datatypes are represented as strings, it is e.g. not possible to determine, // if a value is meant to be a string, float, int or a boolean value from the serialized representation. // That's why we have to transform the values, if one of these non-string basic datatypes is expected. ``` We avoid `xml` due to some special formatting that occurs so the proposed solution uses `csv`. Basically, we start using type enforcement + csv format where non-string values are transformed. Commits ------- e0957a0 [HttpKernel] Denormalize request data using the csv format when using "#[MapQueryString]" or "#[MapRequestPayload]" (except for content data)
2 parents 17364eb + e0957a0 commit 3fc8d0f
Copy full SHA for 3fc8d0f

File tree

2 files changed

+67
-4
lines changed
Filter options

2 files changed

+67
-4
lines changed

‎src/Symfony/Component/HttpKernel/Controller/ArgumentResolver/RequestPayloadValueResolver.php

Copy file name to clipboardExpand all lines: src/Symfony/Component/HttpKernel/Controller/ArgumentResolver/RequestPayloadValueResolver.php
+2-4Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -40,11 +40,9 @@
4040
class RequestPayloadValueResolver implements ValueResolverInterface, EventSubscriberInterface
4141
{
4242
/**
43-
* @see \Symfony\Component\Serializer\Normalizer\AbstractObjectNormalizer::DISABLE_TYPE_ENFORCEMENT
4443
* @see DenormalizerInterface::COLLECT_DENORMALIZATION_ERRORS
4544
*/
4645
private const CONTEXT_DENORMALIZE = [
47-
'disable_type_enforcement' => true,
4846
'collect_denormalization_errors' => true,
4947
];
5048

@@ -165,7 +163,7 @@ private function mapQueryString(Request $request, string $type, MapQueryString $
165163
return null;
166164
}
167165

168-
return $this->serializer->denormalize($data, $type, null, $attribute->serializationContext + self::CONTEXT_DENORMALIZE);
166+
return $this->serializer->denormalize($data, $type, 'csv', $attribute->serializationContext + self::CONTEXT_DENORMALIZE);
169167
}
170168

171169
private function mapRequestPayload(Request $request, string $type, MapRequestPayload $attribute): ?object
@@ -179,7 +177,7 @@ private function mapRequestPayload(Request $request, string $type, MapRequestPay
179177
}
180178

181179
if ($data = $request->request->all()) {
182-
return $this->serializer->denormalize($data, $type, null, $attribute->serializationContext + self::CONTEXT_DENORMALIZE);
180+
return $this->serializer->denormalize($data, $type, 'csv', $attribute->serializationContext + self::CONTEXT_DENORMALIZE);
183181
}
184182

185183
if ('' === $data = $request->getContent()) {

‎src/Symfony/Component/HttpKernel/Tests/Controller/ArgumentResolver/RequestPayloadValueResolverTest.php

Copy file name to clipboardExpand all lines: src/Symfony/Component/HttpKernel/Tests/Controller/ArgumentResolver/RequestPayloadValueResolverTest.php
+65Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
use Symfony\Component\HttpKernel\Exception\HttpException;
2323
use Symfony\Component\HttpKernel\HttpKernelInterface;
2424
use Symfony\Component\PropertyAccess\Exception\InvalidTypeException;
25+
use Symfony\Component\PropertyInfo\Extractor\ReflectionExtractor;
2526
use Symfony\Component\Serializer\Encoder\JsonEncoder;
2627
use Symfony\Component\Serializer\Encoder\XmlEncoder;
2728
use Symfony\Component\Serializer\Exception\NotNormalizableValueException;
@@ -394,6 +395,38 @@ public function testQueryStringValidationPassed()
394395
$this->assertEquals([$payload], $event->getArguments());
395396
}
396397

398+
public function testQueryStringParameterTypeMismatch()
399+
{
400+
$query = ['price' => 'not a float'];
401+
402+
$normalizer = new ObjectNormalizer(null, null, null, new ReflectionExtractor());
403+
$serializer = new Serializer([$normalizer], ['json' => new JsonEncoder()]);
404+
405+
$validator = $this->createMock(ValidatorInterface::class);
406+
$validator->expects($this->never())->method('validate');
407+
408+
$resolver = new RequestPayloadValueResolver($serializer, $validator);
409+
410+
$argument = new ArgumentMetadata('invalid', RequestPayload::class, false, false, null, false, [
411+
MapQueryString::class => new MapQueryString(),
412+
]);
413+
414+
$request = Request::create('/', 'GET', $query);
415+
416+
$kernel = $this->createMock(HttpKernelInterface::class);
417+
$arguments = $resolver->resolve($request, $argument);
418+
$event = new ControllerArgumentsEvent($kernel, function () {}, $arguments, $request, HttpKernelInterface::MAIN_REQUEST);
419+
420+
try {
421+
$resolver->onKernelControllerArguments($event);
422+
$this->fail(sprintf('Expected "%s" to be thrown.', HttpException::class));
423+
} catch (HttpException $e) {
424+
$validationFailedException = $e->getPrevious();
425+
$this->assertInstanceOf(ValidationFailedException::class, $validationFailedException);
426+
$this->assertSame('This value should be of type float.', $validationFailedException->getViolations()[0]->getMessage());
427+
}
428+
}
429+
397430
public function testRequestInputValidationPassed()
398431
{
399432
$input = ['price' => '50'];
@@ -422,6 +455,38 @@ public function testRequestInputValidationPassed()
422455
$this->assertEquals([$payload], $event->getArguments());
423456
}
424457

458+
public function testRequestInputTypeMismatch()
459+
{
460+
$input = ['price' => 'not a float'];
461+
462+
$normalizer = new ObjectNormalizer(null, null, null, new ReflectionExtractor());
463+
$serializer = new Serializer([$normalizer], ['json' => new JsonEncoder()]);
464+
465+
$validator = $this->createMock(ValidatorInterface::class);
466+
$validator->expects($this->never())->method('validate');
467+
468+
$resolver = new RequestPayloadValueResolver($serializer, $validator);
469+
470+
$argument = new ArgumentMetadata('invalid', RequestPayload::class, false, false, null, false, [
471+
MapRequestPayload::class => new MapRequestPayload(),
472+
]);
473+
474+
$request = Request::create('/', 'POST', $input);
475+
476+
$kernel = $this->createMock(HttpKernelInterface::class);
477+
$arguments = $resolver->resolve($request, $argument);
478+
$event = new ControllerArgumentsEvent($kernel, function () {}, $arguments, $request, HttpKernelInterface::MAIN_REQUEST);
479+
480+
try {
481+
$resolver->onKernelControllerArguments($event);
482+
$this->fail(sprintf('Expected "%s" to be thrown.', HttpException::class));
483+
} catch (HttpException $e) {
484+
$validationFailedException = $e->getPrevious();
485+
$this->assertInstanceOf(ValidationFailedException::class, $validationFailedException);
486+
$this->assertSame('This value should be of type float.', $validationFailedException->getViolations()[0]->getMessage());
487+
}
488+
}
489+
425490
public function testItThrowsOnVariadicArgument()
426491
{
427492
$serializer = new Serializer();

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.