Swagger subresource + OperationPathResolver refactor proposal #1245

soyuka · 2017-07-11T13:44:24Z

Q	A
Bug fix?	not really
New feature?	yes but subresources aren't part of a stable release yet
BC breaks?	maybe, we'll get to that below
Deprecations?	yes
Tests pass?	yes
Fixed tickets	#1239
License	MIT
Doc PR	na

1. `OperationPathResolverInterface`

Okay, after lot of thinking here's my proposal about the whole OperationPathResolver mess.

Initially OperationPathResolver was implemented because some users want to use - instead of _ in their paths. Because of this simple need, we introduced a complicated decoration pattern.

My proposal would be to deprecate the use of the OperationPathResolverInterface if all we want is to use a dash instead of an underscore.

Instead, I've introduced a simple PathNameGenerator who's signature looks like this:

    public function getName(string $name, bool $pluralize = true): string;

This class will be configurable the same way as is the OperationPathResolver but it won't compute paths. Instead, it's only used to name things.

What about UnderscoreOperationPathResolver and DashOperationPathResolver then? They're still working and there's no breaking change here but I think we should deprecate them in favor of a this new "naming service". Indeed, we don't need to generate a path, all we want is to use - or _ to format our path.

2. `SubresourceOperationFactory`

With subresources complexity (recursion, start/end-points) we need to be able to retrieve a Subresource path or route_name from a given ResourceClass with ease. To do so, I've implemented a SubresourceOperationFactory (thanks @Simperfit) that will generate the operations from a given resourceClass.
This is better then using a OperationPathResolver based on OperationType and is less complex to implement.

This new class is currently used :

in ApiLoader to add routes to the symfony router
in Swagger\DocumentationNormalizer to provide swagger docs

Note that Subresources are NOT compatible with OperationPathResolvers! Still, it's using PathNameGenerator and subresources paths could be written with dashes instead of default underscores.

Because the subresource feature is not in the stable release yet I think it's fine. Still, if one wants dashes, he would've to use PathNameGenerator.

Thanks at @meyerbaptiste @Simperfit for the talks around this issue.

@dunglas this refactor is needed for a stable release because the current state of handling subresource paths is too messy IMO.

TODO:

Add tests for SubresourceOperationFactory
Add SubresourceOperationFactoryCache - @teohhanhui finally got a clean "visiting" identifier :D
Validate PathNameGenerator
Make the PathNameGenerator configurable + test
Deprecate DashOperationPathResolver UnderscoreOperationPathResolver
Remove commented tests on subresources and operationpathresolver
PathSegmentGenerator => PathSegmentNameGenerator

Simperfit · 2017-07-12T08:18:36Z

I like that idea ;p

Do you want some help on this ?

Simperfit · 2017-07-12T06:35:18Z

src/Bridge/Symfony/Routing/SubresourceOperationFactory.php

+                continue;
+            }
+
+            $subresource = $propertyMetadata->getSubresource();


subResource

Nope it's subresource everywhere :p.

but it feels awkward

soyuka · 2017-07-12T09:08:32Z

It's almost done, just few tests to add! I just want to validate the PathNameGenerator thing before continuing.

soyuka · 2017-07-12T12:36:22Z

src/Bridge/Symfony/Bundle/DependencyInjection/Configuration.php

@@ -43,8 +43,9 @@ public function getConfigTreeBuilder()
                ->scalarNode('title')->defaultValue('')->info('The title of the API.')->end()
                ->scalarNode('description')->defaultValue('')->info('The description of the API.')->end()
                ->scalarNode('version')->defaultValue('0.0.0')->info('The version of the API.')->end()
-                ->scalarNode('default_operation_path_resolver')->defaultValue('api_platform.operation_path_resolver.underscore')->info('Specify the default operation path resolver to use for generating resources operations path.')->end()
+                ->scalarNode('default_operation_path_resolver')->defaultValue('api_platform.operation_path_resolver.generator')->info('[Deprecated] Specify the default operation path resolver to use for generating resources operations path.')->end()


Is it possible to deprecate a configuration?

soyuka · 2017-07-12T13:02:55Z

src/Bridge/Symfony/Bundle/Resources/config/api.xml

+
+        <service id="api_platform.operation_path_resolver.dash" class="ApiPlatform\Core\PathResolver\DashOperationPathResolver" public="false">
+                <deprecated>The "%service_id%" service is deprecated since ApiPlatform 2.1 and will be removed in 3.0. Use PathNameGenerator instead.</deprecated>
+        </service>


