fix(sequelize/query): retry entire query (#8991)

lib/sequelize.js

@@ -163,7 +163,12 @@ class Sequelize {
       pool: {},
       quoteIdentifiers: true,
       hooks: {},
-      retry: {max: 5, match: ['SQLITE_BUSY: database is locked']},
+      retry: {
+        max: 5,
+        match: [
+          'SQLITE_BUSY: database is locked'
+        ]
+      },
       transactionType: Transaction.TYPES.DEFERRED,
       isolationLevel: null,
       databaseVersion: 0,
@@ -425,14 +430,14 @@ class Sequelize {
    * @param {Object|Array}    [options.bind] Either an object of named bind parameter in the format `_param` or an array of unnamed bind parameter to replace `$1, $2, ...` in your SQL.
    * @param {Boolean}         [options.useMaster=false] Force the query to use the write pool, regardless of the query type.
    * @param {Function}        [options.logging=false] A function that gets executed while running the query to log the sql.
-   * @param {new Model()}       [options.instance] A sequelize instance used to build the return instance
+   * @param {new Model()}     [options.instance] A sequelize instance used to build the return instance
    * @param {Model}           [options.model] A sequelize model used to build the returned model instances (used to be called callee)
    * @param {Object}          [options.retry] Set of flags that control when a query is automatically retried.
    * @param {Array}           [options.retry.match] Only retry a query if the error matches one of these strings.
    * @param {Integer}         [options.retry.max] How many times a failing query is automatically retried.
    * @param {String}          [options.searchPath=DEFAULT] An optional parameter to specify the schema search_path (Postgres only)
    * @param {Boolean}         [options.supportsSearchPath] If false do not prepend the query with the search_path (Postgres only)
-   * @param {Boolean}          [options.mapToModel=false] Map returned fields to model's fields if `options.model` or `options.instance` is present. Mapping will occur before building the model instance.
+   * @param {Boolean}         [options.mapToModel=false] Map returned fields to model's fields if `options.model` or `options.instance` is present. Mapping will occur before building the model instance.
    * @param {Object}          [options.fieldMap] Map returned fields to arbitrary names for `SELECT` query type.
    *
    * @return {Promise}
@@ -441,10 +446,13 @@ class Sequelize {
    */
 
   query(sql, options) {
+    options = _.assign({}, this.options.query, options);
+    const retryOptions = _.assignIn({}, this.options.retry, options.retry || {});
+
     let bindParameters;
 
-    return Promise.try(() => {
-      options = _.assign({}, this.options.query, options);
+    return retry(retryParameters => Promise.try(() => {
+      const isFirstTry = retryParameters.current === 1;
 
       if (options.instance && !options.model) {
         options.model = options.instance.constructor;
@@ -517,41 +525,47 @@ class Sequelize {
       }
 
       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)');
+        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 (this.test._trackRunningQueries) {
+      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) {
+      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);
+
+      return options.transaction
+        ? options.transaction.connection
+        : this.connectionManager.getConnection(options);
     }).then(connection => {
       const query = new this.dialect.Query(connection, this, options);
-      const retryOptions = _.extend(this.options.retry, options.retry || {});
 
-      return retry(() => query.run(sql, bindParameters), retryOptions)
+      return query.run(sql, bindParameters)
         .finally(() => {
+          if (this.test._trackRunningQueries) {
+            this.test._runningQueries--;
+          }
+
           if (!options.transaction) {
             return this.connectionManager.releaseConnection(connection);
           }
         });
-    }).finally(() => {
-      if (this.test._trackRunningQueries) {
-        this.test._runningQueries--;
-      }
-    });
+    }), retryOptions);
   }
 
   /**
@@ -772,7 +786,7 @@ class Sequelize {
   databaseVersion(options) {
     return this.getQueryInterface().databaseVersion(options);
   }
-  
+
   /**
    * Get the fn for random based on the dialect
    *

test/unit/query.test.js

-'use strict';
-
-const chai = require('chai'),
-  sinon = require('sinon'),
-  expect = chai.expect,
-  Support = require(__dirname + '/support'),
-  Sequelize = Support.Sequelize,
-  Promise = Sequelize.Promise,
-  current = Support.sequelize;
-
-describe('sequelize.query', () => {
-  it('connection should be released only once when retry fails', () => {
-    const getConnectionStub = sinon.stub(current.connectionManager, 'getConnection').callsFake(() => {
-      return Promise.resolve({});
-    });
-    const releaseConnectionStub = sinon.stub(current.connectionManager, 'releaseConnection').callsFake(() => {
-      return Promise.resolve();
-    });
-    const queryStub = sinon.stub(current.dialect.Query.prototype, 'run').callsFake(() => {
-      return Promise.reject(new Error('wrong sql'));
-    });
-    return current.query('THIS IS A WRONG SQL', {
-      retry: {
-        max: 2,
-        // retry for all errors
-        match: null
-      }
-    })
-      .catch(() => {})
-      .finally(() => {
-        expect(releaseConnectionStub).have.been.calledOnce;
-        queryStub.restore();
-        getConnectionStub.restore();
-        releaseConnectionStub.restore();
-      });
-  });
-});