[Serializer] Rewrite `AbstractObjectNormalizer::createChildContext()` to use the provided `cache_key` from original context #53523

amne · 2024-01-12T13:43:27Z

Q	A
Branch?	6.4
Bug fix?	yes
New feature?	no
Deprecations?	no
Issues	N/A
License	MIT

If the normalization starts with an initial 'cache_key' it is currently discarded when creating child contexts. This change fixes that. The main reason behind it is that if the client code sends a unique identifier (ApiPlatform injects the IRI) and we have a use case that iterates a big resultset we end up with a big private cache that quickly runs out of allowed memory. The solution should be to send a 'cache_key' which ignores the entire cache key calculation that hashes the context.

carsonbot · 2024-01-12T13:43:33Z

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has a contribution guide which I suggest you to read.

In short:

Always add tests
Keep backward compatibility (see https://symfony.com/bc).
Bug fixes must be submitted against the lowest maintained branch where they apply (see https://symfony.com/releases)
Features and deprecations must be submitted against the 7.1 branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

… to use the provided cache_key from original context when creating child contexts If the normalization starts with an initial 'cache_key' it is currently discarded when creating child contexts. This change fixes that. The main reason behind it is that if the client code sends a unique identifier (ApiPlatform injects the IRI) and we have a use case that iterates a big resultset we end up with a big private cache that quickly runs out of allowed memory. The solution should be to send a 'cache_key' which ignores the entire cache key calculation that hashes the context.

mtarld

As it's a bug, you must target the 5.4 branch instead.

mtarld · 2024-01-12T15:17:40Z

src/Symfony/Component/Serializer/Normalizer/AbstractObjectNormalizer.php

@@ -287,8 +285,6 @@ abstract protected function extractAttributes(object $object, string $format = n

    /**
     * Gets the attribute value.
-     *
-     * @return mixed


These coding style changes must not be committed as they don't belong to your bugfix.

mtarld · 2024-01-12T15:19:04Z

src/Symfony/Component/Serializer/Normalizer/AbstractObjectNormalizer.php

@@ -153,9 +153,7 @@ public function supportsNormalization(mixed $data, string $format = null /* , ar
     */
    public function normalize(mixed $object, string $format = null, array $context = [])
    {
-        if (!isset($context['cache_key'])) {


I'd keep this if to avoid calling a method each time. Indeed, calling a method could be costly and we are in the hot path here.

mtarld · 2024-01-12T15:19:12Z

src/Symfony/Component/Serializer/Normalizer/AbstractObjectNormalizer.php

    public function denormalize(mixed $data, string $type, string $format = null, array $context = [])
    {
-        if (!isset($context['cache_key'])) {


mtarld · 2024-01-12T15:25:18Z

src/Symfony/Component/Serializer/Tests/Normalizer/AbstractObjectNormalizerTest.php

+        $normalizer = new class() extends AbstractObjectNormalizerDummy {
+            protected function extractAttributes(object $object, string $format = null, array $context = []): array
+            {
+                return array_keys((array) $object);
+            }
+
+            protected function getAttributeValue(object $object, string $attribute, string $format = null, array $context = []): mixed
+            {
+                return $object->{$attribute};
+            }
+
+            protected function createChildContext(array $parentContext, string $attribute, ?string $format): array
+            {
+                $childContext = parent::createChildContext($parentContext, $attribute, $format);
+                AbstractObjectNormalizerTest::assertSame('hardcoded-'.$attribute, $childContext['cache_key']);
+
+                return $childContext;
+            }
+        };
+
+        $serializer = new Serializer([$normalizer]);
+        $normalizedObject = $serializer->normalize($foobar, null, ['cache_key' => 'hardcoded']);
+        $this->assertSame($data, $normalizedObject);


To make sure that an assertion has been made, I'd use a property and make an assertion directly in the test:

Suggested change

$normalizer = new class() extends AbstractObjectNormalizerDummy {

protected function extractAttributes(object $object, string $format = null, array $context = []): array

{

return array_keys((array) $object);

}

protected function getAttributeValue(object $object, string $attribute, string $format = null, array $context = []): mixed

{

return $object->{$attribute};

}

protected function createChildContext(array $parentContext, string $attribute, ?string $format): array

{

$childContext = parent::createChildContext($parentContext, $attribute, $format);

AbstractObjectNormalizerTest::assertSame('hardcoded-'.$attribute, $childContext['cache_key']);

return $childContext;

}

};

$serializer = new Serializer([$normalizer]);

$normalizedObject = $serializer->normalize($foobar, null, ['cache_key' => 'hardcoded']);

$this->assertSame($data, $normalizedObject);

$normalizer = new class() extends AbstractObjectNormalizerDummy {

public ?string $childContextCacheKey = null;

protected function extractAttributes(object $object, string $format = null, array $context = []): array

{

return array_keys((array) $object);

}

protected function getAttributeValue(object $object, string $attribute, string $format = null, array $context = []): mixed

{

return $object->{$attribute};

}

protected function createChildContext(array $parentContext, string $attribute, ?string $format): array

{

$childContext = parent::createChildContext($parentContext, $attribute, $format);

$this->childContextCacheKey = $childContext['cache_key'];

return $childContext;

}

};

$serializer = new Serializer([$normalizer]);

$normalizedObject = $serializer->normalize($foobar, null, ['cache_key' => 'hardcoded']);

$this->assertSame($data, $normalizedObject);

$this->assertSame('hardcoded-'.$attribute, $normalizer->childContextCacheKey);

mtarld · 2024-01-12T15:26:59Z

src/Symfony/Component/Serializer/Normalizer/AbstractObjectNormalizer.php

@@ -746,6 +743,10 @@ protected function createChildContext(array $parentContext, string $attribute, ?
     */
    private function getCacheKey(?string $format, array $context): bool|string
    {
+        if (isset($context['cache_key'])) {


Should it go after $context[self::EXCLUDE_FROM_CACHE_KEY] foreach loop?

If I revert the removal of the inline ifs then this one goes away so it doesn't matter.

But to answer, I don't it should go after because the method would not be called if there is an existing $context['cache_key']

mtarld

Can you rebase your PR on the 5.4 branch please? 🙂

mtarld · 2024-01-13T09:00:21Z

src/Symfony/Component/Serializer/Tests/Normalizer/AbstractObjectNormalizerTest.php

+
+        $serializer = new Serializer([$normalizer]);
+        $normalizedObject = $serializer->normalize($foobar, null, ['cache_key' => 'hardcoded']);
+        // $this->assertSame($data, $normalizedObject);


Unexpected comment I guess

mtarld · 2024-01-13T09:00:47Z

src/Symfony/Component/Serializer/Normalizer/AbstractObjectNormalizer.php

@@ -302,9 +302,6 @@ public function supportsDenormalization(mixed $data, string $type, string $forma
        return class_exists($type) || (interface_exists($type, false) && null !== $this->classDiscriminatorResolver?->getMappingForClass($type));
    }

-    /**


This change should be reverted

mtarld · 2024-01-13T09:34:58Z

This can be closed in favor of #53530, thanks!

amne requested a review from dunglas as a code owner January 12, 2024 13:43

carsonbot added Status: Needs Review Bug Serializer labels Jan 12, 2024

carsonbot added this to the 6.4 milestone Jan 12, 2024

amne force-pushed the 6.4 branch from 6dc6158 to 96af30f Compare January 12, 2024 13:45

mtarld requested changes Jan 12, 2024

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels Jan 12, 2024

amne added 3 commits January 12, 2024 18:16

changed assert approach for childContextCacheKey

72aac24

reverted to inline ifs before calling getCacheKey

7a28a29

reverted unintended code style change

765d276

mtarld requested changes Jan 13, 2024

View reviewed changes

mtarld mentioned this pull request Jan 13, 2024

[Serializer] Rewrite AbstractObjectNormalizer::createChildContext() to use the provided cache_key from original context when creating child contexts #53530

Merged

OskarStark changed the title ~~[Serializer] Rewrite AbstractObjectNormalizer::createChildContext() to use the provided cache_key from original context~~ [Serializer] Rewrite AbstractObjectNormalizer::createChildContext() to use the provided cache_key from original context Jan 13, 2024

amne closed this Jan 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Serializer] Rewrite `AbstractObjectNormalizer::createChildContext()` to use the provided `cache_key` from original context #53523

[Serializer] Rewrite `AbstractObjectNormalizer::createChildContext()` to use the provided `cache_key` from original context #53523

amne commented Jan 12, 2024

carsonbot commented Jan 12, 2024

mtarld left a comment

mtarld Jan 12, 2024

mtarld Jan 12, 2024

mtarld Jan 12, 2024

mtarld Jan 12, 2024

mtarld Jan 12, 2024

amne Jan 12, 2024

amne Jan 12, 2024

mtarld left a comment

mtarld Jan 13, 2024

mtarld Jan 13, 2024

mtarld commented Jan 13, 2024

[Serializer] Rewrite AbstractObjectNormalizer::createChildContext() to use the provided cache_key from original context #53523

[Serializer] Rewrite AbstractObjectNormalizer::createChildContext() to use the provided cache_key from original context #53523

Conversation

amne commented Jan 12, 2024

carsonbot commented Jan 12, 2024

mtarld left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtarld left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtarld commented Jan 13, 2024

[Serializer] Rewrite `AbstractObjectNormalizer::createChildContext()` to use the provided `cache_key` from original context #53523

[Serializer] Rewrite `AbstractObjectNormalizer::createChildContext()` to use the provided `cache_key` from original context #53523