Note that the operation_path_resolver is still valid. I just want to deprecate those:

operation_path_resolver.underscore

operation_path_resolver.dash

soyuka · 2017-07-12T13:09:03Z

Almost there. The current code is backward compatible with the pre-2.1 OperationPathResolver's! Tests will be red because of deprecations (will fix later).

I'd like your help on naming things 🕺 :

is PathNameGenerator fine?
Where should I put SubresourceOperationFactory? I've put it under ApiPlatform\Core\Operation\Factory but maybe you've a better idea

dunglas

Awesome!

dunglas · 2017-07-12T21:08:03Z

src/Bridge/Symfony/Bundle/DependencyInjection/Configuration.php

@@ -43,8 +43,9 @@ public function getConfigTreeBuilder()
                ->scalarNode('title')->defaultValue('')->info('The title of the API.')->end()
                ->scalarNode('description')->defaultValue('')->info('The description of the API.')->end()
                ->scalarNode('version')->defaultValue('0.0.0')->info('The version of the API.')->end()
-                ->scalarNode('default_operation_path_resolver')->defaultValue('api_platform.operation_path_resolver.underscore')->info('Specify the default operation path resolver to use for generating resources operations path.')->end()
+                ->scalarNode('default_operation_path_resolver')->defaultValue('api_platform.operation_path_resolver.underscore')->info('[Deprecated] Specify the default operation path resolver to use for generating resources operations path.')->end()


It's being added, but it's not merged yet: symfony/symfony#22382. For now you can use this trick: symfony/symfony#21051 (comment)

dunglas · 2017-07-12T21:09:58Z

src/Bridge/Symfony/Bundle/Resources/config/api.xml

        </service>

        <service id="api_platform.operation_path_resolver.custom" class="ApiPlatform\Core\PathResolver\CustomOperationPathResolver" public="false">
+            <!-- @TODO should be "api_platform.operation_path_resolver.generator" when the default is removed -->


Can you remove the @todo annotation? It triggers annoying warnings in SensioLabs Insight.

dunglas · 2017-07-12T21:12:47Z

src/Bridge/Symfony/Routing/RouteNameGenerator.php

@@ -28,7 +28,6 @@
 class RouteNameGenerator


Can you make this make this class final btw?

dunglas · 2017-07-12T21:15:40Z

src/Operation/DashPathNameGenerator.php

+ *
+ * @author Antoine Bluchet <[email protected]>
+ */
+class DashPathNameGenerator implements PathNameGeneratorInterface


dunglas · 2017-07-12T21:18:26Z

src/Operation/Factory/CachedSubresourceOperationFactory.php

