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

diff --git a/quantum/api/v2/attributes.py b/quantum/api/v2/attributes.py
index 8bf10a1f48b9ee63cd0dba4dfae3656de69cae08..3d3e8581c2e9057a22c9fd67357aecb7b7a151bf 100644 (file)
--- 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 31c5fd63dcc17078fdb0823391010d192ac5276b..e5620047933054a07bd025276be176b6b61a819c 100644 (file)
--- 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 437e454b3c41751ccb566ba2c888c6d6173fcab2..144e1a9a8d5453db7f63c8d750a6e2daf22c8c3c 100644 (file)
--- 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 7efa198fd644eafc46adc8524a6c3e23dacabbc2..bd770b21e8c3596215011dd18b98ea8dc35b1c96 100644 (file)
--- 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 a4e2d06b38d47dca4f6f498135fd98e6c7928578..911b3e697b4216c13c49ef4695bdc90a53428a26 100644 (file)
--- 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 dffe31ca4b57587a7a79337a5189e22d0032acd1..7c6bd6765ad5f98334bdbb2ba4d7fc83ba63ae37 100644 (file)
--- 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'