Merge pull request #996 from strongloop/fix/order-in-find

Ensure stable order of items in DAO.find()

lib/dao.js

@@ -1678,26 +1678,22 @@ DataAccessObject.find = function find(query, options, cb) {
   }
 
   var allCb = function(err, data) {
-    var results = [];
     if (!err && Array.isArray(data)) {
-      async.each(data, function(item, callback) {
-        var d = item;//data[i];
-        var Model = self.lookupModel(d);
+      async.map(data, function(item, next) {
+        var Model = self.lookupModel(item);
         if (options.notify === false) {
-          buildResult(d);
+          buildResult(item, next);
         } else {
-          withNotify();
+          withNotify(item, next);
         }
 
-        function buildResult(data) {
-          d = data;
-
+        function buildResult(data, callback) {
           var ctorOpts = {
             fields: query.fields,
             applySetters: false,
             persisted: true,
           };
-          var obj = new Model(d, ctorOpts);
+          var obj = new Model(data, ctorOpts);
 
           if (query && query.include) {
             if (query.collect) {
@@ -1730,18 +1726,13 @@ DataAccessObject.find = function find(query, options, cb) {
             }
           }
 
-          if (obj !== undefined) {
-            results.push(obj);
-            callback();
-          } else {
-            callback();
-          }
+          callback(null, obj);
         }
 
-        function withNotify() {
-          context = {
+        function withNotify(data, callback) {
+          var context = {
             Model: Model,
-            data: d,
+            data: data,
             isNewInstance: false,
             hookState: hookState,
             options: options,
@@ -1749,13 +1740,19 @@ DataAccessObject.find = function find(query, options, cb) {
 
           Model.notifyObserversOf('loaded', context, function(err) {
             if (err) return callback(err);
-            buildResult(context.data);
+            buildResult(context.data, callback);
           });
         }
       },
-      function(err) {
+      function(err, results) {
         if (err) return cb(err);
 
+        // When applying query.collect, some root items may not have
+        // any related/linked item. We store `undefined` in the results
+        // array in such case, which is not desirable from API consumer's
+        // point of view.
+        results = results.filter(isDefined);
+
         if (data && data.countBeforeLimit) {
           results.countBeforeLimit = data.countBeforeLimit;
         }
@@ -1794,6 +1791,10 @@ DataAccessObject.find = function find(query, options, cb) {
   return cb.promise;
 };
 
+function isDefined(value) {
+  return value !== undefined;
+}
+
 /**
  * Find one record, same as `find`, but limited to one result. This function returns an object, not a collection.
  *

test/basic-querying.test.js

@@ -129,6 +129,14 @@ describe('basic-querying', function() {
 
     before(seed);
 
+    before(function setupDelayingLoadedHook() {
+      User.observe('loaded', nextAfterDelay);
+    });
+
+    after(function removeDelayingLoadHook() {
+      User.removeObserver('loaded', nextAfterDelay);
+    });
+
     it('should query collection', function(done) {
       User.find(function(err, users) {
         should.exists(users);
@@ -220,6 +228,18 @@ describe('basic-querying', function() {
       });
     });
 
+    it('should query sorted desc by order integer field even though there' +
+        'is an async model loaded hook', function(done) {
+      User.find({ order: 'order DESC' }, function(err, users) {
+        if (err) return done(err);
+
+        should.exists(users);
+        var order = users.map(function(u) { return u.order; });
+        order.should.eql([6, 5, 4, 3, 2, 1]);
+        done();
+      });
+    });
+
     it('should support "and" operator that is satisfied', function(done) {
       User.find({ where: { and: [
         { name: 'John Lennon' },
@@ -826,3 +846,8 @@ function seed(done) {
     },
   ], done);
 }
+
+function nextAfterDelay(ctx, next) {
+  var randomTimeoutTrigger = Math.floor(Math.random() * 100);
+  setTimeout(function() { process.nextTick(next); }, randomTimeoutTrigger);
+}