From 986a6338a0d8a8d37dd9691715d083ccdb458a80 Mon Sep 17 00:00:00 2001 From: Bungeefan Date: Sat, 9 Mar 2024 20:30:02 +0100 Subject: [PATCH 1/2] [fix] incorrect handling of uris (e.g. deep links) --- package/lib/src/beam_location.dart | 11 ++---- package/lib/src/beamer_delegate.dart | 5 +-- package/lib/src/utils.dart | 16 ++++++--- package/test/utils_test.dart | 51 ++++++++++++++++++++++++++++ 4 files changed, 68 insertions(+), 15 deletions(-) diff --git a/package/lib/src/beam_location.dart b/package/lib/src/beam_location.dart index 1166317..15348d0 100644 --- a/package/lib/src/beam_location.dart +++ b/package/lib/src/beam_location.dart @@ -520,14 +520,9 @@ class RoutesBeamLocation extends BeamLocation { final matched = {}; var overrideNotFound = false; - if (routeInformation.uri.toString() != '/' && - (routeInformation.uri.toString().endsWith('/'))) { - final location = routeInformation.uri.toString(); - routeInformation = routeInformation.copyWith( - location: location.substring(0, location.length - 1), - ); - } - final uri = routeInformation.uri; + final uri = routeInformation.uri.replace( + path: Utils.trimmed(routeInformation.uri.path), + ); for (final route in routes) { if (route is String) { diff --git a/package/lib/src/beamer_delegate.dart b/package/lib/src/beamer_delegate.dart index 11d1023..95047ef 100644 --- a/package/lib/src/beamer_delegate.dart +++ b/package/lib/src/beamer_delegate.dart @@ -76,6 +76,7 @@ class BeamerDelegate extends RouterDelegate /// This is not null only if multiple [Beamer]s are used; /// `*App.router` and at least one more [Beamer] in the Widget tree. BeamerDelegate? get parent => _parent; + set parent(BeamerDelegate? parent) { if (parent == null && _parent != null) { _parent!.removeListener(_updateFromParent); @@ -813,7 +814,7 @@ class BeamerDelegate extends RouterDelegate @override SynchronousFuture setNewRoutePath(RouteInformation configuration) { - if (configuration.uri.toString() == '/' && initialPath != '/') { + if (configuration.uri.path == '/' && initialPath != '/') { configuration = configuration.copyWith(location: initialPath); } update(configuration: configuration); @@ -907,7 +908,7 @@ class BeamerDelegate extends RouterDelegate ); } - if (clearBeamingHistoryOn.contains(configuration.uri.toString())) { + if (clearBeamingHistoryOn.contains(configuration.uri.path)) { _clearBeamingHistory(); } } diff --git a/package/lib/src/utils.dart b/package/lib/src/utils.dart index 846ec02..3cade38 100644 --- a/package/lib/src/utils.dart +++ b/package/lib/src/utils.dart @@ -252,12 +252,18 @@ abstract class Utils { RouteInformation current, RouteInformation incoming, ) { - final incomingLocation = incoming.uri.toString(); - if (!incomingLocation.startsWith('/')) { + if (!incoming.uri.hasAbsolutePath && !incoming.uri.hasEmptyPath) { + String currentPath = current.uri.path.endsWith('/') + ? current.uri.path + : '${current.uri.path}/'; return current.copyWith( - location: current.uri.toString().endsWith('/') - ? '${current.uri}$incomingLocation' - : '${current.uri}/$incomingLocation', + location: current.uri + .replace( + path: currentPath + incoming.uri.path, + query: incoming.uri.hasQuery ? incoming.uri.query : null, + fragment: incoming.uri.hasFragment ? incoming.uri.fragment : null, + ) + .toString(), state: incoming.state, ); } diff --git a/package/test/utils_test.dart b/package/test/utils_test.dart index dde3748..6331d5d 100644 --- a/package/test/utils_test.dart +++ b/package/test/utils_test.dart @@ -146,6 +146,39 @@ void main() { .toString(), '/incoming', ); + expect( + Utils.maybeAppend(current, RouteInformation(uri: Uri.parse(''))) + .uri + .toString(), + '', + ); + expect( + Utils.maybeAppend(current, RouteInformation(uri: Uri.parse('/'))) + .uri + .toString(), + '/', + ); + expect( + Utils.maybeAppend(current, + RouteInformation(uri: Uri.parse('example://app/incoming'))) + .uri + .toString(), + 'example://app/incoming', + ); + expect( + Utils.maybeAppend(current, + RouteInformation(uri: Uri.parse('example://app/incoming'))) + .uri + .toString(), + 'example://app/incoming', + ); + expect( + Utils.maybeAppend( + current, RouteInformation(uri: Uri.parse('//app/incoming'))) + .uri + .toString(), + '//app/incoming', + ); }); test('Appending with new routeState', () { @@ -167,6 +200,24 @@ void main() { .state, 42, ); + expect( + Utils.maybeAppend( + current, + RouteInformation( + uri: Uri.parse('example://app/incoming'), + state: 42, + )).state, + 42, + ); + expect( + Utils.maybeAppend( + current, + RouteInformation( + uri: Uri.parse('//app/incoming'), + state: 42, + )).state, + 42, + ); }); }); From c70aca799f2f20a0e002a5e84a60cf0b3b8eaf4c Mon Sep 17 00:00:00 2001 From: Bungeefan Date: Sat, 9 Mar 2024 20:35:34 +0100 Subject: [PATCH 2/2] [refactor] further migrate to correctly handle uris --- package/lib/src/beam_location.dart | 4 +- package/lib/src/beam_page.dart | 5 +- package/lib/src/beam_state.dart | 9 +-- package/lib/src/beamer_delegate.dart | 7 +- package/lib/src/utils.dart | 66 +++++++++---------- package/test/utils_test.dart | 95 +++++++++++++++++----------- 6 files changed, 99 insertions(+), 87 deletions(-) diff --git a/package/lib/src/beam_location.dart b/package/lib/src/beam_location.dart index 15348d0..84c8029 100644 --- a/package/lib/src/beam_location.dart +++ b/package/lib/src/beam_location.dart @@ -520,9 +520,7 @@ class RoutesBeamLocation extends BeamLocation { final matched = {}; var overrideNotFound = false; - final uri = routeInformation.uri.replace( - path: Utils.trimmed(routeInformation.uri.path), - ); + final uri = Utils.removeTrailingSlash(routeInformation.uri); for (final route in routes) { if (route is String) { diff --git a/package/lib/src/beam_page.dart b/package/lib/src/beam_page.dart index 31aa105..d7cf97c 100644 --- a/package/lib/src/beam_page.dart +++ b/package/lib/src/beam_page.dart @@ -1,9 +1,8 @@ +import 'package:beamer/beamer.dart'; import 'package:beamer/src/utils.dart'; import 'package:flutter/cupertino.dart'; import 'package:flutter/material.dart'; -import 'package:beamer/beamer.dart'; - /// Types for how to route should be built. /// /// See [BeamPage.type] @@ -145,7 +144,7 @@ class BeamPage extends Page { delegate.update( configuration: delegate.configuration.copyWith( - location: popUri.toString(), + uri: popUri, state: lastRouteInformation?.state, ), data: data, diff --git a/package/lib/src/beam_state.dart b/package/lib/src/beam_state.dart index 5d79780..997e016 100644 --- a/package/lib/src/beam_state.dart +++ b/package/lib/src/beam_state.dart @@ -1,9 +1,8 @@ import 'dart:convert'; -import 'package:flutter/widgets.dart'; - -import 'package:beamer/src/utils.dart'; import 'package:beamer/src/beam_location.dart'; +import 'package:beamer/src/utils.dart'; +import 'package:flutter/widgets.dart'; /// A class to mix with when defining a custom state for [BeamLocation]. /// @@ -71,10 +70,8 @@ class BeamState with RouteInformationSerializable { BeamLocation? beamLocation, Object? routeState, }) { - uriString = Utils.trimmed(uriString); - final uri = Uri.parse(uriString); return BeamState.fromUri( - uri, + Utils.removeTrailingSlash(Uri.parse(uriString)), beamLocation: beamLocation, routeState: routeState, ); diff --git a/package/lib/src/beamer_delegate.dart b/package/lib/src/beamer_delegate.dart index 95047ef..4d521bf 100644 --- a/package/lib/src/beamer_delegate.dart +++ b/package/lib/src/beamer_delegate.dart @@ -803,8 +803,9 @@ class BeamerDelegate extends RouterDelegate configuration = currentBeamLocation.state.routeInformation; } else if (uri.path == '/') { configuration = RouteInformation( - uri: Uri.parse( - initialPath + (uri.query.isNotEmpty ? '?${uri.query}' : ''), + uri: uri.replace( + path: initialPath, + query: uri.hasQuery ? uri.query : null, ), ); } @@ -815,7 +816,7 @@ class BeamerDelegate extends RouterDelegate @override SynchronousFuture setNewRoutePath(RouteInformation configuration) { if (configuration.uri.path == '/' && initialPath != '/') { - configuration = configuration.copyWith(location: initialPath); + configuration = configuration.copyWith(uri: Uri.parse(initialPath)); } update(configuration: configuration); return SynchronousFuture(null); diff --git a/package/lib/src/utils.dart b/package/lib/src/utils.dart index 3cade38..51dbc01 100644 --- a/package/lib/src/utils.dart +++ b/package/lib/src/utils.dart @@ -231,43 +231,42 @@ abstract class Utils { } } - /// Removes the trailing / in an URI String and returns the result. + /// Removes the trailing / in an URI path and returns the new URI. /// - /// If there is no trailing /, returns the input. - static String trimmed(String? uriString) { - if (uriString == null) { - return ''; + /// If there is no trailing /, returns the input URI. + static Uri removeTrailingSlash(Uri uri) { + String path = uri.path; + if (path.length > 1 && path.endsWith('/')) { + return uri.replace(path: path.substring(0, path.length - 1)); } - if (uriString.length > 1 && uriString.endsWith('/')) { - return uriString.substring(0, uriString.length - 1); + return uri; + } + + /// If [incoming] is a relative URI append it to [current]. + /// Else, return incoming URI. + static Uri maybeAppend(Uri current, Uri incoming) { + if (!incoming.hasAbsolutePath && !incoming.hasEmptyPath) { + String currentPath = + current.path.endsWith('/') ? current.path : '${current.path}/'; + return current.replace( + path: currentPath + incoming.path, + query: incoming.hasQuery ? incoming.query : null, + fragment: incoming.hasFragment ? incoming.fragment : null, + ); } - return uriString; + return incoming; } - /// If incoming RouteInformation doesn't start with slash, - /// append it to the current RouteInformation.location. + /// Merges the URIs of the RouteInformation's. /// - /// Else, return incoming RouteInformation - static RouteInformation maybeAppend( + /// See [maybeAppend] and [removeTrailingSlash]. + static RouteInformation mergeConfiguration( RouteInformation current, RouteInformation incoming, ) { - if (!incoming.uri.hasAbsolutePath && !incoming.uri.hasEmptyPath) { - String currentPath = current.uri.path.endsWith('/') - ? current.uri.path - : '${current.uri.path}/'; - return current.copyWith( - location: current.uri - .replace( - path: currentPath + incoming.uri.path, - query: incoming.uri.hasQuery ? incoming.uri.query : null, - fragment: incoming.uri.hasFragment ? incoming.uri.fragment : null, - ) - .toString(), - state: incoming.state, - ); - } - return incoming; + return incoming.copyWith( + uri: maybeAppend(current.uri, removeTrailingSlash(incoming.uri)), + ); } /// Creates a new configuration for [BeamerDelegate.update]. @@ -275,15 +274,12 @@ abstract class Utils { /// Takes into consideration trimming and potentially appending /// the incoming to current (used in relative beaming). /// - /// See [trimmed] and [maybeAppend]. + /// See [removeTrailingSlash] and [maybeAppend]. static RouteInformation createNewConfiguration( RouteInformation current, RouteInformation incoming, ) { - incoming = incoming.copyWith( - location: trimmed(incoming.uri.toString()), - ); - return maybeAppend(current, incoming); + return mergeConfiguration(current, incoming); } } @@ -291,11 +287,11 @@ abstract class Utils { extension BeamerRouteInformationExtension on RouteInformation { /// Returns a new [RouteInformation] created from this. RouteInformation copyWith({ - String? location, + Uri? uri, Object? state, }) { return RouteInformation( - uri: Uri.parse(location ?? this.uri.toString()), + uri: uri ?? this.uri, state: state ?? this.state, ); } diff --git a/package/test/utils_test.dart b/package/test/utils_test.dart index 6331d5d..c4d90d6 100644 --- a/package/test/utils_test.dart +++ b/package/test/utils_test.dart @@ -125,83 +125,104 @@ void main() { }); group('Creating new configuration for BeamerDelegate', () { - test('Trimming', () { - expect(Utils.trimmed(null), ''); - expect(Utils.trimmed('/'), '/'); - expect(Utils.trimmed('/xxx/'), '/xxx'); + test('Remove trailing slashes', () { + expect(Utils.removeTrailingSlash(Uri.parse('')), Uri.parse('')); + expect(Utils.removeTrailingSlash(Uri.parse('/')), Uri.parse('/')); + expect(Utils.removeTrailingSlash(Uri.parse('/xxx/')), Uri.parse('/xxx')); + expect( + Utils.removeTrailingSlash(Uri.parse('https://example.com/')), + Uri.parse('https://example.com/'), + ); + expect( + Utils.removeTrailingSlash(Uri.parse('https://example.com/test/')), + Uri.parse('https://example.com/test'), + ); }); - test('Appending without new routeState', () { - final current = RouteInformation(uri: Uri.parse('/current')); + test('Appending URIs to relative URI', () { + final current = Uri.parse('/current'); expect( - Utils.maybeAppend(current, RouteInformation(uri: Uri.parse('incoming'))) - .uri - .toString(), + Utils.maybeAppend(current, Uri.parse('incoming')).toString(), '/current/incoming', ); expect( - Utils.maybeAppend( - current, RouteInformation(uri: Uri.parse('/incoming'))) - .uri - .toString(), + Utils.maybeAppend(current, Uri.parse('/incoming')).toString(), '/incoming', ); expect( - Utils.maybeAppend(current, RouteInformation(uri: Uri.parse(''))) - .uri - .toString(), + Utils.maybeAppend(current, Uri.parse('')).toString(), '', ); expect( - Utils.maybeAppend(current, RouteInformation(uri: Uri.parse('/'))) - .uri - .toString(), + Utils.maybeAppend(current, Uri.parse('/')).toString(), '/', ); expect( - Utils.maybeAppend(current, - RouteInformation(uri: Uri.parse('example://app/incoming'))) - .uri - .toString(), + Utils.maybeAppend( + current, + Uri.parse('example://app/incoming'), + ).toString(), 'example://app/incoming', ); expect( - Utils.maybeAppend(current, - RouteInformation(uri: Uri.parse('example://app/incoming'))) - .uri - .toString(), - 'example://app/incoming', + Utils.maybeAppend(current, Uri.parse('//app/incoming')).toString(), + '//app/incoming', + ); + }); + + test('Appending URIs to absolute URI', () { + final current = Uri.parse('example://app/current'); + expect( + Utils.maybeAppend(current, Uri.parse('incoming')).toString(), + 'example://app/current/incoming', + ); + expect( + Utils.maybeAppend(current, Uri.parse('/incoming')).toString(), + '/incoming', + ); + expect( + Utils.maybeAppend(current, Uri.parse('')).toString(), + '', + ); + expect( + Utils.maybeAppend(current, Uri.parse('/')).toString(), + '/', ); expect( Utils.maybeAppend( - current, RouteInformation(uri: Uri.parse('//app/incoming'))) - .uri - .toString(), + current, + Uri.parse('example://app/incoming'), + ).toString(), + 'example://app/incoming', + ); + expect( + Utils.maybeAppend(current, Uri.parse('//app/incoming')).toString(), '//app/incoming', ); }); - test('Appending with new routeState', () { + test('Merging with new routeState', () { final current = RouteInformation(uri: Uri.parse('/current')); expect( - Utils.maybeAppend(current, RouteInformation(uri: Uri(), state: 42)) + Utils.mergeConfiguration( + current, RouteInformation(uri: Uri(), state: 42)) .state, 42, ); expect( - Utils.maybeAppend(current, + Utils.mergeConfiguration(current, RouteInformation(uri: Uri.parse('incoming'), state: 42)) .state, 42, ); expect( - Utils.maybeAppend(current, + Utils.mergeConfiguration(current, RouteInformation(uri: Uri.parse('/incoming'), state: 42)) .state, 42, ); expect( - Utils.maybeAppend( + Utils.mergeConfiguration( current, RouteInformation( uri: Uri.parse('example://app/incoming'), @@ -210,7 +231,7 @@ void main() { 42, ); expect( - Utils.maybeAppend( + Utils.mergeConfiguration( current, RouteInformation( uri: Uri.parse('//app/incoming'),