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

[discover] Less field list loading #147825

Merged
merged 6 commits into from
Dec 22, 2022
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions src/plugins/data_views/common/data_views/data_views.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,14 @@ describe('IndexPatterns', () => {
SOClientGetDelay = 0;
});

test('force field refresh', async () => {
const id = '1';
await indexPatterns.get(id);
expect(apiClient.getFieldsForWildcard).toBeCalledTimes(1);
await indexPatterns.get(id, undefined, true);
expect(apiClient.getFieldsForWildcard).toBeCalledTimes(2);
});

test('does cache ad-hoc data views', async () => {
const id = '1';

Expand Down
26 changes: 20 additions & 6 deletions src/plugins/data_views/common/data_views/data_views.ts
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,7 @@ export interface DataViewsServicePublicMethods {
* @param id - Id of the data view to get.
* @param displayErrors - If set false, API consumer is responsible for displaying and handling errors.
*/
get: (id: string, displayErrors?: boolean) => Promise<DataView>;
get: (id: string, displayErrors?: boolean, refreshFields?: boolean) => Promise<DataView>;
/**
* Get populated data view saved object cache.
*/
Expand All @@ -214,8 +214,9 @@ export interface DataViewsServicePublicMethods {
getDefaultId: () => Promise<string | null>;
/**
* Get default data view, if it doesn't exist, choose and save new default data view and return it.
* @param refreshFields - refresh field list when true
*/
getDefaultDataView: () => Promise<DataView | null>;
getDefaultDataView: (refreshFields?: boolean) => Promise<DataView | null>;
/**
* Get fields for data view
* @param dataView - Data view instance or spec
Expand Down Expand Up @@ -877,10 +878,22 @@ export class DataViewsService {
* Get an index pattern by id, cache optimized.
* @param id
* @param displayErrors - If set false, API consumer is responsible for displaying and handling errors.
* @param refreshFields - If set true, will fetch fields from the index pattern
*/
get = async (id: string, displayErrors: boolean = true): Promise<DataView> => {
get = async (
id: string,
displayErrors: boolean = true,
refreshFields = false
): Promise<DataView> => {
const dataViewFromCache = this.dataViewCache.get(id)?.then(async (dataView) => {
if (dataView && refreshFields) {
await this.refreshFields(dataView);
}
return dataView;
});

const indexPatternPromise =
this.dataViewCache.get(id) ||
dataViewFromCache ||
this.dataViewCache.set(id, this.getSavedObjectAndInit(id, displayErrors));

// don't cache failed requests
Expand Down Expand Up @@ -1135,9 +1148,10 @@ export class DataViewsService {
* another data view is selected as default and returned.
* If no possible data view found to become a default returns null.
*
* @param {boolean} refreshFields - if true, will refresh the fields of the default data view
* @returns default data view
*/
async getDefaultDataView(): Promise<DataView | null> {
async getDefaultDataView(refreshFields?: boolean): Promise<DataView | null> {
const patterns = await this.getIdsWithTitle();
let defaultId: string | undefined = await this.config.get('defaultIndex');
const exists = defaultId ? patterns.some((pattern) => pattern.id === defaultId) : false;
Expand All @@ -1158,7 +1172,7 @@ export class DataViewsService {
}

if (defaultId) {
return this.get(defaultId);
return this.get(defaultId, undefined, refreshFields);
} else {
return null;
}
Expand Down
1 change: 1 addition & 0 deletions src/plugins/discover/public/__mocks__/data_views.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ export function createDiscoverDataViewsMock() {
return Promise.reject('Invald');
}
},
getDefaultDataView: jest.fn(() => dataViewMock),
updateSavedObject: jest.fn(),
getIdsWithTitle: jest.fn(() => {
return Promise.resolve([dataViewMock, dataViewComplexMock, dataViewWithTimefieldMock]);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -93,13 +93,6 @@ export function DiscoverMainRoute(props: Props) {
return;
}

const defaultDataView = await data.dataViews.getDefaultDataView();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This functionality duplicates what dataViews.hasData is doing. If it doesn't work, it should be fixed.


if (!defaultDataView) {
setShowNoDataPage(true);
return;
}

const { appStateContainer } = getState({ history, savedSearch: nextSavedSearch, services });
const { index } = appStateContainer.getState();
const ip = await loadDataView(
Expand All @@ -111,7 +104,6 @@ export function DiscoverMainRoute(props: Props) {

const ipList = ip.list;
const dataViewData = resolveDataView(ip, nextSavedSearch.searchSource, toastNotifications);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

refreshFields has been added to the data view loading functions - previously the data view would be loaded, triggering the first field list load, and then it would be loaded again to make sure it was fresh.

await data.dataViews.refreshFields(dataViewData);
setDataViewList(ipList);

return dataViewData;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
* Side Public License, v 1.
*/

import { loadDataView, getFallbackDataViewId } from './resolve_data_view';
import { loadDataView } from './resolve_data_view';
import { dataViewsMock } from '../../../__mocks__/data_views';
import { dataViewMock } from '../../../__mocks__/data_view';
import { configMock } from '../../../__mocks__/config';
Expand All @@ -26,13 +26,4 @@ describe('Resolve data view tests', () => {
expect(result.stateValFound).toBe(false);
expect(result.stateVal).toBe(dataViewId);
});
test('getFallbackDataViewId with an empty dataViews array', async () => {
const result = await getFallbackDataViewId([], '');
expect(result).toBe('');
});
test('getFallbackDataViewId with an dataViews array', async () => {
const list = await dataViewsMock.getIdsWithTitle();
const result = await getFallbackDataViewId(list, '');
expect(result).toBe('the-data-view-id');
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -34,46 +34,6 @@ interface DataViewData {
stateValFound: boolean;
}

export function findDataViewById(
dataViews: DataViewListItem[],
id: string
): DataViewListItem | undefined {
if (!Array.isArray(dataViews) || !id) {
return;
}
return dataViews.find((o) => o.id === id);
}

/**
* Checks if the given defaultIndex exists and returns
* the first available data view id if not
*/
export function getFallbackDataViewId(
dataViews: DataViewListItem[],
defaultIndex: string = ''
): string {
if (defaultIndex && findDataViewById(dataViews, defaultIndex)) {
return defaultIndex;
}
return dataViews && dataViews[0]?.id ? dataViews[0].id : '';
}

/**
* A given data view id is checked for existence and a fallback is provided if it doesn't exist
* The provided defaultIndex is usually configured in Advanced Settings, if it's also invalid
* the first entry of the given list of dataViews is used
*/
export function getDataViewId(
id: string = '',
dataViews: DataViewListItem[] = [],
defaultIndex: string = ''
): string {
if (!id || !findDataViewById(dataViews, id)) {
return getFallbackDataViewId(dataViews, defaultIndex);
}
return id;
}

/**
* Function to load the given data view by id, providing a fallback if it doesn't exist
*/
Expand Down Expand Up @@ -104,9 +64,10 @@ export async function loadDataView(
fetchId = dataViewSpec.id!;
}

let fetchedDataView: DataView | undefined;
// try to fetch adhoc data view first
try {
const fetchedDataView = fetchId ? await dataViews.get(fetchId) : undefined;
fetchedDataView = fetchId ? await dataViews.get(fetchId) : undefined;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previously this code was loading a data view, checking if it was adhoc, then tossing the result. Now we hold on to it in case its the data view we wanted to load.

if (fetchedDataView && !fetchedDataView.isPersisted()) {
return {
list: dataViewList || [],
Expand All @@ -122,12 +83,14 @@ export async function loadDataView(
} catch (e) {}

// fetch persisted data view
const actualId = getDataViewId(fetchId, dataViewList, config.get('defaultIndex'));
return {
Copy link
Contributor Author

@mattkime mattkime Dec 20, 2022

Choose a reason for hiding this comment

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

We don't need getDataViewId because dataViews.getDefaultDataView does the work - it provides the default data view and if it doesn't exist then it sets another default and if there aren't any data views...well, thats what the hasData check is for.

list: dataViewList || [],
loaded: await dataViews.get(actualId),
loaded: fetchedDataView
? fetchedDataView
: // we can be certain that the data view exists due to an earlier hasData check
((await dataViews.getDefaultDataView()) as DataView),
stateVal: fetchId,
stateValFound: !!fetchId && actualId === fetchId,
stateValFound: !!fetchId && !!fetchedDataView,
};
}

Expand Down