8000 bug #43469 [Serializer] symfony#36594 attributes cache breaks normali… · symfony/symfony@732acf5 · GitHub
[go: up one dir, main page]
Skip to content

Commit 732acf5

Browse files
fabpotfabpot
committed
bug #43469 [Serializer] #36594 attributes cache breaks normalization (ivannemets-sravniru)
This PR was squashed before being merged into the 5.4 branch. Discussion ---------- [Serializer] #36594 attributes cache breaks normalization | Q | A | ------------- | --- | Branch? | 5.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | Fix #36594 | License | MIT | Doc PR | symfony/symfony-docs#15823 The bug itself is explained in the following (simplified) example: ```php class Dummy { public array $requiredData; // supposed to be set for any object public array $optionalData; // can be not initialized (and if so - ignored by serializer) } $object1 = new Dummy(); $object1->requiredData = ['username' => 'foo']; $json1 = $serializer->serialize($object1, 'json'); // {"requiredData": {"username": "foo"}} // at this point object normalizer has already cached attributes for Dummy::class and context, // now it contains array ['requiredData'] - optionalData has been ignored as it's unitialized // then, while the script is still running, we have another object of the same class with optionalData set $object2 = new Dummy(); $object2->requiredData = ['username' => 'bar']; $object2->optionalData = ['email' => 'bar@test.com']; $json2 = $serializer->serialize($object2, 'json'); // expected: {"requiredData": {"username": "bar"}, "optionalData": {"email": "bar@test.com"}} // actual: {"requiredData": {"username": "bar"}} // here normalizer has no clue about optionalData attribute since it reuses attributes cached // in \Symfony\Component\Serializer\Normalizer\AbstractObjectNormalizer::$attributesCache // while normalizing the first object ``` Though this PR created for 5.4 branch, it actually fixes a bug reproducible in 5.3. The reason why I use 5.4 for this fix is that 5.4 introduces a [new feature](symfony/symfony-docs#15823) related to the same problem. If this PR gets approved and merged, I can potentially implement the same fix for 5.3, but `SKIP_UNINITIALIZED_VALUES` will cause some merge conflicts in the future.. As of v 5.3 symfony ignores uninitialized properties by default in `ObjectNormalizer::extractAttributes` and `PropertyNormalizer::extractAttributes` (implemented in #38900 and #34791) but this approach is wrong - **`extractAttributes` method MUST return the same attributes list for any instance of the same class**, otherwise cached attributes do not match actual attributes list for different instances having different set of initialized/uninitialized properties So, this PR does a few things: 1. removes ignoring attributes from all built-in implementations of `\Symfony\Component\Serializer\Normalizer\AbstractObjectNormalizer::extractAttributes` so that even uninitialized attributes will be cached by normalizer. Instead, we ignore uninitialized attributes when trying to access them while calling `\Symfony\Component\Serializer\Normalizer\AbstractObjectNormalizer::getAttributeValue` 2. sets `true` as the default value for `SKIP_UNINITIALIZED_VALUES` context setting as I believe this is the expected default behavior for any built-in object normalizer 3. makes `SKIP_UNINITIALIZED_VALUES` compatible with not just `ObjectNormalizer`, but with two other built-in normalizers `PropertyNormalizer` and `GetSetMethodNormalizer` as well Commits ------- 55818c3 [Serializer] #36594 attributes cache breaks normalization
2 parents f1b8b6e + 55818c3 commit 732acf5

File tree

9 files changed

+238
-33
lines changed

9 files changed

+238
-33
lines changed

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

