Merge "Remove common context usage from db model_query()"

2 files changed, 120 insertions, 112 deletions

diff --git a/oslo/db/sqlalchemy/utils.py b/oslo/db/sqlalchemy/utils.py
index 12ffcd9..04eb7ce 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):