Merge "Handle null values when sorting"

3 files changed, 80 insertions, 7 deletions

diff --git a/cliff/lister.py b/cliff/lister.py
index cfba233..dde0f2d 100644
--- a/cliff/lister.py
+++ b/cliff/lister.py
@@ -13,7 +13,7 @@
 """Application base class for providing a list of data as output.
 """
 import abc
-import operator
+import logging
 
 from . import display
 
@@ -22,6 +22,8 @@ class Lister(display.DisplayCommandBase, metaclass=abc.ABCMeta):
     """Command base class for providing a list of data as output.
     """
 
+    log = logging.getLogger(__name__)
+
     @property
     def formatter_namespace(self):
         return 'cliff.formatter.list'
@@ -63,8 +65,25 @@ class Lister(display.DisplayCommandBase, metaclass=abc.ABCMeta):
         if parsed_args.sort_columns and self.need_sort_by_cliff:
             indexes = [column_names.index(c) for c in parsed_args.sort_columns
                        if c in column_names]
-            if indexes:
-                data = sorted(data, key=operator.itemgetter(*indexes))
+            for index in indexes[::-1]:
+                try:
+                    # We need to handle unset values (i.e. None) so we sort on
+                    # multiple conditions: the first comparing the results of
+                    # an 'is None' type check and the second comparing the
+                    # actual value. The second condition will only be checked
+                    # if the first returns True, which only happens if the
+                    # returns from the 'is None' check on the two values are
+                    # the same, i.e. both None or both not-None
+                    data = sorted(
+                        data, key=lambda k: (k[index] is None, k[index]),
+                    )
+                except TypeError:
+                    # Simply log and then ignore this; sorting is best effort
+                    self.log.warning(
+                        "Could not sort on field '%s'; unsortable types",
+                        parsed_args.sort_columns[index],
+                    )
+
         (columns_to_include, selector) = self._generate_columns_and_selector(
             parsed_args, column_names)
         if selector:
diff --git a/cliff/tests/test_lister.py b/cliff/tests/test_lister.py
index 8603004..7fc7222 100644
--- a/cliff/tests/test_lister.py
+++ b/cliff/tests/test_lister.py
@@ -32,16 +32,15 @@ class FauxFormatter(object):
 
 class ExerciseLister(lister.Lister):
 
+    data = [('a', 'A'), ('b', 'B'), ('c', 'A')]
+
     def _load_formatter_plugins(self):
         return {
             'test': FauxFormatter(),
         }
 
     def take_action(self, parsed_args):
-        return (
-            parsed_args.columns,
-            [('a', 'A'), ('b', 'B'), ('c', 'A')],
-        )
+        return (parsed_args.columns, self.data)
 
 
 class ExerciseListerCustomSort(ExerciseLister):
@@ -49,6 +48,16 @@ class ExerciseListerCustomSort(ExerciseLister):
     need_sort_by_cliff = False
 
 
+class ExerciseListerNullValues(ExerciseLister):
+
+    data = ExerciseLister.data + [(None, None)]
+
+
+class ExerciseListerDifferentTypes(ExerciseLister):
+
+    data = ExerciseLister.data + [(1, 0)]
+
+
 class TestLister(base.TestBase):
 
     def test_formatter_args(self):
@@ -111,6 +120,43 @@ class TestLister(base.TestBase):
         data = list(args[1])
         self.assertEqual([['a', 'A'], ['b', 'B'], ['c', 'A']], data)
 
+    def test_sort_by_column_with_null(self):
+        test_lister = ExerciseListerNullValues(mock.Mock(), [])
+        parsed_args = mock.Mock()
+        parsed_args.columns = ('Col1', 'Col2')
+        parsed_args.formatter = 'test'
+        parsed_args.sort_columns = ['Col2', 'Col1']
+
+        test_lister.run(parsed_args)
+
+        f = test_lister._formatter_plugins['test']
+        args = f.args[0]
+        data = list(args[1])
+        self.assertEqual(
+            [['a', 'A'], ['c', 'A'], ['b', 'B'], [None, None]], data)
+
+    def test_sort_by_column_with_different_types(self):
+        test_lister = ExerciseListerDifferentTypes(mock.Mock(), [])
+        parsed_args = mock.Mock()
+        parsed_args.columns = ('Col1', 'Col2')
+        parsed_args.formatter = 'test'
+        parsed_args.sort_columns = ['Col2', 'Col1']
+
+        with mock.patch.object(lister.Lister, 'log') as mock_log:
+            test_lister.run(parsed_args)
+
+        f = test_lister._formatter_plugins['test']
+        args = f.args[0]
+        data = list(args[1])
+        # The output should be unchanged
+        self.assertEqual(
+            [['a', 'A'], ['b', 'B'], ['c', 'A'], [1, 0]], data)
+        # but we should have logged a warning
+        mock_log.warning.assert_has_calls([
+            mock.call("Could not sort on field '%s'; unsortable types", col)
+            for col in parsed_args.sort_columns
+        ])
+
     def test_sort_by_non_displayed_column(self):
         test_lister = ExerciseLister(mock.Mock(), [])
         parsed_args = mock.Mock()
diff --git a/releasenotes/notes/handle-none-values-when-sorting-de40e36c66ad95ca.yaml b/releasenotes/notes/handle-none-values-when-sorting-de40e36c66ad95ca.yaml
new file mode 100644
index 0000000..a7368c1
--- /dev/null
+++ b/releasenotes/notes/handle-none-values-when-sorting-de40e36c66ad95ca.yaml
@@ -0,0 +1,8 @@
+---
+fixes:
+  - |
+    Sorting output using the ``--sort-column`` option will now handle ``None``
+    values. This was supported implicitly in Python 2 but was broken in the
+    move to Python 3. In addition, requests to sort a column containing
+    non-comparable types will now be ignored. Previously, these request would
+    result in a ``TypeError``.