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

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)