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.
2024-02-13 16:11:24 -08:00 · 2024-02-13 16:11:24 -08:00 · 14bcc694ff
commit 14bcc694ff
parent 4280d29534
5 changed files with 229 additions and 150 deletions
--- 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<String>? flavors,
+    Set<String>? flavors,
  }) {
    final String directoryPath;
    try {
@ -1000,7 +1000,7 @@ class ManifestAssetBundle implements AssetBundle {
    String? packageName,
    Package? attributedPackage,
    AssetKind assetKind = AssetKind.regular,
-    List<String>? flavors,
+    Set<String>? flavors,
  }) {
    final _Asset asset = _resolveAsset(
      packageConfig,
@ -1116,7 +1116,7 @@ class ManifestAssetBundle implements AssetBundle {
    Package? attributedPackage, {
    Uri? originUri,
    AssetKind assetKind = AssetKind.regular,
-    List<String>? flavors,
+    Set<String>? 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<String>? flavors,
+    Set<String>? 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<String>? flavors,
-  }): originUri = originUri ?? entryUri, flavors = flavors ?? const <String>[];
+    Set<String>? flavors,
+  }): originUri = originUri ?? entryUri, flavors = flavors ?? const <String>{};

  final String baseDir;

@ -1212,7 +1212,7 @@ class _Asset {

  final AssetKind kind;

-  final List<String> flavors;
+  final Set<String> flavors;

  File lookupAssetFile(FileSystem fileSystem) {
    return fileSystem.file(fileSystem.path.join(baseDir, fileSystem.path.fromUri(relativeUri)));
--- 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<T>(Set<T>? a, Set<T>? 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;
+}
--- 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<String> 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<String>(yamlValue, errors, '"$yamlKey"', 'files');
-        }
+        errors.addAll(_validateList<String>(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<String> errors) {
  }
 }

-void _validateListType<T>(YamlList yamlList, List<String> errors, String context, String typeAlias) {
+List<String> _validateList<T>(Object? yamlList, String context, String typeAlias) {
+  final List<String> errors = <String>[];
+
+  if (yamlList is! YamlList) {
+    return <String>['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<Object?, Object?> kvp, List<String> errors) {
  final Object? yamlList = kvp.value;
  if (yamlList != null && (yamlList is! YamlList || yamlList[0] is! YamlMap)) {
@ -588,12 +585,11 @@ void _validateDeferredComponents(MapEntry<Object?, Object?> kvp, List<String> 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<String>(libraries, errors, '"libraries" key in the $i element of "${kvp.key}"', 'dart library Strings');
-        }
+        errors.addAll(_validateList<String>(
+          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<String> errors) {
 class AssetsEntry {
  const AssetsEntry({
    required this.uri,
-    this.flavors = const <String>[],
+    this.flavors = const <String>{},
  });

  final Uri uri;
-  final List<String> flavors;
+  final Set<String> flavors;

  static const String _pathKey = 'path';
  static const String _flavorKey = 'flavors';
@ -762,8 +758,11 @@ class AssetsEntry {
          'Got ${flavors.runtimeType} instead.');
      }

-      final List<String> flavorsListErrors = <String>[];
-      _validateListType<String>(flavors, flavorsListErrors, 'flavors list of entry "$path"', 'String');
+      final List<String> flavorsListErrors = _validateList<String>(
+        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<String>.from(flavors),
+        flavors: Set<String>.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(<Object?>[
+    uri.hashCode,
+    Object.hashAllUnordered(flavors),
+  ]);
+
+  @override
+  String toString() => 'AssetsEntry(uri: $uri, flavors: $flavors)';
 }
--- a/packages/flutter_tools/test/general.shard/flutter_manifest_assets_test.dart
+++ 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>[
+        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<AssetsEntry> assets = flutterManifest.assets;
+
+      expect(assets, <AssetsEntry>[
+        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>[
+        AssetsEntry(
+          uri: Uri.parse('a/foo'),
+          flavors: const <String>{'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.',
+      ));
+    });
+  });
+}
--- 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>[
-      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<AssetsEntry> assets = flutterManifest.assets;
-
-    expect(assets, hasLength(3));
-    expect(assets, <AssetsEntry>[
-      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 {