Merge pull request #1245 from soyuka/swagger-subresource2

Swagger subresource + OperationPathResolver refactor proposal

Author: dunglas

features/bootstrap/FeatureContext.php

@@ -609,9 +609,11 @@ public function thereIsAnAnswerToTheQuestion(string $a, string $q)
         $question = new Question();
         $question->setContent($q);
         $question->setAnswer($answer);
+        $answer->addRelatedQuestion($question);
 
         $this->manager->persist($answer);
         $this->manager->persist($question);
+
         $this->manager->flush();
     }
 

features/main/subresource.feature

@@ -12,12 +12,39 @@ Feature: Subresource support
     And the JSON should be equal to:
     """
     {
-        "@context": "\/contexts\/Answer",
-        "@id": "\/answers\/1",
-        "@type": "Answer",
-        "id": 1,
-        "content": "42",
-        "question": "\/questions\/1"
+			"@context": "/contexts/Answer",
+			"@id": "/answers/1",
+			"@type": "Answer",
+			"id": 1,
+			"content": "42",
+			"question": "/questions/1",
+			"relatedQuestions": [
+				"/questions/1"
+			]
+    }
+    """
+
+  Scenario: Get subresource one to one relation
+    When I send a "GET" request to "/questions/1/answer/related_questions"
+    And print last JSON response
+    And the response status code should be 200
+    And the response should be in JSON
+    And the JSON should be equal to:
+    """
+    {
+      "@context": "/contexts/Question",
+      "@id": "/questions/1/answer/related_questions",
+      "@type": "hydra:Collection",
+      "hydra:member": [
+				{
+					"@id": "/questions/1",
+					"@type": "Question",
+					"content": "What's the answer to the Ultimate Question of Life, the Universe and Everything?",
+					"id": 1,
+					"answer": "/answers/1"
+				}
+      ],
+      "hydra:totalItems": 1
     }
     """
 

src/Bridge/Doctrine/Orm/SubresourceDataProvider.php

@@ -13,6 +13,7 @@
 
 namespace ApiPlatform\Core\Bridge\Doctrine\Orm;
 
+use ApiPlatform\Core\Bridge\Doctrine\Orm\Extension\FilterEagerLoadingExtension;
 use ApiPlatform\Core\Bridge\Doctrine\Orm\Extension\QueryCollectionExtensionInterface;
 use ApiPlatform\Core\Bridge\Doctrine\Orm\Extension\QueryItemExtensionInterface;
 use ApiPlatform\Core\Bridge\Doctrine\Orm\Extension\QueryResultCollectionExtensionInterface;
