Core: 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 2bb4db1583521be72144d1da7da9cbe0b09808a2.

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 https://github.com/api-platform/core/pull/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 https://github.com/api-platform/core/pull/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:

https://github.com/api-platform/core/blob/53793261113b18931ce8a9e2f174f59fdaa0c1c5/tests/Fixtures/TestBundle/Entity/User.php#L39-L51

[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! :smile:

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. :smile:

[asimonf]

@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.

It's about breaking code without notice because a supported feature suddenly becomes unsupported. That's the frustrating part. Now I have to live in limbo.

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

I'd gladly improve the docs, but I have no idea what the supported way to implement CQRS is if the current status is that commands don't belong in a REST API.

The proper way forward is probably to delete the doc entry with the above example or mark it as an unsupported feature starting with 2.4.3. Because that's the current situation.

[teohhanhui]

It's about breaking code without notice because a supported feature suddenly becomes unsupported.

No, we have been consistent in saying that the get item operation is always required. Not defining it might work sometimes, but it's not guaranteed to work. But not all examples in the docs are correct, as you've seen here.

but I have no idea what the supported way to implement CQRS is if the current status is that commands don't belong in a REST API.

I (again) refer you to the same example from the docs, done a different way in our test suite:

https://github.com/api-platform/core/blob/53793261113b18931ce8a9e2f174f59fdaa0c1c5/tests/Fixtures/TestBundle/Entity/User.php#L39-L51

But the short answer is: Don't expose commands as API resources. It's not RESTful.

You can see in the above example we're not exposing a ResetPassword command, but a PasswordResetRequest resource. Of course, if you still want to do it, no one is stopping you.

[asimonf]

I'm sorry to be so insistent but you haven't been consistent if the official docs state in one example to implement CQRS that the get is not required. As it stands, this particular official documentation contradicts what you're stating right now.

If the example stated in the docs is a mistake, that's ok, but you should remove it then. I can make a PR to remove it if you want. But you have to realize that it's not consistent if your docs point to two different stories. It's bound to confuse someone else besides me.

I'm trying to not appear entitled because I understand this is voluntary work. I don't demand that someone fix this, but this discussion still has to happen if the confusion is to be stopped.

[soyuka]

I think that you're issue is related to an erroneous feature that was introduced by mistake: it was never expected that the messenger uses our own serializer. Also, I see no mention in the docs that a Command (input) embeds a Resource (which IIUC is your issue here).

Our serializer only works with Resources, and we're working on allowing empty get operations (https://github.com/api-platform/core/pull/3079) but this isn't a good resolution of your issue.

Sorry that we mislead you because of a feature we didn't intended to state like this and know that messenger is still an experimental feature subject to changes.

If the example stated in the docs is a mistake, that's ok, but you should remove it then. I can make a PR to remove it if you want.

You're free to help us improving the documentation to your liking.
Also, as the license says: "THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED..."

[asimonf]

it was never expected that the messenger uses our own serializer.

Changing the serializer to the default symfony serializer leads to an error I'm API platform code. If this is an unsupported feature by API platform it should, at least, be addressed in API platform with an error notice indicating it's not supported. Or rather, the serializer should skip it entirely. But that means it should also skip it during normalization and not just denormalization which is the crux of the issue here.

There are legitimate reasons to change the default serializer other than mine, like being able to interoperate between systems allowing them to use json rather than php serialization.

Also, as the license says: "THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED..."

I hope you don't misunderstand my comments as me claiming that you owe me warranty. No one owes me anything, and I understand that. I don't expect anything from this, but it is frustrating to see an issue get hand waved.

I'll gladly help fix the issue with code or docs if I see an opening. It's just still not clear to me that you guys consider this to be an issue at all.

I still don't get it. Is CQRS a supported feature or not?

[teohhanhui]

Is CQRS a supported feature or not?

CQRS is a design pattern, not a feature. As explained above, you might be able to implement it by creating an abstraction of "resources", instead of exposing "commands". You may be able to get some advice from the community on the #api-platform channel of Symfony Slack too.

Hope this helps! :smile:

[asimonf]

@teohhanhui

CQRS requires the implementation of, at least, one feature inside Api-Platform itself, like the messenger attribute in the resource annotation. The docs show an example of how to do this. Is that doc entry still showing a supported feature or not?

To be more specific, is the messenger attribute still a supported feature of the resource annotation? Is the example in the documentation a valid, supported, way to implement anything inside Api-platform? I gather it's not because it should have a get operation, but that is not why @soyuka closed the issue. So I'm trying to get some clarification.

[teohhanhui]

I refer you (again) to the way we've implemented the same in our test suite (which I've linked above twice). Have you checked it out yet? It should work perfectly fine, no?

