Handle AssetIds with invalid URI characters

Fixes #1134

Rewrite the URI encoding for AssetId to be more robust by using the
built in URI escaping when using `pathSegments` instead of `path`.

Delete a duplicate of the AssetId test file.

Author: natebosch

build/CHANGELOG.md

@@ -1,3 +1,8 @@
+## 0.12.0+2
+
+- **Bug Fix** Correctly handle encoding `AssetId`s as `URI`s when they contain
+  characters which are not valid for a path segment.
+
 ## 0.12.0+1
 
 - Support the latest `analyzer` package.

build/lib/src/asset/id.dart

@@ -98,10 +98,7 @@ class AssetId implements Comparable<AssetId> {
   /// A `package:` URI suitable for use directly with other systems if this
   /// asset is under it's package's `lib/` directory, else an `asset:` URI
   /// suitable for use within build tools.
-  Uri get uri => _uri ??= path.startsWith('lib/')
-      ? new Uri(
-          scheme: 'package', path: '$package/${path.replaceFirst('lib/','')}')
-      : new Uri(scheme: 'asset', path: '$package/$path');
+  Uri get uri => _uri ??= _constructUri(this);
   Uri _uri;
 
   /// Deserializes an [AssetId] from [data], which must be the result of
@@ -154,3 +151,12 @@ String _normalizePath(String path) {
   // Collapse "." and "..".
   return p.posix.normalize(path);
 }
+
+Uri _constructUri(AssetId id) {
+  final originalSegments = p.split(id.path);
+  final isLib = originalSegments.first == 'lib';
+  final scheme = isLib ? 'package' : 'asset';
+  final pathSegmens = isLib ? originalSegments.skip(1) : originalSegments;
+  return new Uri(
+      scheme: scheme, pathSegments: [id.package]..addAll(pathSegmens));
+}

build/pubspec.yaml

@@ -1,5 +1,5 @@
 name: build
-version: 0.12.0+1
+version: 0.12.0+2
 description: A build system for Dart.
 author: Dart Team <[email protected]>
 homepage: https://github.com/dart-lang/build/tree/master/build

build/test/asset/id_test.dart

@@ -8,114 +8,81 @@ import 'package:build/build.dart';
 
 // Forked from `barback/test/asset_id_test.dart`.
 void main() {
-  group("constructor", () {
-    test("normalizes the path", () {
-      var id = new AssetId("app", r"path/././/to/drop/..//asset.txt");
-      expect(id.path, equals("path/to/asset.txt"));
+  group('constructor', () {
+    test('normalizes the path', () {
+      var id = new AssetId('app', r'path/././/to/drop/..//asset.txt');
+      expect(id.path, equals('path/to/asset.txt'));
     });
 
-    test("normalizes backslashes to slashes in the path", () {
-      var id = new AssetId("app", r"path\to/asset.txt");
-      expect(id.path, equals("path/to/asset.txt"));
+    test('normalizes backslashes to slashes in the path', () {
+      var id = new AssetId('app', r'path\to/asset.txt');
+      expect(id.path, equals('path/to/asset.txt'));
     });
   });
 
-  group("parse", () {
-    test("parses the package and path", () {
-      var id = new AssetId.parse("package|path/to/asset.txt");
-      expect(id.package, equals("package"));
-      expect(id.path, equals("path/to/asset.txt"));
+  group('parse', () {
+    test('parses the package and path', () {
+      var id = new AssetId.parse('package|path/to/asset.txt');
+      expect(id.package, equals('package'));
+      expect(id.path, equals('path/to/asset.txt'));
     });
 
-    test("throws if there are multiple '|'", () {
-      expect(() => new AssetId.parse("app|path|wtf"), throwsFormatException);
+    test('throws if there are multiple "|"', () {
+      expect(() => new AssetId.parse('app|path|wtf'), throwsFormatException);
     });
 
-    test("throws if the package name is empty '|'", () {
-      expect(() => new AssetId.parse("|asset.txt"), throwsFormatException);
+    test('throws if the package name is empty "|"', () {
+      expect(() => new AssetId.parse('|asset.txt'), throwsFormatException);
     });
 
-    test("throws if the path is empty '|'", () {
-      expect(() => new AssetId.parse("app|"), throwsFormatException);
+    test('throws if the path is empty "|"', () {
+      expect(() => new AssetId.parse('app|'), throwsFormatException);
     });
 
-    test("normalizes the path", () {
-      var id = new AssetId.parse(r"app|path/././/to/drop/..//asset.txt");
-      expect(id.path, equals("path/to/asset.txt"));
+    test('normalizes the path', () {
+      var id = new AssetId.parse(r'app|path/././/to/drop/..//asset.txt');
+      expect(id.path, equals('path/to/asset.txt'));
     });
 
-    test("normalizes backslashes to slashes in the path", () {
-      var id = new AssetId.parse(r"app|path\to/asset.txt");
-      expect(id.path, equals("path/to/asset.txt"));
+    test('normalizes backslashes to slashes in the path', () {
+      var id = new AssetId.parse(r'app|path\to/asset.txt');
+      expect(id.path, equals('path/to/asset.txt'));
     });
   });
 
-  group("resolve", () {
-    test("should parse a package: URI", () {
-      var id = new AssetId.resolve(r"package:app/app.dart");
-      expect(id, new AssetId("app", "lib/app.dart"));
+  group('to URI', () {
+    test('uses `package:` URIs inside lib/', () {
+      expect(new AssetId('foo', 'lib/bar.dart').uri,
+          Uri.parse('package:foo/bar.dart'));
     });
 
-    test("should parse a package: URI with a long path", () {
-      var id = new AssetId.resolve(r"package:app/src/some/path.dart");
-      expect(id, new AssetId("app", "lib/src/some/path.dart"));
+    test('uses `asset:` URIs outside lib/', () async {
+      expect(new AssetId('foo', 'web/main.dart').uri,
+          Uri.parse('asset:foo/web/main.dart'));
     });
 
-    test("should parse an asset: URI", () {
-      var id = new AssetId.resolve(r"asset:app/test/foo_test.dart");
-      expect(id, new AssetId("app", "test/foo_test.dart"));
-    });
-
-    test("should throw for a file: URI", () {
-      expect(() => new AssetId.resolve(r"file://localhost/etc/fstab1"),
-          throwsUnsupportedError);
-    });
-
-    test("should throw for a dart: URI", () {
-      expect(() => new AssetId.resolve(r"dart:collection"),
-          throwsUnsupportedError);
-    });
-
-    test("should throw parsing a relative package URI without an origin", () {
-      expect(() => new AssetId.resolve("some/relative/path.dart"),
-          throwsArgumentError);
-    });
-
-    test("should parse a relative URI within the test/ folder", () {
-      var id = new AssetId.resolve("common.dart",
-          from: new AssetId("app", "test/some_test.dart"));
-      expect(id, new AssetId("app", "test/common.dart"));
-    });
-
-    test("should parse a relative package URI", () {
-      var id = new AssetId.resolve("some/relative/path.dart",
-          from: new AssetId("app", "lib/app.dart"));
-      expect(id, new AssetId("app", "lib/some/relative/path.dart"));
-    });
-
-    test("should parse a relative package URI pointing back", () {
-      var id = new AssetId.resolve("../src/some/path.dart",
-          from: new AssetId("app", "folder/folder.dart"));
-      expect(id, new AssetId("app", "src/some/path.dart"));
+    test('handles characters that are valid in a file path', () {
+      expect(new AssetId('foo', 'lib/#bar.dart').uri,
+          Uri.parse('package:foo/%23bar.dart'));
     });
   });
 
-  test("equals another ID with the same package and path", () {
-    expect(new AssetId.parse("foo|asset.txt"),
-        equals(new AssetId.parse("foo|asset.txt")));
+  test('equals another ID with the same package and path', () {
+    expect(new AssetId.parse('foo|asset.txt'),
+        equals(new AssetId.parse('foo|asset.txt')));
 
-    expect(new AssetId.parse("foo|asset.txt"),
-        isNot(equals(new AssetId.parse("bar|asset.txt"))));
+    expect(new AssetId.parse('foo|asset.txt'),
+        isNot(equals(new AssetId.parse('bar|asset.txt'))));
 
-    expect(new AssetId.parse("foo|asset.txt"),
-        isNot(equals(new AssetId.parse("bar|other.txt"))));
+    expect(new AssetId.parse('foo|asset.txt'),
+        isNot(equals(new AssetId.parse('bar|other.txt'))));
   });
 
-  test("identical assets are treated as the same in a Map/Set", () {
+  test('identical assets are treated as the same in a Map/Set', () {
     var id1 = new AssetId('a', 'web/a.txt');
     var id2 = new AssetId('a', 'web/a.txt');
 
     expect({id1: true}.containsKey(id2), isTrue);
-    expect(new Set<AssetId>.from([id1]), contains(id2));
+    expect([id1].toSet(), contains(id2));
   });
 }