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

Invalid (or unclear to me) handling of data returned from MessageHandler::__invoke() #2550

Closed
Perf opened this issue Feb 21, 2019 · 25 comments
Closed

Comments

@Perf
Copy link

Perf commented Feb 21, 2019

Hello,
it seems handling of data returned from __invoke() method of class that implements MessageHandlerInterface is not correct.

Sending request to /users/password-reset-confirmation.

Expected behavior:
Api platform should return a response object that was returned from __invoke().
In this particular case I'm trying to return JWTAuthenticationSuccessResponse which extends JsonResponse.

Current behavior:

Uncaught PHP Exception Symfony\Component\HttpKernel\Exception\ControllerDoesNotReturnResponseException: "The controller must return a "Symfony\Component\HttpFoundation\Response" object but it returned an object of type App\Dto\User\UserPasswordResetConfirmation." at /srv/vendor/api-platform/core/src/Action/PlaceholderAction.php line 32

DTO class UserPasswordResetConfirmation.php:

namespace App\Dto\User;

use App\Validator as AppAssert;
use Symfony\Component\Validator\Constraints as Assert;

final class UserPasswordResetConfirmation
{
    /**
     * @var string
     * @Assert\NotBlank()
     * @AppAssert\IsHash()
     */
    public $passwordResetToken;
    /**
     * @var string
     * @Assert\NotBlank()
     * @AppAssert\PlainPassword()
     */
    public $plainPassword;
}

API Resource config user_password_reset_confirmation.yaml:

App\Dto\User\UserPasswordResetConfirmation:
    attributes:
        messenger: true
        output: false
    itemOperations: {}
    collectionOperations:
        post:
            access_control: 'is_anonymous()'
            path: '/users/password-reset-confirmation'
            status: 200
            swagger_context:
                tags: ['User']
                summary: 'Confirms and sets new password'
                responses:
                    200:
                        description: 'Successfully set new password, returning a new JWT token'
                        schema:
                            type: 'object'
                            properties:
                                token:
                                    type: 'string'
                                    description: 'JWT authentication token'
                    400:
                        description: 'Invalid input'
                    404:
                        description: 'Resource not found'

Message handler class UserPasswordResetConfirmationHandler.php:

namespace App\Handler;

use App\Dto\User\UserPasswordResetConfirmation;
use App\Event\UserEvent;
use App\Service\User\UserService;
use App\Traits\EventDispatcherable;
use Lexik\Bundle\JWTAuthenticationBundle\Security\Http\Authentication\AuthenticationSuccessHandler;
use Symfony\Component\Messenger\Handler\MessageHandlerInterface;

class UserPasswordResetConfirmationHandler implements MessageHandlerInterface
{
    use EventDispatcherable;

    /**
     * @var UserService
     */
    private $userService;
    /**
     * @var AuthenticationSuccessHandler
     */
    private $authenticationHandler;

    public function __construct(UserService $userService, AuthenticationSuccessHandler $authenticationHandler)
    {
        $this->userService = $userService;
        $this->authenticationHandler = $authenticationHandler;
    }

    public function __invoke(UserPasswordResetConfirmation $dto)
    {
        $user = $this->userService->confirmPasswordReset($dto);
        $this->dispatch(UserEvent::PASSWORD_RESET_CONFIRMED, new UserEvent($user));

        // creating a new JWT token
        $response = $this->authenticationHandler->handleAuthenticationSuccess($user);

        return $response;
    }
}

Debugging showed that event is populated with response object vendor/api-platform/core/src/EventListener/WriteListener.php at lines 60-66:

$persistResult = $this->dataPersister->persist($controllerResult);
//
$event->setControllerResult($persistResult ?? $controllerResult);

but later on somewhere controller result is overwritten with DTO object UserPasswordResetConfirmation, than becomes a cause of an exception.

@soyuka
Copy link
Member

soyuka commented Feb 22, 2019

Hi!

When using messenger=true it dispatches the message to the bus directly via the Messenger\DataPersister. Because the handler might be asynchronous it'll not work as you expect here (the returned value from the __invoke method in your handler is not used).
Also you disabled the output, therefore you should not have any response and a 202 code (Accepted).

@Perf
Copy link
Author

Perf commented Feb 22, 2019

Hi @soyuka,
thank you for reply!
Default configuration for processing of messages is synchronous, and it is.
I also tried to use JsonResponse class in output - the result is the same.

I'm trying to follow what is documented in https://api-platform.com/docs/core/messenger/
Especially in:

