Skip to content

Commit

Permalink
[go_router] Fixes deep links with no path (#6447)
Browse files Browse the repository at this point in the history
This PR addresses an issue where root deep links did not correctly navigate to '/'. The problem originated from the assumption that only paths (like '/', '/details', etc.) would be passed to the `canonicalUri` method in `path_utils.dart`. However, in the case of deep links, the full URL (including the scheme and domain) would be passed, causing the trailing slash to be incorrectly removed for root URLs.

The first part of the fix modifies the `canonicalUri` method to check the actual path using `uri.path`. This ensures that the trailing slash is preserved for root URLs, even when the full URL is passed to the method. Importantly, even if '/' or '/details' is passed as a URI to `canonicalUri`, `uri.path` will still return '/' or '/details', ensuring consistent behavior.

The second part of the fix modifies the `parseRouteInformationWithDependencies` function in `parser.dart` to correctly handle URLs with query parameters or fragments. Previously, the trailing slash was added after the query parameters or fragments, which is incorrect. The fix ensures that the trailing slash is added immediately after the base URL, before any query parameters or fragments.

Preserving the trailing slash for root URLs is crucial because the `_matchByNavigatorKey` method in `match.dart` uses `uri.path` as the `remainingLocation` parameter. (why is that method relevant? because `parseRouteInformationWithDependencies` uses `findMatch`, which calls `_getLocRouteMatches`, which calls `match` and it calls `_matchByNavigatorKey`). If the trailing slash is removed from the root deep link URL, `uri.path` will not return '/', and the URL will not match any routes in the Go router.

To validate these changes, new tests have been added. In `path_utils_test.dart`, tests have been added to verify that the trailing slash is not removed from a URL that is not just the path. In `parser_test.dart`, a new test has been added to verify that a URI with an empty path is correctly parsed. This test covers the case where `routeInformation.uri.hasEmptyPath` is true, which was not previously tested.

Issues fixed:
 - flutter/flutter#133928
  • Loading branch information
kforjan committed Apr 8, 2024
1 parent 583f8dd commit fef7ac6
Show file tree
Hide file tree
Showing 6 changed files with 83 additions and 11 deletions.
4 changes: 4 additions & 0 deletions packages/go_router/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## 13.2.3

- Fixes an issue where deep links without path caused an exception

## 13.2.2

- Fixes restoreRouteInformation issue when GoRouter.optionURLReflectsImperativeAPIs is true and the last match is ShellRouteMatch
Expand Down
23 changes: 18 additions & 5 deletions packages/go_router/lib/src/parser.dart
Original file line number Diff line number Diff line change
Expand Up @@ -79,11 +79,24 @@ class GoRouteInformationParser extends RouteInformationParser<RouteMatchList> {
}

late final RouteMatchList initialMatches;
initialMatches = configuration.findMatch(
routeInformation.uri.path.isEmpty
? '${routeInformation.uri}/'
: routeInformation.uri.toString(),
extra: state.extra);
if (routeInformation.uri.hasEmptyPath) {
String newUri = '${routeInformation.uri.origin}/';
if (routeInformation.uri.hasQuery) {
newUri += '?${routeInformation.uri.query}';
}
if (routeInformation.uri.hasFragment) {
newUri += '#${routeInformation.uri.fragment}';
}
initialMatches = configuration.findMatch(
newUri,
extra: state.extra,
);
} else {
initialMatches = configuration.findMatch(
routeInformation.uri.toString(),
extra: state.extra,
);
}
if (initialMatches.isError) {
log('No initial matches: ${routeInformation.uri.path}');
}
Expand Down
9 changes: 6 additions & 3 deletions packages/go_router/lib/src/path_utils.dart
Original file line number Diff line number Diff line change
Expand Up @@ -126,19 +126,22 @@ String canonicalUri(String loc) {
}
String canon = Uri.parse(loc).toString();
canon = canon.endsWith('?') ? canon.substring(0, canon.length - 1) : canon;
final Uri uri = Uri.parse(canon);

// remove trailing slash except for when you shouldn't, e.g.
// /profile/ => /profile
// / => /
// /login?from=/ => login?from=/
canon = canon.endsWith('/') && canon != '/' && !canon.contains('?')
// /login?from=/ => /login?from=/
canon = uri.path.endsWith('/') &&
uri.path != '/' &&
!uri.hasQuery &&
!uri.hasFragment
? canon.substring(0, canon.length - 1)
: canon;

// replace '/?', except for first occurrence, from path only
// /login/?from=/ => /login?from=/
// /?from=/ => /?from=/
final Uri uri = Uri.parse(canon);
final int pathStartIndex = uri.host.isNotEmpty
? uri.toString().indexOf(uri.host) + uri.host.length
: uri.hasScheme
Expand Down
2 changes: 1 addition & 1 deletion packages/go_router/pubspec.yaml
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
name: go_router
description: A declarative router for Flutter based on Navigation 2 supporting
deep linking, data-driven routes and more
version: 13.2.2
version: 13.2.3
repository: https://github.com/flutter/packages/tree/main/packages/go_router
issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A%22p%3A+go_router%22

Expand Down
47 changes: 45 additions & 2 deletions packages/go_router/test/parser_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -82,13 +82,54 @@ void main() {
});

