Skip to content

Commit

Permalink
Simplify Courier interface and organize internals (#20060)
Browse files Browse the repository at this point in the history
**Interface changes**

There are two goals behind the interface changes:

* Make it clearer which courier modules are meant for public consumption by exporting them from the top level.
* Simplify the courier object by removing responsibilities and focusing its responsibility solely on scheduling search requests via the fetch method and timefilter.refreshInterval Angular event.

I did this by taking the following steps:

* Removing redirectWhenMissing, indexPatterns, SearchSource, and SavedObject from the courier object. I also removed some unused methods from its interface.
* redirectWhenMissing is now a service registered on the kibana/url Angular module.
* indexPatterns is now a service registered on the kibana/index_patterns Angular module.
* SearchSourceProvider and SavedObjectProvider are now top-level exports of ui/courier.
* migrateFilter, decorateQuery, buildQueryFromFilters, and luceneStringToDsl are now top-level exports of ui/courier.

**Internal changes**

I also made some internal changes in an effort to organize the code clearly and reduce unnecessary complexity.

* I refactored the async code in CallClient to appear sync with async/await and encapsulated chunks of logic in helper functions. I also used an isAborted flag instead of overwriting the esPromise var with an enum.
* I combined Looper and SearchLooper into a single class and deleted unused functions.
* I reorganized the courier/fetch/request code into SearchRequest, SegmentedSearchRequest, and serializeFetchParams modules.
* Renamed various other methods and variables to improve clarity.
  • Loading branch information
cjcenizal authored Jun 27, 2018
1 parent c8185cf commit 3ea4c3e
Show file tree
Hide file tree
Showing 100 changed files with 632 additions and 605 deletions.
14 changes: 6 additions & 8 deletions src/core_plugins/kibana/public/context/api/__tests__/_stubs.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,15 +19,13 @@

import sinon from 'sinon';

export function createCourierStub() {
export function createIndexPatternsStub() {
return {
indexPatterns: {
get: sinon.spy(indexPatternId =>
Promise.resolve({
id: indexPatternId,
})
),
},
get: sinon.spy(indexPatternId =>
Promise.resolve({
id: indexPatternId,
})
),
};
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,8 @@ import expect from 'expect.js';
import ngMock from 'ng_mock';
import sinon from 'sinon';

import { createCourierStub } from './_stubs';
import { SearchSourceProvider } from 'ui/courier/data_source/search_source';
import { createIndexPatternsStub } from './_stubs';
import { SearchSourceProvider } from 'ui/courier';

import { fetchAnchorProvider } from '../anchor';

Expand All @@ -35,7 +35,7 @@ describe('context app', function () {
let SearchSourceStub;

beforeEach(ngMock.module(function createServiceStubs($provide) {
$provide.value('courier', createCourierStub());
$provide.value('indexPatterns', createIndexPatternsStub());
}));

beforeEach(ngMock.inject(function createPrivateStubs(Private) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,8 @@ import expect from 'expect.js';
import ngMock from 'ng_mock';
import * as _ from 'lodash';

import { createCourierStub, createSearchSourceStubProvider } from './_stubs';
import { SearchSourceProvider } from 'ui/courier/data_source/search_source';
import { createIndexPatternsStub, createSearchSourceStubProvider } from './_stubs';
import { SearchSourceProvider } from 'ui/courier';

import { fetchContextProvider } from '../context';

Expand All @@ -36,7 +36,7 @@ describe('context app', function () {
let getSearchSourceStub;

beforeEach(ngMock.module(function createServiceStubs($provide) {
$provide.value('courier', createCourierStub());
$provide.value('indexPatterns', createIndexPatternsStub());
}));

beforeEach(ngMock.inject(function createPrivateStubs(Private) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,8 @@ import expect from 'expect.js';
import ngMock from 'ng_mock';
import * as _ from 'lodash';

import { createCourierStub, createSearchSourceStubProvider } from './_stubs';
import { SearchSourceProvider } from 'ui/courier/data_source/search_source';
import { createIndexPatternsStub, createSearchSourceStubProvider } from './_stubs';
import { SearchSourceProvider } from 'ui/courier';

import { fetchContextProvider } from '../context';

Expand All @@ -36,7 +36,7 @@ describe('context app', function () {
let getSearchSourceStub;

beforeEach(ngMock.module(function createServiceStubs($provide) {
$provide.value('courier', createCourierStub());
$provide.value('indexPatterns', createIndexPatternsStub());
}));

beforeEach(ngMock.inject(function createPrivateStubs(Private) {
Expand Down
7 changes: 3 additions & 4 deletions src/core_plugins/kibana/public/context/api/anchor.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,9 @@

import _ from 'lodash';

import { SearchSourceProvider } from 'ui/courier/data_source/search_source';
import { SearchSourceProvider } from 'ui/courier';

export function fetchAnchorProvider(courier, Private) {
export function fetchAnchorProvider(indexPatterns, Private) {
const SearchSource = Private(SearchSourceProvider);

return async function fetchAnchor(
Expand All @@ -30,8 +30,7 @@ export function fetchAnchorProvider(courier, Private) {
anchorId,
sort
) {
const indexPattern = await courier.indexPatterns.get(indexPatternId);

const indexPattern = await indexPatterns.get(indexPatternId);
const searchSource = new SearchSource()
.inherits(false)
.set('index', indexPattern)
Expand Down
6 changes: 3 additions & 3 deletions src/core_plugins/kibana/public/context/api/context.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@


// @ts-ignore
import { SearchSourceProvider } from 'ui/courier/data_source/search_source';
import { SearchSourceProvider } from 'ui/courier';

import { reverseSortDirection } from './utils/sorting';

Expand All @@ -46,7 +46,7 @@ const DAY_MILLIS = 24 * 60 * 60 * 1000;
// look from 1 day up to 10000 days into the past and future
const LOOKUP_OFFSETS = [0, 1, 7, 30, 365, 10000].map((days) => days * DAY_MILLIS);

function fetchContextProvider(courier, Private) {
function fetchContextProvider(indexPatterns, Private) {
/**
* @type {{new(): SearchSourceT}}
*/
Expand Down Expand Up @@ -159,7 +159,7 @@ function fetchContextProvider(courier, Private) {
* @returns {Promise<Object>}
*/
async function createSearchSource(indexPatternId, filters) {
const indexPattern = await courier.indexPatterns.get(indexPatternId);
const indexPattern = await indexPatterns.get(indexPatternId);

return new SearchSource()
.inherits(false)
Expand Down
4 changes: 2 additions & 2 deletions src/core_plugins/kibana/public/context/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,8 @@ uiRoutes
controller: ContextAppRouteController,
controllerAs: 'contextAppRoute',
resolve: {
indexPattern: function ($route, courier) {
return courier.indexPatterns.get($route.current.params.indexPatternId);
indexPattern: function ($route, indexPatterns) {
return indexPatterns.get($route.current.params.indexPatternId);
},
},
template: contextAppRouteTemplate,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ import {
} from './constants';


export function QueryParameterActionsProvider(courier, Private) {
export function QueryParameterActionsProvider(indexPatterns, Private) {
const filterManager = Private(FilterManagerProvider);

const setPredecessorCount = (state) => (predecessorCount) => (
Expand Down Expand Up @@ -68,7 +68,7 @@ export function QueryParameterActionsProvider(courier, Private) {
const addFilter = (state) => async (field, values, operation) => {
const indexPatternId = state.queryParameters.indexPatternId;
filterManager.add(field, values, operation, indexPatternId);
const indexPattern = await courier.indexPatterns.get(indexPatternId);
const indexPattern = await indexPatterns.get(indexPatternId);
indexPattern.popularizeField(field.name, 1);
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,7 @@
import _ from 'lodash';
import { FilterBarQueryFilterProvider } from 'ui/filter_bar/query_filter';
import 'ui/state_management/app_state';
import { luceneStringToDsl } from '../../../../ui/public/courier/data_source/build_query/lucene_string_to_dsl';
import { migrateFilter } from 'ui/courier/data_source/_migrate_filter';
import { luceneStringToDsl, migrateFilter } from 'ui/courier';

export function dashboardContextProvider(Private, getAppState) {
return () => {
Expand Down
12 changes: 6 additions & 6 deletions src/core_plugins/kibana/public/dashboard/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ uiRoutes
$scope.initialFilter = ($location.search()).filter || EMPTY_FILTER;
},
resolve: {
dash: function ($route, Private, courier, kbnUrl) {
dash: function ($route, Private, redirectWhenMissing, kbnUrl) {
const savedObjectsClient = Private(SavedObjectsClientProvider);
const title = $route.current.params.title;
if (title) {
Expand All @@ -84,7 +84,7 @@ uiRoutes
kbnUrl.redirect(`${DashboardConstants.LANDING_PAGE_PATH}?filter="${title}"`);
}
throw uiRoutes.WAIT_FOR_URL_CHANGE_TOKEN;
}).catch(courier.redirectWhenMissing({
}).catch(redirectWhenMissing({
'dashboard': DashboardConstants.LANDING_PAGE_PATH
}));
}
Expand All @@ -94,9 +94,9 @@ uiRoutes
.when(DashboardConstants.CREATE_NEW_DASHBOARD_URL, {
template: dashboardTemplate,
resolve: {
dash: function (savedDashboards, courier) {
dash: function (savedDashboards, redirectWhenMissing) {
return savedDashboards.get()
.catch(courier.redirectWhenMissing({
.catch(redirectWhenMissing({
'dashboard': DashboardConstants.LANDING_PAGE_PATH
}));
}
Expand All @@ -105,7 +105,7 @@ uiRoutes
.when(createDashboardEditUrl(':id'), {
template: dashboardTemplate,
resolve: {
dash: function (savedDashboards, Notifier, $route, $location, courier, kbnUrl, AppState) {
dash: function (savedDashboards, Notifier, $route, $location, redirectWhenMissing, kbnUrl, AppState) {
const id = $route.current.params.id;

return savedDashboards.get(id)
Expand All @@ -124,7 +124,7 @@ uiRoutes
throw error;
}
})
.catch(courier.redirectWhenMissing({
.catch(redirectWhenMissing({
'dashboard': DashboardConstants.LANDING_PAGE_PATH
}));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,14 +21,16 @@ import angular from 'angular';
import { uiModules } from 'ui/modules';
import { createDashboardEditUrl } from '../dashboard_constants';
import { createLegacyClass } from 'ui/utils/legacy_class';
import { SavedObjectProvider } from 'ui/courier';

const module = uiModules.get('app/dashboard');

// Used only by the savedDashboards service, usually no reason to change this
module.factory('SavedDashboard', function (courier, config) {
module.factory('SavedDashboard', function (Private, config) {
// SavedDashboard constructor. Usually you'd interact with an instance of this.
// ID is option, without it one will be generated on save.
createLegacyClass(SavedDashboard).inherits(courier.SavedObject);
const SavedObject = Private(SavedObjectProvider);
createLegacyClass(SavedDashboard).inherits(SavedObject);
function SavedDashboard(id) {
// Gives our SavedDashboard the properties of a SavedObject
SavedDashboard.Super.call(this, {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ import '../components/fetch_error';
const app = uiModules.get('apps/discover', [
'kibana/notify',
'kibana/courier',
'kibana/url',
'kibana/index_patterns'
]);

Expand All @@ -68,7 +69,7 @@ uiRoutes
template: indexTemplate,
reloadOnSearch: false,
resolve: {
ip: function (Promise, courier, config, $location, Private) {
ip: function (Promise, indexPatterns, config, $location, Private) {
const State = Private(StateProvider);
const savedObjectsClient = Private(SavedObjectsClientProvider);

Expand Down Expand Up @@ -96,13 +97,13 @@ uiRoutes

return Promise.props({
list: savedObjects,
loaded: courier.indexPatterns.get(id),
loaded: indexPatterns.get(id),
stateVal: state.index,
stateValFound: specified && exists
});
});
},
savedSearch: function (courier, savedSearches, $route) {
savedSearch: function (redirectWhenMissing, savedSearches, $route) {
const savedSearchId = $route.current.params.id;
return savedSearches.get(savedSearchId)
.then((savedSearch) => {
Expand All @@ -114,7 +115,7 @@ uiRoutes
}
return savedSearch;
})
.catch(courier.redirectWhenMissing({
.catch(redirectWhenMissing({
'search': '/discover',
'index-pattern': '/management/kibana/objects/savedSearches/' + $route.current.params.id
}));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,16 +20,18 @@
import 'ui/notify';
import { uiModules } from 'ui/modules';
import { createLegacyClass } from 'ui/utils/legacy_class';
import { SavedObjectProvider } from 'ui/courier';

const module = uiModules.get('discover/saved_searches', [
'kibana/notify',
'kibana/courier'
]);

module.factory('SavedSearch', function (courier) {
createLegacyClass(SavedSearch).inherits(courier.SavedObject);
module.factory('SavedSearch', function (Private) {
const SavedObject = Private(SavedObjectProvider);
createLegacyClass(SavedSearch).inherits(SavedObject);
function SavedSearch(id) {
courier.SavedObject.call(this, {
SavedObject.call(this, {
type: SavedSearch.type,
mapping: SavedSearch.mapping,
searchSource: SavedSearch.searchSource,
Expand Down
4 changes: 2 additions & 2 deletions src/core_plugins/kibana/public/doc/controllers/doc.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,8 @@ const app = uiModules.get('apps/doc', [


const resolveIndexPattern = {
indexPattern: function (courier, savedSearches, $route) {
return courier.indexPatterns.get($route.current.params.indexPattern);
indexPattern: function (indexPatterns, savedSearches, $route) {
return indexPatterns.get($route.current.params.indexPattern);
}
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -147,10 +147,10 @@ uiRoutes
.when('/management/kibana/indices/:indexPatternId', {
template,
resolve: {
indexPattern: function ($route, courier) {
return courier.indexPatterns
indexPattern: function ($route, redirectWhenMissing, indexPatterns) {
return indexPatterns
.get($route.current.params.indexPatternId)
.catch(courier.redirectWhenMissing('/management/kibana/index'));
.catch(redirectWhenMissing('/management/kibana/index'));
}
}
});
Expand All @@ -173,7 +173,7 @@ uiRoutes

uiModules.get('apps/management')
.controller('managementIndicesEdit', function (
$scope, $location, $route, config, courier, Notifier, Private, AppState, docTitle, confirmModal) {
$scope, $location, $route, config, indexPatterns, Notifier, Private, AppState, docTitle, confirmModal) {
const notify = new Notifier();
const $state = $scope.state = new AppState();
const { fieldWildcardMatcher } = Private(FieldWildcardProvider);
Expand Down Expand Up @@ -257,7 +257,7 @@ uiModules.get('apps/management')
}
}

courier.indexPatterns.delete($scope.indexPattern)
indexPatterns.delete($scope.indexPattern)
.then(function () {
$location.url('/management/kibana/index');
})
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,9 +43,9 @@ uiRoutes
});
},
resolve: {
indexPattern: function ($route, courier) {
return courier.indexPatterns.get($route.current.params.indexPatternId)
.catch(courier.redirectWhenMissing('/management/kibana/indices'));
indexPattern: function ($route, redirectWhenMissing, indexPatterns) {
return indexPatterns.get($route.current.params.indexPatternId)
.catch(redirectWhenMissing('/management/kibana/indices'));
}
},
controllerAs: 'fieldSettings',
Expand Down
Loading

0 comments on commit 3ea4c3e

Please sign in to comment.