Incorrect Operation context when serializing nested ApiResource properties

jonnyeom commented 1 year ago

API Platform version(s) affected: 2.7(with new metadata), 3.0, 3.1/main

Description
Hello, I am not sure if this is necessarily a bug, or a misunderstanding on the approach on my part.

While I was upgrading a large project from api-platform 2.6 >> 3.x, I am running into an interesting issue during serialization.

Example
Here is an example to explain the situation.

Lets say I have an ApiResource Car.
- Car has a referenced property Wheel. This can be a collection. But Wheel is not an ApiResource.
- Wheel has a referenced property Tire which is an ApiResource.

You can see in this situation, The nesting of entities to serialize is Resource object > non-Resource object > Resource object.

When you serialize Car, its serialization context bubbles down all the way to Wheel and to Tire.

When serializing Car $context['operation'] is set to Car. This is passed down to Wheel.
- When serializing Wheel, $context['operation'] is still set to Car. But its not used in the AbstractObjectNormalizer, so simply passed on
- When serializing Tire, $context['operation'] is still set to Car. AbstractItemNormalizer will look at the passed $context['operation'] and will not try to determine the correct operation for Tire. This is where the issue happens.

The biggest culprit ive run into is \ApiPlatform\Symfony\Routing\IriConverter::getIriFromResource() uses the given $operation parameter to generate an iri. But with the wrong context, it will throw an Exception saying Unable to generate an IRI for the item of type

Possible Solution

unset the $context['operation'] each time we go into a new nested level (Perhaps at the $childContext level).
\ApiPlatform\Serializer\AbstractItemNormalizer::getAttributeValue seems to generate the $context['operation'] whenever it needs one.
If going with this approach, unsetting the operation context towards the beginning of ::getAttributeValue seems to fix this issue, although I have not fully tested it.
Make everything an ApiResource.

This is another way to solve this does not seem right. If I mark every nested property of an ApiResource class also as an ApiResource, I will not run into problems. But I would have to alter the generated Api docs for each class. I would also really be mis-using the ApiResource attribute for simple serialization purposes.

Bug vs Documentation
Of the two approaches above,
If the intention with 3.x is for every class needing to be marked as ApiResource— This is not a bug. I think additional documentation to make it clearer would help.

If that is not the intention,
Than I will definitely start working on a clean PR for this.

mrossard commented 1 year ago

I think i'm misunderstanding something here : isn'it it perfectly normal for $context['operation'] to keep its value? The operation being processed doesn't change when you process the subresources, and it can have an impact on how they should be serialized.

Would you have a reproducer for this? IMO this is supposed to work, there shouldn't be a need to mark the "middle" entity as ApiResource.

soyuka commented 1 year ago

I don't think that I ever tried that use case, would it be possible to have a reproducer?

Also "marking everything as resource" would be better indeed as its hard to work with non-resources (they don't have any matching metadata...)

jonnyeom commented 1 year ago

@soyuka I was thinking about "marking everything as resource". This actually seems like an anti-pattern. I feel you should be able to serialize an ApiResource within an object. and you can. its just this weird sandwich scenario. I have yet to have time to add a test for this but definitely planning to.

2 points to Id love to get thoughts on. 1. Context should be preserved @mrossard I agree it is normal for $context['operation'] to be passed around. Even when requests are passed around, there is a ->isMainRequest() check to differentiate between main and sub-requests.

Perhaps that is what we need? AbstractCollectionNormalizer saves the current context as the operation context as $context['root_operation']

// \ApiPlatform\Serializer\AbstractCollectionNormalizer::normalize

        if (isset($context['operation'])) {
            $context['root_operation'] = $context['operation'];
        }

        if (isset($context['operation_name'])) {
            $context['root_operation_name'] = $context['operation_name'];
        }

        unset($context['operation']);
        unset($context['operation_type'], $context['operation_name']);

We could do something similar, but globally in the way context is handled. We'd just have to update places where the root_context is required (most likely just serialization groups).

2. Even within AbstractItemNormalizer::getAttributeValue the context['operation] is almost always overwritten There is 3 main sections to this method

Handle collection of Relations
Handle a Resource object
Handle anything else that has a Type (like a Doctrine Collection). (3.1 introduces an extra check to handle 'array')

In the first 2, the operation context is unset or overwritten anyway. See https://github.com/api-platform/core/blob/ebf03104fcbffc5af74d78c3e9b14d02d7527214/src/Serializer/AbstractItemNormalizer.php#L615

Its the anything-else section that is causing problems because we dont know what to do with the operation context seeing that it will not be overwritten.

The more I think about it, I like the root_operation context methodology. It makes sense in these cases where we create child contexts. Again, i'd appreciate any thoughts here! My current workaround is just unsetting it 😆

soyuka commented 1 year ago

I think you got everything right. The operation changes because we need IRIs of the current object (an IRI is an uriTemplate and its uriVariables, both given by the operation). IMO, even if the operation is not found we're able to serialize it since API Platform 3 (serialization of non-resources was hard before that). This would mean that even if a case like a non-resource is found, and therefore no operation is found, if it has an embed object we need to do the check again.

jonnyeom commented 1 year ago

@soyuka This commit https://github.com/api-platform/core/commit/e3e6a0d3213feeffa1365c3e52d7283eb7444d7d seems to introduce root_operation to all child contexts.

I think I can work off of this. Are you planning to make additional changes related to this issue? Just trying to avoid duplicating work

soyuka commented 1 year ago

If you can work with that I won't, let me release that so that you can give me a feedback?

jonnyeom commented 1 year ago

Sounds good 😊

jonnyeom commented 1 year ago

Just an update,

According to my tests, it seems partially resolved. But I still see it happening in my applications. (At least on 2.7). I'm working on moving it to 3.x to fully test. and once there, I should be able to write a test scenario for this issue as well

api-platform / core

Incorrect Operation context when serializing nested ApiResource properties #5704