From f6a2ab7b2f569e29ddf3fd037da2f3df88d1f181 Mon Sep 17 00:00:00 2001 From: Mehdi Abaakouk Date: Wed, 5 Aug 2015 15:01:22 +0200 Subject: [PATCH] pagination: enhance sorting of null values Sort results on nullable column depends on the used backend. Mysql put null after sorted result, while Postgresql do the reverse. This change allows to pass the expected behavior via the sort direction. eg: desc-nullslast, put null values after the sorted result with the value set. Change-Id: Ic637311d84790251e7a3babd290a4da36cbaf73c Closes-bug: #1481749 --- oslo_db/sqlalchemy/utils.py | 39 ++++++++++++++++++-------- oslo_db/tests/sqlalchemy/test_utils.py | 35 +++++++++++++++++++++++ 2 files changed, 63 insertions(+), 11 deletions(-) diff --git a/oslo_db/sqlalchemy/utils.py b/oslo_db/sqlalchemy/utils.py index 7e185435..4f6a0721 100644 --- a/oslo_db/sqlalchemy/utils.py +++ b/oslo_db/sqlalchemy/utils.py @@ -18,6 +18,7 @@ import collections import contextlib +import itertools import logging import re @@ -53,6 +54,10 @@ LOG = logging.getLogger(__name__) _DBURL_REGEX = re.compile(r"[^:]+://([^:]+):([^@]+)@.+") +_VALID_SORT_DIR = [ + "-".join(x) for x in itertools.product(["asc", "desc"], + ["nullsfirst", "nullslast"])] + def sanitize_db_url(url): match = _DBURL_REGEX.match(url) @@ -88,6 +93,8 @@ def paginate_query(query, model, limit, sort_keys, marker=None, :param marker: the last item of the previous page; we returns the next results after this value. :param sort_dir: direction in which results should be sorted (asc, desc) + suffix -nullsfirst, -nullslast can be added to defined + the ordering of null values :param sort_dirs: per-column array of sort_dirs, corresponding to sort_keys :rtype: sqlalchemy.orm.query.Query @@ -114,21 +121,31 @@ def paginate_query(query, model, limit, sort_keys, marker=None, # Add sorting for current_sort_key, current_sort_dir in zip(sort_keys, sort_dirs): try: - sort_dir_func = { - 'asc': sqlalchemy.asc, - 'desc': sqlalchemy.desc, - }[current_sort_dir] - except KeyError: - raise ValueError(_("Unknown sort direction, " - "must be 'desc' or 'asc'")) - try: - inspect(model).\ - all_orm_descriptors[current_sort_key] + inspect(model).all_orm_descriptors[current_sort_key] except KeyError: raise exception.InvalidSortKey() else: sort_key_attr = getattr(model, current_sort_key) + try: + main_sort_dir, __, null_sort_dir = current_sort_dir.partition("-") + sort_dir_func = { + 'asc': sqlalchemy.asc, + 'desc': sqlalchemy.desc, + }[main_sort_dir] + + null_order_by_stmt = { + "": None, + "nullsfirst": sort_key_attr.is_(None), + "nullslast": sort_key_attr.isnot(None), + }[null_sort_dir] + except KeyError: + raise ValueError(_("Unknown sort direction, " + "must be one of: %s") % + ", ".join(_VALID_SORT_DIR)) + + if null_order_by_stmt is not None: + query = query.order_by(sqlalchemy.desc(null_order_by_stmt)) query = query.order_by(sort_dir_func(sort_key_attr)) # Add pagination @@ -147,7 +164,7 @@ def paginate_query(query, model, limit, sort_keys, marker=None, crit_attrs.append((model_attr == marker_values[j])) model_attr = getattr(model, sort_keys[i]) - if sort_dirs[i] == 'desc': + if sort_dirs[i].startswith('desc'): crit_attrs.append((model_attr < marker_values[i])) else: crit_attrs.append((model_attr > marker_values[i])) diff --git a/oslo_db/tests/sqlalchemy/test_utils.py b/oslo_db/tests/sqlalchemy/test_utils.py index 054f99d2..b7a00fba 100644 --- a/oslo_db/tests/sqlalchemy/test_utils.py +++ b/oslo_db/tests/sqlalchemy/test_utils.py @@ -166,6 +166,11 @@ class TestPaginateQuery(test_base.BaseTestCase): utils.paginate_query, self.query, self.model, 5, ['foo']) + def test_paginate_query_attribute_error_invalid_sortkey_3(self): + self.assertRaises(exception.InvalidSortKey, + utils.paginate_query, self.query, + self.model, 5, ['asc-nullinvalid']) + def test_paginate_query_assertion_error(self): self.mox.ReplayAll() self.assertRaises(AssertionError, @@ -200,6 +205,36 @@ class TestPaginateQuery(test_base.BaseTestCase): marker=self.marker, sort_dirs=['asc', 'desc']) + def test_paginate_query_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.project_id, 'is_') + self.model.project_id.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.project_id).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') + sqlalchemy.sql.and_(mock.ANY, mock.ANY).AndReturn('another_crit') + self.mox.StubOutWithMock(sqlalchemy.sql, 'or_') + 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) + self.mox.ReplayAll() + utils.paginate_query(self.query, self.model, 5, + ['user_id', 'project_id'], + 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)