Lines changed: 28 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -123,6 +123,10 @@ public function __construct(ClassMetadataFactoryInterface $classMetadataFactory
123123

124124
$this->defaultContext[self::EXCLUDE_FROM_CACHE_KEY] = array_merge($this->defaultContext[self::EXCLUDE_FROM_CACHE_KEY] ?? [], [self::CIRCULAR_REFERENCE_LIMIT_COUNTERS]);
125125

126+
if (\PHP_VERSION_ID >= 70400) {
127+
$this->defaultContext[self::SKIP_UNINITIALIZED_VALUES] = true;
128+
}
129+
126130
$this->propertyTypeExtractor = $propertyTypeExtractor;
127131

128132
if (null === $classDiscriminatorResolver && null !== $classMetadataFactory) {
@@ -190,7 +194,12 @@ public function normalize($object, string $format = null, array $context = [])
190194
try {
191195
$attributeValue = $this->getAttributeValue($object, $attribute, $format, $attributeContext);
192196
} catch (UninitializedPropertyException $e) {
193-
if ($context[self::SKIP_UNINITIALIZED_VALUES] ?? $this->defaultContext[self::SKIP_UNINITIALIZED_VALUES] ?? false) {
197+
if ($this->shouldSkipUninitializedValues($context)) {
198+
continue;
199+
}
200+
throw $e;
201+
} catch (\Error $e) {
202+
if ($this->shouldSkipUninitializedValues($context) && $this->isUninitializedValueError($e)) {
194203
continue;
195204
}
196205
throw $e;
@@ -724,4 +733,22 @@ private function getCacheKey(?string $format, array $context)
724733
return false;
725734
}
726735
}
736+
737+
private function shouldSkipUninitializedValues(array $context): bool
738+
{
739+
return $context[self::SKIP_UNINITIALIZED_VALUES]
740+
?? $this->defaultContext[self::SKIP_UNINITIALIZED_VALUES]
741+
?? false;
742+
}
743+
744+
/**
745+
* This error may occur when specific object normalizer implementation gets attribute value
746+
* by accessing a public uninitialized property or by calling a method accessing such property.
747+
*/
748+
private function isUninitializedValueError(\Error $e): bool
749+
{
750+
return \PHP_VERSION_ID >= 70400
751+
&& str_starts_with($e->getMessage(), 'Typed property')
752+
&& str_ends_with($e->getMessage(), 'must not be accessed before initialization');
753+
}
727754
}

‎src/Symfony/Component/Serializer/Normalizer/ObjectNormalizer.php‎

Lines changed: 1 addition & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -107,23 +107,9 @@ protected function extractAttributes(object $object, string $format = null, arra
107107
}
108108
}
109109