testWidgets(
"GoRouteInformationParser can parse deeplink route and maintain uri's scheme and host",
"GoRouteInformationParser can parse deeplink root route and maintain uri's scheme, host, query and fragment",
(WidgetTester tester) async {
const String expectedScheme = 'https';
const String expectedHost = 'www.example.com';
const String expectedQuery = 'abc=def';
const String expectedFragment = 'abc';
const String expectedUriString =
'$expectedScheme://$expectedHost/?$expectedQuery#$expectedFragment';
final List<GoRoute> routes = <GoRoute>[
GoRoute(
path: '/',
builder: (_, __) => const Placeholder(),
),
];
final GoRouteInformationParser parser = await createParser(
tester,
routes: routes,
redirectLimit: 100,
redirect: (_, __) => null,
);

final BuildContext context = tester.element(find.byType(Router<Object>));

final RouteMatchList matchesObj =
await parser.parseRouteInformationWithDependencies(
createRouteInformation(expectedUriString), context);
final List<RouteMatchBase> matches = matchesObj.matches;
expect(matches.length, 1);
expect(matchesObj.uri.toString(), expectedUriString);
expect(matchesObj.uri.scheme, expectedScheme);
expect(matchesObj.uri.host, expectedHost);
expect(matchesObj.uri.query, expectedQuery);
expect(matchesObj.uri.fragment, expectedFragment);

expect(matches[0].matchedLocation, '/');
expect(matches[0].route, routes[0]);
});

testWidgets(
"GoRouteInformationParser can parse deeplink route with a path and maintain uri's scheme, host, query and fragment",
(WidgetTester tester) async {
const String expectedScheme = 'https';
const String expectedHost = 'www.example.com';
const String expectedPath = '/abc';
const String expectedQuery = 'abc=def';
const String expectedFragment = 'abc';
const String expectedUriString =
'$expectedScheme://$expectedHost$expectedPath';
'$expectedScheme://$expectedHost$expectedPath?$expectedQuery#$expectedFragment';
final List<GoRoute> routes = <GoRoute>[
GoRoute(
path: '/',
Expand Down Expand Up @@ -119,6 +160,8 @@ void main() {
expect(matchesObj.uri.scheme, expectedScheme);
expect(matchesObj.uri.host, expectedHost);
expect(matchesObj.uri.path, expectedPath);
expect(matchesObj.uri.query, expectedQuery);
expect(matchesObj.uri.fragment, expectedFragment);

expect(matches[0].matchedLocation, '/');
expect(matches[0].route, routes[0]);
Expand Down
9 changes: 9 additions & 0 deletions packages/go_router/test/path_utils_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,15 @@ void main() {
verify('/a/', '/a');
verify('/', '/');
verify('/a/b/', '/a/b');
verify('https://www.example.com/', 'https://www.example.com/');
verify('https://www.example.com/a', 'https://www.example.com/a');
verify('https://www.example.com/a/', 'https://www.example.com/a');
verify('https://www.example.com/a/b/', 'https://www.example.com/a/b');
verify('https://www.example.com/?', 'https://www.example.com/');
verify('https://www.example.com/?a=b', 'https://www.example.com/?a=b');
verify('https://www.example.com/?a=/', 'https://www.example.com/?a=/');
verify('https://www.example.com/a/?b=c', 'https://www.example.com/a?b=c');
verify('https://www.example.com/#a/', 'https://www.example.com/#a/');

expect(() => canonicalUri('::::'), throwsA(isA<FormatException>()));
expect(() => canonicalUri(''), throwsA(anything));
Expand Down

0 comments on commit fef7ac6

Please sign in to comment.