Merge pull request #862 from dresende/has_many_join_order

Add rows to hasMany join table in order provided

Author: dxg

Changelog.md

@@ -1,3 +1,6 @@
+### v8.0.0
+- [POSSIBLY BREAKING] Add rows to hasMany join table in order provided (previously, they were added in reverse order) ([862](../../pull/862))
+
 ### v7.1.1
 - Resolve most development-only security vulnerabilities (no production ones present) ([861](../../pull/861))
 - Don't run test on node 10 & 12 - dev deps don't work

lib/Associations/Many.js

@@ -1,4 +1,5 @@
 var _                   = require("lodash");
+var async               = require("async");
 var Hook                = require("../Hook");
 var Settings            = require("../Settings");
 var Property            = require("../Property");
@@ -369,17 +370,13 @@ function extendInstance(Model, Instance, Driver, association, opts, createInstan
     value: function () {
       var Associations = [];
       var opts = {};
-      var cb = noOperation;
+      var next = noOperation;
 
       var run = function () {
-        var savedAssociations = [];
-        var saveNextAssociation = function () {
-          if (Associations.length === 0) {
-            return cb(null, savedAssociations);
-          }
+        var saveAssociation = function (Association, cb) {
+          const hookOpts = Object.keys(association.props).length > 0 ? opts : undefined;
 
-          var Association = Associations.pop();
-          var saveAssociation = function (err) {
+          Hook.wait(Association, association.hooks.beforeSave, function (err) {
             if (err) {
               return cb(err);
             }
@@ -405,9 +402,7 @@ function extendInstance(Model, Instance, Driver, association, opts, createInstan
                     return cb(err);
                   }
 
-                  savedAssociations.push(Association);
-
-                  return saveNextAssociation();
+                  return cb();
                 });
               }
 
@@ -419,27 +414,24 @@ function extendInstance(Model, Instance, Driver, association, opts, createInstan
                   return cb(err);
                 }
 
-                savedAssociations.push(Association);
-
-                return saveNextAssociation();
+                return cb();
               });
             });
-          };
-
-          if (Object.keys(association.props).length) {
-            Hook.wait(Association, association.hooks.beforeSave, saveAssociation, opts);
-          } else {
-            Hook.wait(Association, association.hooks.beforeSave, saveAssociation);
-          }
+          }, hookOpts);
         };
 
