Skip to content

Commit

Permalink
Refactor, deangularize courier/search source (#45235)
Browse files Browse the repository at this point in the history
* Initial refactor of search source

* Add abort signal to search source fetch and remove cancel queued

* Remove usages of Angular Promises

* Remove usages of angular "sessionId" service

* Remove config as dependency

* Update deps on config and esShardTimeout

* Remove remaining angular dependencies from SearchSource

* Fix Karma tests

* Separate callClient and handleResponse and add tests for each

* Add tests for fetchSoon

* Add back search source test and convert to jest

* Create search strategy registry test

* Revert empty test

* Remove filter predicates from search source

* Update typings and throw response errors

* Fix proxy to properly return response from ES

* Update jest snapshots

* Remove unused translations

* Don't pass search request to onRequestStart, create it afterwards

* Fix search source & get search params tests

* Fix issue with angular scope not firing after setting state on vis

* Fix tag cloud vis

* Fix setting of visConfig to not happen async

* Remove unused snapshots

* Remove unused reference to IPrivate
  • Loading branch information
lukasolson authored Oct 18, 2019
1 parent 9ff6d86 commit 3c7b160
Show file tree
Hide file tree
Showing 87 changed files with 1,493 additions and 3,315 deletions.
26 changes: 26 additions & 0 deletions packages/kbn-es-query/src/es_query/__tests__/from_filters.js
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,32 @@ describe('build query', function () {
expect(result.filter).to.eql(expectedESQueries);
});

it('should remove disabled filters', function () {
const filters = [
{
match_all: {},
meta: { type: 'match_all', negate: true, disabled: true },
},
];

const expectedESQueries = [];

const result = buildQueryFromFilters(filters);

expect(result.must_not).to.eql(expectedESQueries);
});

it('should remove falsy filters', function () {
const filters = [null, undefined];

const expectedESQueries = [];

const result = buildQueryFromFilters(filters);

expect(result.must_not).to.eql(expectedESQueries);
expect(result.must).to.eql(expectedESQueries);
});

it('should place negated filters in the must_not clause', function () {
const filters = [
{
Expand Down
1 change: 1 addition & 0 deletions packages/kbn-es-query/src/es_query/from_filters.js
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ const cleanFilter = function (filter) {
};

export function buildQueryFromFilters(filters = [], indexPattern, ignoreFilterIfFieldNotInIndex) {
filters = filters.filter(filter => filter && !_.get(filter, ['meta', 'disabled']));
return {
must: [],
filter: filters
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -123,12 +123,12 @@ class ListControl extends Control {
this.useTimeFilter,
ancestorFilters
);
this.abortController.signal.addEventListener('abort', () => searchSource.cancelQueued());
const abortSignal = this.abortController.signal;

this.lastQuery = query;
let resp;
try {
resp = await searchSource.fetch();
resp = await searchSource.fetch({ abortSignal });
} catch(error) {
// If the fetch was aborted then no need to surface this error in the UI
if (error.name === 'AbortError') return;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,11 +66,11 @@ class RangeControl extends Control {

const aggs = minMaxAgg(indexPattern.fields.getByName(fieldName));
const searchSource = createSearchSource(this.kbnApi, null, indexPattern, aggs, this.useTimeFilter);
this.abortController.signal.addEventListener('abort', () => searchSource.cancelQueued());
const abortSignal = this.abortController.signal;

let resp;
try {
resp = await searchSource.fetch();
resp = await searchSource.fetch({ abortSignal });
} catch(error) {
// If the fetch was aborted then no need to surface this error in the UI
if (error.name === 'AbortError') return;
Expand Down
38 changes: 16 additions & 22 deletions src/legacy/core_plugins/interpreter/public/functions/esaggs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ import chrome from 'ui/chrome';
import { TimeRange } from 'src/plugins/data/public';
import { SearchSource } from '../../../../ui/public/courier/search_source';
// @ts-ignore
import { SearchSourceProvider } from '../../../../ui/public/courier/search_source';
import { FilterBarQueryFilterProvider } from '../../../../ui/public/filter_manager/query_filter';

import { buildTabularInspectorData } from '../../../../ui/public/inspector/build_tabular_inspector_data';
Expand Down Expand Up @@ -100,8 +99,8 @@ const handleCourierRequest = async ({
return aggs.toDsl(metricsAtAllLevels);
});

requestSearchSource.onRequestStart((paramSearchSource: SearchSource, searchRequest: unknown) => {
return aggs.onSearchRequestStart(paramSearchSource, searchRequest);
requestSearchSource.onRequestStart((paramSearchSource: SearchSource, options: any) => {
return aggs.onSearchRequestStart(paramSearchSource, options);
});

if (timeRange) {
Expand All @@ -118,7 +117,7 @@ const handleCourierRequest = async ({
const queryHash = calculateObjectHash(reqBody);
// We only need to reexecute the query, if forceFetch was true or the hash of the request body has changed
// since the last request
const shouldQuery = forceFetch || searchSource.lastQuery !== queryHash;
const shouldQuery = forceFetch || (searchSource as any).lastQuery !== queryHash;

if (shouldQuery) {
inspectorAdapters.requests.reset();
Expand All @@ -139,18 +138,13 @@ const handleCourierRequest = async ({
request.stats(getRequestInspectorStats(requestSearchSource));

try {
// Abort any in-progress requests before fetching again
if (abortSignal) {
abortSignal.addEventListener('abort', () => requestSearchSource.cancelQueued());
}

const response = await requestSearchSource.fetch();
const response = await requestSearchSource.fetch({ abortSignal });

searchSource.lastQuery = queryHash;
(searchSource as any).lastQuery = queryHash;

request.stats(getResponseInspectorStats(searchSource, response)).ok({ json: response });

searchSource.rawResponse = response;
(searchSource as any).rawResponse = response;
} catch (e) {
// Log any error during request to the inspector
request.error({ json: e });
Expand All @@ -166,7 +160,7 @@ const handleCourierRequest = async ({
// Note that rawResponse is not deeply cloned here, so downstream applications using courier
// must take care not to mutate it, or it could have unintended side effects, e.g. displaying
// response data incorrectly in the inspector.
let resp = searchSource.rawResponse;
let resp = (searchSource as any).rawResponse;
for (const agg of aggs.aggs) {
if (has(agg, 'type.postFlightRequest')) {
resp = await agg.type.postFlightRequest(
Expand All @@ -180,7 +174,7 @@ const handleCourierRequest = async ({
}
}

searchSource.finalResponse = resp;
(searchSource as any).finalResponse = resp;

const parsedTimeRange = timeRange ? getTime(aggs.indexPattern, timeRange) : null;
const tabifyParams = {
Expand All @@ -191,23 +185,24 @@ const handleCourierRequest = async ({

const tabifyCacheHash = calculateObjectHash({ tabifyAggs: aggs, ...tabifyParams });
// We only need to reexecute tabify, if either we did a new request or some input params to tabify changed
const shouldCalculateNewTabify = shouldQuery || searchSource.lastTabifyHash !== tabifyCacheHash;
const shouldCalculateNewTabify =
shouldQuery || (searchSource as any).lastTabifyHash !== tabifyCacheHash;

if (shouldCalculateNewTabify) {
searchSource.lastTabifyHash = tabifyCacheHash;
searchSource.tabifiedResponse = tabifyAggResponse(
(searchSource as any).lastTabifyHash = tabifyCacheHash;
(searchSource as any).tabifiedResponse = tabifyAggResponse(
aggs,
searchSource.finalResponse,
(searchSource as any).finalResponse,
tabifyParams
);
}

inspectorAdapters.data.setTabularLoader(
() => buildTabularInspectorData(searchSource.tabifiedResponse, queryFilter),
() => buildTabularInspectorData((searchSource as any).tabifiedResponse, queryFilter),
{ returnsFormattedValues: true }
);

return searchSource.tabifiedResponse;
return (searchSource as any).tabifiedResponse;
};

export const esaggs = (): ExpressionFunction<typeof name, Context, Arguments, Return> => ({
Expand Down Expand Up @@ -249,15 +244,14 @@ export const esaggs = (): ExpressionFunction<typeof name, Context, Arguments, Re
const $injector = await chrome.dangerouslyGetActiveInjector();
const Private: Function = $injector.get('Private');
const { indexPatterns } = data.indexPatterns;
const SearchSourceClass = Private(SearchSourceProvider);
const queryFilter = Private(FilterBarQueryFilterProvider);

const aggConfigsState = JSON.parse(args.aggConfigs);
const indexPattern = await indexPatterns.get(args.index);
const aggs = new AggConfigs(indexPattern, aggConfigsState);

// we should move searchSource creation inside courier request handler
const searchSource = new SearchSourceClass();
const searchSource = new SearchSource();
searchSource.setField('index', indexPattern);
searchSource.setField('size', 0);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,13 +30,16 @@ export const visualization = () => ({
const { visData, visConfig, params } = config;
const visType = config.visType || visConfig.type;
const $injector = await chrome.dangerouslyGetActiveInjector();
const $rootScope = $injector.get('$rootScope') as any;
const Private = $injector.get('Private') as any;
const Vis = Private(VisProvider);

if (handlers.vis) {
// special case in visualize, we need to render first (without executing the expression), for maps to work
if (visConfig) {
handlers.vis.setCurrentState({ type: visType, params: visConfig });
$rootScope.$apply(() => {
handlers.vis.setCurrentState({ type: visType, params: visConfig });
});
}
} else {
handlers.vis = new Vis({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
* under the License.
*/

import { searchSourceMock } from '../../../../../ui/public/courier/search_source/mocks';
import { SavedObjectDashboard } from '../saved_dashboard/saved_dashboard';

export function getSavedDashboardMock(
Expand All @@ -26,10 +27,7 @@ export function getSavedDashboardMock(
id: '123',
title: 'my dashboard',
panelsJSON: '[]',
searchSource: {
getOwnField: (param: any) => param,
setField: () => {},
},
searchSource: searchSourceMock,
copyOnSave: false,
timeRestore: false,
timeTo: 'now',
Expand Down
14 changes: 10 additions & 4 deletions src/legacy/core_plugins/kibana/public/discover/angular/discover.js
Original file line number Diff line number Diff line change
Expand Up @@ -230,7 +230,10 @@ function discoverController(

// the saved savedSearch
const savedSearch = $route.current.locals.savedSearch;

let abortController;
$scope.$on('$destroy', () => {
if (abortController) abortController.abort();
savedSearch.destroy();
subscriptions.unsubscribe();
});
Expand Down Expand Up @@ -753,15 +756,18 @@ function discoverController(
$scope.updateTime();

// Abort any in-progress requests before fetching again
$scope.searchSource.cancelQueued();
if (abortController) abortController.abort();
abortController = new AbortController();

$scope.updateDataSource()
.then(setupVisualization)
.then(function () {
$state.save();
$scope.fetchStatus = fetchStatuses.LOADING;
logInspectorRequest();
return $scope.searchSource.fetch();
return $scope.searchSource.fetch({
abortSignal: abortController.signal
});
})
.then(onResults)
.catch((error) => {
Expand Down Expand Up @@ -1040,8 +1046,8 @@ function discoverController(
);
visSavedObject.vis = $scope.vis;

$scope.searchSource.onRequestStart((searchSource, searchRequest) => {
return $scope.vis.getAggConfig().onSearchRequestStart(searchSource, searchRequest);
$scope.searchSource.onRequestStart((searchSource, options) => {
return $scope.vis.getAggConfig().onSearchRequestStart(searchSource, options);
});

$scope.searchSource.setField('aggs', function () {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

import sinon from 'sinon';
import moment from 'moment';
import { SearchSource } from 'ui/courier';

export function createIndexPatternsStub() {
return {
Expand All @@ -31,7 +32,10 @@ export function createIndexPatternsStub() {
};
}

export function createSearchSourceStubProvider(hits, timeField) {
/**
* A stubbed search source with a `fetch` method that returns all of `_stubHits`.
*/
export function createSearchSourceStub(hits, timeField) {
const searchSourceStub = {
_stubHits: hits,
_stubTimeField: timeField,
Expand All @@ -41,13 +45,37 @@ export function createSearchSourceStubProvider(hits, timeField) {
}),
};

searchSourceStub.setParent = sinon.stub().returns(searchSourceStub);
searchSourceStub.setField = sinon.stub().returns(searchSourceStub);
searchSourceStub.getField = sinon.spy(key => {
searchSourceStub.setParent = sinon.stub(SearchSource.prototype, 'setParent').returns(searchSourceStub);
searchSourceStub.setField = sinon.stub(SearchSource.prototype, 'setField').returns(searchSourceStub);
searchSourceStub.getField = sinon.stub(SearchSource.prototype, 'getField').callsFake(key => {
const previousSetCall = searchSourceStub.setField.withArgs(key).lastCall;
return previousSetCall ? previousSetCall.args[1] : null;
});
searchSourceStub.fetch = sinon.spy(() => {
searchSourceStub.fetch = sinon.stub(SearchSource.prototype, 'fetch').callsFake(() => Promise.resolve({
hits: {
hits: searchSourceStub._stubHits,
total: searchSourceStub._stubHits.length,
},
}));

searchSourceStub._restore = () => {
searchSourceStub.setParent.restore();
searchSourceStub.setField.restore();
searchSourceStub.getField.restore();
searchSourceStub.fetch.restore();
};

return searchSourceStub;
}

/**
* A stubbed search source with a `fetch` method that returns a filtered set of `_stubHits`.
*/
export function createContextSearchSourceStub(hits, timeField = '@timestamp') {
const searchSourceStub = createSearchSourceStub(hits, timeField);

searchSourceStub.fetch.restore();
searchSourceStub.fetch = sinon.stub(SearchSource.prototype, 'fetch').callsFake(() => {
const timeField = searchSourceStub._stubTimeField;
const lastQuery = searchSourceStub.setField.withArgs('query').lastCall.args[1];
const timeRange = lastQuery.query.constant_score.filter.range[timeField];
Expand All @@ -71,7 +99,5 @@ export function createSearchSourceStubProvider(hits, timeField) {
});
});

return function SearchSourceStubProvider() {
return searchSourceStub;
};
return searchSourceStub;
}
Loading

0 comments on commit 3c7b160

Please sign in to comment.