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

2 files changed, 120 insertions, 112 deletions

diff --git a/oslo/db/sqlalchemy/utils.py b/oslo/db/sqlalchemy/utils.py
index 4e17fc7..3ecd004 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())
 
-    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
+      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 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 5ba80a4..36a2b14 100644
--- a/tests/sqlalchemy/test_utils.py
+++ b/tests/sqlalchemy/test_utils.py
@@ -741,109 +741,72 @@ 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')
+            MyModelSoftDeleted, session=self.session, deleted=False)
 
         deleted_filter = mock_query.filter.call_args[0][0]
         self.assertEqual(str(deleted_filter),
-                         'soft_deleted_test_model.deleted != :deleted_1')
+                         '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):
+    def test_deleted_true(self):
         mock_query = utils.model_query(
-            self.user_context, MyModelSoftDeleted,
-            session=self.session, read_deleted='no')
+            MyModelSoftDeleted, session=self.session, deleted=True)
 
         deleted_filter = mock_query.filter.call_args[0][0]
         self.assertEqual(str(deleted_filter),
-                         'soft_deleted_test_model.deleted = :deleted_1')
+                         'soft_deleted_test_model.deleted != :deleted_1')
         self.assertEqual(deleted_filter.right.value,
                          MyModelSoftDeleted.__mapper__.c.deleted.default.arg)
 
-    def test_read_deleted_yes(self):
-        mock_query = utils.model_query(
-            self.user_context, MyModelSoftDeleted,
-            session=self.session, read_deleted='yes')
-
-        self.assertEqual(mock_query.filter.call_count, 0)
+    @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_wrong_read_deleted(self):
-        self.assertRaises(ValueError, utils.model_query, self.user_context,
-                          MyModelSoftDeleted, session=self.session,
-                          read_deleted='ololo')
+    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)
-
-        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)
-
+    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, self.user_context.read_deleted)
-        _project_filter.assert_called_with(
-            self.session.query, MyModel, self.user_context, False)
 
 
 class TestUtils(db_test_base.DbTestCase):