Skip to content

Commit

Permalink
[ML] Fix unnecessary trigger of wildcard field type search for ML plu…
Browse files Browse the repository at this point in the history
…gin routes. (elastic#84605)

Passing in an empty string '' to useResolver() would trigger a wild card search across all indices and fields, potentially causing a timeout and the page would fail to load. The following pages were affected: Single Metric Viewer, Data frame analytics models list, Data frame analytics jobs list, Data frame analytics exploration page, File Data Visualizer (Data visualizer - Import data from a log file). This PR fixes it by passing undefined instead of '' to useResolver to avoid calling _fields_for_wildcard with an empty pattern. Jest tests were added to cover the two parameter scenarios empty string/undefined.
  • Loading branch information
walterra committed Dec 1, 2020
1 parent d4cc408 commit 722b881
Show file tree
Hide file tree
Showing 10 changed files with 150 additions and 36 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ export function getDefaultDatafeedQuery() {

export function createSearchItems(
kibanaConfig: IUiSettingsClient,
indexPattern: IIndexPattern,
indexPattern: IIndexPattern | undefined,
savedSearch: SavedSearchSavedObject | null
) {
// query is only used by the data visualizer as it needs
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ export const analyticsJobExplorationRouteFactory = (
});

const PageWrapper: FC<PageProps> = ({ location, deps }) => {
const { context } = useResolver('', undefined, deps.config, basicResolvers(deps));
const { context } = useResolver(undefined, undefined, deps.config, basicResolvers(deps));

const [globalState] = useUrlState('_g');

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ export const analyticsJobsListRouteFactory = (
});

const PageWrapper: FC<PageProps> = ({ location, deps }) => {
const { context } = useResolver('', undefined, deps.config, basicResolvers(deps));
const { context } = useResolver(undefined, undefined, deps.config, basicResolvers(deps));
return (
<PageLoader context={context}>
<Page />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ export const analyticsMapRouteFactory = (
});

const PageWrapper: FC<PageProps> = ({ deps }) => {
const { context } = useResolver('', undefined, deps.config, basicResolvers(deps));
const { context } = useResolver(undefined, undefined, deps.config, basicResolvers(deps));

return (
<PageLoader context={context}>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ export const modelsListRouteFactory = (
});

const PageWrapper: FC<PageProps> = ({ location, deps }) => {
const { context } = useResolver('', undefined, deps.config, basicResolvers(deps));
const { context } = useResolver(undefined, undefined, deps.config, basicResolvers(deps));
return (
<PageLoader context={context}>
<Page />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ export const fileBasedRouteFactory = (
const PageWrapper: FC<PageProps> = ({ location, deps }) => {
const { redirectToMlAccessDeniedPage } = deps;

const { context } = useResolver('', undefined, deps.config, {
const { context } = useResolver(undefined, undefined, deps.config, {
checkBasicLicense,
loadIndexPatterns: () => loadIndexPatterns(deps.indexPatterns),
checkFindFileStructurePrivilege: () =>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ export const timeSeriesExplorerRouteFactory = (
});

const PageWrapper: FC<PageProps> = ({ deps }) => {
const { context, results } = useResolver('', undefined, deps.config, {
const { context, results } = useResolver(undefined, undefined, deps.config, {
...basicResolvers(deps),
jobs: mlJobService.loadJobsWrapper,
jobsWithTimeRange: () => ml.jobs.jobsWithTimerange(getDateFormatTz()),
Expand Down
89 changes: 89 additions & 0 deletions x-pack/plugins/ml/public/application/routing/use_resolver.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

import { renderHook, act } from '@testing-library/react-hooks';

import { IUiSettingsClient } from 'kibana/public';

import { useCreateAndNavigateToMlLink } from '../contexts/kibana/use_create_url';
import { useNotifications } from '../contexts/kibana';

import { useResolver } from './use_resolver';

jest.mock('../contexts/kibana/use_create_url', () => {
return {
useCreateAndNavigateToMlLink: jest.fn(),
};
});

jest.mock('../contexts/kibana', () => {
return {
useMlUrlGenerator: () => ({
createUrl: jest.fn(),
}),
useNavigateToPath: () => jest.fn(),
useNotifications: jest.fn(),
};
});

const addError = jest.fn();
(useNotifications as jest.Mock).mockImplementation(() => ({
toasts: { addSuccess: jest.fn(), addDanger: jest.fn(), addError },
}));

const redirectToJobsManagementPage = jest.fn(() => Promise.resolve());
(useCreateAndNavigateToMlLink as jest.Mock).mockImplementation(() => redirectToJobsManagementPage);

describe('useResolver', () => {
afterEach(() => {
jest.useFakeTimers();
});
afterEach(() => {
jest.advanceTimersByTime(0);
jest.useRealTimers();
});

it('should accept undefined as indexPatternId and savedSearchId.', async () => {
const { result, waitForNextUpdate } = renderHook(() =>
useResolver(undefined, undefined, {} as IUiSettingsClient, {})
);

await act(async () => {
await waitForNextUpdate();
});

expect(result.current).toStrictEqual({
context: {
combinedQuery: {
bool: {
must: [
{
match_all: {},
},
],
},
},
currentIndexPattern: null,
currentSavedSearch: null,
indexPatterns: null,
kibanaConfig: {},
},
results: {},
});
expect(addError).toHaveBeenCalledTimes(0);
expect(redirectToJobsManagementPage).toHaveBeenCalledTimes(0);
});

it('should add an error toast and redirect if indexPatternId is an empty string.', async () => {
const { result } = renderHook(() => useResolver('', undefined, {} as IUiSettingsClient, {}));

await act(async () => {});

expect(result.current).toStrictEqual({ context: null, results: {} });
expect(addError).toHaveBeenCalledTimes(1);
expect(redirectToJobsManagementPage).toHaveBeenCalledTimes(1);
});
});
76 changes: 49 additions & 27 deletions x-pack/plugins/ml/public/application/routing/use_resolver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {
getIndexPatternById,
getIndexPatternsContract,
getIndexPatternAndSavedSearch,
IndexPatternAndSavedSearch,
} from '../util/index_utils';
import { createSearchItems } from '../jobs/new_job/utils/new_job_utils';
import { ResolverResults, Resolvers } from './resolvers';
Expand All @@ -19,6 +20,14 @@ import { useNotifications } from '../contexts/kibana';
import { useCreateAndNavigateToMlLink } from '../contexts/kibana/use_create_url';
import { ML_PAGES } from '../../../common/constants/ml_url_generator';

/**
* Hook to resolve route specific requirements
* @param indexPatternId optional Kibana index pattern id, used for wizards
* @param savedSearchId optional Kibana saved search id, used for wizards
* @param config Kibana UI Settings
* @param resolvers an array of resolvers to be executed for the route
* @return { context, results } returns the ML context and resolver results
*/
export const useResolver = (
indexPatternId: string | undefined,
savedSearchId: string | undefined,
Expand Down Expand Up @@ -52,36 +61,49 @@ export const useResolver = (
return;
}

if (indexPatternId !== undefined || savedSearchId !== undefined) {
try {
// note, currently we're using our own kibana context that requires a current index pattern to be set
// this means, if the page uses this context, useResolver must be passed a string for the index pattern id
// and loadIndexPatterns must be part of the resolvers.
const { indexPattern, savedSearch } =
savedSearchId !== undefined
? await getIndexPatternAndSavedSearch(savedSearchId)
: { savedSearch: null, indexPattern: await getIndexPatternById(indexPatternId!) };
try {
if (indexPatternId === '') {
throw new Error(
i18n.translate('xpack.ml.useResolver.errorIndexPatternIdEmptyString', {
defaultMessage: 'indexPatternId must not be empty string.',
})
);
}

const { combinedQuery } = createSearchItems(config, indexPattern!, savedSearch);
let indexPatternAndSavedSearch: IndexPatternAndSavedSearch = {
savedSearch: null,
indexPattern: null,
};

setContext({
combinedQuery,
currentIndexPattern: indexPattern,
currentSavedSearch: savedSearch,
indexPatterns: getIndexPatternsContract()!,
kibanaConfig: config,
});
} catch (error) {
// an unexpected error has occurred. This could be caused by an incorrect index pattern or saved search ID
notifications.toasts.addError(new Error(error), {
title: i18n.translate('xpack.ml.useResolver.errorTitle', {
defaultMessage: 'An error has occurred',
}),
});
await redirectToJobsManagementPage();
if (savedSearchId !== undefined) {
indexPatternAndSavedSearch = await getIndexPatternAndSavedSearch(savedSearchId);
} else if (indexPatternId !== undefined) {
indexPatternAndSavedSearch.indexPattern = await getIndexPatternById(indexPatternId);
}
} else {
setContext({});

const { savedSearch, indexPattern } = indexPatternAndSavedSearch;

const { combinedQuery } = createSearchItems(
config,
indexPattern !== null ? indexPattern : undefined,
savedSearch
);

setContext({
combinedQuery,
currentIndexPattern: indexPattern,
currentSavedSearch: savedSearch,
indexPatterns: getIndexPatternsContract(),
kibanaConfig: config,
});
} catch (error) {
// an unexpected error has occurred. This could be caused by an incorrect index pattern or saved search ID
notifications.toasts.addError(new Error(error), {
title: i18n.translate('xpack.ml.useResolver.errorTitle', {
defaultMessage: 'An error has occurred',
}),
});
await redirectToJobsManagementPage();
}
})();
}, []);
Expand Down
7 changes: 5 additions & 2 deletions x-pack/plugins/ml/public/application/util/index_utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -73,9 +73,12 @@ export function getIndexPatternIdFromName(name: string) {
}
return null;
}

export interface IndexPatternAndSavedSearch {
savedSearch: SavedSearchSavedObject | null;
indexPattern: IIndexPattern | null;
}
export async function getIndexPatternAndSavedSearch(savedSearchId: string) {
const resp: { savedSearch: SavedSearchSavedObject | null; indexPattern: IIndexPattern | null } = {
const resp: IndexPatternAndSavedSearch = {
savedSearch: null,
indexPattern: null,
};
Expand Down

0 comments on commit 722b881

Please sign in to comment.