Sort model fields using getattr(), not inspect()

While inspect(mapping).all_orm_descriptors is a good
way to get at attributes that are known to be mapped or
otherwise described by SQLAlchemy, it does not produce a
SQL expression in the case of a hybrid descriptor, where it
produces the descriptor, not the result of calling it.
This fix ensures that the descriptor is evaulated, while
still retaining the fact that we check the attribute
as coming from an ORM-level descriptor.

Change-Id: I04a03be713348158b8cdfda683624fea01fe34ea
Closes-Bug: #1451880

2 files changed, 53 insertions, 1 deletions

diff --git a/oslo_db/sqlalchemy/utils.py b/oslo_db/sqlalchemy/utils.py
index 9382b04..b4e0664 100644
--- a/oslo_db/sqlalchemy/utils.py
+++ b/oslo_db/sqlalchemy/utils.py
@@ -147,10 +147,13 @@ def paginate_query(query, model, limit, sort_keys, marker=None,
             raise ValueError(_("Unknown sort direction, "
                                "must be 'desc' or 'asc'"))
         try:
-            sort_key_attr = inspect(model).\
+            inspect(model).\
                 all_orm_descriptors[current_sort_key]
         except KeyError:
             raise exception.InvalidSortKey()
+        else:
+            sort_key_attr = getattr(model, current_sort_key)
+
         query = query.order_by(sort_dir_func(sort_key_attr))
 
     # Add pagination
diff --git a/oslo_db/tests/sqlalchemy/test_utils.py b/oslo_db/tests/sqlalchemy/test_utils.py
index 5b7d3d7..6719d0f 100644
--- a/oslo_db/tests/sqlalchemy/test_utils.py
+++ b/oslo_db/tests/sqlalchemy/test_utils.py
@@ -29,6 +29,8 @@ from sqlalchemy.engine import reflection
 from sqlalchemy.engine import url as sa_url
 from sqlalchemy.exc import OperationalError
 from sqlalchemy.ext.declarative import declarative_base
+from sqlalchemy.ext.hybrid import hybrid_property
+from sqlalchemy.orm import Session
 from sqlalchemy.sql import select
 from sqlalchemy.types import UserDefinedType, NullType
 
@@ -73,6 +75,18 @@ class FakeTable(Base):
     project_id = Column(String(50))
     snapshot_id = Column(String(50))
 
+    # mox is comparing in some awkward way that
+    # in this case requires the same identity of object
+    _expr_to_appease_mox = project_id + snapshot_id
+
+    @hybrid_property
+    def some_hybrid(self):
+        raise NotImplementedError()
+
+    @some_hybrid.expression
+    def some_hybrid(cls):
+        return cls._expr_to_appease_mox
+
     def foo(self):
         pass
 
@@ -194,6 +208,41 @@ class TestPaginateQuery(test_base.BaseTestCase):
                           self.query, self.model, 5, ['user_id', 'project_id'],
                           marker=self.marker, sort_dirs=['asc', 'mixed'])
 
+    def test_paginate_on_hybrid(self):
+        sqlalchemy.asc(self.model.user_id).AndReturn('asc_1')
+        self.query.order_by('asc_1').AndReturn(self.query)
+
+        sqlalchemy.desc(self.model.some_hybrid).AndReturn('desc_1')
+        self.query.order_by('desc_1').AndReturn(self.query)
+
+        self.query.limit(5).AndReturn(self.query)
+        self.mox.ReplayAll()
+        utils.paginate_query(self.query, self.model, 5,
+                             ['user_id', 'some_hybrid'],
+                             sort_dirs=['asc', 'desc'])
+
+
+class TestPaginateQueryActualSQL(test_base.BaseTestCase):
+
+    def test_paginate_on_hybrid_assert_stmt(self):
+        s = Session()
+        q = s.query(FakeTable)
+        q = utils.paginate_query(
+            q, FakeTable, 5,
+            ['user_id', 'some_hybrid'],
+            sort_dirs=['asc', 'desc'])
+        expected_core_sql = (
+            select([FakeTable]).
+            order_by(sqlalchemy.asc(FakeTable.user_id)).
+            order_by(sqlalchemy.desc(FakeTable.some_hybrid)).
+            limit(5)
+        )
+
+        self.assertEqual(
+            str(q.statement.compile()),
+            str(expected_core_sql.compile())
+        )
+
 
 class TestMigrationUtils(db_test_base.DbTestCase):