Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: use urlUtil instead of UrlSearchParams #994

Merged
merged 8 commits into from
Jan 10, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 5 additions & 6 deletions src/Components/IndexScene/ShareButtonScene.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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);
};
39 changes: 29 additions & 10 deletions src/services/migrations.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious about the unusual approach. Why not a regex?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was struggling to get the regex to work last night, I'll take another look

Copy link
Contributor Author

@gtk-grafana gtk-grafana Jan 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nevermind, I made this implementation more complicated to fix the user-entered escape char case, so I think it's beyond my regex abilities without dumping a bunch more time into this.

.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;
}

Expand All @@ -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',
},
],
Expand All @@ -41,16 +62,14 @@ export function migrateLineFilterV1(serviceScene: ServiceScene) {
{
key: LineFilterCaseSensitive.caseInsensitive,
operator: LineFilterOp.match,
value: caseInsensitiveMatches[1],
value: removeEscapeChar(caseInsensitiveMatches[1], false),
keyLabel: '0',
},
]);
});
}

// 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));
}
18 changes: 14 additions & 4 deletions tests/exploreServicesBreakDown.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Took me a while to understand the trick with the string encoded/decoded.

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);
});
});
});
10 changes: 7 additions & 3 deletions tests/fixtures/explore.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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`
);
}
}
Expand Down
Loading