From 3a40168056b85de0e53b69dc6ff52bddb4cdd7a7 Mon Sep 17 00:00:00 2001 From: Mehdi Abaakouk Date: Tue, 12 Sep 2017 11:52:35 +0200 Subject: Fix pagination when marker value is None The change b3869d04cff7071c1226758eb8b58fde9eba5b8d was fixing #1615938 only for some cases, where the unicity of the row was done by one column. This change fixes it for other cases. Change-Id: I4acb382c770b168f29fa35f02707fb22d402b13b Closes-bug: #1615938 --- oslo_db/sqlalchemy/utils.py | 14 +++++-- oslo_db/tests/sqlalchemy/test_utils.py | 68 ++++++++++++++++++++++++++++++++-- 2 files changed, 76 insertions(+), 6 deletions(-) diff --git a/oslo_db/sqlalchemy/utils.py b/oslo_db/sqlalchemy/utils.py index 4961a70..4bd8a0e 100644 --- a/oslo_db/sqlalchemy/utils.py +++ b/oslo_db/sqlalchemy/utils.py @@ -238,7 +238,8 @@ def paginate_query(query, model, limit, sort_keys, marker=None, if marker_values[i] is not None: for j in range(i): model_attr = getattr(model, sort_keys[j]) - crit_attrs.append((model_attr == marker_values[j])) + if marker_values[j] is not None: + crit_attrs.append((model_attr == marker_values[j])) model_attr = getattr(model, sort_keys[i]) val = marker_values[i] @@ -247,9 +248,16 @@ def paginate_query(query, model, limit, sort_keys, marker=None, val = int(val) model_attr = cast(model_attr, Integer) if sort_dirs[i].startswith('desc'): - crit_attrs.append((model_attr < val)) + crit_attr = (model_attr < val) + if sort_dirs[i].endswith('nullsfirst'): + crit_attr = sqlalchemy.sql.or_(crit_attr, + model_attr.is_(None)) else: - crit_attrs.append((model_attr > val)) + crit_attr = (model_attr > val) + if sort_dirs[i].endswith('nullslast'): + crit_attr = sqlalchemy.sql.or_(crit_attr, + model_attr.is_(None)) + crit_attrs.append(crit_attr) criteria = sqlalchemy.sql.and_(*crit_attrs) criteria_list.append(criteria) diff --git a/oslo_db/tests/sqlalchemy/test_utils.py b/oslo_db/tests/sqlalchemy/test_utils.py index ad26ee4..28dc9f2 100644 --- a/oslo_db/tests/sqlalchemy/test_utils.py +++ b/oslo_db/tests/sqlalchemy/test_utils.py @@ -290,9 +290,18 @@ class TestPaginateQuery(test_base.BaseTestCase): self.query.order_by('desc_1').AndReturn(self.query) self.mox.StubOutWithMock(sqlalchemy.sql, 'and_') - sqlalchemy.sql.and_(mock.ANY).AndReturn('some_crit') - sqlalchemy.sql.and_(mock.ANY, mock.ANY).AndReturn('another_crit') self.mox.StubOutWithMock(sqlalchemy.sql, 'or_') + self.mox.StubOutWithMock(self.model.user_id, 'is_') + + self.model.user_id.is_(None).AndReturn('desc_null_filter_1') + self.model.user_id.is_(None).AndReturn('desc_null_filter_2') + sqlalchemy.sql.or_(mock.ANY, 'desc_null_filter_2').AndReturn('or_1') + + self.model.project_id.is_(None).AndReturn('asc_null_filter') + sqlalchemy.sql.or_(mock.ANY, 'asc_null_filter').AndReturn('or_2') + + sqlalchemy.sql.and_('or_1').AndReturn('some_crit') + sqlalchemy.sql.and_(mock.ANY, 'or_2').AndReturn('another_crit') sqlalchemy.sql.or_('some_crit', 'another_crit').AndReturn('some_f') self.query.filter('some_f').AndReturn(self.query) self.query.limit(5).AndReturn(self.query) @@ -320,8 +329,14 @@ class TestPaginateQuery(test_base.BaseTestCase): self.query.order_by('desc_1').AndReturn(self.query) self.mox.StubOutWithMock(sqlalchemy.sql, 'and_') - sqlalchemy.sql.and_(mock.ANY).AndReturn('some_crit') self.mox.StubOutWithMock(sqlalchemy.sql, 'or_') + self.mox.StubOutWithMock(self.model.user_id, 'is_') + + self.model.user_id.is_(None).AndReturn('asc_null_filter_1') + self.model.user_id.is_(None).AndReturn('asc_null_filter_2') + sqlalchemy.sql.or_(mock.ANY, 'asc_null_filter_2').AndReturn('or_1') + + sqlalchemy.sql.and_('or_1').AndReturn('some_crit') sqlalchemy.sql.or_('some_crit').AndReturn('some_f') self.query.filter('some_f').AndReturn(self.query) self.query.limit(5).AndReturn(self.query) @@ -331,6 +346,53 @@ class TestPaginateQuery(test_base.BaseTestCase): marker=self.marker, sort_dirs=['asc-nullslast', 'desc-nullsfirst']) + def test_paginate_query_marker_null_with_two_primary_keys(self): + self.mox.StubOutWithMock(self.model.user_id, 'isnot') + self.model.user_id.isnot(None).AndReturn('asc_null_1') + sqlalchemy.desc('asc_null_1').AndReturn('asc_null_2') + self.query.order_by('asc_null_2').AndReturn(self.query) + + sqlalchemy.asc(self.model.user_id).AndReturn('asc_1') + self.query.order_by('asc_1').AndReturn(self.query) + + self.mox.StubOutWithMock(self.model.updated_at, 'is_') + self.model.updated_at.is_(None).AndReturn('desc_null_1') + sqlalchemy.desc('desc_null_1').AndReturn('desc_null_2') + self.query.order_by('desc_null_2').AndReturn(self.query) + + sqlalchemy.desc(self.model.updated_at).AndReturn('desc_1') + self.query.order_by('desc_1').AndReturn(self.query) + + self.mox.StubOutWithMock(self.model.project_id, 'is_') + self.model.project_id.is_(None).AndReturn('desc_null_3') + sqlalchemy.desc('desc_null_3').AndReturn('desc_null_4') + self.query.order_by('desc_null_4').AndReturn(self.query) + + sqlalchemy.desc(self.model.project_id).AndReturn('desc_4') + self.query.order_by('desc_4').AndReturn(self.query) + + self.mox.StubOutWithMock(sqlalchemy.sql, 'and_') + self.mox.StubOutWithMock(sqlalchemy.sql, 'or_') + self.mox.StubOutWithMock(self.model.user_id, 'is_') + + self.model.user_id.is_(None).AndReturn('asc_null_filter_1') + self.model.user_id.is_(None).AndReturn('asc_null_filter_2') + self.model.project_id.is_(None).AndReturn('desc_null_filter_3') + + sqlalchemy.sql.or_(mock.ANY, 'asc_null_filter_2').AndReturn('or_1') + sqlalchemy.sql.or_(mock.ANY, 'desc_null_filter_3').AndReturn('or_2') + sqlalchemy.sql.and_('or_1').AndReturn('some_crit') + sqlalchemy.sql.and_(mock.ANY, 'or_2').AndReturn('other_crit') + sqlalchemy.sql.or_('some_crit', 'other_crit').AndReturn('some_f') + self.query.filter('some_f').AndReturn(self.query) + self.query.limit(5).AndReturn(self.query) + self.mox.ReplayAll() + utils.paginate_query(self.query, self.model, 5, + ['user_id', 'updated_at', 'project_id'], + marker=self.marker, + sort_dirs=['asc-nullslast', 'desc-nullsfirst', + 'desc-nullsfirst']) + def test_paginate_query_value_error(self): sqlalchemy.asc(self.model.user_id).AndReturn('asc_1') self.query.order_by('asc_1').AndReturn(self.query) -- cgit v1.2.1