[vis/requesting] only call when actually requesting (#14017)

* [vis/requesting] only call when actually requesting

* [courier/dataSource] use Promise.map to catch sync errors

* [courier/request] move source mutation logic out of request

(cherry picked from commit b0dc246)

src/core_plugins/kibana/public/discover/controllers/discover.js

@@ -679,8 +679,11 @@ function discoverController(
       aggs: visStateAggs
     });
 
+    $scope.searchSource.onRequestStart(() => {
+      return $scope.vis.requesting();
+    });
+
     $scope.searchSource.aggs(function () {
-      $scope.vis.requesting();
       return $scope.vis.getAggConfig().toDsl();
     });
 

src/core_plugins/kibana/public/visualize/saved_visualizations/_saved_vis.js

@@ -72,8 +72,11 @@ uiModules
       return self.vis ? self._updateVis() : self._createVis();
     })
     .then(function () {
+      self.searchSource.onRequestStart(() => {
+        return self.vis.requesting();
+      });
+
       self.searchSource.aggs(function () {
-        self.vis.requesting();
         return self.vis.aggs.toDsl();
       });
 

src/fixtures/stubbed_search_source.js

@@ -37,7 +37,15 @@ export default function stubSearchSource(Private, $q, Promise) {
     },
     _flatten: function () {
       return Promise.resolve({ index: indexPattern, body: {} });
-    }
+    },
+    _requestStartHandlers: [],
+    onRequestStart(fn) {
+      this._requestStartHandlers.push(fn);
+    },
+    requestIsStarting(req) {
+      return Promise.map(this._requestStartHandlers, fn => fn(req));
+    },
+    requestIsStopped() {}
   };
 
 }

src/ui/public/courier/data_source/_abstract.js

