Skip to content

Commit 5889e36

Browse files
authored
[ML] Fix unnecessary trigger of wildcard field type search for ML plugin routes. (#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.
1 parent 5420177 commit 5889e36

File tree

10 files changed

+150
-36
lines changed

10 files changed

+150
-36
lines changed

x-pack/plugins/ml/public/application/jobs/new_job/utils/new_job_utils.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ export function getDefaultDatafeedQuery() {
3030

3131
export function createSearchItems(
3232
kibanaConfig: IUiSettingsClient,
33-
indexPattern: IIndexPattern,
33+
indexPattern: IIndexPattern | undefined,
3434
savedSearch: SavedSearchSavedObject | null
3535
) {
3636
// query is only used by the data visualizer as it needs

x-pack/plugins/ml/public/application/routing/routes/data_frame_analytics/analytics_job_exploration.tsx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ export const analyticsJobExplorationRouteFactory = (
3838
});
3939

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

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

x-pack/plugins/ml/public/application/routing/routes/data_frame_analytics/analytics_jobs_list.tsx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ export const analyticsJobsListRouteFactory = (
3434
});
3535

3636
const PageWrapper: FC<PageProps> = ({ location, deps }) => {
37-
const { context } = useResolver('', undefined, deps.config, basicResolvers(deps));
37+
const { context } = useResolver(undefined, undefined, deps.config, basicResolvers(deps));
3838
return (
3939
<PageLoader context={context}>
4040
<Page />

x-pack/plugins/ml/public/application/routing/routes/data_frame_analytics/analytics_map.tsx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ export const analyticsMapRouteFactory = (
3434
});
3535

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

3939
return (
4040
<PageLoader context={context}>

x-pack/plugins/ml/public/application/routing/routes/data_frame_analytics/models_list.tsx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ export const modelsListRouteFactory = (
3434
});
3535

3636
const PageWrapper: FC<PageProps> = ({ location, deps }) => {
37-
const { context } = useResolver('', undefined, deps.config, basicResolvers(deps));
37+
const { context } = useResolver(undefined, undefined, deps.config, basicResolvers(deps));
3838
return (
3939
<PageLoader context={context}>
4040
<Page />

x-pack/plugins/ml/public/application/routing/routes/datavisualizer/file_based.tsx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ export const fileBasedRouteFactory = (
4545
const PageWrapper: FC<PageProps> = ({ location, deps }) => {
4646
const { redirectToMlAccessDeniedPage } = deps;
4747

48-
const { context } = useResolver('', undefined, deps.config, {
48+
const { context } = useResolver(undefined, undefined, deps.config, {
4949
checkBasicLicense,
5050
loadIndexPatterns: () => loadIndexPatterns(deps.indexPatterns),
5151
checkFindFileStructurePrivilege: () =>

x-pack/plugins/ml/public/application/routing/routes/timeseriesexplorer.tsx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,7 @@ export const timeSeriesExplorerRouteFactory = (
6363
});
6464

6565
const PageWrapper: FC<PageProps> = ({ deps }) => {
66-
const { context, results } = useResolver('', undefined, deps.config, {
66+
const { context, results } = useResolver(undefined, undefined, deps.config, {
6767
...basicResolvers(deps),
6868
jobs: mlJobService.loadJobsWrapper,
6969
jobsWithTimeRange: () => ml.jobs.jobsWithTimerange(getDateFormatTz()),
Lines changed: 89 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,89 @@
1+
/*
2+
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
3+
* or more contributor license agreements. Licensed under the Elastic License;
4+
* you may not use this file except in compliance with the Elastic License.
5+
*/
6+
7+
import { renderHook, act } from '@testing-library/react-hooks';
8+
9+
import { IUiSettingsClient } from 'kibana/public';
10+
11+
import { useCreateAndNavigateToMlLink } from '../contexts/kibana/use_create_url';
12+
import { useNotifications } from '../contexts/kibana';
13+
14+
import { useResolver } from './use_resolver';
15+
16+
jest.mock('../contexts/kibana/use_create_url', () => {
17+
return {
18+
useCreateAndNavigateToMlLink: jest.fn(),
19+
};
20+
});
21+
22+
jest.mock('../contexts/kibana', () => {
23+
return {
24+
useMlUrlGenerator: () => ({
25+
createUrl: jest.fn(),
26+
}),
27+
useNavigateToPath: () => jest.fn(),
28+
useNotifications: jest.fn(),
29+
};
30+
});
31+
32+
const addError = jest.fn();
33+
(useNotifications as jest.Mock).mockImplementation(() => ({
34+
toasts: { addSuccess: jest.fn(), addDanger: jest.fn(), addError },
35+
}));
36+
37+
const redirectToJobsManagementPage = jest.fn(() => Promise.resolve());
38+
(useCreateAndNavigateToMlLink as jest.Mock).mockImplementation(() => redirectToJobsManagementPage);
39+
40+
describe('useResolver', () => {
41+
afterEach(() => {
42+
jest.useFakeTimers();
43+
});
44+
afterEach(() => {
45+
jest.advanceTimersByTime(0);
46+
jest.useRealTimers();
47+
});
48+
49+
it('should accept undefined as indexPatternId and savedSearchId.', async () => {
50+
const { result, waitForNextUpdate } = renderHook(() =>
51+
useResolver(undefined, undefined, {} as IUiSettingsClient, {})
52+
);
53+
54+
await act(async () => {
55+
await waitForNextUpdate();
56+
});
57+
58+
expect(result.current).toStrictEqual({
59+
context: {
60+
combinedQuery: {
61+
bool: {
62+
must: [
63+
{
64+
match_all: {},
65+
},
66+
],
67+
},
68+
},
69+
currentIndexPattern: null,
70+
currentSavedSearch: null,
71+
indexPatterns: null,
72+
kibanaConfig: {},
73+
},
74+
results: {},
75+
});
76+
expect(addError).toHaveBeenCalledTimes(0);
77+
expect(redirectToJobsManagementPage).toHaveBeenCalledTimes(0);
78+
});
79+
80+
it('should add an error toast and redirect if indexPatternId is an empty string.', async () => {
81+
const { result } = renderHook(() => useResolver('', undefined, {} as IUiSettingsClient, {}));
82+
83+
await act(async () => {});
84+
85+
expect(result.current).toStrictEqual({ context: null, results: {} });
86+
expect(addError).toHaveBeenCalledTimes(1);
87+
expect(redirectToJobsManagementPage).toHaveBeenCalledTimes(1);
88+
});
89+
});

x-pack/plugins/ml/public/application/routing/use_resolver.ts

Lines changed: 49 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import {
1111
getIndexPatternById,
1212
getIndexPatternsContract,
1313
getIndexPatternAndSavedSearch,
14+
IndexPatternAndSavedSearch,
1415
} from '../util/index_utils';
1516
import { createSearchItems } from '../jobs/new_job/utils/new_job_utils';
1617
import { ResolverResults, Resolvers } from './resolvers';
@@ -19,6 +20,14 @@ import { useNotifications } from '../contexts/kibana';
1920
import { useCreateAndNavigateToMlLink } from '../contexts/kibana/use_create_url';
2021
import { ML_PAGES } from '../../../common/constants/ml_url_generator';
2122

23+
/**
24+
* Hook to resolve route specific requirements
25+
* @param indexPatternId optional Kibana index pattern id, used for wizards
26+
* @param savedSearchId optional Kibana saved search id, used for wizards
27+
* @param config Kibana UI Settings
28+
* @param resolvers an array of resolvers to be executed for the route
29+
* @return { context, results } returns the ML context and resolver results
30+
*/
2231
export const useResolver = (
2332
indexPatternId: string | undefined,
2433
savedSearchId: string | undefined,
@@ -52,36 +61,49 @@ export const useResolver = (
5261
return;
5362
}
5463

55-
if (indexPatternId !== undefined || savedSearchId !== undefined) {
56-
try {
57-
// note, currently we're using our own kibana context that requires a current index pattern to be set
58-
// this means, if the page uses this context, useResolver must be passed a string for the index pattern id
59-
// and loadIndexPatterns must be part of the resolvers.
60-
const { indexPattern, savedSearch } =
61-
savedSearchId !== undefined
62-
? await getIndexPatternAndSavedSearch(savedSearchId)
63-
: { savedSearch: null, indexPattern: await getIndexPatternById(indexPatternId!) };
64+
try {
65+
if (indexPatternId === '') {
66+
throw new Error(
67+
i18n.translate('xpack.ml.useResolver.errorIndexPatternIdEmptyString', {
68+
defaultMessage: 'indexPatternId must not be empty string.',
69+
})
70+
);
71+
}
6472

65-
const { combinedQuery } = createSearchItems(config, indexPattern!, savedSearch);
73+
let indexPatternAndSavedSearch: IndexPatternAndSavedSearch = {
74+
savedSearch: null,
75+
indexPattern: null,
76+
};
6677

67-
setContext({
68-
combinedQuery,
69-
currentIndexPattern: indexPattern,
70-
currentSavedSearch: savedSearch,
71-
indexPatterns: getIndexPatternsContract()!,
72-
kibanaConfig: config,
73-
});
74-
} catch (error) {
75-
// an unexpected error has occurred. This could be caused by an incorrect index pattern or saved search ID
76-
notifications.toasts.addError(new Error(error), {
77-
title: i18n.translate('xpack.ml.useResolver.errorTitle', {
78-
defaultMessage: 'An error has occurred',
79-
}),
80-
});
81-
await redirectToJobsManagementPage();
78+
if (savedSearchId !== undefined) {
79+
indexPatternAndSavedSearch = await getIndexPatternAndSavedSearch(savedSearchId);
80+
} else if (indexPatternId !== undefined) {
81+
indexPatternAndSavedSearch.indexPattern = await getIndexPatternById(indexPatternId);
8282
}
83-
} else {
84-
setContext({});
83+
84+
const { savedSearch, indexPattern } = indexPatternAndSavedSearch;
85+
86+
const { combinedQuery } = createSearchItems(
87+
config,
88+
indexPattern !== null ? indexPattern : undefined,
89+
savedSearch
90+
);
91+
92+
setContext({
93+
combinedQuery,
94+
currentIndexPattern: indexPattern,
95+
currentSavedSearch: savedSearch,
96+
indexPatterns: getIndexPatternsContract(),
97+
kibanaConfig: config,
98+
});
99+
} catch (error) {
100+
// an unexpected error has occurred. This could be caused by an incorrect index pattern or saved search ID
101+
notifications.toasts.addError(new Error(error), {
102+
title: i18n.translate('xpack.ml.useResolver.errorTitle', {
103+
defaultMessage: 'An error has occurred',
104+
}),
105+
});
106+
await redirectToJobsManagementPage();
85107
}
86108
})();
87109
}, []);

x-pack/plugins/ml/public/application/util/index_utils.ts

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -73,9 +73,12 @@ export function getIndexPatternIdFromName(name: string) {
7373
}
7474
return null;
7575
}
76-
76+
export interface IndexPatternAndSavedSearch {
77+
savedSearch: SavedSearchSavedObject | null;
78+
indexPattern: IIndexPattern | null;
79+
}
7780
export async function getIndexPatternAndSavedSearch(savedSearchId: string) {
78-
const resp: { savedSearch: SavedSearchSavedObject | null; indexPattern: IIndexPattern | null } = {
81+
const resp: IndexPatternAndSavedSearch = {
7982
savedSearch: null,
8083
indexPattern: null,
8184
};

0 commit comments

Comments
 (0)