From 49eaed489e4650e56ed2e270ba3e15ce2b421cc8 Mon Sep 17 00:00:00 2001 From: Julien Danjou Date: Fri, 2 Sep 2016 10:40:16 +0200 Subject: [PATCH] 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 --- oslo_db/sqlalchemy/utils.py | 31 +++++++++++++++---- oslo_db/tests/sqlalchemy/test_utils.py | 41 ++++++++++++++++++++++---- 2 files changed, 60 insertions(+), 12 deletions(-) diff --git a/oslo_db/sqlalchemy/utils.py b/oslo_db/sqlalchemy/utils.py index b9996d4b..f5dda315 100644 --- a/oslo_db/sqlalchemy/utils.py +++ b/oslo_db/sqlalchemy/utils.py @@ -31,6 +31,7 @@ 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.')) diff --git a/oslo_db/tests/sqlalchemy/test_utils.py b/oslo_db/tests/sqlalchemy/test_utils.py index 9947b63d..e5f5a6cf 100644 --- a/oslo_db/tests/sqlalchemy/test_utils.py +++ b/oslo_db/tests/sqlalchemy/test_utils.py @@ -31,6 +31,7 @@ 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 @@ def test_multiple_primary_keys_stable(self): 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 @@ def test_unique_index(self): [{'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 @@ def indexes(self): 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):