Releases 2.4.3+ break support for using the default messenger serializer with the Messenger component

[asimonf]

Scenario

According to this documentation, one can use the messenger component to implement the CQRS pattern. Such classes don't have an IRI. Starting with Symfony 4.3, the default serializer was changed, but it can be reverted like so:

framework:
    messenger:
        serializer:
            default_serializer: messenger.transport.symfony_serializer
            symfony_serializer:
                format: json
                context: { }

        transports:
            async_priority_normal:
                dsn: # ...
                serializer: messenger.transport.symfony_serializer

When doing this, the AbstractItemNormalizer is used for message serialization like when using Symfony 4.2 on Api-Platform 4.2. The problem arises due to changes on commit 2bb4db1.

Expected Behavior

When using Symfony 4.2 with Api-Platform 2.4.2 using the above serializer, the expected behavior of implementing the CQRS pattern is that it should work as intended.

Actual Behavior

When using Symfony 4.3 with any release after 2.4.2, the above fails when using the messenger.transport.symfony_serializer service for serialization.

The reason is that the normalizer expects to be able to obtain an IRI from the object. This will not work because Commands can't have an IRI. The guilty segment is this one:

$iri = $context['iri'] ?? $this->iriConverter->getIriFromItem($object);

The above was previously guarded by the following if and inside it:

if (isset($context['resources'])) {
    $context['resources'][$iri] = $iri;
}

How to reproduce

Implement the example in https://api-platform.com/docs/core/messenger/ and make changes inside the file messenger.yaml to match the following:

framework:
    messenger:
        serializer:
            default_serializer: messenger.transport.symfony_serializer
            symfony_serializer:
                format: json
                context: { }

# ...

Doing the above will result in the following error:

No item route associated with the type "App\Entity\ResetPasswordRequest".

[soyuka]

Mhh, is the ResetPasswordRequest annotated as an ApiResource ? Maybe that it should not be one and therefore not go through our normalizer?

[asimonf]

Mhh, is the ResetPasswordRequest annotated as an ApiResource ?

It is annotated with it as per the CQRS example:

/**
 * @ApiResource(
 *     messenger=true,
 *     collectionOperations={
 *         "post"={"status"=202}
 *     },
 *     itemOperations={},
 *     output=false
 * )
 */

Maybe that it should not be one

That would be a pretty severe breaking change, would it not?

Right now, the pattern suggested in the docs became broken with Symfony 4.3 and Api-Platform 2.4.3+ whenever the command has resources as fields.

When a command is defined with objects in Api-Platform, the endpoint expects the IRIs for those resources. Once the endpoint data is denormalized, the objects are fetched from the database and assigned to the command object.

The new default serializer for messages in Symfony 4.3 uses PHP's serialize and unserialize. Once a Doctrine object is unserialized at the other end of the transport, that object is no longer managed by the object manager. This is a break in behavior compared to 2.4.2. When trying to go back to the previous serializer used by Symfony in 4.2, the above happens.

I guess one workaround could be to remerge those to the entity manager and refresh them to get the latest data from the DB, but that would mean quite a bit of boiler-plate that wasn't previously required. Moreover, going back to the regular serializer should also be possible, since the symfony docs recommend doing that for when the edge cases break with the new serializer.

Since this behavior change is not documented on the API docs, it's bound to frustrate several users, including me. I spent more than I care to admit debugging this thinking it was something I messed up when upgrading the dependencies.

[soyuka]

Interesting, tyvm about this input I'll definitely try to prioritize this next week (will have some time to work on these issues).

[soyuka]

Hi,

I reproduced this use case and I'm not sure to follow you in the sentence "Right now, the pattern suggested in the docs became broken with Symfony 4.3 and Api-Platform 2.4.3+ whenever the command has resources as fields." as that without any additional configuration everything works as expected.

Composer version shows:

api-platform/core                  v2.4.6  Build a fully-featured hypermedia or GraphQL API in minutes
symfony/messenger                  v4.3.4  Symfony Messenger Component

Messenger configuraiton:

framework:
    messenger:
        transports:
            async: '%env(MESSENGER_TRANSPORT_DSN)%'
        routing:
            'App\Entity\ResetPasswordRequest': async

I don't get why you would need to change the serializer especially that you should handle the doctrine-related things in your handler yourself.
Also, note that messenger is still experimental which means it can break and we can not guarantee it's stability within Api Platform because of this.

Since this behavior change is not documented on the API docs

It's still open source software and we try to follow updates as fast as they get done. Please help us to improve the documentation if you think it's missing something.

