datastore: concat transactional mutations (#1341)

* datastore: concat transactional mutations

* extract the array flattening bit

* fix method ordering & system test

lib/datastore/transaction.js

@@ -21,8 +21,9 @@
 'use strict';
 
 var arrify = require('arrify');
-var extend = require('extend');
+var flatten = require('lodash.flatten');
 var nodeutil = require('util');
+var prop = require('propprop');
 
 /**
  * @type {module:datastore/request}
@@ -375,9 +376,11 @@ Transaction.prototype.commit_ = function(callback) {
       }
     })
 
-    // Group entities together by action (delete or save).
+    // Group entities together by method: `save` mutations, then `delete`. Note:
+    // `save` mutations being first is required to maintain order when assigning
+    // IDs to incomplete keys.
     .sort(function(a, b) {
-      return a.method > b.method ? 1 : a.method < b.method ? -1 : 0;
+      return a.method < b.method ? 1 : a.method > b.method ? -1 : 0;
     })
 
     // Group arguments together so that we only make one call to each method.
@@ -418,10 +421,9 @@ Transaction.prototype.commit_ = function(callback) {
 
   // Take the `req` array built previously, and merge them into one request to
   // send as the final transactional commit.
-  var reqOpts = this.requests_.reduce(function(acc, req) {
-    return extend(true, acc, req);
-  }, {});
-
+  var reqOpts = {
+    mutations: flatten(this.requests_.map(prop('mutations')))
+  };
 
   this.request_(protoOpts, reqOpts, function(err, resp) {
     if (err) {

package.json

@@ -106,6 +106,7 @@
     "grpc": "^0.14.1",
     "hash-stream-validation": "^0.1.0",
     "is": "^3.0.1",
+    "lodash.flatten": "^4.2.0",
     "methmeth": "^1.0.0",
     "mime-types": "^2.0.8",
     "modelo": "^4.2.0",

system-test/datastore.js

@@ -690,48 +690,52 @@ describe('Datastore', function() {
       var key = datastore.key(['Company', 'Google']);
       var incompleteKey = datastore.key('Company');
 
-      datastore.runInTransaction(function(t, tDone) {
-        t.delete(deleteKey);
-
-        t.save([
-          {
-            key: key,
-            data: { rating: 10 }
-          },
-          {
-            key: incompleteKey,
-            data: { rating: 100 }
-          }
-        ]);
-
-        tDone();
+      datastore.save({
+        key: deleteKey,
+        data: {}
       }, function(err) {
         assert.ifError(err);
 
-        // Incomplete key should have been given an ID.
-        assert.strictEqual(incompleteKey.path.length, 2);
-
-        async.parallel([
-          // The key queued for deletion should have been deleted.
-          function(done) {
-            datastore.get(deleteKey, function(err, entity) {
-              assert.ifError(err);
-              assert.strictEqual(typeof entity, 'undefined');
-              done();
-            });
-          },
+        datastore.runInTransaction(function(t, tDone) {
+          t.delete(deleteKey);
+
+          t.save([
+            {
+              key: key,
+              data: { rating: 10 }
+            },
+            {
+              key: incompleteKey,
+              data: { rating: 100 }
+            }
+          ]);
 
-          // Data should have been updated on the key.
-          function(done) {
-            datastore.get(key, function(err, entity) {
-              assert.ifError(err);
-              assert.strictEqual(entity.data.rating, 10);
-              done();
-            });
-          }
-        ], function(err) {
+          tDone();
+        }, function(err) {
           assert.ifError(err);
-          datastore.delete([key, incompleteKey], done);
+
+          // Incomplete key should have been given an ID.
+          assert.strictEqual(incompleteKey.path.length, 2);
+
+          async.parallel([
+            // The key queued for deletion should have been deleted.
+            function(callback) {
+              datastore.get(deleteKey, function(err, entity) {
+                assert.ifError(err);
+                assert.strictEqual(typeof entity, 'undefined');
+                callback();
+              });
+            },
+
+            // Data should have been updated on the key.
+            function(callback) {
+              datastore.get(key, function(err, entity) {
+                assert.ifError(err);
+                assert.strictEqual(entity.data.rating, 10);
+                callback();
+              });
+            }
+          ], done);
         });
       });
     });

test/datastore/transaction.js

@@ -19,7 +19,6 @@
 var arrify = require('arrify');
 var assert = require('assert');
 var entity = require('../../lib/datastore/entity.js');
-var extend = require('extend');
 var mockery = require('mockery-next');
 var util = require('../../lib/common/util.js');
 
@@ -334,9 +333,11 @@ describe('Transaction', function() {
       assert.equal(saveCalled, 1);
 
       assert.equal(args.length, 2);
+
+      // Save arguments must come first.
       assert.deepEqual(args, [
-        [deleteArg1, deleteArg2],
-        [saveArg1, saveArg2]
+        [saveArg1, saveArg2],
+        [deleteArg1, deleteArg2]
       ]);
     });
 
@@ -364,14 +365,29 @@ describe('Transaction', function() {
 
     it('should send the built request object', function(done) {
       transaction.requests_ = [
-        { a: 'b', c: 'd' },
-        { e: 'f', g: 'h' }
+        {
+          mutations: [
+            { a: 'b' },
+            { c: 'd' }
+          ]
+        },
+        {
+          mutations: [
+            { e: 'f' },
+            { g: 'h' }
+          ]
+        }
       ];
 
       transaction.request_ = function(protoOpts, reqOpts) {
-        var req1 = transaction.requests_[0];
-        var req2 = transaction.requests_[1];
-        assert.deepEqual(reqOpts, extend(req1, req2));
+        assert.deepEqual(reqOpts, {
+          mutations: [
+            { a: 'b' },
+            { c: 'd' },
+            { e: 'f' },
+            { g: 'h' }
+          ]
+        });
         done();
       };