@@ -105,7 +106,7 @@ public function getSubresource(string $resourceClass, array $identifiers, array
             $qb = $manager->createQueryBuilder();
             $alias = $queryNameGenerator->generateJoinAlias($identifier);
             $relationType = $classMetadata->getAssociationMapping($previousAssociationProperty)['type'];
-            $normalizedIdentifiers = $this->normalizeIdentifiers($identifiers[$identifier], $manager, $identifierResourceClass);
+            $normalizedIdentifiers = isset($identifiers[$identifier]) ? $this->normalizeIdentifiers($identifiers[$identifier], $manager, $identifierResourceClass) : [];
 
             switch ($relationType) {
                 //MANY_TO_MANY relations need an explicit join so that the identifier part can be retrieved
@@ -169,6 +170,11 @@ public function getSubresource(string $resourceClass, array $identifiers, array
 
         if (true === $context['collection']) {
             foreach ($this->collectionExtensions as $extension) {
+                // We don't need this anymore because we already made sub queries to ensure correct results
+                if ($extension instanceof FilterEagerLoadingExtension) {
+                    continue;
+                }
+
                 $extension->applyToCollection($queryBuilder, $queryNameGenerator, $resourceClass, $operationName);
 
                 if ($extension instanceof QueryResultCollectionExtensionInterface && $extension->supportsResult($resourceClass, $operationName)) {

src/Bridge/Symfony/Bundle/Action/SwaggerUiAction.php

@@ -117,6 +117,8 @@ private function getContext(Request $request, Documentation $documentation): arr
                 $swaggerData['operationId'] = sprintf('%s%sCollection', $collectionOperationName, $swaggerData['shortName']);
             } elseif (null !== $itemOperationName = $request->attributes->get('_api_item_operation_name')) {
                 $swaggerData['operationId'] = sprintf('%s%sItem', $itemOperationName, $swaggerData['shortName']);
+            } elseif (null !== $subresourceOperationContext = $request->attributes->get('_api_subresource_context')) {
+                $swaggerData['operationId'] = $subresourceOperationContext['operationId'];
             }
 
             list($swaggerData['path'], $swaggerData['method']) = $this->getPathAndMethod($swaggerData);

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

@@ -43,8 +43,15 @@ 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')
+                    ->beforeNormalization()->always(function ($v) {
+                        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_segment_name_generator` instead.', E_USER_DEPRECATED);
+                        }
+                    })->end()
+                ->defaultValue('api_platform.operation_path_resolver.underscore')->info('Specify the default operation path resolver to use for generating resources operations path.')->end()
                 ->scalarNode('name_converter')->defaultNull()->info('Specify a name converter to use.')->end()
+                ->scalarNode('path_segment_name_generator')->defaultValue('api_platform.path_segment_name_generator.underscore')->info('Specify a path name generator to use.')->end()
                 ->scalarNode('api_resources_directory')->defaultValue('Entity')->info('The name of the directory within the bundles that contains the api resources.')->end()
                 ->arrayNode('eager_loading')
                     ->canBeDisabled()

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

@@ -36,8 +36,7 @@
             <argument type="service" id="service_container" />
             <argument>%api_platform.formats%</argument>
             <argument>%api_platform.resource_class_directories%</argument>
-            <argument type="service" id="api_platform.metadata.property.name_collection_factory" />
-            <argument type="service" id="api_platform.metadata.property.metadata_factory" />
+            <argument type="service" id="api_platform.subresource_operation_factory" />
 
             <tag name="routing.loader" />
         </service>
@@ -91,14 +90,31 @@
         <service id="api_platform.operation_path_resolver.router" class="ApiPlatform\Core\Bridge\Symfony\Routing\RouterOperationPathResolver" public="false">
             <argument type="service" id="api_platform.router" />
             <argument type="service" id="api_platform.operation_path_resolver.custom" />
+            <argument type="service" id="api_platform.subresource_operation_factory" />
         </service>
 
         <service id="api_platform.operation_path_resolver.custom" class="ApiPlatform\Core\PathResolver\CustomOperationPathResolver" public="false">
+            <!-- should be "api_platform.operation_path_resolver.generator" when the default is removed -->
             <argument type="service" id="api_platform.operation_path_resolver.default" />
         </service>
 
-        <service id="api_platform.operation_path_resolver.underscore" class="ApiPlatform\Core\PathResolver\UnderscoreOperationPathResolver" public="false" />
-        <service id="api_platform.operation_path_resolver.dash" class="ApiPlatform\Core\PathResolver\DashOperationPathResolver" public="false" />
+        <service id="api_platform.operation_path_resolver.generator" class="ApiPlatform\Core\PathResolver\OperationPathResolver" public="false">
+            <argument type="service" id="api_platform.path_segment_name_generator" />
+        </service>
+
+        <service id="api_platform.operation_path_resolver.underscore" class="ApiPlatform\Core\PathResolver\UnderscoreOperationPathResolver" public="false">
+                <deprecated>The "%service_id%" service is deprecated since ApiPlatform 2.1 and will be removed in 3.0. Use PathSegmentNameGenerator instead.</deprecated>
+        </service>
+
+        <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 PathSegmentNameGenerator instead.</deprecated>
+        </service>
+
+        <!-- Path name generator -->
+
+        <service id="api_platform.path_segment_name_generator" alias="api_platform.path_segment_name_generator.underscore" public="false" />
+        <service id="api_platform.path_segment_name_generator.underscore" class="ApiPlatform\Core\Operation\UnderscorePathSegmentNameGenerator" public="false" />
+        <service id="api_platform.path_segment_name_generator.dash" class="ApiPlatform\Core\Operation\DashPathSegmentNameGenerator" public="false" />
 
         <!-- Event listeners -->
 
@@ -194,6 +210,20 @@
             <argument type="service" id="api_platform.property_accessor" />
         </service>
 
+        <!-- Subresources -->
+
+        <service id="api_platform.subresource_operation_factory" class="ApiPlatform\Core\Operation\Factory\SubresourceOperationFactory" public="false">
+            <argument type="service" id="api_platform.metadata.resource.metadata_factory" />
+            <argument type="service" id="api_platform.metadata.property.name_collection_factory" />
+            <argument type="service" id="api_platform.metadata.property.metadata_factory" />
+            <argument type="service" id="api_platform.path_segment_name_generator" />
+        </service>
+
+        <service id="api_platform.subresource_operation_factory.cached" class="ApiPlatform\Core\Operation\Factory\CachedSubresourceOperationFactory" decorates="api_platform.subresource_operation_factory" decoration-priority="-10" public="false">
+            <argument type="service" id="api_platform.cache.subresource_operation_factory" />
+            <argument type="service" id="api_platform.subresource_operation_factory.cached.inner" />
+        </service>
+
         <!-- Cache -->
 
         <service id="api_platform.cache.route_name_resolver" parent="cache.system" public="false">
@@ -203,6 +233,10 @@
         <service id="api_platform.cache.identifiers_extractor" parent="cache.system" public="false">
             <tag name="cache.pool" />
         </service>
+
+        <service id="api_platform.cache.subresource_operation_factory" parent="cache.system" public="false">
+            <tag name="cache.pool" />
+        </service>
     </services>
 
 </container>

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

@@ -22,6 +22,7 @@
             <argument>%api_platform.oauth.tokenUrl%</argument>
             <argument>%api_platform.oauth.authorizationUrl%</argument>
             <argument>%api_platform.oauth.scopes%</argument>
+            <argument type="service" id="api_platform.subresource_operation_factory"></argument>
             <tag name="serializer.normalizer" priority="16" />
         </service>
 

src/Bridge/Symfony/Routing/ApiLoader.php

@@ -16,10 +16,9 @@
 use ApiPlatform\Core\Api\OperationType;
 use ApiPlatform\Core\Exception\InvalidResourceException;
 use ApiPlatform\Core\Exception\RuntimeException;
-use ApiPlatform\Core\Metadata\Property\Factory\PropertyMetadataFactoryInterface;
-use ApiPlatform\Core\Metadata\Property\Factory\PropertyNameCollectionFactoryInterface;
 use ApiPlatform\Core\Metadata\Resource\Factory\ResourceMetadataFactoryInterface;
 use ApiPlatform\Core\Metadata\Resource\Factory\ResourceNameCollectionFactoryInterface;
+use ApiPlatform\Core\Operation\Factory\SubresourceOperationFactoryInterface;
 use ApiPlatform\Core\PathResolver\OperationPathResolverInterface;
 use Symfony\Component\Config\FileLocator;
 use Symfony\Component\Config\Loader\Loader;
@@ -42,19 +41,17 @@ final class ApiLoader extends Loader
      */
     const ROUTE_NAME_PREFIX = 'api_';
     const DEFAULT_ACTION_PATTERN = 'api_platform.action.';
-    const SUBRESOURCE_SUFFIX = '_get_subresource';
 
     private $fileLoader;
-    private $propertyNameCollectionFactory;
-    private $propertyMetadataFactory;
     private $resourceNameCollectionFactory;
     private $resourceMetadataFactory;
     private $operationPathResolver;
     private $container;
     private $formats;
     private $resourceClassDirectories;
+    private $subresourceOperationFactory;
 
-    public function __construct(KernelInterface $kernel, ResourceNameCollectionFactoryInterface $resourceNameCollectionFactory, ResourceMetadataFactoryInterface $resourceMetadataFactory, OperationPathResolverInterface $operationPathResolver, ContainerInterface $container, array $formats, array $resourceClassDirectories = [], PropertyNameCollectionFactoryInterface $propertyNameCollectionFactory = null, PropertyMetadataFactoryInterface $propertyMetadataFactory = null)
+    public function __construct(KernelInterface $kernel, ResourceNameCollectionFactoryInterface $resourceNameCollectionFactory, ResourceMetadataFactoryInterface $resourceMetadataFactory, OperationPathResolverInterface $operationPathResolver, ContainerInterface $container, array $formats, array $resourceClassDirectories = [], SubresourceOperationFactoryInterface $subresourceOperationFactory = null)
     {
         $this->fileLoader = new XmlFileLoader(new FileLocator($kernel->locateResource('@ApiPlatformBundle/Resources/config/routing')));
         $this->resourceNameCollectionFactory = $resourceNameCollectionFactory;
@@ -63,8 +60,7 @@ public function __construct(KernelInterface $kernel, ResourceNameCollectionFacto
         $this->container = $container;
         $this->formats = $formats;
         $this->resourceClassDirectories = $resourceClassDirectories;
-        $this->propertyNameCollectionFactory = $propertyNameCollectionFactory;
-        $this->propertyMetadataFactory = $propertyMetadataFactory;
+        $this->subresourceOperationFactory = $subresourceOperationFactory;
     }
 
     /**
@@ -99,90 +95,35 @@ public function load($data, $type = null): RouteCollection
                 }
             }
 
-            $this->computeSubresourceOperations($routeCollection, $resourceClass);
-        }
-
-        return $routeCollection;
-    }
-
-    /**
-     * Handles subresource operations recursively and declare their corresponding routes.
-     *
-     * @param RouteCollection $routeCollection
-     * @param string          $resourceClass
-     * @param string          $rootResourceClass null on the first iteration, it then keeps track of the origin resource class
-     * @param array           $parentOperation   the previous call operation
-     */
-    private function computeSubresourceOperations(RouteCollection $routeCollection, string $resourceClass, string $rootResourceClass = null, array $parentOperation = null, array $visited = [])
-    {
-        if (null === $this->propertyNameCollectionFactory || null === $this->propertyMetadataFactory) {
-            return;
-        }
-
-        if (null === $rootResourceClass) {
-            $rootResourceClass = $resourceClass;
-        }
-
-        foreach ($this->propertyNameCollectionFactory->create($resourceClass) as $property) {
-            $propertyMetadata = $this->propertyMetadataFactory->create($resourceClass, $property);
-
-            if (!$propertyMetadata->hasSubresource()) {
-                continue;
-            }
-
-            $subresource = $propertyMetadata->getSubresource();
-
-            $operation = [
-                'property' => $property,
-                'collection' => $subresource->isCollection(),
-            ];
-
-            $visiting = "$rootResourceClass $resourceClass $property {$subresource->isCollection()} {$subresource->getResourceClass()}";
-
-            if (in_array($visiting, $visited, true)) {
+            if (null === $this->subresourceOperationFactory) {
                 continue;
             }
 
-            $visited[] = $visiting;
-
-            if (null === $parentOperation) {
-                $rootResourceMetadata = $this->resourceMetadataFactory->create($rootResourceClass);
-                $rootShortname = $rootResourceMetadata->getShortName();
-
-                $operation['identifiers'] = [['id', $rootResourceClass]];
-                $operation['route_name'] = RouteNameGenerator::generate('get', $rootShortname, OperationType::SUBRESOURCE, $operation);
-                $operation['path'] = $this->operationPathResolver->resolveOperationPath($rootShortname, $operation, OperationType::SUBRESOURCE, $operation['route_name']);
-            } else {
-                $operation['identifiers'] = $parentOperation['identifiers'];
-                $operation['identifiers'][] = [$parentOperation['property'], $resourceClass];
-                $operation['route_name'] = str_replace('get'.RouteNameGenerator::SUBRESOURCE_SUFFIX, RouteNameGenerator::routeNameResolver($property, $operation['collection']).'_get'.RouteNameGenerator::SUBRESOURCE_SUFFIX, $parentOperation['route_name']);
-                $operation['path'] = $this->operationPathResolver->resolveOperationPath($parentOperation['path'], $operation, OperationType::SUBRESOURCE, $operation['route_name']);
-            }
-
-            $route = new Route(
-                $operation['path'],
-                [
-                    '_controller' => self::DEFAULT_ACTION_PATTERN.'get_subresource',
-                    '_format' => null,
-                    '_api_resource_class' => $subresource->getResourceClass(),
-                    '_api_subresource_operation_name' => $operation['route_name'],
-                    '_api_subresource_context' => [
-                        'property' => $operation['property'],
-                        'identifiers' => $operation['identifiers'],
-                        'collection' => $subresource->isCollection(),
+            foreach ($this->subresourceOperationFactory->create($resourceClass) as $operationId => $operation) {
+                $routeCollection->add($operation['route_name'], new Route(
+                    $operation['path'],
+                    [
+                        '_controller' => self::DEFAULT_ACTION_PATTERN.'get_subresource',
+                        '_format' => null,
+                        '_api_resource_class' => $operation['resource_class'],
+                        '_api_subresource_operation_name' => $operation['route_name'],
+                        '_api_subresource_context' => [
+                            'property' => $operation['property'],
+                            'identifiers' => $operation['identifiers'],
+                            'collection' => $operation['collection'],
+                            'operationId' => $operationId,
+                        ],
                     ],
-                ],
-                [],
-                [],
-                '',
-                [],
-                ['GET']
-            );
-
-            $routeCollection->add($operation['route_name'], $route);
-
-            $this->computeSubresourceOperations($routeCollection, $subresource->getResourceClass(), $rootResourceClass, $operation, $visited);
+                    [],
+                    [],
+                    '',
+                    [],
+                    ['GET']
+                ));
+            }
         }
+
+        return $routeCollection;
     }
 
     /**