[Serializer] Allow default group in serialization context #44035

noemi-salaun · 2021-11-13T10:22:53Z

Q	A
Branch?	6.1
Bug fix?	no
New feature?	yes
Deprecations?	no
Tickets	Fix #32622
License	MIT
Doc PR	symfony/symfony-docs#...

The _default group can be use on the serialization context to explicitly tell the serializer to allow properties without defined @Groups

Exemple:

class User
{

    public $name;

    /**
     * @Groups({"private"})
     */
    public $age;

    /**
     * @Groups({"private", "moderator"})
     */
    public $isBan;
}

With no context groups: name, age, isBan
With moderator context group: isBan
With * context group: name, age, isBan
With _default context group: name
With _default and moderator context groups: name, isBan

I use the _default keyword here but it can be easily changed for something else like I said in this issue #32622 (comment)

I don't know what could be the best option to reduce the BC break.

A special char, like for the *. Could be _. (not a fan of this solution)

A special word, like Default in the JMS Serializer (high chance of BC breaks with existing project)

The null value

The empty string value

null or empty string value cannot be use in @Groups annotation, so we are sure there is no BC break with this solution

carsonbot · 2021-11-14T06:27:17Z

Hey!

I think @BafS has recently worked with this code. Maybe they can help review this?

Cheers!

Carsonbot

noemi-salaun · 2022-01-21T11:03:14Z

Anyone available to help me with this PR?

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

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

Fix symfony#32622

symfony#44035 (review)

TerjeBr · 2022-07-07T12:25:10Z

I am waiting on this. What is holding this up?

ninsuo · 2022-09-18T17:23:39Z

Hello,
I'm coming back on this, can we somehow help on what blocks the PR?
Have a nice week,
Alain

fabpot

cc @dunglas

fabpot · 2022-09-20T05:51:25Z

src/Symfony/Component/PropertyInfo/Extractor/SerializerExtractor.php

+
+    private function getAttributeGroups(AttributeMetadataInterface $serializerAttributeMetadata): array
+    {
+        $groups = empty($serializerAttributeMetadata->getGroups()) ? [AbstractNormalizer::DEFAULT_GROUP_FOR_ATTRIBUTE_WITHOUT_GROUPS] : $serializerAttributeMetadata->getGroups();


Suggested change

$groups = empty($serializerAttributeMetadata->getGroups()) ? [AbstractNormalizer::DEFAULT_GROUP_FOR_ATTRIBUTE_WITHOUT_GROUPS] : $serializerAttributeMetadata->getGroups();

$groups = !$serializerAttributeMetadata->getGroups() ? [AbstractNormalizer::DEFAULT_GROUP_FOR_ATTRIBUTE_WITHOUT_GROUPS] : $serializerAttributeMetadata->getGroups();

Calling getGroups() two times isn't necessary. Storing the result of this function in a variable will improve the performance.

fabpot · 2022-09-20T05:51:36Z

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

@@ -257,6 +263,13 @@ protected function getGroups(array $context): array
        return is_scalar($groups) ? (array) $groups : $groups;
    }