+     */
+    public function create(string $resourceClass): array
+    {
+        $cacheKey = self::CACHE_KEY_PREFIX.md5(serialize([$resourceClass]));


Why not just using $resourceClass as the cache key? It will be faster (if / must be escaped, you can use str_replace).

dunglas · 2017-07-12T21:20:05Z

src/Operation/Factory/CachedSubresourceOperationFactory.php

+                return $cacheItem->get();
+            }
+        } catch (CacheException $e) {
+            // do nothing


Maybe can you return $subresourceOperations here to prevent the call to isset later?

dunglas · 2017-07-12T21:22:33Z

src/Operation/Factory/SubresourceOperationFactory.php

+                $visiting = "{$parentOperation['property']}-{$parentOperation['resource_class']}-$property-$subresourceClass";
+
+                foreach ($parentOperation['identifiers'] as $key => list($param, $class)) {
+                    $prefix .= 0 === $key ? "$class" : "-$param-$class";


Useless quotes around "$class".

dunglas · 2017-07-12T21:24:30Z

src/Operation/PathNameGeneratorInterface.php

+namespace ApiPlatform\Core\Operation;
+
+/**
+ * Generate a path name according to a string and whether it needs pluralization.


dunglas · 2017-07-12T21:26:45Z

src/PathResolver/DashOperationPathResolver.php

-        if ($operationType === OperationType::ITEM) {
-            $path .= '/{id}';
-        }
+            @trigger_error(sprintf('Method %s() will have a 4th `string $operationName` argument in version 3.0. Not defining it is deprecated since 2.1.', __METHOD__), E_USER_DEPRECATED);


Is this deprecation necessary if the class itself is deprecated?

dunglas · 2017-07-12T21:27:23Z

src/PathResolver/UnderscoreOperationPathResolver.php

-        if ($operationType === OperationType::ITEM) {
-            $path .= '/{id}';
-        }
+            @trigger_error(sprintf('Method %s() will have a 4th `string $operationName` argument in version 3.0. Not defining it is deprecated since 2.1.', __METHOD__), E_USER_DEPRECATED);


teohhanhui · 2017-07-18T08:52:02Z

PathNameGenerator -> PathSegmentNameGenerator (what it really does)

teohhanhui · 2017-07-18T09:16:29Z

src/Operation/PathNameGeneratorInterface.php

+interface PathNameGeneratorInterface
+{
+    /**
+     * Transforms a given string to a tableized, pluralized string.


PHPDoc summary is wrong here, since this is the interface.

soyuka · 2017-07-18T09:57:18Z

tests/Bridge/Symfony/Bundle/Command/SwaggerCommandTest.php

@@ -19,6 +19,7 @@

 /**
 * @author Amrouche Hamza <[email protected]>
+ * @group legacy


Insight on this?

Can you paste the error?

Just a lot of deprecations because of the UnderscoreOperationResolver class deprecation.

Our own code shouldn't trigger deprecations. Can you paste the stack trace?

Remaining deprecation notices (220) The use of ApiPlatform\Core\PathResolver\UnderscoreOperationPathResolver is deprecated since 2.1. Please use PathSegmentNameGenerator instead: 219x 219x in SwaggerCommandTest::testExecute from ApiPlatform\Core\Tests\Bridge\Symfony\Bundle\Command The "api_platform.operation_path_resolver.default" service is deprecated since ApiPlatform 2.1 and will be removed in 3.0. Use PathSegmentNameGenerator instead: 1x 1x in SwaggerCommandTest::testExecute from ApiPlatform\Core\Tests\Bridge\Symfony\Bundle\Command Legacy deprecation notices (62)

This makes phpunit exit with code 1. We're still using UnderscoreOperationPathResolver, should I already remove the service use in the code? If I do so, it will lead to breaking changes.

Or I can just change the api_platform.operation_path_resolver.default but then again, BC break.

Ok got it. It's fine this way.

I'm also having a lot of this deprecation notices. The library shouldn't trigger it, is there any way to use the new version without BC ?

@tseho thanks for the report. Can you try if #1259 fixes the problem for you?

soyuka · 2017-07-18T09:59:26Z

src/Bridge/Symfony/Bundle/DependencyInjection/Configuration.php

+        //     if (isset($v['default_operation_path_resolver'])) {
+        //         @trigger_error('The use of the `default_operation_path_resolver` has been deprecated in 2.1 and will be removed in 3.0. Use `path_name_generator` instead.');
+        //     }
+        // })->end();


Should this hack be added to the rootNode or elsewhere?

soyuka · 2017-07-18T14:34:50Z

I found some bugs while trying this on the framework package, still working on improving the toOne relations:

Get the foobars collection on bar belonging to foo/1:

/foos/1/bar/foobars

teohhanhui · 2017-07-19T06:32:03Z

src/Operation/DashPathSegmentNameGenerator.php

+    /**
+     * {@inheritdoc}
+     */
+    public function getSegmentName(string $name, bool $pluralize = true): string


I don't like the $pluralize argument. It should be up to the implementation to decide whether they want to pluralize or do other things. What we need to provide here is the context (e.g. whether this is for a collection or item).

1. Implement SubresourceOperationFactory 2. Deprecates UnderscoreOperationPathResolver/DashOperationPathResolver in favor of PathSegmentNameGenerator 3. Fix `toOne` relations by not using identifiers 4. Implement Swagger subresources with the correct prefix

dunglas · 2017-07-19T18:05:15Z

Thank you very much @soyuka!

meyerbaptiste · 2017-07-19T18:24:09Z

src/Bridge/Symfony/Routing/RouterOperationPathResolver.php

        } else {
-            return $this->deferred->resolveOperationPath($resourceShortName, $operation, OperationTypeDeprecationHelper::getOperationType($operationType), $operationName);
+            if (null !== $operationName) {


} elseif (OperationType::SUBRESOURCE === $operationType) { throw ... } elseif (null !== $operationName) { $routName = ... } else { return ... }

😛

wlalele · 2017-07-20T09:34:22Z

@soyuka What method would you recommend using to handle access authorization to a subresource ?
Is there any way I can use a « is_granted » property like in the operations ?
Id like to know what would you recommend doing on that subject.
BTW Great work on the project!

desmax · 2017-08-17T05:18:56Z

@soyuka @wlalele Im also confused how to handle access control, please help

soyuka · 2017-08-17T07:08:10Z

@wlalele @desmax it should not be different from other operations.

desmax · 2017-08-17T07:22:57Z

@soyuka please give an example. where to define it? parent or child entity? maybe on property?

desmax · 2017-08-17T08:45:09Z

@soyuka when I define it like this on child entity it seems it does not call my voters at all

 * @ApiResource(
 *  collectionOperations={
 *      "get"={
 *          "method"="GET",
 *          "access_control"="is_granted('get', object)"
 *     }
 *  },
 * )
 */

desmax · 2017-08-17T08:59:54Z

@soyuka But if I do

/**
 * @ApiResource(
 *  collectionOperations={
 *      "api_users_user_project_settings_get_subresource"={
 *          "method"="GET",
 *          "access_control"="is_granted('get', object)"
 *     }
 *  },
 * )
 */

It does work. A bit not obvious, I would say.
P.S. User is my parent entity, UserProjectSettings is a child.

soyuka · 2017-08-17T18:29:14Z

Yes you have to give the correct operation name and it's not easy to find/guess... Your second example is the good one. Sorry about the delay I'm on vacation.

Swagger subresource + OperationPathResolver refactor proposal

soyuka force-pushed the swagger-subresource2 branch 3 times, most recently from cd66784 to 1af17e5 Compare July 11, 2017 15:10

soyuka mentioned this pull request Jul 11, 2017

Subresource feature /dummy/1/relatedDummies #904

Merged

4 tasks

soyuka force-pushed the swagger-subresource2 branch from 1af17e5 to f188c27 Compare July 11, 2017 17:02

Simperfit reviewed Jul 12, 2017

View reviewed changes

soyuka force-pushed the swagger-subresource2 branch from f188c27 to e2020a4 Compare July 12, 2017 12:35

soyuka commented Jul 12, 2017

View reviewed changes

dunglas requested changes Jul 12, 2017

View reviewed changes

teohhanhui reviewed Jul 18, 2017

View reviewed changes

soyuka force-pushed the swagger-subresource2 branch from f835b3b to 80fa3ca Compare July 18, 2017 09:55

soyuka commented Jul 18, 2017

View reviewed changes

soyuka force-pushed the swagger-subresource2 branch 4 times, most recently from ee9bd7f to 7b9c5de Compare July 18, 2017 12:24

dunglas approved these changes Jul 18, 2017

View reviewed changes

soyuka force-pushed the swagger-subresource2 branch 3 times, most recently from 682b437 to 81a279d Compare July 18, 2017 15:35

teohhanhui reviewed Jul 19, 2017

View reviewed changes

soyuka force-pushed the swagger-subresource2 branch 2 times, most recently from f602688 to ddc3ccd Compare July 19, 2017 07:52

soyuka force-pushed the swagger-subresource2 branch from ddc3ccd to 1dd4bdb Compare July 19, 2017 07:55

soyuka force-pushed the swagger-subresource2 branch from 1dd4bdb to e109a25 Compare July 19, 2017 15:10

dunglas merged commit 75b3682 into api-platform:master Jul 19, 2017

meyerbaptiste reviewed Jul 19, 2017

View reviewed changes

api-platform deleted a comment from bco-trey Jul 19, 2017

dunglas mentioned this pull request Jul 20, 2017

Remove deprecation notices #1259

Merged

soyuka deleted the swagger-subresource2 branch January 29, 2018 09:38

hoangnd25 pushed a commit to hoangnd25/core that referenced this pull request Feb 23, 2018

Merge pull request api-platform#1245 from soyuka/swagger-subresource2

2611f1a

Swagger subresource + OperationPathResolver refactor proposal

Uh oh!

Swagger subresource + OperationPathResolver refactor proposal #1245

Swagger subresource + OperationPathResolver refactor proposal #1245

Uh oh!

Conversation

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

Uh oh!

1. OperationPathResolverInterface

2. SubresourceOperationFactory

TODO:

Uh oh!

Simperfit commented Jul 12, 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!

soyuka commented Jul 12, 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!

soyuka commented Jul 12, 2017

Uh oh!

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

Choose a reason for hiding this comment

Uh oh!

teohhanhui commented Jul 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

teohhanhui Jul 18, 2017 • 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!

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 Jul 18, 2017

Uh oh!

Choose a reason for hiding this comment

soyuka commented Jul 11, 2017 •

edited

Loading

1. `OperationPathResolverInterface`

2. `SubresourceOperationFactory`

teohhanhui commented Jul 18, 2017 •

edited

Loading

teohhanhui Jul 18, 2017 •

edited

Loading