From d46a89c04c48292e8d47218e27ecf79f0ad7d29a Mon Sep 17 00:00:00 2001 From: James Saryerwinnie Date: Fri, 7 Feb 2014 12:29:34 -0800 Subject: [PATCH 1/2] Cleanup unused imports in test module --- tests/unit/ec2/test_get_password_data.py | 7 ------- 1 file changed, 7 deletions(-) diff --git a/tests/unit/ec2/test_get_password_data.py b/tests/unit/ec2/test_get_password_data.py index fea608176810..4a4b2982fc9b 100644 --- a/tests/unit/ec2/test_get_password_data.py +++ b/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,7 +46,6 @@ 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 ' @@ -59,7 +53,6 @@ def test_nonexistent_priv_launch_key(self): 'to launch the instance.\n')) 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 From 2a95d9bdefbefeeee4349821cdcb78f6136dbedc Mon Sep 17 00:00:00 2001 From: James Saryerwinnie Date: Fri, 7 Feb 2014 12:26:21 -0800 Subject: [PATCH 2/2] 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 | 29 +++++++++++++++-------- awscli/clidriver.py | 1 + tests/unit/ec2/test_get_password_data.py | 6 ++--- tests/unit/test_argprocess.py | 30 ++++++++++++++++++++++++ 4 files changed, 53 insertions(+), 13 deletions(-) diff --git a/awscli/argprocess.py b/awscli/argprocess.py index da3b623fb6a1..432a085ab659 100644 --- a/awscli/argprocess.py +++ b/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': diff --git a/awscli/clidriver.py b/awscli/clidriver.py index 1545beb29c5d..52ab7de4429a 100644 --- a/awscli/clidriver.py +++ b/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 diff --git a/tests/unit/ec2/test_get_password_data.py b/tests/unit/ec2/test_get_password_data.py index 4a4b2982fc9b..67c03efdc917 100644 --- a/tests/unit/ec2/test_get_password_data.py +++ b/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__), diff --git a/tests/unit/test_argprocess.py b/tests/unit/test_argprocess.py index 04a67215464b..fa1245d105f4 100644 --- a/tests/unit/test_argprocess.py +++ b/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()