From 53642b60ed442319de7507fbed98cfdfc1947f8c Mon Sep 17 00:00:00 2001 From: Giordano Ricci Date: Mon, 20 Nov 2023 13:54:00 +0000 Subject: [PATCH] Explore: handle URLs parse errors (#77784) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Explore: handle URLs parse errors * minor modifications * tentative fix * update docs * omit v0 params from the final url * Apply suggestions from code review Co-authored-by: Piotr Jamróz * remove safeParseJson, make parse return an object --------- Co-authored-by: Piotr Jamróz --- .betterer.results | 3 +- docs/sources/explore/_index.md | 4 + public/app/core/utils/explore.ts | 12 --- .../explore/hooks/useExplorePageTitle.ts | 14 +++- .../explore/hooks/useStateSync/index.ts | 75 ++++++++++-------- .../hooks/useStateSync/migrators/types.ts | 2 +- .../hooks/useStateSync/migrators/v0.test.ts | 25 ++---- .../hooks/useStateSync/migrators/v0.ts | 77 ++++++++++++++----- .../hooks/useStateSync/migrators/v1.test.ts | 22 ++---- .../hooks/useStateSync/migrators/v1.ts | 35 +++++++-- .../explore/hooks/useStateSync/parseURL.ts | 8 +- 11 files changed, 166 insertions(+), 111 deletions(-) diff --git a/.betterer.results b/.betterer.results index cc9ca97d137..b1210b6a1bd 100644 --- a/.betterer.results +++ b/.betterer.results @@ -1546,8 +1546,7 @@ exports[`better eslint`] = { "public/app/core/utils/explore.ts:5381": [ [0, 0, 0, "Do not use any type assertions.", "0"], [0, 0, 0, "Unexpected any. Specify a different type.", "1"], - [0, 0, 0, "Unexpected any. Specify a different type.", "2"], - [0, 0, 0, "Unexpected any. Specify a different type.", "3"] + [0, 0, 0, "Unexpected any. Specify a different type.", "2"] ], "public/app/core/utils/fetch.ts:5381": [ [0, 0, 0, "Do not use any type assertions.", "0"], diff --git a/docs/sources/explore/_index.md b/docs/sources/explore/_index.md index 2c54a52207d..2b9881a8eef 100644 --- a/docs/sources/explore/_index.md +++ b/docs/sources/explore/_index.md @@ -87,6 +87,10 @@ You can then click on any panel icon in the content outline to navigate to that When using Explore, the URL in the browser address bar updates as you make changes to the queries. You can share or bookmark this URL. +{{% admonition type="note" %}} +Explore may generate relatively long URLs, some tools, like messaging or videoconferencing apps, may truncate messages to a fixed length. In such cases Explore will display a warning message and load a default state. If you encounter issues when sharing Explore links in such apps, you can generate shortened links. See [Share shortened link](#share-shortened-link) for more information. +{{% /admonition %}} + ### Generating Explore URLs from external tools Because Explore URLs have a defined structure, you can build a URL from external tools and open it in Grafana. The URL structure is: diff --git a/public/app/core/utils/explore.ts b/public/app/core/utils/explore.ts index 281f6fd997e..07bb33f6cf9 100644 --- a/public/app/core/utils/explore.ts +++ b/public/app/core/utils/explore.ts @@ -148,18 +148,6 @@ export function buildQueryTransaction( export const clearQueryKeys: (query: DataQuery) => DataQuery = ({ key, ...rest }) => rest; -export const safeParseJson = (text?: string): any | undefined => { - if (!text) { - return; - } - - try { - return JSON.parse(text); - } catch (error) { - console.error(error); - } -}; - export const safeStringifyValue = (value: unknown, space?: number) => { if (value === undefined || value === null) { return ''; diff --git a/public/app/features/explore/hooks/useExplorePageTitle.ts b/public/app/features/explore/hooks/useExplorePageTitle.ts index c97f23a63b9..f89fb9ca6d6 100644 --- a/public/app/features/explore/hooks/useExplorePageTitle.ts +++ b/public/app/features/explore/hooks/useExplorePageTitle.ts @@ -3,7 +3,6 @@ import { useEffect, useRef } from 'react'; import { NavModel } from '@grafana/data'; import { Branding } from 'app/core/components/Branding/Branding'; import { useNavModel } from 'app/core/hooks/useNavModel'; -import { safeParseJson } from 'app/core/utils/explore'; import { getDatasourceSrv } from 'app/features/plugins/datasource_srv'; import { ExploreQueryParams } from 'app/types'; @@ -19,8 +18,19 @@ export function useExplorePageTitle(params: ExploreQueryParams) { return; } + let panesObject: unknown; + try { + panesObject = JSON.parse(params.panes); + } catch { + return; + } + + if (typeof panesObject !== 'object' || panesObject === null) { + return; + } + Promise.allSettled( - Object.values(safeParseJson(params.panes)).map((pane) => { + Object.values(panesObject).map((pane) => { if ( !pane || typeof pane !== 'object' || diff --git a/public/app/features/explore/hooks/useStateSync/index.ts b/public/app/features/explore/hooks/useStateSync/index.ts index 79d5906a96c..444ff1d36c6 100644 --- a/public/app/features/explore/hooks/useStateSync/index.ts +++ b/public/app/features/explore/hooks/useStateSync/index.ts @@ -4,6 +4,7 @@ import { useEffect, useRef } from 'react'; import { CoreApp, ExploreUrlState, DataSourceApi, toURLRange, EventBusSrv } from '@grafana/data'; import { DataQuery, DataSourceRef } from '@grafana/schema'; import { useGrafana } from 'app/core/context/GrafanaContext'; +import { useAppNotification } from 'app/core/copy/appNotification'; import { clearQueryKeys, getLastUsedDatasourceUID } from 'app/core/utils/explore'; import { getDatasourceSrv } from 'app/features/plugins/datasource_srv'; import { MIXED_DATASOURCE_NAME } from 'app/plugins/datasource/mixed/MixedDataSource'; @@ -31,6 +32,7 @@ export function useStateSync(params: ExploreQueryParams) { const orgId = useSelector((state) => state.user.orgId); const prevParams = useRef(params); const initState = useRef<'notstarted' | 'pending' | 'done'>('notstarted'); + const { warning } = useAppNotification(); useEffect(() => { // This happens when the user navigates to an explore "empty page" while within Explore. @@ -76,8 +78,11 @@ export function useStateSync(params: ExploreQueryParams) { if (!isEqual(prevParams.current.panes, JSON.stringify(panesQueryParams))) { // If there's no previous state it means we are mounting explore for the first time, // in this case we want to replace the URL instead of pushing a new entry to the history. + // If the init state is 'pending' it means explore still hasn't finished initializing. in that case we skip + // pushing a new entry in the history as the first entry will be pushed after initialization. const replace = - !!prevParams.current.panes && Object.values(prevParams.current.panes).filter(Boolean).length === 0; + (!!prevParams.current.panes && Object.values(prevParams.current.panes).filter(Boolean).length === 0) || + initState.current === 'pending'; prevParams.current = { panes: JSON.stringify(panesQueryParams), @@ -96,7 +101,12 @@ export function useStateSync(params: ExploreQueryParams) { useEffect(() => { const isURLOutOfSync = prevParams.current?.panes !== params.panes; - const urlState = parseURL(params); + const [urlState, hasParseError] = parseURL(params); + hasParseError && + warning( + 'Could not parse Explore URL', + 'The requested URL contains invalid parameters, a default Explore state has been loaded.' + ); async function sync() { // if navigating the history causes one of the time range to not being equal to all the other ones, @@ -225,42 +235,45 @@ export function useStateSync(params: ExploreQueryParams) { }) ); - const newParams = initializedPanes.reduce( - (acc, { exploreId, state }) => { - return { - ...acc, - panes: { - ...acc.panes, - [exploreId]: getUrlStateFromPaneState(state), - }, - }; - }, - { - panes: {}, - } - ); - initState.current = 'done'; + const panesObj = initializedPanes.reduce((acc, { exploreId, state }) => { + return { + ...acc, + [exploreId]: getUrlStateFromPaneState(state), + }; + }, {}); + // we need to use partial here beacuse replace doesn't encode the query params. - location.partial( - { - // partial doesn't remove other parameters, so we delete (by setting them to undefined) all the current one before adding the new ones. - ...Object.keys(location.getSearchObject()).reduce>((acc, key) => { - acc[key] = undefined; - return acc; - }, {}), - panes: JSON.stringify(newParams.panes), - schemaVersion: urlState.schemaVersion, - orgId, - }, - true - ); + const oldQuery = location.getSearchObject(); + + // we create the default query params from the current URL, omitting all the properties we know should be in the final url. + // This includes params from previous schema versions and 'schemaVersion', 'panes', 'orgId' as we want to replace those. + let defaults: Record = {}; + for (const [key, value] of Object.entries(oldQuery).filter( + ([key]) => !['schemaVersion', 'panes', 'orgId', 'left', 'right'].includes(key) + )) { + defaults[key] = value; + } + + const searchParams = new URLSearchParams({ + // we set the schemaVersion as the first parameter so that when URLs are truncated the schemaVersion is more likely to be present. + schemaVersion: `${urlState.schemaVersion}`, + panes: JSON.stringify(panesObj), + orgId: `${orgId}`, + ...defaults, + }); + + location.replace({ + pathname: location.getLocation().pathname, + search: searchParams.toString(), + }); + initState.current = 'done'; }); } prevParams.current = params; isURLOutOfSync && initState.current === 'done' && sync(); - }, [dispatch, panesState, orgId, location, params]); + }, [dispatch, panesState, orgId, location, params, warning]); } function getDefaultQuery(ds: DataSourceApi) { diff --git a/public/app/features/explore/hooks/useStateSync/migrators/types.ts b/public/app/features/explore/hooks/useStateSync/migrators/types.ts index 2983caca848..1ab41c51469 100644 --- a/public/app/features/explore/hooks/useStateSync/migrators/types.ts +++ b/public/app/features/explore/hooks/useStateSync/migrators/types.ts @@ -4,7 +4,7 @@ export interface MigrationHandler { /** * The parse function is used to parse the URL parameters into the state object. */ - parse: (params: UrlQueryMap) => To; + parse: (params: UrlQueryMap) => { to: To; error: boolean }; /** * the migrate function takes a state object from the previous schema version and returns a new state object */ diff --git a/public/app/features/explore/hooks/useStateSync/migrators/v0.test.ts b/public/app/features/explore/hooks/useStateSync/migrators/v0.test.ts index 94a746c2b6e..c437c603715 100644 --- a/public/app/features/explore/hooks/useStateSync/migrators/v0.test.ts +++ b/public/app/features/explore/hooks/useStateSync/migrators/v0.test.ts @@ -4,16 +4,8 @@ import { v0Migrator } from './v0'; describe('v0 migrator', () => { describe('parse', () => { - beforeEach(function () { - jest.spyOn(console, 'error').mockImplementation(() => void 0); - }); - - afterEach(() => { - jest.restoreAllMocks(); - }); - it('returns default state on empty string', () => { - expect(v0Migrator.parse({})).toMatchObject({ + expect(v0Migrator.parse({}).to).toMatchObject({ left: { datasource: null, queries: [], @@ -24,7 +16,7 @@ describe('v0 migrator', () => { it('returns a valid Explore state from URL parameter', () => { const paramValue = '{"datasource":"Local","queries":[{"expr":"metric"}],"range":{"from":"now-1h","to":"now"}}'; - expect(v0Migrator.parse({ left: paramValue })).toMatchObject({ + expect(v0Migrator.parse({ left: paramValue }).to).toMatchObject({ left: { datasource: 'Local', queries: [{ expr: 'metric' }], @@ -38,7 +30,7 @@ describe('v0 migrator', () => { it('returns a valid Explore state from right URL parameter', () => { const paramValue = '{"datasource":"Local","queries":[{"expr":"metric"}],"range":{"from":"now-1h","to":"now"}}'; - expect(v0Migrator.parse({ right: paramValue })).toMatchObject({ + expect(v0Migrator.parse({ right: paramValue }).to).toMatchObject({ right: { datasource: 'Local', queries: [{ expr: 'metric' }], @@ -52,7 +44,7 @@ describe('v0 migrator', () => { it('returns a default state from invalid right URL parameter', () => { const paramValue = 10; - expect(v0Migrator.parse({ right: paramValue })).toMatchObject({ + expect(v0Migrator.parse({ right: paramValue }).to).toMatchObject({ right: { datasource: null, queries: [], @@ -64,7 +56,7 @@ describe('v0 migrator', () => { it('returns a valid Explore state from a compact URL parameter', () => { const paramValue = '["now-1h","now","Local",{"expr":"metric"},{"ui":[true,true,true,"none"],"__panelsState":{"logs":"1"}}]'; - expect(v0Migrator.parse({ left: paramValue })).toMatchObject({ + expect(v0Migrator.parse({ left: paramValue }).to).toMatchObject({ left: { datasource: 'Local', queries: [{ expr: 'metric' }], @@ -81,21 +73,20 @@ describe('v0 migrator', () => { it('returns default state on compact URLs with too few segments ', () => { const paramValue = '["now-1h",{"expr":"metric"},{"ui":[true,true,true,"none"]}]'; - expect(v0Migrator.parse({ left: paramValue })).toMatchObject({ + expect(v0Migrator.parse({ left: paramValue }).to).toMatchObject({ left: { datasource: null, queries: [], range: DEFAULT_RANGE, }, }); - expect(console.error).toHaveBeenCalledWith('Error parsing compact URL state for Explore.'); }); it('should not return a query for mode in the url', () => { // Previous versions of Grafana included "Explore mode" in the URL; this should not be treated as a query. const paramValue = '["now-1h","now","x-ray-datasource",{"queryType":"getTraceSummaries"},{"mode":"Metrics"},{"ui":[true,true,true,"none"]}]'; - expect(v0Migrator.parse({ left: paramValue })).toMatchObject({ + expect(v0Migrator.parse({ left: paramValue }).to).toMatchObject({ left: { datasource: 'x-ray-datasource', queries: [{ queryType: 'getTraceSummaries' }], @@ -110,7 +101,7 @@ describe('v0 migrator', () => { it('should return queries if queryType is present in the url', () => { const paramValue = '["now-1h","now","x-ray-datasource",{"queryType":"getTraceSummaries"},{"ui":[true,true,true,"none"]}]'; - expect(v0Migrator.parse({ left: paramValue })).toMatchObject({ + expect(v0Migrator.parse({ left: paramValue }).to).toMatchObject({ left: { datasource: 'x-ray-datasource', queries: [{ queryType: 'getTraceSummaries' }], diff --git a/public/app/features/explore/hooks/useStateSync/migrators/v0.ts b/public/app/features/explore/hooks/useStateSync/migrators/v0.ts index c1d641facad..d1dceeccc46 100644 --- a/public/app/features/explore/hooks/useStateSync/migrators/v0.ts +++ b/public/app/features/explore/hooks/useStateSync/migrators/v0.ts @@ -1,5 +1,4 @@ import { ExploreUrlState } from '@grafana/data'; -import { safeParseJson } from 'app/core/utils/explore'; import { DEFAULT_RANGE } from 'app/features/explore/state/utils'; import { BaseExploreURL, MigrationHandler } from './types'; @@ -12,12 +11,51 @@ export interface ExploreURLV0 extends BaseExploreURL { export const v0Migrator: MigrationHandler = { parse: (params) => { + // If no params are provided, return the default state without errors + // This means the user accessed the explore page without any params + if (!params.left && !params.right) { + return { + to: { + left: { + datasource: null, + queries: [], + range: { + from: 'now-6h', + to: 'now', + }, + }, + schemaVersion: 0, + }, + error: false, + }; + } + + let left: ExploreUrlState | undefined; + let right: ExploreUrlState | undefined; + let leftError, rightError: boolean | undefined; + + if (typeof params.left === 'string') { + [left, leftError] = parsePaneState(params.left); + } + + if (typeof params.right === 'string') { + [right, rightError] = parsePaneState(params.right); + } else if (params.right) { + right = FALLBACK_PANE_VALUE; + rightError = true; + } + + if (!left) { + left = FALLBACK_PANE_VALUE; + } + return { - schemaVersion: 0, - left: parseUrlState(typeof params.left === 'string' ? params.left : undefined), - ...(params.right && { - right: parseUrlState(typeof params.right === 'string' ? params.right : undefined), - }), + to: { + schemaVersion: 0, + left, + ...(right && { right }), + }, + error: !!leftError || !!rightError, }; }, }; @@ -32,25 +70,26 @@ enum ParseUrlStateIndex { SegmentsStart = 3, } -function parseUrlState(initial: string | undefined): ExploreUrlState { - const parsed = safeParseJson(initial); - const errorResult = { - datasource: null, - queries: [], - range: DEFAULT_RANGE, - }; +const FALLBACK_PANE_VALUE: ExploreUrlState = { + datasource: null, + queries: [], + range: DEFAULT_RANGE, +}; - if (!parsed) { - return errorResult; +function parsePaneState(initial: string): [ExploreUrlState, boolean] { + let parsed; + try { + parsed = JSON.parse(initial); + } catch { + return [FALLBACK_PANE_VALUE, true]; } if (!Array.isArray(parsed)) { - return { queries: [], range: DEFAULT_RANGE, ...parsed }; + return [{ queries: [], range: DEFAULT_RANGE, ...parsed }, false]; } if (parsed.length <= ParseUrlStateIndex.SegmentsStart) { - console.error('Error parsing compact URL state for Explore.'); - return errorResult; + return [FALLBACK_PANE_VALUE, true]; } const range = { @@ -62,5 +101,5 @@ function parseUrlState(initial: string | undefined): ExploreUrlState { const queries = parsedSegments.filter((segment) => !isSegment(segment, 'ui', 'mode', '__panelsState')); const panelsState = parsedSegments.find((segment) => isSegment(segment, '__panelsState'))?.__panelsState; - return { datasource, queries, range, panelsState }; + return [{ datasource, queries, range, panelsState }, false]; } diff --git a/public/app/features/explore/hooks/useStateSync/migrators/v1.test.ts b/public/app/features/explore/hooks/useStateSync/migrators/v1.test.ts index 922efb9e99e..0f87f1bb4b5 100644 --- a/public/app/features/explore/hooks/useStateSync/migrators/v1.test.ts +++ b/public/app/features/explore/hooks/useStateSync/migrators/v1.test.ts @@ -9,16 +9,8 @@ jest.mock('app/core/utils/explore', () => ({ describe('v1 migrator', () => { describe('parse', () => { - beforeEach(function () { - jest.spyOn(console, 'error').mockImplementation(() => void 0); - }); - - afterEach(() => { - jest.restoreAllMocks(); - }); - it('correctly returns default state when no params are provided', () => { - expect(v1Migrator.parse({})).toMatchObject({ + expect(v1Migrator.parse({}).to).toMatchObject({ panes: { ID: { datasource: null, @@ -30,7 +22,7 @@ describe('v1 migrator', () => { }); it('correctly returns default state when panes param is an empty object', () => { - expect(v1Migrator.parse({ panes: '{}' })).toMatchObject({ + expect(v1Migrator.parse({ panes: '{}' }).to).toMatchObject({ panes: { ID: { datasource: null, @@ -42,7 +34,7 @@ describe('v1 migrator', () => { }); it('correctly returns default state when panes param is not a valid JSON object', () => { - expect(v1Migrator.parse({ panes: '{a malformed json}' })).toMatchObject({ + expect(v1Migrator.parse({ panes: '{a malformed json}' }).to).toMatchObject({ panes: { ID: { datasource: null, @@ -51,12 +43,10 @@ describe('v1 migrator', () => { }, }, }); - - expect(console.error).toHaveBeenCalledTimes(1); }); it('correctly returns default state when a pane in panes params is an empty object', () => { - expect(v1Migrator.parse({ panes: '{"aaa": {}}' })).toMatchObject({ + expect(v1Migrator.parse({ panes: '{"aaa": {}}' }).to).toMatchObject({ panes: { aaa: { datasource: null, @@ -68,7 +58,7 @@ describe('v1 migrator', () => { }); it('correctly returns default state when a pane in panes params is not a valid JSON object', () => { - expect(v1Migrator.parse({ panes: '{"aaa": "NOT A VALID URL STATE"}' })).toMatchObject({ + expect(v1Migrator.parse({ panes: '{"aaa": "NOT A VALID URL STATE"}' }).to).toMatchObject({ panes: { aaa: { datasource: null, @@ -96,7 +86,7 @@ describe('v1 migrator', () => { } } }`, - }) + }).to ).toMatchObject({ panes: { aaa: { diff --git a/public/app/features/explore/hooks/useStateSync/migrators/v1.ts b/public/app/features/explore/hooks/useStateSync/migrators/v1.ts index 5dc6a9a6ca0..683c57f2202 100644 --- a/public/app/features/explore/hooks/useStateSync/migrators/v1.ts +++ b/public/app/features/explore/hooks/useStateSync/migrators/v1.ts @@ -1,5 +1,5 @@ import { ExploreUrlState } from '@grafana/data'; -import { generateExploreId, safeParseJson } from 'app/core/utils/explore'; +import { generateExploreId } from 'app/core/utils/explore'; import { DEFAULT_RANGE } from 'app/features/explore/state/utils'; import { hasKey } from '../../utils'; @@ -18,14 +18,32 @@ export const v1Migrator: MigrationHandler = { parse: (params) => { if (!params || !params.panes || typeof params.panes !== 'string') { return { - schemaVersion: 1, - panes: { - [generateExploreId()]: DEFAULT_STATE, + to: { + schemaVersion: 1, + panes: { + [generateExploreId()]: DEFAULT_STATE, + }, }, + error: false, }; } - const rawPanes: Record = safeParseJson(params.panes) || {}; + let rawPanes: unknown; + try { + rawPanes = JSON.parse(params.panes); + } catch {} + + if (rawPanes == null || typeof rawPanes !== 'object') { + return { + to: { + schemaVersion: 1, + panes: { + [generateExploreId()]: DEFAULT_STATE, + }, + }, + error: true, + }; + } const panes = Object.entries(rawPanes) .map(([key, value]) => [key, applyDefaults(value)] as const) @@ -41,8 +59,11 @@ export const v1Migrator: MigrationHandler = { } return { - schemaVersion: 1, - panes, + to: { + schemaVersion: 1, + panes, + }, + error: false, }; }, migrate: (params) => { diff --git a/public/app/features/explore/hooks/useStateSync/parseURL.ts b/public/app/features/explore/hooks/useStateSync/parseURL.ts index 0a67b625880..4325630c1a9 100644 --- a/public/app/features/explore/hooks/useStateSync/parseURL.ts +++ b/public/app/features/explore/hooks/useStateSync/parseURL.ts @@ -11,20 +11,20 @@ export const parseURL = (params: ExploreQueryParams) => { const migrators = [v0Migrator, v1Migrator] as const; -const migrate = (params: ExploreQueryParams): ExploreURL => { +const migrate = (params: ExploreQueryParams): [ExploreURL, boolean] => { const schemaVersion = getSchemaVersion(params); const [parser, ...migratorsToRun] = migrators.slice(schemaVersion); - const parsedUrl = parser.parse(params); + const { error, to } = parser.parse(params); // @ts-expect-error const final: ExploreURL = migratorsToRun.reduce((acc, migrator) => { // @ts-expect-error return migrator.migrate ? migrator.migrate(acc) : acc; - }, parsedUrl); + }, to); - return final; + return [final, error]; }; function getSchemaVersion(params: ExploreQueryParams): number {