Rework to improve and simplify identifiers management (#3825)

* Move logic of identifiers extractor upwards, use only identifier converter downwards

* Implement identifiers configuration

See also: https://github.com/api-platform/core/blob/master/docs/adr/0001-resource-identifiers.md

* Self review

* temp

Author: soyuka

features/main/custom_identifier.feature

@@ -101,3 +101,21 @@ Feature: Using custom identifier on resource
     When I send a "DELETE" request to "/custom_identifier_dummies/1"
     Then the response status code should be 204
     And the response should be empty
+
+  @createSchema
+  Scenario: Get a resource
+    Given there is a custom multiple identifier dummy
+    When I send a "GET" request to "/custom_multiple_identifier_dummies/1/2"
+    Then the response status code should be 200
+    And the response should be in JSON
+    And the JSON should be equal to:
+    """
+    {
+      "@context": "/contexts/CustomMultipleIdentifierDummy",
+      "@id": "/custom_multiple_identifier_dummies/1/2",
+      "@type": "CustomMultipleIdentifierDummy",
+      "firstId": 1,
+      "secondId": 2,
+      "name": "Orwell"
+    }
+    """

features/main/operation.feature

@@ -64,3 +64,22 @@ Feature: Operation support
   Scenario: Get a 404 response for the disabled item operation
     When I send a "GET" request to "/disable_item_operations/1"
     Then the response status code should be 404
+
+  @createSchema
+  Scenario: Get a book by its ISBN
+    Given there is a book
+    When I send a "GET" request to "books/by_isbn/9780451524935"
+    Then the response status code should be 200
+    And the response should be in JSON
+    And the header "Content-Type" should be equal to "application/ld+json; charset=utf-8"
+    And the JSON should be equal to:
+    """
+    {
+        "@context": "/contexts/Book",
+        "@id": "/books/1",
+        "@type": "Book",
+        "name": "1984",
+        "isbn": "9780451524935",
+        "id": 1
+    }
+    """

src/Annotation/ApiResource.php

@@ -28,6 +28,7 @@
  *     @Attribute("attributes", type="array"),
  *     @Attribute("cacheHeaders", type="array"),
  *     @Attribute("collectionOperations", type="array"),
+ *     @Attribute("compositeIdentifier", type="bool"),
  *     @Attribute("denormalizationContext", type="array"),
  *     @Attribute("deprecationReason", type="string"),
  *     @Attribute("description", type="string"),
@@ -217,7 +218,8 @@ public function __construct(
         ?string $sunset = null,
         ?array $swaggerContext = null,
         ?array $validationGroups = null,
-        ?int $urlGenerationStrategy = null
+        ?int $urlGenerationStrategy = null,
+        ?bool $compositeIdentifier = null
     ) {
         if (!\is_array($description)) { // @phpstan-ignore-line Doctrine annotations support
             [$publicProperties, $configurableAttributes] = self::getConfigMetadata();

src/Bridge/Doctrine/MongoDbOdm/SubresourceDataProvider.php

@@ -121,8 +121,16 @@ private function buildAggregation(array $identifiers, array $context, array $exe
 
         $topAggregationBuilder = $topAggregationBuilder ?? $previousAggregationBuilder;
 
-        [$identifier, $identifierResourceClass] = $context['identifiers'][$remainingIdentifiers - 1];
-        $previousAssociationProperty = $context['identifiers'][$remainingIdentifiers][0] ?? $context['property'];
+        if (\is_string(key($context['identifiers']))) {
+            $contextIdentifiers = array_keys($context['identifiers']);
+            $identifier = $contextIdentifiers[$remainingIdentifiers - 1];
+            $identifierResourceClass = $context['identifiers'][$identifier][0];
+            $previousAssociationProperty = $contextIdentifiers[$remainingIdentifiers] ?? $context['property'];
+        } else {
+            @trigger_error('Identifiers should match the convention introduced in ADR 0001-resource-identifiers, this behavior will be removed in 3.0.', E_USER_DEPRECATED);
+            [$identifier, $identifierResourceClass] = $context['identifiers'][$remainingIdentifiers - 1];
+            $previousAssociationProperty = $context['identifiers'][$remainingIdentifiers][0] ?? $context['property'];
+        }
 
         $manager = $this->managerRegistry->getManagerForClass($identifierResourceClass);
         if (!$manager instanceof DocumentManager) {

src/Bridge/Doctrine/Orm/SubresourceDataProvider.php

@@ -130,8 +130,16 @@ private function buildQuery(array $identifiers, array $context, QueryNameGenerat
 
         $topQueryBuilder = $topQueryBuilder ?? $previousQueryBuilder;
 
-        [$identifier, $identifierResourceClass] = $context['identifiers'][$remainingIdentifiers - 1];
-        $previousAssociationProperty = $context['identifiers'][$remainingIdentifiers][0] ?? $context['property'];
+        if (\is_string(key($context['identifiers']))) {
+            $contextIdentifiers = array_keys($context['identifiers']);
+            $identifier = $contextIdentifiers[$remainingIdentifiers - 1];
+            $identifierResourceClass = $context['identifiers'][$identifier][0];
+            $previousAssociationProperty = $contextIdentifiers[$remainingIdentifiers] ?? $context['property'];
+        } else {
+            @trigger_error('Identifiers should match the convention introduced in ADR 0001-resource-identifiers, this behavior will be removed in 3.0.', E_USER_DEPRECATED);
+            [$identifier, $identifierResourceClass] = $context['identifiers'][$remainingIdentifiers - 1];
+            $previousAssociationProperty = $context['identifiers'][$remainingIdentifiers][0] ?? $context['property'];
+        }
 
         $manager = $this->managerRegistry->getManagerForClass($identifierResourceClass);
 

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

@@ -43,6 +43,7 @@
             <argument>%api_platform.enable_docs%</argument>
             <argument>%api_platform.graphql.graphiql.enabled%</argument>
             <argument>%api_platform.graphql.graphql_playground.enabled%</argument>
+            <argument type="service" id="api_platform.identifiers_extractor.cached"></argument>
 
             <tag name="routing.loader" />
         </service>
@@ -288,6 +289,7 @@
             <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" />
+            <argument type="service" id="api_platform.identifiers_extractor.cached" />
         </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">

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

@@ -42,6 +42,7 @@
             <argument type="service" id="api_platform.operation_path_resolver" />
             <argument type="service" id="api_platform.filter_locator" />
             <argument type="service" id="api_platform.subresource_operation_factory" />
+            <argument type="service" id="api_platform.identifiers_extractor.cached" />
             <argument>%api_platform.formats%</argument>
             <argument type="service" id="api_platform.openapi.options" />
             <argument type="service" id="api_platform.pagination_options" />

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

@@ -33,6 +33,7 @@
             <argument>%api_platform.collection.pagination.enabled_parameter_name%</argument>
             <argument type="collection" />
             <argument>%api_platform.swagger.versions%</argument>
+            <argument type="service" id="api_platform.identifiers_extractor.cached" />
             <tag name="serializer.normalizer" priority="-790" />
         </service>
 

src/Bridge/Symfony/Routing/ApiLoader.php

@@ -13,6 +13,7 @@
 
 namespace ApiPlatform\Core\Bridge\Symfony\Routing;
 
+use ApiPlatform\Core\Api\IdentifiersExtractorInterface;
 use ApiPlatform\Core\Api\OperationType;
 use ApiPlatform\Core\Exception\InvalidResourceException;
 use ApiPlatform\Core\Exception\RuntimeException;
@@ -56,8 +57,9 @@ final class ApiLoader extends Loader
     private $graphQlPlaygroundEnabled;
     private $entrypointEnabled;
     private $docsEnabled;
+    private $identifiersExtractor;
 
-    public function __construct(KernelInterface $kernel, ResourceNameCollectionFactoryInterface $resourceNameCollectionFactory, ResourceMetadataFactoryInterface $resourceMetadataFactory, OperationPathResolverInterface $operationPathResolver, ContainerInterface $container, array $formats, array $resourceClassDirectories = [], SubresourceOperationFactoryInterface $subresourceOperationFactory = null, bool $graphqlEnabled = false, bool $entrypointEnabled = true, bool $docsEnabled = true, bool $graphiQlEnabled = false, bool $graphQlPlaygroundEnabled = false)
+    public function __construct(KernelInterface $kernel, ResourceNameCollectionFactoryInterface $resourceNameCollectionFactory, ResourceMetadataFactoryInterface $resourceMetadataFactory, OperationPathResolverInterface $operationPathResolver, ContainerInterface $container, array $formats, array $resourceClassDirectories = [], SubresourceOperationFactoryInterface $subresourceOperationFactory = null, bool $graphqlEnabled = false, bool $entrypointEnabled = true, bool $docsEnabled = true, bool $graphiQlEnabled = false, bool $graphQlPlaygroundEnabled = false, IdentifiersExtractorInterface $identifiersExtractor = null)
     {
         /** @var string[]|string $paths */
         $paths = $kernel->locateResource('@ApiPlatformBundle/Resources/config/routing');
@@ -74,6 +76,7 @@ public function __construct(KernelInterface $kernel, ResourceNameCollectionFacto
         $this->graphQlPlaygroundEnabled = $graphQlPlaygroundEnabled;
         $this->entrypointEnabled = $entrypointEnabled;
         $this->docsEnabled = $docsEnabled;
+        $this->identifiersExtractor = $identifiersExtractor;
     }
 
     /**
@@ -128,6 +131,8 @@ public function load($data, $type = null): RouteCollection
                         '_format' => null,
                         '_stateless' => $operation['stateless'] ?? $resourceMetadata->getAttribute('stateless'),
                         '_api_resource_class' => $operation['resource_class'],
+                        '_api_identifiers' => $operation['identifiers'],
+                        '_api_has_composite_identifier' => false,
                         '_api_subresource_operation_name' => $operation['route_name'],
                         '_api_subresource_context' => [
                             'property' => $operation['property'],
@@ -222,6 +227,8 @@ private function addRoute(RouteCollection $routeCollection, string $resourceClas
             }
         }
 
+        $operation['identifiers'] = (array) ($operation['identifiers'] ?? $resourceMetadata->getAttribute('identifiers', $this->identifiersExtractor ? $this->identifiersExtractor->getIdentifiersFromResourceClass($resourceClass) : ['id']));
+        $operation['has_composite_identifier'] = \count($operation['identifiers']) > 1 ? $resourceMetadata->getAttribute('composite_identifier', true) : false;
         $path = trim(trim($resourceMetadata->getAttribute('route_prefix', '')), '/');
         $path .= $this->operationPathResolver->resolveOperationPath($resourceShortName, $operation, $operationType, $operationName);
 
@@ -232,6 +239,8 @@ private function addRoute(RouteCollection $routeCollection, string $resourceClas
                 '_format' => null,
                 '_stateless' => $operation['stateless'],
                 '_api_resource_class' => $resourceClass,
+                '_api_identifiers' => $operation['identifiers'],
+                '_api_has_composite_identifier' => $operation['has_composite_identifier'],
                 sprintf('_api_%s_operation_name', $operationType) => $operationName,
             ] + ($operation['defaults'] ?? []),
             $operation['requirements'] ?? [],

src/Bridge/Symfony/Routing/IriConverter.php

@@ -26,6 +26,7 @@
 use ApiPlatform\Core\Exception\InvalidIdentifierException;
 use ApiPlatform\Core\Exception\ItemNotFoundException;
 use ApiPlatform\Core\Exception\RuntimeException;
+use ApiPlatform\Core\Identifier\CompositeIdentifierParser;
 use ApiPlatform\Core\Identifier\IdentifierConverterInterface;
 use ApiPlatform\Core\Metadata\Property\Factory\PropertyMetadataFactoryInterface;
 use ApiPlatform\Core\Metadata\Property\Factory\PropertyNameCollectionFactoryInterface;
@@ -60,11 +61,7 @@ public function __construct(PropertyNameCollectionFactoryInterface $propertyName
         $this->subresourceDataProvider = $subresourceDataProvider;
         $this->identifierConverter = $identifierConverter;
         $this->resourceClassResolver = $resourceClassResolver;
-
-        if (null === $identifiersExtractor) {
-            @trigger_error(sprintf('Not injecting "%s" is deprecated since API Platform 2.1 and will not be possible anymore in API Platform 3', IdentifiersExtractorInterface::class), E_USER_DEPRECATED);
-            $this->identifiersExtractor = new IdentifiersExtractor($propertyNameCollectionFactory, $propertyMetadataFactory, $propertyAccessor ?? PropertyAccess::createPropertyAccessor());
-        }
+        $this->identifiersExtractor = $identifiersExtractor ?: new IdentifiersExtractor($propertyNameCollectionFactory, $propertyMetadataFactory, $propertyAccessor ?? PropertyAccess::createPropertyAccessor());
         $this->resourceMetadataFactory = $resourceMetadataFactory;
     }
 
@@ -148,11 +145,14 @@ public function getIriFromResourceClass(string $resourceClass, int $referenceTyp
     public function getItemIriFromResourceClass(string $resourceClass, array $identifiers, int $referenceType = null): string
     {
         $routeName = $this->routeNameResolver->getRouteName($resourceClass, OperationType::ITEM);
+        $metadata = $this->resourceMetadataFactory->create($resourceClass);
 
-        try {
-            $identifiers = $this->generateIdentifiersUrl($identifiers, $resourceClass);
+        if (\count($identifiers) > 1 && true === $metadata->getAttribute('composite_identifier', true)) {
+            $identifiers = ['id' => CompositeIdentifierParser::stringify($identifiers)];
+        }
 
-            return $this->router->generate($routeName, ['id' => implode(';', $identifiers)], $this->getReferenceType($resourceClass, $referenceType));
+        try {
+            return $this->router->generate($routeName, $identifiers, $this->getReferenceType($resourceClass, $referenceType));
         } catch (RoutingExceptionInterface $e) {
             throw new InvalidArgumentException(sprintf('Unable to generate an IRI for "%s".', $resourceClass), $e->getCode(), $e);
         }
@@ -170,30 +170,6 @@ public function getSubresourceIriFromResourceClass(string $resourceClass, array
         }
     }
 
-    /**
-     * Generate the identifier url.
-     *
-     * @throws InvalidArgumentException
-     *
-     * @return string[]
-     */
-    private function generateIdentifiersUrl(array $identifiers, string $resourceClass): array
-    {
-        if (0 === \count($identifiers)) {
-            throw new InvalidArgumentException(sprintf('No identifiers defined for resource of type "%s"', $resourceClass));
-        }
-
-        if (1 === \count($identifiers)) {
-            return [(string) reset($identifiers)];
-        }
-
-        foreach ($identifiers as $name => $value) {
-            $identifiers[$name] = sprintf('%s=%s', $name, $value);
-        }
-
-        return array_values($identifiers);
-    }
-
     private function getReferenceType(string $resourceClass, ?int $referenceType): ?int
     {
         if (null === $referenceType && null !== $this->resourceMetadataFactory) {

src/Bridge/Symfony/Routing/RouteNameResolver.php

@@ -67,8 +67,8 @@ private function isSameSubresource(array $context, array $currentContext): bool
         $subresources = array_keys($context['subresource_resources']);
         $currentSubresources = [];
 
-        foreach ($currentContext['identifiers'] as $identifierContext) {
-            $currentSubresources[] = $identifierContext[1];
+        foreach ($currentContext['identifiers'] as [$class]) {
+            $currentSubresources[] = $class;
         }
 
         return $currentSubresources === $subresources;

src/DataProvider/OperationDataProviderTrait.php

@@ -15,6 +15,7 @@
 
 use ApiPlatform\Core\Exception\InvalidIdentifierException;
 use ApiPlatform\Core\Exception\RuntimeException;
+use ApiPlatform\Core\Identifier\CompositeIdentifierParser;
 use ApiPlatform\Core\Identifier\IdentifierConverterInterface;
 
 /**
@@ -75,7 +76,15 @@ private function getSubresourceData($identifiers, array $attributes, array $cont
             throw new RuntimeException('Subresources not supported');
         }
 
-        return $this->subresourceDataProvider->getSubresource($attributes['resource_class'], $identifiers, $attributes['subresource_context'] + $context, $attributes['subresource_operation_name']);
+        // TODO: SubresourceDataProvider wants: ['id' => ['id' => 1], 'relatedDummies' => ['id' => 2]], identifiers is ['id' => 1, 'relatedDummies' => 2]
+        $subresourceIdentifiers = [];
+        foreach ($attributes['identifiers'] as $parameterName => [$class, $property]) {
+            if (false !== ($attributes['identifiers'][$parameterName][2] ?? null)) {
+                $subresourceIdentifiers[$parameterName] = [$property => $identifiers[$parameterName]];
+            }
+        }
+
+        return $this->subresourceDataProvider->getSubresource($attributes['resource_class'], $subresourceIdentifiers, $attributes['subresource_context'] + $context, $attributes['subresource_operation_name']);
     }
 
     /**
@@ -85,38 +94,32 @@ private function getSubresourceData($identifiers, array $attributes, array $cont
      */
     private function extractIdentifiers(array $parameters, array $attributes)
     {
-        if (isset($attributes['item_operation_name'])) {
-            if (!isset($parameters['id'])) {
-                throw new InvalidIdentifierException('Parameter "id" not found');
-            }
-
-            $id = $parameters['id'];
-
-            if (null !== $this->identifierConverter) {
-                return $this->identifierConverter->convert((string) $id, $attributes['resource_class']);
-            }
+        $identifiersKeys = $attributes['identifiers'] ?? ['id' => [$attributes['resource_class'], 'id']];
+        $identifiers = [];
 
-            return $id;
-        }
+        $identifiersNumber = \count($identifiersKeys);
+        foreach ($identifiersKeys as $parameterName => $identifiedBy) {
+            if (!isset($parameters[$parameterName])) {
+                if ($attributes['has_composite_identifier']) {
+                    $identifiers = CompositeIdentifierParser::parse($parameters['id']);
+                    if (($currentIdentifiersNumber = \count($identifiers)) !== $identifiersNumber) {
+                        throw new InvalidIdentifierException(sprintf('Expected %d identifiers, got %d', $identifiersNumber, $currentIdentifiersNumber));
+                    }
 
-        if (!isset($attributes['subresource_context'])) {
-            throw new RuntimeException('Either "item_operation_name" or "collection_operation_name" must be defined, unless the "_api_receive" request attribute is set to false.');
-        }
+                    return $identifiers;
+                }
 
-        $identifiers = [];
+                // TODO: Subresources tuple may have a third item representing if it is a "collection", this behavior will be removed in 3.0
+                if (false === ($identifiedBy[2] ?? null)) {
+                    continue;
+                }
 
-        foreach ($attributes['subresource_context']['identifiers'] as $key => [$id, $resourceClass, $hasIdentifier]) {
-            if (false === $hasIdentifier) {
-                continue;
+                throw new InvalidIdentifierException(sprintf('Parameter "%s" not found', $parameterName));
             }
 
-            $identifiers[$id] = $parameters[$id];
-
-            if (null !== $this->identifierConverter) {
-                $identifiers[$id] = $this->identifierConverter->convert((string) $identifiers[$id], $resourceClass);
-            }
+            $identifiers[$parameterName] = $parameters[$parameterName];
         }
 
-        return $identifiers;
+        return $this->identifierConverter->convert($identifiers, $attributes['resource_class']);
     }
 }

src/GraphQl/Resolver/Stage/ReadStage.php

@@ -172,7 +172,7 @@ private function getSubresource(string $rootResolvedClass, array $rootResolvedFi
         $resolvedIdentifiers = [];
         $rootIdentifiers = array_keys($rootResolvedFields);
         foreach ($rootIdentifiers as $rootIdentifier) {
-            $resolvedIdentifiers[] = [$rootIdentifier, $rootResolvedClass];
+            $resolvedIdentifiers[$rootIdentifier] = [$rootResolvedClass, $rootIdentifier];
         }
 
         return $this->subresourceDataProvider->getSubresource($subresourceClass, $rootResolvedFields, $normalizationContext + [