fix(Dependency) ignore extra `version` key with path and git deps (dart-lang/pubspec_parse#14)
Streamline the handling of all dependencies
Improve errors when unsupported keys are provided in dependencies
Better handling (and testing) for invalid `sdk:` values
Handle scp-like URIs for git dependencies
- See https://git-scm.com/docs/git-clone#_git_urls_a_id_urls_a
Fixes https://github.com/dart-lang/pubspec_parse/issues/13
Prepare for release
diff --git a/pkgs/pubspec_parse/CHANGELOG.md b/pkgs/pubspec_parse/CHANGELOG.md
index 720f0e7..30551e7 100644
--- a/pkgs/pubspec_parse/CHANGELOG.md
+++ b/pkgs/pubspec_parse/CHANGELOG.md
@@ -1,3 +1,11 @@
+## 0.1.2
+
+- Allow superfluous `version` keys with `git` and `path` dependencies.
+- Improve errors when unsupported keys are provided in dependencies.
+- Provide better errors with invalid `sdk` dependency values.
+- Support "scp-like syntax" for Git SSH URIs in the form
+ `[user@]host.xz:path/to/repo.git/`.
+
## 0.1.1
- Fixed name collision with error type in latest `package:json_annotation`.
diff --git a/pkgs/pubspec_parse/lib/src/dependency.dart b/pkgs/pubspec_parse/lib/src/dependency.dart
index c4f7b26..07d08b5 100644
--- a/pkgs/pubspec_parse/lib/src/dependency.dart
+++ b/pkgs/pubspec_parse/lib/src/dependency.dart
@@ -6,6 +6,8 @@
import 'package:pub_semver/pub_semver.dart';
import 'package:yaml/yaml.dart';
+import 'errors.dart';
+
part 'dependency.g.dart';
Map<String, Dependency> parseDeps(Map source) =>
@@ -40,49 +42,55 @@
}) ??
{};
+const _sourceKeys = const ['sdk', 'git', 'path', 'hosted'];
+
/// Returns `null` if the data could not be parsed.
Dependency _fromJson(dynamic data) {
- var value =
- SdkDependency.tryFromData(data) ?? HostedDependency.tryFromData(data);
-
- if (value != null) {
- return value;
+ if (data is String || data == null) {
+ return _$HostedDependencyFromJson({'version': data});
}
if (data is Map) {
- try {
- return _fromMap(data);
- } on ArgumentError catch (e) {
- throw new CheckedFromJsonException(
- data, e.name, 'Dependency', e.message.toString());
+ var matchedKeys =
+ data.keys.cast<String>().where((key) => key != 'version').toList();
+
+ if (data.isEmpty || (matchedKeys.isEmpty && data.containsKey('version'))) {
+ return _$HostedDependencyFromJson(data);
+ } else {
+ var weirdKey = matchedKeys.firstWhere((k) => !_sourceKeys.contains(k),
+ orElse: () => null);
+
+ if (weirdKey != null) {
+ throw new InvalidKeyException(
+ data, weirdKey, 'Unsupported dependency key.');
+ }
+ if (matchedKeys.length > 1) {
+ throw new CheckedFromJsonException(data, matchedKeys[1], 'Dependency',
+ 'A dependency may only have one source.');
+ }
+
+ var key = matchedKeys.single;
+
+ try {
+ switch (key) {
+ case 'git':
+ return new GitDependency.fromData(data[key]);
+ case 'path':
+ return new PathDependency.fromData(data[key]);
+ case 'sdk':
+ return _$SdkDependencyFromJson(data);
+ case 'hosted':
+ return _$HostedDependencyFromJson(data);
+ }
+ throw new StateError('There is a bug in pubspec_parse.');
+ } on ArgumentError catch (e) {
+ throw new CheckedFromJsonException(
+ data, e.name, 'Dependency', e.message.toString());
+ }
}
}
- return null;
-}
-
-Dependency _fromMap(Map data) {
- assert(data.entries.isNotEmpty);
- if (data.entries.length > 1) {
- throw new CheckedFromJsonException(data, data.keys.skip(1).first as String,
- 'Dependency', 'Expected only one key.');
- }
-
- var entry = data.entries.single;
- var key = entry.key as String;
-
- if (entry.value == null) {
- throw new CheckedFromJsonException(
- data, key, 'Dependency', 'Cannot be null.');
- }
-
- switch (key) {
- case 'path':
- return new PathDependency.fromData(entry.value);
- case 'git':
- return new GitDependency.fromData(entry.value);
- }
-
+ // Not a String or a Map – return null so parent logic can throw proper error
return null;
}
@@ -97,26 +105,20 @@
@JsonSerializable(createToJson: false)
class SdkDependency extends Dependency {
+ @JsonKey(nullable: false, disallowNullValue: true, required: true)
final String sdk;
@JsonKey(fromJson: _constraintFromString)
final VersionConstraint version;
SdkDependency(this.sdk, {this.version}) : super._();
- static SdkDependency tryFromData(Object data) {
- if (data is Map && data.containsKey('sdk')) {
- return _$SdkDependencyFromJson(data);
- }
- return null;
- }
-
@override
String get _info => sdk;
}
@JsonSerializable(createToJson: false)
class GitDependency extends Dependency {
- @JsonKey(fromJson: _parseUri, required: true, disallowNullValue: true)
+ @JsonKey(fromJson: parseGitUri, required: true, disallowNullValue: true)
final Uri url;
final String ref;
final String path;
@@ -139,7 +141,38 @@
String get _info => 'url@$url';
}
-Uri _parseUri(String value) => Uri.parse(value);
+Uri parseGitUri(String value) => _tryParseScpUri(value) ?? Uri.parse(value);
+
+/// Supports URIs like `[user@]host.xz:path/to/repo.git/`
+/// See https://git-scm.com/docs/git-clone#_git_urls_a_id_urls_a
+Uri _tryParseScpUri(String value) {
+ var colonIndex = value.indexOf(':');
+
+ if (colonIndex < 0) {
+ return null;
+ } else if (colonIndex == value.indexOf('://')) {
+ // If the first colon is part of a scheme, it's not an scp-like URI
+ return null;
+ }
+ var slashIndex = value.indexOf('/');
+
+ if (slashIndex >= 0 && slashIndex < colonIndex) {
+ // Per docs: This syntax is only recognized if there are no slashes before
+ // the first colon. This helps differentiate a local path that contains a
+ // colon. For example the local path foo:bar could be specified as an
+ // absolute path or ./foo:bar to avoid being misinterpreted as an ssh url.
+ return null;
+ }
+
+ var atIndex = value.indexOf('@');
+ if (colonIndex > atIndex) {
+ var user = atIndex >= 0 ? value.substring(0, atIndex) : null;
+ var host = value.substring(atIndex + 1, colonIndex);
+ var path = value.substring(colonIndex + 1);
+ return new Uri(scheme: 'ssh', userInfo: user, host: host, path: path);
+ }
+ return null;
+}
class PathDependency extends Dependency {
final String path;
@@ -169,22 +202,6 @@
: this.version = version ?? VersionConstraint.any,
super._();
- static HostedDependency tryFromData(Object data) {
- if (data == null || data is String) {
- data = {'version': data};
- }
-
- if (data is Map) {
- if (data.isEmpty ||
- data.containsKey('version') ||
- data.containsKey('hosted')) {
- return _$HostedDependencyFromJson(data);
- }
- }
-
- return null;
- }
-
@override
String get _info => version.toString();
}
@@ -194,7 +211,7 @@
@JsonKey(required: true, disallowNullValue: true)
final String name;
- @JsonKey(fromJson: _parseUri, disallowNullValue: true)
+ @JsonKey(fromJson: parseGitUri, disallowNullValue: true)
final Uri url;
HostedDetails(this.name, this.url);
diff --git a/pkgs/pubspec_parse/lib/src/dependency.g.dart b/pkgs/pubspec_parse/lib/src/dependency.g.dart
index ed384a9..e4bca58 100644
--- a/pkgs/pubspec_parse/lib/src/dependency.g.dart
+++ b/pkgs/pubspec_parse/lib/src/dependency.g.dart
@@ -12,6 +12,8 @@
SdkDependency _$SdkDependencyFromJson(Map json) {
return $checkedNew('SdkDependency', json, () {
+ $checkKeys(json,
+ requiredKeys: const ['sdk'], disallowNullValues: const ['sdk']);
var val = new SdkDependency(
$checkedConvert(json, 'sdk', (v) => v as String),
version: $checkedConvert(json, 'version',
@@ -26,7 +28,7 @@
requiredKeys: const ['url'], disallowNullValues: const ['url']);
var val = new GitDependency(
$checkedConvert(
- json, 'url', (v) => v == null ? null : _parseUri(v as String)),
+ json, 'url', (v) => v == null ? null : parseGitUri(v as String)),
$checkedConvert(json, 'ref', (v) => v as String),
$checkedConvert(json, 'path', (v) => v as String));
return val;
@@ -56,7 +58,7 @@
var val = new HostedDetails(
$checkedConvert(json, 'name', (v) => v as String),
$checkedConvert(
- json, 'url', (v) => v == null ? null : _parseUri(v as String)));
+ json, 'url', (v) => v == null ? null : parseGitUri(v as String)));
return val;
});
}
diff --git a/pkgs/pubspec_parse/pubspec.yaml b/pkgs/pubspec_parse/pubspec.yaml
index 78dba00..e219060 100644
--- a/pkgs/pubspec_parse/pubspec.yaml
+++ b/pkgs/pubspec_parse/pubspec.yaml
@@ -2,7 +2,7 @@
description: >-
Simple package for parsing pubspec.yaml files with a type-safe API and rich
error reporting.
-version: 0.1.2-dev
+version: 0.1.2
homepage: https://github.com/dart-lang/pubspec_parse
author: Dart Team <misc@dartlang.org>
diff --git a/pkgs/pubspec_parse/test/dependency_test.dart b/pkgs/pubspec_parse/test/dependency_test.dart
index 658e132..def243d 100644
--- a/pkgs/pubspec_parse/test/dependency_test.dart
+++ b/pkgs/pubspec_parse/test/dependency_test.dart
@@ -31,10 +31,17 @@
test('map with too many keys', () {
_expectThrows({'path': 'a', 'git': 'b'}, r'''
-line 5, column 12: Expected only one key.
+line 5, column 12: A dependency may only have one source.
"path": "a",
^^^''');
});
+
+ test('map with unsupported keys', () {
+ _expectThrows({'bob': 'a', 'jones': 'b'}, r'''
+line 6, column 4: Unsupported dependency key.
+ "jones": "b"
+ ^^^^^^^''');
+ });
});
}
@@ -94,15 +101,15 @@
^^^^^^^^^^^^^^^''');
});
- test('map w/ unsupported keys', () {
+ test('map w/ extra keys should fail', () {
_expectThrows({
'version': '^1.0.0',
'hosted': {'name': 'hosted_name', 'url': 'hosted_url'},
'not_supported': null
}, r'''
-line 4, column 10: Unrecognized keys: [not_supported]; supported keys: [version, hosted]
- "dep": {
- ^^''');
+line 10, column 4: Unsupported dependency key.
+ "not_supported": null
+ ^^^^^^^^^^^^^^^''');
});
test('map w/ version and hosted as String', () {
@@ -138,24 +145,38 @@
}
void _sdkDependency() {
- test('SdkDependency without version', () {
+ test('without version', () {
var dep = _dependency<SdkDependency>({'sdk': 'flutter'});
expect(dep.sdk, 'flutter');
expect(dep.version, isNull);
expect(dep.toString(), 'SdkDependency: flutter');
});
- test('SdkDependency with version', () {
+ test('with version', () {
var dep = _dependency<SdkDependency>(
{'sdk': 'flutter', 'version': '>=1.2.3 <2.0.0'});
expect(dep.sdk, 'flutter');
expect(dep.version.toString(), '>=1.2.3 <2.0.0');
expect(dep.toString(), 'SdkDependency: flutter');
});
+
+ test('null content', () {
+ _expectThrows({'sdk': null}, r'''
+line 5, column 11: These keys had `null` values, which is not allowed: [sdk]
+ "sdk": null
+ ^^^^^''');
+ });
+
+ test('number content', () {
+ _expectThrows({'sdk': 42}, r'''
+line 5, column 11: Unsupported value for `sdk`.
+ "sdk": 42
+ ^^^''');
+ });
}
void _gitDependency() {
- test('GitDependency - string', () {
+ test('string', () {
var dep = _dependency<GitDependency>({'git': 'url'});
expect(dep.url.toString(), 'url');
expect(dep.path, isNull);
@@ -163,7 +184,31 @@
expect(dep.toString(), 'GitDependency: url@url');
});
- test('GitDependency - map', () {
+ test('string with version key is ignored', () {
+ // Regression test for https://github.com/dart-lang/pubspec_parse/issues/13
+ var dep = _dependency<GitDependency>({'git': 'url', 'version': '^1.2.3'});
+ expect(dep.url.toString(), 'url');
+ expect(dep.path, isNull);
+ expect(dep.ref, isNull);
+ expect(dep.toString(), 'GitDependency: url@url');
+ });
+
+ test('string with user@ URL', () {
+ var dep = _dependency<GitDependency>({'git': 'git@localhost:dep.git'});
+ expect(dep.url.toString(), 'ssh://git@localhost/dep.git');
+ expect(dep.path, isNull);
+ expect(dep.ref, isNull);
+ expect(dep.toString(), 'GitDependency: url@ssh://git@localhost/dep.git');
+ });
+
+ test('string with random extra key fails', () {
+ _expectThrows({'git': 'url', 'bob': '^1.2.3'}, r'''
+line 6, column 4: Unsupported dependency key.
+ "bob": "^1.2.3"
+ ^^^^^''');
+ });
+
+ test('map', () {
var dep = _dependency<GitDependency>({
'git': {'url': 'url', 'path': 'path', 'ref': 'ref'}
});
@@ -175,7 +220,7 @@
test('git - null content', () {
_expectThrows({'git': null}, r'''
-line 5, column 11: Cannot be null.
+line 5, column 11: Must be a String or a Map.
"git": null
^^^^^''');
});
@@ -214,20 +259,34 @@
}
void _pathDependency() {
- test('PathDependency', () {
+ test('valid', () {
var dep = _dependency<PathDependency>({'path': '../path'});
expect(dep.path, '../path');
expect(dep.toString(), 'PathDependency: path@../path');
});
- test('path - null content', () {
+ test('valid with version key is ignored', () {
+ var dep =
+ _dependency<PathDependency>({'path': '../path', 'version': '^1.2.3'});
+ expect(dep.path, '../path');
+ expect(dep.toString(), 'PathDependency: path@../path');
+ });
+
+ test('valid with random extra key fails', () {
+ _expectThrows({'path': '../path', 'bob': '^1.2.3'}, r'''
+line 6, column 4: Unsupported dependency key.
+ "bob": "^1.2.3"
+ ^^^^^''');
+ });
+
+ test('null content', () {
_expectThrows({'path': null}, r'''
-line 5, column 12: Cannot be null.
+line 5, column 12: Must be a String.
"path": null
^^^^^''');
});
- test('path - int content', () {
+ test('int content', () {
_expectThrows({'path': 42}, r'''
line 5, column 12: Must be a String.
"path": 42
diff --git a/pkgs/pubspec_parse/test/git_uri_test.dart b/pkgs/pubspec_parse/test/git_uri_test.dart
new file mode 100644
index 0000000..a1cfdf9
--- /dev/null
+++ b/pkgs/pubspec_parse/test/git_uri_test.dart
@@ -0,0 +1,25 @@
+import 'package:test/test.dart';
+
+import 'package:pubspec_parse/src/dependency.dart';
+
+void main() {
+ for (var item in {
+ 'git@github.com:google/grinder.dart.git':
+ 'ssh://git@github.com/google/grinder.dart.git',
+ 'host.xz:path/to/repo.git/': 'ssh://host.xz/path/to/repo.git/',
+ 'http:path/to/repo.git/': 'ssh://http/path/to/repo.git/',
+ 'file:path/to/repo.git/': 'ssh://file/path/to/repo.git/',
+ './foo:bar': 'foo%3Abar',
+ '/path/to/repo.git/': '/path/to/repo.git/',
+ 'file:///path/to/repo.git/': 'file:///path/to/repo.git/',
+ }.entries) {
+ test(item.key, () {
+ var uri = parseGitUri(item.key);
+
+ printOnFailure(
+ [uri.scheme, uri.userInfo, uri.host, uri.port, uri.path].join('\n'));
+
+ expect(uri, Uri.parse(item.value));
+ });
+ }
+}