[asimonf]

I may be wrong here, but I don't see how that example uses the messenger component to process the request asynchronously.

[asimonf]

I'm sorry, I'm not trying to be obtuse and I'm afraid my answer could be misunderstood that way.

The asynchronous part of the feature is not represented in the example you sent and that's an essential part to me. I'm not looking to actually implement a password reset, but I use that example to get my point across because that's what the document uses.

For scalability purposes, it is important for me that many such requests that have side-effects can be deferred. I originally thought of using DTOs but I saw no way of marking them for asynchronous dispatch and then I stumbled upon the above document entry.

The asynchronicity is what drove me to consider using the CQRS example in your docs in the first place and that is exactly why I'm asking if it's still a supported use case.

[teohhanhui]

The asynchronous part of the feature is not represented in the example you sent and that's an essential part to me.

Here it is:

  *             "messenger"="input", 

Please see https://api-platform.com/docs/core/messenger/#using-messenger-with-an-input-object :smile:

This means the "input' DTO (e.g. PasswordResetRequest) is the message being dispatched to your Messenger handler (e.g. https://github.com/api-platform/core/blob/v2.4.7/tests/Fixtures/TestBundle/MessageHandler/PasswordResetRequestHandler.php), which could then directly return the "output" DTO (e.g. PasswordResetRequestResult).

Hope it's clearer now.

[asimonf]

I'm more confused now :pensive:. Can I have more than one DTO per entity?

Also, in that example the itemOperations attribute is also empty. Is that not unsupported?

[teohhanhui]

Can I have more than one DTO per entity?

Yes, you can have as many custom operations as you need. Each of them can use their own "input" / "output" DTOs.

Also, in that example the itemOperations attribute is also empty. Is that not unsupported?

As I've mentioned before, some examples in the docs are misleading. It's something to keep in mind for now. :smile: Sorry for the confusion!

[asimonf]

@teohhanhui thanks. I still have one lingering issue. Won't this eventually lead to the same issue I mentioned in the title?

Specifically, in the input/output part of the docs it is specified for the books example that one can use entities as fields for the input DTO. Once a request arrives, that DTO will inevitably pass through the serializer on its way to the message queue, will it not?

BTW, I'm talking about this part:

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.

I'm guessing one would have to manually merge them, no?

[soyuka]

CQRS doesn't require things to be asynchronous. We're using this pattern with Write/Read differentiation in Api Platform's core concepts.

I'm guessing one would have to manually merge them, no?

Yes!

Once a request arrives, that DTO will inevitably pass through the serializer on its way to the message queue, will it not?

Exactly. But I don't think using our normalizers for this transportation phase is needed/recommended. Anyway for now it's expected to fail if you try to denormalize a resource that's not an ApiResource with our AbstractNormalizer (as you experimented).

[asimonf]

@soyuka Hello. Sorry to ask you here, but since this is related, I didn't want to start a whole new issue.

In the upcoming Doctrine version, the merge method will be deprecated and will have no alternative. In that case, what would be the best way to merge back the data on the consumer side to the Doctrine context?