110-
$checkPropertyInitialization = \PHP_VERSION_ID >= 70400;
111-
112110
// properties
113111
foreach ($reflClass->getProperties() as $reflProperty) {
114-
$isPublic = $reflProperty->isPublic();
115-
116-
if ($checkPropertyInitialization) {
117-
if (!$isPublic) {
118-
$reflProperty->setAccessible(true);
119-
}
120-
if (!$reflProperty->isInitialized($object)) {
121-
unset($attributes[$reflProperty->name]);
122-
continue;
123-
}
124-
}
125-
126-
if (!$isPublic) {
112+
if (!$reflProperty->isPublic()) {
127113
continue;
128114
}
129115

‎src/Symfony/Component/Serializer/Normalizer/PropertyNormalizer.php‎

Lines changed: 0 additions & 11 deletions
Original file l 9E88 ine numberDiff line numberDiff line change
@@ -101,20 +101,9 @@ protected function extractAttributes(object $object, string $format = null, arra
101101
{
102102
$reflectionObject = new \ReflectionObject($object);
103103
$attributes = [];
104-
$checkPropertyInitialization = \PHP_VERSION_ID >= 70400;
105104

106105
do {
107106
foreach ($reflectionObject->getProperties() as $property) {
108-
if ($checkPropertyInitialization) {
109-
if (!$property->isPublic()) {
110-
$property->setAccessible(true);
111-
}
112-
113-
if (!$property->isInitialized($object)) {
114-
continue;
115-
}
116-
}
117-
118107
if (!$this->isAllowedAttribute($reflectionObject->getName(), $property->name, $format, $context)) {
119108
continue;
120109
}

‎src/Symfony/Component/Serializer/Tests/Normalizer/Features/CacheableObjectAttributesTestTrait.php‎

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,59 @@
1+
<?php
2+
3+
namespace Symfony\Component\Serializer\Tests\Normalizer\Features;
4+
5+
use Symfony\Component\Serializer\Normalizer\AbstractObjectNormalizer;
6+
7+
/**
8+
* This test ensures that attributes caching implemented in AbstractObjectNormalizer
9+
* does not break normalization of multiple objects having different set of initialized/unInitialized properties.
10+
*
11+
* The attributes cache MUST NOT depend on a specific object state, so that cached attributes could be reused
12+
* while normalizing any number of instances of the same class in any order.
13+
*/
14+
trait CacheableObjectAttributesTestTrait
15+
{
16+
/**
17+
* Returns a collection of objects to be normalized and compared with the expected array.
18+
* It is a specific object normalizer test class responsibility to prepare testing data.
19+
*/
20+
abstract protected function getObjectCollectionWithExpectedArray(): array;
21+
22+
abstract protected function getNormalizerForCacheableObjectAttributesTest(): AbstractObjectNormalizer;
23+
24+
/**
25+
* The same normalizer instance normalizes two objects of the same class in a row:
26+
* 1. an object having some uninitialized properties
27+
* 2. an object with all properties being initialized.
28+
*
29+
* @requires PHP 7.4
30+
*/
31+
public function testObjectCollectionNormalization()
32+
{
33+
[$collection, $expectedArray] = $this->getObjectCollectionWithExpectedArray();
34+
$this->assertCollectionNormalizedProperly($collection, $expectedArray);
35+
}
36+
37+
/**
38+
* The same normalizer instance normalizes two objects of the same class in a row:
39+
* 1. an object with all properties being initialized
40+
* 2. an object having some uninitialized properties.
41+
*
42+
* @requires PHP 7.4
43+
*/
44+
public function testReversedObjectCollectionNormalization()
45+
{
46+
[$collection, $expectedArray] = array_map('array_reverse', $this->getObjectCollectionWithExpectedArray());
47+
$this->assertCollectionNormalizedProperly($collection, $expectedArray);
48+
}
49+
50+
private function assertCollectionNormalizedProperly(array $collection, array $expectedArray): void
51+
{
52+
self::assertCount(\count($expectedArray), $collection);
53+
$normalizer = $this->getNormalizerForCacheableObjectAttributesTest();
54+
foreach ($collection as $i => $object) {
55+
$result = $normalizer->normalize($object);
56+
self::assertSame($expectedArray[$i], $result);
57+
}
58+
}
59+
}

‎src/Symfony/Component/Serializer/Tests/Normalizer/Features/SkipUninitializedValuesTestTrait.php‎

Lines changed: 20 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -14,25 +14,39 @@ abstract protected function getNormalizerForSkipUninitializedValues(): Normalize
1414

1515
/**
1616
* @requires PHP 7.4
17+
* @dataProvider skipUninitializedValuesFlagProvider
1718
*/
18-
public function testSkipUninitializedValues()
19+
public function testSkipUninitializedValues(array $context)
1920
{
20-
$object = new TypedPropertiesObject();
21+
$object = new TypedPropertiesObjectWithGetters();
2122

2223
$normalizer = $this->getNormalizerForSkipUninitializedValues();
23-
$result = $normalizer->normalize($object, null, ['skip_uninitialized_values' => true, 'groups' => ['foo']]);
24+
$result = $normalizer->normalize($object, null, $context);
2425
$this->assertSame(['initialized' => 'value'], $result);
2526
}
2627

28+
public function skipUninitializedValuesFlagProvider(): iterable
29+
{
30+
yield 'passed manually' => [['skip_uninitialized_values' => true, 'groups' => ['foo']]];
31+
yield 'using default context value' => [['groups' => ['foo']]];
32+
}
33+
2734
/**
2835
* @requires PHP 7.4
2936
*/
3037
public function testWithoutSkipUninitializedValues()
3138
{
32-
$object = new TypedPropertiesObject();
39+
$object = new TypedPropertiesObjectWithGetters();
3340

3441
$normalizer = $this->getNormalizerForSkipUninitializedValues();
35-
$this->expectException(UninitializedPropertyException::class);
36-
$normalizer->normalize($object, null, ['groups' => ['foo']]);
42+
43+
try {
44+
$normalizer->normalize($object, null, ['skip_uninitialized_values' => false, 'groups' => ['foo']]);
45+
$this->fail('Normalizing an object with uninitialized property should have failed');
46+
} catch (UninitializedPropertyException $e) {
47+
self::assertSame('The property "Symfony\Component\Serializer\Tests\Normalizer\Features\TypedPropertiesObject::$unInitialized" is not readable because it is typed "string". You should initialize it or declare a default value instead.', $e->getMessage());
48+
} catch (\Error $e) {
49+
self::assertSame('Typed property Symfony\Component\Serializer\Tests\Normalizer\Features\TypedPropertiesObject::$unInitialized must not be accessed before initialization', $e->getMessage());
50+
}
3751
}
3852
}

‎src/Symfony/Component/Serializer/Tests/Normalizer/Features/TypedPropertiesObjectWithGetters.php‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
<?php
2+
3+
namespace Symfony\Component\Serializer\Tests\Normalizer\Features;
4+
5+
class TypedPropertiesObjectWithGetters extends TypedPropertiesObject
6+
{
7+
public function getUnInitialized(): string
8+
{
9+
return $this->unInitialized;
10+
}
11+
12+
public function setUnInitialized(string $unInitialized): self
13+
{
14+
$this->unInitialized = $unInitialized;
15+
16+
return $this;
17+
}
18+
19+
public function getInitialized(): string
20+
{
21+
return $this->initialized;
22+
}
23+
24+
public function setInitialized(string $initialized): self
25+
{
26+
$this->initialized = $initialized;
27+
28+
return $this;
29+
}
30+
31+
public function getInitialized2(): string
32+
{
33+
return $this->initialized2;
34+
}
35+
36+
public function setInitialized2(string $initialized2): self
37+
{
38+
$this->initialized2 = $initialized2;
39+
40+
return $this;
41+
}
42+
}

‎src/Symfony/Component/Serializer/Tests/Normalizer/GetSetMethodNormalizerTest.php‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,24 +31,29 @@
3131
use Symfony\Component\Serializer\Tests\Fixtures\Annotations\GroupDummy;
3232
use Symfony\Component\Serializer\Tests\Fixtures\CircularReferenceDummy;
3333
use Symfony\Component\Serializer\Tests\Fixtures\SiblingHolder;
34+
use Symfony\Component\Serializer\Tests\Normalizer\Features\CacheableObjectAttributesTestTrait;
3435
use Symfony\Component\Serializer\Tests\Normalizer\Features\CallbacksTestTrait;
3536
use Symfony\Component\Serializer\Tests\Normalizer\Features\CircularReferenceTestTrait;
3637
use Symfony\Component\Serializer\Tests\Normalizer\Features\ConstructorArgumentsTestTrait;
3738
use Symfony\Component\Serializer\Tests\Normalizer\Features\GroupsTestTrait;
3839
use Symfony\Component\Serializer\Tests\Normalizer\Features\IgnoredAttributesTestTrait;
3940
use Symfony\Component\Serializer\Tests\Normalizer\Features\MaxDepthTestTrait;
4041
use Symfony\Component\Serializer\Tests\Normalizer\Features\ObjectToPopulateTestTrait;
42+
use Symfony\Component\Serializer\Tests\Normalizer\Features\SkipUninitializedValuesTestTrait;
43+
use Symfony\Component\Serializer\Tests\Normalizer\Features\TypedPropertiesObjectWithGetters;
4144
use Symfony\Component\Serializer\Tests\Normalizer\Features\TypeEnforcementTestTrait;
4245

4346
class GetSetMethodNormalizerTest extends TestCase
4447
{
48+
use CacheableObjectAttributesTestTrait;
4549
use CallbacksTestTrait;
4650
use CircularReferenceTestTrait;
4751
use ConstructorArgumentsTestTrait;
4852
use GroupsTestTrait;
4953
use IgnoredAttributesTestTrait;
5054
use MaxDepthTestTrait;
5155
use ObjectToPopulateTestTrait;
56+
use SkipUninitializedValuesTestTrait;
5257
use TypeEnforcementTestTrait;
5358

5459
/**
@@ -440,6 +445,27 @@ public function testHasGetterNormalize()
440445
$this->normalizer->normalize($obj, 'any')
441446
);
442447
}
448+
449+
protected function getObjectCollectionWithExpectedArray(): array
450+
{
451+
return [[
452+
new TypedPropertiesObjectWithGetters(),
453+
(new TypedPropertiesObjectWithGetters())->setUninitialized('value2'),
454+
], [
455+
['initialized' => 'value', 'initialized2' => 'value'],
456+
['unInitialized' => 'value2', 'initialized' => 'value', 'initialized2' => 'value'],
457+
]];
458+
}
459+
460+
protected function getNormalizerForCacheableObjectAttributesTest(): GetSetMethodNormalizer
461+
{
462+
return new GetSetMethodNormalizer();
463+
}
464+
465+
protected function getNormalizerForSkipUninitializedValues(): NormalizerInterface
466+
{
467+
return new GetSetMethodNormalizer(new ClassMetadataFactory(new AnnotationLoader(new AnnotationReader())));
468+
}
443469
}
444470

445471
class GetSetDummy

‎src/Symfony/Component/Serializer/Tests/Normalizer/ObjectNormalizerTest.php‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@
3939
use Symfony\Component\Serializer\Tests\Fixtures\Php74DummyPrivate;
4040
use Symfony\Component\Serializer\Tests\Fixtures\SiblingHolder;
4141
use Symfony\Component\Serializer\Tests\Normalizer\Features\AttributesTestTrait;
42+
use Symfony\Component\Serializer\Tests\Normalizer\Features\CacheableObjectAttributesTestTrait;
4243
use Symfony\Component\Serializer\Tests\Normalizer\Features\CallbacksTestTrait;
4344
use Symfony\Component\Serializer\Tests\Normalizer\Features\CircularReferenceTestTrait;
4445
use Symfony\Component\Serializer\Tests\Normalizer\Features\ConstructorArgumentsTestTrait;
@@ -50,6 +51,8 @@
5051
use Symfony\Component\Serializer\Tests\ 92BA Normalizer\Features\ObjectToPopulateTestTrait;
5152
use Symfony\Component\Serializer\Tests\Normalizer\Features\SkipNullValuesTestTrait;
5253
use Symfony\Component\Serializer\Tests\Normalizer\Features\SkipUninitializedValuesTestTrait;
54+
use Symfony\Component\Serializer\Tests\Normalizer\Features\TypedPropertiesObject;
55+
use Symfony\Component\Serializer\Tests\Normalizer\Features\TypedPropertiesObjectWithGetters;
5356
use Symfony\Component\Serializer\Tests\Normalizer\Features\TypeEnforcementTestTrait;
5457

5558
/**
@@ -58,6 +61,7 @@
5861
class ObjectNormalizerTest extends TestCase
5962
{
6063
use AttributesTestTrait;
64+
use CacheableObjectAttributesTestTrait;
6165
use CallbacksTestTrait;
6266
use CircularReferenceTestTrait;
6367
use ConstructorArgumentsTestTrait;
@@ -558,6 +562,33 @@ protected function getNormalizerForSkipUninitializedValues(): ObjectNormalizer
558562
return new ObjectNormalizer($classMetadataFactory);
559563
}
560564

565+
protected function getObjectCollectionWithExpectedArray(): array
566+
{
567+
$typedPropsObject = new TypedPropertiesObject();
568+
$typedPropsObject->unInitialized = 'value2';
569+
570+
$collection = [
571+
new TypedPropertiesObject(),
572+
$typedPropsObject,
573+
new TypedPropertiesObjectWithGetters(),
574+
(new TypedPropertiesObjectWithGetters())->setUninitialized('value2'),
575+
];
576+
577+
$expectedArrays = [
578+
['initialized' => 'value', 'initialized2' => 'value'],
579+
['unInitialized' => 'value2', 'initialized' => 'value', 'initialized2' => 'value'],
580+
['initialized' => 'value', 'initialized2' => 'value'],
581+
['unInitialized' => 'value2', 'initialized' => 'value', 'initialized2' => 'value'],
582+
];
583+
584+
return [$collection, $expectedArrays];
585+
}
586+
587+
protected function getNormalizerForCacheableObjectAttributesTest(): ObjectNormalizer
588+
{
589+
return new ObjectNormalizer();
590+
}
591+
561592
// type enforcement
562593

563594
protected function getDenormalizerForTypeEnforcement(): ObjectNormalizer

0 commit comments

Comments
 (0)
0