Ignore routes that are already considered tenant routes from cloning, update test accordingly

Author: lukinovec

src/Actions/CloneRoutesAsTenant.php

@@ -111,6 +111,11 @@ public function cloneRoute(Route|string $route): static
 
     protected function shouldBeCloned(Route $route): bool
     {
+        // Don't clone routes that already have tenant parameter or prefix
+        if ($this->routeIsTenant($route)) {
+            return false;
+        }
+
         if ($this->shouldClone) {
             return ($this->shouldClone)($route);
         }
@@ -166,45 +171,26 @@ protected function copyMiscRouteProperties(Route $originalRoute, Route $newRoute
             ->setDefaults($originalRoute->defaults);
     }
 
-    /**
-     * Process middleware for cloning, handling middleware groups properly.
-     * This extracts middleware from groups (up to 3 levels deep), filters out
-     * cloneRoutesWithMiddleware, and adds the 'tenant' middleware.
-     *
-     * Uses approach similar to getRouteMiddleware() in DealsWithRouteContexts for consistency.
-     */
+    /** Removes top-level cloneRoutesWithMiddleware and adds 'tenant' middleware. */
     protected function processMiddlewareForCloning(array $middlewares): array
     {
-        $middlewareGroups = $this->router->getMiddlewareGroups();
-
-        $unpackGroupMiddleware = function (array $middleware) use ($middlewareGroups) {
-            $innerMiddleware = [];
-
-            foreach ($middleware as $inner) {
-                if (isset($middlewareGroups[$inner])) {
-                    $innerMiddleware = array_merge($innerMiddleware, $middlewareGroups[$inner]);
-                } else {
-                    // Actual middleware, not a group
-                    $innerMiddleware[] = $inner;
-                }
-            }
-
-            return $innerMiddleware;
-        };
-
-        // Extract all middleware from groups (up to 3 levels deep)
-        $firstLevelUnpacked = $unpackGroupMiddleware($middlewares);
-        $secondLevelUnpacked = $unpackGroupMiddleware($firstLevelUnpacked);
-        $thirdLevelUnpacked = $unpackGroupMiddleware($secondLevelUnpacked);
-
-        // Filter out MW in cloneRoutesWithMiddleware and add the 'tenant' flag
+        // Filter out top-level cloneRoutesWithMiddleware and add the 'tenant' flag
         $processedMiddleware = array_filter(
-            $thirdLevelUnpacked,
+            $middlewares,
             fn ($mw) => ! in_array($mw, $this->cloneRoutesWithMiddleware)
         );
 
         $processedMiddleware[] = 'tenant';
 
         return array_unique($processedMiddleware);
     }
+
+    /** Check if route already has tenant parameter or name prefix. */
+    protected function routeIsTenant(Route $route): bool
+    {
+        $routeHasTenantParameter = in_array(PathTenantResolver::tenantParameterName(), $route->parameterNames());
+        $routeHasTenantPrefix = $route->getName() && str_starts_with($route->getName(), PathTenantResolver::tenantRouteNamePrefix());
+
+        return $routeHasTenantParameter || $routeHasTenantPrefix;
+    }
 }

tests/CloneActionTest.php

@@ -215,43 +215,62 @@
         ->toBe("http://localhost/prefix/{$tenant->getTenantKey()}/home");
 });
 
