fix(symfony): throw on route name collisions between resource classes

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);
                 }
             }

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(),

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],

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();

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]));