From e5d644d9918c7e8b38e1791cca4a5f610c3349e2 Mon Sep 17 00:00:00 2001 From: Konrad Lalik Date: Fri, 14 Oct 2022 10:06:00 +0200 Subject: [PATCH] Alerting: Fix source and rule name decoding on Find route (#56805) --- .../unified/RedirectToRuleViewer.test.tsx | 69 +++++++++++++------ .../alerting/unified/RedirectToRuleViewer.tsx | 24 +++++-- 2 files changed, 67 insertions(+), 26 deletions(-) diff --git a/public/app/features/alerting/unified/RedirectToRuleViewer.test.tsx b/public/app/features/alerting/unified/RedirectToRuleViewer.test.tsx index c6610139044..ce6bcba3e5f 100644 --- a/public/app/features/alerting/unified/RedirectToRuleViewer.test.tsx +++ b/public/app/features/alerting/unified/RedirectToRuleViewer.test.tsx @@ -1,10 +1,10 @@ import { render, screen } from '@testing-library/react'; import React from 'react'; import { Provider } from 'react-redux'; -import { Router } from 'react-router-dom'; +import { MemoryRouter } from 'react-router-dom'; +import { useLocation } from 'react-use'; import { DataSourceJsonData, PluginMeta } from '@grafana/data'; -import { locationService } from '@grafana/runtime'; import { configureStore } from 'app/store/configureStore'; import { CombinedRule, Rule } from '../../../types/unified-alerting'; @@ -12,6 +12,7 @@ import { PromRuleType } from '../../../types/unified-alerting-dto'; import { RedirectToRuleViewer } from './RedirectToRuleViewer'; import { useCombinedRulesMatching } from './hooks/useCombinedRule'; +import * as combinedRuleHooks from './hooks/useCombinedRule'; import { getRulesSourceByName } from './utils/datasource'; jest.mock('./hooks/useCombinedRule'); @@ -21,13 +22,17 @@ jest.mock('react-router-dom', () => ({ Redirect: jest.fn(({}) => `Redirected`), })); +jest.mock('react-use'); + const store = configureStore(); -const renderRedirectToRuleViewer = () => { +const renderRedirectToRuleViewer = (pathname: string) => { + jest.mocked(useLocation).mockReturnValue({ pathname, trigger: '' }); + return render( - - - + + + ); }; @@ -55,7 +60,7 @@ describe('Redirect to Rule viewer', () => { error: undefined, }); mockRuleSourceByName(); - renderRedirectToRuleViewer(); + renderRedirectToRuleViewer('/alerting/test prom/prom alert/find'); expect(screen.getAllByText('Cloud test alert')).toHaveLength(2); }); @@ -68,7 +73,41 @@ describe('Redirect to Rule viewer', () => { error: undefined, }); mockRuleSourceByName(); - renderRedirectToRuleViewer(); + renderRedirectToRuleViewer('/alerting/test prom/prom alert/find'); + expect(screen.getByText('Redirected')).toBeInTheDocument(); + }); + + it('should properly decode rule name', () => { + const rulesMatchingSpy = jest.spyOn(combinedRuleHooks, 'useCombinedRulesMatching').mockReturnValue({ + result: [mockedRules[0]], + loading: false, + dispatched: true, + requestId: 'A', + error: undefined, + }); + + const ruleName = 'cloud rule++ !@#$%^&*()-/?'; + + renderRedirectToRuleViewer(`/alerting/prom-db/${encodeURIComponent(ruleName)}/find`); + + expect(rulesMatchingSpy).toHaveBeenCalledWith(ruleName, 'prom-db'); + expect(screen.getByText('Redirected')).toBeInTheDocument(); + }); + + it('should properly decode source name', () => { + const rulesMatchingSpy = jest.spyOn(combinedRuleHooks, 'useCombinedRulesMatching').mockReturnValue({ + result: [mockedRules[0]], + loading: false, + dispatched: true, + requestId: 'A', + error: undefined, + }); + + const sourceName = 'prom<|>++ !@#$%^&*()-/?'; + + renderRedirectToRuleViewer(`/alerting/${encodeURIComponent(sourceName)}/prom alert/find`); + + expect(rulesMatchingSpy).toHaveBeenCalledWith('prom alert', sourceName); expect(screen.getByText('Redirected')).toBeInTheDocument(); }); }); @@ -135,17 +174,3 @@ const mockedRules: CombinedRule[] = [ }, }, ]; - -const mockRoute = (ruleName: string, sourceName: string) => { - return { - route: { - path: '/', - component: RedirectToRuleViewer, - }, - queryParams: { returnTo: '/alerting/list' }, - match: { params: { name: ruleName, sourceName: sourceName }, isExact: false, url: 'asdf', path: '' }, - history: locationService.getHistory(), - location: { pathname: '', hash: '', search: '', state: '' }, - staticContext: {}, - }; -}; diff --git a/public/app/features/alerting/unified/RedirectToRuleViewer.tsx b/public/app/features/alerting/unified/RedirectToRuleViewer.tsx index b7b79d889af..e7f10bd1aee 100644 --- a/public/app/features/alerting/unified/RedirectToRuleViewer.tsx +++ b/public/app/features/alerting/unified/RedirectToRuleViewer.tsx @@ -1,10 +1,10 @@ import { css } from '@emotion/css'; import React from 'react'; import { Redirect } from 'react-router-dom'; +import { useLocation } from 'react-use'; import { GrafanaTheme2 } from '@grafana/data'; import { Alert, Card, Icon, LoadingPlaceholder, useStyles2, withErrorBoundary } from '@grafana/ui'; -import { GrafanaRouteComponentProps } from 'app/core/navigation/types'; import { AlertLabels } from './components/AlertLabels'; import { RuleViewerLayout } from './components/rule-viewer/RuleViewerLayout'; @@ -12,12 +12,28 @@ import { useCombinedRulesMatching } from './hooks/useCombinedRule'; import { getRulesSourceByName } from './utils/datasource'; import { createViewLink } from './utils/misc'; -type RedirectToRuleViewerProps = GrafanaRouteComponentProps<{ name?: string; sourceName?: string }>; const pageTitle = 'Find rule'; -export function RedirectToRuleViewer(props: RedirectToRuleViewerProps): JSX.Element | null { - const { name, sourceName } = props.match.params; +function useRuleFindParams() { + // DO NOT USE REACT-ROUTER HOOKS FOR THIS CODE + // React-router's useLocation/useParams/props.match are broken and don't preserve original param values when parsing location + // so, they cannot be used to parse name and sourceName path params + // React-router messes the pathname up resulting in a string that is neither encoded nor decoded + // Relevant issue: https://github.com/remix-run/history/issues/505#issuecomment-453175833 + // It was probably fixed in React-Router v6 + const location = useLocation(); + const segments = location.pathname?.split('/') ?? []; // ["", "alerting", "{sourceName}", "{name}] + + const name = decodeURIComponent(segments[3]); + const sourceName = decodeURIComponent(segments[2]); + + return { name, sourceName }; +} + +export function RedirectToRuleViewer(): JSX.Element | null { const styles = useStyles2(getStyles); + + const { name, sourceName } = useRuleFindParams(); const { error, loading, result: rules, dispatched } = useCombinedRulesMatching(name, sourceName); if (error) {