Show better error message for invalid JSON

awscli/argprocess.py

@@ -26,10 +26,11 @@
 
 class ParamError(Exception):
     def __init__(self, param, message):
-        full_message = ("Error parsing parameter %s, should be: %s" %
+        full_message = ("Error parsing parameter %s: %s" %
                         (param.cli_name, message))
         super(ParamError, self).__init__(full_message)
         self.param = param
+        self.message = message
 
 
 class ParamSyntaxError(Exception):
@@ -129,7 +130,7 @@ def __call__(self, param, value, **kwargs):
                 if doc_fn is None:
                     raise e
                 else:
-                    raise ParamError(param, doc_fn(param))
+                    raise ParamError(param, "should be: %s" % doc_fn(param))
             return parsed
 
     def get_parse_method_for_param(self, param, value=None):
@@ -353,12 +354,13 @@ def unpack_cli_arg(parameter, value):
 def unpack_complex_cli_arg(parameter, value):
     if parameter.type == 'structure' or parameter.type == 'map':
         if value.lstrip()[0] == '{':
-            d = json.loads(value, object_pairs_hook=OrderedDict)
-        else:
-            msg = 'The value for parameter "%s" must be JSON or path to file.' % (
-                parameter.cli_name)
-            raise ValueError(msg)
-        return d
+            try:
+                return json.loads(value, object_pairs_hook=OrderedDict)
+            except ValueError as e:
+                raise ParamError(
+                    parameter, "Invalid JSON: %s\nJSON received: %s"
+                    % (e, value))
+        raise ParamError(parameter, "Invalid JSON:\n%s" % value)
     elif parameter.type == 'list':
         if isinstance(value, six.string_types):
             if value.lstrip()[0] == '[':
@@ -367,7 +369,14 @@ def unpack_complex_cli_arg(parameter, value):
             single_value = value[0].strip()
             if single_value and single_value[0] == '[':
                 return json.loads(value[0], object_pairs_hook=OrderedDict)
-        return [unpack_cli_arg(parameter.members, v) for v in value]
+        try:
+            return [unpack_cli_arg(parameter.members, v) for v in value]
+        except ParamError as e:
+            # The list params don't have a name/cli_name attached to them
+            # so they will have bad error messages.  We're going to
+            # attach the parent parmeter to this error message to provide
+            # a more helpful error message.
+            raise ParamError(parameter, e.message)
 
 
 def unpack_scalar_cli_arg(parameter, value):
@@ -381,7 +390,7 @@ def unpack_scalar_cli_arg(parameter, value):
         file_path = os.path.expanduser(file_path)
         if not os.path.isfile(file_path):
             msg = 'Blob values must be a path to a file.'
-            raise ValueError(msg)
+            raise ParamError(parameter, msg)
         return open(file_path, 'rb')
     elif parameter.type == 'boolean':
         if isinstance(value, str) and value.lower() == 'false':

awscli/clidriver.py

@@ -202,6 +202,7 @@ def main(self, args=None):
         except Exception as e:
             LOG.debug("Exception caught in main()", exc_info=True)
             LOG.debug("Exiting with rc 255")
+            sys.stderr.write("\n")
             sys.stderr.write("%s\n" % e)
             return 255
 

tests/unit/ec2/test_get_password_data.py

@@ -13,10 +13,6 @@
 # language governing permissions and limitations under the License.
 from tests.unit import BaseAWSCommandParamsTest
 import os
-import re
-
-from six.moves import cStringIO
-import mock
 
 
 PASSWORD_DATA = ("GWDnuoj/7pbMQkg125E8oGMUVCI+r98sGbFFl8SX+dEYxMZzz+byYwwjvyg8i"
@@ -38,7 +34,6 @@ def setUp(self):
                                 'PasswordData': PASSWORD_DATA}
 
     def test_no_priv_launch_key(self):
-        captured = cStringIO()
         args = ' --instance-id i-12345678'
         cmdline = self.prefix + args
         result = {'InstanceId': 'i-12345678'}
@@ -51,15 +46,13 @@ def test_nonexistent_priv_launch_key(self):
         args = ' --instance-id i-12345678 --priv-launch-key foo.pem'
         cmdline = self.prefix + args
         result = {}
-        captured = cStringIO()
         error_msg = self.assert_params_for_cmd(
             cmdline, result, expected_rc=255)[1]
-        self.assertEqual(error_msg, ('priv-launch-key should be a path to '
-                                     'the local SSH private key file used '
-                                     'to launch the instance.\n'))
+        self.assertIn('priv-launch-key should be a path to '
+                      'the local SSH private key file used '
+                      'to launch the instance.\n', error_msg)
 
     def test_priv_launch_key(self):
-        captured = cStringIO()
         key_path = os.path.join(os.path.dirname(__file__),
                                 'testcli.pem')
         args = ' --instance-id i-12345678 --priv-launch-key %s' % key_path

tests/unit/test_argprocess.py

@@ -335,5 +335,35 @@ def test_gen_list_structure_multiple_scalar_docs(self):
         self.assertEqual(doc_string, s)
 
 
+class TestUnpackJSONParams(BaseArgProcessTest):
+    def setUp(self):
+        super(TestUnpackJSONParams, self).setUp()
+        self.simplify = ParamShorthand()
+
+    def test_json_with_spaces(self):
+        p = self.get_param_object('ec2.RunInstances.BlockDeviceMappings')
+        # If a user specifies the json with spaces, it will show up as
+        # a multi element list.  For example:
+        # --block-device-mappings [{ "DeviceName":"/dev/sdf",
+        # "VirtualName":"ephemeral0"}, {"DeviceName":"/dev/sdg",
+        # "VirtualName":"ephemeral1" }]
+        #
+        # Will show up as:
+        block_device_mapping = [
+            '[{', 'DeviceName:/dev/sdf,', 'VirtualName:ephemeral0},',
+            '{DeviceName:/dev/sdg,', 'VirtualName:ephemeral1', '}]']
+        # The shell has removed the double quotes so this is invalid
+        # JSON, but we should still raise a better exception.
+        with self.assertRaises(ParamError) as e:
+            unpack_cli_arg(p, block_device_mapping)
+        # Parameter name should be in error message.
+        self.assertIn('--block-device-mappings', str(e.exception))
+        # The actual JSON itself should be in the error message.
+        # Becaues this is a list, only the first element in the JSON
+        # will show.  This will at least let customers know what
+        # we tried to parse.
+        self.assertIn('[{', str(e.exception))
+
+
 if __name__ == '__main__':
     unittest.main()