From 404e12c2d287521c3ec90a245d33aff8ac4a8be1 Mon Sep 17 00:00:00 2001 From: Spencer Date: Fri, 22 Sep 2017 02:40:47 -0700 Subject: [PATCH] [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 b0dc2469c7033f287f96eafe9560b6b36808cf58) --- .../public/discover/controllers/discover.js | 5 ++- .../saved_visualizations/_saved_vis.js | 5 ++- src/fixtures/stubbed_search_source.js | 10 ++++- .../public/courier/data_source/_abstract.js | 34 +++++++++++++++ .../request/__tests__/abstact_request.js | 27 ++++++++++++ .../fetch/request/__tests__/search_request.js | 27 ++++++++++++ .../fetch/request/__tests__/segmented.js | 24 ++++++++--- .../public/courier/fetch/request/request.js | 11 +---- .../public/courier/fetch/request/segmented.js | 43 ++++++++++--------- 9 files changed, 147 insertions(+), 39 deletions(-) create mode 100644 src/ui/public/courier/fetch/request/__tests__/abstact_request.js create mode 100644 src/ui/public/courier/fetch/request/__tests__/search_request.js diff --git a/src/core_plugins/kibana/public/discover/controllers/discover.js b/src/core_plugins/kibana/public/discover/controllers/discover.js index 2a711a1fd5b0a5..e150e31c1c5910 100644 --- a/src/core_plugins/kibana/public/discover/controllers/discover.js +++ b/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(); }); diff --git a/src/core_plugins/kibana/public/visualize/saved_visualizations/_saved_vis.js b/src/core_plugins/kibana/public/visualize/saved_visualizations/_saved_vis.js index 9ba5ef4375a80c..df5142a355ed99 100644 --- a/src/core_plugins/kibana/public/visualize/saved_visualizations/_saved_vis.js +++ b/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(); }); diff --git a/src/fixtures/stubbed_search_source.js b/src/fixtures/stubbed_search_source.js index 3afe46fd74f048..1df9f821ffb076 100644 --- a/src/fixtures/stubbed_search_source.js +++ b/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() {} }; } diff --git a/src/ui/public/courier/data_source/_abstract.js b/src/ui/public/courier/data_source/_abstract.js index 0b3cd6f3f88159..3d9b849b5a0775 100644 --- a/src/ui/public/courier/data_source/_abstract.js +++ b/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} + */ + 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; }; /***** diff --git a/src/ui/public/courier/fetch/request/__tests__/abstact_request.js b/src/ui/public/courier/fetch/request/__tests__/abstact_request.js new file mode 100644 index 00000000000000..d0a17af83a8a83 --- /dev/null +++ b/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); + })); + }); +}); diff --git a/src/ui/public/courier/fetch/request/__tests__/search_request.js b/src/ui/public/courier/fetch/request/__tests__/search_request.js new file mode 100644 index 00000000000000..3d31def53b986e --- /dev/null +++ b/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); + })); + }); +}); diff --git a/src/ui/public/courier/fetch/request/__tests__/segmented.js b/src/ui/public/courier/fetch/request/__tests__/segmented.js index 87cacd7eb7d67a..99b0ea335eb856 100644 --- a/src/ui/public/courier/fetch/request/__tests__/segmented.js +++ b/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()), }; } diff --git a/src/ui/public/courier/fetch/request/request.js b/src/ui/public/courier/fetch/request/request.js index c88a3f84cc5d23..40aa74e351e26f 100644 --- a/src/ui/public/courier/fetch/request/request.js +++ b/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); } diff --git a/src/ui/public/courier/fetch/request/segmented.js b/src/ui/public/courier/fetch/request/segmented.js index 2811b0cd6ef4dc..c5932c70042358 100644 --- a/src/ui/public/courier/fetch/request/segmented.js +++ b/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(); }); }