If I manage to add blank node identifiers (see #2954) this will work but I still would not recommend to change the messenger serializer in this use case as I don't get the interest, especially that it'd have a negative impact in performances.

[asimonf]

I reproduced this use case and I'm not sure to follow you in the sentence "Right now, the pattern suggested in the docs became broken with Symfony 4.3 and Api-Platform 2.4.3+ whenever the command has resources as fields." as that without any additional configuration everything works as expected.

If I add a resource field in 4.2 with 2.4.2, I don't need to do anything extra. If I want to retain that behavior and I follow the docs on the Symfony site for 4.3, the application breaks on the Api-platform code. Specifically on the AbstractNormalizer code above.

I don't get why you would need to change the serializer especially that you should handle the doctrine-related things in your handler yourself.

Because if I send resources on the command, they no longer work. I didn't have to manually merge and refetch them before, now I would have to add a lot of boilerplate for stuff I didn't have to do if the previously default serializer worked.

Also, note that messenger is still experimental which means it can break and we can not guarantee it's stability within Api Platform because of this.

I get that, and since I couldn't find anywhere else where to discuss this, I opened this issue. If this is intended, I'll gladly close it, but I at least have to give my opinion that there should, at least, be a reasoning for the break on behavior.

In any case, if I follow the documentation in symfony, the code breaks on api-platform's side, which is why I opened the issue here.

Experimental code doesn't mean breaking changes happen just because, or that's how I understand it, at least. As far as I can tell, this break was not intended.

It's still open source software and we try to follow updates as fast as they get done. Please help us to improve the documentation if you think it's missing something.

I get that, and I agree. I will gladly submit a patch to the docs for this once the dust settles. However, a discussion is warranted IMHO in order to determine if this change in behavior is intentional and here to stay or if it is a bug and should be fixed.

If I manage to add blank node identifiers (see #2954 ) this will work but I still would not recommend to change the messenger serializer in this use case as I don't get the interest, especially that it'd have a negative impact in performances.

Any performance impact will end up being mitigated by the fact that entities need to be rehydrated and refreshed from the DB regardless. By going this route, I save myself from the boilerplate required to do that and from having to test it since the Api-platform normalizer is much more tested than whatever solution I can come up with.

Thanks for taking the time to review this.

[teohhanhui]

So, if I understand correctly, this is expected behaviour. A get item operation is always required.

Some examples in the docs are wrong. But in fact in this case ResetPasswordRequest should not be an API resource, but an input DTO instead. See a better example in our test suite:

core/tests/Fixtures/TestBundle/Entity/User.php

Lines 39 to 51 in 5379326

	* "post_password_reset_request"={
	* "method"="POST",
	* "path"="/users/password_reset_request",
	* "messenger"="input",
	* "input"=PasswordResetRequest::class,
	* "output"=PasswordResetRequestResult::class,
	* "normalization_context"={
	* "groups"={"user_password_reset_request"},
	* },
	* "denormalization_context"={
	* "groups"={"user_password_reset_request"},
	* },
	* },

[asimonf]

@teohhanhui so if I change the default serializer to the one used in 4.2 it is expected to be broken?

[teohhanhui]

@asimonf It's actually not related to which serializer is being used in Messenger. It is only a symptom of the problem, which is the missing get item operation. Sometimes you may get away without declaring it, but that's something we have never supported, as we've explained on multiple occasions in various issues.

[asimonf]

But the problem wasn't there on Symfony 4.2 with Api-Platform 2.4.2. Moreover, the example given to implement CQRS makes reference to get-less resources. Is there a different way to implement CQRS using async queues then?

[teohhanhui]

But the problem wasn't there on Symfony 4.2 with Api-Platform 2.4.2

We fixed something. It's normal if an unsupported use case becomes broken, because if something is not supported, it might (and you could expect it to) break anytime! 😄

That said, we should definitely make it clearer in the docs that the get item operation is required. PR welcome!

[asimonf]

@teohhanhui a get item operation is required on a command? Now it becomes weirder then, what is the proper way to implement an async command?

[teohhanhui]

Sorry, I can't advise on CQRS as I'm not familiar enough with that. But a RESTful API is not made up of commands (verbs), but resources (nouns). The closest equivalent to what you're looking for is already in our test suite (code I've shared above).

[asimonf]

But that's the core of the issue here, to claim that an example in the documentation is not supported code is mind-blowing to me. I'm literally going off from the docs here. If that's unsupported, then how do I tell the difference?

[teohhanhui]

@asimonf I understand your frustration, but it's only normal that when we find better solutions, it'd mean the docs become outdated / does not follow the best practices. (And of course, we're humans so we all make mistakes too.)

So please help us improve the docs. It will benefit everyone in the community. 😄