-        return saveNextAssociation();
+        async.eachSeries(Associations, saveAssociation, function (err) {
+          if (err) {
+            return next(err);
+          }
+          next(null, Associations);
+        });
       };
 
       for (var i = 0; i < arguments.length; i++) {
         switch (typeof arguments[i]) {
           case "function":
-            cb = arguments[i];
+            next = arguments[i];
             break;
           case "object":
             if (Array.isArray(arguments[i])) {
@@ -462,7 +454,7 @@ function extendInstance(Model, Instance, Driver, association, opts, createInstan
       } else {
         this.save(function (err) {
           if (err) {
-            return cb(err);
+            return next(err);
           }
 
           return run();

lib/Associations/One.js

@@ -1,4 +1,5 @@
 var _          = require("lodash");
+var async      = require("async");
 var util       = require("../Utilities");
 var ORMError   = require("../Error");
 var Accessors  = { "get": "get", "set": "set", "has": "has", "del": "remove" };
@@ -214,52 +215,39 @@ function extendInstance(Model, Instance, Driver, association) {
     writable: true
   });
   Object.defineProperty(Instance, association.setAccessor, {
-    value: function (OtherInstance, cb) {
+    value: function (OtherInstance, next) {
       if (association.reversed) {
         Instance.save(function (err) {
           if (err) {
-            return cb(err);
+            return next(err);
           }
 
           if (!Array.isArray(OtherInstance)) {
             util.populateConditions(Model, Object.keys(association.field), Instance, OtherInstance, true);
 
-            return OtherInstance.save({}, { saveAssociations: false }, cb);
+            return OtherInstance.save({}, { saveAssociations: false }, next);
           }
 
-          var associations = _.clone(OtherInstance);
-
-          var saveNext = function () {
-            if (!associations.length) {
-              return cb();
-            }
-
-            var other = associations.pop();
+          var saveAssociation = function (otherInstance, cb) {
+            util.populateConditions(Model, Object.keys(association.field), Instance, otherInstance, true);
 
-            util.populateConditions(Model, Object.keys(association.field), Instance, other, true);
-
-            other.save({}, { saveAssociations: false }, function (err) {
-              if (err) {
-                return cb(err);
-              }
-
-              saveNext();
-            });
+            otherInstance.save({}, { saveAssociations: false }, cb);
           };
 
-          return saveNext();
+          var associations = _.clone(OtherInstance);
+          async.eachSeries(associations, saveAssociation, next);
         });
       } else {
         OtherInstance.save({}, { saveAssociations: false }, function (err) {
           if (err) {
-            return cb(err);
+            return next(err);
           }
 
           Instance[association.name] = OtherInstance;
 
           util.populateConditions(association.model, Object.keys(association.field), OtherInstance, Instance);
 
-          return Instance.save({}, { saveAssociations: false }, cb);
+          return Instance.save({}, { saveAssociations: false }, next);
         });
       }
 

lib/Hook.js

@@ -8,30 +8,30 @@ exports.trigger = function () {
   }
 };
 
-exports.wait = function () {
-  var args = Array.prototype.slice.apply(arguments);
-  var self = args.shift();
-  var hook = args.shift();
-  var next = args.shift();
+exports.wait = function (self, hook, next, opts) {
+  if (typeof hook !== "function") {
+    return next();
+  }
 
-  args.push(next);
-  if (typeof hook === "function") {
-    var hookValue = hook.apply(self, args);
+  var hookDoesntExpectCallback = hook.length < 1;
+  var hookValue;
+  if (opts) {
+    hookValue = hook.call(self, opts, next);
+    hookDoesntExpectCallback = hook.length < 2;
+  } else {
+    hookValue = hook.call(self, next);
+  }
 
-    var hookDoesntExpectCallback = hook.length < args.length;
-    var isPromise = hookValue && typeof(hookValue.then) === "function";
+  var isPromise = hookValue && typeof(hookValue.then) === "function";
 
-    if (hookDoesntExpectCallback) {
-      if (isPromise) {
-        return hookValue
-          .then(function () {
-            next();
-          })
-          .catch(next);
-      }
-      return next();
+  if (hookDoesntExpectCallback) {
+    if (isPromise) {
+      return hookValue
+        .then(function () {
+          next();
+        })
+        .catch(next);
     }
-  } else {
     return next();
   }
 };

package-lock.json

@@ -12,7 +12,7 @@
     "sqlite",
     "mongodb"
   ],
-  "version": "7.1.1",
+  "version": "8.0.0",
   "license": "MIT",
   "homepage": "http://dresende.github.io/node-orm2",
   "repository": "http://github.com/dresende/node-orm2.git",

package.json

@@ -513,6 +513,24 @@ describe("hasMany", function () {
         });
       });
 
+      it("should add rows to join tables in order provided", async function () {
+        const pets = await Pet.createAsync([{ name: 'Fluffy' }, { name: 'Quacky' }, { name: 'Horsey' }]);
+        const Justin = await Person.oneAsync({ name: "Justin" });
+        const justinsPetsBefore = await Justin.getPetsAsync();
+        await Justin.addPetsAsync(pets);
+        const justinsPetsAfter = await Justin.getPetsAsync();
+
+        should.equal(justinsPetsBefore.length, justinsPetsAfter.length - 3);
+
+        const joinRows = await db.driver.execQueryAsync("SELECT * FROM person_pets");
+
+        should.deepEqual(
+          // Mysql returns array of RowDataPacket. Concert to plain object.
+          joinRows.slice(-pets.length).map((item) => Object.assign({}, item)),
+          pets.map(function (pet) { return { person_id: Justin.id, pets_id: pet.id }; }),
+        );
+      });
+
       it("should throw if no items passed", function (done) {
         Person.one(function (err, person) {
           should.equal(err, null);