core icon indicating copy to clipboard operation
core copied to clipboard

MutationResolverInterface no longer working properly in v3.3

Open NicoHaase opened this issue 1 year ago • 2 comments

API Platform version(s) affected: 3.3.x

Description
In my project, I use a MutationResolver to set the value of the currently logged in user to an entity ActivityLog during a GraphQL mutation. The mutation itself does not contain a value for the user, instead it's loaded from Symfony's Security component. A validator ensures that the User entity assigned to ActivityLog matches some conditions.

In v.3.2, this worked properly: the entity was populated properly and validated afterwards. After updating to ApiPlatform v3.3, the validation stage is already run before the MutationResolver can set the User entity. The validation fails, as the User entity on ActivityLog is still null

How to reproduce
Configuration for the entity:

#[ApiResource(
    graphQlOperations: [
        new Mutation(
            resolver: CreateActivityLogResolver::class,
            name: 'create'
        )
    ]
)]
class ActivityLog
{
// some other variables

    #[EqualsLoggedInUser]
    #[ORM\ManyToOne(targetEntity: User::class)]
    private User $user;
}

Configuration for the resolver:

<?php

declare(strict_types=1);

namespace App\Resolver;

use ApiPlatform\GraphQl\Resolver\MutationResolverInterface;
use App\Entity\ActivityLog;
use App\Entity\User;
use InvalidArgumentException;
use Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException;
use Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorageInterface;
use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;

class CreateActivityLogResolver implements MutationResolverInterface
{
    public function __construct(
        private readonly TokenStorageInterface $tokenStorage
    ) {
    }

    /**
     * @param object|null $item
     * @param mixed[]     $context
     */
    public function __invoke($item, array $context): ActivityLog
    {
        if (!$item instanceof ActivityLog) {
            throw new InvalidArgumentException('Missing input of type ActivityLog');
        }

        $token = $this->tokenStorage->getToken();

        if (!$token instanceof TokenInterface) {
            throw new AccessDeniedHttpException('Missing user');
        }

        $user = $token->getUser();

        if (!$user instanceof User) {
            throw new AccessDeniedHttpException('Missing user');
        }

        $item->setUser($user);

        return $item;
    }
}

Configuration for the validator:

class EqualsLoggedInUserValidator extends ConstraintValidator
{
    /**
     * @param User $value
     */
    public function validate($value, Constraint $constraint): void
    {
        if (!$constraint instanceof EqualsLoggedInUser) {
            throw new UnexpectedTypeException($constraint, EqualsLoggedInUser::class);
        }

        if (!$value instanceof User) {
// this is where the validator stage throws an exception
            throw new UnexpectedTypeException($value, User::class);
        }
   }
}

Possible Solution
I'm not sure where this behaviour changed, but I would expect that the MutationResolver has finished its work before the validator is run

Additional Context

NicoHaase avatar May 07 '24 08:05 NicoHaase

thanks for the detailed report, there's nothing that said that resolvers should happen before validation but I think it's a valid behavior, I added a new test for that.

soyuka avatar May 10 '24 11:05 soyuka

The fix for this has been reverted, thus the bug has appeared again

NicoHaase avatar May 24 '24 17:05 NicoHaase

Back to this, we need to find a solution, I think it's confusing that we validate the data that gets resolved as validation should occur on the user input. What I think would work is to disable validation (validate: false) and do the validation inside the resolver, would that work?

soyuka avatar May 31 '24 09:05 soyuka

What I've found in the meantime (maybe this is obvious to me, but it wasn't for me): setting event_listeners_backward_compatibility_layer: true helps to overcome this problem. But this is not a longtime solution, as this compatibility layer is removed within the next versions....


Still, validating the entity before it is completely populated looks strange to me. By simply thinking about the semantics of that, I can't see how this is right? I know that @codedge reported this as a new problem in #6370, but I would like to know more about their use case

NicoHaase avatar Jun 04 '24 09:06 NicoHaase

Isn't the resolver supposedly have the same behavior as a provider? Anyways, I'm considering adding an option to validate after the resolver is being called, this would work for both cases.

soyuka avatar Jun 04 '24 11:06 soyuka

Isn't the resolver supposedly have the same behavior as a provider?

I'm sorry, but I don't get the question. This feature has worked for some versions, and I thought I was on the save side when implementing it just as described at https://api-platform.com/docs/core/graphql/#custom-mutations, so I'm a bit confused about why this has changed when updating from 3.2 to 3.3

NicoHaase avatar Jun 04 '24 12:06 NicoHaase

What I've found in the meantime (maybe this is obvious to me, but it wasn't for me): setting event_listeners_backward_compatibility_layer: true helps to overcome this problem. But this is not a longtime solution, as this compatibility layer is removed within the next versions....

Still, validating the entity before it is completely populated looks strange to me. By simply thinking about the semantics of that, I can't see how this is right? I know that @codedge reported this as a new problem in #6370, but I would like to know more about their use case

We do not validate the entity before, but the input data.

Example: We expect the input of an amount passed to a mutation. The amount should be between two number, in a certain range. Why should we hit the custom logic in our custom resolver, if already the input is wrong? With using a custom DTO for the input, that has validation rules attached to the amount field, we make sure to get a correct input for further processing.

Even with 3.3.5 and these settings (no entry for event_listeners_backward_compatibility_layer)

api_platform:
  keep_legacy_inflector: false
  use_symfony_listeners: true

the validation does not run before reaching the resolver. We currently pin api-platform/core to 3.3.2, which works for us.

codedge avatar Jun 05 '24 09:06 codedge

Same issue here, MutationResolverInterface called after validation. We used MutationResolvers for the same reason @NicoHaase uses, to populate fields before validation, like user or default status which is relation, we have cases that we need to check data and populate a fields before validation. In RestApi I did all this with PRE_VALIDATE event subscribers, in graphql there is no events, before I used WriteStage to implement PRE_VALIDATE but since api_platform.graphql.resolver.stage.write service has been moved to legacy, also that is not possible anymore.

Implementing PRE_VALIDATE and POST_VALIDATE, would be a better solution for this and keep resolvers after validation.

durimjusaj avatar Jun 10 '24 16:06 durimjusaj

Can you check my PR? It adds an validateAfterResolver: true

soyuka avatar Jun 13 '24 14:06 soyuka

Yes, this part is working properly. Thanks for your help! I'll post an additional report, as I'm still facing a problem with security

NicoHaase avatar Jun 14 '24 09:06 NicoHaase