fix #857 - fix on put when both @id and id are present #877

Simperfit · 2016-12-10T11:28:13Z

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

Add an non-regression test and modify the comportement of itemNormalizer

dunglas · 2016-12-10T12:43:23Z

src/Serializer/ItemNormalizer.php

@@ -28,12 +28,12 @@
    public function denormalize($data, $class, $format = null, array $context = [])
    {
        // Avoid issues with proxies if we populated the object
-        if (isset($data['id']) && !isset($context['object_to_populate'])) {
+        if (isset($data['@id']) && !isset($context['object_to_populate'])) {


This snippet already exists in the JSON-LD normalizer: https://github.com/api-platform/core/blob/master/src/JsonLd/Serializer/ItemNormalizer.php#L93

@id is specific to JSON-LD, so it should not be here. However id can contain an IRI too (when not using JSON-LD). So it should be kept. I guess it's a priority problem, in https://github.com/api-platform/core/blob/master/src/JsonLd/Serializer/ItemNormalizer.php#L101 the call to the parent method should be done before.

Simperfit · 2016-12-10T16:20:47Z

@dunglas This is now working but i'm not sure about the fix, since I don't think it's in the same scope then the iriConverter is, clearly this is not an IRI, do you have any thing in mind that would be better ?

EDIT: it's not the right fix ;)

Simperfit · 2016-12-10T19:55:08Z

Again, maybe not the best fixes, but better than before since it's working and it does not break anything.

dunglas · 2016-12-13T16:23:02Z

src/Serializer/ItemNormalizer.php

@@ -32,8 +32,7 @@ public function denormalize($data, $class, $format = null, array $context = [])
            if (isset($context['api_allow_update']) && true !== $context['api_allow_update']) {
                throw new InvalidArgumentException('Update is not allowed for this operation.');
            }
-
-            $context['object_to_populate'] = $this->iriConverter->getItemFromIri($data['id'], $context + ['fetch_data' => false]);
+            $context['object_to_populate'] = $this->iriConverter->getItemFromIri(is_int($data['id']) ? $this->iriConverter->getIriFromResourceClass($context['resource_class']).'/'.$data['id'] : $data['id'], $context + ['fetch_data' => false]);


id can be from other types (uuid are strings for instance).

What could I do about that ?

Edit: Was thinking about try catching and doing the getIriFromResourceClass, WDYT @dunglas ?

dunglas

When test will be green.

theofidry

See my comments for some nitpicking changes, 👍 overall :)

theofidry · 2016-12-19T19:31:28Z

tests/Fixtures/TestBundle/Entity/RelatedNormalizedDummy.php

+ * @ApiResource(attributes={
+ *     "normalization_context"={"groups"={"related_output", "output"}},
+ *     "denormalization_context"={"groups"={"related_input", "input"}}
+ *


useless extra line

theofidry · 2016-12-19T19:31:51Z

tests/Fixtures/TestBundle/Entity/RelatedNormalizedDummy.php

+    /**
+     * @return int
+     */
+    public function getId()


wouldn't a return typehint more suitable here and the other places in the same class?

teohhanhui · 2016-12-20T03:34:28Z

features/main/custom_normalized.feature

      "name": "My Dummy",
      "alias": "My alias"
    }
    """

+  Scenario: Create a resource with relation


Please fix the scenario summary (same for all below)...

teohhanhui · 2016-12-20T03:36:52Z

The fix is simple: We must not support embedded relations with application/json, because the semantics are undefined. We have had this discussion before on Slack, and I thought a decision was made...

So I'm -1 since this is the wrong fix.

/cc @api-platform/core-team

dunglas · 2016-12-20T09:12:06Z

@teohhanhui the issue here isn't with embedded relations but using raw ids instead of IRIs in raw JSON. Not sure we should support that too.

teohhanhui · 2016-12-20T11:37:46Z

What is the issue with deserializing using a raw id property (or composite id represented as separate properties)? I think it should be supported.

Simperfit · 2016-12-22T09:50:01Z

@teohhanhui updated

teohhanhui · 2016-12-27T04:47:03Z

src/Serializer/ItemNormalizer.php

+            try {
+                $context['object_to_populate'] = $this->iriConverter->getItemFromIri($data['id'], $context + ['fetch_data' => false]);
+            } catch (InvalidArgumentException $e) {
+                $context['object_to_populate'] = $this->iriConverter->getItemFromIri($this->iriConverter->getIriFromResourceClass($context['resource_class']).'/'.$data['id'], $context + ['fetch_data' => false]);


How is this going to work with composite identifiers?

And also this is hardcoded to use the property "id"? 😢

@teohhanhui @dunglas do we want to support composite identifiers ?

Simperfit · 2017-01-03T08:56:05Z

ping @teohhanhui @dunglas modification done, WDYT ?

dunglas · 2017-01-03T10:02:34Z

src/Serializer/ItemNormalizer.php

+                $context['object_to_populate'] = $this->iriConverter->getItemFromIri($data['id'], $context + ['fetch_data' => false]);
+            } catch (InvalidArgumentException $e) {
+                foreach ($this->propertyNameCollectionFactory->create($context['resource_class'], $context) as $propertyName) {
+                    if (true === $this->propertyMetadataFactory->create($context['resource_class'], $propertyName)->isIdentifier()) {


If we don't support yet composite identifiers, you can break after the first find. Or you can add support for composite identifiers :)

+1 for the composite support :D

I think for now we should bug fix this and then add that feature ?

dunglas · 2017-01-03T10:04:10Z

src/Serializer/ItemNormalizer.php

+                        $identifier = $propertyName;
+                    }
+                }
+                $context['object_to_populate'] = $this->iriConverter->getItemFromIri($this->iriConverter->getIriFromResourceClass($context['resource_class']).'/'.$data[$identifier], $context + ['fetch_data' => false]);


I would use sprintf here for clarity.

dunglas · 2017-01-25T09:51:57Z

@api-platform/core-team is everyone OK to merge this one too? I want to publish a bugfix release today.

dunglas · 2017-01-25T13:47:23Z

Thanks @Simperfit!

@id

fix api-platform#857 - fix on put when both @id and id are present

Simperfit force-pushed the fix/857-embedded branch from c84c974 to 1caa29c Compare December 10, 2016 11:29

dunglas requested changes Dec 10, 2016

View reviewed changes

Simperfit force-pushed the fix/857-embedded branch from 769085e to d17eb1f Compare December 10, 2016 14:38

Simperfit force-pushed the fix/857-embedded branch from 4962c1f to ae817d5 Compare December 10, 2016 16:23

Simperfit changed the title ~~fix #857 - fix on put when both @id and id are present~~ [WIP] fix #857 - fix on put when both @id and id are present Dec 10, 2016

Simperfit force-pushed the fix/857-embedded branch from 9c730f2 to d644132 Compare December 10, 2016 19:52

Simperfit changed the title ~~[WIP] fix #857 - fix on put when both @id and id are present~~ fix #857 - fix on put when both @id and id are present Dec 10, 2016

dunglas requested changes Dec 13, 2016

View reviewed changes

Simperfit force-pushed the fix/857-embedded branch from 4016c69 to 00b4d93 Compare December 19, 2016 13:42

dunglas approved these changes Dec 19, 2016

View reviewed changes

Simperfit force-pushed the fix/857-embedded branch from 00b4d93 to 3cfd553 Compare December 19, 2016 15:53

Simperfit requested review from teohhanhui and theofidry December 19, 2016 17:10

theofidry approved these changes Dec 19, 2016

View reviewed changes

teohhanhui suggested changes Dec 20, 2016

View reviewed changes

Simperfit force-pushed the fix/857-embedded branch from 3cfd553 to 6244be0 Compare December 22, 2016 09:49

dunglas added the bug label Dec 23, 2016

teohhanhui suggested changes Dec 27, 2016

View reviewed changes

Simperfit force-pushed the fix/857-embedded branch from 5402a00 to a76f79f Compare December 27, 2016 18:48

dunglas requested changes Jan 3, 2017

View reviewed changes

Simperfit force-pushed the fix/857-embedded branch from c1a931a to 0cb7e08 Compare January 7, 2017 17:20

soyuka approved these changes Jan 25, 2017

View reviewed changes

fix api-platform#857 - fix on put when both @id and id are present

137a36e

dunglas force-pushed the fix/857-embedded branch from 0cb7e08 to 137a36e Compare January 25, 2017 13:41

dunglas approved these changes Jan 25, 2017

View reviewed changes

dunglas merged commit 01303fa into api-platform:2.0 Jan 25, 2017

Simperfit deleted the fix/857-embedded branch January 25, 2017 15:01

magarzon pushed a commit to magarzon/core that referenced this pull request Feb 12, 2017

Merge pull request api-platform#877 from Simperfit/fix/857-embedded

6f32537

fix api-platform#857 - fix on put when both @id and id are present

teohhanhui mentioned this pull request Feb 16, 2017

Deserialization bug on background job (IriConverter) #946

Closed

teohhanhui mentioned this pull request Mar 9, 2017

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

Closed

bendavies mentioned this pull request Jun 19, 2018

Fix allow creating resources with custom identifers #2022

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix #857 - fix on put when both @id and id are present #877

fix #857 - fix on put when both @id and id are present #877

Simperfit commented Dec 10, 2016

dunglas Dec 10, 2016 •

edited

Loading

Simperfit commented Dec 10, 2016 •

edited

Loading

Simperfit commented Dec 10, 2016

dunglas Dec 13, 2016

Simperfit Dec 13, 2016 •

edited

Loading

dunglas left a comment

theofidry left a comment

theofidry Dec 19, 2016

theofidry Dec 19, 2016

teohhanhui Dec 20, 2016

teohhanhui commented Dec 20, 2016 •

edited

Loading

dunglas commented Dec 20, 2016

teohhanhui commented Dec 20, 2016 •

edited

Loading

Simperfit commented Dec 22, 2016

teohhanhui Dec 27, 2016

teohhanhui Dec 27, 2016

Simperfit Dec 27, 2016

Simperfit commented Jan 3, 2017

dunglas Jan 3, 2017

soyuka Jan 3, 2017

Simperfit Jan 7, 2017

dunglas Jan 3, 2017

dunglas commented Jan 25, 2017

dunglas commented Jan 25, 2017

fix #857 - fix on put when both @id and id are present #877

fix #857 - fix on put when both @id and id are present #877

Conversation

Simperfit commented Dec 10, 2016

dunglas Dec 10, 2016 • edited Loading

Choose a reason for hiding this comment

Simperfit commented Dec 10, 2016 • edited Loading

Simperfit commented Dec 10, 2016

Choose a reason for hiding this comment

Simperfit Dec 13, 2016 • edited Loading

Choose a reason for hiding this comment

dunglas left a comment

Choose a reason for hiding this comment

theofidry left a 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

teohhanhui commented Dec 20, 2016 • edited Loading

dunglas commented Dec 20, 2016

teohhanhui commented Dec 20, 2016 • edited Loading

Simperfit commented Dec 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Simperfit commented Jan 3, 2017

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

dunglas commented Jan 25, 2017

dunglas commented Jan 25, 2017

dunglas Dec 10, 2016 •

edited

Loading

Simperfit commented Dec 10, 2016 •

edited

Loading

Simperfit Dec 13, 2016 •

edited

Loading

teohhanhui commented Dec 20, 2016 •

edited

Loading

teohhanhui commented Dec 20, 2016 •

edited

Loading