From fa1a9ca64dff24e554d91fc475d0290a7f49d9e1 Mon Sep 17 00:00:00 2001
From: "rnystrom@google.com" <rnystrom@google.com>
Date: Thu, 22 Aug 2013 18:05:52 +0000
Subject: [PATCH] Handle invalid version syntax in pubspecs.

BUG=https://code.google.com/p/dart/issues/detail?id=11046
R=nweiz@google.com

Review URL: https://codereview.chromium.org//23295025

git-svn-id: https://dart.googlecode.com/svn/branches/bleeding_edge@26541 260f80e4-7a28-3924-810f-c04153c831b5
---
 lib/src/pubspec.dart   | 52 +++++++++++++++++++++++++++++++-----------
 test/pubspec_test.dart | 37 ++++++++++++++++++++++++++++++
 2 files changed, 76 insertions(+), 13 deletions(-)

diff --git a/lib/src/pubspec.dart b/lib/src/pubspec.dart
index 6da96ba6..df7ebd93 100644
--- a/lib/src/pubspec.dart
+++ b/lib/src/pubspec.dart
@@ -121,7 +121,6 @@ void _validateFieldUrl(url, String field) {
 
 Pubspec _parseMap(String filePath, Map map, SourceRegistry sources) {
   var name = null;
-  var version = Version.none;
 
   if (map.containsKey('name')) {
     name = map['name'];
@@ -131,9 +130,9 @@ Pubspec _parseMap(String filePath, Map map, SourceRegistry sources) {
     }
   }
 
-  if (map.containsKey('version')) {
-    version = new Version.parse(map['version']);
-  }
+  var version = _parseVersion(map['version'], (v) =>
+      'The pubspec "version" field should be a semantic version number, '
+      'but was "$v".');
 
   var dependencies = _parseDependencies(filePath, sources,
       map['dependencies']);
@@ -183,14 +182,9 @@ Pubspec _parseMap(String filePath, Map map, SourceRegistry sources) {
           '"$environmentYaml".');
     }
 
-    var sdkYaml = environmentYaml['sdk'];
-    if (sdkYaml is! String) {
-      throw new FormatException(
-          'The "sdk" field of "environment" should be a string, but was '
-          '"$sdkYaml".');
-    }
-
-    sdkConstraint = new VersionConstraint.parse(sdkYaml);
+    sdkConstraint = _parseVersionConstraint(environmentYaml['sdk'], (v) =>
+        'The "sdk" field of "environment" should be a semantic version '
+        'constraint, but was "$v".');
   }
   var environment = new PubspecEnvironment(sdkConstraint);
 
@@ -241,6 +235,36 @@ Pubspec _parseMap(String filePath, Map map, SourceRegistry sources) {
       environment, map);
 }
 
+/// Parses [yaml] to a [Version] or throws a [FormatException] with the result
+/// of calling [message] if it isn't valid.
+///
+/// If [yaml] is `null`, returns [Version.none].
+Version _parseVersion(yaml, String message(yaml)) {
+  if (yaml == null) return Version.none;
+  if (yaml is! String) throw new FormatException(message(yaml));
+
+  try {
+    return new Version.parse(yaml);
+  } on FormatException catch(_) {
+    throw new FormatException(message(yaml));
+  }
+}
+
+/// Parses [yaml] to a [VersionConstraint] or throws a [FormatException] with
+/// the result of calling [message] if it isn't valid.
+///
+/// If [yaml] is `null`, returns [VersionConstraint.any].
+VersionConstraint _parseVersionConstraint(yaml, String getMessage(yaml)) {
+  if (yaml == null) return VersionConstraint.any;
+  if (yaml is! String) throw new FormatException(getMessage(yaml));
+
+  try {
+    return new VersionConstraint.parse(yaml);
+  } on FormatException catch(_) {
+    throw new FormatException(getMessage(yaml));
+  }
+}
+
 List<PackageDep> _parseDependencies(String pubspecPath, SourceRegistry sources,
     yaml) {
   var dependencies = <PackageDep>[];
@@ -268,7 +292,9 @@ List<PackageDep> _parseDependencies(String pubspecPath, SourceRegistry sources,
       versionConstraint = new VersionConstraint.parse(spec);
     } else if (spec is Map) {
       if (spec.containsKey('version')) {
-        versionConstraint = new VersionConstraint.parse(spec.remove('version'));
+        versionConstraint = _parseVersionConstraint(spec.remove('version'),
+            (v) => 'The "version" field for $name should be a semantic '
+                   'version constraint, but was "$v".');
       }
 
       var sourceNames = spec.keys.toList();
diff --git a/test/pubspec_test.dart b/test/pubspec_test.dart
index 47f838a0..e7608b4c 100644
--- a/test/pubspec_test.dart
+++ b/test/pubspec_test.dart
@@ -111,10 +111,40 @@ dependencies:
 ''');
     });
 
+    test("throws if dependency version is not a string", () {
+      expectFormatError('''
+dependencies:
+  foo:
+    mock: ok
+    version: 1.2
+''');
+    });
+
+    test("throws if version is not a version constraint", () {
+      expectFormatError('''
+dependencies:
+  foo:
+    mock: ok
+    version: not constraint
+''');
+    });
+
     test("throws if 'name' is not a string", () {
       expectFormatError('name: [not, a, string]');
     });
 
+    test("throws if version is not a string", () {
+      expectFormatError('''
+version: 1.0
+''');
+    });
+
+    test("throws if version is not a version", () {
+      expectFormatError('''
+version: not version
+''');
+    });
+
     test("throws if 'homepage' is not a string", () {
       expectFormatError('homepage:');
       expectFormatError('homepage: [not, a, string]');
@@ -212,6 +242,13 @@ environment:
 ''');
       });
 
+    test("throws if the sdk is not a string", () {
+      expectFormatError('''
+environment:
+  sdk: 1.0
+''');
+    });
+
       test("throws if the sdk isn't a valid version constraint", () {
         expectFormatError('''
 environment:
-- 
GitLab