feat(postgres): change returning option to only return model attributes (#11526)

Ricardo Proença · Sushant
Commit 26ea410e authored Oct 19, 2019 by Ricardo Proença Committed by Sushant Oct 19, 2019
Showing with 89 additions and 23 deletions
lib/dialects/abstract/query-generator.js
lib/dialects/mssql/query-generator.js
lib/dialects/postgres/query-generator.js
lib/model.js
test/integration/model.test.js
test/integration/model/bulk-create.test.js
test/integration/model/create.test.js
test/unit/sql/insert.test.js
test/unit/sql/update.test.js
--- a/lib/dialects/abstract/query-generator.js
+++ b/lib/dialects/abstract/query-generator.js
--- a/lib/dialects/mssql/query-generator.js
+++ b/lib/dialects/mssql/query-generator.js
@@ -316,7 +316,9 @@ class MSSQLQueryGenerator extends AbstractQueryGenerator {
      outputFragment = '';
    if (options.returning) {
-      outputFragment = ' OUTPUT INSERTED.*';
+      const returnValues = this.generateReturnValues(attributes, options);
+      outputFragment = returnValues.outputFragment;
    }
    const emptyQuery = `INSERT INTO ${quotedTable}${outputFragment} DEFAULT VALUES`;

--- a/lib/dialects/postgres/query-generator.js
+++ b/lib/dialects/postgres/query-generator.js
@@ -343,7 +343,7 @@ class PostgresQueryGenerator extends AbstractQueryGenerator {
  upsertQuery(tableName, insertValues, updateValues, where, model, options) {
    const primaryField = this.quoteIdentifier(model.primaryKeyField);
-    const upsertOptions = _.defaults({ bindParam: false }, options);
+    const upsertOptions = _.defaults({ bindParam: false, returning: ['*'] }, options);
    const insert = this.insertQuery(tableName, insertValues, model.rawAttributes, upsertOptions);
    const update = this.updateQuery(tableName, updateValues, where, upsertOptions, model.rawAttributes);

--- a/lib/model.js
+++ b/lib/model.js
@@ -2187,18 +2187,17 @@ class Model {
  /**
   * Builds a new model instance and calls save on it.
+   *
   * @see
   * {@link Model.build}
   * @see
   * {@link Model.save}
   *
-   * @param {Object}        values hash of data values to create new record with
+   * @param  {Object}         values                       Hash of data values to create new record with
-   * @param {Object}        [options] build and query options
+   * @param  {Object}         [options]                    Build and query options
   * @param  {boolean}        [options.raw=false]          If set to true, values will ignore field and virtual setters.
   * @param  {boolean}        [options.isNewRecord=true]   Is this new record
-   * @param {Array}         [options.include] an array of include options - Used to build prefetched/included model instances. See `set`
+   * @param  {Array}          [options.include]            An array of include options - Used to build prefetched/included model instances. See `set`
-   * @param {Array}         [options.fields] If set, only columns matching those in fields will be saved
   * @param  {string[]}       [options.fields]             An optional array of strings, representing database columns. If fields is provided, only those columns will be validated and saved.
   * @param  {boolean}        [options.silent=false]       If true, the updatedAt timestamp will not be updated.
   * @param  {boolean}        [options.validate=true]      If false, validations won't be run.
@@ -2207,7 +2206,7 @@ class Model {
   * @param  {boolean}        [options.benchmark=false]    Pass query execution time in milliseconds as second argument to logging function (options.logging).
   * @param  {Transaction}    [options.transaction]        Transaction to run query under
   * @param  {string}         [options.searchPath=DEFAULT] An optional parameter to specify the schema search_path (Postgres only)
-   * @param {boolean}       [options.returning=true] Return the affected rows (only for postgres)
+   * @param  {boolean|Array}  [options.returning=true]     Appends RETURNING <model columns> to get back all defined values; if an array of column names, append RETURNING <columns> to get back specific columns (Postgres only)
   *
   * @returns {Promise<Model>}
   *
@@ -2326,6 +2325,7 @@ class Model {
      }
      options.exception = true;
+      options.returning = true;
      return this.create(values, options).then(instance => {
        if (instance.get(this.primaryKeyAttribute, { raw: true }) === null) {
@@ -2426,13 +2426,13 @@ class Model {
   * @param  {boolean}      [options.validate=true]                       Run validations before the row is inserted
   * @param  {Array}        [options.fields=Object.keys(this.attributes)] The fields to insert / update. Defaults to all changed fields
   * @param  {boolean}      [options.hooks=true]                          Run before / after upsert hooks?
-   * @param  {boolean}      [options.returning=false] Append RETURNING * to get back auto generated values (Postgres only)
+   * @param  {boolean}      [options.returning=false]                     If true, fetches back auto generated values (Postgres only)
   * @param  {Transaction}  [options.transaction]                         Transaction to run query under
   * @param  {Function}     [options.logging=false]                       A function that gets executed while running the query to log the sql.
   * @param  {boolean}      [options.benchmark=false]                     Pass query execution time in milliseconds as second argument to logging function (options.logging).
   * @param  {string}       [options.searchPath=DEFAULT]                  An optional parameter to specify the schema search_path (Postgres only)
   *
-   * @returns {Promise<boolean>} Returns a boolean indicating whether the row was created or updated. For MySQL/MariaDB, it returns `true` when inserted and `false` when updated. For Postgres/MSSQL with (options.returning=true), it returns record and created boolean with signature `<Model, created>`.
+   * @returns {Promise<boolean>} Returns a boolean indicating whether the row was created or updated. For MySQL/MariaDB, it returns `true` when inserted and `false` when updated. For Postgres/MSSQL with `options.returning` true, it returns record and created boolean with signature `<Model, created>`.
   */
  static upsert(values, options) {
    options = Object.assign({
@@ -2524,7 +2524,7 @@ class Model {
   * @param  {Transaction}    [options.transaction]            Transaction to run query under
   * @param  {Function}       [options.logging=false]          A function that gets executed while running the query to log the sql.
   * @param  {boolean}        [options.benchmark=false]        Pass query execution time in milliseconds as second argument to logging function (options.logging).
-   * @param  {boolean|Array} [options.returning=false]       If true, append RETURNING * to get back all values; if an array of column names, append RETURNING <columns> to get back specific columns (Postgres only)
+   * @param  {boolean|Array}  [options.returning=false]        If true, append RETURNING <model columns> to get back all defined values; if an array of column names, append RETURNING <columns> to get back specific columns (Postgres only)
   * @param  {string}         [options.searchPath=DEFAULT]     An optional parameter to specify the schema search_path (Postgres only)
   *
   * @returns {Promise<Array<Model>>}
@@ -2708,7 +2708,7 @@ class Model {
          // Map returning attributes to fields
          if (options.returning && Array.isArray(options.returning)) {
-            options.returning = options.returning.map(attr => model.rawAttributes[attr].field || attr);
+            options.returning = options.returning.map(attr => _.get(model.rawAttributes[attr], 'field', attr));
          }
          return model.QueryInterface.bulkInsert(model.getTableName(options), records, options, fieldMappedAttributes).then(results => {
@@ -3034,7 +3034,7 @@ class Model {
   * @param  {boolean}        [options.hooks=true]            Run before / after bulk update hooks?
   * @param  {boolean}        [options.sideEffects=true]      Whether or not to update the side effects of any virtual setters.
   * @param  {boolean}        [options.individualHooks=false] Run before / after update hooks?. If true, this will execute a SELECT followed by individual UPDATEs. A select is needed, because the row data needs to be passed to the hooks
-   * @param  {boolean}      [options.returning=false]       Return the affected rows (only for postgres)
+   * @param  {boolean|Array}  [options.returning=false]       If true, append RETURNING <model columns> to get back all defined values; if an array of column names, append RETURNING <columns> to get back specific columns (Postgres only)
   * @param  {number}         [options.limit]                 How many rows to update (only for mysql and mariadb, implemented as TOP(n) for MSSQL; for sqlite it is supported only when rowid is present)
   * @param  {Function}       [options.logging=false]         A function that gets executed while running the query to log the sql.
   * @param  {boolean}        [options.benchmark=false]       Pass query execution time in milliseconds as second argument to logging function (options.logging).
@@ -3042,7 +3042,7 @@ class Model {
   * @param  {boolean}        [options.silent=false]          If true, the updatedAt timestamp will not be updated.
   *
   * @returns {Promise<Array<number,number>>}  The promise returns an array with one or two elements. The first element is always the number
-   * of affected rows, while the second element is the actual affected rows (only supported in postgres with `options.returning` true.)
+   * of affected rows, while the second element is the actual affected rows (only supported in postgres with `options.returning` true).
   *
   */
  static update(values, options) {
@@ -3325,7 +3325,7 @@ class Model {
   * @param  {Transaction}          [options.transaction]        Transaction to run query under
   * @param  {string}               [options.searchPath=DEFAULT] An optional parameter to specify the schema search_path (Postgres only)
   *
-   * @returns {Promise<Model[],?number>} returns an array of affected rows and affected count with `options.returning: true`, whenever supported by dialect
+   * @returns {Promise<Model[],?number>} returns an array of affected rows and affected count with `options.returning` true, whenever supported by dialect
   */
  static increment(fields, options) {
    options = options || {};
@@ -3411,8 +3411,8 @@ class Model {
   * @see
   * {@link Model#reload}
   * @since 4.36.0
+   *
-   * @returns {Promise<Model[],?number>} returns an array of affected rows and affected count with `options.returning: true`, whenever supported by dialect
+   * @returns {Promise<Model[],?number>} returns an array of affected rows and affected count with `options.returning` true, whenever supported by dialect
   */
  static decrement(fields, options) {
    options = _.defaults({ increment: false }, options, {

--- a/test/integration/model.test.js
+++ b/test/integration/model.test.js
@@ -929,14 +929,16 @@ describe(Support.getTestDialectTeaser('Model'), () => {
      return User.sync({ force: true }).then(() => {
        return User.create({ username: 'Peter', secretValue: '42' }).then(user => {
          return user.update({ secretValue: '43' }, {
-            fields: ['secretValue'], logging(sql) {
+            fields: ['secretValue'],
+            logging(sql) {
              test = true;
              if (dialect === 'mssql') {
                expect(sql).to.not.contain('createdAt');
              } else {
                expect(sql).to.match(/UPDATE\s+[`"]+User1s[`"]+\s+SET\s+[`"]+secretValue[`"]=(\$1|\?),[`"]+updatedAt[`"]+=(\$2|\?)\s+WHERE [`"]+id[`"]+\s=\s(\$3|\?)/);
              }
-            }
+            },
+            returning: ['*']
          });
        });
      }).then(() => {

--- a/test/integration/model/bulk-create.test.js
+++ b/test/integration/model/bulk-create.test.js
@@ -628,6 +628,56 @@ describe(Support.getTestDialectTeaser('Model'), () => {
              });
            });
        });
+        it('should only return fields that are not defined in the model (with returning: true)', function() {
+          const User = this.sequelize.define('user');
+          return User
+            .sync({ force: true })
+            .then(() => this.sequelize.queryInterface.addColumn('users', 'not_on_model', Sequelize.STRING))
+            .then(() => User.bulkCreate([
+              {},
+              {},
+              {}
+            ], {
+              returning: true
+            }))
+            .then(users =>
+              User.findAll()
+                .then(actualUsers => [users, actualUsers])
+            )
+            .then(([users, actualUsers]) => {
+              expect(users.length).to.eql(actualUsers.length);
+              users.forEach(user => {
+                expect(user.get()).not.to.have.property('not_on_model');
+              });
+            });
+        });
+        it('should return fields that are not defined in the model (with returning: ["*"])', function() {
+          const User = this.sequelize.define('user');
+          return User
+            .sync({ force: true })
+            .then(() => this.sequelize.queryInterface.addColumn('users', 'not_on_model', Sequelize.STRING))
+            .then(() => User.bulkCreate([
+              {},
+              {},
+              {}
+            ], {
+              returning: ['*']
+            }))
+            .then(users =>
+              User.findAll()
+                .then(actualUsers => [users, actualUsers])
+            )
+            .then(([users, actualUsers]) => {
+              expect(users.length).to.eql(actualUsers.length);
+              users.forEach(user => {
+                expect(user.get()).to.have.property('not_on_model');
+              });
+            });
+        });
      });
    }

--- a/test/integration/model/create.test.js
+++ b/test/integration/model/create.test.js
@@ -393,6 +393,18 @@ describe(Support.getTestDialectTeaser('Model'), () => {
      });
    });
+    it('should ignore option returning', function() {
+      return this.User.findOrCreate({
+        where: { username: 'Username' },
+        defaults: { data: 'ThisIsData' },
+        returning: false
+      }).then(([user, created]) => {
+        expect(user.username).to.equal('Username');
+        expect(user.data).to.equal('ThisIsData');
+        expect(created).to.be.true;
+      });
+    });
    if (current.dialect.supports.transactions) {
      it('should release transaction when meeting errors', function() {
        const test = times => {

--- a/test/unit/sql/insert.test.js
+++ b/test/unit/sql/insert.test.js
@@ -28,8 +28,8 @@ describe(Support.getTestDialectTeaser('SQL'), () => {
      expectsql(sql.insertQuery(User.tableName, { user_name: 'triggertest' }, User.rawAttributes, options),
        {
          query: {
-            mssql: 'declare @tmp table ([id] INTEGER,[user_name] NVARCHAR(255));INSERT INTO [users] ([user_name]) OUTPUT INSERTED.[id],INSERTED.[user_name] into @tmp VALUES ($1);select * from @tmp;',
+            mssql: 'DECLARE @tmp TABLE ([id] INTEGER,[user_name] NVARCHAR(255)); INSERT INTO [users] ([user_name]) OUTPUT INSERTED.[id],INSERTED.[user_name] INTO @tmp VALUES ($1); SELECT * FROM @tmp;',
-            postgres: 'INSERT INTO "users" ("user_name") VALUES ($1) RETURNING *;',
+            postgres: 'INSERT INTO "users" ("user_name") VALUES ($1) RETURNING "id","user_name";',
            default: 'INSERT INTO `users` (`user_name`) VALUES ($1);'
          },
          bind: ['triggertest']

--- a/test/unit/sql/update.test.js
+++ b/test/unit/sql/update.test.js
@@ -28,8 +28,8 @@ describe(Support.getTestDialectTeaser('SQL'), () => {
      expectsql(sql.updateQuery(User.tableName, { user_name: 'triggertest' }, { id: 2 }, options, User.rawAttributes),
        {
          query: {
-            mssql: 'declare @tmp table ([id] INTEGER,[user_name] NVARCHAR(255)); UPDATE [users] SET [user_name]=$1 OUTPUT INSERTED.[id],INSERTED.[user_name] into @tmp WHERE [id] = $2;select * from @tmp',
+            mssql: 'DECLARE @tmp TABLE ([id] INTEGER,[user_name] NVARCHAR(255)); UPDATE [users] SET [user_name]=$1 OUTPUT INSERTED.[id],INSERTED.[user_name] INTO @tmp WHERE [id] = $2; SELECT * FROM @tmp',
-            postgres: 'UPDATE "users" SET "user_name"=$1 WHERE "id" = $2 RETURNING *',
+            postgres: 'UPDATE "users" SET "user_name"=$1 WHERE "id" = $2 RETURNING "id","user_name"',
            default: 'UPDATE `users` SET `user_name`=$1 WHERE `id` = $2'
          },
          bind: {