Fix allow creating resources with custom identifers #2022

bendavies · 2018-06-13T22:34:35Z

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#1937, #2018, #1741, api-platform/api-platform#343
License	MIT

This is a re-submission of #1939 with fixes and passing tests. @Toflar's commits are retained.

…s can be used to identify nested relations

soyuka · 2018-06-14T07:53:57Z

src/Serializer/AbstractItemNormalizer.php

+            throw new InvalidArgumentException('Update is not allowed for this operation.');
+        }
+
+        // TODO: use $this->iriConverter->getIriFromPlainIdentifier() once https://github.com/api-platform/core/pull/1837 is merged.


The #1837 PR is based on master.

yeah, but the todo is still valid, it can be updated in master.

Was a note for myself :D

soyuka · 2018-06-14T07:57:31Z

src/Serializer/AbstractItemNormalizer.php

+                ),
+                $context + ['fetch_data' => true]
+            );
+        }


Looks like we're using twice the same logic here, can't we avoid to repeat these?

i noticed yes... it would be nice,

@soyuka if you have some ideas, please say. I can't see a very nice way atm.

Can you shed some light on why this is needed? Looks like a fallback algorithm that should go into the iriconverter?

I've added a comment a few lines up. it is a fix for #857

Not sure I agree with the fix, but I have kept it as per the original as a behat feature tests it.

Toflar · 2018-06-14T08:02:50Z

Nice one! Want me to close the other PR then, right?

bendavies · 2018-06-14T08:28:26Z

@Toflar I'm happy to close this and you can cherry pick into the original PR if you like. this PR is rebased on 2.2 btw.

Toflar · 2018-06-14T08:54:17Z

Nah, I'm perfectly fine with you taking over! Thanks for the work!

bendavies · 2018-06-15T12:49:44Z

@dunglas @teohhanhui @meyerbaptiste can i ask for a review please?

soyuka · 2018-06-17T11:11:05Z

features/main/custom_normalized.feature:52 failed though

bendavies · 2018-06-17T16:53:40Z

Hmm. I'll check it out.

…

On Sun, 17 Jun 2018, 12:11 Antoine Bluchet, ***@***.***> wrote: features/main/custom_normalized.feature:52 failed though — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2022 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAmK8JwfRY6olgiGteZCIwTXX4BzmX1Aks5t9jlJgaJpZM4UnCoq> .

bendavies · 2018-06-18T10:29:57Z

think I've fixed this by making the feature correct, to correctly address #857.

meyerbaptiste · 2018-06-19T07:45:36Z

src/JsonLd/Serializer/ItemNormalizer.php

