From 609f8467c4735c24bbd2f7f8537c159213873604 Mon Sep 17 00:00:00 2001 From: Collin Jackson Date: Wed, 27 Nov 2019 12:06:53 -0800 Subject: [PATCH 01/24] Fix to prevent isMock from being used in release builds --- .../CHANGELOG.md | 5 +++++ .../lib/url_launcher_platform_interface.dart | 20 ++++++++++++++++--- 2 files changed, 22 insertions(+), 3 deletions(-) diff --git a/packages/url_launcher/url_launcher_platform_interface/CHANGELOG.md b/packages/url_launcher/url_launcher_platform_interface/CHANGELOG.md index 18d373d183f2..5ca46411c4cc 100644 --- a/packages/url_launcher/url_launcher_platform_interface/CHANGELOG.md +++ b/packages/url_launcher/url_launcher_platform_interface/CHANGELOG.md @@ -1,3 +1,8 @@ +## 1.0.4 + +* Use an assertion to ensure `isMock` is not used in release builds. +* Prevent `noSuchMethod` from being used to bypass `isMock`. + ## 1.0.3 * Minor DartDoc changes and add a lint for missing DartDocs. diff --git a/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart b/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart index 64eeec7cae2d..d8e00dc9f83e 100644 --- a/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart +++ b/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart @@ -21,6 +21,8 @@ abstract class UrlLauncherPlatform { /// Mockito mocks are implementing this class with `implements` which is forbidden for anything /// other than mocks (see class docs). This property provides a backdoor for mockito mocks to /// skip the verification that the class isn't implemented with `implements`. + /// + /// This flag has no effect in release builds. @visibleForTesting bool get isMock => false; @@ -36,9 +38,17 @@ abstract class UrlLauncherPlatform { // TODO(amirh): Extract common platform interface logic. // https://github.com/flutter/flutter/issues/43368 static set instance(UrlLauncherPlatform instance) { - if (!instance.isMock) { + bool assertionsEnabled = false; + assert(() { + assertionsEnabled = true; + return true; + }); + if (!assertionsEnabled || !instance.isMock) { try { - instance._verifyProvidesDefaultImplementations(); + if (_verificationToken != instance._verifyProvidesDefaultImplementations()) { + throw AssertionError( + 'Platform interfaces must not be implemented with `implements`'); + } } on NoSuchMethodError catch (_) { throw AssertionError( 'Platform interfaces must not be implemented with `implements`'); @@ -79,5 +89,9 @@ abstract class UrlLauncherPlatform { // // This private method is called by the instance setter, which fails if the class is // implemented with `implements`. - void _verifyProvidesDefaultImplementations() {} + Object _verifyProvidesDefaultImplementations() => _verificationToken; + + // Private object used to determine whether mocks from instances of genuine if _verifyProvidesDefaultImplementations + // has been overridden with noSuchMethod. + static Object _verificationToken = Object(); } From d2ff05dd6d3e881968ec8fc8e1309174c3bc428a Mon Sep 17 00:00:00 2001 From: Collin Jackson Date: Wed, 27 Nov 2019 12:09:30 -0800 Subject: [PATCH 02/24] update pubspec --- .../url_launcher/url_launcher_platform_interface/pubspec.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/url_launcher/url_launcher_platform_interface/pubspec.yaml b/packages/url_launcher/url_launcher_platform_interface/pubspec.yaml index 3f7aa4832c41..9793c2b6f3cc 100644 --- a/packages/url_launcher/url_launcher_platform_interface/pubspec.yaml +++ b/packages/url_launcher/url_launcher_platform_interface/pubspec.yaml @@ -4,7 +4,7 @@ author: Flutter Team homepage: https://github.com/flutter/plugins/tree/master/packages/url_launcher/url_launcher_platform_interface # NOTE: We strongly prefer non-breaking changes, even at the expense of a # less-clean API. See https://flutter.dev/go/platform-interface-breaking-changes -version: 1.0.3 +version: 1.0.4 dependencies: flutter: From ecf964d0e56fe61e5ca044208d52fe0c6928f7c8 Mon Sep 17 00:00:00 2001 From: Collin Jackson Date: Wed, 27 Nov 2019 12:10:12 -0800 Subject: [PATCH 03/24] Fix assertion --- .../lib/url_launcher_platform_interface.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart b/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart index d8e00dc9f83e..9ce5e4ca3647 100644 --- a/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart +++ b/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart @@ -42,7 +42,7 @@ abstract class UrlLauncherPlatform { assert(() { assertionsEnabled = true; return true; - }); + }()); if (!assertionsEnabled || !instance.isMock) { try { if (_verificationToken != instance._verifyProvidesDefaultImplementations()) { From 69a06fe0f956bd3f9da7523ce6f849f95a44bc4a Mon Sep 17 00:00:00 2001 From: Collin Jackson Date: Wed, 27 Nov 2019 12:13:21 -0800 Subject: [PATCH 04/24] Fix comment --- .../lib/url_launcher_platform_interface.dart | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart b/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart index 9ce5e4ca3647..4c058d0ba2ca 100644 --- a/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart +++ b/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart @@ -91,7 +91,7 @@ abstract class UrlLauncherPlatform { // implemented with `implements`. Object _verifyProvidesDefaultImplementations() => _verificationToken; - // Private object used to determine whether mocks from instances of genuine if _verifyProvidesDefaultImplementations - // has been overridden with noSuchMethod. + // Private object used to determine if `_verifyProvidesDefaultImplementations` + // has been overridden with `noSuchMethod`. static Object _verificationToken = Object(); } From 0f2dd57eb243d60523a5be4bdb9526e388690303 Mon Sep 17 00:00:00 2001 From: Collin Jackson Date: Wed, 27 Nov 2019 12:26:29 -0800 Subject: [PATCH 05/24] Update CHANGELOG --- .../url_launcher/url_launcher_platform_interface/CHANGELOG.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/url_launcher/url_launcher_platform_interface/CHANGELOG.md b/packages/url_launcher/url_launcher_platform_interface/CHANGELOG.md index 5ca46411c4cc..f5b8d04b9cd0 100644 --- a/packages/url_launcher/url_launcher_platform_interface/CHANGELOG.md +++ b/packages/url_launcher/url_launcher_platform_interface/CHANGELOG.md @@ -1,7 +1,8 @@ ## 1.0.4 * Use an assertion to ensure `isMock` is not used in release builds. -* Prevent `noSuchMethod` from being used to bypass `isMock`. +* Prevent `noSuchMethod` from being used to bypass checks that + `UrlLauncherPlatform` isn't implemented with `implements`. ## 1.0.3 From 85b233ef75a81249eabce05b64ffd9ae13207bea Mon Sep 17 00:00:00 2001 From: Collin Jackson Date: Wed, 27 Nov 2019 12:29:24 -0800 Subject: [PATCH 06/24] use `identical` --- .../lib/url_launcher_platform_interface.dart | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart b/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart index 4c058d0ba2ca..37fb8ce69c50 100644 --- a/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart +++ b/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart @@ -45,7 +45,8 @@ abstract class UrlLauncherPlatform { }()); if (!assertionsEnabled || !instance.isMock) { try { - if (_verificationToken != instance._verifyProvidesDefaultImplementations()) { + if (identical(_verificationToken, + instance._verifyProvidesDefaultImplementations())) { throw AssertionError( 'Platform interfaces must not be implemented with `implements`'); } From 6941bf5a90ce0234a2f8e09431459e0c886bb245 Mon Sep 17 00:00:00 2001 From: Collin Jackson Date: Wed, 27 Nov 2019 12:35:53 -0800 Subject: [PATCH 07/24] Make `Object` const and fix test --- .../lib/url_launcher_platform_interface.dart | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart b/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart index 37fb8ce69c50..06828d5f46c2 100644 --- a/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart +++ b/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart @@ -45,7 +45,7 @@ abstract class UrlLauncherPlatform { }()); if (!assertionsEnabled || !instance.isMock) { try { - if (identical(_verificationToken, + if (!identical(_verificationToken, instance._verifyProvidesDefaultImplementations())) { throw AssertionError( 'Platform interfaces must not be implemented with `implements`'); @@ -94,5 +94,5 @@ abstract class UrlLauncherPlatform { // Private object used to determine if `_verifyProvidesDefaultImplementations` // has been overridden with `noSuchMethod`. - static Object _verificationToken = Object(); + static const Object _verificationToken = const Object(); } From 5762a2d47939090b9b5a3ea27d87b1a182068279 Mon Sep 17 00:00:00 2001 From: Collin Jackson Date: Wed, 27 Nov 2019 12:39:47 -0800 Subject: [PATCH 08/24] Update comment --- .../lib/url_launcher_platform_interface.dart | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart b/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart index 06828d5f46c2..0f9ab9fde4b6 100644 --- a/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart +++ b/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart @@ -92,7 +92,7 @@ abstract class UrlLauncherPlatform { // implemented with `implements`. Object _verifyProvidesDefaultImplementations() => _verificationToken; - // Private object used to determine if `_verifyProvidesDefaultImplementations` - // has been overridden with `noSuchMethod`. + // Private object used to ensure that `_verifyProvidesDefaultImplementations` + // cannot be implemented using `noSuchMethod`. static const Object _verificationToken = const Object(); } From fff7923689851ca23a0c01c78a0448c32dee8435 Mon Sep 17 00:00:00 2001 From: Collin Jackson Date: Wed, 27 Nov 2019 14:15:06 -0800 Subject: [PATCH 09/24] Alternative approach --- .../url_launcher/test/url_launcher_test.dart | 3 +- .../lib/url_launcher_platform_interface.dart | 99 +++++++++++-------- .../method_channel_url_launcher_test.dart | 6 +- 3 files changed, 64 insertions(+), 44 deletions(-) diff --git a/packages/url_launcher/url_launcher/test/url_launcher_test.dart b/packages/url_launcher/url_launcher/test/url_launcher_test.dart index 2481e5a0ee63..861b18e06625 100644 --- a/packages/url_launcher/url_launcher/test/url_launcher_test.dart +++ b/packages/url_launcher/url_launcher/test/url_launcher_test.dart @@ -13,7 +13,6 @@ import 'package:flutter/services.dart' show PlatformException; void main() { final MockUrlLauncher mock = MockUrlLauncher(); - when(mock.isMock).thenReturn(true); UrlLauncherPlatform.instance = mock; test('closeWebView default behavior', () async { @@ -208,4 +207,4 @@ void main() { }); } -class MockUrlLauncher extends Mock implements UrlLauncherPlatform {} +class MockUrlLauncher extends Mock with MockPlatformInterface implements UrlLauncherPlatform {} diff --git a/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart b/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart index 0f9ab9fde4b6..d6e3d5c902c8 100644 --- a/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart +++ b/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart @@ -8,6 +8,61 @@ import 'package:meta/meta.dart' show required, visibleForTesting; import 'method_channel_url_launcher.dart'; +/// Base class for platform interfaces. +/// +/// Provides helper methods for ensuring that platform interfaces are +/// implemented using `extends` instead of `implements`. +class PlatformInterface { + PlatformInterface({ Object token }) : _verificationToken = token; + + final Object _token; + + // Mock implementations can return true here using `noSuchMethod`. + // + // Mockito mocks are implementing this class with `implements` which is forbidden for anything + // other than mocks (see class docs). This property provides `MockPlatformInterface` + // a backdoor for mockito mocks to skip the verification that the class isn't + // implemented with `implements`. + bool get _isMock => false; + + /// Returns the instance. + static void verifyToken(Object token, PlatformInterface instance) { + assert(identical(token, instance._token)); + } + + // This method makes sure that UrlLauncher isn't implemented with `implements`. + // + // See class doc for more details on why implementing this class is forbidden. + // + // This private method is called by the instance setter, which fails if the class is + // implemented with `implements`. + Object _verifyProvidesDefaultImplementations() => _verificationToken; + + // Private object used to ensure that `_verifyProvidesDefaultImplementations` + // cannot be implemented using `noSuchMethod`. + static const Object _verificationToken = const Object(); +} + +/// A [PlatformInterface] that can be mocked with mockito. +/// +/// Throws an `AssertionError` when used in release builds. +@visibleForTesting +class MockPlatformInterface extends PlatformInterface { + @override + bool get _isMock { + bool assertionsEnabled = false; + assert(() { + assertionsEnabled = true; + return true; + }()); + if (!assertionsEnabled) { + throw AssertionError( + '`MockPlatformInterface` is not intended for use in release builds.'); + } + return true; + } +} + /// The interface that implementations of url_launcher must implement. /// /// Platform implementations should extend this class rather than implement it as `url_launcher` @@ -15,19 +70,13 @@ import 'method_channel_url_launcher.dart'; /// (using `extends`) ensures that the subclass will get the default implementation, while /// platform implementations that `implements` this interface will be broken by newly added /// [UrlLauncherPlatform] methods. -abstract class UrlLauncherPlatform { - /// Only mock implementations should set this to true. - /// - /// Mockito mocks are implementing this class with `implements` which is forbidden for anything - /// other than mocks (see class docs). This property provides a backdoor for mockito mocks to - /// skip the verification that the class isn't implemented with `implements`. - /// - /// This flag has no effect in release builds. - @visibleForTesting - bool get isMock => false; +abstract class UrlLauncherPlatform extends PlatformInterface { + UrlLauncherPlatform() : super(token: _token); static UrlLauncherPlatform _instance = MethodChannelUrlLauncher(); + static const Object _token = const Object(); + /// The default instance of [UrlLauncherPlatform] to use. /// /// Defaults to [MethodChannelUrlLauncher]. @@ -38,23 +87,7 @@ abstract class UrlLauncherPlatform { // TODO(amirh): Extract common platform interface logic. // https://github.com/flutter/flutter/issues/43368 static set instance(UrlLauncherPlatform instance) { - bool assertionsEnabled = false; - assert(() { - assertionsEnabled = true; - return true; - }()); - if (!assertionsEnabled || !instance.isMock) { - try { - if (!identical(_verificationToken, - instance._verifyProvidesDefaultImplementations())) { - throw AssertionError( - 'Platform interfaces must not be implemented with `implements`'); - } - } on NoSuchMethodError catch (_) { - throw AssertionError( - 'Platform interfaces must not be implemented with `implements`'); - } - } + PlatformInterface.verifyToken(_token, instance); _instance = instance; } @@ -83,16 +116,4 @@ abstract class UrlLauncherPlatform { Future closeWebView() { throw UnimplementedError('closeWebView() has not been implemented.'); } - - // This method makes sure that UrlLauncher isn't implemented with `implements`. - // - // See class doc for more details on why implementing this class is forbidden. - // - // This private method is called by the instance setter, which fails if the class is - // implemented with `implements`. - Object _verifyProvidesDefaultImplementations() => _verificationToken; - - // Private object used to ensure that `_verifyProvidesDefaultImplementations` - // cannot be implemented using `noSuchMethod`. - static const Object _verificationToken = const Object(); } diff --git a/packages/url_launcher/url_launcher_platform_interface/test/method_channel_url_launcher_test.dart b/packages/url_launcher/url_launcher_platform_interface/test/method_channel_url_launcher_test.dart index 1d6b0c9f3f8c..2f511d871d3d 100644 --- a/packages/url_launcher/url_launcher_platform_interface/test/method_channel_url_launcher_test.dart +++ b/packages/url_launcher/url_launcher_platform_interface/test/method_channel_url_launcher_test.dart @@ -27,7 +27,6 @@ void main() { test('Can be mocked with `implements`', () { final ImplementsUrlLauncherPlatform mock = ImplementsUrlLauncherPlatform(); - when(mock.isMock).thenReturn(true); UrlLauncherPlatform.instance = mock; }); @@ -280,7 +279,8 @@ void main() { }); } -class ImplementsUrlLauncherPlatform extends Mock - implements UrlLauncherPlatform {} +class ImplementsUrlLauncherPlatform extends Mock with MockPlatformInterface + implements UrlLauncherPlatform { +} class ExtendsUrlLauncherPlatform extends UrlLauncherPlatform {} From 5f04abd5814ed1333e0d59cb57d5b16a7680c9b4 Mon Sep 17 00:00:00 2001 From: Collin Jackson Date: Wed, 27 Nov 2019 14:25:12 -0800 Subject: [PATCH 10/24] More improvements --- .../lib/url_launcher_platform_interface.dart | 25 ++++++------------- 1 file changed, 8 insertions(+), 17 deletions(-) diff --git a/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart b/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart index d6e3d5c902c8..ffa6d2d66aa1 100644 --- a/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart +++ b/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart @@ -13,8 +13,9 @@ import 'method_channel_url_launcher.dart'; /// Provides helper methods for ensuring that platform interfaces are /// implemented using `extends` instead of `implements`. class PlatformInterface { - PlatformInterface({ Object token }) : _verificationToken = token; + PlatformInterface({ Object token }) : _token = token; + // Pass a `const Object()` here to distinguish final Object _token; // Mock implementations can return true here using `noSuchMethod`. @@ -25,22 +26,12 @@ class PlatformInterface { // implemented with `implements`. bool get _isMock => false; - /// Returns the instance. - static void verifyToken(Object token, PlatformInterface instance) { - assert(identical(token, instance._token)); + /// Return true if the platform instance has a token that matches the + /// provided token. This is used to ensure that implementers are using + /// `extends` rather than `implements`. + static bool isValid(PlatformInterface instance, Object token) { + return _isMock || identical(token, instance._token); } - - // This method makes sure that UrlLauncher isn't implemented with `implements`. - // - // See class doc for more details on why implementing this class is forbidden. - // - // This private method is called by the instance setter, which fails if the class is - // implemented with `implements`. - Object _verifyProvidesDefaultImplementations() => _verificationToken; - - // Private object used to ensure that `_verifyProvidesDefaultImplementations` - // cannot be implemented using `noSuchMethod`. - static const Object _verificationToken = const Object(); } /// A [PlatformInterface] that can be mocked with mockito. @@ -87,7 +78,7 @@ abstract class UrlLauncherPlatform extends PlatformInterface { // TODO(amirh): Extract common platform interface logic. // https://github.com/flutter/flutter/issues/43368 static set instance(UrlLauncherPlatform instance) { - PlatformInterface.verifyToken(_token, instance); + assert(PlatformInterface.isValid(instance, _token)); _instance = instance; } From 4b31918d062f49e796e9a11eb8ea5e77d170ff23 Mon Sep 17 00:00:00 2001 From: Collin Jackson Date: Wed, 27 Nov 2019 14:29:23 -0800 Subject: [PATCH 11/24] Fix tests --- .../lib/url_launcher_platform_interface.dart | 8 +++++--- .../test/method_channel_url_launcher_test.dart | 13 ++++++++----- 2 files changed, 13 insertions(+), 8 deletions(-) diff --git a/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart b/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart index ffa6d2d66aa1..7e90bc83f983 100644 --- a/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart +++ b/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart @@ -13,10 +13,10 @@ import 'method_channel_url_launcher.dart'; /// Provides helper methods for ensuring that platform interfaces are /// implemented using `extends` instead of `implements`. class PlatformInterface { - PlatformInterface({ Object token }) : _token = token; + PlatformInterface({Object token}) : _instanceToken = token; // Pass a `const Object()` here to distinguish - final Object _token; + final Object _instanceToken; // Mock implementations can return true here using `noSuchMethod`. // @@ -29,8 +29,10 @@ class PlatformInterface { /// Return true if the platform instance has a token that matches the /// provided token. This is used to ensure that implementers are using /// `extends` rather than `implements`. + /// + /// Subclasses of [MockPlatformInterface] are assumed to be valid. static bool isValid(PlatformInterface instance, Object token) { - return _isMock || identical(token, instance._token); + return instance._isMock || identical(token, instance._instanceToken); } } diff --git a/packages/url_launcher/url_launcher_platform_interface/test/method_channel_url_launcher_test.dart b/packages/url_launcher/url_launcher_platform_interface/test/method_channel_url_launcher_test.dart index 2f511d871d3d..6c9334f6c524 100644 --- a/packages/url_launcher/url_launcher_platform_interface/test/method_channel_url_launcher_test.dart +++ b/packages/url_launcher/url_launcher_platform_interface/test/method_channel_url_launcher_test.dart @@ -25,8 +25,8 @@ void main() { }); test('Can be mocked with `implements`', () { - final ImplementsUrlLauncherPlatform mock = - ImplementsUrlLauncherPlatform(); + final UrlLauncherPlatform mock = + ImplementsUrlLauncherPlatformUsingMockPlatformInterface(); UrlLauncherPlatform.instance = mock; }); @@ -279,8 +279,11 @@ void main() { }); } -class ImplementsUrlLauncherPlatform extends Mock with MockPlatformInterface - implements UrlLauncherPlatform { -} +class ImplementsUrlLauncherPlatform extends Mock + implements UrlLauncherPlatform {} + +class ImplementsUrlLauncherPlatformUsingMockPlatformInterface extends Mock + with MockPlatformInterface + implements UrlLauncherPlatform {} class ExtendsUrlLauncherPlatform extends UrlLauncherPlatform {} From b170b933df8e66d7329294ab0538972ad0b38f3b Mon Sep 17 00:00:00 2001 From: Collin Jackson Date: Wed, 27 Nov 2019 14:31:21 -0800 Subject: [PATCH 12/24] CHANGELOG --- .../url_launcher_platform_interface/CHANGELOG.md | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/packages/url_launcher/url_launcher_platform_interface/CHANGELOG.md b/packages/url_launcher/url_launcher_platform_interface/CHANGELOG.md index f5b8d04b9cd0..a58dbcd4aa8a 100644 --- a/packages/url_launcher/url_launcher_platform_interface/CHANGELOG.md +++ b/packages/url_launcher/url_launcher_platform_interface/CHANGELOG.md @@ -1,8 +1,7 @@ ## 1.0.4 -* Use an assertion to ensure `isMock` is not used in release builds. -* Prevent `noSuchMethod` from being used to bypass checks that - `UrlLauncherPlatform` isn't implemented with `implements`. +* Remove `@visibleForTesting` API `isMock`. +* Introduces `MockPlatformInterface` and `PlatformInterface`. ## 1.0.3 From df9b2f8ebe69e200972d8670eddc0c79c53de4e1 Mon Sep 17 00:00:00 2001 From: Collin Jackson Date: Wed, 27 Nov 2019 14:32:15 -0800 Subject: [PATCH 13/24] Move comment --- .../lib/url_launcher_platform_interface.dart | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart b/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart index 7e90bc83f983..a8d3508fdeae 100644 --- a/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart +++ b/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart @@ -12,6 +12,8 @@ import 'method_channel_url_launcher.dart'; /// /// Provides helper methods for ensuring that platform interfaces are /// implemented using `extends` instead of `implements`. +// TODO(amirh): Extract common platform interface logic. +// https://github.com/flutter/flutter/issues/43368 class PlatformInterface { PlatformInterface({Object token}) : _instanceToken = token; @@ -77,8 +79,6 @@ abstract class UrlLauncherPlatform extends PlatformInterface { /// Platform-specific plugins should set this with their own platform-specific /// class that extends [UrlLauncherPlatform] when they register themselves. - // TODO(amirh): Extract common platform interface logic. - // https://github.com/flutter/flutter/issues/43368 static set instance(UrlLauncherPlatform instance) { assert(PlatformInterface.isValid(instance, _token)); _instance = instance; From 12bebdf5357597741249959b5fa6eb565448ee80 Mon Sep 17 00:00:00 2001 From: Collin Jackson Date: Wed, 27 Nov 2019 14:40:50 -0800 Subject: [PATCH 14/24] Update comments --- .../lib/url_launcher_platform_interface.dart | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart b/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart index a8d3508fdeae..90418f9b346d 100644 --- a/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart +++ b/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart @@ -10,14 +10,14 @@ import 'method_channel_url_launcher.dart'; /// Base class for platform interfaces. /// -/// Provides helper methods for ensuring that platform interfaces are +/// Provides a static helper method for ensuring that platform interfaces are /// implemented using `extends` instead of `implements`. // TODO(amirh): Extract common platform interface logic. // https://github.com/flutter/flutter/issues/43368 -class PlatformInterface { +abstract class PlatformInterface { + /// Pass a class-specific `const Object()` as the `token`. PlatformInterface({Object token}) : _instanceToken = token; - // Pass a `const Object()` here to distinguish final Object _instanceToken; // Mock implementations can return true here using `noSuchMethod`. @@ -38,11 +38,13 @@ class PlatformInterface { } } -/// A [PlatformInterface] that can be mocked with mockito. +/// A [PlatformInterface] mixin that can be combined with mockito's `Mock`. +/// +/// It always returns true when passed to [PlatformInterface.isValid]. /// /// Throws an `AssertionError` when used in release builds. @visibleForTesting -class MockPlatformInterface extends PlatformInterface { +abstract class MockPlatformInterface extends PlatformInterface { @override bool get _isMock { bool assertionsEnabled = false; From a3ad7a33cfba17e71526653a77ee14fb8a42e811 Mon Sep 17 00:00:00 2001 From: Collin Jackson Date: Wed, 27 Nov 2019 14:49:35 -0800 Subject: [PATCH 15/24] Update to syntax --- .../lib/url_launcher_platform_interface.dart | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart b/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart index 90418f9b346d..76a8bd672318 100644 --- a/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart +++ b/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart @@ -16,7 +16,7 @@ import 'method_channel_url_launcher.dart'; // https://github.com/flutter/flutter/issues/43368 abstract class PlatformInterface { /// Pass a class-specific `const Object()` as the `token`. - PlatformInterface({Object token}) : _instanceToken = token; + PlatformInterface({ @required Object token }) : _instanceToken = token; final Object _instanceToken; @@ -42,9 +42,9 @@ abstract class PlatformInterface { /// /// It always returns true when passed to [PlatformInterface.isValid]. /// -/// Throws an `AssertionError` when used in release builds. +/// For use in testing only. Throws `AssertionError` when used in release mode. @visibleForTesting -abstract class MockPlatformInterface extends PlatformInterface { +abstract class MockPlatformInterface implements PlatformInterface { @override bool get _isMock { bool assertionsEnabled = false; From 2bd58286a0e678dc9a42aad1be11c0087ff835c2 Mon Sep 17 00:00:00 2001 From: Collin Jackson Date: Wed, 27 Nov 2019 14:50:06 -0800 Subject: [PATCH 16/24] Format --- .../lib/url_launcher_platform_interface.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart b/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart index 76a8bd672318..772f192fb02b 100644 --- a/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart +++ b/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart @@ -16,7 +16,7 @@ import 'method_channel_url_launcher.dart'; // https://github.com/flutter/flutter/issues/43368 abstract class PlatformInterface { /// Pass a class-specific `const Object()` as the `token`. - PlatformInterface({ @required Object token }) : _instanceToken = token; + PlatformInterface({@required Object token}) : _instanceToken = token; final Object _instanceToken; From 4fe61d0ac1c585a0b0bac95af64e376548038459 Mon Sep 17 00:00:00 2001 From: Collin Jackson Date: Wed, 27 Nov 2019 14:56:15 -0800 Subject: [PATCH 17/24] Defend against noSuchMethod --- .../lib/url_launcher_platform_interface.dart | 38 +++++++++---------- 1 file changed, 17 insertions(+), 21 deletions(-) diff --git a/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart b/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart index 772f192fb02b..17191f7ae0e6 100644 --- a/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart +++ b/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart @@ -20,21 +20,26 @@ abstract class PlatformInterface { final Object _instanceToken; - // Mock implementations can return true here using `noSuchMethod`. - // - // Mockito mocks are implementing this class with `implements` which is forbidden for anything - // other than mocks (see class docs). This property provides `MockPlatformInterface` - // a backdoor for mockito mocks to skip the verification that the class isn't - // implemented with `implements`. - bool get _isMock => false; - /// Return true if the platform instance has a token that matches the /// provided token. This is used to ensure that implementers are using /// `extends` rather than `implements`. /// /// Subclasses of [MockPlatformInterface] are assumed to be valid. static bool isValid(PlatformInterface instance, Object token) { - return instance._isMock || identical(token, instance._instanceToken); + if (identical(instance._instanceToken, MockPlatformInterface._token)) { + bool assertionsEnabled = false; + assert(() { + assertionsEnabled = true; + return true; + }()); + if (!assertionsEnabled) { + throw AssertionError( + '`MockPlatformInterface` is not intended for use in release builds.'); + } else { + return true; + } + } + return identical(token, instance._instanceToken); } } @@ -45,19 +50,10 @@ abstract class PlatformInterface { /// For use in testing only. Throws `AssertionError` when used in release mode. @visibleForTesting abstract class MockPlatformInterface implements PlatformInterface { + static const Object _token = const Object(); + @override - bool get _isMock { - bool assertionsEnabled = false; - assert(() { - assertionsEnabled = true; - return true; - }()); - if (!assertionsEnabled) { - throw AssertionError( - '`MockPlatformInterface` is not intended for use in release builds.'); - } - return true; - } + final Object _instanceToken = _token; } /// The interface that implementations of url_launcher must implement. From 8a4a3774d2f2b6398a0e1b330f4d45e15a536ed1 Mon Sep 17 00:00:00 2001 From: Collin Jackson Date: Wed, 27 Nov 2019 15:01:47 -0800 Subject: [PATCH 18/24] Flip if statement --- .../lib/url_launcher_platform_interface.dart | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart b/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart index 17191f7ae0e6..1d3aff4aa4f5 100644 --- a/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart +++ b/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart @@ -32,11 +32,11 @@ abstract class PlatformInterface { assertionsEnabled = true; return true; }()); - if (!assertionsEnabled) { + if (assertionsEnabled) { + return true; + } else { throw AssertionError( '`MockPlatformInterface` is not intended for use in release builds.'); - } else { - return true; } } return identical(token, instance._instanceToken); From 335f098b5a7872a31ec207508fa5852992d5fcb7 Mon Sep 17 00:00:00 2001 From: Collin Jackson Date: Wed, 27 Nov 2019 15:03:46 -0800 Subject: [PATCH 19/24] Clean up comments more --- .../lib/url_launcher_platform_interface.dart | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart b/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart index 1d3aff4aa4f5..fc05964c0853 100644 --- a/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart +++ b/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart @@ -20,9 +20,11 @@ abstract class PlatformInterface { final Object _instanceToken; - /// Return true if the platform instance has a token that matches the - /// provided token. This is used to ensure that implementers are using - /// `extends` rather than `implements`. + /// Return `true `if the platform instance has a token that matches the + /// provided token. + /// + /// This is used to ensure that implementers are using `extends` rather than + /// `implements`. /// /// Subclasses of [MockPlatformInterface] are assumed to be valid. static bool isValid(PlatformInterface instance, Object token) { @@ -45,7 +47,7 @@ abstract class PlatformInterface { /// A [PlatformInterface] mixin that can be combined with mockito's `Mock`. /// -/// It always returns true when passed to [PlatformInterface.isValid]. +/// It always returns `true` when passed to [PlatformInterface.isValid]. /// /// For use in testing only. Throws `AssertionError` when used in release mode. @visibleForTesting From 556ce46907dd7389f7788c31b6ef5757967f8eff Mon Sep 17 00:00:00 2001 From: Collin Jackson Date: Wed, 27 Nov 2019 15:08:03 -0800 Subject: [PATCH 20/24] Move assertion logic into MockPlatformInterface --- .../lib/url_launcher_platform_interface.dart | 26 ++++++++++--------- 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart b/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart index fc05964c0853..61f1c9c5506b 100644 --- a/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart +++ b/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart @@ -29,17 +29,7 @@ abstract class PlatformInterface { /// Subclasses of [MockPlatformInterface] are assumed to be valid. static bool isValid(PlatformInterface instance, Object token) { if (identical(instance._instanceToken, MockPlatformInterface._token)) { - bool assertionsEnabled = false; - assert(() { - assertionsEnabled = true; - return true; - }()); - if (assertionsEnabled) { - return true; - } else { - throw AssertionError( - '`MockPlatformInterface` is not intended for use in release builds.'); - } + return true; } return identical(token, instance._instanceToken); } @@ -55,7 +45,19 @@ abstract class MockPlatformInterface implements PlatformInterface { static const Object _token = const Object(); @override - final Object _instanceToken = _token; + Object get _instanceToken { + bool assertionsEnabled = false; + assert(() { + assertionsEnabled = true; + return true; + }()); + if (assertionsEnabled) { + return _token; + } else { + throw AssertionError( + '`MockPlatformInterface` is not intended for use in release builds.'); + } + } } /// The interface that implementations of url_launcher must implement. From 86b8d3d956f82a22c09a35af5a7f8ad9fbb2b52c Mon Sep 17 00:00:00 2001 From: Collin Jackson Date: Wed, 27 Nov 2019 15:32:39 -0800 Subject: [PATCH 21/24] feedback from Amir --- .../CHANGELOG.md | 2 +- .../lib/url_launcher_platform_interface.dart | 42 ++++++++++--------- 2 files changed, 23 insertions(+), 21 deletions(-) diff --git a/packages/url_launcher/url_launcher_platform_interface/CHANGELOG.md b/packages/url_launcher/url_launcher_platform_interface/CHANGELOG.md index a58dbcd4aa8a..f12ad149e05a 100644 --- a/packages/url_launcher/url_launcher_platform_interface/CHANGELOG.md +++ b/packages/url_launcher/url_launcher_platform_interface/CHANGELOG.md @@ -1,7 +1,7 @@ ## 1.0.4 * Remove `@visibleForTesting` API `isMock`. -* Introduces `MockPlatformInterface` and `PlatformInterface`. +* Introduces `MockPlatformInterface` and `PlatformInterface` classes. ## 1.0.3 diff --git a/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart b/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart index 61f1c9c5506b..89f23d5454ee 100644 --- a/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart +++ b/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart @@ -15,23 +15,37 @@ import 'method_channel_url_launcher.dart'; // TODO(amirh): Extract common platform interface logic. // https://github.com/flutter/flutter/issues/43368 abstract class PlatformInterface { - /// Pass a class-specific `const Object()` as the `token`. + /// Pass a private, class-specific `const Object()` as the `token`. PlatformInterface({@required Object token}) : _instanceToken = token; final Object _instanceToken; - /// Return `true `if the platform instance has a token that matches the - /// provided token. + /// Ensures that the platform instance has a token that matches the + /// provided token and throws [AssertionError] if not. /// /// This is used to ensure that implementers are using `extends` rather than /// `implements`. /// /// Subclasses of [MockPlatformInterface] are assumed to be valid. - static bool isValid(PlatformInterface instance, Object token) { + /// + /// This is implemented as a static method so that it cannot be overridden + /// with `noSuchMethod`. + static void verifyToken(PlatformInterface instance, Object token) { if (identical(instance._instanceToken, MockPlatformInterface._token)) { - return true; + bool assertionsEnabled = false; + assert(() { + assertionsEnabled = true; + return true; + }()); + if (!assertionsEnabled) { + throw AssertionError( + '`MockPlatformInterface` is not intended for use in release builds.'); + } + } + if (!identical(token, instance._instanceToken)) { + throw AssertionError( + 'Platform interfaces must not be implemented with `implements`'); } - return identical(token, instance._instanceToken); } } @@ -45,19 +59,7 @@ abstract class MockPlatformInterface implements PlatformInterface { static const Object _token = const Object(); @override - Object get _instanceToken { - bool assertionsEnabled = false; - assert(() { - assertionsEnabled = true; - return true; - }()); - if (assertionsEnabled) { - return _token; - } else { - throw AssertionError( - '`MockPlatformInterface` is not intended for use in release builds.'); - } - } + Object get _instanceToken => _token; } /// The interface that implementations of url_launcher must implement. @@ -82,7 +84,7 @@ abstract class UrlLauncherPlatform extends PlatformInterface { /// Platform-specific plugins should set this with their own platform-specific /// class that extends [UrlLauncherPlatform] when they register themselves. static set instance(UrlLauncherPlatform instance) { - assert(PlatformInterface.isValid(instance, _token)); + PlatformInterface.verifyToken(instance, _token); _instance = instance; } From 512077e02a28c0a8fac40229232e37fd5244ae2d Mon Sep 17 00:00:00 2001 From: Collin Jackson Date: Wed, 27 Nov 2019 15:33:05 -0800 Subject: [PATCH 22/24] Reformat --- .../lib/url_launcher_platform_interface.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart b/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart index 89f23d5454ee..1ae48fdb95ad 100644 --- a/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart +++ b/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart @@ -22,7 +22,7 @@ abstract class PlatformInterface { /// Ensures that the platform instance has a token that matches the /// provided token and throws [AssertionError] if not. - /// + /// /// This is used to ensure that implementers are using `extends` rather than /// `implements`. /// From 3d566179f6d64cedecdafdff184c29703224b304 Mon Sep 17 00:00:00 2001 From: Collin Jackson Date: Wed, 27 Nov 2019 15:37:08 -0800 Subject: [PATCH 23/24] Update comments --- .../lib/url_launcher_platform_interface.dart | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart b/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart index 1ae48fdb95ad..8467f370a62f 100644 --- a/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart +++ b/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart @@ -26,7 +26,8 @@ abstract class PlatformInterface { /// This is used to ensure that implementers are using `extends` rather than /// `implements`. /// - /// Subclasses of [MockPlatformInterface] are assumed to be valid. + /// Subclasses of [MockPlatformInterface] are assumed to be valid in debug + /// builds. /// /// This is implemented as a static method so that it cannot be overridden /// with `noSuchMethod`. @@ -51,9 +52,10 @@ abstract class PlatformInterface { /// A [PlatformInterface] mixin that can be combined with mockito's `Mock`. /// -/// It always returns `true` when passed to [PlatformInterface.isValid]. +/// It passes the [PlatformInterface.verifyToken] check even though it isn't +/// using `extends`. /// -/// For use in testing only. Throws `AssertionError` when used in release mode. +/// This class is intended for use in tests only. @visibleForTesting abstract class MockPlatformInterface implements PlatformInterface { static const Object _token = const Object(); From 144ea9d9a6afd5618c8556a31c8ac23b3f018b94 Mon Sep 17 00:00:00 2001 From: Collin Jackson Date: Wed, 27 Nov 2019 15:38:57 -0800 Subject: [PATCH 24/24] Fix analyzer --- .../lib/url_launcher_platform_interface.dart | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart b/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart index 8467f370a62f..cfad2f87bf90 100644 --- a/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart +++ b/packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart @@ -58,7 +58,7 @@ abstract class PlatformInterface { /// This class is intended for use in tests only. @visibleForTesting abstract class MockPlatformInterface implements PlatformInterface { - static const Object _token = const Object(); + static const Object _token = Object(); @override Object get _instanceToken => _token; @@ -76,7 +76,7 @@ abstract class UrlLauncherPlatform extends PlatformInterface { static UrlLauncherPlatform _instance = MethodChannelUrlLauncher(); - static const Object _token = const Object(); + static const Object _token = Object(); /// The default instance of [UrlLauncherPlatform] to use. ///