[HttpKernel] #[MapQueryString] with an empty query string always returns null #50690
Comments
|
Workaround for the time being: class MyDto {
public function __construct(public string $foo = "bar")
{}
}
class FooController extends AbstractController
{
#[Route("/foo", name: "foo", methods: ["GET"])]
public function getPaginated(
#[MapQueryString] ?MyDto $myDto,
): JsonResponse
{
return $this->json($myDto ?? new MyDto());
}
} |
|
I don't think removing the early return is the right way to do it imho. What if the argument is nullable and you want it to be null when no query parameters are present? So instead of removing it, it should add a condition that checks if the parameter is nullable. If it is, it should return null. If it isn't it should just let the serialiser handle it. |
|
@mdeboer I would let the serializer handle it first. If it can not deserialize, return |
|
Oh no. You would never be able to handle other errors like you can now. I don't see what good that would do. Can you elaborate? |
|
@mdeboer If the serializer can turn an empty query into the requested object, it should get the chance to do that. With an early return you assume that an empty query should always result in a null, even though the serializer could produce valid output. |
|
I don't think you understand what I meant, what I meant was this: Current code: private function mapQueryString(Request $request, string $type, MapQueryString $attribute): ?object
{
if (!$data = $request->query->all()) {
return null;
}
return $this->serializer->denormalize($data, $type, null, self::CONTEXT_DENORMALIZE + $attribute->serializationContext);
}My proposal: private function mapQueryString(Request $request, string $type, MapQueryString $attribute): ?object
{
if (!$data = $request->query->all() && $attribute->metadata->isNullable()) { // THIS LINE
return null;
}
return $this->serializer->denormalize($data, $type, null, self::CONTEXT_DENORMALIZE + $attribute->serializationContext);
}This makes sure it only returns null when the argument is indeed nullable. In your case this means you should not set it as nullable and it will simply use the serializer to convert it into an empty object, as with your object that is possible. Say that regardless of whether an object can be created without arguments, you want to know whether any query parameters are given. You simply make your argument nullable and it will be null when no query parameters are given. Now in case you don't have an object that can be created without arguments, it will throw an exception by the serializer, allowing you to debug it or do whatever with it. |
|
I understood what you mean, I am just of the opinion that the serializer should have the say what happens here. Otherwise you assume that nullable + empty query means you always want to get null. But that might now always be the case.
Then you should model your DTO to not be creatable from an empty array, or configure the serializer accordingly to not accept this input. By returning null however the choice on how to handle this is taken away. |
|
I wouldn't let the serializer define the behavior to have when empty query-string or payload happens. This would be unexpected and too implicit. Instead, I'd suggest to fallback to the default value of the argument (which can be an empty array, object or whatever). Here is what I have in mind. Anyone to turn that into a PR with a test case? --- a/src/Symfony/Component/HttpKernel/Controller/ArgumentResolver/RequestPayloadValueResolver.php
+++ b/src/Symfony/Component/HttpKernel/Controller/ArgumentResolver/RequestPayloadValueResolver.php
@@ -134,8 +134,12 @@ class RequestPayloadValueResolver implements ValueResolverInterface, EventSubscr
}
}
- if (null === $payload && !$argument->metadata->isNullable()) {
- throw new HttpException($validationFailedCode);
+ if (null === $payload) {
+ $payload = match (true) {
+ $argument->metadata->hasDefaultValue() => $argument->metadata->getDefaultValue(),
+ $argument->metadata->isNullable() => null,
+ default => throw new HttpException($validationFailedCode),
+ };
} |
Why is that unexpected and too implicit? It would align with how the serializer works everywhere else. As per the serializer component documentation, all you need to deserialize is:
So the criteria would be fully met here when you have an empty array + a class with all default values, especially when adding skipping of undefined properties to the default deserialization context. And in my opinion, that is more explicit than having to remember the special case of having to add a default value to the controller arguments for when the query is empty. |
but no information was given 🤔 for non-nullable maybe opt-in to deserializing defaults via the attribute
IMHO it's 100% the case |
Not quite, an empty array is the information that no query parameters were given. No information would be |
|
how woud an empty array in the query look like? |
|
The fact that there's a serializer behind the scene is an implementation detail. What matters is that this is a regular controller. Default values on controller arguments are used for that. |
|
agree, i forgot |
I'd be happy to 👍🏻
It isn't though (if that is a method declaration you are referring to, e.g. a controller method). Default values can only be constants. |
future is now https://3v4l.org/TNWXC but now im wondering how |
please go ahead 🙏
DefaultValueResolver has the answer: it should take the default value. |
|
sure, but it implies null can be given somehow, which never happens |
|
I'll make a PR after work 👍🏻 |
|
There we go, took your implementation @nicolas-grekas and added some tests that should cover everything I think. |
…stPayloadValueResolver (mdeboer) This PR was squashed before being merged into the 6.3 branch. Discussion ---------- [HttpKernel] Nullable and default value arguments in RequestPayloadValueResolver | Q | A | ------------- | --- | Branch? | 6.3 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | Fix #50690 | License | MIT This PR adds support to RequestPayloadValueResolver for nullable arguments and arguments with a default value set. See #50690 (comment) Credits go to `@nicolas`-grekas for the implentation 👍🏻 Commits ------- 8d31ed5 [HttpKernel] Nullable and default value arguments in RequestPayloadValueResolver
|
Hi and thanks for all your work here @mdeboer. |
Can you share a really simple controller where this happens (or link to repo with the problem isolated as much as possible)? I'd be happy to take a look! |
|
#I have the same problem as @blaugueux here. if (null === $payload) {
$payload = match (true) {
$argument->metadata->hasDefaultValue() => $argument->metadata->getDefaultValue(),
$argument->metadata->isNullable() => null,
default => throw new HttpException($validationFailedCode),
};
}A code example class ListRequest
{
public function #__construct(
#[Assert\NotBlank]
#[Assert\GreaterThanOrEqual(1)]
public readonly int $page = 1,
#[Assert\NotBlank]
#[Assert\GreaterThanOrEqual(1)]
public readonly int $pageSize = 15,
) {}
}
class CustomerController extends AbstractController
{
#[Route('/list', name: 'list')]
public function index(
#[MapQueryString] ListRequest $listRequest,
): Response {
...
}
}I think the solution would be to try to initialise the request object if the payload is null. And if we can do it, then our object has a default value. For example like this try {
$payload = match (true) {
$argument->metadata->hasDefaultValue() => $argument->metadata->getDefaultValue(),
!$argument->metadata->hasDefaultValue() => new ($argument->metadata->getType())(),
$argument->metadata->isNullable() => null,
default => throw new HttpException($validationFailedCode),
};
} catch (\Throwable) {
throw new HttpException($validationFailedCode);
} |
|
@dddenisov I was feeling alone with this! Thanks for joining me. |
|
@blaugueux This is weird... My code is updating the values with this fix. Could you show your request class? |
|
Something basic IMO. class OrganizationsController extends AbstractController
{
#[Route('/organizations', methods:['GET'])]
public function listOrganizations(
OrganizationRepository $repository,
#[MapQueryString] PaginationDTO $query
) {
...
}
}
class PaginationDto
{
public function __construct(
#[Assert\Choice([5, 10, 15, 20, 25])]
public readonly int $limit = 20,
#[Assert\Positive]
public readonly int $page = 1,
) {}
} |
|
I tested it with your code and it works. Maybe you changed the code in the wrong place? |
|
@dddenisov it's working fine with line 137 not deleted, my mistake :( Do you plan to propose a PR with this fix? |
|
@blaugueux Yes, I will try =) |
|
Why don't you use the default value? |
|
Because in this way we always must control the situation - Does our request class have only optional properties or not? |
|
I'm sorry I don't understand the issue. Can you be more explicit please? What's the issue with relying on the default value? |
|
@nicolas-grekas Of course.
If we used PaginationDTO in many controllers, then in situation 3 we should set the default value everywhere. And if in the future we add a new property into PaginationDTO without default value, then we will have to remove the default value for PaginationDTO in all these controllers. |
|
I still don't understand sorry:
use
use
Yes. That's the only issue that I understand from your description: the repetition. I'm not sure it deserves changing the way controllers work. Auto-creating a DTO as you describe would also make it more complex to express that providing a query string is mandatory. |
|
I think the confusion is due to As everyone has already said in this thread, with class TestDTO
{
public function __construct(
public readonly string $sort = 'DESC',
public readonly int $limit = 10,
) {}
}
class TestController extends AbstractController
{
#[Route('/get', methods:['GET'])]
public function get(#[MapQueryString] TestDTO $dto): JsonResponse
{
return $this->json([ 'dto' => $dto ]);
}
}The request on But, in case of a POST request with class TestDTO
{
public function __construct(
public readonly string $sort = 'DESC',
public readonly int $limit = 10,
) {}
}
class TestController extends AbstractController
{
#[Route('/post', methods: ['POST'])]
public function post(#[MapRequestPayload] TestDTO $dto): JsonResponse
{
return $this->json([ 'dto' => $dto ]);
}
}The request on curl -X POST -H "Content-Type: application/json" -d '{}' http://localhost:8000/post
# => {"dto":{"sort":"DESC","limit":0}}This different behavior between |
I agree, behaviour should be the same... But... |
|
As the |
Symfony version(s) affected
6.3.0
Description
RequestPayloadValueResolveralways returns null when mapping query strings and query string is empty.This is problematic when using a DTO where all arguments have default values, e.g.
How to reproduce
Controller:
GET /foo->404 HttpExceptionGET /foo?foo=baz->200 {"foo": "baz"}Possible Solution
Remove this early return and let the serializer decide what to do with an empty query string.
Additional Context
No response
The text was updated successfully, but these errors were encountered: