Fix marker checking when value is None
There are cases where users sort a table using compound-values sort_key
and one of the key has nullable set to True. For example, sorting a
table using ['id', 'updated_at'] where 'updated_at' can be None.
When marker_value is None, we cannot do value comparison using '<' or
'>' operators. This patch adds a check if the value from the marker
corresponding to the nullable-key has None value. If that is the case,
we skip the comparison.
Back to the example above, instead of always getting the following
criteria (which doesn't work):
(id > MARKER_ID) or (id == MARKER_ID && updated_at > None) <-- failure
we will get the following criteria when 'updated_at' is None:
(id > MARKER_ID)
This is not hurting in any way to existing / legal use cases where
callers are expected to include a unique key in sort keys. If there are
such cases, this patch is not making things worse because the sorting
is already unpredictable.
Closes-Bug: #1615938
Change-Id: Iea2cd0bb2556b0b15a0baaa76ef522a3097f9928
(cherry picked from commit b3869d04cf
)
This commit is contained in:
parent
3e2591cb5e
commit
3787363005
|
@ -146,7 +146,8 @@ def paginate_query(query, model, limit, sort_keys, marker=None,
|
|||
the lexicographical ordering:
|
||||
(k1 > X1) or (k1 == X1 && k2 > X2) or (k1 == X1 && k2 == X2 && k3 > X3)
|
||||
|
||||
We also have to cope with different sort_directions.
|
||||
We also have to cope with different sort_directions and cases where k2,
|
||||
k3, ... are nullable.
|
||||
|
||||
Typically, the id of the last row is used as the client-facing pagination
|
||||
marker, then the actual marker object must be fetched from the db and
|
||||
|
@ -223,18 +224,24 @@ def paginate_query(query, model, limit, sort_keys, marker=None,
|
|||
criteria_list = []
|
||||
for i in range(len(sort_keys)):
|
||||
crit_attrs = []
|
||||
for j in range(i):
|
||||
model_attr = getattr(model, sort_keys[j])
|
||||
crit_attrs.append((model_attr == marker_values[j]))
|
||||
# NOTE: We skip the marker value comparison if marker_values[i] is
|
||||
# None, for two reasons: 1) the comparison operators below
|
||||
# ('<', '>') are not applicable on None value; 2) this is
|
||||
# safe because we can assume the primary key is included in
|
||||
# sort_key, thus checked as (one of) marker values.
|
||||
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]))
|
||||
|
||||
model_attr = getattr(model, sort_keys[i])
|
||||
if sort_dirs[i].startswith('desc'):
|
||||
crit_attrs.append((model_attr < marker_values[i]))
|
||||
else:
|
||||
crit_attrs.append((model_attr > marker_values[i]))
|
||||
model_attr = getattr(model, sort_keys[i])
|
||||
if sort_dirs[i].startswith('desc'):
|
||||
crit_attrs.append((model_attr < marker_values[i]))
|
||||
else:
|
||||
crit_attrs.append((model_attr > marker_values[i]))
|
||||
|
||||
criteria = sqlalchemy.sql.and_(*crit_attrs)
|
||||
criteria_list.append(criteria)
|
||||
criteria = sqlalchemy.sql.and_(*crit_attrs)
|
||||
criteria_list.append(criteria)
|
||||
|
||||
f = sqlalchemy.sql.or_(*criteria_list)
|
||||
query = query.filter(f)
|
||||
|
|
|
@ -77,6 +77,7 @@ class FakeTable(Base):
|
|||
user_id = Column(String(50), primary_key=True)
|
||||
project_id = Column(String(50))
|
||||
snapshot_id = Column(String(50))
|
||||
updated_at = Column(DateTime, nullable=True)
|
||||
|
||||
# mox is comparing in some awkward way that
|
||||
# in this case requires the same identity of object
|
||||
|
@ -171,7 +172,8 @@ class TestPaginateQuery(test_base.BaseTestCase):
|
|||
self.mox.StubOutWithMock(sqlalchemy, 'desc')
|
||||
self.marker = FakeTable(user_id='user',
|
||||
project_id='p',
|
||||
snapshot_id='s')
|
||||
snapshot_id='s',
|
||||
updated_at=None)
|
||||
self.model = FakeTable
|
||||
|
||||
def test_paginate_query_no_pagination_no_sort_dirs(self):
|
||||
|
@ -301,6 +303,35 @@ class TestPaginateQuery(test_base.BaseTestCase):
|
|||
marker=self.marker,
|
||||
sort_dirs=['asc-nullslast', 'desc-nullsfirst'])
|
||||
|
||||
def test_paginate_query_marker_null(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(sqlalchemy.sql, 'and_')
|
||||
sqlalchemy.sql.and_(mock.ANY).AndReturn('some_crit')
|
||||
self.mox.StubOutWithMock(sqlalchemy.sql, 'or_')
|
||||
sqlalchemy.sql.or_('some_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'],
|
||||
marker=self.marker,
|
||||
sort_dirs=['asc-nullslast', '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)
|
||||
|
|
Loading…
Reference in New Issue