diff --git a/changelog/unreleased/issue-22105.toml b/changelog/unreleased/issue-22105.toml new file mode 100644 index 000000000000..1ff6a7452e8b --- /dev/null +++ b/changelog/unreleased/issue-22105.toml @@ -0,0 +1,5 @@ +type = "f" +message = "Fix browser navigation not restoring the previous search" + +issues = ["22105", "18484"] +pulls = ["24583"] diff --git a/graylog2-web-interface/src/views/components/Search.test.tsx b/graylog2-web-interface/src/views/components/Search.test.tsx index 45607c6047ee..4b96a4e9e75d 100644 --- a/graylog2-web-interface/src/views/components/Search.test.tsx +++ b/graylog2-web-interface/src/views/components/Search.test.tsx @@ -30,7 +30,7 @@ import mockSearchesClusterConfig from 'fixtures/searchClusterConfig'; import OriginalSearch from './Search'; -import { useSyncWithQueryParameters } from '../hooks/SyncWithQueryParameters'; +import useSyncWithQueryParameters from '../hooks/useSyncWithQueryParameters'; jest.mock('views/logic/fieldtypes/useFieldTypes'); @@ -49,7 +49,7 @@ jest.mock('views/components/DashboardSearchBar', () => () => ( )); -jest.mock('views/hooks/SyncWithQueryParameters'); +jest.mock('views/hooks/useSyncWithQueryParameters'); jest.mock('routing/withLocation', () => (Component) => (props) => ( diff --git a/graylog2-web-interface/src/views/components/SynchronizeUrl.tsx b/graylog2-web-interface/src/views/components/SynchronizeUrl.tsx index b0da03d51c23..4d82456a6a02 100644 --- a/graylog2-web-interface/src/views/components/SynchronizeUrl.tsx +++ b/graylog2-web-interface/src/views/components/SynchronizeUrl.tsx @@ -16,21 +16,33 @@ */ import { useEffect } from 'react'; -import { useSyncWithQueryParameters } from 'views/hooks/SyncWithQueryParameters'; +import useSyncWithQueryParameters from 'views/hooks/useSyncWithQueryParameters'; import bindSearchParamsFromQuery from 'views/hooks/BindSearchParamsFromQuery'; import type { ViewsDispatch } from 'views/stores/useViewsDispatch'; +import useViewsDispatch from 'views/stores/useViewsDispatch'; import type { RootState } from 'views/types'; import { selectView } from 'views/logic/slices/viewSelectors'; -import useViewsDispatch from 'views/stores/useViewsDispatch'; import { selectSearchExecutionState } from 'views/logic/slices/searchExecutionSelectors'; import useLocation from 'routing/useLocation'; import useQuery from 'routing/useQuery'; +import { updateView } from 'views/logic/slices/viewSlice'; const bindSearchParamsFromQueryThunk = - (query: { [key: string]: unknown }) => (_dispatch: ViewsDispatch, getState: () => RootState) => { + (query: { [key: string]: unknown }) => async (dispatch: ViewsDispatch, getState: () => RootState) => { const view = selectView(getState()); const executionState = selectSearchExecutionState(getState()); - bindSearchParamsFromQuery({ view, query, retry: () => Promise.resolve(), executionState }); + + const result = await bindSearchParamsFromQuery({ view, query, retry: () => Promise.resolve(), executionState }); + + if (!result) { + return Promise.resolve(); + } + + const [newView] = result; + + if (newView !== view) { + return dispatch(updateView(newView, true)); + } }; const useBindSearchParamsFromQuery = (query: { [key: string]: unknown }) => { diff --git a/graylog2-web-interface/src/views/hooks/BindSearchParamsFromQuery.ts b/graylog2-web-interface/src/views/hooks/BindSearchParamsFromQuery.ts index b9bfbad62043..7797b71960ce 100644 --- a/graylog2-web-interface/src/views/hooks/BindSearchParamsFromQuery.ts +++ b/graylog2-web-interface/src/views/hooks/BindSearchParamsFromQuery.ts @@ -20,7 +20,6 @@ import isDeepEqual from 'stores/isDeepEqual'; import type { ViewHook, ViewHookArguments } from 'views/logic/hooks/ViewHook'; import View from 'views/logic/views/View'; import normalizeSearchURLQueryParams from 'views/logic/NormalizeSearchURLQueryParams'; -import createSearch from 'views/logic/slices/createSearch'; const bindSearchParamsFromQuery: ViewHook = async ({ query, view, executionState }: ViewHookArguments) => { if (view.type !== View.Type.Search) { @@ -68,11 +67,8 @@ const bindSearchParamsFromQuery: ViewHook = async ({ query, view, executionState return [view, executionState]; } - const newSearch = view.search.toBuilder().newId().queries([newQuery]).build(); - - const savedSearch = await createSearch(newSearch); - - const newView = view.toBuilder().search(savedSearch).build(); + const newSearch = view.search.toBuilder().queries([newQuery]).build(); + const newView = view.toBuilder().search(newSearch).build(); return [newView, executionState]; }; diff --git a/graylog2-web-interface/src/views/hooks/SyncWithQueryParameters.test.tsx b/graylog2-web-interface/src/views/hooks/SyncWithQueryParameters.test.tsx deleted file mode 100644 index 8571c461ab69..000000000000 --- a/graylog2-web-interface/src/views/hooks/SyncWithQueryParameters.test.tsx +++ /dev/null @@ -1,152 +0,0 @@ -/* - * Copyright (C) 2020 Graylog, Inc. - * - * This program is free software: you can redistribute it and/or modify - * it under the terms of the Server Side Public License, version 1, - * as published by MongoDB, Inc. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * Server Side Public License for more details. - * - * You should have received a copy of the Server Side Public License - * along with this program. If not, see - * . - */ -import * as Immutable from 'immutable'; - -import View from 'views/logic/views/View'; -import Query, { createElasticsearchQueryString, filtersForQuery } from 'views/logic/queries/Query'; -import type { TimeRange, RelativeTimeRange } from 'views/logic/queries/Query'; - -import { syncWithQueryParameters } from './SyncWithQueryParameters'; - -jest.mock('views/logic/queries/useCurrentQuery'); -jest.mock('views/hooks/useViewType'); - -const lastFiveMinutes: RelativeTimeRange = { type: 'relative', from: 300 }; -const createQuery = (timerange: TimeRange = lastFiveMinutes, streams: Array = [], queryString = 'foo:42') => - Query.builder() - .timerange(timerange) - .filter(filtersForQuery(streams) || Immutable.Map()) - .query(createElasticsearchQueryString(queryString)) - .build(); - -describe('SyncWithQueryParameters', () => { - afterEach(() => { - jest.clearAllMocks(); - }); - - it('does not do anything if no view is loaded', () => { - const push = jest.fn(); - syncWithQueryParameters(View.Type.Search, '', undefined, push); - - expect(push).not.toHaveBeenCalled(); - }); - - it('does not do anything if current view is not a search', () => { - const push = jest.fn(); - syncWithQueryParameters(View.Type.Dashboard, '', undefined, push); - - expect(push).not.toHaveBeenCalled(); - }); - - describe('if current view is search, adds state to history', () => { - it('with current time range and query', () => { - const push = jest.fn(); - syncWithQueryParameters(View.Type.Search, '/search', createQuery({ type: 'relative', range: 600 }), push); - - expect(push).toHaveBeenCalledWith('/search?q=foo%3A42&rangetype=relative&relative=600'); - }); - - it('preserving query parameters present before', () => { - const push = jest.fn(); - syncWithQueryParameters( - View.Type.Search, - '/search?somevalue=23&somethingelse=foo', - createQuery({ type: 'relative', range: 600 }), - push, - ); - - expect(push).toHaveBeenCalledWith( - '/search?somevalue=23&somethingelse=foo&q=foo%3A42&rangetype=relative&relative=600', - ); - }); - - it('if time range is relative with from and to', () => { - const push = jest.fn(); - syncWithQueryParameters(View.Type.Search, '/search', createQuery({ ...lastFiveMinutes, to: 240 }), push); - - expect(push).toHaveBeenCalledWith('/search?q=foo%3A42&rangetype=relative&from=300&to=240'); - }); - - it('if time range is relative with from only', () => { - const push = jest.fn(); - syncWithQueryParameters(View.Type.Search, '/search', createQuery(lastFiveMinutes), push); - - expect(push).toHaveBeenCalledWith('/search?q=foo%3A42&rangetype=relative&from=300'); - }); - - it('if time range is absolute', () => { - const push = jest.fn(); - - syncWithQueryParameters( - View.Type.Search, - '/search', - createQuery({ - type: 'absolute', - from: '2019-01-12T13:42:23.000Z', - to: '2020-01-12T13:42:23.000Z', - }), - push, - ); - - expect(push).toHaveBeenCalledWith( - '/search?q=foo%3A42&rangetype=absolute&from=2019-01-12T13%3A42%3A23.000Z&to=2020-01-12T13%3A42%3A23.000Z', - ); - }); - - it('if time range is keyword time range', () => { - const push = jest.fn(); - - syncWithQueryParameters( - View.Type.Search, - '/search', - createQuery({ - type: 'keyword', - keyword: 'Last five minutes', - }), - push, - ); - - expect(push).toHaveBeenCalledWith('/search?q=foo%3A42&rangetype=keyword&keyword=Last+five+minutes'); - }); - - it('by calling the provided action', () => { - const replace = jest.fn(); - syncWithQueryParameters(View.Type.Search, '/search', createQuery({ type: 'relative', range: 600 }), replace); - - expect(replace).toHaveBeenCalledWith('/search?q=foo%3A42&rangetype=relative&relative=600'); - }); - - it('adds list of streams to query', () => { - const push = jest.fn(); - syncWithQueryParameters(View.Type.Search, '/search', createQuery(lastFiveMinutes, ['stream1', 'stream2']), push); - - expect(push).toHaveBeenCalledWith('/search?q=foo%3A42&rangetype=relative&from=300&streams=stream1%2Cstream2'); - }); - - it('removes list of streams to query if they become empty', () => { - const push = jest.fn(); - syncWithQueryParameters( - View.Type.Search, - '/search?q=foo%3A42&rangetype=relative&from=300&streams=stream1%2Cstream2', - createQuery(lastFiveMinutes), - push, - ); - - expect(push).toHaveBeenCalledWith('/search?q=foo%3A42&rangetype=relative&from=300'); - }); - }); -}); diff --git a/graylog2-web-interface/src/views/hooks/useSyncWithQueryParameters.test.ts b/graylog2-web-interface/src/views/hooks/useSyncWithQueryParameters.test.ts new file mode 100644 index 000000000000..569f2ef69ae1 --- /dev/null +++ b/graylog2-web-interface/src/views/hooks/useSyncWithQueryParameters.test.ts @@ -0,0 +1,184 @@ +/* + * Copyright (C) 2020 Graylog, Inc. + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the Server Side Public License, version 1, + * as published by MongoDB, Inc. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * Server Side Public License for more details. + * + * You should have received a copy of the Server Side Public License + * along with this program. If not, see + * . + */ +import * as Immutable from 'immutable'; +import { renderHook, waitFor } from 'wrappedTestingLibrary/hooks'; + +import View from 'views/logic/views/View'; +import Query, { createElasticsearchQueryString, filtersForQuery } from 'views/logic/queries/Query'; +import type { TimeRange, RelativeTimeRange } from 'views/logic/queries/Query'; +import { asMock } from 'helpers/mocking'; +import useCurrentQuery from 'views/logic/queries/useCurrentQuery'; +import useViewType from 'views/hooks/useViewType'; +import useHistory from 'routing/useHistory'; +import mockHistory from 'helpers/mocking/mockHistory'; + +import useSyncWithQueryParameters from './useSyncWithQueryParameters'; + +jest.mock('views/logic/queries/useCurrentQuery'); +jest.mock('views/hooks/useViewType'); +jest.mock('routing/useHistory'); + +const lastFiveMinutes: RelativeTimeRange = { type: 'relative', from: 300 }; +const createQuery = (timerange: TimeRange = lastFiveMinutes, streams: Array = [], queryString = 'foo:42') => + Query.builder() + .timerange(timerange) + .filter(filtersForQuery(streams) || Immutable.Map()) + .query(createElasticsearchQueryString(queryString)) + .build(); + +describe('SyncWithQueryParameters', () => { + let history; + + beforeEach(() => { + history = mockHistory(); + asMock(useHistory).mockReturnValue(history); + asMock(useViewType).mockReturnValue(View.Type.Search); + asMock(useCurrentQuery).mockReturnValue(createQuery(lastFiveMinutes)); + }); + + afterEach(() => { + jest.clearAllMocks(); + }); + + it('does not do anything if no view is loaded', () => { + asMock(useViewType).mockReturnValue(undefined); + renderHook(() => useSyncWithQueryParameters('/search')); + + expect(history.replace).not.toHaveBeenCalled(); + expect(history.push).not.toHaveBeenCalled(); + }); + + it('does not do anything if current view is not a search', () => { + asMock(useViewType).mockReturnValue(View.Type.Dashboard); + renderHook(() => useSyncWithQueryParameters('/search')); + + expect(history.replace).not.toHaveBeenCalled(); + expect(history.push).not.toHaveBeenCalled(); + }); + + describe('if current view is search, adds state to history', () => { + it('with current time range and query', () => { + asMock(useCurrentQuery).mockReturnValue(createQuery({ type: 'relative', range: 600 })); + renderHook(() => useSyncWithQueryParameters('/search')); + + expect(history.replace).toHaveBeenCalledWith('/search?q=foo%3A42&rangetype=relative&relative=600'); + }); + + it('preserving query parameters present before', () => { + asMock(useCurrentQuery).mockReturnValue(createQuery({ type: 'relative', range: 600 })); + renderHook(() => useSyncWithQueryParameters('/search?somevalue=23&somethingelse=foo')); + + expect(history.replace).toHaveBeenCalledWith( + '/search?somevalue=23&somethingelse=foo&q=foo%3A42&rangetype=relative&relative=600', + ); + }); + + it('if time range is relative with from and to', () => { + asMock(useCurrentQuery).mockReturnValue(createQuery({ ...lastFiveMinutes, to: 240 })); + renderHook(() => useSyncWithQueryParameters('/search')); + + expect(history.replace).toHaveBeenCalledWith('/search?q=foo%3A42&rangetype=relative&from=300&to=240'); + }); + + it('if time range is relative with from only', () => { + asMock(useCurrentQuery).mockReturnValue(createQuery(lastFiveMinutes)); + renderHook(() => useSyncWithQueryParameters('/search')); + + expect(history.replace).toHaveBeenCalledWith('/search?q=foo%3A42&rangetype=relative&from=300'); + }); + + it('if time range is absolute', () => { + asMock(useCurrentQuery).mockReturnValue( + createQuery({ + type: 'absolute', + from: '2019-01-12T13:42:23.000Z', + to: '2020-01-12T13:42:23.000Z', + }), + ); + renderHook(() => useSyncWithQueryParameters('/search')); + + expect(history.replace).toHaveBeenCalledWith( + '/search?q=foo%3A42&rangetype=absolute&from=2019-01-12T13%3A42%3A23.000Z&to=2020-01-12T13%3A42%3A23.000Z', + ); + }); + + it('if time range is keyword time range', () => { + asMock(useCurrentQuery).mockReturnValue( + createQuery({ + type: 'keyword', + keyword: 'Last five minutes', + }), + ); + renderHook(() => useSyncWithQueryParameters('/search')); + + expect(history.replace).toHaveBeenCalledWith('/search?q=foo%3A42&rangetype=keyword&keyword=Last+five+minutes'); + }); + + it('adds list of streams to query', () => { + asMock(useCurrentQuery).mockReturnValue(createQuery(lastFiveMinutes, ['stream1', 'stream2'])); + renderHook(() => useSyncWithQueryParameters('/search')); + + expect(history.replace).toHaveBeenCalledWith( + '/search?q=foo%3A42&rangetype=relative&from=300&streams=stream1%2Cstream2', + ); + }); + + it('removes list of streams to query if they become empty', () => { + renderHook(() => + useSyncWithQueryParameters('/search?q=foo%3A42&rangetype=relative&from=300&streams=stream1%2Cstream2'), + ); + + expect(history.replace).toHaveBeenCalledWith('/search?q=foo%3A42&rangetype=relative&from=300'); + }); + + it('does not update history if URI is already correct for current view', () => { + renderHook(() => useSyncWithQueryParameters('/search?rangetype=relative&from=300&q=foo%3A42')); + + expect(history.replace).not.toHaveBeenCalled(); + expect(history.push).not.toHaveBeenCalled(); + }); + + it('does not update history if query parameters only differ in order', () => { + const { rerender } = renderHook(({ uri }) => useSyncWithQueryParameters(uri), { + initialProps: { uri: '/search?rangetype=relative&from=300&q=foo%3A42' }, + }); + + rerender({ uri: '/search?q=foo%3A42&rangetype=relative&from=300' }); + + expect(history.replace).not.toHaveBeenCalled(); + expect(history.push).not.toHaveBeenCalled(); + }); + + it('uses push on updates after the initial sync', async () => { + asMock(useCurrentQuery).mockReturnValue(createQuery({ type: 'relative', range: 300 })); + + const { rerender } = renderHook(({ uri }) => useSyncWithQueryParameters(uri), { + initialProps: { uri: '/search' }, + }); + + expect(history.replace).toHaveBeenCalledWith('/search?q=foo%3A42&rangetype=relative&relative=300'); + + asMock(useCurrentQuery).mockReturnValue(createQuery({ type: 'relative', range: 900 })); + + rerender({ uri: '/search?q=foo%3A42&rangetype=relative&relative=300' }); + + await waitFor(() => + expect(history.push).toHaveBeenCalledWith('/search?q=foo%3A42&rangetype=relative&relative=900'), + ); + }); + }); +}); diff --git a/graylog2-web-interface/src/views/hooks/SyncWithQueryParameters.ts b/graylog2-web-interface/src/views/hooks/useSyncWithQueryParameters.ts similarity index 64% rename from graylog2-web-interface/src/views/hooks/SyncWithQueryParameters.ts rename to graylog2-web-interface/src/views/hooks/useSyncWithQueryParameters.ts index 282cde0b2642..99c5bacf3967 100644 --- a/graylog2-web-interface/src/views/hooks/SyncWithQueryParameters.ts +++ b/graylog2-web-interface/src/views/hooks/useSyncWithQueryParameters.ts @@ -14,7 +14,7 @@ * along with this program. If not, see * . */ -import { useEffect } from 'react'; +import { useEffect, useRef } from 'react'; import * as Immutable from 'immutable'; import URI from 'urijs'; @@ -58,14 +58,9 @@ const extractTimerangeParams = (timerange: TimeRange): [string, string | number] } }; -export const syncWithQueryParameters = ( - viewType: ViewType, - query: string, - searchQuery: Query, - action: (to: string) => unknown, -) => { +const uriForView = (viewType: ViewType, query: string, searchQuery: Query) => { if (viewType !== View.Type.Search) { - return; + return undefined; } if (searchQuery) { @@ -95,21 +90,66 @@ export const syncWithQueryParameters = ( ? uriWithStreams.removeSearch('stream_categories') : uriWithStreams.setSearch('stream_categories', currentStreamCategories.join(',')); - if (query !== uri.toString()) { - action(uri.toString()); - } + return uri.toString(); } + + return undefined; }; -export const useSyncWithQueryParameters = (query: string) => { +function canonicalizeUri(uri: string) { + const [path, queryString = ''] = uri.split('?', 2); + if (!queryString) return path; + + const params = queryString + .split('&') + .filter(Boolean) + .map((part) => { + const [k, v = ''] = part.split('=', 2); + + return [k, v]; + }) + .sort(([k1, v1], [k2, v2]) => (k1 === k2 ? v1.localeCompare(v2) : k1.localeCompare(k2))); + + const canonicalQuery = params.map(([k, v]) => `${k}=${v}`).join('&'); + + return `${path}?${canonicalQuery}`; +} + +function isSameUri(a: string, b: string) { + return canonicalizeUri(a) === canonicalizeUri(b); +} + +// Update the URL query parameters when the current view changes. +const useSyncWithQueryParameters = (currentUri: string) => { const viewType = useViewType(); const currentQuery = useCurrentQuery(); const history = useHistory(); + const lastSyncedUriRef = useRef(currentUri); + const isFirstSyncRef = useRef(true); + + useEffect(() => { + const uriForCurrentView = uriForView(viewType, currentUri, currentQuery); - // eslint-disable-next-line react-hooks/exhaustive-deps - useEffect(() => syncWithQueryParameters(viewType, query, currentQuery, history.replace), []); - useEffect( - () => syncWithQueryParameters(viewType, query, currentQuery, history.push), - [currentQuery, history.push, query, viewType], - ); + if (!uriForCurrentView) { + return; + } + + const currentUriMatchesCurrentView = isSameUri(currentUri, uriForCurrentView); + const currentUriMatchesLastSync = isSameUri(currentUri, lastSyncedUriRef.current); + + // Don't update the URI if it was changed outside of this sync. For example when using the browser navigation. + if (!currentUriMatchesLastSync && !currentUriMatchesCurrentView) { + return; + } + + if (!currentUriMatchesCurrentView) { + const updateHistory = isFirstSyncRef.current ? history.replace : history.push; + updateHistory(uriForCurrentView); + } + + isFirstSyncRef.current = false; + lastSyncedUriRef.current = uriForCurrentView; + }, [currentQuery, history, currentUri, viewType]); }; + +export default useSyncWithQueryParameters; diff --git a/graylog2-web-interface/src/views/logic/NormalizeSearchURLQueryParams.ts b/graylog2-web-interface/src/views/logic/NormalizeSearchURLQueryParams.ts index 346a7ef0cfd3..092bfa87cf32 100644 --- a/graylog2-web-interface/src/views/logic/NormalizeSearchURLQueryParams.ts +++ b/graylog2-web-interface/src/views/logic/NormalizeSearchURLQueryParams.ts @@ -109,6 +109,7 @@ export const useSearchURLQueryParams = () => { streams: filtersToStreamSet(streamsFilter).toArray(), streamCategories: filtersToStreamCategorySet(streamCategoriesFilter).toArray(), }; + // eslint-disable-next-line @tanstack/query/no-unstable-deps }, [query]); };