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.

Sign up for GitHub

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

Jump to bottom

Remove Angular from courier request handler #20032

Merged
merged 6 commits into from Jun 26, 2018
Merged

Remove Angular from courier request handler #20032

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
143d5d7
remove angular dependency from courier request handler
ppisljar Jun 19, 2018
6efb1b7
vis cleanup
ppisljar Jun 22, 2018
7a28adf
ups
ppisljar Jun 25, 2018
169c33e
adding searchsource back
ppisljar Jun 26, 2018
5601a4c
adding params to makeChild
ppisljar Jun 26, 2018
6188a4b
rebasing
ppisljar Jun 26, 2018
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
2 changes: 1 addition & 1 deletion src/core_plugins/kibana/public/discover/controllers/discover.js
View file
Open in desktop
Expand Up @@ -726,7 +726,7 @@ function discoverController(
});

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

$scope.searchSource.aggs(function () {
Expand Down
2 changes: 1 addition & 1 deletion src/core_plugins/tile_map/public/coordinate_maps_visualization.js
View file
Open in desktop
Expand Up @@ -185,7 +185,7 @@ export function CoordinateMapsVisualizationProvider(Notifier, Private) {
async getGeohashBounds() {
const agg = this._getGeoHashAgg();
if (agg) {
const searchSource = this.vis.API.createInheritedSearchSource(this.vis.searchSource);
const searchSource = this.vis.searchSource.makeChild();
Copy link
Contributor

Choose a reason for hiding this comment

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

The same here as below (yeah sorry for the mixed up order). We'll change the behavior here slightly, by calling the parent searchsource start handler when calling this. The handler will eventually call the modifyAggConfigOnSearchRequestStart of the individual aggregations. I am not sure if we want this to happen in this place already (and the one below). It seems to work fine, but I am wondering about some weird situations we might end up with, where we should not call that method "out of place". Maybe we could give the makeChild method an options, parameter to pass to the inherit method, so we can only enable callParentStartHandlers in the one place in courier request handler where it was also enabled beforehand?

searchSource.size(0);
searchSource.aggs(function () {
const geoBoundsAgg = new AggConfig(agg.vis, {
Expand Down
4 changes: 2 additions & 2 deletions src/ui/public/courier/data_source/search_source.js
View file
Open in desktop
Expand Up @@ -277,8 +277,8 @@ export function SearchSourceProvider(Promise, Private, config) {
return clone;
}

makeChild() {
return new SearchSource().inherits(this, { callParentStartHandlers: true });
makeChild(params) {
return new SearchSource().inherits(this, params);
}

new() {
Expand Down
3 changes: 3 additions & 0 deletions src/ui/public/registry/field_formats.js
View file
Open in desktop
Expand Up @@ -86,6 +86,9 @@ class FieldFormatRegistry extends IndexedArray {
*/
getInstance = _.memoize(function (formatId) {
const FieldFormat = this.byId[formatId];
if (!FieldFormat) {
throw new Error(`Field Format '${formatId}' not found!`);
}
return new FieldFormat(null, this.getConfig);
});

Expand Down
13 changes: 13 additions & 0 deletions src/ui/public/vis/agg_configs.js
View file
Open in desktop
Expand Up @@ -208,5 +208,18 @@ AggConfigs.prototype.getResponseAggById = function (id) {
if (!reqAgg) return;
return _.find(reqAgg.getResponseAggs(), { id: id });
};
/**
* Hook for pre-flight logic, see AggType#onSearchRequestStart()
* @param {Courier.SearchSource} searchSource
* @param {Courier.SearchRequest} searchRequest
* @return {Promise<undefined>}
*/
AggConfigs.prototype.onSearchRequestStart = function (searchSource, searchRequest) {
return Promise.all(
this.getRequestAggs().map(agg =>
agg.onSearchRequestStart(searchSource, searchRequest)
)
);
};

export { AggConfigs };
27 changes: 12 additions & 15 deletions src/ui/public/vis/request_handlers/courier.js
View file
Open in desktop
Expand Up @@ -18,17 +18,15 @@
*/

import _ from 'lodash';
import { SearchSourceProvider } from '../../courier/data_source/search_source';
import { VisRequestHandlersRegistryProvider } from '../../registry/vis_request_handlers';
import { calculateObjectHash } from '../lib/calculate_object_hash';
import { timefilter } from 'ui/timefilter';
import { getRequestInspectorStats, getResponseInspectorStats } from '../../courier/utils/courier_inspector_utils';
import { tabifyAggResponse } from '../../agg_response/tabify/tabify';

import { FormattedData } from '../../inspector/adapters';
import { getTime } from '../../timefilter/get_time';

const CourierRequestHandlerProvider = function (Private, courier) {
const SearchSource = Private(SearchSourceProvider);
const CourierRequestHandlerProvider = function () {

/**
* This function builds tabular data from the response and attaches it to the
Expand Down Expand Up @@ -74,7 +72,7 @@ const CourierRequestHandlerProvider = function (Private, courier) {

return {
name: 'courier',
handler: function (vis, { searchSource, timeRange, query, filters, forceFetch }) {
handler: function (vis, { searchSource, aggs, timeRange, query, filters, forceFetch }) {

// Create a new search source that inherits the original search source
// but has the propriate timeRange applied via a filter.
Expand All @@ -83,8 +81,8 @@ const CourierRequestHandlerProvider = function (Private, courier) {
// Using callParentStartHandlers: true we make sure, that the parent searchSource
// onSearchRequestStart will be called properly even though we use an inherited
// search source.
const timeFilterSearchSource = searchSource.makeChild();
const requestSearchSource = timeFilterSearchSource.makeChild();
const timeFilterSearchSource = searchSource.makeChild({ callParentStartHandlers: true });
const requestSearchSource = timeFilterSearchSource.makeChild({ callParentStartHandlers: true });

// For now we need to mirror the history of the passed search source, since
// the spy panel wouldn't work otherwise.
Expand All @@ -98,15 +96,15 @@ const CourierRequestHandlerProvider = function (Private, courier) {
});

requestSearchSource.aggs(function () {
return vis.getAggConfig().toDsl();
return aggs.toDsl();
});

requestSearchSource.onRequestStart((searchSource, searchRequest) => {
return vis.onSearchRequestStart(searchSource, searchRequest);
return aggs.onSearchRequestStart(searchSource, searchRequest);
});

timeFilterSearchSource.set('filter', () => {
return timefilter.createFilter(searchSource.get('index'), timeRange);
return getTime(searchSource.get('index'), timeRange);
});

requestSearchSource.set('filter', filters);
Expand All @@ -128,7 +126,7 @@ const CourierRequestHandlerProvider = function (Private, courier) {
});
request.stats(getRequestInspectorStats(requestSearchSource));

requestSearchSource.onResults().then(resp => {
requestSearchSource.fetch().then(resp => {
searchSource.lastQuery = queryHash;

request
Expand All @@ -138,10 +136,10 @@ const CourierRequestHandlerProvider = function (Private, courier) {
searchSource.rawResponse = resp;
return _.cloneDeep(resp);
}).then(async resp => {
for (const agg of vis.getAggConfig()) {
for (const agg of aggs) {
if (_.has(agg, 'type.postFlightRequest')) {
const nestedSearchSource = new SearchSource().inherits(requestSearchSource);
resp = await agg.type.postFlightRequest(resp, vis.aggs, agg, nestedSearchSource);
const nestedSearchSource = requestSearchSource.makeChild();
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to note that down: this will change the behavior slightly, since we now call all parents start handler (i.e. the one of requestSearchSource in case this nestedSearchSource will receive the requestIsStarting. I am not yet sure if that actually will ever happen of what that influences, but wanted to point it out for now.

resp = await agg.type.postFlightRequest(resp, aggs, agg, nestedSearchSource);
}
}

Expand All @@ -159,7 +157,6 @@ const CourierRequestHandlerProvider = function (Private, courier) {
request.json(req);
});

courier.fetch();
} else {
resolve(searchSource.finalResponse);
}
Expand Down
23 changes: 1 addition & 22 deletions src/ui/public/vis/vis.js
View file
Open in desktop
Expand Up @@ -36,7 +36,6 @@ import { onBrushEvent } from '../utils/brush_event';
import { FilterBarQueryFilterProvider } from '../filter_bar/query_filter';
import { FilterBarClickHandlerProvider } from '../filter_bar/filter_bar_click_handler';
import { updateVisualizationConfig } from './vis_update';
import { queryManagerFactory } from '../query_manager';
import { SearchSourceProvider } from '../courier/data_source/search_source';
import { SavedObjectsClientProvider } from '../saved_objects';
import { timefilter } from 'ui/timefilter';
Expand All @@ -56,7 +55,7 @@ const getTerms = (table, columnIndex, rowIndex) => {
}))];
};

export function VisProvider(Private, Promise, indexPatterns, getAppState) {
export function VisProvider(Private, indexPatterns, getAppState) {
const visTypes = Private(VisTypesRegistryProvider);
const queryFilter = Private(FilterBarQueryFilterProvider);
const filterBarClickHandler = Private(FilterBarClickHandlerProvider);
Expand Down Expand Up @@ -88,7 +87,6 @@ export function VisProvider(Private, Promise, indexPatterns, getAppState) {
indexPatterns: indexPatterns,
timeFilter: timefilter,
queryFilter: queryFilter,
queryManager: queryManagerFactory(getAppState),
Copy link
Member

Choose a reason for hiding this comment

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

Whoops! vis.API.queryManager is a documented Vis API feature: https://www.elastic.co/guide/en/kibana/current/development-vis-object.html Removing this broke integration for plugin authors.

Should this be re-added?

events: {
// the filter method will be removed in the near feature
// you should rather use addFilter method below
Expand All @@ -111,12 +109,6 @@ export function VisProvider(Private, Promise, indexPatterns, getAppState) {
onBrushEvent(event, getAppState());
}
},
createInheritedSearchSource: (parentSearchSource) => {
if (!parentSearchSource) {
throw new Error('Unable to inherit search source, visualize saved object does not have search source.');
}
return new SearchSource().inherits(parentSearchSource);
},
inspectorAdapters: this._getActiveInspectorAdapters(),
};
}
Expand Down Expand Up @@ -247,19 +239,6 @@ export function VisProvider(Private, Promise, indexPatterns, getAppState) {
return this.getStateInternal(true);
}

/**
* Hook for pre-flight logic, see AggType#onSearchRequestStart()
* @param {Courier.SearchSource} searchSource
* @param {Courier.SearchRequest} searchRequest
* @return {Promise<undefined>}
*/
onSearchRequestStart(searchSource, searchRequest) {
return Promise.map(
this.aggs.getRequestAggs(),
agg => agg.onSearchRequestStart(searchSource, searchRequest)
);
}

isHierarchical() {
if (_.isFunction(this.type.hierarchicalData)) {
return !!this.type.hierarchicalData(this);
Expand Down
1 change: 1 addition & 0 deletions src/ui/public/visualize/visualize.js
View file
Open in desktop
Expand Up @@ -99,6 +99,7 @@ uiModules
uiState: $scope.uiState,
queryFilter: queryFilter,
searchSource: $scope.savedObj.searchSource,
aggs: $scope.vis.getAggConfig(),
timeRange: $scope.timeRange,
filters: $scope.filters,
query: $scope.query,
Expand Down