Skip to content

Commit 14bcc69

Browse files
andrewkolosandrewkolos
authored
Fix AssetsEntry::equals (#143355)
In service of flutter/flutter#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.
1 parent 4280d29 commit 14bcc69

5 files changed

Lines changed: 229 additions & 150 deletions

File tree

‎packages/flutter_tools/lib/src/asset.dart‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -949,7 +949,7 @@ class ManifestAssetBundle implements AssetBundle {
949949
Uri assetUri, {
950950
String? packageName,
951951
Package? attributedPackage,
952-
List<String>? flavors,
952+
Set<String>? flavors,
953953
}) {
954954
final String directoryPath;
955955
try {
@@ -1000,7 +1000,7 @@ class ManifestAssetBundle implements AssetBundle {
10001000
String? packageName,
10011001
Package? attributedPackage,
10021002
AssetKind assetKind = AssetKind.regular,
1003-
List<String>? flavors,
1003+
Set<String>? flavors,
10041004
}) {
10051005
final _Asset asset = _resolveAsset(
10061006
packageConfig,
@@ -1116,7 +1116,7 @@ class ManifestAssetBundle implements AssetBundle {
11161116
Package? attributedPackage, {
11171117
Uri? originUri,
11181118
AssetKind assetKind = AssetKind.regular,
1119-
List<String>? flavors,
1119+
Set<String>? flavors,
11201120
}) {
11211121
final String assetPath = _fileSystem.path.fromUri(assetUri);
11221122
if (assetUri.pathSegments.first == 'packages'
@@ -1155,7 +1155,7 @@ class ManifestAssetBundle implements AssetBundle {
11551155
Package? attributedPackage, {
11561156
AssetKind assetKind = AssetKind.regular,
11571157
Uri? originUri,
1158-
List<String>? flavors,
1158+
Set<String>? flavors,
11591159
}) {
11601160
assert(assetUri.pathSegments.first == 'packages');
11611161
if (assetUri.pathSegments.length > 1) {
@@ -1192,8 +1192,8 @@ class _Asset {
11921192
required this.entryUri,
11931193
required this.package,
11941194
this.kind = AssetKind.regular,
1195-
List<String>? flavors,
1196-
}): originUri = originUri ?? entryUri, flavors = flavors ?? const <String>[];
1195+
Set<String>? flavors,
1196+
}): originUri = originUri ?? entryUri, flavors = flavors ?? const <String>{};
11971197

11981198
final String baseDir;
11991199

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

12131213
final AssetKind kind;
12141214

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

12171217
File lookupAssetFile(FileSystem fileSystem) {
12181218
return fileSystem.file(fileSystem.path.join(baseDir, fileSystem.path.fromUri(relativeUri)));

‎packages/flutter_tools/lib/src/base/utils.dart‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -479,3 +479,22 @@ Match? firstMatchInFile(File file, RegExp regExp) {
479479
}
480480
return null;
481481
}
482+
483+
/// Tests for shallow equality on two sets.
484+
bool setEquals<T>(Set<T>? a, Set<T>? b) {
485+
if (a == null) {
486+
return b == null;
487+
}
488+
if (b == null || a.length != b.length) {
489+
return false;
490+
}
491+
if (identical(a, b)) {
492+
return true;
493+
}
494+
for (final T value in a) {
495+
if (!b.contains(value)) {
496+
return false;
497+
}
498+
}
499+
return true;
500+
}

‎packages/flutter_tools/lib/src/flutter_manifest.dart‎

Lines changed: 32 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -519,17 +519,7 @@ void _validateFlutter(YamlMap? yaml, List<String> errors) {
519519
_validateFonts(yamlValue, errors);
520520
}
521521
case 'licenses':
522-
if (yamlValue is! YamlList) {
523-
errors.add('Expected "$yamlKey" to be a list of files, but got $yamlValue (${yamlValue.runtimeType})');
524-
} else if (yamlValue.isEmpty) {
525-
break;
526-
} else if (yamlValue.first is! String) {
527-
errors.add(
528-
'Expected "$yamlKey" to contain strings, but the first element is $yamlValue (${yamlValue.runtimeType}).',
529-
);
530-
} else {
531-
_validateListType<String>(yamlValue, errors, '"$yamlKey"', 'files');
532-
}
522+
errors.addAll(_validateList<String>(yamlValue, '"$yamlKey"', 'files'));
533523
case 'module':
534524
if (yamlValue is! YamlMap) {
535525
errors.add('Expected "$yamlKey" to be an object, but got $yamlValue (${yamlValue.runtimeType}).');
@@ -563,15 +553,22 @@ void _validateFlutter(YamlMap? yaml, List<String> errors) {
563553
}
564554
}
565555

566-
void _validateListType<T>(YamlList yamlList, List<String> errors, String context, String typeAlias) {
556+
List<String> _validateList<T>(Object? yamlList, String context, String typeAlias) {
557+
final List<String> errors = <String>[];
558+
559+
if (yamlList is! YamlList) {
560+
return <String>['Expected $context to be a list of $typeAlias, but got $yamlList (${yamlList.runtimeType}).'];
561+
}
562+
567563
for (int i = 0; i < yamlList.length; i++) {
568564
if (yamlList[i] is! T) {
569565
// ignore: avoid_dynamic_calls
570-
errors.add('Expected $context to be a list of $typeAlias, but element $i was a ${yamlList[i].runtimeType}');
566+
errors.add('Expected $context to be a list of $typeAlias, but element at index $i was a ${yamlList[i].runtimeType}.');
571567
}
572568
}
573-
}
574569

570+
return errors;
571+
}
575572
void _validateDeferredComponents(MapEntry<Object?, Object?> kvp, List<String> errors) {
576573
final Object? yamlList = kvp.value;
577574
if (yamlList != null && (yamlList is! YamlList || yamlList[0] is! YamlMap)) {
@@ -588,12 +585,11 @@ void _validateDeferredComponents(MapEntry<Object?, Object?> kvp, List<String> er
588585
errors.add('Expected the $i element in "${kvp.key}" to have required key "name" of type String');
589586
}
590587
if (valueMap.containsKey('libraries')) {
591-
final Object? libraries = valueMap['libraries'];
592-
if (libraries is! YamlList) {
593-
errors.add('Expected "libraries" key in the $i element of "${kvp.key}" to be a list, but got $libraries (${libraries.runtimeType}).');
594-
} else {
595-
_validateListType<String>(libraries, errors, '"libraries" key in the $i element of "${kvp.key}"', 'dart library Strings');
596-
}
588+
errors.addAll(_validateList<String>(
589+
valueMap['libraries'],
590+
'"libraries" key in the element at index $i of "${kvp.key}"',
591+
'String',
592+
));
597593
}
598594
if (valueMap.containsKey('assets')) {
599595
errors.addAll(_validateAssets(valueMap['assets']));
@@ -700,11 +696,11 @@ void _validateFonts(YamlList fonts, List<String> errors) {
700696
class AssetsEntry {
701697
const AssetsEntry({
702698
required this.uri,
703-
this.flavors = const <String>[],
699+
this.flavors = const <String>{},
704700
});
705701

706702
final Uri uri;
707-
final List<String> flavors;
703+
final Set<String> flavors;
708704

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

765-
final List<String> flavorsListErrors = <String>[];
766-
_validateListType<String>(flavors, flavorsListErrors, 'flavors list of entry "$path"', 'String');
761+
final List<String> flavorsListErrors = _validateList<String>(
762+
flavors,
763+
'flavors list of entry "$path"',
764+
'String',
765+
);
767766
if (flavorsListErrors.isNotEmpty) {
768767
return (null, 'Asset manifest entry is malformed. '
769768
'Expected "$_flavorKey" entry to be a list of strings.\n'
@@ -772,7 +771,7 @@ class AssetsEntry {
772771

773772
final AssetsEntry entry = AssetsEntry(
774773
uri: Uri(pathSegments: path.split('/')),
775-
flavors: List<String>.from(flavors),
774+
flavors: Set<String>.from(flavors),
776775
);
777776

778777
return (entry, null);
@@ -788,9 +787,15 @@ class AssetsEntry {
788787
return false;
789788
}
790789

791-
return uri == other.uri && flavors == other.flavors;
790+
return uri == other.uri && setEquals(flavors, other.flavors);
792791
}
793792

794793
@override
795-
int get hashCode => Object.hash(uri.hashCode, flavors.hashCode);
794+
int get hashCode => Object.hashAll(<Object?>[
795+
uri.hashCode,
796+
Object.hashAllUnordered(flavors),
797+
]);
798+
799+
@override
800+
String toString() => 'AssetsEntry(uri: $uri, flavors: $flavors)';
796801
}

‎packages/flutter_tools/test/general.shard/flutter_manifest_assets_test.dart‎

Lines changed: 162 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,162 @@
1+
// Copyright 2014 The Flutter Authors. All rights reserved.
2+
// Use of this source code is governed by a BSD-style license that can be
3+
// found in the LICENSE file.
4+
5+
import 'package:flutter_tools/src/base/logger.dart';
6+
import 'package:flutter_tools/src/flutter_manifest.dart';
7+
8+
import '../src/common.dart';
9+
10+
void main() {
11+
group('parsing of assets section in flutter manifests', () {
12+
testWithoutContext('ignores empty list of assets', () {
13+
final BufferLogger logger = BufferLogger.test();
14+
15+
const String manifest = '''
16+
name: test
17+
dependencies:
18+
flutter:
19+
sdk: flutter
20+
flutter:
21+
assets: []
22+
''';
23+
24+
final FlutterManifest? flutterManifest = FlutterManifest.createFromString(
25+
manifest,
26+
logger: logger,
27+
);
28+
29+
expect(flutterManifest, isNotNull);
30+
expect(flutterManifest!.assets, isEmpty);
31+
});
32+
33+
testWithoutContext('parses two simple asset declarations', () async {
34+
final BufferLogger logger = BufferLogger.test();
35+
const String manifest = '''
36+
name: test
37+
dependencies:
38+
flutter:
39+
sdk: flutter
40+
flutter:
41+
uses-material-design: true
42+
assets:
43+
- a/foo
44+
- a/bar
45+
''';
46+
47+
final FlutterManifest flutterManifest = FlutterManifest.createFromString(
48+
manifest,
49+
logger: logger,
50+
)!;
51+
52+
expect(flutterManifest.assets, <AssetsEntry>[
53+
AssetsEntry(uri: Uri.parse('a/foo')),
54+
AssetsEntry(uri: Uri.parse('a/bar')),
55+
]);
56+
});
57+
58+
testWithoutContext('does not crash on empty entry', () {
59+
final BufferLogger logger = BufferLogger.test();
60+
const String manifest = '''
61+
name: test
62+
dependencies:
63+
flutter:
64+
sdk: flutter
65+
flutter:
66+
uses-material-design: true
67+
assets:
68+
- lib/gallery/example_code.dart
69+
-
70+
''';
71+
72+
FlutterManifest.createFromString(
73+
manifest,
74+
logger: logger,
75+
);
76+
77+
expect(logger.errorText, contains('Asset manifest contains a null or empty uri.'));
78+
});
79+
80+
testWithoutContext('handles special characters in asset URIs', () {
81+
final BufferLogger logger = BufferLogger.test();
82+
83+
const String manifest = '''
84+
name: test
85+
dependencies:
86+
flutter:
87+
sdk: flutter
88+
flutter:
89+
uses-material-design: true
90+
assets:
91+
- lib/gallery/abc#xyz
92+
- lib/gallery/abc?xyz
93+
- lib/gallery/aaa bbb
94+
''';
95+
96+
final FlutterManifest flutterManifest = FlutterManifest.createFromString(
97+
manifest,
98+
logger: logger,
99+
)!;
100+
final List<AssetsEntry> assets = flutterManifest.assets;
101+
102+
expect(assets, <AssetsEntry>[
103+
AssetsEntry(uri: Uri.parse('lib/gallery/abc%23xyz')),
104+
AssetsEntry(uri: Uri.parse('lib/gallery/abc%3Fxyz')),
105+
AssetsEntry(uri: Uri.parse('lib/gallery/aaa%20bbb')),
106+
]);
107+
});
108+
109+
testWithoutContext('parses an asset with flavors', () async {
110+
final BufferLogger logger = BufferLogger.test();
111+
const String manifest = '''
112+
name: test
113+
dependencies:
114+
flutter:
115+
sdk: flutter
116+
flutter:
117+
uses-material-design: true
118+
assets:
119+
- path: a/foo
120+
flavors:
121+
- apple
122+
- strawberry
123+
''';
124+
125+
final FlutterManifest flutterManifest = FlutterManifest.createFromString(
126+
manifest,
127+
logger: logger,
128+
)!;
129+
130+
expect(flutterManifest.assets, <AssetsEntry>[
131+
AssetsEntry(
132+
uri: Uri.parse('a/foo'),
133+
flavors: const <String>{'apple', 'strawberry'},
134+
),
135+
]);
136+
});
137+
138+
testWithoutContext("prints an error when an asset entry's flavor is not a string", () async {
139+
final BufferLogger logger = BufferLogger.test();
140+
141+
const String manifest = '''
142+
name: test
143+
dependencies:
144+
flutter:
145+
sdk: flutter
146+
flutter:
147+
uses-material-design: true
148+
assets:
149+
- assets/folder/
150+
- path: assets/vanilla/
151+
flavors:
152+
- key1: value1
153+
key2: value2
154+
''';
155+
FlutterManifest.createFromString(manifest, logger: logger);
156+
expect(logger.errorText, contains(
157+
'Asset manifest entry is malformed. '
158+
'Expected "flavors" entry to be a list of strings.',
159+
));
160+
});
161+
});
162+
}

0 commit comments

Comments
 (0)