From 5412b39063afa03b3541ddeff163ddae6ec17ab0 Mon Sep 17 00:00:00 2001 From: Walter Rafelsberger Date: Tue, 1 Dec 2020 09:28:34 +0100 Subject: [PATCH 1/5] [ML] Don't pass empty strings to useResolver. --- .../routes/data_frame_analytics/analytics_job_exploration.tsx | 2 +- .../routing/routes/data_frame_analytics/analytics_jobs_list.tsx | 2 +- .../routing/routes/data_frame_analytics/analytics_map.tsx | 2 +- .../routing/routes/data_frame_analytics/models_list.tsx | 2 +- .../application/routing/routes/datavisualizer/file_based.tsx | 2 +- .../ml/public/application/routing/routes/timeseriesexplorer.tsx | 2 +- 6 files changed, 6 insertions(+), 6 deletions(-) diff --git a/x-pack/plugins/ml/public/application/routing/routes/data_frame_analytics/analytics_job_exploration.tsx b/x-pack/plugins/ml/public/application/routing/routes/data_frame_analytics/analytics_job_exploration.tsx index e436ff468ccf0..771123532dcbf 100644 --- a/x-pack/plugins/ml/public/application/routing/routes/data_frame_analytics/analytics_job_exploration.tsx +++ b/x-pack/plugins/ml/public/application/routing/routes/data_frame_analytics/analytics_job_exploration.tsx @@ -38,7 +38,7 @@ export const analyticsJobExplorationRouteFactory = ( }); const PageWrapper: FC = ({ location, deps }) => { - const { context } = useResolver('', undefined, deps.config, basicResolvers(deps)); + const { context } = useResolver(undefined, undefined, deps.config, basicResolvers(deps)); const [globalState] = useUrlState('_g'); diff --git a/x-pack/plugins/ml/public/application/routing/routes/data_frame_analytics/analytics_jobs_list.tsx b/x-pack/plugins/ml/public/application/routing/routes/data_frame_analytics/analytics_jobs_list.tsx index 80706a82121d5..3b68c5078e99e 100644 --- a/x-pack/plugins/ml/public/application/routing/routes/data_frame_analytics/analytics_jobs_list.tsx +++ b/x-pack/plugins/ml/public/application/routing/routes/data_frame_analytics/analytics_jobs_list.tsx @@ -34,7 +34,7 @@ export const analyticsJobsListRouteFactory = ( }); const PageWrapper: FC = ({ location, deps }) => { - const { context } = useResolver('', undefined, deps.config, basicResolvers(deps)); + const { context } = useResolver(undefined, undefined, deps.config, basicResolvers(deps)); return ( diff --git a/x-pack/plugins/ml/public/application/routing/routes/data_frame_analytics/analytics_map.tsx b/x-pack/plugins/ml/public/application/routing/routes/data_frame_analytics/analytics_map.tsx index 18002648cfaa6..3acd12402932f 100644 --- a/x-pack/plugins/ml/public/application/routing/routes/data_frame_analytics/analytics_map.tsx +++ b/x-pack/plugins/ml/public/application/routing/routes/data_frame_analytics/analytics_map.tsx @@ -34,7 +34,7 @@ export const analyticsMapRouteFactory = ( }); const PageWrapper: FC = ({ deps }) => { - const { context } = useResolver('', undefined, deps.config, basicResolvers(deps)); + const { context } = useResolver(undefined, undefined, deps.config, basicResolvers(deps)); return ( diff --git a/x-pack/plugins/ml/public/application/routing/routes/data_frame_analytics/models_list.tsx b/x-pack/plugins/ml/public/application/routing/routes/data_frame_analytics/models_list.tsx index b1fd6e93a744c..2f58ef756e51c 100644 --- a/x-pack/plugins/ml/public/application/routing/routes/data_frame_analytics/models_list.tsx +++ b/x-pack/plugins/ml/public/application/routing/routes/data_frame_analytics/models_list.tsx @@ -34,7 +34,7 @@ export const modelsListRouteFactory = ( }); const PageWrapper: FC = ({ location, deps }) => { - const { context } = useResolver('', undefined, deps.config, basicResolvers(deps)); + const { context } = useResolver(undefined, undefined, deps.config, basicResolvers(deps)); return ( diff --git a/x-pack/plugins/ml/public/application/routing/routes/datavisualizer/file_based.tsx b/x-pack/plugins/ml/public/application/routing/routes/datavisualizer/file_based.tsx index 837616a8a76d2..f651d17e02de4 100644 --- a/x-pack/plugins/ml/public/application/routing/routes/datavisualizer/file_based.tsx +++ b/x-pack/plugins/ml/public/application/routing/routes/datavisualizer/file_based.tsx @@ -45,7 +45,7 @@ export const fileBasedRouteFactory = ( const PageWrapper: FC = ({ location, deps }) => { const { redirectToMlAccessDeniedPage } = deps; - const { context } = useResolver('', undefined, deps.config, { + const { context } = useResolver(undefined, undefined, deps.config, { checkBasicLicense, loadIndexPatterns: () => loadIndexPatterns(deps.indexPatterns), checkFindFileStructurePrivilege: () => diff --git a/x-pack/plugins/ml/public/application/routing/routes/timeseriesexplorer.tsx b/x-pack/plugins/ml/public/application/routing/routes/timeseriesexplorer.tsx index df92c77252565..7de59cba495af 100644 --- a/x-pack/plugins/ml/public/application/routing/routes/timeseriesexplorer.tsx +++ b/x-pack/plugins/ml/public/application/routing/routes/timeseriesexplorer.tsx @@ -63,7 +63,7 @@ export const timeSeriesExplorerRouteFactory = ( }); const PageWrapper: FC = ({ 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()), From 1af2323834204f7c8383b687b30137fc42a175a9 Mon Sep 17 00:00:00 2001 From: Walter Rafelsberger Date: Tue, 1 Dec 2020 11:35:42 +0100 Subject: [PATCH 2/5] [ML] Fix analytics results page. --- .../jobs/new_job/utils/new_job_utils.ts | 2 +- .../application/routing/use_resolver.ts | 65 +++++++++++-------- .../ml/public/application/util/index_utils.ts | 7 +- 3 files changed, 43 insertions(+), 31 deletions(-) diff --git a/x-pack/plugins/ml/public/application/jobs/new_job/utils/new_job_utils.ts b/x-pack/plugins/ml/public/application/jobs/new_job/utils/new_job_utils.ts index b7b6aa15ffe44..543e6898193a4 100644 --- a/x-pack/plugins/ml/public/application/jobs/new_job/utils/new_job_utils.ts +++ b/x-pack/plugins/ml/public/application/jobs/new_job/utils/new_job_utils.ts @@ -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 diff --git a/x-pack/plugins/ml/public/application/routing/use_resolver.ts b/x-pack/plugins/ml/public/application/routing/use_resolver.ts index e4cd90145bee4..ff212ff874b2a 100644 --- a/x-pack/plugins/ml/public/application/routing/use_resolver.ts +++ b/x-pack/plugins/ml/public/application/routing/use_resolver.ts @@ -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'; @@ -52,36 +53,44 @@ 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!) }; - - const { combinedQuery } = createSearchItems(config, indexPattern!, savedSearch); + 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. + 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(); } })(); }, []); diff --git a/x-pack/plugins/ml/public/application/util/index_utils.ts b/x-pack/plugins/ml/public/application/util/index_utils.ts index 42be3dd8252f9..de08553af9906 100644 --- a/x-pack/plugins/ml/public/application/util/index_utils.ts +++ b/x-pack/plugins/ml/public/application/util/index_utils.ts @@ -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, }; From 668e0b112549a0857a2901fc497c18ca72c1ca72 Mon Sep 17 00:00:00 2001 From: Walter Rafelsberger Date: Tue, 1 Dec 2020 13:11:08 +0100 Subject: [PATCH 3/5] [ML] Adds jest tests. --- .../application/routing/use_resolver.test.ts | 88 +++++++++++++++++++ .../application/routing/use_resolver.ts | 8 ++ 2 files changed, 96 insertions(+) create mode 100644 x-pack/plugins/ml/public/application/routing/use_resolver.test.ts diff --git a/x-pack/plugins/ml/public/application/routing/use_resolver.test.ts b/x-pack/plugins/ml/public/application/routing/use_resolver.test.ts new file mode 100644 index 0000000000000..93de859991957 --- /dev/null +++ b/x-pack/plugins/ml/public/application/routing/use_resolver.test.ts @@ -0,0 +1,88 @@ +/* + * 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); + }); + + 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); + }); +}); diff --git a/x-pack/plugins/ml/public/application/routing/use_resolver.ts b/x-pack/plugins/ml/public/application/routing/use_resolver.ts index ff212ff874b2a..ad514d08c55d9 100644 --- a/x-pack/plugins/ml/public/application/routing/use_resolver.ts +++ b/x-pack/plugins/ml/public/application/routing/use_resolver.ts @@ -54,6 +54,14 @@ export const useResolver = ( } try { + if (indexPatternId === '') { + throw new Error( + i18n.translate('xpack.ml.useResolver.errorIndexPatternIdEmptyString', { + defaultMessage: 'indexPatternId must not be empty string.', + }) + ); + } + // 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. From 2a916a97840056bac2d4472d827bca55134445b5 Mon Sep 17 00:00:00 2001 From: Walter Rafelsberger Date: Tue, 1 Dec 2020 15:26:52 +0100 Subject: [PATCH 4/5] add comment, extend test. --- .../ml/public/application/routing/use_resolver.test.ts | 1 + .../plugins/ml/public/application/routing/use_resolver.ts | 8 ++++++++ 2 files changed, 9 insertions(+) diff --git a/x-pack/plugins/ml/public/application/routing/use_resolver.test.ts b/x-pack/plugins/ml/public/application/routing/use_resolver.test.ts index 93de859991957..07cc038538745 100644 --- a/x-pack/plugins/ml/public/application/routing/use_resolver.test.ts +++ b/x-pack/plugins/ml/public/application/routing/use_resolver.test.ts @@ -74,6 +74,7 @@ describe('useResolver', () => { results: {}, }); expect(addError).toHaveBeenCalledTimes(0); + expect(redirectToJobsManagementPage).toHaveBeenCalledTimes(0); }); it('should add an error toast and redirect if indexPatternId is an empty string.', async () => { diff --git a/x-pack/plugins/ml/public/application/routing/use_resolver.ts b/x-pack/plugins/ml/public/application/routing/use_resolver.ts index ad514d08c55d9..96df101f5d154 100644 --- a/x-pack/plugins/ml/public/application/routing/use_resolver.ts +++ b/x-pack/plugins/ml/public/application/routing/use_resolver.ts @@ -20,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, From 7a106511af1c126cf6a7d3bc836a4229eb9bc675 Mon Sep 17 00:00:00 2001 From: Walter Rafelsberger Date: Tue, 1 Dec 2020 16:13:13 +0100 Subject: [PATCH 5/5] [ML] remove outdated comment --- x-pack/plugins/ml/public/application/routing/use_resolver.ts | 3 --- 1 file changed, 3 deletions(-) diff --git a/x-pack/plugins/ml/public/application/routing/use_resolver.ts b/x-pack/plugins/ml/public/application/routing/use_resolver.ts index 96df101f5d154..3ce23f8b8a19c 100644 --- a/x-pack/plugins/ml/public/application/routing/use_resolver.ts +++ b/x-pack/plugins/ml/public/application/routing/use_resolver.ts @@ -70,9 +70,6 @@ export const useResolver = ( ); } - // 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. let indexPatternAndSavedSearch: IndexPatternAndSavedSearch = { savedSearch: null, indexPattern: null,