Accessing to the Data Returned by the Handler
API Platform automatically uses the Symfony\Component\Messenger\Stamp\HandledStamp when set. It means that if you use a synchronous handler, the data returned by the __invoke method replaces the original data.

Could you please clarify - how I should configure/code to be able to return JsonResponse from the class that extends MessageHandlerInterface to the client?

Message bus is in sync mode.

@soyuka
Copy link
Member

soyuka commented Feb 22, 2019

Did you check out the symfony documentation to see how stamps work: https://symfony.com/doc/current/components/messenger.html ?

@Perf
Copy link
Author

Perf commented Feb 27, 2019

Hi @soyuka
tbh, I'm a beginner with Messenger, but I read all the docs about Messenger.
And I don't see what should I do with Stamps in this particular case.

Debugging shows that envelope with result object (that was returned from handler's __invoke()) are being stamped with HandledStamp.

I'm using sync handler, HandledStamp is set automatically, I tried to set output attribute to JsonResponse, but it still

Uncaught PHP Exception Symfony\Component\HttpKernel\Exception\ControllerDoesNotReturnResponseException: "The controller must return a "Symfony\Component\HttpFoundation\Response" object but it returned an object of type App\Dto\User\UserPasswordResetConfirmation." at /srv/vendor/api-platform/core/src/Action/PlaceholderAction.php line 32

@soyuka
Copy link
Member

soyuka commented Feb 28, 2019

You can't return a response directly from the Handler, best is to return an object that will replace the original one.
If you want to return this response, you'll need to use a listener on kernel.view PRE_RESPOND where you'll call setResponse manually.
Right now you're going through this path.

Another solution would be to return your updated resource, and if you need a different representation of your data to create an Output with a data transformer (transforms your resource to the output representation). This way, ApiPlatform does the response job.

@Perf
Copy link
Author

Perf commented Feb 28, 2019

Hi @soyuka ,
thank you for explanation!
Basically, what I'm trying to do is so-called an "edge case".
I'm sending request not to a resource that is an entity, but to a specific DTO, dedicated just to one case.
In this particular case it's a request to confirm password change.
User sends request to /users/password-reset-confirmation with a json

{
  "passwordResetToken": "xyz",
  "plainPassword": "abc"
}

In handler, I'm verifying if passwordResetToken is valid, and if so, then reset a password with provided one.
Of course I could respond with the user entity, of even just an empty 200 response, but to make nore efficient (to avoid following additional auth request) I'm want to authenticate user and respond with JWT token.

I had it implemented before with DTO and Event Listener, but it was more code and not so beautiful like with message handler.

Adding of additional middlware into the message bus just for this case - seems to be an overcomlication.

If I explicitly set that output class should be for example JsonResponse, and when returning from handler's __invoke() instance of JsonResponse - why it couldn't be processed properly?

@soyuka
Copy link
Member

soyuka commented Feb 28, 2019

Adding of additional middlware into the message bus just for this case - seems to be an overcomlication.

The alternative would be to have an event listener before the WriteListener or a data persister but then you'll have issues with the token.

I'm want to authenticate user and respond with JWT token.

Can't you return the token object instead of a Response?

@Perf
Copy link
Author

Perf commented Feb 28, 2019

The alternative would be to have an event listener before the WriteListener or a data persister but then you'll have issues with the token.

Well, I already have a working solution with Event listener, but I'm trying to rewrite it in much more clear way using Messenger:

  1. in DTO class configure that is should be handled using Messenger
  2. in message handler do all the stuff and return result (if necessary)
  3. result is shown to the user

Can't you return the token object instead of a Response?

AuthenticationSuccessHandler::handleAuthenticationSuccess($user); returns JWTAuthenticationSuccessResponse which extends JsonResponse, so I can get content of response and return it from __invoke, but what then?

        // creating a new JWT token
        $response = $this->authenticationHandler->handleAuthenticationSuccess($user);
        $content = $response->getContent();

        return $content;

@soyuka
Copy link
Member

soyuka commented Feb 28, 2019

I need to try this, would you happen to have a repository with this example?

@Perf
Copy link
Author

Perf commented Feb 28, 2019

Unfortunately, it's in private repo.
But I could extract this use case as a working (potentially) solution and push into a public repo.

@soyuka
Copy link
Member

soyuka commented Feb 28, 2019

Would be amazing if you could!

@Perf
Copy link
Author

Perf commented Feb 28, 2019

hey @soyuka
here is a public repo https://github.com/Perf/test-messenger
please don't forget to execute steps from Setup.

to test the functionality I'm talking about - check tests/http directory.

  1. execute user_password_reset_request.http with context 'dev-user'
  2. check email in http://localhost:8025, copy token from it
  3. paste that token in user_password_reset_confirmation.http and try to execute it

@Perf
Copy link
Author

Perf commented Mar 4, 2019

hey @soyuka
did you had a chance to investigate that issue?

@soyuka
Copy link
Member

soyuka commented Mar 6, 2019

I haven't forget just couldn't find the time yet. Note that I've discussed this with other's and it really looks like messenger shouldn't be used in this case the event listener is fine. Although, I still want to check if we can use a Response directly from the messenger as you tried, I'll keep you updated asap.

@Perf
Copy link
Author

Perf commented Mar 6, 2019

Thank you @soyuka !
I believe the way I'm trying to use Messenger could be a good and more easier (I would say - more handy) alternative to process specific requests to DTO resources... and probably not only to DTO ;)

@soyuka
Copy link
Member

soyuka commented Mar 11, 2019

Still didn't had time but in the meantime maybe that this could be of interest: #2495 (comment)

@Perf
Copy link
Author

Perf commented Mar 14, 2019

Hi @soyuka,
thanks a lot for this link - it definitely interesting and it gave me some more things to consider.
But I tried CQRS approach (from your gist) and it didn't helped.

The problem still the same - when I return Response (or any subclass of it) from handler's __invoke() it ends up with exception.

Uncaught PHP Exception Symfony\Component\HttpKernel\Exception\ControllerDoesNotReturnResponseException: "The controller must return a "Symfony\Component\HttpFoundation\Response" object but it returned an object of type App\Dto\User\UserPasswordResetConfirmation." at /srv/vendor/api-platform/core/src/Action/PlaceholderAction.php line 32

And it doesn't matter what I will put in output for this operation.

But see how beautiful it could be:
You have an entity with many custom POST operations.
For every operation you can define a specific Input class.
Message handler is invoked to process this specific Input class.
If Response is returned from message handler - then just send it back to the user.
Otherwise continue normal flow of processing request.

More or less the same thing implemented with Events - if event handler returns response - processing stops and it will be sent to user.

@soyuka
Copy link
Member

soyuka commented Mar 21, 2019

fixed with #2628 really sorry about the long delay :)

@Perf
Copy link
Author

Perf commented Mar 21, 2019

Hi @soyuka
thanks for an update!
Please correct me if I'm wrong.
#2628 brings the following changes:

  • messenger attribute now could hold a value 'input'
  • input attribute should hold a name of DTO or input class
  • respond attribute should be set to true for the operation that should return a Response

@soyuka
Copy link
Member

soyuka commented Mar 22, 2019

  1. messenger can hold a input value that says: "This input will not be transformed and will actually be handled by the messenger directly".
  2. input is indeed a string or an array, it can be an array when you need to specify an iri for example but usually just the class
  3. don't mind about the respond thing, internally it's there so that our event listener behaves like this only in an Api Platform request. If not, this behavior might not be desired

@Perf
Copy link
Author

Perf commented Mar 22, 2019

Ok, I will try this new feature and reply back here with the notes )
Thank you!

@karser
Copy link
Contributor

karser commented Mar 22, 2019

Got this working on v2.4:

 App\UseCases\ResetPasswordRequest:
    collectionOperations:
        post:
            path: '/security/reset-password'
            messenger: true
            output: 'App\UseCases\ResetPasswordResponse'
    itemOperations: {}

class ResetPasswordUseCase implements MessageHandlerInterface
{
    public function __invoke(ResetPasswordRequest $request)
    {
        return new ResetPasswordResponse();
    }
}

@soyuka
Copy link
Member

soyuka commented Mar 25, 2019

Closing this as it's tested, lmk if you still have issues.

@soyuka soyuka closed this as completed Mar 25, 2019
@Perf
Copy link
Author

Perf commented Mar 25, 2019

ok @soyuka
thanks for all support!
just one question - when it will be merged to master?
the reason I'm on master, because of one fix (don't remember number) related to serialization/deserialization of Ramsey::uuid identifier.
Afaik, you or Dunglas mentioned that it will be released only in version 3.0, before that it's in master. That's why I'm on master.

@soyuka
Copy link
Member

soyuka commented Mar 25, 2019

Answer: #2638 :p

Although I think that the Ramsey identifiers work on 2.4

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

3 participants