Skip to content
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.

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

MutationResolverInterface no longer working properly in v3.3 #6354

Closed
NicoHaase opened this issue May 7, 2024 · 10 comments
Closed

MutationResolverInterface no longer working properly in v3.3 #6354

NicoHaase opened this issue May 7, 2024 · 10 comments
Assignees

Comments

@NicoHaase
Copy link
Contributor

NicoHaase commented May 7, 2024

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

@soyuka soyuka self-assigned this May 7, 2024
soyuka added a commit to soyuka/core that referenced this issue May 10, 2024
@soyuka
Copy link
Member

soyuka commented May 10, 2024

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.

@NicoHaase
Copy link
Contributor Author

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

@soyuka soyuka reopened this May 25, 2024
@soyuka
Copy link
Member

soyuka commented May 31, 2024

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?

@NicoHaase
Copy link
Contributor Author

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

@soyuka
Copy link
Member

soyuka commented Jun 4, 2024

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.

@NicoHaase
Copy link
Contributor Author

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

@codedge
Copy link

codedge commented Jun 5, 2024

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.

@durimjusaj
Copy link

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.

@soyuka
Copy link
Member

soyuka commented Jun 13, 2024

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

@NicoHaase
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants