api-platform / core

The server component of API Platform: hypermedia and GraphQL APIs in minutes
https://api-platform.com
MIT License
2.42k stars 864 forks source link

Having multiple QueryParameters with different providers does not work as expected #6673

Open Ilyes512 opened 1 day ago

Ilyes512 commented 1 day ago

API Platform version(s) affected: 3+

Description

If we have 2 different ParameterProviders set to 2 different query parameters, only the last provider has effect on the context/operation. So if I add a new value to the context in the first provider, that change is then not visible in the next provider.

How to reproduce

I have a DTO with a GetCollection attribute. In that attribute I have added an array of two QueryParameters to the parameters-parameter of the GetCollection attribute.

So something like this:

#[GetCollection(
    uriTemplate: 'foobar',
    parameters: [
        'a' => new QueryParameter(
            provider: ParameterOneProvider::class,
        ),
        'b' => new QueryParameter(
            provider: ParameterTwoProvider::class,
        ),
    ]
)]
final readonly class FoobarDto
{
}

Both ParameterOneProvider and ParameterTwoProvider implement the ParameterProviderInterface. In both parameters I do something like this:

class ParameterOneProvider implements ParameterProviderInterface
{
    /**
     * @param array<string, mixed>                                                                         $parameters
     * @param array<string, mixed>|array{request?: Request, resource_class?: string, operation: Operation} $context
     */
    public function provide(Parameter $parameter, array $parameters = [], array $context = []): ?Operation
    {
        $aQuery = $this->getSearchQuery($parameter);

        if ($searchQuery === null) {
            return null;
        }

        $operation = $this->getOperation($context);
        $context = $operation->getNormalizationContext() ?? [];
        $context['a'] = $searchQuery; // <-- the key in the ParameterTwoProvider would be `b`

        return $operation->withNormalizationContext($context);
    }

    private function getOperation(array $context): Operation
    {
        $operation = $context['operation'];

        Assert::isInstanceOf($operation, Operation::class);

        return $operation;
    }

    private function getAQuery(Parameter $parameter): ?string
    {
        $searchQuery = $parameter->getValue();

        if ($searchQuery instanceof ParameterNotFound) {
            return null;
        }

        Assert::string($searchQuery);

        return trim($searchQuery);
    }
}

If we now take a look at the ApiPlatform\State\Provider\ParameterProvider we can see that the operation that is returned from the first loop is not given to the next provider. See https://github.com/api-platform/core/blob/cead16a02592e8a2446f72286a6e9d2c3503e2eb/src/State/Provider/ParameterProvider.php#L86-L90

The new operation ($op) that is returned from the $providerInstance->provide() call is assigned to the $operator variable. Using my providers the $provider instance will always be a new instance since we called withNormalizationContext that uses clone to return a new instance.

Within the loop nothing is really done with the $operator except for possibly overwriting it again with a second call to a provider. Only after leaving the loop are we then assigning it to the context https://github.com/api-platform/core/blob/cead16a02592e8a2446f72286a6e9d2c3503e2eb/src/State/Provider/ParameterProvider.php#L97

So that means we are only seeing any changes made to the operation from the last provider called.

So unless I have totally misunderstood the use cases for using ParameterProviderInterface, what's the point of creating multiple custom providers?

Possible Solution

Not really sure if my use case is what the custom ParameterProvider was meant for.

Additional Context

I work on a project where we use DTO's and don't have api-platform directly tight to Doctrine (so we don't make use of the Doctrine integration in our Symfony project).

So my use case for the custom parameter provider is for example to have a query parameter that can contain a string uuid. And what I do is turn that string uuid into a Uuid instance and make it available to the custom provider for the specific DTO. I thought I would be able to do this by setting a unique value on the context so it becomes available in the DTO's provider.

CC: @soyuka (since it seems like you have added this)

soyuka commented 1 day ago

Nice one, I definitely want to fix this.

soyuka commented 1 day ago

can you state your use_symfony_listeners configuration value ?

Ilyes512 commented 1 day ago

Nice one, I definitely want to fix this.

Ah, so you are confirming that multiple ParameterProvider should be something that works.

can you state your use_symfony_listeners configuration value ?

# relative file path: config/packages/api_platform.yaml
api_platform:
    # ...
    use_symfony_listeners: false
    # ...

Anything I can do to help? In any case I will keep an eye on this issue and I usually can react pretty fast. I think it would be nice to create a clear picture of how we want it to work?

I am currently experimenting locally for a possible fix. If I got something I might just share it here or apply it right away in a fork.

Ilyes512 commented 1 day ago

So a possible solution could be https://github.com/api-platform/core/compare/3.4...Ilyes512:api-platform-core:6673-paramater-providers-fix

soyuka commented 14 hours ago

For now:

class GroupsParameterProvider implements ParameterProviderInterface {
    public function provider(Parameter $parameter, array $uriVariables = [], array $context = []): HttpOperation 
    {
        $request = $context['request'];
        $request->attributes->set('organization', $this->organizationRepository->find($organizationId));
        return $operation;
    }
}

Mutating the operation needs to be written into the request attributes, I thought I did in in the ParameterProvider I'll check. Anyways, request attributes are there to store request informations and this works fine.

Ilyes512 commented 13 hours ago

So mutating the request is the way to go? Or do you mean it more like a temp solution?

My gut feeling says "don't mutate the request", but I know the api-platform already does. So my gut feeling is probably unfounded.

edit:

https://github.com/symfony/http-foundation/blob/6.4/Request.php#L82-L87

I was not familiar with this attribute. It seems to be meant for mutation by the application.

soyuka commented 10 hours ago

it's a possible solution, even if you change the operation I'll mutation the operation inside the request attributes :). I know that this works but changing the operation should also work. I'll cherry-pick your commit and add a test tomorrow!

Ilyes512 commented 8 hours ago

FYI: I have now implemented my providers by setting a value in the attribute bag and that works fine for my use cases đź‘Ť.