From 67487a40997082f5dfbdc9379833c7d799f57060 Mon Sep 17 00:00:00 2001 From: Court Ewing Date: Fri, 23 Oct 2015 10:51:24 -0400 Subject: [PATCH 1/7] Fix timing issue when fetching pre-flighted requests The fetch_these strategy in courier was assuming that any request that existed was ready to be sent off to the backend. Now, if req.start or req.continue return a promise, it will wait until any promises are resolved before attempting to make requests. --- src/ui/public/courier/fetch/_fetch_these.js | 27 ++++++++++--------- .../public/courier/fetch/request/segmented.js | 6 ++--- 2 files changed, 18 insertions(+), 15 deletions(-) diff --git a/src/ui/public/courier/fetch/_fetch_these.js b/src/ui/public/courier/fetch/_fetch_these.js index 81891f8e1b3e2..be1dcf63920d7 100644 --- a/src/ui/public/courier/fetch/_fetch_these.js +++ b/src/ui/public/courier/fetch/_fetch_these.js @@ -25,20 +25,10 @@ define(function (require) { function fetchWithStrategy(strategy, requests) { requests = requests.map(function (req) { - if (req.aborted) { - return ABORTED; - } - - if (req.started) { - req.continue(); - } else { - req.start(); - } - - return req; + return req.aborted ? ABORTED : req; }); - return Promise.resolve() + return startRequests(requests) .then(function () { return callClient(strategy, requests); }) @@ -63,6 +53,19 @@ define(function (require) { }); } + function startRequests(requests) { + return Promise.map(requests, function (req) { + if (req === ABORTED) { + return req; + } + + return new Promise(function (resolve) { + var action = req.started ? req.continue : req.start; + resolve(action.call(req)); + }); + }); + } + return fetchThese; }; }); diff --git a/src/ui/public/courier/fetch/request/segmented.js b/src/ui/public/courier/fetch/request/segmented.js index 29dee54f7e67d..552d13e110ed9 100644 --- a/src/ui/public/courier/fetch/request/segmented.js +++ b/src/ui/public/courier/fetch/request/segmented.js @@ -54,14 +54,14 @@ define(function (require) { // give the request consumer a chance to receive each segment and set // parameters via the handle if (_.isFunction(this._initFn)) this._initFn(this._handle); - this._createQueue().then(function (queue) { + return this._createQueue().then(function (queue) { self._all = queue.slice(0); // Send the initial fetch status self._reportStatus(); - }); - return SearchReq.prototype.start.call(this); + return SearchReq.prototype.start.call(self); + }); }; SegmentedReq.prototype.continue = function () { From 5c16a344ac0f9cc7dc8b54f1ffb2d5dd3b218741 Mon Sep 17 00:00:00 2001 From: Court Ewing Date: Fri, 23 Oct 2015 11:08:09 -0400 Subject: [PATCH 2/7] Do not flicker "no results" while preparing requests The recent changes to make segmented request preparation asynchronous resulted in a regression where the "no results found" screen was being displayed for a moment before being replaced with the "searching..." screen. This update ensures that the state where requests are being prepared is represented by a loading screen as well. --- src/plugins/kibana/public/discover/controllers/discover.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/plugins/kibana/public/discover/controllers/discover.js b/src/plugins/kibana/public/discover/controllers/discover.js index 832b73508774c..25462b58706d2 100644 --- a/src/plugins/kibana/public/discover/controllers/discover.js +++ b/src/plugins/kibana/public/discover/controllers/discover.js @@ -217,7 +217,10 @@ define(function (require) { if (rows == null && oldRows == null) return status.LOADING; var rowsEmpty = _.isEmpty(rows); - if (rowsEmpty && fetchStatus) return status.LOADING; + // an undefined fetchStatus means the requests are still being + // prepared to be sent + if (_.isUndefined(fetchStatus)) return status.LOADING; + else if (rowsEmpty && fetchStatus) return status.LOADING; else if (!rowsEmpty) return status.READY; else return status.NO_RESULTS; } From 341b6542a673ff8ec4eb8071bfa79cb75ad49af6 Mon Sep 17 00:00:00 2001 From: Court Ewing Date: Mon, 26 Oct 2015 10:22:00 -0400 Subject: [PATCH 3/7] Unit test async start for segmented requests --- .../courier/fetch/__tests__/fetch_these.js | 84 +++++++++++++++++++ .../fetch/request/__tests__/segmented.js | 55 ++++++++++++ 2 files changed, 139 insertions(+) create mode 100644 src/ui/public/courier/fetch/__tests__/fetch_these.js create mode 100644 src/ui/public/courier/fetch/request/__tests__/segmented.js diff --git a/src/ui/public/courier/fetch/__tests__/fetch_these.js b/src/ui/public/courier/fetch/__tests__/fetch_these.js new file mode 100644 index 0000000000000..af20d23f79548 --- /dev/null +++ b/src/ui/public/courier/fetch/__tests__/fetch_these.js @@ -0,0 +1,84 @@ +describe('ui/courier/fetch/_fetch_these', () => { + const _ = require('lodash'); + const sinon = require('auto-release-sinon'); + const expect = require('expect.js'); + const ngMock = require('ngMock'); + + let _Promise; + let $rootScope; + let fetchThese; + let request; + let requests; + let fakeResponses; + + beforeEach(ngMock.module('kibana', (PrivateProvider) => { + function FakeResponsesProvider(Promise) { + fakeResponses = sinon.spy(function () { + return Promise.map(requests, mockRequest => { + return { mockRequest }; + }); + }); + return fakeResponses; + } + + PrivateProvider.swap(require('ui/courier/fetch/_call_client'), FakeResponsesProvider); + PrivateProvider.swap(require('ui/courier/fetch/_call_response_handlers'), FakeResponsesProvider); + PrivateProvider.swap(require('ui/courier/fetch/_continue_incomplete'), FakeResponsesProvider); + })); + + beforeEach(ngMock.inject((Private, Promise, $injector) => { + _Promise = Promise; + $rootScope = $injector.get('$rootScope'); + fetchThese = Private(require('ui/courier/fetch/_fetch_these')); + request = mockRequest(); + requests = [ request ]; + })); + + context('when request has not started', () => { + beforeEach(() => _.forEach(requests, req => req.started = false)); + + it('starts request', () => { + fetchThese(requests); + expect(request.start.called).to.be(true); + expect(request.continue.called).to.be(false); + }); + + it('waits for returned promise from start() to be fulfilled', () => { + request.start = sinon.stub().returns(_Promise.resolve(request)); + fetchThese(requests); + + expect(request.start.callCount).to.be(1); + expect(fakeResponses.callCount).to.be(0); + $rootScope.$apply(); + expect(fakeResponses.callCount).to.be(3); + }); + }); + + context('when request has already started', () => { + it('continues request', () => { + fetchThese(requests); + expect(request.start.called).to.be(false); + expect(request.continue.called).to.be(true); + }); + it('waits for returned promise to be fulfilled', () => { + request.continue = sinon.stub().returns(_Promise.resolve(request)); + fetchThese(requests); + + expect(request.continue.callCount).to.be(1); + expect(fakeResponses.callCount).to.be(0); + $rootScope.$apply(); + expect(fakeResponses.callCount).to.be(3); + }); + }); + + function mockRequest() { + return { + strategy: 'mock', + started: true, + aborted: false, + retry: sinon.spy(function () { return this; }), + continue: sinon.spy(function () { return this; }), + start: sinon.spy(function () { return this; }) + }; + } +}); diff --git a/src/ui/public/courier/fetch/request/__tests__/segmented.js b/src/ui/public/courier/fetch/request/__tests__/segmented.js new file mode 100644 index 0000000000000..932bd4f18dfbd --- /dev/null +++ b/src/ui/public/courier/fetch/request/__tests__/segmented.js @@ -0,0 +1,55 @@ +describe('ui/courier/fetch/request/segmented', () => { + const sinon = require('auto-release-sinon'); + const expect = require('expect.js'); + const ngMock = require('ngMock'); + + let _Promise; + let $rootScope; + let SegmentedReq; + let segmentedReq; + let searchReqStart; + + beforeEach(ngMock.module('kibana')); + + beforeEach(ngMock.inject((Private, Promise, $injector) => { + _Promise = Promise; + $rootScope = $injector.get('$rootScope'); + SegmentedReq = Private(require('ui/courier/fetch/request/segmented')); + searchReqStart = sinon.spy(Private(require('ui/courier/fetch/request/search')).prototype, 'start'); + })); + + describe('#start()', () => { + let returned; + beforeEach(() => { + init(); + returned = segmentedReq.start(); + }); + + it('returns promise', () => { + expect(returned.then).to.be.Function; + }); + + it('does not call super.start() until promise is resolved', () => { + expect(searchReqStart.called).to.be(false); + $rootScope.$apply(); + expect(searchReqStart.called).to.be(true); + }); + }); + + function init() { + segmentedReq = new SegmentedReq(mockSource()); + } + + function mockSource() { + return { + get: sinon.stub().returns(mockIndexPattern()) + }; + } + + function mockIndexPattern() { + const queue = [1, 2, 3]; + return { + toIndexList: sinon.stub().returns(_Promise.resolve(queue)) + }; + } +}); From 84acf86da9ffb06e9cfc03f10c9d0b1589b3ec48 Mon Sep 17 00:00:00 2001 From: Court Ewing Date: Mon, 26 Oct 2015 13:15:19 -0400 Subject: [PATCH 4/7] Retrieve test Promise via $injector This allows us to ditch the usage of _Promise throughout the test. --- src/ui/public/courier/fetch/__tests__/fetch_these.js | 10 +++++----- .../courier/fetch/request/__tests__/segmented.js | 8 ++++---- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/ui/public/courier/fetch/__tests__/fetch_these.js b/src/ui/public/courier/fetch/__tests__/fetch_these.js index af20d23f79548..cf6de862d1533 100644 --- a/src/ui/public/courier/fetch/__tests__/fetch_these.js +++ b/src/ui/public/courier/fetch/__tests__/fetch_these.js @@ -4,7 +4,7 @@ describe('ui/courier/fetch/_fetch_these', () => { const expect = require('expect.js'); const ngMock = require('ngMock'); - let _Promise; + let Promise; let $rootScope; let fetchThese; let request; @@ -26,9 +26,9 @@ describe('ui/courier/fetch/_fetch_these', () => { PrivateProvider.swap(require('ui/courier/fetch/_continue_incomplete'), FakeResponsesProvider); })); - beforeEach(ngMock.inject((Private, Promise, $injector) => { - _Promise = Promise; + beforeEach(ngMock.inject((Private, $injector) => { $rootScope = $injector.get('$rootScope'); + Promise = $injector.get('Promise'); fetchThese = Private(require('ui/courier/fetch/_fetch_these')); request = mockRequest(); requests = [ request ]; @@ -44,7 +44,7 @@ describe('ui/courier/fetch/_fetch_these', () => { }); it('waits for returned promise from start() to be fulfilled', () => { - request.start = sinon.stub().returns(_Promise.resolve(request)); + request.start = sinon.stub().returns(Promise.resolve(request)); fetchThese(requests); expect(request.start.callCount).to.be(1); @@ -61,7 +61,7 @@ describe('ui/courier/fetch/_fetch_these', () => { expect(request.continue.called).to.be(true); }); it('waits for returned promise to be fulfilled', () => { - request.continue = sinon.stub().returns(_Promise.resolve(request)); + request.continue = sinon.stub().returns(Promise.resolve(request)); fetchThese(requests); expect(request.continue.callCount).to.be(1); diff --git a/src/ui/public/courier/fetch/request/__tests__/segmented.js b/src/ui/public/courier/fetch/request/__tests__/segmented.js index 932bd4f18dfbd..8c7745d47bed1 100644 --- a/src/ui/public/courier/fetch/request/__tests__/segmented.js +++ b/src/ui/public/courier/fetch/request/__tests__/segmented.js @@ -3,7 +3,7 @@ describe('ui/courier/fetch/request/segmented', () => { const expect = require('expect.js'); const ngMock = require('ngMock'); - let _Promise; + let Promise; let $rootScope; let SegmentedReq; let segmentedReq; @@ -11,8 +11,8 @@ describe('ui/courier/fetch/request/segmented', () => { beforeEach(ngMock.module('kibana')); - beforeEach(ngMock.inject((Private, Promise, $injector) => { - _Promise = Promise; + beforeEach(ngMock.inject((Private, $injector) => { + Promise = $injector.get('Promise'); $rootScope = $injector.get('$rootScope'); SegmentedReq = Private(require('ui/courier/fetch/request/segmented')); searchReqStart = sinon.spy(Private(require('ui/courier/fetch/request/search')).prototype, 'start'); @@ -49,7 +49,7 @@ describe('ui/courier/fetch/request/segmented', () => { function mockIndexPattern() { const queue = [1, 2, 3]; return { - toIndexList: sinon.stub().returns(_Promise.resolve(queue)) + toIndexList: sinon.stub().returns(Promise.resolve(queue)) }; } }); From febb456eacb461743b60560dff326e7232b42650 Mon Sep 17 00:00:00 2001 From: Court Ewing Date: Mon, 26 Oct 2015 13:20:00 -0400 Subject: [PATCH 5/7] Properly mark requests as not started in fetch_these test --- src/ui/public/courier/fetch/__tests__/fetch_these.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ui/public/courier/fetch/__tests__/fetch_these.js b/src/ui/public/courier/fetch/__tests__/fetch_these.js index cf6de862d1533..bb9efe5c589fa 100644 --- a/src/ui/public/courier/fetch/__tests__/fetch_these.js +++ b/src/ui/public/courier/fetch/__tests__/fetch_these.js @@ -35,7 +35,7 @@ describe('ui/courier/fetch/_fetch_these', () => { })); context('when request has not started', () => { - beforeEach(() => _.forEach(requests, req => req.started = false)); + beforeEach(() => requests.forEach(req => req.started = false)); it('starts request', () => { fetchThese(requests); From b3401fb5bc73992ba47577ba25ddca2796bf666f Mon Sep 17 00:00:00 2001 From: Court Ewing Date: Mon, 26 Oct 2015 15:16:14 -0400 Subject: [PATCH 6/7] More expressiveness in preparing fetchStatus condition This does not change the behavior of the conditional checks on fetchStatus. --- src/plugins/kibana/public/discover/controllers/discover.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/plugins/kibana/public/discover/controllers/discover.js b/src/plugins/kibana/public/discover/controllers/discover.js index 25462b58706d2..9912fc95d2d96 100644 --- a/src/plugins/kibana/public/discover/controllers/discover.js +++ b/src/plugins/kibana/public/discover/controllers/discover.js @@ -219,7 +219,8 @@ define(function (require) { var rowsEmpty = _.isEmpty(rows); // an undefined fetchStatus means the requests are still being // prepared to be sent - if (_.isUndefined(fetchStatus)) return status.LOADING; + var preparingForFetch = _.isUndefined(fetchStatus); + if (preparingForFetch) return status.LOADING; else if (rowsEmpty && fetchStatus) return status.LOADING; else if (!rowsEmpty) return status.READY; else return status.NO_RESULTS; From e3d36e47a4b529d135e6ce9a009b3a231e6e0bf3 Mon Sep 17 00:00:00 2001 From: Court Ewing Date: Mon, 26 Oct 2015 15:24:27 -0400 Subject: [PATCH 7/7] More detail in fetchStatus inline comments The fetchStatus logic is a bit specific, so it's worth having some more explanation around its usage. --- src/plugins/kibana/public/discover/controllers/discover.js | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/plugins/kibana/public/discover/controllers/discover.js b/src/plugins/kibana/public/discover/controllers/discover.js index 9912fc95d2d96..82056d4785074 100644 --- a/src/plugins/kibana/public/discover/controllers/discover.js +++ b/src/plugins/kibana/public/discover/controllers/discover.js @@ -217,8 +217,10 @@ define(function (require) { if (rows == null && oldRows == null) return status.LOADING; var rowsEmpty = _.isEmpty(rows); - // an undefined fetchStatus means the requests are still being - // prepared to be sent + // An undefined fetchStatus means the requests are still being + // prepared to be sent. When all requests are completed, + // fetchStatus is set to null, so it's important that we + // specifically check for undefined to determine a loading status. var preparingForFetch = _.isUndefined(fetchStatus); if (preparingForFetch) return status.LOADING; else if (rowsEmpty && fetchStatus) return status.LOADING;