[Serializer] Respect ignored attributes in cache key of normalizer #30907

dbu · 2019-04-06T14:36:13Z

EUFOSSA

Q	A
Branch	3.4
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	-
License	MIT
Doc PR	-

Do not share the attributes cache in object normalizer when using a different setting for the ignoredAttributes setting.

In Symfony 4.2, the setter is deprecated in favor of the ignored_attibutes option in the $context. When merging this up, we will however still need to respect the field as well for BC, the cache key does not look at the default context (apart from the deprecated modifiers, the default context is immutable)

There might be performance regression for some use cases, but also could be a performance improvement when using 'attributes' in the context with lists of objects of the same class.

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

dbu · 2019-04-06T14:40:24Z

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

@@ -128,15 +128,13 @@ protected function getAttributes($object, $format = null, array $context)
            return $allowedAttributes;
        }

-        if (isset($context['attributes'])) {


we previously skipped the cache if attributes are defined in the context. now we use the attributes as part of the cache key.

dbu · 2019-04-07T08:07:16Z

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

+                'context' => $context,
+                'ignored' => $this->ignoredAttributes,
+                'camelized' => $this->camelizedAttributes,
+            ]));


i hope i caught all normalizer state that could influence which attributes are available and not too many.

things like circular reference or callbacks should not change which attributes are available i guess?

(serialize one array rather than concat several serialize result, assuming that gives better performance)

dunglas

Such changes require careful manual testing and profiling. But LGTM.

dbu · 2019-04-07T12:08:52Z

FTR: build error is due to a travis install error during hhvm build. i am confident this is not related to the PR ;-) the other builds are green.

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

dbu · 2019-04-08T08:07:09Z

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

     *
     * @return array
     *
     * @internal
     */
-    protected function createChildContext(array $parentContext, $attribute)
+    protected function createChildContext(array $parentContext, $attribute/*, ?string $format*/)


should i also trigger a deprecation warning here? we don't actually use the format at all...

not on 3.4 but on master yes - also, this should be /*, string $format = null */, isn't it?

@dbu Can you fix the typos? (no ? and a space before */?)

as its a protected function, i actually think ?string would make more sense, you will always pass the parameter even when its null. nullable types is php 7.2 only, which i guess symfony 5 will require at least. i changed it to the optional argument syntax for now.

also, if its indeed an optional argument, the deprecation warning would be unnecessary.

ok, fair enough

shall i change it to nullable argument? the nullable syntax was introduced in 7.1 actually. or shall i only change it on master when i also add a deprecation warning?

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

dbu · 2019-04-08T08:09:36Z

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

