From 14bcc694ff7b5c3109b7988a0bc3ec58c55da97a Mon Sep 17 00:00:00 2001 From: Andrew Kolos Date: Tue, 13 Feb 2024 16:11:24 -0800 Subject: [PATCH] Fix `AssetsEntry::equals` (#143355) In service of https://github.com/flutter/flutter/issues/143348. **Issue.** The `equals` implementation of `AssetsEntry` is incorrect. It compares `flavors` lists using reference equality. This PR addresses this. This also adds a test to make sure valid asset `flavors` declarations are parsed correctly. While we are here, this PR also includes a couple of refactorings: * `flutter_manifest_test.dart` is a bit large. To better match our style guide, I've factored out some related tests into their own file. * A couple of changes to the `_validateListType` function in `flutter_manifest.dart`: * The function now returns a list of errors instead of accepting a list to append onto. This is more readable and also allows callers to know which errors were found by the call. * The function is renamed to `_validateList` and now accepts an `Object?` instead of an `YamlList`. If the argument is null, an appropriate error message is contained in the output. This saves callers that are only interested in validation from having to write their own null-check, which they all did before. * Some error strings were tweaked for increased readability and/or grammatical correctness. --- packages/flutter_tools/lib/src/asset.dart | 14 +- .../flutter_tools/lib/src/base/utils.dart | 19 ++ .../lib/src/flutter_manifest.dart | 59 ++++--- .../flutter_manifest_assets_test.dart | 162 ++++++++++++++++++ .../general.shard/flutter_manifest_test.dart | 125 +------------- 5 files changed, 229 insertions(+), 150 deletions(-) create mode 100644 packages/flutter_tools/test/general.shard/flutter_manifest_assets_test.dart diff --git a/packages/flutter_tools/lib/src/asset.dart b/packages/flutter_tools/lib/src/asset.dart index 2f12d160ce..bc38ef8461 100644 --- a/packages/flutter_tools/lib/src/asset.dart +++ b/packages/flutter_tools/lib/src/asset.dart @@ -949,7 +949,7 @@ class ManifestAssetBundle implements AssetBundle { Uri assetUri, { String? packageName, Package? attributedPackage, - List? flavors, + Set? flavors, }) { final String directoryPath; try { @@ -1000,7 +1000,7 @@ class ManifestAssetBundle implements AssetBundle { String? packageName, Package? attributedPackage, AssetKind assetKind = AssetKind.regular, - List? flavors, + Set? flavors, }) { final _Asset asset = _resolveAsset( packageConfig, @@ -1116,7 +1116,7 @@ class ManifestAssetBundle implements AssetBundle { Package? attributedPackage, { Uri? originUri, AssetKind assetKind = AssetKind.regular, - List? flavors, + Set? flavors, }) { final String assetPath = _fileSystem.path.fromUri(assetUri); if (assetUri.pathSegments.first == 'packages' @@ -1155,7 +1155,7 @@ class ManifestAssetBundle implements AssetBundle { Package? attributedPackage, { AssetKind assetKind = AssetKind.regular, Uri? originUri, - List? flavors, + Set? flavors, }) { assert(assetUri.pathSegments.first == 'packages'); if (assetUri.pathSegments.length > 1) { @@ -1192,8 +1192,8 @@ class _Asset { required this.entryUri, required this.package, this.kind = AssetKind.regular, - List? flavors, - }): originUri = originUri ?? entryUri, flavors = flavors ?? const []; + Set? flavors, + }): originUri = originUri ?? entryUri, flavors = flavors ?? const {}; final String baseDir; @@ -1212,7 +1212,7 @@ class _Asset { final AssetKind kind; - final List flavors; + final Set flavors; File lookupAssetFile(FileSystem fileSystem) { return fileSystem.file(fileSystem.path.join(baseDir, fileSystem.path.fromUri(relativeUri))); diff --git a/packages/flutter_tools/lib/src/base/utils.dart b/packages/flutter_tools/lib/src/base/utils.dart index 46d0ef38fa..ad522ddb03 100644 --- a/packages/flutter_tools/lib/src/base/utils.dart +++ b/packages/flutter_tools/lib/src/base/utils.dart @@ -479,3 +479,22 @@ Match? firstMatchInFile(File file, RegExp regExp) { } return null; } + +/// Tests for shallow equality on two sets. +bool setEquals(Set? a, Set? b) { + if (a == null) { + return b == null; + } + if (b == null || a.length != b.length) { + return false; + } + if (identical(a, b)) { + return true; + } + for (final T value in a) { + if (!b.contains(value)) { + return false; + } + } + return true; +} diff --git a/packages/flutter_tools/lib/src/flutter_manifest.dart b/packages/flutter_tools/lib/src/flutter_manifest.dart index 106c402ea7..5ce2f72661 100644 --- a/packages/flutter_tools/lib/src/flutter_manifest.dart +++ b/packages/flutter_tools/lib/src/flutter_manifest.dart @@ -519,17 +519,7 @@ void _validateFlutter(YamlMap? yaml, List errors) { _validateFonts(yamlValue, errors); } case 'licenses': - if (yamlValue is! YamlList) { - errors.add('Expected "$yamlKey" to be a list of files, but got $yamlValue (${yamlValue.runtimeType})'); - } else if (yamlValue.isEmpty) { - break; - } else if (yamlValue.first is! String) { - errors.add( - 'Expected "$yamlKey" to contain strings, but the first element is $yamlValue (${yamlValue.runtimeType}).', - ); - } else { - _validateListType(yamlValue, errors, '"$yamlKey"', 'files'); - } + errors.addAll(_validateList(yamlValue, '"$yamlKey"', 'files')); case 'module': if (yamlValue is! YamlMap) { errors.add('Expected "$yamlKey" to be an object, but got $yamlValue (${yamlValue.runtimeType}).'); @@ -563,15 +553,22 @@ void _validateFlutter(YamlMap? yaml, List errors) { } } -void _validateListType(YamlList yamlList, List errors, String context, String typeAlias) { +List _validateList(Object? yamlList, String context, String typeAlias) { + final List errors = []; + + if (yamlList is! YamlList) { + return ['Expected $context to be a list of $typeAlias, but got $yamlList (${yamlList.runtimeType}).']; + } + for (int i = 0; i < yamlList.length; i++) { if (yamlList[i] is! T) { // ignore: avoid_dynamic_calls - errors.add('Expected $context to be a list of $typeAlias, but element $i was a ${yamlList[i].runtimeType}'); + errors.add('Expected $context to be a list of $typeAlias, but element at index $i was a ${yamlList[i].runtimeType}.'); } } -} + return errors; +} void _validateDeferredComponents(MapEntry kvp, List errors) { final Object? yamlList = kvp.value; if (yamlList != null && (yamlList is! YamlList || yamlList[0] is! YamlMap)) { @@ -588,12 +585,11 @@ void _validateDeferredComponents(MapEntry kvp, List er errors.add('Expected the $i element in "${kvp.key}" to have required key "name" of type String'); } if (valueMap.containsKey('libraries')) { - final Object? libraries = valueMap['libraries']; - if (libraries is! YamlList) { - errors.add('Expected "libraries" key in the $i element of "${kvp.key}" to be a list, but got $libraries (${libraries.runtimeType}).'); - } else { - _validateListType(libraries, errors, '"libraries" key in the $i element of "${kvp.key}"', 'dart library Strings'); - } + errors.addAll(_validateList( + valueMap['libraries'], + '"libraries" key in the element at index $i of "${kvp.key}"', + 'String', + )); } if (valueMap.containsKey('assets')) { errors.addAll(_validateAssets(valueMap['assets'])); @@ -700,11 +696,11 @@ void _validateFonts(YamlList fonts, List errors) { class AssetsEntry { const AssetsEntry({ required this.uri, - this.flavors = const [], + this.flavors = const {}, }); final Uri uri; - final List flavors; + final Set flavors; static const String _pathKey = 'path'; static const String _flavorKey = 'flavors'; @@ -762,8 +758,11 @@ class AssetsEntry { 'Got ${flavors.runtimeType} instead.'); } - final List flavorsListErrors = []; - _validateListType(flavors, flavorsListErrors, 'flavors list of entry "$path"', 'String'); + final List flavorsListErrors = _validateList( + flavors, + 'flavors list of entry "$path"', + 'String', + ); if (flavorsListErrors.isNotEmpty) { return (null, 'Asset manifest entry is malformed. ' 'Expected "$_flavorKey" entry to be a list of strings.\n' @@ -772,7 +771,7 @@ class AssetsEntry { final AssetsEntry entry = AssetsEntry( uri: Uri(pathSegments: path.split('/')), - flavors: List.from(flavors), + flavors: Set.from(flavors), ); return (entry, null); @@ -788,9 +787,15 @@ class AssetsEntry { return false; } - return uri == other.uri && flavors == other.flavors; + return uri == other.uri && setEquals(flavors, other.flavors); } @override - int get hashCode => Object.hash(uri.hashCode, flavors.hashCode); + int get hashCode => Object.hashAll([ + uri.hashCode, + Object.hashAllUnordered(flavors), + ]); + + @override + String toString() => 'AssetsEntry(uri: $uri, flavors: $flavors)'; } diff --git a/packages/flutter_tools/test/general.shard/flutter_manifest_assets_test.dart b/packages/flutter_tools/test/general.shard/flutter_manifest_assets_test.dart new file mode 100644 index 0000000000..3d76855fd8 --- /dev/null +++ b/packages/flutter_tools/test/general.shard/flutter_manifest_assets_test.dart @@ -0,0 +1,162 @@ +// Copyright 2014 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +import 'package:flutter_tools/src/base/logger.dart'; +import 'package:flutter_tools/src/flutter_manifest.dart'; + +import '../src/common.dart'; + +void main() { + group('parsing of assets section in flutter manifests', () { + testWithoutContext('ignores empty list of assets', () { + final BufferLogger logger = BufferLogger.test(); + + const String manifest = ''' +name: test +dependencies: + flutter: + sdk: flutter +flutter: + assets: [] +'''; + + final FlutterManifest? flutterManifest = FlutterManifest.createFromString( + manifest, + logger: logger, + ); + + expect(flutterManifest, isNotNull); + expect(flutterManifest!.assets, isEmpty); + }); + + testWithoutContext('parses two simple asset declarations', () async { + final BufferLogger logger = BufferLogger.test(); + const String manifest = ''' +name: test +dependencies: + flutter: + sdk: flutter +flutter: + uses-material-design: true + assets: + - a/foo + - a/bar +'''; + + final FlutterManifest flutterManifest = FlutterManifest.createFromString( + manifest, + logger: logger, + )!; + + expect(flutterManifest.assets, [ + AssetsEntry(uri: Uri.parse('a/foo')), + AssetsEntry(uri: Uri.parse('a/bar')), + ]); + }); + + testWithoutContext('does not crash on empty entry', () { + final BufferLogger logger = BufferLogger.test(); + const String manifest = ''' +name: test +dependencies: + flutter: + sdk: flutter +flutter: + uses-material-design: true + assets: + - lib/gallery/example_code.dart + - +'''; + + FlutterManifest.createFromString( + manifest, + logger: logger, + ); + + expect(logger.errorText, contains('Asset manifest contains a null or empty uri.')); + }); + + testWithoutContext('handles special characters in asset URIs', () { + final BufferLogger logger = BufferLogger.test(); + + const String manifest = ''' +name: test +dependencies: + flutter: + sdk: flutter +flutter: + uses-material-design: true + assets: + - lib/gallery/abc#xyz + - lib/gallery/abc?xyz + - lib/gallery/aaa bbb +'''; + + final FlutterManifest flutterManifest = FlutterManifest.createFromString( + manifest, + logger: logger, + )!; + final List assets = flutterManifest.assets; + + expect(assets, [ + AssetsEntry(uri: Uri.parse('lib/gallery/abc%23xyz')), + AssetsEntry(uri: Uri.parse('lib/gallery/abc%3Fxyz')), + AssetsEntry(uri: Uri.parse('lib/gallery/aaa%20bbb')), + ]); + }); + + testWithoutContext('parses an asset with flavors', () async { + final BufferLogger logger = BufferLogger.test(); + const String manifest = ''' +name: test +dependencies: + flutter: + sdk: flutter +flutter: + uses-material-design: true + assets: + - path: a/foo + flavors: + - apple + - strawberry +'''; + + final FlutterManifest flutterManifest = FlutterManifest.createFromString( + manifest, + logger: logger, + )!; + + expect(flutterManifest.assets, [ + AssetsEntry( + uri: Uri.parse('a/foo'), + flavors: const {'apple', 'strawberry'}, + ), + ]); + }); + + testWithoutContext("prints an error when an asset entry's flavor is not a string", () async { + final BufferLogger logger = BufferLogger.test(); + + const String manifest = ''' +name: test +dependencies: + flutter: + sdk: flutter +flutter: + uses-material-design: true + assets: + - assets/folder/ + - path: assets/vanilla/ + flavors: + - key1: value1 + key2: value2 +'''; + FlutterManifest.createFromString(manifest, logger: logger); + expect(logger.errorText, contains( + 'Asset manifest entry is malformed. ' + 'Expected "flavors" entry to be a list of strings.', + )); + }); + }); +} diff --git a/packages/flutter_tools/test/general.shard/flutter_manifest_test.dart b/packages/flutter_tools/test/general.shard/flutter_manifest_test.dart index dc048f5c8f..a7b8ad7a98 100644 --- a/packages/flutter_tools/test/general.shard/flutter_manifest_test.dart +++ b/packages/flutter_tools/test/general.shard/flutter_manifest_test.dart @@ -136,50 +136,6 @@ flutter: expect(flutterManifest.generateSyntheticPackage, false); }); - testWithoutContext('FlutterManifest has two assets', () async { - const String manifest = ''' -name: test -dependencies: - flutter: - sdk: flutter -flutter: - uses-material-design: true - assets: - - a/foo - - a/bar -'''; - - final FlutterManifest flutterManifest = FlutterManifest.createFromString( - manifest, - logger: logger, - )!; - - expect(flutterManifest.assets, [ - AssetsEntry(uri: Uri.parse('a/foo')), - AssetsEntry(uri: Uri.parse('a/bar')), - ]); - }); - - testWithoutContext('FlutterManifest assets entry flavor is not a string', () async { - const String manifest = ''' -name: test -dependencies: - flutter: - sdk: flutter -flutter: - uses-material-design: true - assets: - - assets/folder/ - - path: assets/vanilla/ - flavors: - - key1: value1 - key2: value2 -'''; - FlutterManifest.createFromString(manifest, logger: logger); - expect(logger.errorText, contains('Asset manifest entry is malformed. ' - 'Expected "flavors" entry to be a list of strings.')); - }); - testWithoutContext('FlutterManifest has one font family with one asset', () async { const String manifest = ''' name: test @@ -796,25 +752,6 @@ flutter: expect(flutterManifest!.fonts.length, 0); }); - testWithoutContext('FlutterManifest ignores empty list of assets', () { - const String manifest = ''' -name: test -dependencies: - flutter: - sdk: flutter -flutter: - assets: [] -'''; - - final FlutterManifest? flutterManifest = FlutterManifest.createFromString( - manifest, - logger: logger, - ); - - expect(flutterManifest, isNotNull); - expect(flutterManifest!.assets.length, 0); - }); - testWithoutContext('FlutterManifest returns proper error when font detail is ' 'not a list of maps', () { const String manifest = ''' @@ -887,55 +824,6 @@ flutter: expect(logger.errorText, contains('Expected a map.')); }); - testWithoutContext('FlutterManifest does not crash on empty entry', () { - const String manifest = ''' -name: test -dependencies: - flutter: - sdk: flutter -flutter: - uses-material-design: true - assets: - - lib/gallery/example_code.dart - - -'''; - - FlutterManifest.createFromString( - manifest, - logger: logger, - ); - - expect(logger.errorText, contains('Asset manifest contains a null or empty uri.')); - }); - - testWithoutContext('FlutterManifest handles special characters in asset URIs', () { - const String manifest = ''' -name: test -dependencies: - flutter: - sdk: flutter -flutter: - uses-material-design: true - assets: - - lib/gallery/abc#xyz - - lib/gallery/abc?xyz - - lib/gallery/aaa bbb -'''; - - final FlutterManifest flutterManifest = FlutterManifest.createFromString( - manifest, - logger: logger, - )!; - final List assets = flutterManifest.assets; - - expect(assets, hasLength(3)); - expect(assets, [ - AssetsEntry(uri: Uri.parse('lib/gallery/abc%23xyz')), - AssetsEntry(uri: Uri.parse('lib/gallery/abc%3Fxyz')), - AssetsEntry(uri: Uri.parse('lib/gallery/aaa%20bbb')), - ]); - }); - testWithoutContext('FlutterManifest returns proper error when flutter is a ' 'list instead of a map', () { const String manifest = ''' @@ -1186,7 +1074,7 @@ flutter: ); expect(flutterManifest, null); - expect(logger.errorText, 'Expected "licenses" to be a list of files, but got foo.txt (String)\n'); + expect(logger.errorText, 'Expected "licenses" to be a list of files, but got foo.txt (String).\n'); }); testWithoutContext('FlutterManifest validates individual list items', () async { @@ -1207,7 +1095,8 @@ flutter: ); expect(flutterManifest, null); - expect(logger.errorText, 'Expected "licenses" to be a list of files, but element 1 was a YamlMap\n'); + expect(logger.errorText, 'Expected "licenses" to be a list of files, but ' + 'element at index 1 was a YamlMap.\n'); }); testWithoutContext('FlutterManifest parses single deferred components', () async { @@ -1360,7 +1249,9 @@ flutter: ); expect(flutterManifest, null); - expect(logger.errorText, 'Expected "libraries" key in the 0 element of "deferred-components" to be a list, but got blah (String).\n'); + expect(logger.errorText, 'Expected "libraries" key in the element at ' + 'index 0 of "deferred-components" to be a list of String, but ' + 'got blah (String).\n'); }); testWithoutContext('FlutterManifest deferred component libraries is string', () async { @@ -1382,7 +1273,9 @@ flutter: ); expect(flutterManifest, null); - expect(logger.errorText, 'Expected "libraries" key in the 0 element of "deferred-components" to be a list of dart library Strings, but element 0 was a YamlMap\n'); + expect(logger.errorText, 'Expected "libraries" key in the element at ' + 'index 0 of "deferred-components" to be a list of String, but ' + 'element at index 0 was a YamlMap.\n'); }); testWithoutContext('FlutterManifest deferred component assets is string', () async {