From be77455856fcc88300b0ad5a36211ebd37f0e453 Mon Sep 17 00:00:00 2001 From: Henry Gessau Date: Thu, 7 Feb 2013 16:01:28 -0500 Subject: [PATCH] Improvements to API validation logic. Do not automatically map generic exceptions like AttributeError to http errors (instead they should be handled closer to where they occur so that they can be "intelligently" converted to the appropriate error). Fix up some expected error codes in the unit tests. Improve some of the validation messages. Remove all use of locals() in attributes.py Fixes: bug #1076813 Change-Id: Iabf8808a840e927307bbcae4cd41790af3d79a9e --- quantum/api/v2/attributes.py | 39 ++++++++++++------- quantum/api/v2/base.py | 8 ++-- quantum/api/v2/resource.py | 3 +- .../tests/unit/cisco/test_network_plugin.py | 12 +++--- quantum/tests/unit/test_attributes.py | 32 +++++++++++---- quantum/tests/unit/test_db_plugin.py | 14 +++---- 6 files changed, 68 insertions(+), 40 deletions(-) diff --git a/quantum/api/v2/attributes.py b/quantum/api/v2/attributes.py index 8bf10a1f4..3d3e8581c 100644 --- a/quantum/api/v2/attributes.py +++ b/quantum/api/v2/attributes.py @@ -33,13 +33,16 @@ SHARED = 'shared' def _verify_dict_keys(expected_keys, target_dict): if not isinstance(target_dict, dict): - msg = _("Invalid input. %s must be a dictionary.") % target_dict + msg = (_("Invalid input. '%(target_dict)s' must be a dictionary " + "with keys: %(expected_keys)s") % + dict(target_dict=target_dict, expected_keys=expected_keys)) return msg provided_keys = target_dict.keys() if set(expected_keys) != set(provided_keys): - msg = (_("Expected keys not found. Expected: %(expected_keys)s " - "Provided: %(provided_keys)s") % locals()) + msg = (_("Expected keys not found. Expected: '%(expected_keys)s', " + "Provided: '%(provided_keys)s'") % + dict(expected_keys=expected_keys, provided_keys=provided_keys)) return msg @@ -49,7 +52,8 @@ def is_attr_set(attribute): def _validate_values(data, valid_values=None): if data not in valid_values: - msg = _("'%(data)s' is not in %(valid_values)s") % locals() + msg = (_("'%(data)s' is not in %(valid_values)s") % + dict(data=data, valid_values=valid_values)) LOG.debug(msg) return msg @@ -61,7 +65,8 @@ def _validate_string(data, max_len=None): return msg if max_len is not None and len(data) > max_len: - msg = _("'%(data)s' exceeds maximum length of %(max_len)s") % locals() + msg = (_("'%(data)s' exceeds maximum length of %(max_len)s") % + dict(data=data, max_len=max_len)) LOG.debug(msg) return msg @@ -71,7 +76,9 @@ def _validate_range(data, valid_values=None): max_value = valid_values[1] if not min_value <= data <= max_value: msg = _("'%(data)s' is not in range %(min_value)s through " - "%(max_value)s") % locals() + "%(max_value)s") % dict(data=data, + min_value=min_value, + max_value=max_value) LOG.debug(msg) return msg @@ -101,7 +108,7 @@ def _validate_ip_pools(data, valid_values=None): """ if not isinstance(data, list): - msg = _("'%s' is not a valid IP pool") % data + msg = _("Invalid data format for IP pool: '%s'") % data LOG.debug(msg) return msg @@ -120,18 +127,22 @@ def _validate_ip_pools(data, valid_values=None): def _validate_fixed_ips(data, valid_values=None): if not isinstance(data, list): - msg = _("'%s' is not a valid fixed IP") % data + msg = _("Invalid data format for fixed IP: '%s'") % data LOG.debug(msg) return msg ips = [] for fixed_ip in data: + if not isinstance(fixed_ip, dict): + msg = _("Invalid data format for fixed IP: '%s'") % fixed_ip + LOG.debug(msg) + return msg if 'ip_address' in fixed_ip: # Ensure that duplicate entries are not set - just checking IP # suffices. Duplicate subnet_id's are legitimate. fixed_ip_address = fixed_ip['ip_address'] if fixed_ip_address in ips: - msg = _("Duplicate entry %s") % fixed_ip + msg = _("Duplicate IP address '%s'") % fixed_ip_address else: msg = _validate_ip_address(fixed_ip_address) if msg: @@ -147,7 +158,7 @@ def _validate_fixed_ips(data, valid_values=None): def _validate_nameservers(data, valid_values=None): if not hasattr(data, '__iter__'): - msg = _("'%s' is not a valid nameserver") % data + msg = _("Invalid data format for nameserver: '%s'") % data LOG.debug(msg) return msg @@ -162,7 +173,7 @@ def _validate_nameservers(data, valid_values=None): LOG.debug(msg) return msg if ip in ips: - msg = _("Duplicate nameserver %s") % ip + msg = _("Duplicate nameserver '%s'") % ip LOG.debug(msg) return msg ips.append(ip) @@ -170,7 +181,7 @@ def _validate_nameservers(data, valid_values=None): def _validate_hostroutes(data, valid_values=None): if not isinstance(data, list): - msg = _("'%s' is not a valid hostroute") % data + msg = _("Invalid data format for hostroute: '%s'") % data LOG.debug(msg) return msg @@ -190,7 +201,7 @@ def _validate_hostroutes(data, valid_values=None): LOG.debug(msg) return msg if hostroute in hostroutes: - msg = _("Duplicate hostroute %s") % hostroute + msg = _("Duplicate hostroute '%s'") % hostroute LOG.debug(msg) return msg hostroutes.append(hostroute) @@ -252,7 +263,7 @@ def _validate_uuid_list(data, valid_values=None): return msg if len(set(data)) != len(data): - msg = _("Duplicate items in the list: %s") % ', '.join(data) + msg = _("Duplicate items in the list: '%s'") % ', '.join(data) LOG.debug(msg) return msg diff --git a/quantum/api/v2/base.py b/quantum/api/v2/base.py index 31c5fd63d..e56200479 100644 --- a/quantum/api/v2/base.py +++ b/quantum/api/v2/base.py @@ -36,8 +36,6 @@ FAULT_MAP = {exceptions.NotFound: webob.exc.HTTPNotFound, exceptions.ServiceUnavailable: webob.exc.HTTPServiceUnavailable, exceptions.NotAuthorized: webob.exc.HTTPForbidden, netaddr.AddrFormatError: webob.exc.HTTPBadRequest, - AttributeError: webob.exc.HTTPBadRequest, - ValueError: webob.exc.HTTPBadRequest, } @@ -359,7 +357,11 @@ class Controller(object): def update(self, request, id, body=None, **kwargs): """Updates the specified entity's attributes""" parent_id = kwargs.get(self._parent_id_name) - payload = body.copy() + try: + payload = body.copy() + except AttributeError: + msg = _("Invalid format: %s") % request.body + raise exceptions.BadRequest(resource='body', msg=msg) payload['id'] = id notifier_api.notify(request.context, self._publisher_id, diff --git a/quantum/api/v2/resource.py b/quantum/api/v2/resource.py index 437e454b3..144e1a9a8 100644 --- a/quantum/api/v2/resource.py +++ b/quantum/api/v2/resource.py @@ -80,8 +80,7 @@ def Resource(controller, faults=None, deserializers=None, serializers=None): method = getattr(controller, action) result = method(request=request, **args) - except (ValueError, AttributeError, - exceptions.QuantumException, + except (exceptions.QuantumException, netaddr.AddrFormatError) as e: LOG.exception(_('%s failed'), action) body = serializer.serialize({'QuantumError': str(e)}) diff --git a/quantum/tests/unit/cisco/test_network_plugin.py b/quantum/tests/unit/cisco/test_network_plugin.py index 7efa198fd..bd770b21e 100644 --- a/quantum/tests/unit/cisco/test_network_plugin.py +++ b/quantum/tests/unit/cisco/test_network_plugin.py @@ -92,7 +92,7 @@ class TestCiscoPortsV2(CiscoNetworkPluginV2TestCase, 'test', True) # We expect a 500 as we injected a fault in the plugin - self._validate_behavior_on_bulk_failure(res, 'ports') + self._validate_behavior_on_bulk_failure(res, 'ports', 500) def test_create_ports_bulk_native_plugin_failure(self): if self._skip_native_bulk: @@ -112,7 +112,7 @@ class TestCiscoPortsV2(CiscoNetworkPluginV2TestCase, res = self._create_port_bulk(self.fmt, 2, net['network']['id'], 'test', True, context=ctx) # We expect a 500 as we injected a fault in the plugin - self._validate_behavior_on_bulk_failure(res, 'ports') + self._validate_behavior_on_bulk_failure(res, 'ports', 500) class TestCiscoNetworksV2(CiscoNetworkPluginV2TestCase, @@ -140,7 +140,7 @@ class TestCiscoNetworksV2(CiscoNetworkPluginV2TestCase, res = self._create_network_bulk(self.fmt, 2, 'test', True) LOG.debug("response is %s" % res) # We expect a 500 as we injected a fault in the plugin - self._validate_behavior_on_bulk_failure(res, 'networks') + self._validate_behavior_on_bulk_failure(res, 'networks', 500) def test_create_networks_bulk_native_plugin_failure(self): if self._skip_native_bulk: @@ -157,7 +157,7 @@ class TestCiscoNetworksV2(CiscoNetworkPluginV2TestCase, patched_plugin.side_effect = side_effect res = self._create_network_bulk(self.fmt, 2, 'test', True) # We expect a 500 as we injected a fault in the plugin - self._validate_behavior_on_bulk_failure(res, 'networks') + self._validate_behavior_on_bulk_failure(res, 'networks', 500) class TestCiscoSubnetsV2(CiscoNetworkPluginV2TestCase, @@ -189,7 +189,7 @@ class TestCiscoSubnetsV2(CiscoNetworkPluginV2TestCase, net['network']['id'], 'test') # We expect a 500 as we injected a fault in the plugin - self._validate_behavior_on_bulk_failure(res, 'subnets') + self._validate_behavior_on_bulk_failure(res, 'subnets', 500) def test_create_subnets_bulk_native_plugin_failure(self): if self._skip_native_bulk: @@ -209,7 +209,7 @@ class TestCiscoSubnetsV2(CiscoNetworkPluginV2TestCase, 'test') # We expect a 500 as we injected a fault in the plugin - self._validate_behavior_on_bulk_failure(res, 'subnets') + self._validate_behavior_on_bulk_failure(res, 'subnets', 500) class TestCiscoPortsV2XML(TestCiscoPortsV2): diff --git a/quantum/tests/unit/test_attributes.py b/quantum/tests/unit/test_attributes.py index a4e2d06b3..911b3e697 100644 --- a/quantum/tests/unit/test_attributes.py +++ b/quantum/tests/unit/test_attributes.py @@ -139,17 +139,32 @@ class TestAttributes(unittest2.TestCase): self.assertIsNone(msg) def test_validate_fixed_ips(self): - fixed_ips = [[{'subnet_id': '00000000-ffff-ffff-ffff-000000000000', + fixed_ips = [ + {'data': [{'subnet_id': '00000000-ffff-ffff-ffff-000000000000', 'ip_address': '1111.1.1.1'}], - [{'subnet_id': 'invalid'}], - None, - [{'subnet_id': '00000000-0fff-ffff-ffff-000000000000', + 'error_msg': "'1111.1.1.1' is not a valid IP address"}, + {'data': [{'subnet_id': 'invalid', + 'ip_address': '1.1.1.1'}], + 'error_msg': "'invalid' is not a valid UUID"}, + {'data': None, + 'error_msg': "Invalid data format for fixed IP: 'None'"}, + {'data': "1.1.1.1", + 'error_msg': "Invalid data format for fixed IP: '1.1.1.1'"}, + {'data': ['00000000-ffff-ffff-ffff-000000000000', '1.1.1.1'], + 'error_msg': "Invalid data format for fixed IP: " + "'00000000-ffff-ffff-ffff-000000000000'"}, + {'data': [['00000000-ffff-ffff-ffff-000000000000', '1.1.1.1']], + 'error_msg': "Invalid data format for fixed IP: " + "'['00000000-ffff-ffff-ffff-000000000000', " + "'1.1.1.1']'"}, + {'data': [{'subnet_id': '00000000-0fff-ffff-ffff-000000000000', 'ip_address': '1.1.1.1'}, {'subnet_id': '00000000-ffff-ffff-ffff-000000000000', - 'ip_address': '1.1.1.1'}]] + 'ip_address': '1.1.1.1'}], + 'error_msg': "Duplicate IP address '1.1.1.1'"}] for fixed in fixed_ips: - msg = attributes._validate_fixed_ips(fixed) - self.assertIsNotNone(msg) + msg = attributes._validate_fixed_ips(fixed['data']) + self.assertEqual(msg, fixed['error_msg']) fixed_ips = [[{'subnet_id': '00000000-ffff-ffff-ffff-000000000000', 'ip_address': '1.1.1.1'}], @@ -402,7 +417,8 @@ class TestAttributes(unittest2.TestCase): 'f3eeab00-8367-4524-b662-55e64d4cacb5', 'e5069610-744b-42a7-8bd8-ceac1a229cd4'] msg = attributes._validate_uuid_list(duplicate_uuids) - error = "Duplicate items in the list: %s" % ', '.join(duplicate_uuids) + error = ("Duplicate items in the list: " + "'%s'" % ', '.join(duplicate_uuids)) self.assertEquals(msg, error) # check valid uuid lists diff --git a/quantum/tests/unit/test_db_plugin.py b/quantum/tests/unit/test_db_plugin.py index dffe31ca4..7c6bd6765 100644 --- a/quantum/tests/unit/test_db_plugin.py +++ b/quantum/tests/unit/test_db_plugin.py @@ -411,7 +411,7 @@ class QuantumDbPluginV2TestCase(testlib_api.WebTestCase): def _do_side_effect(self, patched_plugin, orig, *args, **kwargs): """ Invoked by test cases for injecting failures in plugin """ def second_call(*args, **kwargs): - raise AttributeError + raise q_exc.QuantumException patched_plugin.side_effect = second_call return orig(*args, **kwargs) @@ -740,7 +740,7 @@ class TestPortsV2(QuantumDbPluginV2TestCase): 'test', True) # We expect a 500 as we injected a fault in the plugin - self._validate_behavior_on_bulk_failure(res, 'ports') + self._validate_behavior_on_bulk_failure(res, 'ports', 500) def test_create_ports_bulk_native_plugin_failure(self): if self._skip_native_bulk: @@ -759,7 +759,7 @@ class TestPortsV2(QuantumDbPluginV2TestCase): res = self._create_port_bulk(self.fmt, 2, net['network']['id'], 'test', True, context=ctx) # We expect a 500 as we injected a fault in the plugin - self._validate_behavior_on_bulk_failure(res, 'ports') + self._validate_behavior_on_bulk_failure(res, 'ports', 500) def test_list_ports(self): # for this test we need to enable overlapping ips @@ -1731,7 +1731,7 @@ class TestNetworksV2(QuantumDbPluginV2TestCase): patched_plugin.side_effect = side_effect res = self._create_network_bulk(self.fmt, 2, 'test', True) # We expect a 500 as we injected a fault in the plugin - self._validate_behavior_on_bulk_failure(res, 'networks') + self._validate_behavior_on_bulk_failure(res, 'networks', 500) def test_create_networks_bulk_native_plugin_failure(self): if self._skip_native_bulk: @@ -1747,7 +1747,7 @@ class TestNetworksV2(QuantumDbPluginV2TestCase): patched_plugin.side_effect = side_effect res = self._create_network_bulk(self.fmt, 2, 'test', True) # We expect a 500 as we injected a fault in the plugin - self._validate_behavior_on_bulk_failure(res, 'networks') + self._validate_behavior_on_bulk_failure(res, 'networks', 500) def test_list_networks(self): with contextlib.nested(self.network(), @@ -1976,7 +1976,7 @@ class TestSubnetsV2(QuantumDbPluginV2TestCase): net['network']['id'], 'test') # We expect a 500 as we injected a fault in the plugin - self._validate_behavior_on_bulk_failure(res, 'subnets') + self._validate_behavior_on_bulk_failure(res, 'subnets', 500) def test_create_subnets_bulk_native_plugin_failure(self): if self._skip_native_bulk: @@ -1995,7 +1995,7 @@ class TestSubnetsV2(QuantumDbPluginV2TestCase): 'test') # We expect a 500 as we injected a fault in the plugin - self._validate_behavior_on_bulk_failure(res, 'subnets') + self._validate_behavior_on_bulk_failure(res, 'subnets', 500) def test_delete_subnet(self): gateway_ip = '10.0.0.1' -- 2.45.2