fix(orderBy): ensure correct ordering with arrays of objects and no predicate

By refactoring to use a Schwartzian transform, we can ensure that objects
with no custom `toString` or `toValue` methods are just ordered using
their position in the original collection.

Closes #11866
Closes #11312
Closes #4282

Author: petebacondarwin

src/ng/filter/orderBy.js

@@ -176,88 +176,114 @@
 orderByFilter.$inject = ['$parse'];
 function orderByFilter($parse) {
   return function(array, sortPredicate, reverseOrder) {
+
     if (!(isArrayLike(array))) return array;
-    sortPredicate = isArray(sortPredicate) ? sortPredicate : [sortPredicate];
+
+    if (!isArray(sortPredicate)) { sortPredicate = [sortPredicate]; }
     if (sortPredicate.length === 0) { sortPredicate = ['+']; }
-    sortPredicate = sortPredicate.map(function(predicate) {
-      var descending = false, get = predicate || identity;
-      if (isString(predicate)) {
+
+    var predicates = processPredicates(sortPredicate, reverseOrder);
+
+    // The next three lines are a version of a Swartzian Transform idiom from Perl
+    // (sometimes called the Decorate-Sort-Undecorate idiom)
+    // See https://en.wikipedia.org/wiki/Schwartzian_transform
+    var compareValues = Array.prototype.map.call(array, getComparisonObject);
+    compareValues.sort(doComparison);
+    array = compareValues.map(function(item) { return item.value; });
+
+    return array;
+
+    function getComparisonObject(value, index) {
+      return {
+        value: value,
+        predicateValues: predicates.map(function(predicate) {
+          return getPredicateValue(predicate.get(value), index);
+        })
+      };
+    }
+
+    function doComparison(v1, v2) {
+      var result = 0;
+      for (var index=0, length = predicates.length; index < length; ++index) {
+        result = compare(v1.predicateValues[index], v2.predicateValues[index]) * predicates[index].descending;
+        if (result) break;
+      }
+      return result;
+    }
+  };
+
+  function processPredicates(sortPredicate, reverseOrder) {
+    reverseOrder = reverseOrder ? -1 : 1;
+    return sortPredicate.map(function(predicate) {
+      var descending = 1, get = identity;
+
+      if (isFunction(predicate)) {
+        get = predicate;
+      } else if (isString(predicate)) {
         if ((predicate.charAt(0) == '+' || predicate.charAt(0) == '-')) {
-          descending = predicate.charAt(0) == '-';
+          descending = predicate.charAt(0) == '-' ? -1 : 1;
           predicate = predicate.substring(1);
         }
-        if (predicate === '') {
-          // Effectively no predicate was passed so we compare identity
-          return reverseComparator(compare, descending);
-        }
-        get = $parse(predicate);
-        if (get.constant) {
-          var key = get();
-          return reverseComparator(function(a, b) {
-            return compare(a[key], b[key]);
-          }, descending);
+        if (predicate !== '') {
+          get = $parse(predicate);
+          if (get.constant) {
+            var key = get();
+            get = function(value) { return value[key]; };
+          }
         }
       }
-      return reverseComparator(function(a, b) {
-        return compare(get(a),get(b));
-      }, descending);
+      return { get: get, descending: descending * reverseOrder };
     });
-    return slice.call(array).sort(reverseComparator(comparator, reverseOrder));
+  }
 
-    function comparator(o1, o2) {
-      for (var i = 0; i < sortPredicate.length; i++) {
-        var comp = sortPredicate[i](o1, o2);
-        if (comp !== 0) return comp;
-      }
-      return 0;
-    }
-    function reverseComparator(comp, descending) {
-      return descending
-          ? function(a, b) {return comp(b,a);}
-          : comp;
+  function isPrimitive(value) {
+    switch (typeof value) {
+      case 'number': /* falls through */
+      case 'boolean': /* falls through */
+      case 'string':
+        return true;
+      default:
+        return false;
     }
+  }
 
-    function isPrimitive(value) {
-      switch (typeof value) {
-        case 'number': /* falls through */
-        case 'boolean': /* falls through */
-        case 'string':
-          return true;
-        default:
-          return false;
-      }
+  function objectValue(value, index) {
+    // If `valueOf` is a valid function use that
+    if (typeof value.valueOf === 'function') {
+      value = value.valueOf();
+      if (isPrimitive(value)) return value;
+    }
+    // If `toString` is a valid function and not the one from `Object.prototype` use that
+    if (hasCustomToString(value)) {
+      value = value.toString();
+      if (isPrimitive(value)) return value;
     }
+    // We have a basic object so we use the position of the object in the collection
+    return index;
+  }
 
-    function objectToString(value) {
-      if (value === null) return 'null';
-      if (typeof value.valueOf === 'function') {
-        value = value.valueOf();
-        if (isPrimitive(value)) return value;
-      }
-      if (typeof value.toString === 'function') {
-        value = value.toString();
-        if (isPrimitive(value)) return value;
-      }
-      return '';
+  function getPredicateValue(value, index) {
+    var type = typeof value;
+    if (value === null) {
+      type = 'string';
+      value = 'null';
+    } else if (type === 'string') {
+      value = value.toLowerCase();
+    } else if (type === 'object') {
+      value = objectValue(value, index);
     }
+    return { value: value, type: type };
+  }
 
-    function compare(v1, v2) {
-      var t1 = typeof v1;
-      var t2 = typeof v2;
-      if (t1 === t2 && t1 === "object") {
-        v1 = objectToString(v1);
-        v2 = objectToString(v2);
-      }
-      if (t1 === t2) {
-        if (t1 === "string") {
-           v1 = v1.toLowerCase();
-           v2 = v2.toLowerCase();
-        }
-        if (v1 === v2) return 0;
-        return v1 < v2 ? -1 : 1;
-      } else {
-        return t1 < t2 ? -1 : 1;
+  function compare(v1, v2) {
+    var result = 0;
+    if (v1.type === v2.type) {
+      if (v1.value !== v2.value) {
+        result = v1.value < v2.value ? -1 : 1;
       }
+    } else {
+      result = v1.type < v2.type ? -1 : 1;
     }
-  };
+    return result;
+  }
 }

test/ng/filter/orderBySpec.js

@@ -23,7 +23,7 @@ describe('Filter: orderBy', function() {
     });
 
 
-    it('shouldSortArrayInReverse', function() {
+    it('should reverse collection if `reverseOrder` param is truthy', function() {
       expect(orderBy([{a:15}, {a:2}], 'a', true)).toEqualData([{a:15}, {a:2}]);
       expect(orderBy([{a:15}, {a:2}], 'a', "T")).toEqualData([{a:15}, {a:2}]);
       expect(orderBy([{a:15}, {a:2}], 'a', "reverse")).toEqualData([{a:15}, {a:2}]);
@@ -116,7 +116,7 @@ describe('Filter: orderBy', function() {
     });
 
 
-    it('should not reverse array of objects with no predicate', function() {
+    it('should not reverse array of objects with no predicate and reverse is not `true`', function() {
       var array = [
         { id: 2 },
         { id: 1 },
@@ -126,6 +126,39 @@ describe('Filter: orderBy', function() {
       expect(orderBy(array)).toEqualData(array);
     });
 
+    it('should reverse array of objects with no predicate and reverse is `true`', function() {
+      var array = [
+        { id: 2 },
+        { id: 1 },
+        { id: 4 },
+        { id: 3 }
+      ];
+      var reversedArray = [
+        { id: 3 },
+        { id: 4 },
+        { id: 1 },
+        { id: 2 }
+      ];
+      expect(orderBy(array, '', true)).toEqualData(reversedArray);
+    });
+
+
+    it('should reverse array of objects with predicate of "-"', function() {
+      var array = [
+        { id: 2 },
+        { id: 1 },
+        { id: 4 },
+        { id: 3 }
+      ];
+      var reversedArray = [
+        { id: 3 },
+        { id: 4 },
+        { id: 1 },
+        { id: 2 }
+      ];
+      expect(orderBy(array, '-')).toEqualData(reversedArray);
+    });
+
 
     it('should not reverse array of objects with null prototype and no predicate', function() {
       var array = [2,1,4,3].map(function(id) {
@@ -151,6 +184,16 @@ describe('Filter: orderBy', function() {
         null
       ]);
     });
+
+
+    it('should sort array of arrays as Array.prototype.sort', function() {
+      expect(orderBy([['one'], ['two'], ['three']])).toEqualData([['one'], ['three'], ['two']]);
+    });
+
+
+    it('should sort mixed array of objects and values in a stable way', function() {
+      expect(orderBy([{foo: 2}, {foo: {}}, {foo: 3}, {foo: 4}], 'foo')).toEqualData([{foo: 2}, {foo: 3}, {foo: 4}, {foo: {}}]);
+    });
   });