alwaysIdentifier annotation option implementation #1528

adelplace · 2017-11-28T22:33:05Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#1130
License	MIT
Doc PR	api-platform/docs#373

The goal here is to allow the users to display only the IRI of a nested resource. It's very usefull on trees or on big entities which have multiple links on a same other entity.

As the MaxDepth annotation doesn't allow us to do that I have created a new annotation option :

/**
 * @ApiResource(attributes={
 *     "normalization_context"={"groups": {"alwaysIdentifierGroup"}, "enable_always_identifier"=true}
 * })
 * @ORM\Entity
 */
class AlwaysIdentifierDummy 
{
    /**
     * @var ArrayCollection children dummies
     *
     * @ORM\ManyToMany(targetEntity="AlwaysIdentifierDummy")
     * @ApiProperty(alwaysIdentifier=true)
     * @Groups({"alwaysIdentifierGroup"})
     */
    private $children;
}

Will be displayed as :

{
      "children": [
          "/always_identifier_dummies/1"
      ]
}

I don't know if this work fit with the project guidelines but i would be happy to discuss about it ;)

soyuka · 2017-11-29T07:59:34Z

~~We're successfully doing this by using groups and max depth. (going to check that anyway, coming back with an answer soon).~~

Nevermind I don't think we managed to do it.

What about using an attribute of @ApiProperty instead of a new annotation? For example @ApiProperty(skip_serialization=true) (not sure about the key).

I'm not fond of the word IriOnly by the way because we have to keep in mind that IRI's are related to JSON-ld and sometimes we might just want id's properties (ie force to skip serialization of the property).

adelplace · 2017-11-30T09:23:55Z

What about using an attribute of @ApiProperty instead of a new annotation? For example @ApiProperty(skip_serialization=true) (not sure about the key).

Yes it would surely be a better solution.

I'm not fond of the word IriOnly by the way because we have to keep in mind that IRI's are related to JSON-ld and sometimes we might just want id's properties (ie force to skip serialization of the property).

I'll look to implement it on the other formats.

meyerbaptiste · 2017-12-01T15:52:33Z

src/JsonLd/Serializer/ItemNormalizer.php

+     *
+     * @return bool
+     */
+    private function isIriOnlyItem(array &$context)


Why is $context passed by reference?

meyerbaptiste · 2017-12-01T15:53:55Z

src/JsonLd/Serializer/ItemNormalizer.php

+     *
+     * @return bool
+     */
+    private function isIriOnlyAttribute($propertyMetadata, array &$context)


Why is $context passed by reference?

meyerbaptiste · 2017-12-01T15:55:10Z

src/JsonLd/Serializer/ItemNormalizer.php

+    private function isIriOnlyAttribute($propertyMetadata, array &$context)
+    {
+        return
+            isset($context[static::ENABLE_IRI_ONLY]) &&


This class is final, use self instead of static.

adelplace · 2017-12-27T18:38:42Z

@soyuka @meyerbaptiste
The changes have been done, tell me if it's better. I also added tests for json and hal formats.

soyuka · 2017-12-28T07:18:00Z

src/Metadata/Property/Factory/AnnotationPropertyMetadataFactory.php

-                $annotation->attributes
+                $annotation->attributes,
+                null,
+                $annotation->skipSerialization


I think that the word "serializable" would be better:

isSerializable withSerializable serializable

soyuka · 2017-12-28T07:20:01Z

src/Serializer/AbstractItemNormalizer.php

+     *
+     * @return bool
+     */
+    private function isSkippedSerializationAttribute($propertyMetadata, array $context)


isPropertySerializable

Also missing typehints and you can remove the block comments (@param/@return):

function isPropertySerializable(PropertyMetadata $propertyMetadata, array $context): bool {}

Then negate: !$this->isPropertySerializable(...)

soyuka · 2017-12-28T07:23:27Z

src/Serializer/AbstractItemNormalizer.php

+     *
+     * @return bool
+     */
+    protected function isItemSerializationShouldBeSkipped(array $context)


shouldSkipSerialization same you can add a return type :bool and remove @param/@return.

dunglas · 2017-12-28T10:32:00Z

What about always_id instead? I don't like the "skip serialization" term because the serialization is actually not skipped. To me it would mean "just skip this property", here it is serialized in a special type: an IRI (btw, IRI is a generic term; not specific to JSON-LD: https://en.wikipedia.org/wiki/Internationalized_Resource_Identifier)

soyuka · 2017-12-28T10:59:29Z

👍 @dunglas , I'd propose in the metadata:

isAlwaysIdentifier
withAlwaysIdentifier
alwaysIdentifier

adelplace · 2017-12-29T20:29:48Z

@soyuka @dunglas
Done ;)

soyuka · 2017-12-30T11:27:24Z

features/main/skip_serialization.feature

@@ -0,0 +1,118 @@
+@skip


dunglas · 2018-01-11T09:37:39Z

src/Metadata/Property/PropertyMetadata.php

+    /**
+     * Returns a new instance with the given alwaysIdentifier flag.
+     *
+     * @param bool $alwaysIdentifier


You can remove both @param and @return, they are useless because of the typehints.

dunglas · 2018-01-11T09:46:44Z

src/Serializer/AbstractItemNormalizer.php

@@ -97,9 +100,21 @@ public function normalize($object, $format = null, array $context = [])
            $context['resources'][$resource] = $resource;
        }

+        if ($this->isAlwaysIdentifierItem($context)) {


Can you inline this one? We try to reduce the number of function calls in the serialization process (which is recursive) because a function call is costly:

if (($context[self::ALWAYS_IDENTIFIER_KEY] ?? false) && ($context[self::ALWAYS_IDENTIFIER_KEY] ?? false))

Same for isAlwaysIdentifierProperty, I would drop these methods.

dunglas · 2018-01-11T09:48:05Z

src/Serializer/AbstractItemNormalizer.php

@@ -39,6 +39,9 @@
 {
    use ContextTrait;

+    const ENABLE_ALWAYS_IDENTIFIER = 'enable_always_identifier';
+    const ALWAYS_IDENTIFIER_KEY = 'always_identifier_key';


We use the _key suffix nowhere. Can you drop it?

dunglas · 2018-01-11T09:49:45Z

tests/Fixtures/TestBundle/Entity/AlwaysIdentifierDummy.php

+ * @author Alexandre Delplace <[email protected]>
+ *
+ * @ApiResource(attributes={
+ *     "normalization_context"={"groups"={"alwaysIdentifierGroup"}, "enable_always_identifier"=true}


Why do we need this enable_always_identifier flag? IMO we can remove it, if the alwaysIdentifier is set on a property, we use it, that's all, it doesn't make sense to allow to disable this feature. If you don't want to use it, just don't set the flag.

I took example from the max_depth implementation. I'll remove it

dunglas · 2018-01-11T09:51:02Z