Fix issue with passing lists in filters

The Cinder db code already allows searching for a value in a list,
for example, id in ['id1', 'id2', 'id3']. The problem is when we
get the value at the API it comes as a string (e.g.
"['id1', 'id2', 'id3']", and therefore the filter fails. This was
already fixed for metadata filters, so just apply it to all filters.

We do a try/except because if it really is a string, then the
literal_eval will fail because we don't pass double quotes, such as
"'string'".  The current code therefore will now cover lists and
fall back to the original behavior for strings.

Change-Id: Idfd8298401a330bb99ddcd130268fabb6ef9d162
Closes-Bug: 1403875

diff --git a/cinder/api/v1/volumes.py b/cinder/api/v1/volumes.py
index bc50c0b46e227481f4a5bc0086a90f1d7a35bc6a..3821cac7a3d4b3184ba85834b18164b7e6d7bbe4 100644 (file)
--- a/cinder/api/v1/volumes.py
+++ b/cinder/api/v1/volumes.py
@@ -270,8 +270,11 @@ class VolumeController(wsgi.Controller):
         search_opts.pop('limit', None)
         search_opts.pop('offset', None)
 
-        if 'metadata' in search_opts:
-            search_opts['metadata'] = ast.literal_eval(search_opts['metadata'])
+        for k, v in search_opts.iteritems():
+            try:
+                search_opts[k] = ast.literal_eval(v)
+            except ValueError:
+                LOG.debug('Could not evaluate value %s, assuming string', v)
 
         context = req.environ['cinder.context']
         utils.remove_invalid_filter_options(context,

diff --git a/cinder/api/v2/volumes.py b/cinder/api/v2/volumes.py
index 0f30eea02ac40383da78743ab2d2a73823c3b4bd..afe8ba4f9f9ef4a5a00c85ed4431a4989fc864c6 100644 (file)
--- a/cinder/api/v2/volumes.py
+++ b/cinder/api/v2/volumes.py
@@ -230,8 +230,11 @@ class VolumeController(wsgi.Controller):
             filters['display_name'] = filters['name']
             del filters['name']
 
-        if 'metadata' in filters:
-            filters['metadata'] = ast.literal_eval(filters['metadata'])
+        for k, v in filters.iteritems():
+            try:
+                filters[k] = ast.literal_eval(v)
+            except ValueError:
+                LOG.debug('Could not evaluate value %s, assuming string', v)
 
         volumes = self.volume_api.get_all(context, marker, limit, sort_key,
                                           sort_dir, filters,

diff --git a/cinder/tests/api/v1/test_volumes.py b/cinder/tests/api/v1/test_volumes.py
index e3a7b11b90cdef405b03bc8233f050beb88146f6..8975bdf07fe9563968c4c47f117cf8172f0bc807 100644 (file)
--- a/cinder/tests/api/v1/test_volumes.py
+++ b/cinder/tests/api/v1/test_volumes.py
@@ -16,6 +16,7 @@
 import datetime
 
 from lxml import etree
+import mock
 from oslo.config import cfg
 import webob
 
@@ -761,6 +762,35 @@ 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):
+        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')
+    @mock.patch('cinder.volume.api.API.get_all')
+    def test_get_volumes_filter_with_list(self, get_all, add_meta):
+        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)
+
 
 class VolumeSerializerTest(test.TestCase):
     def _verify_volume_attachment(self, attach, tree):

diff --git a/cinder/tests/api/v2/test_volumes.py b/cinder/tests/api/v2/test_volumes.py
index 1c38dda1156e818f03581c85edff7c81b194edb8..55e59428097822fc53c34da510c0ba624e0346da 100644 (file)
--- a/cinder/tests/api/v2/test_volumes.py
+++ b/cinder/tests/api/v2/test_volumes.py
@@ -17,6 +17,7 @@
 import datetime
 
 from lxml import etree
+import mock
 from oslo.config import cfg
 import six
 import six.moves.urllib.parse as urlparse
@@ -1541,6 +1542,32 @@ 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):
+        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)
+
+    @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):
+        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)
+
 
 class VolumeSerializerTest(test.TestCase):
     def _verify_volume_attachment(self, attach, tree):