From f39eeebf6eca2745b83a79d2a0def57a126846e7 Mon Sep 17 00:00:00 2001 From: Valeriy Ponomaryov Date: Tue, 30 Dec 2014 17:51:31 +0200 Subject: [PATCH] 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 --- cinder/api/v1/volumes.py | 2 +- cinder/api/v2/volumes.py | 2 +- cinder/tests/api/v1/test_volumes.py | 39 +++++++++++++++++------------ cinder/tests/api/v2/test_volumes.py | 30 ++++++++++++++-------- 4 files changed, 45 insertions(+), 28 deletions(-) diff --git a/cinder/api/v1/volumes.py b/cinder/api/v1/volumes.py index 3821cac7a..a7bbda7f3 100644 --- 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 afe8ba4f9..de31de3c4 100644 --- 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 8975bdf07..88a6beba6 100644 --- 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 55e594280..5b79deff9 100644 --- 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): -- 2.45.2