Merge pull request #2917 from mbroadst/return-values-tests

refactor support for return values on insert/update

lib/dialects/abstract/index.js

@@ -9,12 +9,13 @@ AbstractDialect.prototype.supports = {
   'DEFAULT VALUES': false,
   'VALUES ()': false,
   'LIMIT ON UPDATE': false,
+  'ON DUPLICATE KEY': true,
 
-  /* does the dialect support returning values for inserted/updated fields using RETURNING */
-  'RETURNING': false,
+  /* What is the dialect's keyword for INSERT IGNORE */
+  'IGNORE': '',
 
-  /* does the dialect support returning values for inserted/updated fields using OUTPUT */
-  'OUTPUT': false,
+  /* does the dialect support returning values for inserted/updated fields */
+  returnValues: false,
 
   /* features specific to autoIncrement values */
   autoIncrement: {
@@ -27,10 +28,7 @@ AbstractDialect.prototype.supports = {
     /* does the dialect support updating autoincrement fields */
     update: true
   },
-  'ON DUPLICATE KEY': true,
 
-  /* What is the dialect's keyword for INSERT IGNORE */
-  'IGNORE': '',
   schemas: false,
   transactions: true,
   migrations: true,

lib/dialects/abstract/query-generator.js

@@ -190,14 +190,14 @@ module.exports = (function() {
         emptyQuery += ' VALUES ()';
       }
 
-      // FIXME: ideally these two can be merged in the future, the only
-      //        difference is placement of the value in the query
-      if (this._dialect.supports['RETURNING'] && options.returning) {
+      if (this._dialect.supports.returnValues && options.returning) {
+        if (!!this._dialect.supports.returnValues.returning) {
           valueQuery += ' RETURNING *';
           emptyQuery += ' RETURNING *';
-      } else if (this._dialect.supports['OUTPUT'] && options.returning) {
+        } else if (!!this._dialect.supports.returnValues.output) {
           outputFragment = ' OUTPUT INSERTED.*';
         }
+      }
 
       if (this._dialect.supports['EXCEPTION'] && options.exception) {
         // Mostly for internal use, so we expect the user to know what he's doing!
@@ -303,10 +303,13 @@ module.exports = (function() {
         query += ' LIMIT ' + this.escape(options.limit) + ' ';
       }
 
-      if (this._dialect.supports['RETURNING'] && options.returning) {
-        query += ' RETURNING *';
-      } else if (this._dialect.supports['OUTPUT']) {
+      if (this._dialect.supports.returnValues) {
+        if (!!this._dialect.supports.returnValues.output) {
+          // we always need this for mssql
           outputFragment = ' OUTPUT INSERTED.*';
+        } else if (this._dialect.supports.returnValues && options.returning) {
+          query += ' RETURNING *';
+        }
       }
 
       if (attributes) {
@@ -388,11 +391,16 @@ module.exports = (function() {
       var query
         , key
         , value
-        , values = [];
+        , values = []
+        , outputFragment;
 
-      query = 'UPDATE <%= table %> SET <%= values %> WHERE <%= where %>';
-      if (this._dialect.supports['RETURNING']) {
+      query = 'UPDATE <%= table %> SET <%= values %><%= output %> WHERE <%= where %>';
+      if (this._dialect.supports.returnValues) {
+        if (!!this._dialect.supports.returnValues.returning) {
           query += ' RETURNING *';
+        } else if (!!this._dialect.supports.returnValues.output) {
+          outputFragment = ' OUTPUT INSERTED.*';
+        }
       }
 
       for (key in attrValueHash) {
@@ -409,6 +417,7 @@ module.exports = (function() {
       var replacements = {
         table: this.quoteTable(tableName),
         values: values.join(','),
+        output: outputFragment,
         where: this.getWhereConditions(where)
       };
 

lib/dialects/mssql/index.js

@@ -12,8 +12,6 @@ var MssqlDialect = function(sequelize) {
 };
 
 MssqlDialect.prototype.supports = _.merge(_.cloneDeep(Abstract.prototype.supports), {
-  'RETURNING': false,
-  'OUTPUT': true,
   'DEFAULT': true,
   'DEFAULT VALUES': true,
   'LIMIT ON UPDATE': true,
@@ -21,6 +19,9 @@ MssqlDialect.prototype.supports = _.merge(_.cloneDeep(Abstract.prototype.support
   transactions: false,
   migrations: false,
   upserts: false,
+  returnValues: {
+    output: true
+  },
   autoIncrement: {
     identityInsert: true,
     defaultValue: false,

lib/dialects/mssql/query-generator.js

@@ -196,12 +196,17 @@ module.exports = (function() {
     },
 
     bulkInsertQuery: function(tableName, attrValueHashes, options, attributes) {
-      var query = 'INSERT INTO <%= table %> (<%= attributes %>) VALUES <%= tuples %>;'
-        , emptyQuery = 'INSERT INTO <%= table %> DEFAULT VALUES'
+      var query = 'INSERT INTO <%= table %> (<%= attributes %>)<%= output %> VALUES <%= tuples %>;'
+        , emptyQuery = 'INSERT INTO <%= table %><%= output %> DEFAULT VALUES'
         , tuples = []
         , allAttributes = []
         , needIdentityInsertWrapper = false
-        , allQueries = [];
+        , allQueries = []
+        , outputFragment;
+
+      if (options.returning) {
+        outputFragment = ' OUTPUT INSERTED.*';
+      }
 
       Utils._.forEach(attrValueHashes, function(attrValueHash, i) {
         // special case for empty objects with primary keys
@@ -243,7 +248,8 @@ module.exports = (function() {
         attributes: allAttributes.map(function(attr) {
                       return this.quoteIdentifier(attr);
                     }.bind(this)).join(','),
-        tuples: tuples
+        tuples: tuples,
+        output: outputFragment
       };
 
       var generatedQuery = Utils._.template(allQueries.join(';'))(replacements);

lib/dialects/mssql/query.js

@@ -114,12 +114,16 @@ module.exports = (function() {
       this.handleInsertQuery(data);
 
       if (!this.callee) {
+        if (this.options.plain) {
           // NOTE: super contrived. This just passes the newly added query-interface
           //       test returning only the PK. There isn't a way in MSSQL to identify
           //       that a given return value is the PK, and we have no schema information
           //       because there was no calling Model.
           var record = data[0];
           result = record[Object.keys(record)[0]];
+        } else {
+          result = data;
+        }
       }
     }
 

lib/dialects/postgres/index.js

@@ -12,10 +12,12 @@ var PostgresDialect = function(sequelize) {
 };
 
 PostgresDialect.prototype.supports = _.merge(_.cloneDeep(Abstract.prototype.supports), {
-  'RETURNING': true,
   'DEFAULT VALUES': true,
   'EXCEPTION': true,
   'ON DUPLICATE KEY': false,
+  returnValues: {
+    returning: true
+  },
   schemas: true,
   lock: true,
   forShare: 'FOR SHARE',

lib/dialects/postgres/query-generator.js

@@ -354,7 +354,7 @@ module.exports = (function() {
         , serials = []
         , allAttributes = [];
 
-      if (this._dialect.supports['RETURNING'] && options.returning) {
+      if (options.returning) {
         query += ' RETURNING *';
       }
 

lib/query-interface.js

@@ -459,7 +459,7 @@ module.exports = (function() {
   QueryInterface.prototype.insert = function(dao, tableName, values, options) {
     var sql = this.QueryGenerator.insertQuery(tableName, values, dao && dao.Model.rawAttributes, options);
 
-    options.type = 'INSERT';
+    options.type = QueryTypes.INSERT;
     return this.sequelize.query(sql, dao, options).then(function(result) {
       if (dao) result.isNewRecord = false;
       return result;
@@ -536,6 +536,7 @@ module.exports = (function() {
   };
 
   QueryInterface.prototype.bulkInsert = function(tableName, records, options, attributes) {
+    options.type = QueryTypes.INSERT;
     var sql = this.QueryGenerator.bulkInsertQuery(tableName, records, options, attributes);
     return this.sequelize.query(sql, null, options);
   };

test/model/create.test.js

@@ -449,27 +449,20 @@ describe(Support.getTestDialectTeaser('Model'), function() {
       });
     }
 
-    if (current.dialect.supports['RETURNING']) {
-      describe('Autoincrement values', function () {
+    if (current.dialect.supports.returnValues) {
+      describe('return values', function () {
         it('should make the autoincremented values available on the returned instances', function () {
           var User = this.sequelize.define('user', {});
 
           return User.sync({force: true}).then(function () {
-            return User.bulkCreate([
-              {},
-              {},
-              {}
-            ], {returning: true}).then(function (users) {
-              expect(users.length).to.be.ok;
-              users.forEach(function (user, i) {
+            return User.create({}, {returning: true}).then(function (user) {
               expect(user.get('id')).to.be.ok;
-                expect(user.get('id')).to.equal(i+1);
-              });
+              expect(user.get('id')).to.equal(1);
             });
           });
         });
 
-        it('should make the autoincremented values available on the returned instances', function () {
+        it('should make the autoincremented values available on the returned instances with custom fields', function () {
           var User = this.sequelize.define('user', {
             maId: {
               type: DataTypes.INTEGER,
@@ -480,16 +473,9 @@ describe(Support.getTestDialectTeaser('Model'), function() {
           });
 
           return User.sync({force: true}).then(function () {
-            return User.bulkCreate([
-              {},
-              {},
-              {}
-            ], {returning: true}).then(function (users) {
-              expect(users.length).to.be.ok;
-              users.forEach(function (user, i) {
+            return User.create({}, {returning: true}).then(function (user) {
               expect(user.get('maId')).to.be.ok;
-                expect(user.get('maId')).to.equal(i+1);
-              });
+              expect(user.get('maId')).to.equal(1);
             });
           });
         });
@@ -1506,6 +1492,53 @@ describe(Support.getTestDialectTeaser('Model'), function() {
       });
     }
 
+    if (current.dialect.supports.returnValues) {
+      describe('return values', function () {
+        it('should make the autoincremented values available on the returned instances', function () {
+          var User = this.sequelize.define('user', {});
+
+          return User.sync({force: true}).then(function () {
+            return User.bulkCreate([
+              {},
+              {},
+              {}
+            ], {returning: true}).then(function (users) {
+              expect(users.length).to.be.ok;
+              users.forEach(function (user, i) {
+                expect(user.get('id')).to.be.ok;
+                expect(user.get('id')).to.equal(i+1);
+              });
+            });
+          });
+        });
+
+        it('should make the autoincremented values available on the returned instances with custom fields', function () {
+          var User = this.sequelize.define('user', {
+            maId: {
+              type: DataTypes.INTEGER,
+              primaryKey: true,
+              autoIncrement: true,
+              field: 'yo_id'
+            }
+          });
+
+          return User.sync({force: true}).then(function () {
+            return User.bulkCreate([
+              {},
+              {},
+              {}
+            ], {returning: true}).then(function (users) {
+              expect(users.length).to.be.ok;
+              users.forEach(function (user, i) {
+                expect(user.get('maId')).to.be.ok;
+                expect(user.get('maId')).to.equal(i+1);
+              });
+            });
+          });
+        });
+      });
+    }
+
     describe('enums', function() {
       it('correctly restores enum values', function(done) {
         var self = this