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.

# Conflicts:
#	x-pack/plugins/ml/public/application/routing/routes/data_frame_analytics/analytics_job_exploration.tsx
#	x-pack/plugins/ml/public/application/routing/routes/data_frame_analytics/analytics_map.tsx
  • Loading branch information
walterra committed Dec 1, 2020
1 parent 55b7342 commit 7b49067
Show file tree
Hide file tree
Showing 9 changed files with 149 additions and 35 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 @@ -39,7 +39,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 { _g }: Record<string, any> = parse(location.search, { sort: false });

const urlGenerator = useMlUrlGenerator();
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 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 @@ -58,7 +58,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 7b49067

Please sign in to comment.