+    protected function getAttributeGroups(AttributeMetadataInterface $attributeMetadata): array
+    {
+        $groups = empty($attributeMetadata->getGroups()) ? [self::DEFAULT_GROUP_FOR_ATTRIBUTE_WITHOUT_GROUPS] : $attributeMetadata->getGroups();


Suggested change

$groups = empty($attributeMetadata->getGroups()) ? [self::DEFAULT_GROUP_FOR_ATTRIBUTE_WITHOUT_GROUPS] : $attributeMetadata->getGroups();

$groups = !$attributeMetadata->getGroups() ? [self::DEFAULT_GROUP_FOR_ATTRIBUTE_WITHOUT_GROUPS] : $attributeMetadata->getGroups();

Or even

$groups = $attributeMetadata->getGroups() ?: [self::DEFAULT_GROUP_FOR_ATTRIBUTE_WITHOUT_GROUPS];

fabpot · 2022-09-20T05:52:13Z

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

@@ -112,6 +112,12 @@ abstract class AbstractNormalizer implements NormalizerInterface, DenormalizerIn
     */
    public const IGNORED_ATTRIBUTES = 'ignored_attributes';

+    /**
+     * The default group can be use on the serialization context to explicitly


Suggested change

* The default group can be use on the serialization context to explicitly

* The default group can be used on the serialization context to explicitly

dunglas · 2022-09-20T06:12:53Z

Couldn't we use the same conventions as for the Validator component for consistency?

TerjeBr · 2022-09-20T10:40:05Z

Couldn't we use the same conventions as for the Validator component for consistency?

By that you mean use the group name "Default" instead of "_default"? Yes, I am in favour of that. (The only problem is that it is a BC break for code that already uses the group name "Default")

dunglas · 2022-09-20T19:12:12Z

Yes, I mean that, and also support using the name of the class as a group name (User in the example provided in the docs).

Regarding the BC break, isn't using _default a BC break too?

dunglas · 2022-09-20T19:14:26Z

src/Symfony/Component/PropertyInfo/Extractor/SerializerExtractor.php

+
+    private function getAttributeGroups(AttributeMetadataInterface $serializerAttributeMetadata): array
+    {
+        $groups = empty($serializerAttributeMetadata->getGroups()) ? [AbstractNormalizer::DEFAULT_GROUP_FOR_ATTRIBUTE_WITHOUT_GROUPS] : $serializerAttributeMetadata->getGroups();


Calling getGroups() two times isn't necessary. Storing the result of this function in a variable will improve the performance.

dunglas · 2022-09-20T19:15:49Z

src/Symfony/Component/PropertyInfo/Extractor/SerializerExtractor.php

+    {
+        $groups = empty($serializerAttributeMetadata->getGroups()) ? [AbstractNormalizer::DEFAULT_GROUP_FOR_ATTRIBUTE_WITHOUT_GROUPS] : $serializerAttributeMetadata->getGroups();
+
+        return array_merge($groups, ['*']);


Suggested change

return array_merge($groups, ['*']);

$groups[] = '*';

return $groups;

This will also probably be a bit faster.

dunglas · 2022-09-20T19:18:19Z

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

@@ -257,6 +263,13 @@ protected function getGroups(array $context): array
        return is_scalar($groups) ? (array) $groups : $groups;
    }

+    protected function getAttributeGroups(AttributeMetadataInterface $attributeMetadata): array


This is on the hot patch. Introducing a new function will have a significant performance impact when serializing a large set of data. Also, we try to not add new protected methods in this class (we're in the process of splitting it and using composition instead of inheritance).

Could you inline it? By the way, you should also inline the one in the extractor.

mtarld · 2022-10-07T09:00:19Z

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

@@ -257,6 +263,13 @@ protected function getGroups(array $context): array
        return is_scalar($groups) ? (array) $groups : $groups;
    }

+    protected function getAttributeGroups(AttributeMetadataInterface $attributeMetadata): array
+    {
+        $groups = empty($attributeMetadata->getGroups()) ? [self::DEFAULT_GROUP_FOR_ATTRIBUTE_WITHOUT_GROUPS] : $attributeMetadata->getGroups();


Or even

$groups = $attributeMetadata->getGroups() ?: [self::DEFAULT_GROUP_FOR_ATTRIBUTE_WITHOUT_GROUPS];

mtarld · 2022-10-07T09:03:49Z

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

+     * The default group can be use on the serialization context to explicitly
+     * tell the serializer to allow properties without defined groups.
+     */
+    public const DEFAULT_GROUP_FOR_ATTRIBUTE_WITHOUT_GROUPS = '_default';


Is this supposed to be overridden in the userland?
If not, we could mark it as final I guess, and maybe move it on top of L36 so that it's not considered as a context constant.
If yes, then why not store it in the context (and update the AbstractNormalizerContextBuilder)?

nicolas-grekas · 2023-11-20T08:49:19Z

Closing in favor of #51514, thanks for pushing this forward!

noemi-salaun requested a review from dunglas as a code owner November 13, 2021 10:22

carsonbot added Status: Needs Review Feature Serializer labels Nov 13, 2021

carsonbot added this to the 5.4 milestone Nov 13, 2021

ogizanagi modified the milestones: 5.4, 6.1 Nov 13, 2021

chadyred reviewed May 19, 2022

View reviewed changes

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

chadyred reviewed May 19, 2022

View reviewed changes

src/Symfony/Component/Serializer/Tests/Normalizer/AbstractNormalizerTest.php Show resolved Hide resolved

src/Symfony/Component/Serializer/Tests/Normalizer/AbstractNormalizerTest.php Show resolved Hide resolved

fabpot modified the milestones: 6.1, 6.2 May 20, 2022

noemi-salaun added 2 commits May 23, 2022 09:52

[Serializer] Allow default group in serialization context

ba0c3b7

Fix symfony#32622

Extract _default in a public constant as suggested by chadyred

4efbc42

symfony#44035 (review)

noemi-salaun force-pushed the feat32622-serializer-default-group branch from cfdef73 to 4efbc42 Compare May 23, 2022 08:05

chadyred approved these changes May 26, 2022

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels May 26, 2022

chadyred approved these changes May 26, 2022

View reviewed changes

fabpot requested changes Sep 20, 2022

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Reviewed labels Sep 20, 2022

dunglas requested changes Sep 20, 2022

View reviewed changes

mtarld requested changes Oct 7, 2022

View reviewed changes

nicolas-grekas modified the milestones: 6.2, 6.3 Nov 5, 2022

nicolas-grekas modified the milestones: 6.3, 6.4 May 23, 2023

nicolas-grekas modified the milestones: 6.4, 7.1 Nov 15, 2023

nicolas-grekas closed this Nov 20, 2023

	$groups = empty($serializerAttributeMetadata->getGroups()) ? [AbstractNormalizer::DEFAULT_GROUP_FOR_ATTRIBUTE_WITHOUT_GROUPS] : $serializerAttributeMetadata->getGroups();
	$groups = !$serializerAttributeMetadata->getGroups() ? [AbstractNormalizer::DEFAULT_GROUP_FOR_ATTRIBUTE_WITHOUT_GROUPS] : $serializerAttributeMetadata->getGroups();

	$groups = empty($attributeMetadata->getGroups()) ? [self::DEFAULT_GROUP_FOR_ATTRIBUTE_WITHOUT_GROUPS] : $attributeMetadata->getGroups();
	$groups = !$attributeMetadata->getGroups() ? [self::DEFAULT_GROUP_FOR_ATTRIBUTE_WITHOUT_GROUPS] : $attributeMetadata->getGroups();

	* The default group can be use on the serialization context to explicitly
	* The default group can be used on the serialization context to explicitly

-        return array_merge($groups, ['*']);
+        $groups[] = '*';
+        return $groups;

Uh oh!

[Serializer] Allow default group in serialization context #44035

[Serializer] Allow default group in serialization context #44035

Uh oh!

Conversation

noemi-salaun commented Nov 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

carsonbot commented Nov 14, 2021

Uh oh!

noemi-salaun commented Jan 21, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TerjeBr commented Jul 7, 2022

Uh oh!

ninsuo commented Sep 18, 2022

Uh oh!

fabpot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dunglas commented Sep 20, 2022

Uh oh!

TerjeBr commented Sep 20, 2022

Uh oh!

dunglas commented Sep 20, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas commented Nov 20, 2023

Uh oh!

Uh oh!

noemi-salaun commented Nov 13, 2021 •

edited

Loading