diff --git a/src/Symfony/Routing/ApiLoader.php b/src/Symfony/Routing/ApiLoader.php index 1fa70ab2963..67a4fcbf5ee 100644 --- a/src/Symfony/Routing/ApiLoader.php +++ b/src/Symfony/Routing/ApiLoader.php @@ -66,6 +66,7 @@ public function load(mixed $data, ?string $type = null): RouteCollection $this->loadExternalFiles($routeCollection); } + $exposedOperations = []; foreach ($this->resourceNameCollectionFactory->create() as $resourceClass) { foreach ($this->resourceMetadataFactory->create($resourceClass) as $resourceMetadata) { foreach ($resourceMetadata->getOperations() as $operationName => $operation) { @@ -126,6 +127,22 @@ public function load(mixed $data, ?string $type = null): RouteCollection $operation->getCondition() ?? '' ); + // A NotExposed placeholder may share its name with an exposed operation of another class, the exposed one always wins. + $existing = $routeCollection->get($operationName); + if (null !== $existing && ($existingResourceClass = $existing->getDefault('_api_resource_class')) !== $resourceClass) { + if ($operation instanceof NotExposed) { + continue; + } + + if (isset($exposedOperations[$operationName])) { + throw new RuntimeException(\sprintf('Operation "%s" is declared by both "%s" and "%s". Operation names must be unique because they are also used as Symfony route names, and the second declaration would silently replace the first. Use distinct operation names or URI templates, or make one resource reuse the route of the other with "routeName".', $operationName, $existingResourceClass, $resourceClass)); + } + } + + if (!$operation instanceof NotExposed) { + $exposedOperations[$operationName] = true; + } + $routeCollection->add($operationName, $route); } } diff --git a/tests/Fixtures/TestBundle/ApiResource/Issue7916/UserActionResourceOdm.php b/tests/Fixtures/TestBundle/ApiResource/Issue7916/UserActionResourceOdm.php index d15f5d5bd89..3c268b64ee7 100644 --- a/tests/Fixtures/TestBundle/ApiResource/Issue7916/UserActionResourceOdm.php +++ b/tests/Fixtures/TestBundle/ApiResource/Issue7916/UserActionResourceOdm.php @@ -29,7 +29,7 @@ #[ApiResource( operations: [ new GetCollection( - uriTemplate: '/user-actions', + uriTemplate: '/user-actions-odm', parameters: [ 'name' => new QueryParameter( filter: new PartialSearchFilter(), diff --git a/tests/Fixtures/TestBundle/Entity/DummyResourceWithComplexConstructor.php b/tests/Fixtures/TestBundle/Entity/DummyResourceWithComplexConstructor.php index 2a850595ec3..fc8c60f86e5 100644 --- a/tests/Fixtures/TestBundle/Entity/DummyResourceWithComplexConstructor.php +++ b/tests/Fixtures/TestBundle/Entity/DummyResourceWithComplexConstructor.php @@ -20,7 +20,7 @@ #[Post] #[ApiResource( shortName: 'DummyResourceWithComplexConstructorByCompany', - uriTemplate: '/companies/{companyId}/employees/{id}', + uriTemplate: '/companies/{companyId}/dummy-employees/{id}', uriVariables: [ 'companyId' => ['from_class' => Company::class, 'to_property' => 'company'], 'id' => ['from_class' => DummyResourceWithComplexConstructor::class], diff --git a/tests/Functional/Parameters/Issue7916NestedFilterOnNonResourceTest.php b/tests/Functional/Parameters/Issue7916NestedFilterOnNonResourceTest.php index 77f0240aa55..b877bb88e84 100644 --- a/tests/Functional/Parameters/Issue7916NestedFilterOnNonResourceTest.php +++ b/tests/Functional/Parameters/Issue7916NestedFilterOnNonResourceTest.php @@ -52,6 +52,14 @@ public static function getResources(): array return [UserActionResource::class, UserResource::class]; } + /** + * The ORM and ODM resources cannot share one URI template: operation names double as route names. + */ + private function path(): string + { + return $this->isMongoDB() ? '/user-actions-odm' : '/user-actions'; + } + protected function setUp(): void { $entities = $this->isMongoDB() @@ -69,7 +77,7 @@ protected function setUp(): void */ public function testFilteringOnNonResourceRelationName(): void { - $response = self::createClient()->request('GET', '/user-actions?name=john'); + $response = self::createClient()->request('GET', $this->path().'?name=john'); $this->assertResponseIsSuccessful(); $data = $response->toArray(); @@ -82,7 +90,7 @@ public function testFilteringOnNonResourceRelationName(): void */ public function testFilteringOnNonResourceRelationEmail(): void { - $response = self::createClient()->request('GET', '/user-actions?email=john@example.com'); + $response = self::createClient()->request('GET', $this->path().'?email=john@example.com'); $this->assertResponseIsSuccessful(); $data = $response->toArray(); @@ -95,7 +103,7 @@ public function testFilteringOnNonResourceRelationEmail(): void */ public function testPartialFilteringOnNonResourceRelation(): void { - $response = self::createClient()->request('GET', '/user-actions?name=ane'); + $response = self::createClient()->request('GET', $this->path().'?name=ane'); $this->assertResponseIsSuccessful(); $data = $response->toArray(); @@ -108,7 +116,7 @@ public function testPartialFilteringOnNonResourceRelation(): void */ public function testNoMatchFilteringOnNonResourceRelation(): void { - $response = self::createClient()->request('GET', '/user-actions?name=nonexistent'); + $response = self::createClient()->request('GET', $this->path().'?name=nonexistent'); $this->assertResponseIsSuccessful(); $data = $response->toArray(); diff --git a/tests/Symfony/Routing/ApiLoaderTest.php b/tests/Symfony/Routing/ApiLoaderTest.php index 38734a6e623..4f029a9d02b 100644 --- a/tests/Symfony/Routing/ApiLoaderTest.php +++ b/tests/Symfony/Routing/ApiLoaderTest.php @@ -16,6 +16,7 @@ use ApiPlatform\Metadata\ApiProperty; use ApiPlatform\Metadata\ApiResource; use ApiPlatform\Metadata\Delete; +use ApiPlatform\Metadata\Exception\RuntimeException; use ApiPlatform\Metadata\Get; use ApiPlatform\Metadata\GetCollection; use ApiPlatform\Metadata\Link; @@ -76,7 +77,7 @@ public function testApiLoader(): void $path, 'api_platform.action.get_item', null, - RelatedDummyEntity::class, + DummyEntity::class, [], 'api_dummies_get_item', ['my_default' => 'default_value', '_controller' => 'should_not_be_overriden'], @@ -91,7 +92,7 @@ public function testApiLoader(): void $path, 'api_platform.action.placeholder', null, - RelatedDummyEntity::class, + DummyEntity::class, [], 'api_dummies_delete_item', [], @@ -106,7 +107,7 @@ public function testApiLoader(): void $path, 'api_platform.action.placeholder', null, - RelatedDummyEntity::class, + DummyEntity::class, [], 'api_dummies_put_item', [], @@ -121,7 +122,7 @@ public function testApiLoader(): void '/dummies.{_format}', 'some.service.name', null, - RelatedDummyEntity::class, + DummyEntity::class, [], 'api_dummies_my_op_collection', ['my_default' => 'default_value', '_format' => 'a valid format'], @@ -140,7 +141,7 @@ public function testApiLoader(): void '/dummies.{_format}', 'api_platform.action.placeholder', null, - RelatedDummyEntity::class, + DummyEntity::class, [], 'api_dummies_my_second_op_collection', [], @@ -158,7 +159,7 @@ public function testApiLoader(): void 'some/custom/path', 'api_platform.action.placeholder', null, - RelatedDummyEntity::class, + DummyEntity::class, [], 'api_dummies_my_path_op_collection', [], @@ -173,7 +174,7 @@ public function testApiLoader(): void '/dummies.{_format}', 'api_platform.action.placeholder', true, - RelatedDummyEntity::class, + DummyEntity::class, [], 'api_dummies_my_stateless_op_collection', [], @@ -188,7 +189,7 @@ public function testApiLoader(): void '/foo', 'Foo\\Bar\\MyController::method', null, - RelatedDummyEntity::class, + DummyEntity::class, [], 'api_dummies_my_controller_method_item', [], @@ -219,7 +220,7 @@ public function testApiLoaderWithPrefix(): void $prefixedPath, 'api_platform.action.placeholder', null, - RelatedDummyEntity::class, + DummyEntity::class, [], 'api_dummies_get_item', ['my_default' => 'default_value', '_controller' => 'should_not_be_overriden'], @@ -234,7 +235,7 @@ public function testApiLoaderWithPrefix(): void $prefixedPath, 'api_platform.action.placeholder', null, - RelatedDummyEntity::class, + DummyEntity::class, [], 'api_dummies_delete_item', [], @@ -248,7 +249,7 @@ public function testApiLoaderWithPrefix(): void $prefixedPath, 'api_platform.action.placeholder', null, - RelatedDummyEntity::class, + DummyEntity::class, [], 'api_dummies_put_item', [], @@ -296,7 +297,65 @@ public function testApiLoaderWithUndefinedControllerService(): void $routeCollection->get('api_dummies_my_undefined_controller_method_item'); } - private function getApiLoaderWithResourceMetadataCollection(ResourceMetadataCollection $resourceCollection): ApiLoader + public function testApiLoaderThrowsWhenTwoResourceClassesDeclareTheSameOperationName(): void + { + $this->expectException(RuntimeException::class); + $this->expectExceptionMessage(\sprintf('Operation "_api_/dummies/{id}_get" is declared by both "%s" and "%s".', DummyEntity::class, RelatedDummyEntity::class)); + + $operations = new Operations([ + '_api_/dummies/{id}_get' => (new Get())->withUriTemplate('/dummies/{id}'), + ]); + + $this->getApiLoaderWithResourceMetadataCollection( + new ResourceMetadataCollection(DummyEntity::class, [(new ApiResource())->withShortName('dummy')->withOperations($operations)]), + new ResourceMetadataCollection(RelatedDummyEntity::class, [(new ApiResource())->withShortName('relatedDummy')->withOperations($operations)]), + )->load(null); + } + + public function testApiLoaderLetsAResourceReuseTheRouteOfAnotherOneWithRouteName(): void + { + $operationName = '_api_/dummies/{id}_get'; + + $routeCollection = $this->getApiLoaderWithResourceMetadataCollection( + new ResourceMetadataCollection(DummyEntity::class, [(new ApiResource())->withShortName('dummy')->withOperations(new Operations([ + $operationName => (new Get())->withUriTemplate('/dummies/{id}'), + ]))]), + new ResourceMetadataCollection(RelatedDummyEntity::class, [(new ApiResource())->withShortName('relatedDummy')->withOperations(new Operations([ + $operationName => (new Get())->withUriTemplate('/dummies/{id}')->withRouteName($operationName), + ]))]), + )->load(null); + + $route = $routeCollection->get($operationName); + $this->assertNotNull($route); + $this->assertSame(DummyEntity::class, $route->getDefault('_api_resource_class')); + } + + /** + * A NotExposed placeholder of one class may share its name with the exposed operation of another class: + * the exposed operation owns the route, whatever the discovery order. + */ + public function testApiLoaderLetsAnExposedOperationWinOverANotExposedPlaceholder(): void + { + $operationName = '_api_/dummies/{id}_get'; + $exposed = new ResourceMetadataCollection(DummyEntity::class, [(new ApiResource())->withShortName('dummy')->withOperations(new Operations([ + $operationName => (new Get())->withUriTemplate('/dummies/{id}'), + ]))]); + $placeholder = new ResourceMetadataCollection(RelatedDummyEntity::class, [(new ApiResource())->withShortName('dummy')->withOperations(new Operations([ + $operationName => (new NotExposed())->withUriTemplate('/dummies/{id}'), + ]))]); + + // exposed first: DummyEntity is loaded first and owns the exposed operation + $route = $this->getApiLoaderWithResourceMetadataCollection($exposed, $placeholder)->load(null)->get($operationName); + $this->assertNotNull($route); + $this->assertSame(DummyEntity::class, $route->getDefault('_api_resource_class')); + + // placeholder first: the exposed operation now belongs to RelatedDummyEntity, loaded second, and still wins + $route = $this->getApiLoaderWithResourceMetadataCollection($placeholder, $exposed)->load(null)->get($operationName); + $this->assertNotNull($route); + $this->assertSame(RelatedDummyEntity::class, $route->getDefault('_api_resource_class')); + } + + private function getApiLoaderWithResourceMetadataCollection(ResourceMetadataCollection $resourceCollection, ?ResourceMetadataCollection $relatedResourceCollection = null): ApiLoader { $routingConfig = __DIR__.'/../../../src/Symfony/Bundle/Resources/config/routing'; @@ -323,7 +382,7 @@ private function getApiLoaderWithResourceMetadataCollection(ResourceMetadataColl $resourceMetadataFactoryProphecy = $this->prophesize(ResourceMetadataCollectionFactoryInterface::class); $resourceMetadataFactoryProphecy->create(DummyEntity::class)->willReturn($resourceCollection); - $resourceMetadataFactoryProphecy->create(RelatedDummyEntity::class)->willReturn($resourceCollection); + $resourceMetadataFactoryProphecy->create(RelatedDummyEntity::class)->willReturn($relatedResourceCollection ?? new ResourceMetadataCollection(RelatedDummyEntity::class, [])); $resourceNameCollectionFactoryProphecy = $this->prophesize(ResourceNameCollectionFactoryInterface::class); $resourceNameCollectionFactoryProphecy->create()->willReturn(new ResourceNameCollection([DummyEntity::class, RelatedDummyEntity::class]));