Improve ":id" param normalization by soyuka · Pull Request #1478 · api-platform/core

soyuka · 2017-11-03T16:59:12Z

Q	A
Bug fix?	yes
New feature?	yes/no (depends)
BC breaks?	yes
Deprecations?	yes
Tests pass?	not yet
Fixed tickets	#1309 #1476 and maybe other (#1376 start) #1234 #1309
License	MIT
Doc PR	n/a

Issue

Currently, as I mentioned in #1309, our DataProviders get the raw id param coming from the request. ie

core/src/EventListener/ReadListener.php

Line 103 in 1835269

$id = $request->attributes->get('id');

There is an issue here where we sometimes want our DataProviders to get a correct instance of the given identifier (eg Ramsey\UUID for an uuid or \DateTime for a date).

What's being done

This is what I've come up with. Basically it adds a IdentifierNormalizer class (does not implement the NormalizerInterface because it's not one) that takes the string from $request->attributes->get('id') and returns an array with correct Types.
This is not working yet because I have a train to catch haha. Going to add tests and the ability to add normalizers asap (those are NormalizerInterface, for example DateTimeNormalizer etc.)!

There is a BC break but I've added deprecation and my goal is to keep everything working as before by getting rid of the IdentifierManagerTrait in future versions.

Benefits

With this fix the identifier normalization isn't linked to Doctrine anymore. This is a great improvement for future DataProviders that might need the correct types without forcing the developer to use Doctrine. Also, it allows the developper to add custom normalizers for identifiers (for example the UUID mentioned above).
By having a cleaner way to parse those, it'll also help us manage errors and throw a NotFoundException when needed (mentioned by @theofidry in #1309).
Last but not least, I've improved the composite identifier parser as for now something like a=test;b=bar ;foo;c=123;459; would totally fail because of the ; in the identifier value (nb try with =).

ping @dkarlovi

mickaelandrieu · 2017-11-03T17:38:45Z

+    }
+}
+
+// $id = new IdentifierNormalizer();


to be removed, isn't it? :)

soyuka · 2017-11-03T20:17:42Z

@mickaelandrieu haha of course sorry!

PR updated with tests, a compiler pass, correct deprecation, BC break coverage

I just need to update the tests for the deprecation and open a doc PR!

Let me know your thoughts @api-platform/core-team !

soyuka · 2017-11-03T20:21:11Z

+
+        <service id="api_platform.identifier.date_normalizer" class="Symfony\Component\Serializer\Normalizer\DateTimeNormalizer" public="false">
+            <tag name="api_platform.identifier_normalizer" />
+        </service>


I'm not sure that this is allowed, lmk if there's something better to use instead.

What about adding uuid normalizer? It is very common I think.

added below :) Just need to test it!

Why would not it be?

dkarlovi · 2017-11-04T10:22:45Z

This looks great @soyuka!

