Fix handling of serialized data in filtering of volumes

Commit 4aaf40ba1aab4d7c347b05750d0fe21f8d1bcc68 has introduced a bug.
'ast.literal_eval(v)' throws exception 'SyntaxError' if 'v' is an expression
such as UUID '920da701-93c1-4178-9f1a-ef1c7a8a384d', 'd-', or 'd+'.
Catch the 'SyntaxError' exception in addition to 'ValueError' and
assume 'v' is a string. So the API can handle the request successfully
rather than returning a '500' error code.

Change-Id: Ib50cf3be41ae96ed1f9ef0463ced71443e59061c
Closes-Bug: #1406504

diff --git a/cinder/api/v1/volumes.py b/cinder/api/v1/volumes.py
index 3821cac7a3d4b3184ba85834b18164b7e6d7bbe4..a7bbda7f33139732ecc46ac60180e95f26a34d60 100644 (file)
--- a/cinder/api/v1/volumes.py
+++ b/cinder/api/v1/volumes.py
@@ -273,7 +273,7 @@ class VolumeController(wsgi.Controller):
         for k, v in search_opts.iteritems():
             try:
                 search_opts[k] = ast.literal_eval(v)
-            except ValueError:
+            except (ValueError, SyntaxError):
                 LOG.debug('Could not evaluate value %s, assuming string', v)
 
         context = req.environ['cinder.context']

diff --git a/cinder/api/v2/volumes.py b/cinder/api/v2/volumes.py
index afe8ba4f9f9ef4a5a00c85ed4431a4989fc864c6..de31de3c4535bcfe191ab9bb44ad3adf50910c9e 100644 (file)
--- a/cinder/api/v2/volumes.py
+++ b/cinder/api/v2/volumes.py
@@ -233,7 +233,7 @@ class VolumeController(wsgi.Controller):
         for k, v in filters.iteritems():
             try:
                 filters[k] = ast.literal_eval(v)
