From 4f58e79436a92acbdb6a738f3f8aa8200ebcc471 Mon Sep 17 00:00:00 2001 From: Victor Sergeyev Date: Tue, 13 May 2014 11:54:23 +0300 Subject: [PATCH] Remove common context usage from db model_query() Currently, function model_query() from oslo.db.sqlalchemy.utils uses is_use_context() function in order to determine whether a normal or an admin user request is being processed. But usage of RequestContext is not unified across OpenStack projects, so there is no way for oslo.db to guarantee it processes project_id/deleted filters correctly here. To remove this ambiguity, project_id/deleted filters should be passed explicitly instead. At the same time, developers of OpenStack projects are encouraged to provide wrappers for oslo.db model_query() function, so that they could determine the values for project_id/deleted filters depending on how they use RequestContext in their project. Closes-bug: #1288846 Change-Id: Iec5cbbc44cf9626f0464ab684dff1ba37e006151 --- oslo/db/sqlalchemy/utils.py | 141 ++++++++++++++++++++++----------- tests/sqlalchemy/test_utils.py | 99 ++++++++--------------- 2 files changed, 124 insertions(+), 116 deletions(-) diff --git a/oslo/db/sqlalchemy/utils.py b/oslo/db/sqlalchemy/utils.py index 4e17fc7e..3ecd0047 100644 --- a/oslo/db/sqlalchemy/utils.py +++ b/oslo/db/sqlalchemy/utils.py @@ -29,14 +29,12 @@ from sqlalchemy import func from sqlalchemy import Index from sqlalchemy import Integer from sqlalchemy import MetaData -from sqlalchemy import or_ from sqlalchemy.sql.expression import literal_column from sqlalchemy.sql.expression import UpdateBase from sqlalchemy import String from sqlalchemy import Table from sqlalchemy.types import NullType -from oslo.db.openstack.common import context as request_context from oslo.db.openstack.common.gettextutils import _, _LI, _LW from oslo.db.openstack.common import timeutils from oslo.db.sqlalchemy import models @@ -157,46 +155,37 @@ def paginate_query(query, model, limit, sort_keys, marker=None, return query -def _read_deleted_filter(query, db_model, read_deleted): +def _read_deleted_filter(query, db_model, deleted): if 'deleted' not in db_model.__table__.columns: raise ValueError(_("There is no `deleted` column in `%s` table. " "Project doesn't use soft-deleted feature.") % db_model.__name__) default_deleted_value = db_model.__table__.c.deleted.default.arg - if read_deleted == 'no': - query = query.filter(db_model.deleted == default_deleted_value) - elif read_deleted == 'yes': - pass # omit the filter to include deleted and active - elif read_deleted == 'only': + if deleted: query = query.filter(db_model.deleted != default_deleted_value) else: - raise ValueError(_("Unrecognized read_deleted value '%s'") - % read_deleted) + query = query.filter(db_model.deleted == default_deleted_value) return query -def _project_filter(query, db_model, context, project_only): - if project_only and 'project_id' not in db_model.__table__.columns: +def _project_filter(query, db_model, project_id): + if 'project_id' not in db_model.__table__.columns: raise ValueError(_("There is no `project_id` column in `%s` table.") % db_model.__name__) - if request_context.is_user_context(context) and project_only: - if project_only == 'allow_none': - is_none = None - query = query.filter(or_(db_model.project_id == context.project_id, - db_model.project_id == is_none)) - else: - query = query.filter(db_model.project_id == context.project_id) + if isinstance(project_id, (list, tuple, set)): + query = query.filter(db_model.project_id.in_(project_id)) + else: + query = query.filter(db_model.project_id == project_id) return query -def model_query(context, model, session, args=None, project_only=False, - read_deleted=None): - """Query helper that accounts for context's `read_deleted` field. +def model_query(model, session, args=None, **kwargs): + """Query helper for db.sqlalchemy api methods. - :param context: context to query under + This accounts for `deleted` and `project_id` fields. :param model: Model to query. Must be a subclass of ModelBase. :type model: models.ModelBase @@ -207,44 +196,100 @@ def model_query(context, model, session, args=None, project_only=False, :param args: Arguments to query. If None - model is used. :type args: tuple - :param project_only: If present and context is user-type, then restrict - query to match the context's project_id. If set to - 'allow_none', restriction includes project_id = None. - :type project_only: bool + Keyword arguments: + + :keyword project_id: If present, allows filtering by project_id(s). + Can be either a project_id value, or an iterable of + project_id values, or None. If an iterable is passed, + only rows whose project_id column value is on the + `project_id` list will be returned. If None is passed, + only rows which are not bound to any project, will be + returned. + :type project_id: iterable, + model.__table__.columns.project_id.type, + None type + + :keyword deleted: If present, allows filtering by deleted field. + If True is passed, only deleted entries will be + returned, if False - only existing entries. + :type deleted: bool - :param read_deleted: If present, overrides context's read_deleted field. - :type read_deleted: bool Usage: - ..code:: python + .. code-block:: python - result = (utils.model_query(context, models.Instance, session=session) - .filter_by(uuid=instance_uuid) - .all()) + from oslo.db.sqlalchemy import utils - query = utils.model_query( - context, Node, - session=session, - args=(func.count(Node.id), func.sum(Node.ram)) - ).filter_by(project_id=project_id) + + def get_instance_by_uuid(uuid): + session = get_session() + with session.begin() + return (utils.model_query(models.Instance, session=session) + .filter(models.Instance.uuid == uuid) + .first()) + + def get_nodes_stat(): + data = (Node.id, Node.cpu, Node.ram, Node.hdd) + + session = get_session() + with session.begin() + return utils.model_query(Node, session=session, args=data).all() + + Also you can create your own helper, based on ``utils.model_query()``. + For example, it can be useful if you plan to use ``project_id`` and + ``deleted`` parameters from project's ``context`` + + .. code-block:: python + + from oslo.db.sqlalchemy import utils + + + def _model_query(context, model, session=None, args=None, + project_id=None, project_only=False, + read_deleted=None): + + # We suppose, that functions ``_get_project_id()`` and + # ``_get_deleted()`` should handle passed parameters and + # context object (for example, decide, if we need to restrict a user + # to query his own entries by project_id or only allow admin to read + # deleted entries). For return values, we expect to get + # ``project_id`` and ``deleted``, which are suitable for the + # ``model_query()`` signature. + kwargs = {} + if project_id is not None: + kwargs['project_id'] = _get_project_id(context, project_id, + project_only) + if read_deleted is not None: + kwargs['deleted'] = _get_deleted_dict(context, read_deleted) + session = session or get_session() + + with session.begin(): + return utils.model_query(model, session=session, + args=args, **kwargs) + + def get_instance_by_uuid(context, uuid): + return (_model_query(context, models.Instance, read_deleted='yes') + .filter(models.Instance.uuid == uuid) + .first()) + + def get_nodes_data(context, project_id, project_only='allow_none'): + data = (Node.id, Node.cpu, Node.ram, Node.hdd) + + return (_model_query(context, Node, args=data, project_id=project_id, + project_only=project_only) + .all()) """ - if not read_deleted: - if hasattr(context, 'read_deleted'): - # NOTE(viktors): some projects use `read_deleted` attribute in - # their contexts instead of `show_deleted`. - read_deleted = context.read_deleted - else: - read_deleted = context.show_deleted - if not issubclass(model, models.ModelBase): raise TypeError(_("model should be a subclass of ModelBase")) query = session.query(model) if not args else session.query(*args) - query = _read_deleted_filter(query, model, read_deleted) - query = _project_filter(query, model, context, project_only) + if 'deleted' in kwargs: + query = _read_deleted_filter(query, model, kwargs['deleted']) + if 'project_id' in kwargs: + query = _project_filter(query, model, kwargs['project_id']) return query diff --git a/tests/sqlalchemy/test_utils.py b/tests/sqlalchemy/test_utils.py index 5ba80a45..36a2b14b 100644 --- a/tests/sqlalchemy/test_utils.py +++ b/tests/sqlalchemy/test_utils.py @@ -741,32 +741,18 @@ class TestModelQuery(test_base.BaseTestCase): self.session = mock.MagicMock() self.session.query.return_value = self.session.query self.session.query.filter.return_value = self.session.query - self.user_context = mock.MagicMock(is_admin=False, read_deleted='yes', - user_id=42, project_id=43) def test_wrong_model(self): - self.assertRaises(TypeError, utils.model_query, self.user_context, + self.assertRaises(TypeError, utils.model_query, FakeModel, session=self.session) def test_no_soft_deleted(self): - self.assertRaises(ValueError, utils.model_query, self.user_context, - MyModel, session=self.session) + self.assertRaises(ValueError, utils.model_query, + MyModel, session=self.session, deleted=True) - def test_read_deleted_only(self): + def test_deleted_false(self): mock_query = utils.model_query( - self.user_context, MyModelSoftDeleted, - session=self.session, read_deleted='only') - - deleted_filter = mock_query.filter.call_args[0][0] - self.assertEqual(str(deleted_filter), - 'soft_deleted_test_model.deleted != :deleted_1') - self.assertEqual(deleted_filter.right.value, - MyModelSoftDeleted.__mapper__.c.deleted.default.arg) - - def test_read_deleted_no(self): - mock_query = utils.model_query( - self.user_context, MyModelSoftDeleted, - session=self.session, read_deleted='no') + MyModelSoftDeleted, session=self.session, deleted=False) deleted_filter = mock_query.filter.call_args[0][0] self.assertEqual(str(deleted_filter), @@ -774,76 +760,53 @@ class TestModelQuery(test_base.BaseTestCase): self.assertEqual(deleted_filter.right.value, MyModelSoftDeleted.__mapper__.c.deleted.default.arg) - def test_read_deleted_yes(self): + def test_deleted_true(self): mock_query = utils.model_query( - self.user_context, MyModelSoftDeleted, - session=self.session, read_deleted='yes') + MyModelSoftDeleted, session=self.session, deleted=True) - self.assertEqual(mock_query.filter.call_count, 0) + deleted_filter = mock_query.filter.call_args[0][0] + self.assertEqual(str(deleted_filter), + 'soft_deleted_test_model.deleted != :deleted_1') + self.assertEqual(deleted_filter.right.value, + MyModelSoftDeleted.__mapper__.c.deleted.default.arg) - def test_wrong_read_deleted(self): - self.assertRaises(ValueError, utils.model_query, self.user_context, - MyModelSoftDeleted, session=self.session, - read_deleted='ololo') + @mock.patch.object(utils, "_read_deleted_filter") + def test_no_deleted_value(self, _read_deleted_filter): + utils.model_query(MyModelSoftDeleted, session=self.session) + self.assertEqual(_read_deleted_filter.call_count, 0) + + def test_project_filter(self): + project_id = 10 - def test_project_only_true(self): mock_query = utils.model_query( - self.user_context, MyModelSoftDeletedProjectId, - session=self.session, project_only=True) + MyModelSoftDeletedProjectId, session=self.session, + project_only=True, project_id=project_id) deleted_filter = mock_query.filter.call_args[0][0] self.assertEqual( str(deleted_filter), 'soft_deleted_project_id_test_model.project_id = :project_id_1') - self.assertEqual(deleted_filter.right.value, - self.user_context.project_id) + self.assertEqual(deleted_filter.right.value, project_id) def test_project_filter_wrong_model(self): - self.assertRaises(ValueError, utils.model_query, self.user_context, + self.assertRaises(ValueError, utils.model_query, MyModelSoftDeleted, session=self.session, - project_only=True) + project_id=10) - def test_read_deleted_allow_none(self): + def test_project_filter_allow_none(self): mock_query = utils.model_query( - self.user_context, MyModelSoftDeletedProjectId, - session=self.session, project_only='allow_none') + MyModelSoftDeletedProjectId, + session=self.session, project_id=(10, None)) self.assertEqual( str(mock_query.filter.call_args[0][0]), - 'soft_deleted_project_id_test_model.project_id = :project_id_1 OR' - ' soft_deleted_project_id_test_model.project_id IS NULL' + 'soft_deleted_project_id_test_model.project_id' + ' IN (:project_id_1, NULL)' ) - @mock.patch.object(utils, "_read_deleted_filter") - @mock.patch.object(utils, "_project_filter") - def test_context_show_deleted(self, _project_filter, _read_deleted_filter): - user_context = mock.MagicMock(is_admin=False, show_deleted='yes', - user_id=42, project_id=43) - delattr(user_context, 'read_deleted') - _read_deleted_filter.return_value = self.session.query - _project_filter.return_value = self.session.query - utils.model_query(user_context, MyModel, - args=(MyModel.id,), session=self.session) - + def test_model_query_common(self): + utils.model_query(MyModel, args=(MyModel.id,), session=self.session) self.session.query.assert_called_with(MyModel.id) - _read_deleted_filter.assert_called_with( - self.session.query, MyModel, user_context.show_deleted) - _project_filter.assert_called_with( - self.session.query, MyModel, user_context, False) - - @mock.patch.object(utils, "_read_deleted_filter") - @mock.patch.object(utils, "_project_filter") - def test_model_query_common(self, _project_filter, _read_deleted_filter): - _read_deleted_filter.return_value = self.session.query - _project_filter.return_value = self.session.query - utils.model_query(self.user_context, MyModel, - args=(MyModel.id,), session=self.session) - - self.session.query.assert_called_with(MyModel.id) - _read_deleted_filter.assert_called_with( - self.session.query, MyModel, self.user_context.read_deleted) - _project_filter.assert_called_with( - self.session.query, MyModel, self.user_context, False) class TestUtils(db_test_base.DbTestCase):