From a584f856be4f68d9869d7fc4a18bf3bb03f90020 Mon Sep 17 00:00:00 2001 From: Bryan Oltman Date: Tue, 2 May 2023 14:11:23 -0400 Subject: [PATCH] feat(shorebird_cli): enable autofixing issues with `shorebird doctor --fix` (#405) --- .../lib/src/commands/doctor_command.dart | 73 ++++++- ...android_internet_permission_validator.dart | 19 +- .../lib/src/validators/validators.dart | 11 +- .../src/commands/doctor_command_test.dart | 191 +++++++++++++++++- ...id_internet_permission_validator_test.dart | 28 +++ 5 files changed, 310 insertions(+), 12 deletions(-) diff --git a/packages/shorebird_cli/lib/src/commands/doctor_command.dart b/packages/shorebird_cli/lib/src/commands/doctor_command.dart index 0e5f5b3b..c98725fb 100644 --- a/packages/shorebird_cli/lib/src/commands/doctor_command.dart +++ b/packages/shorebird_cli/lib/src/commands/doctor_command.dart @@ -18,6 +18,13 @@ class DoctorCommand extends ShorebirdCommand with ShorebirdVersionMixin { super.validators, }) { validators = _allValidators(baseValidators: validators); + + argParser.addFlag( + 'fix', + abbr: 'f', + help: 'Fix issues where possible.', + negatable: false, + ); } late final List _doctorValidators = [ @@ -36,33 +43,85 @@ class DoctorCommand extends ShorebirdCommand with ShorebirdVersionMixin { @override Future run() async { + final shouldFix = results['fix'] == true; + logger.info(''' Shorebird v$packageVersion Shorebird Engine • revision ${ShorebirdEnvironment.shorebirdEngineRevision}'''); - var numIssues = 0; + final allIssues = []; + final allFixableIssues = []; for (final validator in validators) { + final failedFixes = {}; final progress = logger.progress(validator.description); final issues = await validator.validate(process); - numIssues += issues.length; if (issues.isEmpty) { progress.complete(); - } else { - progress.fail(); + continue; + } - for (final issue in issues) { - logger.info(' ${issue.displayMessage}'); + final fixableIssues = issues.where((issue) => issue.fix != null); + var unresolvedIssues = issues; + if (fixableIssues.isNotEmpty) { + if (shouldFix) { + // If --fix flag was used and there are fixable issues, fix them. + progress.update('Fixing'); + for (final issue in fixableIssues) { + try { + await issue.fix!(); + } catch (error) { + failedFixes[issue] = error; + } + } + + // Re-run the validator to see if there are any remaining issues that + // we couldn't fix. + unresolvedIssues = await validator.validate(process); + if (unresolvedIssues.isEmpty) { + final numFixed = issues.length - unresolvedIssues.length; + final fixAppliedMessage = + '($numFixed fix${numFixed == 1 ? '' : 'es'} applied)'; + progress.complete( + '''${validator.description} ${green.wrap(fixAppliedMessage)}''', + ); + continue; + } + } else { + allFixableIssues.addAll(issues); } } + + progress.fail(validator.description); + + for (final issue in failedFixes.keys) { + logger.err( + ''' An error occurred while attempting to fix ${issue.message}: ${failedFixes[issue]}''', + ); + } + + for (final issue in unresolvedIssues) { + logger.info(' ${issue.displayMessage}'); + } + + allIssues.addAll(unresolvedIssues); } logger.info(''); - if (numIssues == 0) { + if (allIssues.isEmpty) { logger.info('No issues detected!'); } else { + final numIssues = allIssues.length; logger.info('$numIssues issue${numIssues == 1 ? '' : 's'} detected.'); + + if (allFixableIssues.isNotEmpty && !shouldFix) { + final fixableIssueCount = allFixableIssues.length; + logger.info( + ''' +$fixableIssueCount issue${fixableIssueCount == 1 ? '' : 's'} can be fixed automatically with ${lightCyan.wrap('shorebird doctor --fix')}.''', + ); + } } return ExitCode.success.code; diff --git a/packages/shorebird_cli/lib/src/validators/android_internet_permission_validator.dart b/packages/shorebird_cli/lib/src/validators/android_internet_permission_validator.dart index 87edf760..e585e207 100644 --- a/packages/shorebird_cli/lib/src/validators/android_internet_permission_validator.dart +++ b/packages/shorebird_cli/lib/src/validators/android_internet_permission_validator.dart @@ -44,7 +44,7 @@ class AndroidInternetPermissionValidator extends Validator { (dir) => dir .listSync() .whereType() - .any((file) => p.basename(file.path) == 'AndroidManifest.xml'), + .any((file) => p.basename(file.path) == manifestFileName), ) .map((e) => p.join(e.path, manifestFileName)); @@ -67,6 +67,7 @@ class AndroidInternetPermissionValidator extends Validator { (String manifestPath) => ValidationIssue( severity: ValidationIssueSeverity.error, message: '$manifestPath is missing the INTERNET permission.', + fix: () => _addInternetPermissionToFile(manifestPath), ), ) .toList(); @@ -90,4 +91,20 @@ class AndroidInternetPermissionValidator extends Validator { return attribute.qualifiedName == 'android:name' && attribute.value == 'android.permission.INTERNET'; } + + void _addInternetPermissionToFile(String path) { + final xmlDocument = XmlDocument.parse(File(path).readAsStringSync()); + xmlDocument.rootElement.children.add( + XmlElement( + XmlName('uses-permission'), + [ + XmlAttribute( + XmlName('android:name'), + 'android.permission.INTERNET', + ), + ], + ), + ); + File(path).writeAsStringSync(xmlDocument.toXmlString(pretty: true)); + } } diff --git a/packages/shorebird_cli/lib/src/validators/validators.dart b/packages/shorebird_cli/lib/src/validators/validators.dart index 84feb902..25b603af 100644 --- a/packages/shorebird_cli/lib/src/validators/validators.dart +++ b/packages/shorebird_cli/lib/src/validators/validators.dart @@ -1,3 +1,5 @@ +import 'dart:async'; + import 'package:mason_logger/mason_logger.dart'; import 'package:meta/meta.dart'; import 'package:shorebird_cli/src/shorebird_process.dart'; @@ -30,7 +32,11 @@ extension Display on ValidationIssueSeverity { /// A (potential) problem with the current Shorebird installation or project. @immutable class ValidationIssue { - const ValidationIssue({required this.severity, required this.message}); + const ValidationIssue({ + required this.severity, + required this.message, + this.fix, + }); /// How important it is to fix this issue. final ValidationIssueSeverity severity; @@ -38,6 +44,9 @@ class ValidationIssue { /// A description of the issue. final String message; + /// Fixes this issue. + final FutureOr Function()? fix; + /// A console-friendly description of this issue. String? get displayMessage { return '${severity.leading} $message'; diff --git a/packages/shorebird_cli/test/src/commands/doctor_command_test.dart b/packages/shorebird_cli/test/src/commands/doctor_command_test.dart index a4e8f4a1..db09a60e 100644 --- a/packages/shorebird_cli/test/src/commands/doctor_command_test.dart +++ b/packages/shorebird_cli/test/src/commands/doctor_command_test.dart @@ -1,3 +1,4 @@ +import 'package:args/args.dart'; import 'package:mason_logger/mason_logger.dart'; import 'package:mocktail/mocktail.dart'; import 'package:shorebird_cli/src/commands/commands.dart'; @@ -6,6 +7,8 @@ import 'package:shorebird_cli/src/shorebird_process.dart'; import 'package:shorebird_cli/src/validators/validators.dart'; import 'package:test/test.dart'; +class _MockArgResults extends Mock implements ArgResults {} + class _MockShorebirdVersionValidator extends Mock implements ShorebirdVersionValidator {} @@ -23,6 +26,9 @@ class _MockShorebirdProcess extends Mock implements ShorebirdProcess {} void main() { group('doctor', () { + const androidValidatorDescription = 'Android'; + + late ArgResults argResults; late Logger logger; late Progress progress; late DoctorCommand command; @@ -32,11 +38,14 @@ void main() { late ShorebirdProcess shorebirdProcess; setUp(() { + argResults = _MockArgResults(); logger = _MockLogger(); progress = _MockProgress(); ShorebirdEnvironment.shorebirdEngineRevision = 'test-revision'; + when(() => argResults['fix']).thenReturn(false); + when(() => logger.progress(any())).thenReturn(progress); when(() => logger.info(any())).thenReturn(null); @@ -50,7 +59,7 @@ void main() { when(() => androidInternetPermissionValidator.id) .thenReturn('$AndroidInternetPermissionValidator'); when(() => androidInternetPermissionValidator.description) - .thenReturn('Android'); + .thenReturn(androidValidatorDescription); when(() => androidInternetPermissionValidator.validate(any())) .thenAnswer((_) async => []); @@ -76,6 +85,7 @@ void main() { shorebirdFlutterValidator, ], ) + ..testArgResults = argResults ..testProcess = shorebirdProcess ..testEngineConfig = const EngineConfig.empty(); }); @@ -113,16 +123,191 @@ void main() { } verify( - () => logger.info(any(that: contains('${yellow.wrap('[!]')} oh no!'))), + () => logger.info(any(that: stringContainsInOrder(['[!]', 'oh no!']))), ).called(1); verify( - () => logger.info(any(that: contains('${red.wrap('[✗]')} OH NO!'))), + () => logger.info(any(that: stringContainsInOrder(['[✗]', 'OH NO!']))), ).called(1); verify( () => logger.info(any(that: contains('2 issues detected.'))), ).called(1); }); + + test('tells the user we can fix issues if we can', () async { + when( + () => androidInternetPermissionValidator.validate(any()), + ).thenAnswer( + (_) async => [ + ValidationIssue( + severity: ValidationIssueSeverity.warning, + message: 'oh no!', + fix: () async {}, + ), + ], + ); + + await command.run(); + + verify( + () => logger.info( + any( + that: stringContainsInOrder([ + '1 issue can be fixed automatically', + 'shorebird doctor --fix', + ]), + ), + ), + ).called(1); + }); + + test('does not tell the user we can fix issues if we cannot', () async { + when( + () => androidInternetPermissionValidator.validate(any()), + ).thenAnswer( + (_) async => [ + const ValidationIssue( + severity: ValidationIssueSeverity.warning, + message: 'oh no!', + ), + ], + ); + + await command.run(); + + verifyNever( + () => logger.info( + any( + that: stringContainsInOrder([ + 'We can fix some of these issues', + 'shorebird doctor --fix', + ]), + ), + ), + ); + }); + + test('does not fix issues if --fix flag is not provided', () async { + when(() => argResults['fix']).thenReturn(false); + + var fixCalled = false; + when( + () => androidInternetPermissionValidator.validate(any()), + ).thenAnswer( + (_) async => [ + ValidationIssue( + severity: ValidationIssueSeverity.warning, + message: 'oh no!', + fix: () => fixCalled = true, + ), + ], + ); + + await command.run(); + + expect(fixCalled, isFalse); + verifyNever(() => progress.update('Fixing')); + verify(() => progress.fail(androidValidatorDescription)).called(1); + verify( + () => androidInternetPermissionValidator.validate(any()), + ).called(1); + }); + + test('fixes issues if the --fix flag is provided', () async { + when(() => argResults['fix']).thenReturn(true); + + var fixCalled = false; + final issues = [ + ValidationIssue( + severity: ValidationIssueSeverity.warning, + message: 'oh no!', + fix: () => fixCalled = true, + ), + ]; + when( + () => androidInternetPermissionValidator.validate(any()), + ).thenAnswer( + (_) async { + if (issues.isEmpty) return []; + return [issues.removeLast()]; + }, + ); + + await command.run(); + + expect(fixCalled, isTrue); + verify(() => progress.update('Fixing')).called(1); + verify( + () => progress.complete(any(that: contains('1 fix applied'))), + ).called(1); + verify( + () => androidInternetPermissionValidator.validate(any()), + ).called(2); + }); + + test('does not print "fixed" if fix fails', () async { + when(() => argResults['fix']).thenReturn(true); + + var fixCalled = false; + when( + () => androidInternetPermissionValidator.validate(any()), + ).thenAnswer( + (_) async => [ + ValidationIssue( + severity: ValidationIssueSeverity.warning, + message: 'oh no!', + fix: () => fixCalled = true, + ), + ], + ); + + await command.run(); + + expect(fixCalled, isTrue); + verify(() => progress.update('Fixing')).called(1); + verify(() => progress.fail(androidValidatorDescription)).called(1); + verifyNever( + () => progress.complete(any(that: contains('fix applied'))), + ); + verifyNever( + () => progress.complete(any(that: contains('fixes applied'))), + ); + verify( + () => androidInternetPermissionValidator.validate(any()), + ).called(2); + }); + + test('prints error and continues if fix() throws', () async { + when(() => argResults['fix']).thenReturn(true); + when( + () => androidInternetPermissionValidator.validate(any()), + ).thenAnswer( + (_) async => [ + ValidationIssue( + severity: ValidationIssueSeverity.warning, + message: 'oh no!', + fix: () => throw Exception('oh no!'), + ), + ], + ); + + await command.run(); + + verify(() => progress.update('Fixing')).called(1); + verify( + () => androidInternetPermissionValidator.validate(any()), + ).called(2); + verify( + () => logger.err( + any( + that: stringContainsInOrder([ + 'An error occurred while attempting to fix', + 'oh no!', + ]), + ), + ), + ).called(1); + }); }); } diff --git a/packages/shorebird_cli/test/src/validators/android_internet_permission_validator_test.dart b/packages/shorebird_cli/test/src/validators/android_internet_permission_validator_test.dart index 18a55500..9c189c8b 100644 --- a/packages/shorebird_cli/test/src/validators/android_internet_permission_validator_test.dart +++ b/packages/shorebird_cli/test/src/validators/android_internet_permission_validator_test.dart @@ -82,6 +82,7 @@ void main() { expect(results, hasLength(1)); expect(results.first.severity, ValidationIssueSeverity.error); expect(results.first.message, 'No Android project found'); + expect(results.first.fix, isNull); }); test('returns an error if no AndroidManifest.xml files are found', @@ -101,6 +102,7 @@ void main() { results.first.message, startsWith('No AndroidManifest.xml files found in'), ); + expect(results.first.fix, isNull); }); test( @@ -165,5 +167,31 @@ void main() { ); }, ); + + test('fix() adds permission to manifest file', () async { + final tempDirectory = createTempDir(); + writeManifestToPath( + manifestWithNonInternetPermissions, + p.join(tempDirectory.path, 'android', 'app', 'src', 'debug'), + ); + + var results = await IOOverrides.runZoned( + () => AndroidInternetPermissionValidator().validate(shorebirdProcess), + getCurrentDirectory: () => tempDirectory, + ); + expect(results, hasLength(1)); + expect(results.first.fix, isNotNull); + + await IOOverrides.runZoned( + () => results.first.fix!(), + getCurrentDirectory: () => tempDirectory, + ); + + results = await IOOverrides.runZoned( + () => AndroidInternetPermissionValidator().validate(shorebirdProcess), + getCurrentDirectory: () => tempDirectory, + ); + expect(results, isEmpty); + }); }); }