fix(query): don't prepare options & sql for every retry (#10498)

lib/sequelize.js

@@ -460,22 +460,49 @@ class Sequelize {
 
   query(sql, options) {
     options = Object.assign({}, this.options.query, options);
-    const retryOptions = Object.assign({}, this.options.retry, options.retry || {});
 
-    let bindParameters;
+    if (options.instance && !options.model) {
+      options.model = options.instance.constructor;
+    }
 
-    return Promise.resolve(retry(retryParameters => Promise.try(() => {
-      const isFirstTry = retryParameters.current === 1;
+    if (!options.instance && !options.model) {
+      options.raw = true;
+    }
 
-      if (options.instance && !options.model) {
-        options.model = options.instance.constructor;
-      }
+    // map raw fields to model attributes
+    if (options.mapToModel) {
+      options.fieldMap = _.get(options, 'model.fieldAttributeMap', {});
+    }
 
-      // map raw fields to model attributes
-      if (options.mapToModel) {
-        options.fieldMap = _.get(options, 'model.fieldAttributeMap', {});
+    options = _.defaults(options, {
+      logging: this.options.hasOwnProperty('logging') ? this.options.logging : console.log,
+      searchPath: this.options.hasOwnProperty('searchPath') ? this.options.searchPath : 'DEFAULT'
+    });
+
+    if (!options.type) {
+      if (options.model || options.nest || options.plain) {
+        options.type = QueryTypes.SELECT;
+      } else {
+        options.type = QueryTypes.RAW;
       }
+    }
 
+    //if dialect doesn't support search_path or dialect option
+    //to prepend searchPath is not true delete the searchPath option
+    if (
+      !this.dialect.supports.searchPath ||
+      !this.options.dialectOptions ||
+      !this.options.dialectOptions.prependSearchPath ||
+      options.supportsSearchPath === false
+    ) {
+      delete options.searchPath;
+    } else if (!options.searchPath) {
+      //if user wants to always prepend searchPath (dialectOptions.preprendSearchPath = true)
+      //then set to DEFAULT if none is provided
+      options.searchPath = 'DEFAULT';
+    }
+
+    return Promise.try(() => {
       if (typeof sql === 'object') {
         if (sql.values !== undefined) {
           if (options.replacements !== undefined) {
@@ -498,10 +525,6 @@ class Sequelize {
 
       sql = sql.trim();
 
-      if (!options.instance && !options.model) {
-        options.raw = true;
-      }
-
       if (options.replacements && options.bind) {
         throw new Error('Both `replacements` and `bind` cannot be set at the same time');
       }
@@ -514,71 +537,49 @@ class Sequelize {
         }
       }
 
+      let bindParameters;
+
       if (options.bind) {
-        const bindSql = this.dialect.Query.formatBindParameters(sql, options.bind, this.options.dialect);
-        sql = bindSql[0];
-        bindParameters = bindSql[1];
+        [sql, bindParameters] = this.dialect.Query.formatBindParameters(sql, options.bind, this.options.dialect);
       }
 
-      options = _.defaults(options, {
-        logging: this.options.hasOwnProperty('logging') ? this.options.logging : console.log,
-        searchPath: this.options.hasOwnProperty('searchPath') ? this.options.searchPath : 'DEFAULT'
-      });
+      const retryOptions = Object.assign({}, this.options.retry, options.retry || {});
 
-      if (options.transaction === undefined && Sequelize._cls) {
-        options.transaction = Sequelize._cls.get('transaction');
-      }
+      return Promise.resolve(retry(retryParameters => Promise.try(() => {
+        const isFirstTry = retryParameters.current === 1;
 
-      if (!options.type) {
-        if (options.model || options.nest || options.plain) {
-          options.type = QueryTypes.SELECT;
-        } else {
-          options.type = QueryTypes.RAW;
+        if (isFirstTry && this.test._trackRunningQueries) {
+          this.test._runningQueries++;
         }
-      }
-
-      if (options.transaction && options.transaction.finished) {
-        const error = new Error(`${options.transaction.finished} has been called on this transaction(${options.transaction.id}), you can no longer use it. (The rejected query is attached as the \'sql\' property of this error)`);
-        error.sql = sql;
-        return Promise.reject(error);
-      }
-
-      if (isFirstTry && this.test._trackRunningQueries) {
-        this.test._runningQueries++;
-      }
-
-      //if dialect doesn't support search_path or dialect option
-      //to prepend searchPath is not true delete the searchPath option
-      if (
-        !this.dialect.supports.searchPath ||
-        !this.options.dialectOptions ||
-        !this.options.dialectOptions.prependSearchPath ||
-        options.supportsSearchPath === false
-      ) {
-        delete options.searchPath;
-      } else if (!options.searchPath) {
-        //if user wants to always prepend searchPath (dialectOptions.preprendSearchPath = true)
-        //then set to DEFAULT if none is provided
-        options.searchPath = 'DEFAULT';
-      }
 
-      return options.transaction
-        ? options.transaction.connection
-        : this.connectionManager.getConnection(options);
-    }).then(connection => {
-      const query = new this.dialect.Query(connection, this, options);
+        if (options.transaction === undefined && Sequelize._cls) {
+          options.transaction = Sequelize._cls.get('transaction');
+        }
 
-      return query.run(sql, bindParameters)
-        .finally(() => {
-          if (this.test._trackRunningQueries) {
-            this.test._runningQueries--;
-          }
+        if (options.transaction && options.transaction.finished) {
+          const error = new Error(`${options.transaction.finished} has been called on this transaction(${options.transaction.id}), you can no longer use it. (The rejected query is attached as the \'sql\' property of this error)`);
+          error.sql = sql;
+          throw error;
+        }
 
-          if (!options.transaction) {
-            return this.connectionManager.releaseConnection(connection);
-          }
-        });
-    }), retryOptions));
+        return options.transaction
+          ? options.transaction.connection
+          : this.connectionManager.getConnection(options);
+      }).then(connection => {
+        const query = new this.dialect.Query(connection, this, options);
+
+        return query.run(sql, bindParameters)
+          .finally(() => {
+            if (!options.transaction) {
+              return this.connectionManager.releaseConnection(connection);
+            }
+          });
+      }), retryOptions)).finally(() => {
+        if (this.test._trackRunningQueries) {
+          this.test._runningQueries--;
+        }
+      });
+    });
   }
 
   /**

test/integration/sequelize.test.js

@@ -225,13 +225,15 @@ describe(Support.getTestDialectTeaser('Sequelize'), () => {
   describe('query', () => {
     afterEach(function() {
       this.sequelize.options.quoteIdentifiers = true;
-
       console.log.restore && console.log.restore();
     });
 
     beforeEach(function() {
       this.User = this.sequelize.define('User', {
-        username: DataTypes.STRING,
+        username: {
+          type: DataTypes.STRING,
+          unique: true
+        },
         emailAddress: {
           type: DataTypes.STRING,
           field: 'email_address'
@@ -271,6 +273,33 @@ describe(Support.getTestDialectTeaser('Sequelize'), () => {
         });
     });
 
+    describe('retry',  () => {
+      it('properly bind parameters on extra retries', function() {
+        const payload = {
+          username: 'test',
+          createdAt: '2010-10-10 00:00:00',
+          updatedAt: '2010-10-10 00:00:00'
+        };
+
+        const spy = sinon.spy();
+
+        return expect(this.User.create(payload).then(() => this.sequelize.query(`
+          INSERT INTO ${qq(this.User.tableName)} (username,${qq('createdAt')},${qq('updatedAt')}) VALUES ($username,$createdAt,$updatedAt);
+        `, {
+          bind: payload,
+          logging: spy,
+          retry: {
+            max: 3,
+            match: [
+              /Validation/
+            ]
+          }
+        }))).to.be.rejectedWith(Sequelize.UniqueConstraintError).then(() => {
+          expect(spy.callCount).to.eql(3);
+        });
+      });
+    });
+
     describe('logging', () => {
       it('executes a query with global benchmarking option and default logger', () => {
         const logger = sinon.spy(console, 'log');