[RFC] Refactor population of object in serializer #1939

Toflar · 2018-05-09T13:53:48Z

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

This is completely untested but I think it should be enough to outline the plan as described in #1937

Basically this would fix two things:

No more hardcoded dependency on id but using the IdentifiersExtractor instead.
It would actually fix Cannot POST id to create new resources #1937 because the serializers now test if the object even exists and only then disallow populating the object.

Looking for feedback here :-)

Toflar · 2018-05-09T13:54:38Z

src/Serializer/AbstractItemNormalizer.php

+                $identifiers = $this->identifiersExtractor->getIdentifiersFromResourceClass($class);
+            }
+
+            if (0 !== count(array_intersect(array_keys($data), $identifiers))) {


I have no clue if this works, it was just a quick hack and needs to be covered by tests. The idea here is just to check if the data contains all identifiers.

Toflar · 2018-05-09T13:57:53Z

src/Serializer/ItemNormalizer.php

-    private function updateObjectToPopulate(array $data, array &$context)
-    {
-        try {
-            $context[self::OBJECT_TO_POPULATE] = $this->iriConverter->getItemFromIri((string) $data['id'], $context + ['fetch_data' => true]);


This whole method was nonsense imho. Looks like it was developed before there was the IdentifiersExtractor.

Looks like it was developed before there was the IdentifiersExtractor.

It's the case.

You should add a deprecated error here saying this class is deprecated, and "unabstract" the AbstractItemNormalizer

I don't think that's a good plan. I'd keep it as is because the AbstractItemNormalizer is not used directly anywhere now and maybe there will be the day we have to add something to the ItemNormalizer. It can still be refactored for 3.0 even if there's no deprecation message.

antograssiot · 2018-05-09T14:51:48Z

src/Serializer/AbstractItemNormalizer.php

+     */
+    protected function getIdentifiersForDenormalization(string $class): array
+    {
+        return $this->identifiersExtractor->getIdentifiersFromResourceClass($class);


$identifierExtractor is nullable so you can't call a metod on it directly

Taluu · 2018-05-09T15:03:49Z

src/Serializer/AbstractItemNormalizer.php

@@ -65,6 +67,12 @@ public function __construct(PropertyNameCollectionFactoryInterface $propertyName
        $this->setCircularReferenceHandler(function ($object) {
            return $this->iriConverter->getIriFromItem($object);
        });
+
+        if (null === $identifiersExtractor) {
+            @trigger_error(sprintf('Not passing an instance of IdentifiersExtractorInterface in "%s" is deprecated and will be mandatory in version 3.0. Not passing it is deprecated since 2.2.', __METHOD__), E_USER_DEPRECATED);


2.3 nay ? Kinda strange to introduce a deprecated on > 2.2.0...

Not really, not being able to pass an id in a POST request is a bug to me but I don't care as long as it's published soon it can also go into 2.3 😄 Deprecating in bugfix releases is fine.

src/Serializer/AbstractItemNormalizer.php

+
+                $context[self::OBJECT_TO_POPULATE] = $object;
+            }
+        }


soyuka · 2018-05-09T15:38:54Z

src/Serializer/AbstractItemNormalizer.php

+
+        $identifiers = $this->getIdentifiersForDenormalization($class);
+
+        if (0 !== count(array_intersect(array_keys($data), $identifiers))) {


I'm really not sure about this but maybe that performances would be better with a simple foreach test:

foreach ($identifiers as $identifier) { if (!isset($data[$identifier])) { return false; } } return true;

Also it should be \count

Can be simplified to if (\count(array_intersect(array_keys($data), $identifiers))

soyuka · 2018-05-09T15:39:26Z

src/Serializer/AbstractItemNormalizer.php

+     * @param array $context
+     * @param array $identifiers
+     */
+    protected function populateObjectIfAllowed($data, $class, array $context = [])


can't we type the $class as string?

Toflar · 2018-05-10T06:51:05Z

Hey guys, this PR is not ready to review at all 😄 I mean, I know there's loads of stuff to cleanup, null pointer exceptions etc. but all I wanted to know is if you think the general concept could work and it's worth continuing into this direction or if I'm completely wrong. What I'm e.g. missing is the use of the IriConverter. What's the idea there. Asking the IriConverter first if there's a result and if not, asking the ItemDataProvider so both is supported?

soyuka · 2018-05-11T08:35:48Z

The ItemDataProvider shouldn't be useful here. IMO IriConverter should be sufficient and we would need to merge #1837 before being able to fix this correctly.

Toflar · 2018-05-11T08:45:36Z

@soyuka do you mean like so?

soyuka · 2018-05-14T10:16:56Z

@Toflar yes this is really good!

Toflar · 2018-05-14T17:07:36Z

Okay, phpunit is fixed. Behat left 😓

Taluu · 2018-05-14T17:11:53Z

Sorry, couldn't help it. :D

soyuka · 2018-05-14T17:16:58Z

Don't you run tests locally?

Toflar · 2018-05-14T17:19:43Z

Sure but as it‘s squashed anyway I keep my motivation high by making smaller nonsense commits🤔😂

Toflar · 2018-05-15T10:13:02Z

Okay ~~guys~~folks, this is as far as I could get. It's finished. But some behat tests are left to fix which I can't get done because they are not isolated and it's mostly database schema errors. I'll leave it here again for the record: It's a horrible idea not to isolate and create a new schema for every test. You choose performance over DX.

Toflar · 2018-05-15T10:45:57Z

@soyuka will try to help with the behat tests. I've restored them to the initial state as per his request.

bendavies · 2018-06-13T21:24:12Z

@Toflar can you rebase on 2.2 and see how the tests look. I'm working on this and can PR to your branch. I'll raise some comments here.

bendavies · 2018-06-13T21:24:48Z

src/Serializer/AbstractItemNormalizer.php

+            return;
+        }
+
+        $updateAllowed = isset($context['api_allow_update']) && true !== $context['api_allow_update'];


should be === not !== or just:
$updateAllowed = $context['api_allow_update'] ?? false;

bendavies · 2018-06-13T21:25:19Z

src/Serializer/AbstractItemNormalizer.php

+            return;
+        }
+
+        if (null !== $object && $updateAllowed) {


should be !$updateAllowed

bendavies · 2018-06-13T21:25:46Z

src/Serializer/AbstractItemNormalizer.php

+     * @param array  $context
+     * @param array  $identifiers
+     */
+    protected function setObjectToPopulate($data, $class, array $context = [])


you need to pass $context by reference as you are modifying it.

bendavies · 2018-06-13T21:27:54Z

src/Serializer/AbstractItemNormalizer.php

+
+        $updateAllowed = isset($context['api_allow_update']) && true !== $context['api_allow_update'];
+        $identifiers = $this->getIdentifiersForDenormalization($class);
+        $writeableIdentifiers = [];


this logic is flawed unfortunately. You cannot validate the writability of identifiers here, as identifiers can be used to reference nested relations, to update them. example:
https://github.com/api-platform/core/blob/master/features/hal/max_depth.feature#L58

summary. having an identifier in the data does not mean it is trying to be updated.

bendavies · 2018-06-13T21:55:29Z

src/Serializer/AbstractItemNormalizer.php

+        $iri = implode(';', $identifiersData); // TODO: use $this->iriConverter->getIriFromPlainIdentifier() once https://github.com/api-platform/core/pull/1837 got merged.
+
+        try {
+            $object = $this->iriConverter->getItemFromIri($iri, $context + ['fetch_data' => true]);


exceptions thrown here should not be caught. the old code did not catch.

bendavies · 2018-06-13T22:05:54Z

src/Serializer/ItemNormalizer.php

-    {
-        try {
-            $context[self::OBJECT_TO_POPULATE] = $this->iriConverter->getItemFromIri((string) $data['id'], $context + ['fetch_data' => true]);
-        } catch (InvalidArgumentException $e) {


this has to be retained, it fixes #857

bendavies · 2018-06-13T22:13:46Z

I have all the tests passing. I'll PR in the morning.

Toflar · 2018-06-14T08:53:36Z

Closing in favor of #2022.

Toflar commented May 9, 2018

View reviewed changes

Toflar mentioned this pull request May 9, 2018

Cannot POST id to create new resources #1937

Closed

Toflar commented May 9, 2018

View reviewed changes

antograssiot reviewed May 9, 2018

View reviewed changes

Taluu reviewed May 9, 2018

View reviewed changes

soyuka reviewed May 9, 2018

View reviewed changes

src/Serializer/AbstractItemNormalizer.php Outdated

$context[self::OBJECT_TO_POPULATE] = $object;

}

}

This comment was marked as resolved.

Sign in to view

soyuka reviewed May 9, 2018

View reviewed changes

Toflar force-pushed the fix-allow-post-id branch from e63641a to afa1873 Compare May 14, 2018 15:20

Toflar force-pushed the fix-allow-post-id branch from a0f7b28 to c6a6911 Compare May 15, 2018 09:40

Refactored the way the object to populate is set during denormalization

55b1850

Toflar force-pushed the fix-allow-post-id branch from c6a6911 to 55b1850 Compare May 15, 2018 09:43

Toflar changed the title ~~[WIP] Refactor population of object in serializer~~ [RFC] Refactor population of object in serializer May 15, 2018

Restore behat tests

4d0f659

Toflar force-pushed the fix-allow-post-id branch from 3c9cc89 to 4d0f659 Compare May 15, 2018 10:43

soyuka mentioned this pull request May 15, 2018

How to update instead of create a child entity that is not a @ApiResource nor a @ApiSubresource #1950

Closed

Check if identifiers are writeable

1c9302f

bendavies mentioned this pull request Jun 13, 2018

ID not writable when creating resource #2018

Closed

bendavies suggested changes Jun 13, 2018

View reviewed changes

bendavies reviewed Jun 13, 2018

View reviewed changes

bendavies mentioned this pull request Jun 13, 2018

Fix allow creating resources with custom identifers #2022

Closed

Toflar closed this Jun 14, 2018

Toflar deleted the fix-allow-post-id branch December 18, 2018 12:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC] Refactor population of object in serializer #1939

[RFC] Refactor population of object in serializer #1939

Toflar commented May 9, 2018

Toflar May 9, 2018

Toflar May 9, 2018

dunglas May 9, 2018

Taluu May 9, 2018

Toflar May 11, 2018

antograssiot May 9, 2018

Taluu May 9, 2018

Toflar May 11, 2018 •

edited

Loading

This comment was marked as resolved.

soyuka May 9, 2018

soyuka May 9, 2018

dunglas May 9, 2018

soyuka May 9, 2018

Toflar commented May 10, 2018

soyuka commented May 11, 2018

Toflar commented May 11, 2018

soyuka commented May 14, 2018

Toflar commented May 14, 2018

Taluu commented May 14, 2018

soyuka commented May 14, 2018

Toflar commented May 14, 2018

Toflar commented May 15, 2018 •

edited

Loading

Toflar commented May 15, 2018

bendavies commented Jun 13, 2018

bendavies Jun 13, 2018 •

edited

Loading

bendavies Jun 13, 2018

bendavies Jun 13, 2018

bendavies Jun 13, 2018

bendavies Jun 13, 2018

bendavies Jun 13, 2018

bendavies commented Jun 13, 2018

Toflar commented Jun 14, 2018


		$identifiers = $this->getIdentifiersForDenormalization($class);

		if (0 !== count(array_intersect(array_keys($data), $identifiers))) {

[RFC] Refactor population of object in serializer #1939

[RFC] Refactor population of object in serializer #1939

Conversation

Toflar commented May 9, 2018

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

Toflar May 11, 2018 • edited Loading

Choose a reason for hiding this comment

This comment was marked as resolved.

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

Toflar commented May 10, 2018

soyuka commented May 11, 2018

Toflar commented May 11, 2018

soyuka commented May 14, 2018

Toflar commented May 14, 2018

Taluu commented May 14, 2018

soyuka commented May 14, 2018

Toflar commented May 14, 2018

Toflar commented May 15, 2018 • edited Loading

Toflar commented May 15, 2018

bendavies commented Jun 13, 2018

bendavies Jun 13, 2018 • edited Loading

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

bendavies commented Jun 13, 2018

Toflar commented Jun 14, 2018

Toflar May 11, 2018 •

edited

Loading

Toflar commented May 15, 2018 •

edited

Loading

bendavies Jun 13, 2018 •

edited

Loading