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

[HttpKernel] RequestPayloadValueResolver Add support for custom http status code #50767

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Aug 1, 2023
Merged

[HttpKernel] RequestPayloadValueResolver Add support for custom http status code #50767

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions 2 src/Symfony/Component/HttpKernel/Attribute/MapQueryString.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@

namespace Symfony\Component\HttpKernel\Attribute;

use Symfony\Component\HttpFoundation\Response;
use Symfony\Component\HttpKernel\Controller\ArgumentResolver\RequestPayloadValueResolver;
use Symfony\Component\HttpKernel\ControllerMetadata\ArgumentMetadata;
use Symfony\Component\Validator\Constraints\GroupSequence;
Expand All @@ -29,6 +30,7 @@ public function __construct(
public readonly array $serializationContext = [],
public readonly string|GroupSequence|array|null $validationGroups = null,
string $resolver = RequestPayloadValueResolver::class,
public readonly int $validationFailedStatusCode = Response::HTTP_NOT_FOUND,
) {
parent::__construct($resolver);
}
Expand Down
2 changes: 2 additions & 0 deletions 2 src/Symfony/Component/HttpKernel/Attribute/MapRequestPayload.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@

namespace Symfony\Component\HttpKernel\Attribute;

use Symfony\Component\HttpFoundation\Response;
use Symfony\Component\HttpKernel\Controller\ArgumentResolver\RequestPayloadValueResolver;
use Symfony\Component\HttpKernel\ControllerMetadata\ArgumentMetadata;
use Symfony\Component\Validator\Constraints\GroupSequence;
Expand All @@ -30,6 +31,7 @@ public function __construct(
public readonly array $serializationContext = [],
public readonly string|GroupSequence|array|null $validationGroups = null,
string $resolver = RequestPayloadValueResolver::class,
public readonly int $validationFailedStatusCode = Response::HTTP_UNPROCESSABLE_ENTITY,
) {
parent::__construct($resolver);
}
Expand Down
1 change: 1 addition & 0 deletions 1 src/Symfony/Component/HttpKernel/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ CHANGELOG
* `BundleInterface` no longer extends `ContainerAwareInterface`
* Add optional `$className` parameter to `ControllerEvent::getAttributes()`
* Add native return types to `TraceableEventDispatcher` and to `MergeExtensionConfigurationPass`
* Add argument `$validationFailedStatusCode` to `#[MapQueryString]` and `#[MapRequestPayload]`

6.3
---
Expand Down
4 changes: 2 additions & 2 deletions 4 src/Symfony/Component/HttpKernel/Controller/ArgumentResolver/RequestPayloadValueResolver.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,10 +88,10 @@ public function onKernelControllerArguments(ControllerArgumentsEvent $event): vo
foreach ($arguments as $i => $argument) {
if ($argument instanceof MapQueryString) {
$payloadMapper = 'mapQueryString';
$validationFailedCode = Response::HTTP_NOT_FOUND;
$validationFailedCode = $argument->validationFailedStatusCode;
} elseif ($argument instanceof MapRequestPayload) {
$payloadMapper = 'mapRequestPayload';
$validationFailedCode = Response::HTTP_UNPROCESSABLE_ENTITY;
$validationFailedCode = $argument->validationFailedStatusCode;
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 it would be great to be able to define another error by default (from the constructor of this class). Otherwise it means that for every attribute we would have to do validationFailedStatusCode: Response::HTTP_SOMETHING.

Copy link
Author

@zim32 zim32 Jul 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Agree. But maybe it will be better to do this by another MR?

Copy link
Contributor

Choose a reason for hiding this comment

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

Don't see why that can't be added to this PR? Seems easy enough to add two constructor arguments and rely on those for the default value. This means in the attribute those arguments should be nullable and null by default. This way we can override it with the argument on the attribute but don't rely on the attribute for the default value of those arguments.

If you want, I can help with that?

Copy link
Author

Choose a reason for hiding this comment

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

Well this is another feature amd it is always better to separate features into several MR otherwise MR can become quite messy

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure this is a good idea. Descriptive info (attributes here) should be self-descriptive. Making them rely on some external configuration blurs the line and readers can't expect a stable outcome anymore.

The solution to your concern is to create and use a child class that sets the code to what you want.

} else {
continue;
}
Expand Down
77 changes: 76 additions & 1 deletion 77 ...omponent/HttpKernel/Tests/Controller/ArgumentResolver/RequestPayloadValueResolverTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ public function testNullableValueArgument()

$resolver->onKernelControllerArguments($event);

$this->assertEquals([null], $event->getArguments());
$this->assertSame([null], $event->getArguments());
}