@@ -382,8 +406,13 @@ private function isMaxDepthReached(array $attributesMetadata, $class, $attribute
     */
    private function getCacheKey($format, array $context)
    {
+        unset($context['cache_key']); // avoid artifically different keys


if we would keep this field, every call to this method generates a new key as the previous key is part of what we hash. the correct thing is to not include the previous key in the hash as its not meaningful information.

fabpot · 2019-04-08T10:10:33Z

Thank you @dbu.

…rmalizer (dbu) This PR was squashed before being merged into the 3.4 branch (closes #30907). Discussion ---------- [Serializer] Respect ignored attributes in cache key of normalizer EUFOSSA | Q | A | ------------- | --- | Branch | 3.4 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - Do not share the attributes cache in object normalizer when using a different setting for the ignoredAttributes setting. In Symfony 4.2, the setter is deprecated in favor of the ignored_attibutes option in the $context. When merging this up, we will however still need to respect the field as well for BC, the cache key does not look at the default context (apart from the deprecated modifiers, the default context is immutable) There might be performance regression for some use cases, but also could be a performance improvement when using 'attributes' in the context with lists of objects of the same class. Commits ------- 926d228 [Serializer] Respect ignored attributes in cache key of normalizer

fabpot · 2019-04-08T10:28:39Z

@dbu I think we need your help with merging this one on 4.2 and master.

dbu · 2019-04-08T10:47:53Z

happy to help. do i create a branch from 4.2, merge 3.4 to it, then solve conflicts and do a PR with that?

fabpot · 2019-04-08T10:50:37Z

@dbu That would be super hepful, yes.

xabbuh · 2019-04-08T11:50:10Z

Sorry, I didn't see your conversation before merging 3.4 up.

@dbu Can you review my conflict resolution in ec41d76 and also take a look into #31005 which should solve the remaining deprecation (and thus the failing test)?

dbu · 2019-04-08T12:09:54Z

ah, now i was confused until i red this :-D

resolution looks good to me.

xabbuh · 2019-04-08T12:17:38Z

@dbu Thank you! 👍

dbu · 2019-04-08T12:18:16Z

should i do the deprecation change against 4.2 or master? ec41d76#r33074748 => will do against master when changes have been merged there.

fabpot · 2019-04-08T14:12:09Z

@dbu 4.2 has been merged into master now.

@joelwurtz

This PR was squashed before being merged into the 4.3-dev branch (closes #30888). Discussion ---------- [serializer] extract normalizer tests to traits eufossa | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | Relates to #30818 | License | MIT | Doc PR | - As discussed with @joelwurtz, extract normalizer functionality tests into traits to ensure consistent behaviour of all normalizers. * [x] Rebase when #30977, #30950 and #30907 are merged to master **blocker** * [x] Clean up order of trait inclusion and methods in the tests * [x] Clean up fixture classes of the traits. I started having one class named the same as the trait, where possible Stuff that we should do eventually, but can also do in separate pull requests, after this one has been merged: * [ ] Extract all features that we can (the existing normalizer tests should more or less only have the legacy tests in them, all functionality should be in trait) * [ ] Run test coverage and increase coverage so that we cover all important features and all relevant error cases. Commits ------- 2b6ebea [serializer] extract normalizer tests to traits

…out the format parameter (dbu) This PR was submitted for the master branch but it was merged into the 4.3 branch instead (closes #31030). Discussion ---------- [Serializer] Deprecate calling createChildContext without the format parameter | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - as discussed in #30907 deprecate omitting the format parameter when extending the AbstractNormalizer. Commits ------- cb77902 deprecate calling createChildContext without the format parameter

…lasr) This PR was merged into the 4.4 branch. Discussion ---------- [Serializer] Fix tests wrongly marked as incomplete | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | no | New feature? | no | Deprecations? | no | Tickets | - | License | MIT | Doc PR | - #30907 has been merged meanwhile. Commits ------- e632302 [Serializer] Fix tests marked as incomplete

carsonbot added Status: Needs Review Serializer Bug labels Apr 6, 2019

dbu commented Apr 6, 2019

View reviewed changes

src/Symfony/Component/Serializer/Normalizer/AbstractObjectNormalizer.php Outdated Show resolved Hide resolved

dbu commented Apr 6, 2019

View reviewed changes

sroze added the ⭐️ EUFOSSA Hackathon label Apr 6, 2019

dbu force-pushed the fix-object-normalizer-attributes-cache branch 2 times, most recently from 6737389 to 7b64d88 Compare April 6, 2019 14:45

nicolas-grekas added this to the 3.4 milestone Apr 6, 2019

jewome62 approved these changes Apr 7, 2019

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Apr 7, 2019

dbu commented Apr 7, 2019

View reviewed changes

dunglas approved these changes Apr 7, 2019

View reviewed changes

dbu mentioned this pull request Apr 7, 2019

[serializer] extract normalizer tests to traits #30888

Merged

5 tasks

fabpot requested changes Apr 7, 2019

View reviewed changes

src/Symfony/Component/Serializer/Normalizer/AbstractObjectNormalizer.php Outdated Show resolved Hide resolved

src/Symfony/Component/Serializer/Normalizer/AbstractObjectNormalizer.php Outdated Show resolved Hide resolved

carsonbot added Status: Needs Work and removed Status: Reviewed labels Apr 7, 2019

dbu force-pushed the fix-object-normalizer-attributes-cache branch from a81cbc0 to ea4f387 Compare April 7, 2019 13:55

carsonbot removed the Status: Needs Work label Apr 7, 2019

maxhelias approved these changes Apr 7, 2019

View reviewed changes

carsonbot added the Status: Reviewed label Apr 7, 2019

dbu force-pushed the fix-object-normalizer-attributes-cache branch from ea4f387 to be2e3fa Compare April 8, 2019 08:05

dbu commented Apr 8, 2019

View reviewed changes

src/Symfony/Component/Serializer/Normalizer/AbstractObjectNormalizer.php Outdated Show resolved Hide resolved

dbu commented Apr 8, 2019

View reviewed changes

dbu force-pushed the fix-object-normalizer-attributes-cache branch 3 times, most recently from 20aae5f to 100fe8d Compare April 8, 2019 09:30

fabpot approved these changes Apr 8, 2019

View reviewed changes

[Serializer] Respect ignored attributes in cache key of normalizer

926d228

fabpot force-pushed the fix-object-normalizer-attributes-cache branch from 100fe8d to 926d228 Compare April 8, 2019 10:10

fabpot merged commit 926d228 into symfony:3.4 Apr 8, 2019

dbu deleted the fix-object-normalizer-attributes-cache branch April 8, 2019 10:48

dbu mentioned this pull request Apr 9, 2019

[Serializer] Deprecate calling createChildContext without the format parameter #31030

Merged

This was referenced Apr 16, 2019

Release v3.4.25 #31123

Merged

Release v4.2.6 #31125

Merged

chalasr mentioned this pull request Jan 26, 2021

[Serializer] Fix tests wrongly marked as incomplete #39975

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Serializer] Respect ignored attributes in cache key of normalizer #30907

[Serializer] Respect ignored attributes in cache key of normalizer #30907

dbu commented Apr 6, 2019 •

edited

dbu Apr 6, 2019

dbu Apr 7, 2019

dunglas left a comment

dbu commented Apr 7, 2019

dbu Apr 8, 2019

nicolas-grekas Apr 8, 2019

fabpot Apr 8, 2019

dbu Apr 8, 2019

dbu Apr 8, 2019

fabpot Apr 8, 2019

dbu Apr 8, 2019

dbu Apr 8, 2019

fabpot commented Apr 8, 2019

fabpot commented Apr 8, 2019

dbu commented Apr 8, 2019

fabpot commented Apr 8, 2019

xabbuh commented Apr 8, 2019

dbu commented Apr 8, 2019

xabbuh commented Apr 8, 2019

dbu commented Apr 8, 2019 •

edited

fabpot commented Apr 8, 2019

[Serializer] Respect ignored attributes in cache key of normalizer #30907

[Serializer] Respect ignored attributes in cache key of normalizer #30907

Conversation

dbu commented Apr 6, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dunglas left a comment

Choose a reason for hiding this comment

dbu commented Apr 7, 2019

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

fabpot commented Apr 8, 2019

fabpot commented Apr 8, 2019

dbu commented Apr 8, 2019

fabpot commented Apr 8, 2019

xabbuh commented Apr 8, 2019

dbu commented Apr 8, 2019

xabbuh commented Apr 8, 2019

dbu commented Apr 8, 2019 • edited

fabpot commented Apr 8, 2019

dbu commented Apr 6, 2019 •

edited

dbu commented Apr 8, 2019 •

edited