[go_router] Fixes deep links with no path #6447

kforjan · 2024-04-01T18:05:31Z

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:

[go_router] Page Not Found when cold opening an app from a deep link with no path flutter#133928

Pre-launch Checklist

…ters

kforjan · 2024-04-01T21:24:21Z

@Hangyujin @johnpryan I have noticed that the code owner who is added as a reviewer hasn't been active for some time. Could you take a look at this, or do you know who could provide some feedback? Thank you!

hannah-hyj · 2024-04-02T19:28:00Z

@kforjan I will take a look

hannah-hyj

Thanks for your contribution. This looks great to me, the fix looks clean and also thank you for your detailed description and tests.

hannah-hyj · 2024-04-04T22:27:00Z

packages/go_router/lib/src/path_utils.dart

    throw GoException('Location cannot be empty.');
  }
  String canon = Uri.parse(loc).toString();
+  final Uri uri = Uri.parse(canon);


line128 should be after line 129

I can think of a corner test case /profile/?

hannah-hyj · 2024-04-04T22:34:57Z

packages/go_router/lib/src/path_utils.dart

  // / => /
  // /login?from=/ => login?from=/
-  canon = canon.endsWith('/') && canon != '/' && !canon.contains('?')
+  canon = uri.path.endsWith('/') && uri.path != '/' && !uri.hasQuery


add && !uri.hasFragment

hannah-hyj · 2024-04-04T22:41:45Z

packages/go_router/lib/src/parser.dart

+      initialMatches = configuration.findMatch(newUri, extra: state.extra);
+    } else {
+      initialMatches = configuration.findMatch(routeInformation.uri.toString(),
+          extra: state.extra);


nit: add comma

kforjan · 2024-04-04T23:31:00Z

@Hangyujin Thank you for the feedback! I appreciate it very much and I am really glad you find it helpful. I made all the changes you proposed and added a few more tests for canonicalUri - now all edge cases should be covered.

chunhtai

LGTM

DennisKragekjaer · 2024-04-08T22:27:10Z

DennisKragekjaer · 2024-04-08T22:28:05Z

So I still see an issue, also after update to new version 13.2.3

chunhtai · 2024-04-08T22:30:01Z

Since this is merged, please file a new issue with repro.

kforjan · 2024-04-09T00:25:36Z

Hello @DennisKragekjaer! I have just tried using the exact same url with a same custom scheme setup like the one in your example. It worked for me and I couldn't reproduce your problem. Please note that /link is the path which you're trying to open in your example, so you have to include a GoRoute with the path /link in the routes parameter which you provide to your GoRouter instance. If this doesn't help - please open an issue with reproduction steps.

Note for @chunhtai: while I investigated this, I noticed an edge case that I missed in my original implementation. When using custom schemes uri.origin cannot be used. After parsing a custom scheme with Uri.parse() calling uri.origin will produce the following Exception:

Bad state: Origin is only applicable to schemes http and https: customScheme://example

It doesn't break the flow or affect the app launch in any way, rather it just produces this error message in the debug console. It happens only with custom schemes with no path, for example:
customScheme://example will have an exception in the console while customScheme://example/ won't have it.
It isn't breaking for those cases, but it should be fixed anyway. One way to do it is to use uri.schema and uri.host to construct the uri, since both are available for custom schemas and http/https, but I am not sure if there are any other cases which I am missing where schema and host wont be in the same format as schema://host

DennisKragekjaer · 2024-04-09T13:31:21Z

@kforjan Thanks for looking into this. The thing is I have no idea where this "dk.kragekjaer.mille...." comes from.

)

flutter/packages@6b4d8b6...17f55d3 2024-04-09 [email protected] Release compileSdk changes (flutter/packages#6491) 2024-04-08 [email protected] [camera_android] Remove `TestUtils.java` (flutter/packages#6490) 2024-04-08 [email protected] Roll Flutter from 98d23f7 to 533d04d (12 revisions) (flutter/packages#6488) 2024-04-08 [email protected] [go_router_builder] Add `restorationScopeId` to `ShellRouteData` (flutter/packages#6238) 2024-04-08 [email protected] [go_router] Fixes deep links with no path (flutter/packages#6447) 2024-04-08 [email protected] [in_app_purchase] Convert Android data objects to Pigeon (flutter/packages#6453) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

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

kforjan added 5 commits April 1, 2024 18:06

Preserve trailing slash for root URLs in canonicalUri

3c788b9

Add tests for preserving trailing slash in root URLs

7df6b6c

Ensure trailing slash for empty paths with query and/or fragment

1735d96

Add and modify tests to include empty path, query and fragment parame…

d2f8e52

…ters

Update changelog.md and pubspec.yaml

f576b70

kforjan requested a review from chunhtai as a code owner April 1, 2024 18:05

github-actions bot added the p: go_router label Apr 1, 2024

kforjan and others added 2 commits April 1, 2024 23:08

Merge branch 'main' into fix-deeplink-root-route-launch

c37723c

Update changelog for clarification

cd5f14b

Merge branch 'main' into fix-deeplink-root-route-launch

e6e44f0

Merge branch 'main' into fix-deeplink-root-route-launch

25196df

hannah-hyj approved these changes Apr 4, 2024

View reviewed changes

hannah-hyj requested a review from johnpryan April 4, 2024 22:56

kforjan added 2 commits April 5, 2024 01:25

minore fixes, fix parsing placement, add fragment check

e9b0887

cover canonical url edge cases

10d34c8

kforjan and others added 2 commits April 5, 2024 01:57

fix formatting

80ef126

Merge branch 'main' into fix-deeplink-root-route-launch

f43cc5f

chunhtai approved these changes Apr 8, 2024

View reviewed changes

chunhtai added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 8, 2024

auto-submit bot merged commit fef7ac6 into flutter:main Apr 8, 2024

engine-flutter-autoroll mentioned this pull request Apr 9, 2024

Roll Packages from 6b4d8b6cdb39 to 17f55d3ac94f (6 revisions) flutter/flutter#146508

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 9, 2024

fef7ac6 [go_router] Fixes deep links with no path (flutter/packages#6447

e5e199c

)

[go_router] Fixes deep links with no path #6447

[go_router] Fixes deep links with no path #6447

Uh oh!

Conversation

kforjan commented Apr 1, 2024

Pre-launch Checklist

Uh oh!

kforjan commented Apr 1, 2024

Uh oh!

hannah-hyj commented Apr 2, 2024

Uh oh!

hannah-hyj left a comment

Choose a reason for hiding this comment

Uh oh!

hannah-hyj Apr 4, 2024

Choose a reason for hiding this comment

Uh oh!

hannah-hyj Apr 4, 2024

Choose a reason for hiding this comment

Uh oh!

hannah-hyj Apr 4, 2024

Choose a reason for hiding this comment

Uh oh!

kforjan commented Apr 4, 2024

Uh oh!

chunhtai left a comment

Choose a reason for hiding this comment

Uh oh!

DennisKragekjaer commented Apr 8, 2024

Uh oh!

DennisKragekjaer commented Apr 8, 2024

Uh oh!

chunhtai commented Apr 8, 2024

Uh oh!

kforjan commented Apr 9, 2024

Uh oh!

DennisKragekjaer commented Apr 9, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants