From 0f5f88ff062f74f15abbdd909cf9c862d2f4e779 Mon Sep 17 00:00:00 2001 From: David Miguel Date: Tue, 24 Feb 2026 17:15:18 +0100 Subject: [PATCH] [go_router] Fix chained top-level redirects not being fully resolved Fixes https://github.com/flutter/flutter/issues/178984 --- packages/go_router/lib/src/configuration.dart | 32 +- .../fix_chained_redirect_regression.yaml | 4 + packages/go_router/test/on_enter_test.dart | 171 +++++ .../go_router/test/redirect_chain_test.dart | 630 ++++++++++++++++++ 4 files changed, 832 insertions(+), 5 deletions(-) create mode 100644 packages/go_router/pending_changelogs/fix_chained_redirect_regression.yaml create mode 100644 packages/go_router/test/redirect_chain_test.dart diff --git a/packages/go_router/lib/src/configuration.dart b/packages/go_router/lib/src/configuration.dart index ef408b442708..f59abd498aec 100644 --- a/packages/go_router/lib/src/configuration.dart +++ b/packages/go_router/lib/src/configuration.dart @@ -427,7 +427,20 @@ class RouteConfiguration { if (newMatch.isError) { return newMatch; } - return redirect(context, newMatch, redirectHistory: redirectHistory); + // Re-evaluate top-level redirect on the new location before + // recursing into route-level redirects. This ensures that + // route-level redirects landing on a new path also trigger + // top-level redirect evaluation for that path. + final FutureOr afterTopLevel = applyTopLegacyRedirect( + context, + newMatch, + redirectHistory: redirectHistory, + ); + return redirect( + context, + afterTopLevel, + redirectHistory: redirectHistory, + ); } return prevMatchList; } @@ -484,9 +497,10 @@ class RouteConfiguration { /// /// Shares [redirectHistory] with later route-level redirects for proper loop detection. /// - /// Note: Legacy top-level redirect is executed at most once per navigation, - /// before route-level redirects. It does not re-evaluate if it redirects to - /// a location that would itself trigger another top-level redirect. + /// Recursively re-evaluates the top-level redirect when it produces a new + /// location, so that top-level redirect chains (e.g. `/` → `/a` → `/b`) are + /// fully resolved. Loop detection and redirect limit are enforced via the + /// shared [redirectHistory]. FutureOr applyTopLegacyRedirect( BuildContext context, RouteMatchList prevMatchList, { @@ -500,7 +514,15 @@ class RouteConfiguration { prevMatchList.uri, redirectHistory, ); - return newMatch; + if (newMatch.isError) { + return newMatch; + } + // Recursively re-evaluate the top-level redirect on the new location. + return applyTopLegacyRedirect( + context, + newMatch, + redirectHistory: redirectHistory, + ); } return prevMatchList; } diff --git a/packages/go_router/pending_changelogs/fix_chained_redirect_regression.yaml b/packages/go_router/pending_changelogs/fix_chained_redirect_regression.yaml new file mode 100644 index 000000000000..4804674f05eb --- /dev/null +++ b/packages/go_router/pending_changelogs/fix_chained_redirect_regression.yaml @@ -0,0 +1,4 @@ +changelog: | + - Fixes chained top-level redirects not being fully resolved (e.g. `/ → /a → /b` stopping at `/a`). + - Fixes route-level redirects not triggering top-level redirect re-evaluation on the new location. +version: patch diff --git a/packages/go_router/test/on_enter_test.dart b/packages/go_router/test/on_enter_test.dart index 0656b16bfaf1..2b7a861fcc5a 100644 --- a/packages/go_router/test/on_enter_test.dart +++ b/packages/go_router/test/on_enter_test.dart @@ -1582,5 +1582,176 @@ void main() { expect(find.text('Home'), findsOneWidget); }, ); + + // Tests for onEnter interaction with chained redirects. + // These validate that onEnter works correctly when top-level and + // route-level redirects produce chains. + group('onEnter with chained redirects', () { + testWidgets('onEnter called once when top-level redirect chains', ( + WidgetTester tester, + ) async { + // Regression test for https://github.com/flutter/flutter/issues/178984 + // + // Top-level redirect: / -> /a -> /b + // onEnter should allow navigation and be called exactly once + // for the final resolved location. + var onEnterCallCount = 0; + var redirectCallCount = 0; + + router = GoRouter( + initialLocation: '/', + onEnter: + ( + BuildContext context, + GoRouterState current, + GoRouterState next, + GoRouter goRouter, + ) async { + onEnterCallCount++; + return const Allow(); + }, + redirect: (BuildContext context, GoRouterState state) { + redirectCallCount++; + if (state.matchedLocation == '/') { + return '/a'; + } + if (state.matchedLocation == '/a') { + return '/b'; + } + return null; + }, + routes: [ + GoRoute(path: '/', builder: (_, __) => const Text('Home')), + GoRoute(path: '/a', builder: (_, __) => const Text('A')), + GoRoute(path: '/b', builder: (_, __) => const Text('B')), + ], + ); + + await tester.pumpWidget(MaterialApp.router(routerConfig: router)); + await tester.pumpAndSettle(); + + // Chain should resolve to /b. + expect(router.routerDelegate.currentConfiguration.uri.toString(), '/b'); + expect(find.text('B'), findsOneWidget); + // onEnter should be called exactly once for the initial navigation. + expect(onEnterCallCount, 1); + // Redirect should be called for /, /a, and /b. + expect(redirectCallCount, 3); + }); + + testWidgets( + 'onEnter called once when route-level triggers top-level redirect', + (WidgetTester tester) async { + // Route-level on /src: /src -> /dst + // Top-level: /dst -> /final + // onEnter should be called exactly once. + var onEnterCallCount = 0; + + router = GoRouter( + initialLocation: '/src', + onEnter: + ( + BuildContext context, + GoRouterState current, + GoRouterState next, + GoRouter goRouter, + ) async { + onEnterCallCount++; + return const Allow(); + }, + redirect: (BuildContext context, GoRouterState state) { + if (state.matchedLocation == '/dst') { + return '/final'; + } + return null; + }, + routes: [ + GoRoute( + path: '/', + builder: (_, __) => const Text('Home'), + routes: [ + GoRoute( + path: 'src', + builder: (_, __) => const Text('Src'), + redirect: (BuildContext context, GoRouterState state) => + '/dst', + ), + ], + ), + GoRoute(path: '/dst', builder: (_, __) => const Text('Dst')), + GoRoute(path: '/final', builder: (_, __) => const Text('Final')), + ], + ); + + await tester.pumpWidget(MaterialApp.router(routerConfig: router)); + await tester.pumpAndSettle(); + + // Chain should resolve to /final. + expect( + router.routerDelegate.currentConfiguration.uri.toString(), + '/final', + ); + expect(find.text('Final'), findsOneWidget); + // onEnter should be called exactly once for the initial navigation. + expect(onEnterCallCount, 1); + }, + ); + + testWidgets('onEnter block prevents redirect chain evaluation', ( + WidgetTester tester, + ) async { + // onEnter blocks navigation to /a. + // Top-level redirect: /a -> /b (should never be evaluated). + var redirectCallCount = 0; + + router = GoRouter( + initialLocation: '/', + onEnter: + ( + BuildContext context, + GoRouterState current, + GoRouterState next, + GoRouter goRouter, + ) async { + // Block all navigation except to /. + if (next.uri.path == '/') { + return const Allow(); + } + return const Block.stop(); + }, + redirect: (BuildContext context, GoRouterState state) { + redirectCallCount++; + if (state.matchedLocation == '/a') { + return '/b'; + } + return null; + }, + routes: [ + GoRoute(path: '/', builder: (_, __) => const Text('Home')), + GoRoute(path: '/a', builder: (_, __) => const Text('A')), + GoRoute(path: '/b', builder: (_, __) => const Text('B')), + ], + ); + + await tester.pumpWidget(MaterialApp.router(routerConfig: router)); + await tester.pumpAndSettle(); + + // Initial navigation to / is allowed. + expect(find.text('Home'), findsOneWidget); + final redirectCountAfterInit = redirectCallCount; + + // Navigate to /a — should be blocked by onEnter. + router.go('/a'); + await tester.pumpAndSettle(); + + // Navigation was blocked; still on Home. + expect(find.text('Home'), findsOneWidget); + expect(find.text('A'), findsNothing); + expect(find.text('B'), findsNothing); + // Redirect function should NOT have been called for /a since + // onEnter blocked before redirects were evaluated. + expect(redirectCallCount, redirectCountAfterInit); + }); + }); }); } diff --git a/packages/go_router/test/redirect_chain_test.dart b/packages/go_router/test/redirect_chain_test.dart new file mode 100644 index 000000000000..ce8caa8c7f74 --- /dev/null +++ b/packages/go_router/test/redirect_chain_test.dart @@ -0,0 +1,630 @@ +// Copyright 2013 The Flutter Authors +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +import 'package:flutter/material.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:go_router/go_router.dart'; + +import 'test_helpers.dart'; + +// Tests for chained redirect behavior. +// +// These tests validate that top-level redirects are fully re-evaluated +// when they produce a new location, and that route-level redirects +// trigger top-level re-evaluation on the new location. +void main() { + group('chained redirects', () { + testWidgets('top-level redirect chain', (WidgetTester tester) async { + // Regression test for https://github.com/flutter/flutter/issues/178984 + // + // Top-level redirect: / -> /a -> /b + // Expected: navigating to / should end up at /b + final redirectLog = []; + final GoRouter router = await createRouter( + [ + GoRoute( + path: '/', + builder: (BuildContext context, GoRouterState state) => + const HomeScreen(), + ), + GoRoute( + path: '/a', + builder: (BuildContext context, GoRouterState state) => + const Page1Screen(), + ), + GoRoute( + path: '/b', + builder: (BuildContext context, GoRouterState state) => + const Page2Screen(), + ), + ], + tester, + redirect: (BuildContext context, GoRouterState state) { + redirectLog.add(state.matchedLocation); + if (state.matchedLocation == '/') { + return '/a'; + } + if (state.matchedLocation == '/a') { + return '/b'; + } + return null; + }, + ); + + expect(router.routerDelegate.currentConfiguration.uri.toString(), '/b'); + expect(find.byType(Page2Screen), findsOneWidget); + // Redirect evaluated for /, /a, and /b. + expect(redirectLog, ['/', '/a', '/b']); + }); + + testWidgets('top-level redirect chain with three hops', ( + WidgetTester tester, + ) async { + final GoRouter router = await createRouter( + [ + GoRoute( + path: '/', + builder: (BuildContext context, GoRouterState state) => + const HomeScreen(), + ), + GoRoute( + path: '/step1', + builder: (BuildContext context, GoRouterState state) => + const DummyScreen(), + ), + GoRoute( + path: '/step2', + builder: (BuildContext context, GoRouterState state) => + const DummyScreen(), + ), + GoRoute( + path: '/step3', + builder: (BuildContext context, GoRouterState state) => + const LoginScreen(), + ), + ], + tester, + redirect: (BuildContext context, GoRouterState state) { + return switch (state.matchedLocation) { + '/' => '/step1', + '/step1' => '/step2', + '/step2' => '/step3', + _ => null, + }; + }, + ); + + expect( + router.routerDelegate.currentConfiguration.uri.toString(), + '/step3', + ); + expect(find.byType(LoginScreen), findsOneWidget); + }); + + testWidgets('async top-level redirect chain', (WidgetTester tester) async { + final GoRouter router = await createRouter( + [ + GoRoute( + path: '/', + builder: (BuildContext context, GoRouterState state) => + const HomeScreen(), + ), + GoRoute( + path: '/a', + builder: (BuildContext context, GoRouterState state) => + const DummyScreen(), + ), + GoRoute( + path: '/b', + builder: (BuildContext context, GoRouterState state) => + const LoginScreen(), + ), + ], + tester, + redirect: (BuildContext context, GoRouterState state) async { + // Simulate an async operation (e.g., checking auth status). + await Future.delayed(Duration.zero); + if (state.matchedLocation == '/') { + return '/a'; + } + if (state.matchedLocation == '/a') { + return '/b'; + } + return null; + }, + ); + + await tester.pumpAndSettle(); + + expect(router.routerDelegate.currentConfiguration.uri.toString(), '/b'); + expect(find.byType(LoginScreen), findsOneWidget); + }); + + testWidgets('top-level redirect chain loop detection', ( + WidgetTester tester, + ) async { + // Redirect loop: / -> /a -> /b -> /a (loop) + await createRouter( + [ + GoRoute( + path: '/', + builder: (BuildContext context, GoRouterState state) => + const HomeScreen(), + ), + GoRoute( + path: '/a', + builder: (BuildContext context, GoRouterState state) => + const DummyScreen(), + ), + GoRoute( + path: '/b', + builder: (BuildContext context, GoRouterState state) => + const DummyScreen(), + ), + ], + tester, + redirect: (BuildContext context, GoRouterState state) { + return switch (state.matchedLocation) { + '/' => '/a', + '/a' => '/b', + '/b' => '/a', // Loop: /a -> /b -> /a + _ => null, + }; + }, + errorBuilder: (BuildContext context, GoRouterState state) => + TestErrorScreen(state.error!), + ); + + expect(find.byType(TestErrorScreen), findsOneWidget); + final TestErrorScreen screen = tester.widget( + find.byType(TestErrorScreen), + ); + expect( + (screen.ex as GoException).message, + startsWith('redirect loop detected'), + ); + }); + + testWidgets('top-level redirect chain loop to initial location', ( + WidgetTester tester, + ) async { + // Redirect loop returning to initial location: / -> /a -> / + await createRouter( + [ + GoRoute( + path: '/', + builder: (BuildContext context, GoRouterState state) => + const HomeScreen(), + ), + GoRoute( + path: '/a', + builder: (BuildContext context, GoRouterState state) => + const DummyScreen(), + ), + ], + tester, + redirect: (BuildContext context, GoRouterState state) { + return switch (state.matchedLocation) { + '/' => '/a', + '/a' => '/', // Loop back to initial. + _ => null, + }; + }, + errorBuilder: (BuildContext context, GoRouterState state) => + TestErrorScreen(state.error!), + ); + + expect(find.byType(TestErrorScreen), findsOneWidget); + final TestErrorScreen screen = tester.widget( + find.byType(TestErrorScreen), + ); + expect( + (screen.ex as GoException).message, + startsWith('redirect loop detected'), + ); + }); + + testWidgets('top-level redirect chain into route-level redirect', ( + WidgetTester tester, + ) async { + // Top-level: / -> /intermediate + // Route-level on /intermediate: /intermediate -> /final + final GoRouter router = await createRouter( + [ + GoRoute( + path: '/', + builder: (BuildContext context, GoRouterState state) => + const HomeScreen(), + ), + GoRoute( + path: '/intermediate', + builder: (BuildContext context, GoRouterState state) => + const DummyScreen(), + redirect: (BuildContext context, GoRouterState state) => '/final', + ), + GoRoute( + path: '/final', + builder: (BuildContext context, GoRouterState state) => + const LoginScreen(), + ), + ], + tester, + redirect: (BuildContext context, GoRouterState state) { + if (state.matchedLocation == '/') { + return '/intermediate'; + } + return null; + }, + ); + + expect( + router.routerDelegate.currentConfiguration.uri.toString(), + '/final', + ); + expect(find.byType(LoginScreen), findsOneWidget); + }); + + testWidgets('route-level redirect triggers top-level redirect', ( + WidgetTester tester, + ) async { + // Route-level on /src: /src -> /dst + // Top-level: /dst -> /final + final topRedirectLog = []; + final GoRouter router = await createRouter( + [ + GoRoute( + path: '/', + builder: (BuildContext context, GoRouterState state) => + const HomeScreen(), + routes: [ + GoRoute( + path: 'src', + builder: (BuildContext context, GoRouterState state) => + const DummyScreen(), + redirect: (BuildContext context, GoRouterState state) => '/dst', + ), + ], + ), + GoRoute( + path: '/dst', + builder: (BuildContext context, GoRouterState state) => + const DummyScreen(), + ), + GoRoute( + path: '/final', + builder: (BuildContext context, GoRouterState state) => + const LoginScreen(), + ), + ], + tester, + initialLocation: '/src', + redirect: (BuildContext context, GoRouterState state) { + topRedirectLog.add(state.matchedLocation); + if (state.matchedLocation == '/dst') { + return '/final'; + } + return null; + }, + ); + + expect( + router.routerDelegate.currentConfiguration.uri.toString(), + '/final', + ); + expect(find.byType(LoginScreen), findsOneWidget); + // Top-level redirect was evaluated on /dst (after route-level redirect). + expect(topRedirectLog, contains('/dst')); + }); + + testWidgets('top-level redirect returns null after first redirect', ( + WidgetTester tester, + ) async { + // The most common pattern: single redirect, then null on re-evaluation. + var callCount = 0; + final GoRouter router = await createRouter( + [ + GoRoute( + path: '/', + builder: (BuildContext context, GoRouterState state) => + const HomeScreen(), + ), + GoRoute( + path: '/login', + builder: (BuildContext context, GoRouterState state) => + const LoginScreen(), + ), + ], + tester, + redirect: (BuildContext context, GoRouterState state) { + callCount++; + if (state.matchedLocation == '/') { + return '/login'; + } + return null; + }, + ); + + expect( + router.routerDelegate.currentConfiguration.uri.toString(), + '/login', + ); + expect(find.byType(LoginScreen), findsOneWidget); + // Redirect is called twice: once for /, once for /login. + expect(callCount, 2); + }); + + testWidgets('top-level redirect chain respects redirect limit', ( + WidgetTester tester, + ) async { + // Endless chain: /0 -> /1 -> /2 -> ... with a low limit. + await createRouter( + [ + for (int i = 0; i <= 20; i++) + GoRoute( + path: '/$i', + builder: (BuildContext context, GoRouterState state) => + const DummyScreen(), + ), + ], + tester, + initialLocation: '/0', + redirect: (BuildContext context, GoRouterState state) { + final RegExpMatch? match = RegExp( + r'^/(\d+)$', + ).firstMatch(state.matchedLocation); + if (match != null) { + final int current = int.parse(match.group(1)!); + return '/${current + 1}'; + } + return null; + }, + errorBuilder: (BuildContext context, GoRouterState state) => + TestErrorScreen(state.error!), + ); + + expect(find.byType(TestErrorScreen), findsOneWidget); + }); + + testWidgets('top-level redirect chain succeeds at exact redirect limit', ( + WidgetTester tester, + ) async { + // Chain of 2 redirects: / -> /a -> /b. With limit=2, this is exactly + // at the boundary (2 entries added to redirectHistory). + final GoRouter router = await createRouter( + [ + GoRoute( + path: '/', + builder: (BuildContext context, GoRouterState state) => + const HomeScreen(), + ), + GoRoute( + path: '/a', + builder: (BuildContext context, GoRouterState state) => + const DummyScreen(), + ), + GoRoute( + path: '/b', + builder: (BuildContext context, GoRouterState state) => + const LoginScreen(), + ), + ], + tester, + redirect: (BuildContext context, GoRouterState state) { + return switch (state.matchedLocation) { + '/' => '/a', + '/a' => '/b', + _ => null, + }; + }, + redirectLimit: 2, + ); + + // Exactly 2 redirects with limit=2 should succeed. + expect(router.routerDelegate.currentConfiguration.uri.toString(), '/b'); + expect(find.byType(LoginScreen), findsOneWidget); + }); + + testWidgets( + 'top-level redirect chain fails when exceeding redirect limit', + (WidgetTester tester) async { + // Chain of 2 redirects: / -> /a -> /b. With limit=1, the second + // redirect exceeds the limit and should error. + await createRouter( + [ + GoRoute( + path: '/', + builder: (BuildContext context, GoRouterState state) => + const HomeScreen(), + ), + GoRoute( + path: '/a', + builder: (BuildContext context, GoRouterState state) => + const DummyScreen(), + ), + GoRoute( + path: '/b', + builder: (BuildContext context, GoRouterState state) => + const LoginScreen(), + ), + ], + tester, + redirect: (BuildContext context, GoRouterState state) { + return switch (state.matchedLocation) { + '/' => '/a', + '/a' => '/b', + _ => null, + }; + }, + redirectLimit: 1, + errorBuilder: (BuildContext context, GoRouterState state) => + TestErrorScreen(state.error!), + ); + + // 2 redirects with limit=1 should error. + expect(find.byType(TestErrorScreen), findsOneWidget); + final TestErrorScreen screen = tester.widget( + find.byType(TestErrorScreen), + ); + expect( + (screen.ex as GoException).message, + startsWith('too many redirects'), + ); + }, + ); + + testWidgets('top-level and route-level redirects share redirect limit', ( + WidgetTester tester, + ) async { + // Top-level: / -> /a (1 redirect) + // Route-level on /a: /a -> /b (1 redirect) + // Route-level on /b: /b -> /c (1 redirect) + // Total: 3 redirects, limit=2 → should error. + await createRouter( + [ + GoRoute( + path: '/', + builder: (BuildContext context, GoRouterState state) => + const HomeScreen(), + ), + GoRoute( + path: '/a', + builder: (BuildContext context, GoRouterState state) => + const DummyScreen(), + redirect: (BuildContext context, GoRouterState state) => '/b', + ), + GoRoute( + path: '/b', + builder: (BuildContext context, GoRouterState state) => + const DummyScreen(), + redirect: (BuildContext context, GoRouterState state) => '/c', + ), + GoRoute( + path: '/c', + builder: (BuildContext context, GoRouterState state) => + const LoginScreen(), + ), + ], + tester, + redirect: (BuildContext context, GoRouterState state) { + if (state.matchedLocation == '/') { + return '/a'; + } + return null; + }, + redirectLimit: 2, + errorBuilder: (BuildContext context, GoRouterState state) => + TestErrorScreen(state.error!), + ); + + // Combined chain of 3 redirects exceeds the shared limit of 2. + expect(find.byType(TestErrorScreen), findsOneWidget); + final TestErrorScreen screen = tester.widget( + find.byType(TestErrorScreen), + ); + expect( + (screen.ex as GoException).message, + startsWith('too many redirects'), + ); + }); + + testWidgets('async top-level redirect chain loop detection', ( + WidgetTester tester, + ) async { + // Async redirect loop: / -> /a -> /b -> /a (loop) + await createRouter( + [ + GoRoute( + path: '/', + builder: (BuildContext context, GoRouterState state) => + const HomeScreen(), + ), + GoRoute( + path: '/a', + builder: (BuildContext context, GoRouterState state) => + const DummyScreen(), + ), + GoRoute( + path: '/b', + builder: (BuildContext context, GoRouterState state) => + const DummyScreen(), + ), + ], + tester, + redirect: (BuildContext context, GoRouterState state) async { + await Future.delayed(Duration.zero); + return switch (state.matchedLocation) { + '/' => '/a', + '/a' => '/b', + '/b' => '/a', // Loop + _ => null, + }; + }, + errorBuilder: (BuildContext context, GoRouterState state) => + TestErrorScreen(state.error!), + ); + + await tester.pumpAndSettle(); + + expect(find.byType(TestErrorScreen), findsOneWidget); + final TestErrorScreen screen = tester.widget( + find.byType(TestErrorScreen), + ); + expect( + (screen.ex as GoException).message, + startsWith('redirect loop detected'), + ); + }); + + testWidgets('top-level redirect chain works with router.go()', ( + WidgetTester tester, + ) async { + // Start at /, no redirect from /. Navigate to /a which chains to /c. + final GoRouter router = await createRouter( + [ + GoRoute( + path: '/', + builder: (BuildContext context, GoRouterState state) => + const HomeScreen(), + ), + GoRoute( + path: '/a', + builder: (BuildContext context, GoRouterState state) => + const DummyScreen(), + ), + GoRoute( + path: '/b', + builder: (BuildContext context, GoRouterState state) => + const Page1Screen(), + ), + GoRoute( + path: '/c', + builder: (BuildContext context, GoRouterState state) => + const LoginScreen(), + ), + ], + tester, + redirect: (BuildContext context, GoRouterState state) { + if (state.matchedLocation == '/a') { + return '/b'; + } + if (state.matchedLocation == '/b') { + return '/c'; + } + return null; + }, + ); + + // Initial location is / (no redirect for /). + expect(router.routerDelegate.currentConfiguration.uri.toString(), '/'); + expect(find.byType(HomeScreen), findsOneWidget); + + // Navigate to /a — should chain /a -> /b -> /c. + router.go('/a'); + await tester.pumpAndSettle(); + + expect(router.routerDelegate.currentConfiguration.uri.toString(), '/c'); + expect(find.byType(LoginScreen), findsOneWidget); + }); + }); +}