utils: fix get_unique_keys() when model has no table attached to it

Using the internal SQLAlchemy __table__ to find unique keys on a model
class does not work if the model is not attached to a class. In this
case, it seems there is no way to find what the keys are: rather than
crashing or returning garbage, the function just returns None,
indicating it does not know how to get it.

The warning from paginate_query() is not emitted in this case, as
there's no way to guess if the developer did wrong or not.

Change-Id: I1291fe1f1e78471f18f230a66edff31ed199bc7a
(cherry picked from commit 49eaed489e)

oslo_db/sqlalchemy/utils.py

@@ -31,6 +31,7 @@ from sqlalchemy import Column
 from sqlalchemy.engine import Connectable
 from sqlalchemy.engine import reflection
 from sqlalchemy.engine import url as sa_url
+from sqlalchemy import exc
 from sqlalchemy import func
 from sqlalchemy import Index
 from sqlalchemy import inspect
@@ -71,16 +72,26 @@ def _get_unique_keys(model):
 
     :param model: the ORM model class
     :rtype: list of sets of strings
-    :return: unique model keys
+    :return: unique model keys or None if unable to find them
     """
+
+    try:
+        mapper = inspect(model)
+    except exc.NoInspectionAvailable:
+        return None
+    else:
+        table = mapper.mapped_table
+        if table is None:
+            return None
+
     # extract result from cache if present
-    info = model.__table__.info
+    info = table.info
     if 'oslodb_unique_keys' in info:
         return info['oslodb_unique_keys']
 
     res = []
     try:
-        constraints = model.__table__.constraints
+        constraints = table.constraints
     except AttributeError:
         constraints = []
     for constraint in constraints:
@@ -89,7 +100,7 @@ def _get_unique_keys(model):
                                    sqlalchemy.PrimaryKeyConstraint)):
             res.append({c.name for c in constraint.columns})
     try:
-        indexes = model.__table__.indexes
+        indexes = table.indexes
     except AttributeError:
         indexes = []
     for index in indexes:
@@ -101,8 +112,16 @@ def _get_unique_keys(model):
 
 
 def _stable_sorting_order(model, sort_keys):
+    """Check whetever the sorting order is stable.
+
+    :return: True if it is stable, False if it's not, None if it's impossible
+    to determine.
+    """
+    keys = _get_unique_keys(model)
+    if keys is None:
+        return None
     sort_keys_set = set(sort_keys)
-    for unique_keys in _get_unique_keys(model):
+    for unique_keys in keys:
         if unique_keys.issubset(sort_keys_set):
             return True
     return False
@@ -142,7 +161,7 @@ def paginate_query(query, model, limit, sort_keys, marker=None,
     :rtype: sqlalchemy.orm.query.Query
     :return: The query with sorting/pagination added.
     """
-    if not _stable_sorting_order(model, sort_keys):
+    if _stable_sorting_order(model, sort_keys) is False:
         LOG.warning(_LW('Unique keys not in sort_keys. '
                         'The sorting order may be unstable.'))
 

oslo_db/tests/sqlalchemy/test_utils.py

@@ -31,6 +31,7 @@ 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 mapper
 from sqlalchemy.orm import Session
 from sqlalchemy.sql import select
 from sqlalchemy.types import UserDefinedType, NullType
@@ -115,6 +116,20 @@ class FakeTableWithIndexes(Base):
     )
 
 
+class FakeTableClassicalyMapped(object):
+    pass
+
+
+fake_table = Table(
+    'fake_table_classically_mapped',
+    Base.metadata,
+    Column('id', Integer, primary_key=True),
+    Column('key', String(50))
+)
+
+mapper(FakeTableClassicalyMapped, fake_table)
+
+
 class FakeModel(object):
     def __init__(self, values):
         self.values = values
@@ -304,11 +319,19 @@ class Test_UnstableSortingOrder(test_base.BaseTestCase):
             utils._stable_sorting_order(
                 FakeTableWithMultipleKeys, ['key1', 'key2']))
 
+    def test_classically_mapped_primary_keys_stable(self):
+        self.assertTrue(
+            utils._stable_sorting_order(FakeTableClassicalyMapped, ['id']))
+
     def test_multiple_primary_keys_unstable(self):
         self.assertFalse(
             utils._stable_sorting_order(
                 FakeTableWithMultipleKeys, ['key1', 'key3']))
 
+    def test_unknown_primary_keys_stable(self):
+        self.assertIsNone(
+            utils._stable_sorting_order(object, ['key1', 'key2']))
+
     def test_unique_index_stable(self):
         self.assertTrue(
             utils._stable_sorting_order(
@@ -331,6 +354,9 @@ class TestGetUniqueKeys(test_base.BaseTestCase):
             [{'id'}, {'key1', 'key2'}],
             utils._get_unique_keys(FakeTableWithIndexes))
 
+    def test_unknown_primary_keys(self):
+        self.assertIsNone(utils._get_unique_keys(object))
+
     def test_cache(self):
 
         class CacheTable(object):
@@ -349,21 +375,24 @@ class TestGetUniqueKeys(test_base.BaseTestCase):
                 return []
 
         class CacheModel(object):
-            __table__ = CacheTable()
+            pass
 
+        table = CacheTable()
+        mock_inspect = mock.Mock(return_value=mock.Mock(mapped_table=table))
         model = CacheModel()
         self.assertNotIn('oslodb_unique_keys', CacheTable.info)
-        utils._get_unique_keys(model)
+        with mock.patch("oslo_db.sqlalchemy.utils.inspect", mock_inspect):
+            utils._get_unique_keys(model)
 
         self.assertIn('oslodb_unique_keys', CacheTable.info)
-        self.assertEqual(1, model.__table__.constraints_called)
-        self.assertEqual(1, model.__table__.indexes_called)
+        self.assertEqual(1, table.constraints_called)
+        self.assertEqual(1, table.indexes_called)
 
         for i in range(10):
             utils._get_unique_keys(model)
 
-        self.assertEqual(1, model.__table__.constraints_called)
-        self.assertEqual(1, model.__table__.indexes_called)
+        self.assertEqual(1, table.constraints_called)
+        self.assertEqual(1, table.indexes_called)
 
 
 class TestPaginateQueryActualSQL(test_base.BaseTestCase):