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

BC introduced in 3.1.15 when denormalizing a variable in a type than already been denormalized previously #6024

Closed
MariusJam opened this issue Dec 6, 2023 · 6 comments

Comments

@MariusJam
Copy link
Contributor

API Platform version(s) affected: ^3.1.15

Description
Hi,

It took me ages to find the problem on this one but a part of our app is broken as soon as we upgrade to ^3.1.15. The BC has been indroduced with this PR : #5663 and is linked to the modifications in src/Serializer/AbstractItemNormalizer.php.

The problem happens in a very specific case: if you try to denormalize manually an item of a type that has been already previously denormalized, the denormalization doesn't hydrate correctly the object.

How to reproduce

With :

<?php

namespace App\ApiResource;

use ApiPlatform\Metadata\ApiProperty;
use ApiPlatform\Metadata\ApiResource;
use ApiPlatform\Metadata\NotExposed;
use ApiPlatform\Metadata\Post;
use App\State\Processor\DummyProcessor;
use Symfony\Component\Serializer\Annotation\Groups;

#[ApiResource(operations: [
    new NotExposed(),
    new Post(denormalizationContext: ['groups' => ['dummy:write']], processor: DummyProcessor::class
    ),
])]
class Dummy
{
    public string $bar;

    #[ApiProperty(writableLink: true)]
    #[Groups(['dummy:write'])]
    public Foo $foo;
}
<?php

namespace App\ApiResource;

use ApiPlatform\Metadata\ApiResource;
use ApiPlatform\Metadata\Get;
use Symfony\Component\Serializer\Annotation\Groups;

#[ApiResource(operations: [new Get(normalizationContext: ['groups' => ['foo:read']],)])]
class Foo
{
    #[Groups(['foo:read'])]
    public string $bar;

    #[Groups(['dummy:write', 'foo:read'])]
    public string $name;
}
<?php

namespace App\State\Processor;

use ApiPlatform\Metadata\Operation;
use ApiPlatform\State\ProcessorInterface;
use App\ApiResource\Dummy;
use App\ApiResource\Foo;
use Symfony\Component\Serializer\Normalizer\DenormalizerInterface;

class DummyProcessor implements ProcessorInterface
{
    public function __construct(private readonly DenormalizerInterface $denormalizer)
    {
    }

    public function process(mixed $data, Operation $operation, array $uriVariables = [], array $context = [])
    {
        /**
         * In real context, $foo would be retrieved via an API call using $data->foo->name
         * i.e. $foo = response from GET https://external-api.org/foos?name={$data->foo->name}
         */
        $foo = ['bar' => 'barFromExternalApi'];

        $foo = $this->denormalizer->denormalize($foo, Foo::class);

        $dummy = new Dummy();
        $dummy->bar = $foo->bar;

        return $dummy;
    }
}

If you do a POST on /dummies with the following body:

{
	"foo": {
		"name": "FooName"
	}
}

In v3.1.14, you correctly received a 201 response with the following body:

{
	"@context": "\/contexts\/Dummy",
	"@id": "\/dummies",
	"@type": "Dummy",
	"bar": "barFromExternalApi"
}

In v3.1.15, the denormalization in DummyProcessor does not hydrate correctly the Foo object and you get an Exception : "Typed property App\ApiResource\Foo::$bar must not be accessed before initialization"

Possible Solution

No real idea so far

Additional Context

I haven't been able to find the exact problem but this is linked to the $operationCacheKey used in AbstractItemNormalizer::getFactoryOptions

@soyuka
Copy link
Member

soyuka commented Dec 7, 2023

looks weird, does the exception occurs when you denormalize by hand? You should use a context in your call, maybe that you:

  • need to specify the object_to_populate
  • or add a default null value to your property

@MariusJam
Copy link
Contributor Author

Hi Soyuka,

What do you mean by "denormalizing by hand" ? Is it not what I'm already doing in DummyProcessor?

  • Adding an object_to_populate does not fix the issue and still, it should be possible to denormalize without specifying it.
  • adding a default null value fix the Exception but not the root issue, I just receive a 201 response with body:
{
	"@context": "\/contexts\/Dummy",
	"@id": "\/dummies",
	"@type": "Dummy"
}

Instead of :

{
	"@context": "\/contexts\/Dummy",
	"@id": "\/dummies",
	"@type": "Dummy",
	"bar": "barFromExternalApi"
}

The denormamization of Foo is just completely skipped. My hypothesis is that happens because of getFactoryOptions() returning wrong cached $options. I can see that $operationCacheKey values change between 3.1.14 and 3.1.15.

@soyuka
Copy link
Member

soyuka commented Dec 8, 2023

I see, do you have any idea on how we could fix this?

@MariusJam
Copy link
Contributor Author

MariusJam commented Dec 12, 2023

I understand now why it breaks:

In v3.1.14, the $context was built in AbstractItemNormalizer::createOperationContext() and, if the class you were denormalizing was an ApiResource, $context['operation_name'] was unset then set again :

        unset($context['operation'], $context['operation_name']);
        $context['resource_class'] = $resourceClass;
        if ($this->resourceMetadataCollectionFactory) {
            try {
                $context['operation'] = $this->resourceMetadataCollectionFactory->create($resourceClass)->getOperation();
                $context['operation_name'] = $context['operation']->getName();
            } catch (OperationNotFoundException) {
            }
        }

From v3.1.15 onwards, the $context is built in src/Serializer/OperationContextTrait.php and the $context['operation_name'] is just unset.

This is a problem because the $operationCacheKey in AbstractItemNormalizer::getFactoryOptions() is a concatenation using $context['operation_name'] :

        $operationCacheKey = ($context['resource_class'] ?? '').($context['operation_name'] ?? '').($context['api_normalize'] ?? '');

So, in v3.1.15 onwards and for a given class, the $operationCacheKey is always the same in all denormalization calls whereas it had different values before v3.1.15 depending on $context['operation_name'] .

To fix it, we should either continue to set $context['operation_name'] in OperationContextTrait (like it was the case before) or use another value to generate the $operationCacheKey. I don't know which solution is the best one. I guess it also depends of the original purpose of #5663 and I'm not really aware of it.

@MariusJam
Copy link
Contributor Author

I close the issue, this has been fixed via #6073, thanks @soyuka !

@soyuka
Copy link
Member

soyuka commented Jan 15, 2024

Thanks for digging into this!

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

2 participants