From eddafaede4b410352675b475accf7045bed875b5 Mon Sep 17 00:00:00 2001 From: Audain <35590376+audain-dg@users.noreply.github.com> Date: Sat, 5 Sep 2026 10:44:56 +0200 Subject: [PATCH] fix(symfony): throw on route name collisions between resource classes Operation names double as Symfony route names, and `ApiLoader` registered them with `RouteCollection::add()`, which silently replaces an existing entry. Two resource classes exposing the same URI template and method therefore produced one route only, owned by whichever class was discovered last, without any error or warning. `MetadataCollectionFactoryTrait::assertOperationNameIsUnique()` already rejects a duplicate name within one class; this extends the guarantee across classes: - two exposed operations from different classes under one name throw a `RuntimeException` naming both classes and pointing to `routeName` as the way to share a route on purpose; - a `NotExposed` placeholder never wins over an exposed operation of another class, whatever the discovery order, and never throws. The test application had four such collisions, each dropping a route silently: `Issue7916\UserActionResource` vs its ODM twin, `DummyResourceWithComplexConstructor` vs `Employee`, and two `NotExposed` placeholders shadowing `Book` and `Person`. The first two now use distinct URI templates, the last two are handled by the placeholder rule. --- src/Symfony/Routing/ApiLoader.php | 17 ++++ .../Issue7916/UserActionResourceOdm.php | 2 +- .../DummyResourceWithComplexConstructor.php | 2 +- ...Issue7916NestedFilterOnNonResourceTest.php | 16 +++- tests/Symfony/Routing/ApiLoaderTest.php | 85 ++++++++++++++++--- 5 files changed, 103 insertions(+), 19 deletions(-) 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]));