About the "Not found" when un-normalizable (that's actually a word, or my Chrome spell-checker is asleep), maybe introducing a new Exception will enable the desired behaviour, it can be hooked up to a 404 status by default.

soyuka · 2017-11-04T12:15:45Z

TODO:

Test uuid denormalizer
Improve deprecations

soyuka · 2017-11-04T12:18:29Z

     * @throws RuntimeException
     */
-    public function getItem(string $resourceClass, $id, string $operationName = null, array $context = [])
+    public function getItem(string $resourceClass, $id, string $operationName = null, array $context = []/** , array $identifiers **/)


This was the only solution I had to avoid breaking the DataProvider::getItem signature. Also, in future versions $identifier should replace $id and it will be typed as array.

soyuka · 2017-11-04T17:00:28Z

About the "Not found" when un-normalizable (that's actually a word, or my Chrome spell-checker is asleep), maybe introducing a new Exception will enable the desired behaviour, it can be hooked up to a 404 status by default.

It's the behavior now see https://github.com/api-platform/core/pull/1478/files#diff-31d120dbe9271ce5d3bd5dfd939121a7R94

@norkunas note that you'll have to setup ramsey/uuid-doctrine for things to work correctly.

norkunas · 2017-11-04T18:38:21Z

Yes, I'm using it already;)

soyuka · 2017-11-09T11:29:09Z

@norkunas could you try this branch to see if it fixes your issue? I think that my implementation is as far as it will get :).

norkunas · 2017-11-10T07:48:04Z

I'll try this evening.

dkarlovi · 2017-11-15T09:42:25Z

@soyuka I'm testing it now.

soyuka · 2018-04-04T16:41:24Z

Thanks for the review I've refactored a bit:

uses DenormalizeInterface on identifier normalizers
CompositeIdentifierNormalizer is now CompositeIdentifierParser
tests are green (locally at least I need to debug travis :|)
fixed IriConverter where the flag was not set

About the deprecation message, IMO we can remove it as it's internal an no-one really uses it? We can't remove the trait because the ItemDataProvider would require $id to be an array, for example:

['id' => 1] instead of 1
['id' => 1, 'foo' => 2] instead of id=1;foo=2

In fact, if we could break the DataProvider signature the best would be to type $id to an array.

dunglas · 2018-04-04T18:06:20Z

About the deprecation message, IMO we can remove it as it's internal an no-one really uses it?

👍

soyuka · 2018-04-05T07:47:20Z

Any ideas for the deps=low test suite? I used the tagged thing to avoid using a compiler pass but looks like there's something missing in 3.4: Error: Class 'Symfony\Component\DependencyInjection\Argument\ClosureProxyArgument' not found

bendavies · 2018-04-05T08:12:59Z

@soyuka symfony/event-dispatcher v3.3.0 is being installed and using Symfony\Component\DependencyInjection\Argument\ClosureProxyArgument but symfony/dependency-injection v3.4.0 is installed where it doesn't exist any more.

bendavies · 2018-04-05T08:16:24Z

I think the options are,

restore symfony/dependency-injection to ^3.3..., restore the compiler pass and dropped the tagged stuff.
raise symfony/event-dispatcher low to ^3.4
raise all low deps to ^3.4

soyuka · 2018-04-05T15:28:48Z

Green 😄

dunglas · 2018-04-06T09:51:46Z

Just great! Thanks @soyuka

redecs · 2018-04-06T09:53:30Z

Thank you all for the effort put into this. It's been an issue bugging me for a while now when trying to work with UUIDs (v4).

dkarlovi · 2018-04-06T09:55:59Z

\o/

meyerbaptiste

You merged while I was doing my review 😭

meyerbaptiste · 2018-04-06T09:12:36Z

+        try {
+            return Uuid::fromString($data);
+        } catch (InvalidUuidStringException $e) {
+            throw new InvalidIdentifierException($e->getMessage());


throw new InvalidIdentifierException($e->getMessage(), $e->getCode(), $e);?

meyerbaptiste · 2018-04-06T09:19:46Z

+            <tag name="api_platform.identifier.normalizer" />
+        </service>
+
+        <service id="api_platform.identifier.uuid_normalizer" class="ApiPlatform\Core\Bridge\RamseyUuid\Identifier\Normalizer\UuidNormalizer" public="false">


Why not putting this definition in a separate file (e.g. ramsey_uuid.xml)? And then use LoaderInterface::load() instead of ContainerBuilder::removeDefinition().

meyerbaptiste · 2018-04-06T09:21:50Z

    private $routeNameResolver;
    private $router;
    private $identifiersExtractor;
+    private $identifierNormalizer;


$identifierDenormalizer (same for the other classes)?

meyerbaptiste · 2018-04-06T09:26:24Z

        $this->itemDataProvider = $itemDataProvider;
        $this->routeNameResolver = $routeNameResolver;
        $this->router = $router;
+        $this->identifierNormalizer = $identifierNormalizer;


Trigger error if null is $identifierNormalizer?

nope we check below we don't want to break this class.

meyerbaptiste · 2018-04-06T09:46:53Z

                $identifiers[$id] = $request->attributes->get($id);
+
+                if ($this->identifierNormalizer) {
+                    $identifiers[$id] = $this->identifierNormalizer->denormalize((string) $identifiers[$id], $resourceClass);


The try-catch should be here, isn't it?

meyerbaptiste · 2018-04-06T11:08:58Z

+        foreach ($keys as $key) {
+            foreach ($this->identifierNormalizers as $normalizer) {
+                if (!isset($identifiers[$key])) {
+                    throw new InvalidIdentifierException(sprintf('Invalid identifier "%s", "%s" was not found.', $key, $key));


sprintf('... %1$s ... %1$s ...', $key)

meyerbaptiste · 2018-04-06T11:17:28Z

+    private function getIdentifierMetadata($class, $propertyName)
+    {
+        $propertyMetadata = $this->propertyMetadataFactory->create($class, $propertyName);
+        $type = $propertyMetadata->getType();


$propertyMetadata var is useless:

$type = $this->propertyMetadataFactory->create($class, $propertyName)->getType();

meyerbaptiste · 2018-04-06T11:22:19Z

+        // Normalize every identifier (DateTime, UUID etc.)
+        foreach ($keys as $key) {
+            foreach ($this->identifierNormalizers as $normalizer) {
+                if (!isset($identifiers[$key])) {


Could not this condition be moved in the first loop?

meyerbaptiste · 2018-04-06T11:24:48Z

+        try {
+            return parent::denormalize($data, $class, $format, $context);
+        } catch (InvalidArgumentException $e) {
+            throw new InvalidIdentifierException($e->getMessage());


You should populate the previous exception argument.

meyerbaptiste · 2018-04-06T11:27:23Z

  - echo "memory_limit=-1" >> ~/.phpenv/versions/$(phpenv version-name)/etc/conf.d/travis.ini
  - npm install -g swagger-cli
-  - if [[ $lint = 1 ]]; then wget https://github.com/FriendsOfPHP/PHP-CS-Fixer/releases/download/v2.8.2/php-cs-fixer.phar; fi
+  - if [[ $lint = 1 ]]; then wget https://github.com/FriendsOfPHP/PHP-CS-Fixer/releases/download/v2.8.4/php-cs-fixer.phar; fi


I think 2.10.0 should work!

mickaelandrieu reviewed Nov 3, 2017

View reviewed changes

soyuka force-pushed the fix/identifier-normalizer-1309 branch from e2bd783 to c4a6523 Compare November 3, 2017 20:10

soyuka changed the title ~~[WIP] Improve ":id" param normalization~~ Improve ":id" param normalization Nov 3, 2017

soyuka added the [RFR] Ready for Review label Nov 3, 2017

soyuka force-pushed the fix/identifier-normalizer-1309 branch from c4a6523 to 1a2ba53 Compare November 3, 2017 20:19

soyuka commented Nov 3, 2017

View reviewed changes

soyuka force-pushed the fix/identifier-normalizer-1309 branch from 1a2ba53 to 5681d08 Compare November 3, 2017 22:04

norkunas mentioned this pull request Nov 4, 2017

Catch dbal conversion exception #1476

Closed

soyuka removed the [RFR] Ready for Review label Nov 4, 2017

soyuka force-pushed the fix/identifier-normalizer-1309 branch from 5681d08 to fd3ee1b Compare November 4, 2017 12:15

soyuka commented Nov 4, 2017

View reviewed changes

soyuka force-pushed the fix/identifier-normalizer-1309 branch 3 times, most recently from abf574d to f0be548 Compare November 4, 2017 16:57

soyuka force-pushed the fix/identifier-normalizer-1309 branch from f0be548 to 5f3078d Compare November 4, 2017 17:05

soyuka added the [RFR] Ready for Review label Nov 4, 2017

soyuka force-pushed the fix/identifier-normalizer-1309 branch from 5f3078d to a3c4705 Compare November 4, 2017 17:38

soyuka force-pushed the fix/identifier-normalizer-1309 branch 5 times, most recently from 5727ba6 to a090ddb Compare November 9, 2017 11:28

soyuka force-pushed the fix/identifier-normalizer-1309 branch from 150c586 to 9bbeab6 Compare April 4, 2018 16:41

soyuka force-pushed the fix/identifier-normalizer-1309 branch 4 times, most recently from 2356798 to 5f2c95f Compare April 5, 2018 07:44

soyuka force-pushed the fix/identifier-normalizer-1309 branch from 5f2c95f to 73c0baa Compare April 5, 2018 09:02

soyuka mentioned this pull request Apr 5, 2018

Bump requirements to use tagged services #1813

Merged

bump php-cs-fixer to allow self-accessor

89deb76

soyuka force-pushed the fix/identifier-normalizer-1309 branch from 73c0baa to 6a9320b Compare April 5, 2018 14:50

soyuka force-pushed the fix/identifier-normalizer-1309 branch from 6a9320b to 88a70c3 Compare April 5, 2018 20:13

Implement identifier normalizer

d4aa84f

soyuka force-pushed the fix/identifier-normalizer-1309 branch from 88a70c3 to d4aa84f Compare April 6, 2018 08:02

Simperfit approved these changes Apr 6, 2018

View reviewed changes

dunglas approved these changes Apr 6, 2018

View reviewed changes

dunglas merged commit cef2464 into api-platform:master Apr 6, 2018

soyuka deleted the fix/identifier-normalizer-1309 branch April 6, 2018 09:58

meyerbaptiste reviewed Apr 6, 2018

View reviewed changes

soyuka restored the fix/identifier-normalizer-1309 branch April 6, 2018 12:04

This was referenced Apr 6, 2018

throw NotFound exception when use plain identifiers #1716

Closed

feature: allow plain identifiers in json api #1834

Closed

cve mentioned this pull request May 16, 2018

Add an option for skip type conversion #1234

Closed

Uh oh!

Uh oh!

Conversation

soyuka commented Nov 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue

What's being done

Benefits

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soyuka commented Nov 3, 2017

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!

dkarlovi commented Nov 4, 2017

Uh oh!

soyuka commented Nov 4, 2017 • 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!

soyuka commented Nov 4, 2017

Uh oh!

norkunas commented Nov 4, 2017

Uh oh!

soyuka commented Nov 9, 2017

Uh oh!

norkunas commented Nov 10, 2017

Uh oh!

dkarlovi commented Nov 15, 2017

Uh oh!

soyuka commented Apr 4, 2018

Uh oh!

dunglas commented Apr 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

soyuka commented Apr 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bendavies commented Apr 5, 2018

Uh oh!

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

Uh oh!

Uh oh!

soyuka commented Apr 5, 2018

Uh oh!

dunglas commented Apr 6, 2018

Uh oh!

redecs commented Apr 6, 2018

Uh oh!

dkarlovi commented Apr 6, 2018

Uh oh!

meyerbaptiste 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!

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!

soyuka commented Nov 3, 2017 •

edited

Loading

soyuka commented Nov 4, 2017 •

edited

Loading

dunglas commented Apr 4, 2018 •

edited

Loading

soyuka commented Apr 5, 2018 •

edited

Loading

bendavies commented Apr 5, 2018 •

edited

Loading