-            if (isset($context['api_allow_update']) && true !== $context['api_allow_update']) {
-                throw new InvalidArgumentException('Update is not allowed for this operation.');
+        try {
+            parent::setObjectToPopulate($data, $class, $context);


Can you add a return; here and move the code in the catch part outside?

you mean an empty catch right?

meyerbaptiste · 2018-06-19T07:47:25Z

src/JsonLd/Serializer/ItemNormalizer.php

+            // https://github.com/api-platform/core/issues/857
+            $identifiers = $this->identifiersExtractor->getIdentifiersFromResourceClass($class);
+            $identifiersData = \array_intersect_key($data, array_flip($identifiers));
+            if (0 === \count($identifiersData)) {


if (!$identifiersData) {

is this a style adopted by API-P? i prefer to be explicit but happy to align with project style.

meyerbaptiste · 2018-06-19T07:51:50Z

src/Serializer/AbstractItemNormalizer.php

+        $identifiers = $this->getIdentifiersForDenormalization($class);
+        $identifiersData = \array_intersect_key($data, array_flip($identifiers));
+
+        if (0 === \count($identifiersData)) {


if (!$identifiersData)) {

meyerbaptiste · 2018-06-19T07:54:07Z

src/Serializer/AbstractItemNormalizer.php

+    protected function getIdentifiersForDenormalization(string $class): array
+    {
+        // Backwards compatibility
+        if (null === $this->identifiersExtractor) {


if (!$this->identifiersExtractor) {

again, happy to follow the style of the project, but if (null === $variable) is done quite a lot in this project, so i'm not sure there's any point unless consistency is introduced.

meyerbaptiste · 2018-06-19T07:57:45Z

src/Serializer/AbstractItemNormalizer.php

+        }
+
+        if (!$updateAllowed) {
+            throw new InvalidArgumentException('Update is not allowed for this operation.');


Should not this exception be thrown before the previous condition (the return;)?

don't think so, we only throw if identifiers are provided.

meyerbaptiste · 2018-06-19T07:59:05Z

features/main/custom_normalized.feature

    When I add "Accept" header equal to "application/json"
    And I send a "PUT" request to "/related_normalized_dummies/1" with body:
    """
    {
      "name": "My Dummy",
      "customNormalizedDummy":[{
        "@context": "/contexts/CustomNormalizedDummy",
-        "@id": "/custom_normalized_dummies/1",
+        "@id": "app_dev.php/custom_normalized_dummies/1",


Why app_dev.php?

nothing specific about app_dev.php. this is a scenario for #857 (#877) and i needed a way to force the custom behavior in the Json LD ItemNormalizer. i.e the iri must be incorrect.

bendavies · 2018-06-21T11:03:06Z

For completeness, as I said to @dunglas privately, I won't have a chance to work on this proposal for quite a while. So if anyone else has some time, please feel free :)

teohhanhui · 2018-06-22T10:23:54Z

I'm -1 for using @id field in a non JSON-LD context. It only creates confusion and adds no value.

dunglas · 2018-06-22T10:44:39Z

I'm -1 for using @id field in a non JSON-LD context. It only creates confusion and adds no value.

@id has several benefits:

@ is a reserved char in PHP, we're sure it cannot conflict with a property defined by the user
it's an existing convention (JSON-LD), we don't reinvent something entirely proprietary

Anyway we need a special key for the IRI. We may also use the GraphQL convention (id for the IRI, _id for the internal raw id) but it has several drawbacks:

it will be harder to port legacy API to API Platform: most legacy APIs usually use id for the raw id, it's not possible with this convention
_ isn't a special char in PHP

So to me @id is the best bet. It has a last advantage: it allows to drop the ItemNormalizer for the JSON-LD one (less code, less maintenance, only 1 implementation = less edge cases).

teohhanhui · 2018-06-22T10:50:03Z

My stand is consistent with what I've said about the Problem RFC. We don't get any benefit by mimicking a standard. It only leads to confusion as to whether we're actually following the standard or not (we aren't, so it shouldn't look like we are).

maks-rafalko · 2018-09-03T11:22:22Z

Hello,

what is the status of this particular PR and the issue itself?

From time to time people keep creating bugs in different repositories about this problem, and the freshest PR is this one which is also not active anymore.

Not able to send ID in POST method api-platform#343 (June 22, 2017)
POST resource with a non-autogenerated identifier property named "id" (works with another property name) #1741 (March 1, 2018)
ID not writable when creating resource #2018 (June 13, 2018)

Questions:

Have you agreed with the approach internally @dunglas?
Do you need any help and can you guide someone to resolve this issue?
Do you plan to move forward with this approach or start from the scratch?

This bug is quite old and very annoying, so I would like to point your attention to it and try to move it forward.

Thank you.

maks-rafalko · 2018-09-06T08:45:49Z

Well, a couple of days with debugger and it works now.

The code below is for those who find a solution right meow before anything is done officially in the repository (which I suspect may take months)
Use it on your own risk, but it works for me and all tests are green

What we did is just replaced ItemNormalizer with our own implementation, that allows to post the following JSON:

POST /resources
{
    "id": 1234545,
    "anyOther": "fields"
}

Custom ItemNormalizer (click to show)

declare(strict_types=1);

namespace App\Serializer\Normalizer;

use ApiPlatform\Core\Exception\InvalidArgumentException;
use ApiPlatform\Core\Serializer\AbstractItemNormalizer;


class ItemNormalizer extends AbstractItemNormalizer
{
    private const IDENTIFIER = 'id';

    public function denormalize($data, $class, $format = null, array $context = [])
    {
        $context['api_denormalize'] = true;

        if (!isset($context['resource_class'])) {
            $context['resource_class'] = $class;
        }

        $this->setObjectToPopulate($data, $context);

        return parent::denormalize($data, $class, $format, $context);
    }

    protected function setObjectToPopulate($data, array &$context): void
    {
        // in PUT request OBJECT_TO_POPULATE is already set by this moment
        if (!\is_array($data) || isset($context[self::OBJECT_TO_POPULATE])) {
            return;
        }

        [$identifierName, $identifierMetadata] = $this->getResourceIdentifierData($context);

        $isUpdateAllowed = $context['api_allow_update'] ?? false;
        $hasIdentifierInRequest = array_key_exists(self::IDENTIFIER, $data);
        $hasWritableIdentifierInRequest = $hasIdentifierInRequest && $identifierMetadata->isWritable();
        // when it is POST, update is not allowed for top level resource, but is allowed for nested resources
        $isTopLevelResourceInPostRequest = !$isUpdateAllowed
            && $context['operation_type'] === 'collection'
            && $context['collection_operation_name'] === 'post';

        // if Resource does not have an ID OR if it is writable custom id - we should not populate Entity from DB
        if (!$hasIdentifierInRequest || ($hasWritableIdentifierInRequest && $isTopLevelResourceInPostRequest)) {
            return;
        }

        if (!$isUpdateAllowed) {
            throw new InvalidArgumentException('Update is not allowed for this operation.');
        }

        try {
            $context[self::OBJECT_TO_POPULATE] = $this->iriConverter->getItemFromIri(
                (string) $data[self::IDENTIFIER],
                $context + ['fetch_data' => true]
            );
        } catch (InvalidArgumentException $e) {
            $context[self::OBJECT_TO_POPULATE] = $this->iriConverter->getItemFromIri(
                sprintf(
                    '%s/%s',
                    $this->iriConverter->getIriFromResourceClass($context['resource_class']),
                    $data[$identifierName]
                ),
                $context + ['fetch_data' => true]
            );
        }
    }

    private function getResourceIdentifierData(array $context): array
    {
        $identifierPropertyName = null;
        $identifierPropertyMetadata = null;
        $className = $context['resource_class'];

        $properties = $this->propertyNameCollectionFactory->create($className, $context);

        foreach ($properties as $propertyName) {
            $property = $this->propertyMetadataFactory->create($className, $propertyName);

            if ($property->isIdentifier()) {
                $identifierPropertyName = $propertyName;
                $identifierPropertyMetadata = $property;
                break;
            }
        }

        if ($identifierPropertyMetadata === null) {
            throw new \LogicException(
                sprintf(
                    'Resource "%s" must have an identifier. Properties: %s.',
                    $className,
                    implode(',', iterator_to_array($properties->getIterator()))
                )
            );
        }

        return [$identifierPropertyName, $identifierPropertyMetadata];
    }
}

To replace api-platform's analyzer with this custom one - just override class in CompilerPass

src/Kernel.php

+ use App\Serializer\Normalizer\ItemNormalizer;

- class Kernel extends BaseKernel
+ class Kernel extends BaseKernel implements CompilerPassInterface

+    public function process(ContainerBuilder $container)
+    {
+        $container->getDefinition('api_platform.serializer.normalizer.item')->setClass(ItemNormalizer::class);
+    }
 }

Still, I want to move forward this discussion. This is one of the feature that I expect to work out of the box from platform named api-platform.

And by the way, provided patch in this PR does not work, I literally copied code to vendor folder and try to debug it.

UPD: the full code for api-platform 2.4.3+ is here api-platform/api-platform#343 (comment)

dunglas · 2018-09-11T17:35:55Z

Can you rebase against 2.3 please?

bendavies · 2018-09-11T17:37:22Z

@dunglas why? You have proposed a better solution above. This pr should not be merged, I thought we decided.

dunglas · 2018-09-11T18:28:11Z

My bad, I read this PR too fast.

soyuka · 2018-09-12T06:56:40Z

Let's close this then.

bendavies · 2018-09-12T07:51:47Z

let's try not forget about your proposal @dunglas :)

maks-rafalko · 2018-09-12T07:58:52Z

I suggest opening another issue with that proposal, because it will be forgotten 100% ;)

soyuka · 2018-09-12T08:40:07Z

lol was doing too many things at once and forgot to submit the issue, done :)

clemherreman · 2018-12-28T09:51:38Z

Hello everyone,
Are there any news on this? I noticed #2201, and I wanted to know if there is any ETA on this. Like @borNfreee I'm willing to help on that, but I guess I'd need some guidance on how :)

My use case is that I have an application, consuming my API, that needs to work offline, and still create new resources instances locally, while offline. A good solution would be to have my resources uses UUID as identifiers, and allow to specify the identifiers in POST calls. This way the application and the API can create new instances independently.

As this issues is closed, I'll post it on ##2201 too, because I'm not sure somebody is reading here :)

nfacciolo · 2019-08-29T07:27:43Z

Well, a couple of days with debugger and it works now.
* The code below is for those who find a solution right meow before anything is done officially in the repository (which I suspect may take months)

* Use it on your own risk, but it works for me and all tests are green
What we did is just replaced ItemNormalizer with our own implementation, that allows to post the following JSON:
POST /resources
{
    "id": 1234545,
    "anyOther": "fields"
}
Custom ItemNormalizer (click to show)

To replace api-platform's analyzer with this custom one - just override class in CompilerPass

src/Kernel.php
+ use App\Serializer\Normalizer\ItemNormalizer;

- class Kernel extends BaseKernel
+ class Kernel extends BaseKernel implements CompilerPassInterface

+    public function process(ContainerBuilder $container)
+    {
+        $container->getDefinition('api_platform.serializer.normalizer.item')->setClass(ItemNormalizer::class);
+    }
 }
Still, I want to move forward this discussion. This is one of the feature that I expect to work out of the box from platform named api-platform.

And by the way, provided patch in this PR does not work, I literally copied code to vendor folder and try to debug it.

UPD: the full code for api-platform 2.4.3+ is here api-platform/api-platform#343 (comment)

See api-platform/api-platform#343 (comment) if you want make it work

loicleng · 2022-10-21T09:03:55Z

Hello everyone,

Here is an example I made for ApiPlatform >= 3.0 that converts plain ids sent for relation field (either for item or collection relation) to IRI's before denormalization.
It may be improved but seems to be working for me so I'll still post it here in case it may help someone as I personnally struggled to find custom normalizer examples.

<?php

namespace App\Serializer;

use ApiPlatform\Api\IriConverterInterface;
use ApiPlatform\Metadata\Property\Factory\PropertyMetadataFactoryInterface;
use Symfony\Component\Serializer\Normalizer\DenormalizerInterface;
use Symfony\Component\Serializer\Normalizer\NormalizerInterface;
use Symfony\Component\PropertyInfo\Type;

// Enable to send plain identifiers as object relation or collection
final class CustomItemNormalizer implements NormalizerInterface, DenormalizerInterface
{

    public function __construct(
        private readonly NormalizerInterface $normalizer,
        private readonly PropertyMetadataFactoryInterface $propertyMetadataFactory,
        private readonly IriConverterInterface $iriConverter
    )
    {
        if (!$normalizer instanceof DenormalizerInterface) {
            throw new \InvalidArgumentException('The normalizer must implement the DenormalizerInterface');
        }
    }

    public function denormalize($data, $class, $format = null, array $context = [])
    {
        if ($class == $context["operation"]->getClass()) {
            foreach ($data as $key => $value) {
                $propertyMetadata = $this->propertyMetadataFactory->create($class, $key);
                $propertyTypeObject = $propertyMetadata->getBuiltinTypes()[0];
                $propertyType = $propertyTypeObject->getBuiltinType();

                // If property is a relation and is not a translation relation
                if ($propertyType === 'object' && $key !== 'translations') {
                    $data = $this->convertIdRelationsToIRIs($data, $key, $value, $propertyTypeObject);
                }
            }
        }

        return $this->normalizer->denormalize($data, $class, $format, $context);
    }

    private function convertIdRelationsToIRIs(array $data, string $key, mixed $value, Type $propertyTypeObject): array {

        // For collection relation: Will replace data embbed relation sent as "categories": [1, 2, 3] to "categories": ['/catalog/categories/1', '/catalog/categories/2', ...]
        if ($propertyTypeObject->isCollection() && !str_contains('Translation', $propertyTypeObject->getCollectionValueTypes()[0]->getClassName())) {
            $data = $this->handleCollectionIdsConversion($data, $key, $value, $propertyTypeObject);

        // For single relation : Converts "category": 1 to "category": "/catalog/categories/1"
        } else {
            $data = $this->handleItemIdConversion($data, $key, $value, $propertyTypeObject);
        }

        return $data;
    }

    private function handleCollectionIdsConversion(array $data, string $key, mixed $value, Type $propertyTypeObject): array
    {
        $embbedClass = new ($propertyTypeObject->getCollectionValueTypes()[0]->getClassName());
        $manyToManyIRIsList = [];
        foreach ($value as $manyToManyEmbbedObjectId) {
            if (!is_int($manyToManyEmbbedObjectId)) {
                throw new \InvalidArgumentException('Parameters for relation ' . $embbedClass::class . ' must be integers' );
            }
            $iri = $this->iriConverter->getIriFromResource(resource: $embbedClass::class, context: ['uri_variables' => ['id' => $manyToManyEmbbedObjectId]]);
            $manyToManyIRIsList[] = $iri;
        }
        if (count($manyToManyIRIsList) > 0) {
            $data[$key] = $manyToManyIRIsList;
        }

        return $data;
    }

    private function handleItemIdConversion(array $data, string $key, mixed $value, Type $propertyTypeObject): array
    {
        $embbedClass = new ($propertyTypeObject->getClassName());

        if (!is_int($value)) {
            throw new \InvalidArgumentException('Parameter for relation ' . $embbedClass::class . ' must be integer' );
        }

        $iri = $this->iriConverter->getIriFromResource(resource: $embbedClass::class, context: ['uri_variables' => ['id' => $value]]);
        $data[$key] = $iri;

        return $data;
    }

    public function supportsDenormalization($data, $type, $format = null)
    {
        return $this->normalizer->supportsDenormalization($data, $type, $format);
    }

    public function normalize($object, $format = null, array $context = [])
    {
        return $this->normalizer->normalize($object, $format, $context);
    }

    public function supportsNormalization($data, $format = null)
    {
        return $this->normalizer->supportsNormalization($data, $format);
    }
}

Which is registered such as follow in services.yaml :

services:

    'App\Serializer\CustomItemNormalizer':
        arguments: [ '@api_platform.serializer.normalizer.item' ]

Toflar added 3 commits June 13, 2018 14:18

Refactored the way the object to populate is set during denormalization

1a604e6

Restore behat tests

268168c

Check if identifiers are writeable

7a72bab

bendavies changed the title ~~Fix allow creating resources with custom idenfifers~~ Fix allow creating resources with custom identifers Jun 13, 2018

bendavies added 5 commits June 13, 2018 23:35

add feature to test writing identifiers

4daf9a0

needs to be passed by reference

4020d67

fix incorrect assign

09f0de6

writability of identifiers cannot be checked like this, as identifier…

cb6106d

…s can be used to identify nested relations

only @id is used for json ld

01a100f

soyuka reviewed Jun 14, 2018

View reviewed changes

fix deprecation

710504b

Toflar mentioned this pull request Jun 14, 2018

[RFC] Refactor population of object in serializer #1939

Closed

move json ld fallback to correct item normalizer

a9ea985

soyuka approved these changes Jun 17, 2018

View reviewed changes

fix feature to correctly address #857

6f31412

soyuka requested review from dunglas, teohhanhui and meyerbaptiste June 18, 2018 11:55

Toflar approved these changes Jun 19, 2018

View reviewed changes

meyerbaptiste requested changes Jun 19, 2018

View reviewed changes

meyerbaptiste reviewed Jun 19, 2018

View reviewed changes

dunglas mentioned this pull request Jul 15, 2018

[BUG] Plain Ids not working with JSON #2101

Closed

jamesisaac mentioned this pull request Sep 7, 2018

[GraphQL] Requesting _id without id leads to "cannot return null for non-nullable field" #2194

Closed

soyuka closed this Sep 12, 2018

soyuka mentioned this pull request Sep 12, 2018

Provide an opt-in ItemNormalizer decorator to help with id identified resources #2200

Closed

bendavies mentioned this pull request Sep 12, 2018

Allow writable IDs #2201

Closed

dunglas mentioned this pull request Nov 8, 2018

Plain identifier does not work for an entity #2320

Closed

dunglas mentioned this pull request Feb 6, 2019

Use plain IDs api-platform/api-platform#249

Closed

soyuka mentioned this pull request Feb 15, 2019

Document "allow_plain_identifiers" #2521

Closed

soyuka mentioned this pull request Mar 14, 2019

POST new data by id not URI api-platform/api-platform#1047

Closed

soyuka mentioned this pull request Apr 4, 2019

[RFC] Deal with plain identifiers #2686

Closed

Uh oh!

Fix allow creating resources with custom identifers #2022

Fix allow creating resources with custom identifers #2022

Uh oh!

Conversation

bendavies commented Jun 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

bendavies Jun 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Toflar commented Jun 14, 2018

Uh oh!

bendavies commented Jun 14, 2018

Uh oh!

Toflar commented Jun 14, 2018

Uh oh!

bendavies commented Jun 15, 2018

Uh oh!

soyuka commented Jun 17, 2018

Uh oh!

bendavies commented Jun 17, 2018 via email

Uh oh!

bendavies commented Jun 18, 2018

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bendavies Jun 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

bendavies Jun 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bendavies commented Jun 21, 2018

Uh oh!

teohhanhui commented Jun 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dunglas commented Jun 22, 2018

Uh oh!

teohhanhui commented Jun 22, 2018

Uh oh!

maks-rafalko commented Sep 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bendavies commented Jun 13, 2018 •

edited

Loading

bendavies Jun 14, 2018 •

edited

Loading

bendavies Jun 19, 2018 •

edited

Loading

bendavies Jun 19, 2018 •

edited

Loading

teohhanhui commented Jun 22, 2018 •

edited

Loading

maks-rafalko commented Sep 3, 2018 •

edited

Loading

maks-rafalko commented Sep 6, 2018 •

edited

Loading

bendavies commented Sep 11, 2018 •

edited

Loading