symfony: [HttpKernel] #[MapQueryString] with an empty query string always returns null

I think the confusion is due to MapRequestPayload.

As everyone has already said in this thread, with MapQueryString and the following:

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 GET /get will result in a 404 HttpException error.

But, in case of a POST request with MapRequestPayload and the following:

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 POST /post with an empty JSON object will correctly respond:

curl -X POST -H "Content-Type: application/json" -d '{}' http://localhost:8000/post

# => {"dto":{"sort":"DESC","limit":0}}

This different behavior between MapQueryString and MapRequestPayload is pretty confusing.

@nicolas-grekas Of course. We have class PaginationDTO with 2 properties page and limit. And we wanna use it as request object with validation constraints in Controller. Let’s look at a few situations:

1. Properties page and limit don’t have default values -> the argument declaration in the controller looks like #[MapQueryString] PaginationDTO $query and we can’t set default value for PaginationDTO because properties need values in constructor.
2. Property page has default value and limit doesn’t have default value -> the argument declaration in the controller looks like #[MapQueryString] PaginationDTO $query and we can’t set default value for PaginationDTO because property limit needs value in constructor.
3. Properties page and limit have default values -> the argument declaration in the controller looks like #[MapQueryString] PaginationDTO $query = new PaginationDTO()

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.

As theMapQueryString function description says: [...] map the query string of the request to typed object and validate it. So I expect that the validator will run, and that I will get the appropriate violations based on asserts in DTO class. But now asserts and custom validators don’t run when there are no GET parameters.

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),
+                };
             }

I still don’t understand sorry:

Properties page and limit don’t have default values -> the argument declaration in the controller looks like #[MapQueryString] PaginationDTO $query and we can’t set default value for PaginationDTO because properties need values in constructor.

use #[MapQueryString] PaginationDTO $query = new PaginationDTO(page: 1, limit: 10)

Property page has default value and limit doesn’t have default value -> the argument declaration in the controller looks like #[MapQueryString] PaginationDTO $query and we can’t set default value for PaginationDTO because property limit needs value in constructor.

use #[MapQueryString] PaginationDTO $query = new PaginationDTO(limit: 10)

PaginationDTO in many controllers, then in situation 3 we should set the default value everywhere

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.

@blaugueux Yes, I will try =)

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.

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.

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:

• The information to be decoded
• The name of the class this information will be decoded to
• The encoder used to convert that information into an array

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.

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.

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.

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.

Because in this way we always must control the situation - Does our request class have only optional properties or not? And when we add a new property without a default value, we won’t be able to define that class as the default value in the controller.

Why don’t you use the default value? #[MapQueryString] PaginationDTO $query = new PaginationDTO

I tested it with your code and it works. Maybe you changed the code in the wrong place? I changed this block https://github.com/symfony/http-kernel/blob/6.3/Controller/ArgumentResolver/RequestPayloadValueResolver.php#L138

#I have the same problem as @blaugueux here. Why is this happening? - Because if you have a request class with only optional parameters, then anyway you will get here null when call endpoint without parameters.

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);
}

There we go, took your implementation @nicolas-grekas and added some tests that should cover everything I think.

sure, but it implies null can be given somehow, which never happens

I’d be happy to 👍🏻

please go ahead 🙏

how ?Dto $dto = new Dto() should behave

DefaultValueResolver has the answer: it should take the default value.