From c4356543237cb6bf51b9ae179b945f168a395f52 Mon Sep 17 00:00:00 2001 From: Galen Kistler <109082771+gtk-grafana@users.noreply.github.com> Date: Fri, 10 Jan 2025 11:03:34 -0600 Subject: [PATCH] fix: use urlUtil instead of UrlSearchParams (#994) * fix: use urlUtil instead of UrlSearchParams * fix: fix migration with escaped chars --- .../IndexScene/ShareButtonScene.tsx | 11 +++--- src/services/migrations.ts | 39 ++++++++++++++----- tests/exploreServicesBreakDown.spec.ts | 18 +++++++-- tests/fixtures/explore.ts | 10 +++-- 4 files changed, 55 insertions(+), 23 deletions(-) diff --git a/src/Components/IndexScene/ShareButtonScene.tsx b/src/Components/IndexScene/ShareButtonScene.tsx index 672a514c..c8ae876c 100644 --- a/src/Components/IndexScene/ShareButtonScene.tsx +++ b/src/Components/IndexScene/ShareButtonScene.tsx @@ -8,7 +8,7 @@ import { import { ButtonGroup, Dropdown, IconName, Menu, MenuGroup, ToolbarButton } from '@grafana/ui'; import React from 'react'; import { config, getAppEvents, getBackendSrv, locationService, reportInteraction } from '@grafana/runtime'; -import { AppEvents, toUtc } from '@grafana/data'; +import { AppEvents, toUtc, urlUtil } from '@grafana/data'; import { copyText } from '../../services/text'; interface ShortLinkMenuItemData { @@ -244,9 +244,8 @@ const constructAbsoluteUrl = (timeRange: SceneTimeRangeLike): string => { const from = toUtc(timeRange.state.value.from); const to = toUtc(timeRange.state.value.to); const location = locationService.getLocation(); - const searchParams = new URLSearchParams(location.search); - searchParams.set('from', from.toISOString()); - searchParams.set('to', to.toISOString()); - - return `${location.pathname}?${searchParams.toString()}`; + const searchParams = urlUtil.getUrlSearchParams(); + searchParams['from'] = from.toISOString(); + searchParams['to'] = to.toISOString(); + return urlUtil.renderUrl(location.pathname, searchParams); }; diff --git a/src/services/migrations.ts b/src/services/migrations.ts index 1824b2cf..5283dbce 100644 --- a/src/services/migrations.ts +++ b/src/services/migrations.ts @@ -3,16 +3,37 @@ import { getLineFiltersVariable } from './variableGetters'; import { LineFilterCaseSensitive } from '../Components/ServiceScene/LineFilter/LineFilterScene'; import { LineFilterOp } from './filterTypes'; import { ServiceScene } from '../Components/ServiceScene/ServiceScene'; +import { urlUtil } from '@grafana/data'; + +function removeEscapeChar(value: string, caseSensitive: boolean) { + const charsEscapedByEscapeRegExp = ['^', '$', '.', '*', '+', '?', '(', ')', '[', ']', '{', '}', '|']; + if (!caseSensitive) { + charsEscapedByEscapeRegExp.push('\\'); + } + return value + .split('') + .filter((char, index, stringArray) => { + // We need to differentiate between user entered escape chars, and escape chars added by lodash escapeRegExp to return the same query results in urls from before the line filter regex feature + // Since there is no reverse of the escapeRegExp method provided by lodash we're essentially building our own "unescapeRegExp" + const nextChar = stringArray[index + 1]; + const isNextCharRegex = charsEscapedByEscapeRegExp.includes(nextChar); + return !(char === '\\' && isNextCharRegex); + }) + .join(''); +} /** * Migrates old line filter to new variables */ export function migrateLineFilterV1(serviceScene: ServiceScene) { - const search = locationService.getSearch(); + const search = urlUtil.getUrlSearchParams(); - const deprecatedLineFilter = search.get('var-lineFilter'); - - if (!deprecatedLineFilter) { + const deprecatedLineFilterArray = search['var-lineFilter']; + if (!Array.isArray(deprecatedLineFilterArray) || !deprecatedLineFilterArray.length) { + return; + } + const deprecatedLineFilter = deprecatedLineFilterArray[0]; + if (typeof deprecatedLineFilter !== 'string' || !deprecatedLineFilter) { return; } @@ -26,7 +47,7 @@ export function migrateLineFilterV1(serviceScene: ServiceScene) { { key: LineFilterCaseSensitive.caseSensitive, operator: LineFilterOp.match, - value: caseSensitiveMatches[1], + value: removeEscapeChar(caseSensitiveMatches[1], true), keyLabel: '0', }, ], @@ -41,7 +62,7 @@ export function migrateLineFilterV1(serviceScene: ServiceScene) { { key: LineFilterCaseSensitive.caseInsensitive, operator: LineFilterOp.match, - value: caseInsensitiveMatches[1], + value: removeEscapeChar(caseInsensitiveMatches[1], false), keyLabel: '0', }, ]); @@ -49,8 +70,6 @@ export function migrateLineFilterV1(serviceScene: ServiceScene) { } // Remove from url without refreshing - const newLocation = locationService.getLocation(); - search.delete('var-lineFilter'); - newLocation.search = search.toString(); - locationService.replace(newLocation.pathname + '?' + newLocation.search); + delete search['var-lineFilter']; + locationService.replace(urlUtil.renderUrl(location.pathname, search)); } diff --git a/tests/exploreServicesBreakDown.spec.ts b/tests/exploreServicesBreakDown.spec.ts index c48e78ca..e84a2baa 100644 --- a/tests/exploreServicesBreakDown.spec.ts +++ b/tests/exploreServicesBreakDown.spec.ts @@ -1455,23 +1455,33 @@ test.describe('explore services breakdown page', () => { }); test('line filter migration case sensitive', async ({ page }) => { - await explorePage.gotoServicesOldUrlLineFilters('tempo-distributor', true); + // Checks chars that are escaped on-behalf of the user and chars that are user-escaped, e.g. `\n` (`%5C%5Cn`) => \n, `%5C.` (\.) => . + const urlEncodedAndEscaped = + '%5C%5Cnpage_url%3D%22https:%2F%2Fgrafana%5C.net%2Fexplore%5C%3Fleft%3D%5C%7B%22datasource%22:%22grafanacloud-prom%22,%22queries%22:%5C%5B%5C%7B%22datasource%22:%5C%7B%22type%22:%22prometheus%22,%22uid%22:%22grafanacloud-prom%22%5C%7D,%22expr%22:%22max%20by%20%5C%28kube_cluster_name,%20kube_namespace%5C%29%20%5C%28quantile_over_time%5C%280%5C.85,%20kubernetes_state_pod_age%5C%7Bplatform%3D%22data%22,kube_namespace%21~%22data-dev%5C%7Cdata-stg-%5C.%5C%2B%22,pod_phase%3D%22pending%22%5C%7D%5C%5B5m%5C%5D%5C%29%5C%29%20%3E%20600%22,%22refId%22:%22A%22%5C%7D%5C%5D,%22range%22:%5C%7B%22from%22:%22now-1h%22,%22to%22:%22now%22%5C%7D%5C%7D%22%60'; + const decodedAndUnescaped = + '`\\npage_url="https://grafana.net/explore?left={"datasource":"grafanacloud-prom","queries":[{"datasource":{"type":"prometheus","uid":"grafanacloud-prom"},"expr":"max by (kube_cluster_name, kube_namespace) (quantile_over_time(0.85, kubernetes_state_pod_age{platform="data",kube_namespace!~"data-dev|data-stg-.+",pod_phase="pending"}[5m])) > 600","refId":"A"}],"range":{"from":"now-1h","to":"now"}}"`'; + await explorePage.gotoServicesOldUrlLineFilters('tempo-distributor', true, urlEncodedAndEscaped); const firstLineFilterLoc = page.getByTestId(testIds.exploreServiceDetails.searchLogs).first(); await expect(firstLineFilterLoc).toHaveCount(1); await expect(page.getByLabel('Enable case match').nth(0)).toHaveCount(1); await expect(page.getByLabel('Disable case match')).toHaveCount(0); - await expect(firstLineFilterLoc).toHaveValue('debug'); + await expect(firstLineFilterLoc).toHaveValue(decodedAndUnescaped); }); test('line filter migration case insensitive', async ({ page }) => { - await explorePage.gotoServicesOldUrlLineFilters('tempo-distributor', false); + // The behavior for user entered escape chars differed between case sensitive/insensitive before the line filter regex feature, we want to preserve this bug in the migration so links from before this feature will return the same results + const urlEncodedAndEscaped = + '%5C%5Cnpage_url%3D%22https:%2F%2Fgrafana%5C.net%2Fexplore%5C%3Fleft%3D%5C%7B%22datasource%22:%22grafanacloud-prom%22,%22queries%22:%5C%5B%5C%7B%22datasource%22:%5C%7B%22type%22:%22prometheus%22,%22uid%22:%22grafanacloud-prom%22%5C%7D,%22expr%22:%22max%20by%20%5C%28kube_cluster_name,%20kube_namespace%5C%29%20%5C%28quantile_over_time%5C%280%5C.85,%20kubernetes_state_pod_age%5C%7Bplatform%3D%22data%22,kube_namespace%21~%22data-dev%5C%7Cdata-stg%22,pod_phase%3D%22pending%22%5C%7D%5C%5B5m%5C%5D%5C%29%5C%29%20%3E%20600%22,%22refId%22:%22A%22%5C%7D%5C%5D,%22range%22:%5C%7B%22from%22:%22now-1h%22,%22to%22:%22now%22%5C%7D%5C%7D%22%60'; + const decodedAndUnescaped = + '\\\\npage_url="https://grafana.net/explore?left={"datasource":"grafanacloud-prom","queries":[{"datasource":{"type":"prometheus","uid":"grafanacloud-prom"},"expr":"max by (kube_cluster_name, kube_namespace) (quantile_over_time(0.85, kubernetes_state_pod_age{platform="data",kube_namespace!~"data-dev|data-stg",pod_phase="pending"}[5m])) > 600","refId":"A"}],"range":{"from":"now-1h","to":"now"}}"'; + await explorePage.gotoServicesOldUrlLineFilters('tempo-distributor', false, urlEncodedAndEscaped); const firstLineFilterLoc = page.getByTestId(testIds.exploreServiceDetails.searchLogs).first(); await expect(firstLineFilterLoc).toHaveCount(1); await expect(page.getByLabel('Disable case match')).toHaveCount(1); await expect(page.getByLabel('Enable case match')).toHaveCount(1); - await expect(firstLineFilterLoc).toHaveValue('debug'); + await expect(firstLineFilterLoc).toHaveValue(decodedAndUnescaped); }); }); }); diff --git a/tests/fixtures/explore.ts b/tests/fixtures/explore.ts index e1ed72af..28987530 100644 --- a/tests/fixtures/explore.ts +++ b/tests/fixtures/explore.ts @@ -236,16 +236,20 @@ export class ExplorePage { ); } - async gotoServicesOldUrlLineFilters(serviceName = 'tempo-distributor', caseSensitive?: boolean) { + async gotoServicesOldUrlLineFilters( + serviceName = 'tempo-distributor', + caseSensitive?: boolean, + lineFilterValue = 'debug' + ) { if (caseSensitive) { await this.page.goto( // case insensitive - `/a/${pluginJson.id}/explore/service/tempo-distributor/logs?mode=service_details&patterns=[]&var-lineFilter=%7C~%20%60%28%3Fi%29debug%60&var-filters=service_name|=|${serviceName}&var-logsFormat= | logfmt` + `/a/${pluginJson.id}/explore/service/tempo-distributor/logs?mode=service_details&patterns=[]&var-lineFilter=%7C~%20%60%28%3Fi%29%60${lineFilterValue}%60&var-filters=service_name|=|${serviceName}&var-logsFormat= | logfmt` ); } else { await this.page.goto( // case insensitive - `/a/${pluginJson.id}/explore/service/tempo-distributor/logs?mode=service_details&patterns=[]&var-lineFilter=%7C%3D%20%60debug%60&var-filters=service_name|=|${serviceName}&var-logsFormat= | logfmt` + `/a/${pluginJson.id}/explore/service/tempo-distributor/logs?mode=service_details&patterns=[]&var-lineFilter=%7C%3D%20%60${lineFilterValue}%60&var-filters=service_name|=|${serviceName}&var-logsFormat= | logfmt` ); } }