Show better error message for invalid JSON

Especially for list params, their error messages were not helpful.
Error messages now include the actual value itself, as we have seen
cases where customers specify valid JSON but the shell they're using
will process arguments and strip out characters such as quotes.
By showing the actual JSON contents it will make this more obvious.

Before:

  The value for parameter "--" must be JSON or path to file.

After:

  Error parsing parameter --block-device-mappings: Invalid JSON:
  [{ ...BAD JSON CONTENTS ...}]

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

@@ -48,9 +48,9 @@ def test_nonexistent_priv_launch_key(self):
         result = {}
         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):
         key_path = os.path.join(os.path.dirname(__file__),

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()