@@ -47,6 +47,7 @@ export function AbstractDataSourceProvider(Private, Promise, PromiseEmitter, con
 
     self.history = [];
     self._fetchStrategy = strategy;
+    self._requestStartHandlers = [];
   }
 
   /*****
@@ -222,6 +223,39 @@ export function AbstractDataSourceProvider(Private, Promise, PromiseEmitter, con
    */
   SourceAbstract.prototype.destroy = function () {
     this.cancelQueued();
+    this._requestStartHandlers.length = 0;
+  };
+
+  /**
+   *  Add a handler that will be notified whenever requests start
+   *  @param  {Function} handler
+   *  @return {undefined}
+   */
+  SourceAbstract.prototype.onRequestStart = function (handler) {
+    this._requestStartHandlers.push(handler);
+  };
+
+  /**
+   *  Called by requests of this search source when they are started
+   *  @param  {Courier.Request} request
+   *  @return {Promise<undefined>}
+   */
+  SourceAbstract.prototype.requestIsStarting = function (request) {
+    this.activeFetchCount = (this.activeFetchCount || 0) + 1;
+    this.history = [request];
+
+    return Promise
+      .map(this._requestStartHandlers, fn => fn(this, request))
+      .then(_.noop);
+  };
+
+  /**
+   *  Called by requests of this search source when they are done
+   *  @param  {Courier.Request} request
+   *  @return {undefined}
+   */
+  SourceAbstract.prototype.requestIsStopped = function (/* request */) {
+    this.activeFetchCount -= 1;
   };
 
   /*****

src/ui/public/courier/fetch/request/__tests__/abstact_request.js

@@ -0,0 +1,27 @@
+import ngMock from 'ng_mock';
+import sinon from 'sinon';
+
+import { AbstractRequestProvider } from '../request';
+import { requestQueue } from '../../../_request_queue';
+
+describe('courier/fetch abstract request', () => {
+  beforeEach(ngMock.module('kibana'));
+
+  afterEach(() => {
+    requestQueue.clear();
+  });
+
+  describe('#start()', () => {
+    it('calls this.source.requestIsStarting(request)', ngMock.inject((Private) => {
+      const AbstractReq = Private(AbstractRequestProvider);
+
+      const spy = sinon.spy(() => Promise.resolve());
+      const source = { requestIsStarting: spy };
+
+      const req = new AbstractReq(source);
+      expect(req.start()).to.have.property('then').a('function');
+      sinon.assert.calledOnce(spy);
+      sinon.assert.calledWithExactly(spy, req);
+    }));
+  });
+});

src/ui/public/courier/fetch/request/__tests__/search_request.js

@@ -0,0 +1,27 @@
+import ngMock from 'ng_mock';
+import sinon from 'sinon';
+
+import { SearchRequestProvider } from '../search';
+import { requestQueue } from '../../../_request_queue';
+
+describe('ui/courier/fetch search request', () => {
+  beforeEach(ngMock.module('kibana'));
+
+  afterEach(() => {
+    requestQueue.clear();
+  });
+
+  describe('#start()', () => {
+    it('calls this.source.requestIsStarting(request)', ngMock.inject((Private) => {
+      const SearchReq = Private(SearchRequestProvider);
+
+      const spy = sinon.spy(() => Promise.resolve());
+      const source = { requestIsStarting: spy };
+
+      const req = new SearchReq(source);
+      expect(req.start()).to.have.property('then').a('function');
+      sinon.assert.calledOnce(spy);
+      sinon.assert.calledWithExactly(spy, req);
+    }));
+  });
+});

src/ui/public/courier/fetch/request/__tests__/segmented.js

@@ -3,20 +3,26 @@ import expect from 'expect.js';
 import ngMock from 'ng_mock';
 
 import { SegmentedRequestProvider } from '../segmented';
-import { SearchRequestProvider } from '../search';
+import { AbstractRequestProvider } from '../request';
 
 describe('SegmentedRequestProvider', () => {
   let Promise;
   let SegmentedReq;
   let segmentedReq;
-  let searchReqStart;
+  let abstractReqStart;
 
   beforeEach(ngMock.module('kibana'));
 
   beforeEach(ngMock.inject((Private, $injector) => {
     Promise = $injector.get('Promise');
     SegmentedReq = Private(SegmentedRequestProvider);
-    searchReqStart = sinon.spy(Private(SearchRequestProvider).prototype, 'start');
+
+    const AbstractReq = Private(AbstractRequestProvider);
+    abstractReqStart = sinon.stub(AbstractReq.prototype, 'start', () => {
+      const promise = Promise.resolve();
+      sinon.spy(promise, 'then');
+      return promise;
+    });
   }));
 
   describe('#start()', () => {
@@ -30,8 +36,14 @@ describe('SegmentedRequestProvider', () => {
       expect(returned.then).to.be.Function;
     });
 
-    it('calls super.start() synchronously', () => {
-      expect(searchReqStart.called).to.be(true);
+    it('calls AbstractReq#start()', () => {
+      sinon.assert.calledOnce(abstractReqStart);
+    });
+
+    it('listens to promise from super.start()', () => {
+      sinon.assert.calledOnce(abstractReqStart);
+      const promise = abstractReqStart.firstCall.returnValue;
+      sinon.assert.calledOnce(promise.then);
     });
   });
 
@@ -41,7 +53,7 @@ describe('SegmentedRequestProvider', () => {
 
   function mockSource() {
     return {
-      get: sinon.stub().returns(mockIndexPattern())
+      get: sinon.stub().returns(mockIndexPattern()),
     };
   }
 

src/ui/public/courier/fetch/request/request.js

@@ -57,14 +57,7 @@ export function AbstractRequestProvider(Private, Promise) {
       this.started = true;
       this.moment = moment();
 
-      const source = this.source;
-      if (source.activeFetchCount) {
-        source.activeFetchCount += 1;
-      } else {
-        source.activeFetchCount = 1;
-      }
-
-      source.history = [this];
+      return this.source.requestIsStarting(this);
     }
 
     getFetchParams() {
@@ -107,7 +100,7 @@ export function AbstractRequestProvider(Private, Promise) {
     _markStopped() {
       if (this.stopped) return;
       this.stopped = true;
-      this.source.activeFetchCount -= 1;
+      this.source.requestIsStopped(this);
       _.pull(requestQueue, this);
     }
 

src/ui/public/courier/fetch/request/segmented.js

@@ -39,33 +39,34 @@ export function SegmentedRequestProvider(es, Private, Promise, timefilter, confi
     *********/
 
     start() {
-      super.start();
-
-      this._complete = [];
-      this._active = null;
-      this._segments = [];
-      this._all = [];
-      this._queue = [];
-
-      this._mergedResp = {
-        took: 0,
-        hits: {
-          hits: [],
-          total: 0,
-          max_score: 0
-        }
-      };
+      return super.start().then(() => {
+        this._complete = [];
+        this._active = null;
+        this._segments = [];
+        this._all = [];
+        this._queue = [];
+
+        this._mergedResp = {
+          took: 0,
+          hits: {
+            hits: [],
+            total: 0,
+            max_score: 0
+          }
+        };
 
-      // give the request consumer a chance to receive each segment and set
-      // parameters via the handle
-      if (_.isFunction(this._initFn)) this._initFn(this._handle);
-      return this._createQueue().then((queue) => {
+        // give the request consumer a chance to receive each segment and set
+        // parameters via the handle
+        if (_.isFunction(this._initFn)) this._initFn(this._handle);
+        return this._createQueue();
+      })
+      .then((queue) => {
         if (this.stopped) return;
 
         this._all = queue.slice(0);
 
         // Send the initial fetch status
-        this._reportStatus();
+        return this._reportStatus();
       });
     }