Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from Sep 22, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -671,8 +671,11 @@ function discoverController(
aggs: visStateAggs
});

$scope.searchSource.onRequestStart(() => {
return $scope.vis.requesting();
});

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can be shortened to $scope.searchSource.onRequestStart($scope.vis.requesting);?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calling vis.requesting without it's this context might work, but seems like an unnecessary risk.

$scope.searchSource.aggs(function () {
$scope.vis.requesting();
return $scope.vis.getAggConfig().toDsl();
});

Expand Down
Expand Up @@ -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();
});

Expand Down
10 changes: 9 additions & 1 deletion src/fixtures/stubbed_search_source.js
Expand Up @@ -38,7 +38,15 @@ export default function stubSearchSource(Private, $q, Promise) {
onError: function () { return $q.defer().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() {}
};

}
34 changes: 34 additions & 0 deletions src/ui/public/courier/data_source/_abstract.js
Expand Up @@ -48,6 +48,7 @@ export function AbstractDataSourceProvider(Private, Promise, PromiseEmitter, con

self.history = [];
self._fetchStrategy = strategy;
self._requestStartHandlers = [];
}

/*****
Expand Down Expand Up @@ -237,6 +238,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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How come you need this .then(_.noop) part?

Copy link
Contributor Author

@spalger spalger Sep 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Documented the method as returning Promise<undefined> and realized that it was actually returning Promise<Array<WhateverStartHandlersReturn>>, so I used _.noop to actually resolve to undefined.

};

/**
* 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;
};

/*****
Expand Down
27 changes: 27 additions & 0 deletions 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);
}));
});
});
27 changes: 27 additions & 0 deletions 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);
}));
});
});
24 changes: 18 additions & 6 deletions src/ui/public/courier/fetch/request/__tests__/segmented.js
Expand Up @@ -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()', () => {
Expand All @@ -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);
});
});

Expand All @@ -41,7 +53,7 @@ describe('SegmentedRequestProvider', () => {

function mockSource() {
return {
get: sinon.stub().returns(mockIndexPattern())
get: sinon.stub().returns(mockIndexPattern()),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the extra comma here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a line below it at one point and deleted it later

};
}

Expand Down
11 changes: 2 additions & 9 deletions src/ui/public/courier/fetch/request/request.js
Expand Up @@ -63,14 +63,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() {
Expand Down Expand Up @@ -113,7 +106,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);
}

Expand Down
43 changes: 22 additions & 21 deletions src/ui/public/courier/fetch/request/segmented.js
Expand Up @@ -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();
});
}

Expand Down