-            except ValueError:
+            except (ValueError, SyntaxError):
                 LOG.debug('Could not evaluate value %s, assuming string', v)
 
         volumes = self.volume_api.get_all(context, marker, limit, sort_key,

diff --git a/cinder/tests/api/v1/test_volumes.py b/cinder/tests/api/v1/test_volumes.py
index 8975bdf07fe9563968c4c47f117cf8172f0bc807..88a6beba6a9fdb7236e960e60b3af0ec275890dd 100644 (file)
--- a/cinder/tests/api/v1/test_volumes.py
+++ b/cinder/tests/api/v1/test_volumes.py
@@ -762,34 +762,41 @@ class VolumeApiTest(test.TestCase):
         self.assertIn('volumes', res)
         self.assertEqual(1, len(res['volumes']))
 
-    @mock.patch('cinder.utils.add_visible_admin_metadata')
     @mock.patch('cinder.volume.api.API.get_all')
-    def test_get_volumes_filter_with_string(self, get_all, add_meta):
+    def test_get_volumes_filter_with_string(self, get_all):
         req = mock.MagicMock()
         req.GET.copy.return_value = {'display_name': 'Volume-573108026'}
         context = mock.Mock()
         req.environ = {'cinder.context': context}
         self.controller._items(req, mock.Mock)
-        get_all.assert_any_call(context, sort_dir='desc',
-                                viewable_admin_meta=True,
-                                sort_key='created_at',
-                                limit=None,
-                                filters={'display_name': 'Volume-573108026'},
-                                marker=None)
-
-    @mock.patch('cinder.utils.add_visible_admin_metadata')
+        get_all.assert_called_once_with(
+            context, sort_dir='desc', viewable_admin_meta=True,
+            sort_key='created_at', limit=None,
+            filters={'display_name': 'Volume-573108026'}, marker=None)
+
     @mock.patch('cinder.volume.api.API.get_all')
-    def test_get_volumes_filter_with_list(self, get_all, add_meta):
+    def test_get_volumes_filter_with_list(self, get_all):
         req = mock.MagicMock()
         req.GET.copy.return_value = {'id': "['1', '2', '3']"}
         context = mock.Mock()
         req.environ = {'cinder.context': context}
         self.controller._items(req, mock.Mock)
-        get_all.assert_any_call(context, sort_dir='desc',
-                                viewable_admin_meta=True,
-                                sort_key='created_at',
-                                limit=None, filters={'id': ['1', '2', '3']},
-                                marker=None)
+        get_all.assert_called_once_with(
+            context, sort_dir='desc', viewable_admin_meta=True,
+            sort_key='created_at', limit=None,
+            filters={'id': ['1', '2', '3']}, marker=None)
+
+    @mock.patch('cinder.volume.api.API.get_all')
+    def test_get_volumes_filter_with_expression(self, get_all):
+        req = mock.MagicMock()
+        req.GET.copy.return_value = {'id': "d+"}
+        context = mock.Mock()
+        req.environ = {'cinder.context': context}
+        self.controller._items(req, mock.Mock)
+        get_all.assert_called_once_with(
+            context, sort_dir='desc', viewable_admin_meta=True,
+            sort_key='created_at', limit=None, filters={'id': 'd+'},
+            marker=None)
 
 
 class VolumeSerializerTest(test.TestCase):

diff --git a/cinder/tests/api/v2/test_volumes.py b/cinder/tests/api/v2/test_volumes.py
index 55e59428097822fc53c34da510c0ba624e0346da..5b79deff90435cb2bb607f2cf1d91f284a0560f6 100644 (file)
--- a/cinder/tests/api/v2/test_volumes.py
+++ b/cinder/tests/api/v2/test_volumes.py
@@ -1542,31 +1542,41 @@ class VolumeApiTest(test.TestCase):
         body = {'volume': 'string'}
         self._create_volume_bad_request(body=body)
 
-    @mock.patch('cinder.utils.add_visible_admin_metadata')
     @mock.patch('cinder.volume.api.API.get_all')
-    def test_get_volumes_filter_with_string(self, get_all, add_meta):
+    def test_get_volumes_filter_with_string(self, get_all):
         req = mock.MagicMock()
         context = mock.Mock()
         req.environ = {'cinder.context': context}
         req.params = {'display_name': 'Volume-573108026'}
         self.controller._view_builder.detail_list = mock.Mock()
         self.controller._get_volumes(req, True)
-        get_all.assert_any_call(context, None, None, 'created_at', 'desc',
-                                {'display_name': 'Volume-573108026'},
-                                viewable_admin_meta=True)
+        get_all.assert_called_once_with(
+            context, None, None, 'created_at', 'desc',
+            {'display_name': 'Volume-573108026'}, viewable_admin_meta=True)
 
-    @mock.patch('cinder.utils.add_visible_admin_metadata')
     @mock.patch('cinder.volume.api.API.get_all')
-    def test_get_volumes_filter_with_list(self, get_all, add_meta):
+    def test_get_volumes_filter_with_list(self, get_all):
         req = mock.MagicMock()
         context = mock.Mock()
         req.environ = {'cinder.context': context}
         req.params = {'id': "['1', '2', '3']"}
         self.controller._view_builder.detail_list = mock.Mock()
         self.controller._get_volumes(req, True)
-        get_all.assert_any_call(context, None, None, 'created_at', 'desc',
-                                {'id': ['1', '2', '3']},
-                                viewable_admin_meta=True)
+        get_all.assert_called_once_with(
+            context, None, None, 'created_at', 'desc',
+            {'id': ['1', '2', '3']}, viewable_admin_meta=True)
+
+    @mock.patch('cinder.volume.api.API.get_all')
+    def test_get_volumes_filter_with_expression(self, get_all):
+        req = mock.MagicMock()
+        context = mock.Mock()
+        req.environ = {'cinder.context': context}
+        req.params = {'name': "d-"}
+        self.controller._view_builder.detail_list = mock.Mock()
+        self.controller._get_volumes(req, True)
+        get_all.assert_called_once_with(
+            context, None, None, 'created_at', 'desc',
+            {'display_name': 'd-'}, viewable_admin_meta=True)
 
 
 class VolumeSerializerTest(test.TestCase):