From a20f1e96b296fa3bb1bd40d4718a302dda831e0d Mon Sep 17 00:00:00 2001 From: Constance Date: Tue, 2 Feb 2021 09:41:02 -0800 Subject: [PATCH] [App Search] DRY helper for encoding/decoding routes that can have special characters in params (#89811) * Add encodePathParam helper + update components that need it - Primarily document URLs & analytics queries (which uses generateEnginePath) * Add useDecodedParams helper + update components that need it - Documents titles & Analytics queries * [Misc] Change popout icon to eye - Feedback from Davey - the pages don't open in a new window, so shouldn't use the popout icon - Not strictly related but since we're touching these links anyway, I'm shoving it in (sorry) * Remove document detail decode test - now handled/tested by useDecodedParams helper * Add new generateEncodedPath helper - Should be used in place of generatePath * Update all instances of generatePath to generateEncodedPath for consistency across the App Search codebase * Fix failing tests due to extra encodeURI() done by generatePath * Add missing branch test for analytics query titles Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> --- .../__mocks__/react_router_history.mock.ts | 20 +++++--- .../app_search/__mocks__/engine_logic.mock.ts | 4 +- .../analytics_tables/shared_columns.tsx | 2 +- .../analytics/views/query_detail.test.tsx | 11 ++++- .../analytics/views/query_detail.tsx | 7 +-- .../documents/document_detail.test.tsx | 6 --- .../components/documents/document_detail.tsx | 8 +-- .../app_search/components/engine/utils.ts | 4 +- .../components/engines/engines_table.tsx | 4 +- .../app_search/components/result/result.tsx | 6 +-- .../utils/encode_path_params/index.test.ts | 49 +++++++++++++++++++ .../utils/encode_path_params/index.ts | 34 +++++++++++++ 12 files changed, 125 insertions(+), 30 deletions(-) create mode 100644 x-pack/plugins/enterprise_search/public/applications/app_search/utils/encode_path_params/index.test.ts create mode 100644 x-pack/plugins/enterprise_search/public/applications/app_search/utils/encode_path_params/index.ts diff --git a/x-pack/plugins/enterprise_search/public/applications/__mocks__/react_router_history.mock.ts b/x-pack/plugins/enterprise_search/public/applications/__mocks__/react_router_history.mock.ts index 1516aa9096eca0..d3c11d33fdbf7e 100644 --- a/x-pack/plugins/enterprise_search/public/applications/__mocks__/react_router_history.mock.ts +++ b/x-pack/plugins/enterprise_search/public/applications/__mocks__/react_router_history.mock.ts @@ -24,12 +24,20 @@ export const mockLocation = { state: {}, }; -jest.mock('react-router-dom', () => ({ - ...(jest.requireActual('react-router-dom') as object), - useHistory: jest.fn(() => mockHistory), - useLocation: jest.fn(() => mockLocation), - useParams: jest.fn(() => ({})), -})); +jest.mock('react-router-dom', () => { + const originalModule = jest.requireActual('react-router-dom'); + return { + ...originalModule, + useHistory: jest.fn(() => mockHistory), + useLocation: jest.fn(() => mockLocation), + useParams: jest.fn(() => ({})), + // Note: RR's generatePath() opinionatedly encodeURI()s paths (although this doesn't actually + // show up/affect the final browser URL). Since we already have a generateEncodedPath helper & + // RR is removing this behavior in history 5.0+, I'm mocking tests to remove the extra encoding + // for now to make reading generateEncodedPath URLs a little less of a pain + generatePath: jest.fn((path, params) => decodeURI(originalModule.generatePath(path, params))), + }; +}); /** * For example usage, @see public/applications/shared/react_router_helpers/eui_link.test.tsx diff --git a/x-pack/plugins/enterprise_search/public/applications/app_search/__mocks__/engine_logic.mock.ts b/x-pack/plugins/enterprise_search/public/applications/app_search/__mocks__/engine_logic.mock.ts index 6326a41c1d2ca4..edc87d7025c5d0 100644 --- a/x-pack/plugins/enterprise_search/public/applications/app_search/__mocks__/engine_logic.mock.ts +++ b/x-pack/plugins/enterprise_search/public/applications/app_search/__mocks__/engine_logic.mock.ts @@ -4,7 +4,7 @@ * you may not use this file except in compliance with the Elastic License. */ -import { generatePath } from 'react-router-dom'; +import { generateEncodedPath } from '../utils/encode_path_params'; export const mockEngineValues = { engineName: 'some-engine', @@ -12,7 +12,7 @@ export const mockEngineValues = { }; export const mockGenerateEnginePath = jest.fn((path, pathParams = {}) => - generatePath(path, { engineName: mockEngineValues.engineName, ...pathParams }) + generateEncodedPath(path, { engineName: mockEngineValues.engineName, ...pathParams }) ); jest.mock('../components/engine', () => ({ diff --git a/x-pack/plugins/enterprise_search/public/applications/app_search/components/analytics/components/analytics_tables/shared_columns.tsx b/x-pack/plugins/enterprise_search/public/applications/app_search/components/analytics/components/analytics_tables/shared_columns.tsx index 16743405e0b5e1..8546ee428b6815 100644 --- a/x-pack/plugins/enterprise_search/public/applications/app_search/components/analytics/components/analytics_tables/shared_columns.tsx +++ b/x-pack/plugins/enterprise_search/public/applications/app_search/components/analytics/components/analytics_tables/shared_columns.tsx @@ -56,7 +56,7 @@ export const ACTIONS_COLUMN = { { defaultMessage: 'View query analytics' } ), type: 'icon', - icon: 'popout', + icon: 'eye', color: 'primary', onClick: (item: Query | RecentQuery) => { const { navigateToUrl } = KibanaLogic.values; diff --git a/x-pack/plugins/enterprise_search/public/applications/app_search/components/analytics/views/query_detail.test.tsx b/x-pack/plugins/enterprise_search/public/applications/app_search/components/analytics/views/query_detail.test.tsx index 7705d342ecdce5..42f13a0631a0aa 100644 --- a/x-pack/plugins/enterprise_search/public/applications/app_search/components/analytics/views/query_detail.test.tsx +++ b/x-pack/plugins/enterprise_search/public/applications/app_search/components/analytics/views/query_detail.test.tsx @@ -13,6 +13,7 @@ import { shallow } from 'enzyme'; import { SetAppSearchChrome as SetPageChrome } from '../../../../shared/kibana_chrome'; +import { AnalyticsLayout } from '../analytics_layout'; import { AnalyticsCards, AnalyticsChart, QueryClicksTable } from '../components'; import { QueryDetail } from './'; @@ -20,7 +21,7 @@ describe('QueryDetail', () => { const mockBreadcrumbs = ['Engines', 'some-engine', 'Analytics']; beforeEach(() => { - (useParams as jest.Mock).mockReturnValueOnce({ query: 'some-query' }); + (useParams as jest.Mock).mockReturnValue({ query: 'some-query' }); setMockValues({ totalQueriesForQuery: 100, @@ -31,6 +32,7 @@ describe('QueryDetail', () => { it('renders', () => { const wrapper = shallow(); + expect(wrapper.find(AnalyticsLayout).prop('title')).toEqual('"some-query"'); expect(wrapper.find(SetPageChrome).prop('trail')).toEqual([ 'Engines', 'some-engine', @@ -43,4 +45,11 @@ describe('QueryDetail', () => { expect(wrapper.find(AnalyticsChart)).toHaveLength(1); expect(wrapper.find(QueryClicksTable)).toHaveLength(1); }); + + it('renders empty "" search titles correctly', () => { + (useParams as jest.Mock).mockReturnValue({ query: '""' }); + const wrapper = shallow(); + + expect(wrapper.find(AnalyticsLayout).prop('title')).toEqual('""'); + }); }); diff --git a/x-pack/plugins/enterprise_search/public/applications/app_search/components/analytics/views/query_detail.tsx b/x-pack/plugins/enterprise_search/public/applications/app_search/components/analytics/views/query_detail.tsx index d5d864f35f6819..0ec81f5caa9357 100644 --- a/x-pack/plugins/enterprise_search/public/applications/app_search/components/analytics/views/query_detail.tsx +++ b/x-pack/plugins/enterprise_search/public/applications/app_search/components/analytics/views/query_detail.tsx @@ -5,7 +5,6 @@ */ import React from 'react'; -import { useParams } from 'react-router-dom'; import { useValues } from 'kea'; import { i18n } from '@kbn/i18n'; @@ -13,6 +12,7 @@ import { EuiSpacer } from '@elastic/eui'; import { SetAppSearchChrome as SetPageChrome } from '../../../../shared/kibana_chrome'; import { BreadcrumbTrail } from '../../../../shared/kibana_chrome/generate_breadcrumbs'; +import { useDecodedParams } from '../../../utils/encode_path_params'; import { AnalyticsLayout } from '../analytics_layout'; import { AnalyticsSection, QueryClicksTable } from '../components'; @@ -27,14 +27,15 @@ interface Props { breadcrumbs: BreadcrumbTrail; } export const QueryDetail: React.FC = ({ breadcrumbs }) => { - const { query } = useParams() as { query: string }; + const { query } = useDecodedParams(); + const queryTitle = query === '""' ? query : `"${query}"`; const { totalQueriesForQuery, queriesPerDayForQuery, startDate, topClicksForQuery } = useValues( AnalyticsLogic ); return ( - + { expect(actions.deleteDocument).toHaveBeenCalledWith('1'); }); - - it('correctly decodes document IDs', () => { - (useParams as jest.Mock).mockReturnValueOnce({ documentId: 'hello%20world%20%26%3F!' }); - const wrapper = shallow(); - expect(wrapper.find('h1').text()).toEqual('Document: hello world &?!'); - }); }); diff --git a/x-pack/plugins/enterprise_search/public/applications/app_search/components/documents/document_detail.tsx b/x-pack/plugins/enterprise_search/public/applications/app_search/components/documents/document_detail.tsx index 1be7e6c53d3431..3fadda6165c9b3 100644 --- a/x-pack/plugins/enterprise_search/public/applications/app_search/components/documents/document_detail.tsx +++ b/x-pack/plugins/enterprise_search/public/applications/app_search/components/documents/document_detail.tsx @@ -23,6 +23,7 @@ import { i18n } from '@kbn/i18n'; import { Loading } from '../../../shared/loading'; import { SetAppSearchChrome as SetPageChrome } from '../../../shared/kibana_chrome'; import { FlashMessages } from '../../../shared/flash_messages'; +import { useDecodedParams } from '../../utils/encode_path_params'; import { ResultFieldValue } from '../result'; import { DocumentDetailLogic } from './document_detail_logic'; @@ -43,6 +44,7 @@ export const DocumentDetail: React.FC = ({ engineBreadcrumb }) => { const { deleteDocument, getDocumentDetails, setFields } = useActions(DocumentDetailLogic); const { documentId } = useParams() as { documentId: string }; + const { documentId: documentTitle } = useDecodedParams(); useEffect(() => { getDocumentDetails(documentId); @@ -74,13 +76,11 @@ export const DocumentDetail: React.FC = ({ engineBreadcrumb }) => { return ( <> - + -

{DOCUMENT_DETAIL_TITLE(decodeURIComponent(documentId))}

+

{DOCUMENT_DETAIL_TITLE(documentTitle)}

 diff --git a/x-pack/plugins/enterprise_search/public/applications/app_search/components/engine/utils.ts b/x-pack/plugins/enterprise_search/public/applications/app_search/components/engine/utils.ts index b7efcbb6e6b27a..8e197eb402ea7b 100644 --- a/x-pack/plugins/enterprise_search/public/applications/app_search/components/engine/utils.ts +++ b/x-pack/plugins/enterprise_search/public/applications/app_search/components/engine/utils.ts @@ -4,7 +4,7 @@ * you may not use this file except in compliance with the Elastic License. */ -import { generatePath } from 'react-router-dom'; +import { generateEncodedPath } from '../../utils/encode_path_params'; import { EngineLogic } from './'; @@ -13,5 +13,5 @@ import { EngineLogic } from './'; */ export const generateEnginePath = (path: string, pathParams: object = {}) => { const { engineName } = EngineLogic.values; - return generatePath(path, { engineName, ...pathParams }); + return generateEncodedPath(path, { engineName, ...pathParams }); }; diff --git a/x-pack/plugins/enterprise_search/public/applications/app_search/components/engines/engines_table.tsx b/x-pack/plugins/enterprise_search/public/applications/app_search/components/engines/engines_table.tsx index a9455b4a2306ab..34bf0fe1b3a04e 100644 --- a/x-pack/plugins/enterprise_search/public/applications/app_search/components/engines/engines_table.tsx +++ b/x-pack/plugins/enterprise_search/public/applications/app_search/components/engines/engines_table.tsx @@ -5,7 +5,6 @@ */ import React from 'react'; -import { generatePath } from 'react-router-dom'; import { useActions } from 'kea'; import { EuiBasicTable, EuiBasicTableColumn } from '@elastic/eui'; import { FormattedMessage, FormattedDate, FormattedNumber } from '@kbn/i18n/react'; @@ -13,6 +12,7 @@ import { i18n } from '@kbn/i18n'; import { TelemetryLogic } from '../../../shared/telemetry'; import { EuiLinkTo } from '../../../shared/react_router_helpers'; +import { generateEncodedPath } from '../../utils/encode_path_params'; import { ENGINE_PATH } from '../../routes'; import { ENGINES_PAGE_SIZE } from '../../../../../common/constants'; @@ -41,7 +41,7 @@ export const EnginesTable: React.FC = ({ const { sendAppSearchTelemetry } = useActions(TelemetryLogic); const engineLinkProps = (engineName: string) => ({ - to: generatePath(ENGINE_PATH, { engineName }), + to: generateEncodedPath(ENGINE_PATH, { engineName }), onClick: () => sendAppSearchTelemetry({ action: 'clicked', diff --git a/x-pack/plugins/enterprise_search/public/applications/app_search/components/result/result.tsx b/x-pack/plugins/enterprise_search/public/applications/app_search/components/result/result.tsx index a3935bb782f906..ff8b373f1bee33 100644 --- a/x-pack/plugins/enterprise_search/public/applications/app_search/components/result/result.tsx +++ b/x-pack/plugins/enterprise_search/public/applications/app_search/components/result/result.tsx @@ -5,7 +5,6 @@ */ import React, { useState, useMemo } from 'react'; -import { generatePath } from 'react-router-dom'; import classNames from 'classnames'; import './result.scss'; @@ -14,6 +13,7 @@ import { EuiPanel, EuiIcon } from '@elastic/eui'; import { i18n } from '@kbn/i18n'; import { ReactRouterHelper } from '../../../shared/react_router_helpers/eui_components'; +import { generateEncodedPath } from '../../utils/encode_path_params'; import { ENGINE_DOCUMENT_DETAIL_PATH } from '../../routes'; import { Schema } from '../../../shared/types'; @@ -52,7 +52,7 @@ export const Result: React.FC = ({ if (schemaForTypeHighlights) return schemaForTypeHighlights[fieldName]; }; - const documentLink = generatePath(ENGINE_DOCUMENT_DETAIL_PATH, { + const documentLink = generateEncodedPath(ENGINE_DOCUMENT_DETAIL_PATH, { engineName: resultMeta.engine, documentId: resultMeta.id, }); @@ -135,7 +135,7 @@ export const Result: React.FC = ({ { defaultMessage: 'Visit document details' } )} > - + )} diff --git a/x-pack/plugins/enterprise_search/public/applications/app_search/utils/encode_path_params/index.test.ts b/x-pack/plugins/enterprise_search/public/applications/app_search/utils/encode_path_params/index.test.ts new file mode 100644 index 00000000000000..f311909bdf5bec --- /dev/null +++ b/x-pack/plugins/enterprise_search/public/applications/app_search/utils/encode_path_params/index.test.ts @@ -0,0 +1,49 @@ +/* + * 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 '../../../__mocks__/react_router_history.mock'; +import { useParams } from 'react-router-dom'; + +import { encodePathParams, generateEncodedPath, useDecodedParams } from './'; + +describe('encodePathParams', () => { + it('encodeURIComponent()s all object values', () => { + const params = { + someValue: 'hello world???', + anotherValue: 'test!@#$%^&*[]/|;:"<>~`', + }; + expect(encodePathParams(params)).toEqual({ + someValue: 'hello%20world%3F%3F%3F', + anotherValue: 'test!%40%23%24%25%5E%26*%5B%5D%2F%7C%3B%3A%22%3C%3E~%60', + }); + }); +}); + +describe('generateEncodedPath', () => { + it('generates a react router path with encoded path parameters', () => { + expect( + generateEncodedPath('/values/:someValue/:anotherValue/new', { + someValue: 'hello world???', + anotherValue: 'test!@#$%^&*[]/|;:"<>~`', + }) + ).toEqual( + '/values/hello%20world%3F%3F%3F/test!%40%23%24%25%5E%26*%5B%5D%2F%7C%3B%3A%22%3C%3E~%60/new' + ); + }); +}); + +describe('useDecodedParams', () => { + it('decodeURIComponent()s all object values from useParams()', () => { + (useParams as jest.Mock).mockReturnValue({ + someValue: 'hello%20world%3F%3F%3F', + anotherValue: 'test!%40%23%24%25%5E%26*%5B%5D%2F%7C%3B%3A%22%3C%3E~%60', + }); + expect(useDecodedParams()).toEqual({ + someValue: 'hello world???', + anotherValue: 'test!@#$%^&*[]/|;:"<>~`', + }); + }); +}); diff --git a/x-pack/plugins/enterprise_search/public/applications/app_search/utils/encode_path_params/index.ts b/x-pack/plugins/enterprise_search/public/applications/app_search/utils/encode_path_params/index.ts new file mode 100644 index 00000000000000..c8934ba47fe454 --- /dev/null +++ b/x-pack/plugins/enterprise_search/public/applications/app_search/utils/encode_path_params/index.ts @@ -0,0 +1,34 @@ +/* + * 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 { generatePath, useParams } from 'react-router-dom'; + +type PathParams = Record; + +export const encodePathParams = (pathParams: PathParams) => { + const encodedParams: PathParams = {}; + + Object.entries(pathParams).map(([key, value]) => { + encodedParams[key] = encodeURIComponent(value); + }); + + return encodedParams; +}; + +export const generateEncodedPath = (path: string, pathParams: PathParams) => { + return generatePath(path, encodePathParams(pathParams)); +}; + +export const useDecodedParams = () => { + const decodedParams: PathParams = {}; + + const params = useParams(); + Object.entries(params).map(([key, value]) => { + decodedParams[key] = decodeURIComponent(value as string); + }); + + return decodedParams; +};