-test('clone middleware within middleware groups is properly handled during cloning', function () {
-    // Simple MW group with 'clone' flag
-    RouteFacade::middlewareGroup('simple-group', ['auth', 'clone']);
-
-    // Define nested middleware groups 3 levels deep
-    RouteFacade::middlewareGroup('level-3-group', ['clone']);
-    RouteFacade::middlewareGroup('level-2-group', ['auth', 'level-3-group']);
-    RouteFacade::middlewareGroup('nested-group', ['web', 'level-2-group']);
-
-    // Create routes using both simple and nested middleware groups
-    RouteFacade::get('/simple', fn () => true)
-        ->middleware('simple-group')
-        ->name('simple');
-
-    RouteFacade::get('/nested', fn () => true)
-        ->middleware('nested-group')
-        ->name('nested');
-
-    app(CloneRoutesAsTenant::class)->handle();
-
-    // Test simple middleware group handling
-    $clonedSimpleRoute = RouteFacade::getRoutes()->getByName('tenant.simple');
-    expect($clonedSimpleRoute)->not()->toBeNull();
-
-    $simpleRouteMiddleware = tenancy()->getRouteMiddleware($clonedSimpleRoute);
-    expect($simpleRouteMiddleware)
-        ->toContain('auth', 'tenant')
-        ->not()->toContain('clone', 'simple-group');
-
-    // Test nested middleware group handling (3 levels deep)
-    $clonedNestedRoute = RouteFacade::getRoutes()->getByName('tenant.nested');
-    expect($clonedNestedRoute)->not()->toBeNull();
-
-    $nestedRouteMiddleware = tenancy()->getRouteMiddleware($clonedNestedRoute);
+test('tenant routes are ignored from cloning and clone middleware in groups causes no issues', function () {
+    // Should NOT be cloned, already has tenant parameter
+    RouteFacade::get("/{tenant}/route-with-tenant-parameter", fn () => true)
+        ->middleware(['clone'])
+        ->name("tenant.route-with-tenant-parameter");
+
+    // Should NOT be cloned
+    // The route already has tenant name prefix
+    RouteFacade::get("/route-with-tenant-name-prefix", fn () => true)
+        ->middleware(['clone'])
+        ->name("tenant.route-with-tenant-name-prefix");
+
+    // Should NOT be cloned
+    // The route already has a tenant parameter + 'clone' middleware in group
+    // 'clone' MW in groups won't be removed, this also doesn't cause any issues
+    RouteFacade::middlewareGroup('group', ['auth', 'clone']);
+    RouteFacade::get("/{tenant}/route-with-clone-in-mw-group", fn () => true)
+        ->middleware('group')
+        ->name("tenant.route-with-clone-in-mw-group");
+
+    // SHOULD be cloned (with clone middleware)
+    RouteFacade::get('/foo', fn () => true)
+        ->middleware(['clone'])
+        ->name('foo');
+
+    // SHOULD be cloned (with nested clone middleware)
+    RouteFacade::get('/bar', fn () => true)
+        ->middleware(['group'])
+        ->name('bar');
+
+    $cloneAction = app(CloneRoutesAsTenant::class);
+    $initialRouteCount = count(RouteFacade::getRoutes()->get());
+
+    // Run clone action multiple times
+    $cloneAction->handle();
+    $firstRunCount = count(RouteFacade::getRoutes()->get());
+
+    $cloneAction->handle();
+    $secondRunCount = count(RouteFacade::getRoutes()->get());
+
+    $cloneAction->handle();
+    $thirdRunCount = count(RouteFacade::getRoutes()->get());
+
+    // Two route should have been cloned, and only once
+    expect($firstRunCount)->toBe($initialRouteCount + 2);
+    // No new routes on subsequent runs
+    expect($secondRunCount)->toBe($firstRunCount);
+    expect($thirdRunCount)->toBe($firstRunCount);
+
+    // Verify the correct routes were cloned
+    expect(RouteFacade::getRoutes()->getByName('tenant.foo'))->not()->toBeNull();
+    expect(RouteFacade::getRoutes()->getByName('tenant.bar'))->not()->toBeNull();
 
-    expect($nestedRouteMiddleware)
-        ->toContain('web', 'auth', 'tenant')
-        // Shouldn't contain 'clone' or any nested group names
-        ->not()->toContain('clone', 'nested-group', 'level-2-group', 'level-3-group');
+    // Tenant routes were not duplicated
+    $allRouteNames = collect(RouteFacade::getRoutes()->get())->map->getName()->filter();
+    expect($allRouteNames->filter(fn($name) => str_contains($name, 'route-with-tenant-parameter'))->count())->toBe(1);
+    expect($allRouteNames->filter(fn($name) => str_contains($name, 'route-with-tenant-name-prefix'))->count())->toBe(1);
+    expect($allRouteNames->filter(fn($name) => str_contains($name, 'route-with-clone-in-mw-group'))->count())->toBe(1);
 });