public function testQueryNullableValueArgument()
Expand Down Expand Up @@ -251,6 +251,7 @@ public function testValidationNotPassed()
$this->fail(sprintf('Expected "%s" to be thrown.', HttpException::class));
} catch (HttpException $e) {
$validationFailedException = $e->getPrevious();
$this->assertSame(404, $e->getStatusCode());
$this->assertInstanceOf(ValidationFailedException::class, $validationFailedException);
$this->assertSame('This value should be of type unknown.', $validationFailedException->getViolations()[0]->getMessage());
$this->assertSame('Test', $validationFailedException->getViolations()[1]->getMessage());
Expand Down Expand Up @@ -601,6 +602,73 @@ public static function provideValidationGroupsOnManyTypes(): iterable
new MapQueryString(validationGroups: new Assert\GroupSequence(['strict'])),
];
}

public function testQueryValidationErrorCustomStatusCode()
{
$serializer = new Serializer([new ObjectNormalizer()], []);

$validator = $this->createMock(ValidatorInterface::class);

$validator->expects($this->once())
->method('validate')
->willReturn(new ConstraintViolationList([new ConstraintViolation('Page is invalid', null, [], '', null, '')]));

$resolver = new RequestPayloadValueResolver($serializer, $validator);

$argument = new ArgumentMetadata('page', QueryPayload::class, false, false, null, false, [
MapQueryString::class => new MapQueryString(validationFailedStatusCode: 400),
]);
$request = Request::create('/?page=123');

$kernel = $this->createMock(HttpKernelInterface::class);
$arguments = $resolver->resolve($request, $argument);
$event = new ControllerArgumentsEvent($kernel, function () {}, $arguments, $request, HttpKernelInterface::MAIN_REQUEST);

try {
$resolver->onKernelControllerArguments($event);
$this->fail(sprintf('Expected "%s" to be thrown.', HttpException::class));
} catch (HttpException $e) {
$validationFailedException = $e->getPrevious();
$this->assertSame(400, $e->getStatusCode());
$this->assertInstanceOf(ValidationFailedException::class, $validationFailedException);
$this->assertSame('Page is invalid', $validationFailedException->getViolations()[0]->getMessage());
}
}

public function testRequestPayloadValidationErrorCustomStatusCode()
{
$content = '{"price": 50, "title": ["not a string"]}';
$payload = new RequestPayload(50);
$serializer = new Serializer([new ObjectNormalizer()], ['json' => new JsonEncoder()]);

$validator = $this->createMock(ValidatorInterface::class);
$validator->expects($this->once())
->method('validate')
->with($payload)
->willReturn(new ConstraintViolationList([new ConstraintViolation('Test', null, [], '', null, '')]));

$resolver = new RequestPayloadValueResolver($serializer, $validator);

$argument = new ArgumentMetadata('invalid', RequestPayload::class, false, false, null, false, [
MapRequestPayload::class => new MapRequestPayload(validationFailedStatusCode: 400),
]);
$request = Request::create('/', 'POST', server: ['CONTENT_TYPE' => 'application/json'], content: $content);

$kernel = $this->createMock(HttpKernelInterface::class);
$arguments = $resolver->resolve($request, $argument);
$event = new ControllerArgumentsEvent($kernel, function () {}, $arguments, $request, HttpKernelInterface::MAIN_REQUEST);

try {
$resolver->onKernelControllerArguments($event);
$this->fail(sprintf('Expected "%s" to be thrown.', HttpException::class));
} catch (HttpException $e) {
$validationFailedException = $e->getPrevious();
$this->assertSame(400, $e->getStatusCode());
$this->assertInstanceOf(ValidationFailedException::class, $validationFailedException);
$this->assertSame('This value should be of type unknown.', $validationFailedException->getViolations()[0]->getMessage());
$this->assertSame('Test', $validationFailedException->getViolations()[1]->getMessage());
}
}
}

class RequestPayload
Expand All @@ -612,3 +680,10 @@ public function __construct(public readonly float $price)
{
}
}

class QueryPayload
{
public function __construct(public readonly float $page)
{
}
}
Morty Proxy This is a proxified and sanitized view of the page, visit original site.