From 96cacc6225a3fe48d06e0e41d6e74e6af1595f7f Mon Sep 17 00:00:00 2001 From: Sean McCully Date: Thu, 10 Sep 2015 16:33:37 -0400 Subject: [PATCH] Use of ast for integers doesn't changes type ast.literal_eval doesn't change types for Integer Booleans resulting in DBAPIError exceptions with at least postgresql. Boolean columns need to be explicitly cast to boolean types. This changes the way filters are processed in the volume API. APIImpact Change-Id: Ice9c57fa99c17e4c87de6362bf30ed30503a1bed Closes-Bug: #1494475 --- cinder/api/v2/volumes.py | 9 +------- cinder/db/api.py | 4 ++++ cinder/db/sqlalchemy/api.py | 17 ++++++++++++-- cinder/tests/unit/api/v2/test_volumes.py | 28 ++++++++++++++++++++++++ cinder/volume/api.py | 13 ++++++++++- 5 files changed, 60 insertions(+), 11 deletions(-) diff --git a/cinder/api/v2/volumes.py b/cinder/api/v2/volumes.py index a73b65a0e..eec160bff 100644 --- a/cinder/api/v2/volumes.py +++ b/cinder/api/v2/volumes.py @@ -16,8 +16,6 @@ """The volumes api.""" -import ast - from oslo_config import cfg from oslo_log import log as logging from oslo_utils import uuidutils @@ -243,12 +241,7 @@ class VolumeController(wsgi.Controller): filters['display_name'] = filters['name'] del filters['name'] - for k, v in filters.items(): - try: - filters[k] = ast.literal_eval(v) - except (ValueError, SyntaxError): - LOG.debug('Could not evaluate value %s, assuming string', v) - + self.volume_api.check_volume_filters(filters) volumes = self.volume_api.get_all(context, marker, limit, sort_keys=sort_keys, sort_dirs=sort_dirs, diff --git a/cinder/db/api.py b/cinder/db/api.py index 5fe56e443..9472c59dc 100644 --- a/cinder/db/api.py +++ b/cinder/db/api.py @@ -1020,6 +1020,10 @@ def purge_deleted_rows(context, age_in_days): return IMPL.purge_deleted_rows(context, age_in_days=age_in_days) +def get_booleans_for_table(table_name): + return IMPL.get_booleans_for_table(table_name) + + ################### diff --git a/cinder/db/sqlalchemy/api.py b/cinder/db/sqlalchemy/api.py index 622b7eb0d..148c993f7 100644 --- a/cinder/db/sqlalchemy/api.py +++ b/cinder/db/sqlalchemy/api.py @@ -46,6 +46,7 @@ from sqlalchemy.sql.expression import desc from sqlalchemy.sql.expression import literal_column from sqlalchemy.sql.expression import true from sqlalchemy.sql import func +from sqlalchemy.sql import sqltypes from cinder.api import common from cinder.common import sqlalchemyutils @@ -1104,6 +1105,18 @@ def volume_create(context, values): return _volume_get(context, values['id'], session=session) +def get_booleans_for_table(table_name): + booleans = set() + table = getattr(models, table_name.capitalize()) + if hasattr(table, '__table__'): + columns = table.__table__.columns + for column in columns: + if isinstance(column.type, sqltypes.Boolean): + booleans.add(column.name) + + return booleans + + @require_admin_context def volume_data_get_for_host(context, host, count_only=False): host_attr = models.Volume.host @@ -3180,8 +3193,8 @@ def volume_type_encryption_update(context, volume_type_id, values): session) if not encryption: - raise exception.VolumeTypeEncryptionNotFound(type_id= - volume_type_id) + raise exception.VolumeTypeEncryptionNotFound( + type_id=volume_type_id) encryption.update(values) diff --git a/cinder/tests/unit/api/v2/test_volumes.py b/cinder/tests/unit/api/v2/test_volumes.py index 73697464c..546602afd 100644 --- a/cinder/tests/unit/api/v2/test_volumes.py +++ b/cinder/tests/unit/api/v2/test_volumes.py @@ -1306,6 +1306,34 @@ class VolumeApiTest(test.TestCase): filters={'display_name': 'Volume-573108026'}, viewable_admin_meta=True, offset=0) + @mock.patch('cinder.volume.api.API.get_all') + def test_get_volumes_filter_with_true(self, get_all): + req = mock.MagicMock() + context = mock.Mock() + req.environ = {'cinder.context': context} + req.params = {'display_name': 'Volume-573108026', 'bootable': 1} + self.controller._view_builder.detail_list = mock.Mock() + self.controller._get_volumes(req, True) + get_all.assert_called_once_with( + context, None, CONF.osapi_max_limit, + sort_keys=['created_at'], sort_dirs=['desc'], + filters={'display_name': 'Volume-573108026', 'bootable': True}, + viewable_admin_meta=True, offset=0) + + @mock.patch('cinder.volume.api.API.get_all') + def test_get_volumes_filter_with_false(self, get_all): + req = mock.MagicMock() + context = mock.Mock() + req.environ = {'cinder.context': context} + req.params = {'display_name': 'Volume-573108026', 'bootable': 0} + self.controller._view_builder.detail_list = mock.Mock() + self.controller._get_volumes(req, True) + get_all.assert_called_once_with( + context, None, CONF.osapi_max_limit, + sort_keys=['created_at'], sort_dirs=['desc'], + filters={'display_name': 'Volume-573108026', 'bootable': False}, + viewable_admin_meta=True, offset=0) + @mock.patch('cinder.volume.api.API.get_all') def test_get_volumes_filter_with_list(self, get_all): req = mock.MagicMock() diff --git a/cinder/volume/api.py b/cinder/volume/api.py index 03f1d74fa..2fd8db425 100644 --- a/cinder/volume/api.py +++ b/cinder/volume/api.py @@ -16,7 +16,7 @@ """Handles all requests relating to volumes.""" - +import ast import collections import datetime import functools @@ -1691,6 +1691,17 @@ class API(base.Base): # level to show an admin how a backend is configured. return self.volume_rpcapi.list_replication_targets(ctxt, volume) + def check_volume_filters(self, filters): + booleans = self.db.get_booleans_for_table('volume') + for k, v in filters.iteritems(): + try: + if k in booleans: + filters[k] = bool(v) + else: + filters[k] = ast.literal_eval(v) + except (ValueError, SyntaxError): + LOG.debug('Could not evaluate value %s, assuming string', v) + class HostAPI(base.Base): def __init__(self): -- 2.45.2