From e471064cdaa9032cc5d65761841b8592b2643db2 Mon Sep 17 00:00:00 2001 From: Giordano Ricci Date: Fri, 12 Jan 2024 14:05:46 +0000 Subject: [PATCH] Explore: use numeric ids for panel ids when building query transactions (#80135) --- .betterer.results | 4 +-- jest.config.js | 1 + package.json | 1 + public/app/core/utils/explore.ts | 29 +++++++++---------- .../hooks/useStateSync/migrators/v1.ts | 16 ++++++++-- public/app/features/explore/state/main.ts | 13 +++++---- yarn.lock | 10 +++++++ 7 files changed, 48 insertions(+), 26 deletions(-) diff --git a/.betterer.results b/.betterer.results index cd630533c99..3cfced5f9d1 100644 --- a/.betterer.results +++ b/.betterer.results @@ -1188,9 +1188,7 @@ exports[`better eslint`] = { [0, 0, 0, "Unexpected any. Specify a different type.", "0"] ], "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.", "0"] ], "public/app/core/utils/fetch.ts:5381": [ [0, 0, 0, "Do not use any type assertions.", "0"], diff --git a/jest.config.js b/jest.config.js index 979c33eec3d..01ede1b0eda 100644 --- a/jest.config.js +++ b/jest.config.js @@ -12,6 +12,7 @@ const esModules = [ 'internmap', 'robust-predicates', 'leven', + 'nanoid', ].join('|'); module.exports = { diff --git a/package.json b/package.json index d632c1c6c62..8d4e86abd10 100644 --- a/package.json +++ b/package.json @@ -356,6 +356,7 @@ "mousetrap": "1.6.5", "mousetrap-global-bind": "1.1.0", "moveable": "0.43.1", + "nanoid": "^5.0.4", "node-forge": "^1.3.1", "ol": "7.4.0", "ol-ext": "4.0.10", diff --git a/public/app/core/utils/explore.ts b/public/app/core/utils/explore.ts index 7c138740d6b..912189120e7 100644 --- a/public/app/core/utils/explore.ts +++ b/public/app/core/utils/explore.ts @@ -1,4 +1,4 @@ -import { nanoid } from '@reduxjs/toolkit'; +import { customAlphabet } from 'nanoid'; import { Unsubscribable } from 'rxjs'; import { v4 as uuidv4 } from 'uuid'; @@ -34,6 +34,9 @@ export const DEFAULT_UI_STATE = { dedupStrategy: LogsDedupStrategy.none, }; +export const ID_ALPHABET = '0123456789abcdefghijklmnopqrstuvwxyz'; +const nanoid = customAlphabet(ID_ALPHABET, 3); + const MAX_HISTORY_ITEMS = 100; const LAST_USED_DATASOURCE_KEY = 'grafana.explore.datasource'; @@ -52,7 +55,13 @@ export interface GetExploreUrlArguments { } export function generateExploreId() { - return nanoid(3); + while (true) { + const id = nanoid(3); + + if (!/^\d+$/.test(id)) { + return id; + } + } } /** @@ -101,19 +110,9 @@ export function buildQueryTransaction( timeZone?: TimeZone, scopedVars?: ScopedVars ): QueryTransaction { - const key = queries.reduce((combinedKey, query) => { - combinedKey += query.key; - return combinedKey; - }, ''); - + const panelId = Number.parseInt(exploreId, 36); const { interval, intervalMs } = getIntervals(range, queryOptions.minInterval, queryOptions.maxDataPoints); - // Most datasource is using `panelId + query.refId` for cancellation logic. - // Using `format` here because it relates to the view panel that the request is for. - // However, some datasources don't use `panelId + query.refId`, but only `panelId`. - // Therefore panel id has to be unique. - const panelId = `${key}`; - const request: DataQueryRequest = { app: CoreApp.Explore, // TODO probably should be taken from preferences but does not seem to be used anyway. @@ -121,9 +120,7 @@ export function buildQueryTransaction( startTime: Date.now(), interval, intervalMs, - // TODO: the query request expects number and we are using string here. Seems like it works so far but can create - // issues down the road. - panelId: panelId as any, + panelId, targets: queries, // Datasources rely on DataQueries being passed under the targets key. range, requestId: 'explore_' + exploreId, diff --git a/public/app/features/explore/hooks/useStateSync/migrators/v1.ts b/public/app/features/explore/hooks/useStateSync/migrators/v1.ts index 683c57f2202..57b23ce4076 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 } from 'app/core/utils/explore'; +import { ID_ALPHABET, generateExploreId } from 'app/core/utils/explore'; import { DEFAULT_RANGE } from 'app/features/explore/state/utils'; import { hasKey } from '../../utils'; @@ -48,9 +48,21 @@ export const v1Migrator: MigrationHandler = { const panes = Object.entries(rawPanes) .map(([key, value]) => [key, applyDefaults(value)] as const) .reduce>((acc, [key, value]) => { + let newKey = key; + // Panes IDs must be 3 characters long and contain at least one letter + if ( + newKey.length !== 3 || + /^\d+$/.test(newKey) || + newKey.split('').some((ch) => { + return ID_ALPHABET.indexOf(ch) === -1; + }) + ) { + newKey = generateExploreId(); + } + return { ...acc, - [key]: value, + [newKey]: value, }; }, {}); diff --git a/public/app/features/explore/state/main.ts b/public/app/features/explore/state/main.ts index ebe59018553..ca9e73cc891 100644 --- a/public/app/features/explore/state/main.ts +++ b/public/app/features/explore/state/main.ts @@ -65,7 +65,7 @@ export const clearPanes = createAction('explore/clearPanes'); */ export const splitOpen = createAsyncThunk( 'explore/splitOpen', - async (options: SplitOpenOptions | undefined, { getState, dispatch, requestId }) => { + async (options: SplitOpenOptions | undefined, { getState, dispatch }) => { // we currently support showing only 2 panes in explore, so if this action is dispatched we know it has been dispatched from the "first" pane. const originState = Object.values(getState().explore.panes)[0]; @@ -80,9 +80,15 @@ export const splitOpen = createAsyncThunk( const splitRange = options?.range || originState?.range.raw || DEFAULT_RANGE; + let newPaneId = generateExploreId(); + // in case we have a duplicate id, generate a new one + while (getState().explore.panes[newPaneId]) { + newPaneId = generateExploreId(); + } + await dispatch( createNewSplitOpenPane({ - exploreId: requestId, + exploreId: newPaneId, datasource: options?.datasourceUid || originState?.datasourceInstance?.getRef(), queries: withUniqueRefIds(queries), range: splitRange, @@ -95,9 +101,6 @@ export const splitOpen = createAsyncThunk( if (originState?.range) { await dispatch(syncTimesAction({ syncedTimes: isEqual(originState.range.raw, splitRange) })); // if time ranges are equal, mark times as synced } - }, - { - idGenerator: generateExploreId, } ); diff --git a/yarn.lock b/yarn.lock index 37d0ce014ad..6c6d80dd8c0 100644 --- a/yarn.lock +++ b/yarn.lock @@ -17273,6 +17273,7 @@ __metadata: moveable: "npm:0.43.1" msw: "npm:1.3.2" mutationobserver-shim: "npm:0.3.7" + nanoid: "npm:^5.0.4" ngtemplate-loader: "npm:2.1.0" node-forge: "npm:^1.3.1" node-notifier: "npm:10.0.1" @@ -21889,6 +21890,15 @@ __metadata: languageName: node linkType: hard +"nanoid@npm:^5.0.4": + version: 5.0.4 + resolution: "nanoid@npm:5.0.4" + bin: + nanoid: bin/nanoid.js + checksum: cf09cca3774f3147100948f7478f75f4c9ee97a4af65c328dd9abbd83b12f8bb35cf9f89a21c330f3b759d667a4cd0140ed84aa5fdd522c61e0d341aeaa7fb6f + languageName: node + linkType: hard + "natural-compare@npm:^1.4.0": version: 1.4.0 resolution: "natural-compare@npm:1.4.0"