From af5296bee7571584af3c543e5738393fba3c1676 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hugo=20H=C3=A4ggmark?= Date: Thu, 23 Sep 2021 11:56:33 +0200 Subject: [PATCH] Variables: Prevents panel from crashing when using adhoc variable in data links (#39546) * Variables: Prevents panel from crashing when using adhoc variable in data links * Refactor: uses isAdhoc instead * Chore: updates after PR feedback --- .../app/features/templating/formatRegistry.ts | 51 +++++++++---- .../features/templating/template_srv.test.ts | 73 ++++++++++++++++++- .../app/features/templating/template_srv.ts | 22 ++++-- 3 files changed, 123 insertions(+), 23 deletions(-) diff --git a/public/app/features/templating/formatRegistry.ts b/public/app/features/templating/formatRegistry.ts index 1934add8c23..a6f11e7d9eb 100644 --- a/public/app/features/templating/formatRegistry.ts +++ b/public/app/features/templating/formatRegistry.ts @@ -16,10 +16,29 @@ export interface FormatRegistryItem extends RegistryItem { formatter(options: FormatOptions, variable: VariableModel): string; } +export enum FormatRegistryID { + lucene = 'lucene', + raw = 'raw', + regex = 'regex', + pipe = 'pipe', + distributed = 'distributed', + csv = 'csv', + html = 'html', + json = 'json', + percentEncode = 'percentencode', + singleQuote = 'singlequote', + doubleQuote = 'doublequote', + sqlString = 'sqlstring', + date = 'date', + glob = 'glob', + text = 'text', + queryParam = 'queryparam', +} + export const formatRegistry = new Registry(() => { const formats: FormatRegistryItem[] = [ { - id: 'lucene', + id: FormatRegistryID.lucene, name: 'Lucene', description: 'Values are lucene escaped and multi-valued variables generate an OR expression', formatter: ({ value }) => { @@ -39,13 +58,13 @@ export const formatRegistry = new Registry(() => { }, }, { - id: 'raw', + id: FormatRegistryID.raw, name: 'raw', description: 'Keep value as is', formatter: ({ value }) => value, }, { - id: 'regex', + id: FormatRegistryID.regex, name: 'Regex', description: 'Values are regex escaped and multi-valued variables generate a (|) expression', formatter: ({ value }) => { @@ -61,7 +80,7 @@ export const formatRegistry = new Registry(() => { }, }, { - id: 'pipe', + id: FormatRegistryID.pipe, name: 'Pipe', description: 'Values are separated by | character', formatter: ({ value }) => { @@ -72,7 +91,7 @@ export const formatRegistry = new Registry(() => { }, }, { - id: 'distributed', + id: FormatRegistryID.distributed, name: 'Distributed', description: 'Multiple values are formatted like variable=value', formatter: ({ value }, variable) => { @@ -91,7 +110,7 @@ export const formatRegistry = new Registry(() => { }, }, { - id: 'csv', + id: FormatRegistryID.csv, name: 'Csv', description: 'Comma-separated values', formatter: ({ value }) => { @@ -102,7 +121,7 @@ export const formatRegistry = new Registry(() => { }, }, { - id: 'html', + id: FormatRegistryID.html, name: 'HTML', description: 'HTML escaping of values', formatter: ({ value }) => { @@ -113,7 +132,7 @@ export const formatRegistry = new Registry(() => { }, }, { - id: 'json', + id: FormatRegistryID.json, name: 'JSON', description: 'JSON stringify valu', formatter: ({ value }) => { @@ -121,7 +140,7 @@ export const formatRegistry = new Registry(() => { }, }, { - id: 'percentencode', + id: FormatRegistryID.percentEncode, name: 'Percent encode', description: 'Useful for URL escaping values', formatter: ({ value }) => { @@ -133,7 +152,7 @@ export const formatRegistry = new Registry(() => { }, }, { - id: 'singlequote', + id: FormatRegistryID.singleQuote, name: 'Single quote', description: 'Single quoted values', formatter: ({ value }) => { @@ -146,7 +165,7 @@ export const formatRegistry = new Registry(() => { }, }, { - id: 'doublequote', + id: FormatRegistryID.doubleQuote, name: 'Double quote', description: 'Double quoted values', formatter: ({ value }) => { @@ -159,7 +178,7 @@ export const formatRegistry = new Registry(() => { }, }, { - id: 'sqlstring', + id: FormatRegistryID.sqlString, name: 'SQL string', description: 'SQL string quoting and commas for use in IN statements and other scenarios', formatter: ({ value }) => { @@ -172,7 +191,7 @@ export const formatRegistry = new Registry(() => { }, }, { - id: 'date', + id: FormatRegistryID.date, name: 'Date', description: 'Format date in different ways', formatter: ({ value, args }) => { @@ -191,7 +210,7 @@ export const formatRegistry = new Registry(() => { }, }, { - id: 'glob', + id: FormatRegistryID.glob, name: 'Glob', description: 'Format multi-valued variables using glob syntax, example {value1,value2}', formatter: ({ value }) => { @@ -202,7 +221,7 @@ export const formatRegistry = new Registry(() => { }, }, { - id: 'text', + id: FormatRegistryID.text, name: 'Text', description: 'Format variables in their text representation. Example in multi-variable scenario A + B + C.', formatter: (options, variable) => { @@ -220,7 +239,7 @@ export const formatRegistry = new Registry(() => { }, }, { - id: 'queryparam', + id: FormatRegistryID.queryParam, name: 'Query parameter', description: 'Format variables as URL parameters. Example in multi-variable scenario A + B + C => var-foo=A&var-foo=B&var-foo=C.', diff --git a/public/app/features/templating/template_srv.test.ts b/public/app/features/templating/template_srv.test.ts index f0b85fe347f..26724604cab 100644 --- a/public/app/features/templating/template_srv.test.ts +++ b/public/app/features/templating/template_srv.test.ts @@ -5,6 +5,7 @@ import { VariableAdapter, variableAdapters } from '../variables/adapters'; import { createQueryVariableAdapter } from '../variables/query/adapter'; import { createAdHocVariableAdapter } from '../variables/adhoc/adapter'; import { VariableModel } from '../variables/types'; +import { FormatRegistryID } from './formatRegistry'; variableAdapters.setInit(() => [ (createQueryVariableAdapter() as unknown) as VariableAdapter, @@ -660,6 +661,46 @@ describe('templateSrv', () => { }); }); + describe('adhoc variables', () => { + beforeEach(() => { + _templateSrv = initTemplateSrv([ + { + type: 'adhoc', + name: 'adhoc', + filters: [ + { + condition: '', + key: 'alertstate', + operator: '=', + value: 'firing', + }, + { + condition: '', + key: 'alertname', + operator: '=', + value: 'ExampleAlertAlwaysFiring', + }, + ], + }, + ]); + }); + + it(`should not be handled by any registry items except for queryparam`, () => { + const registryItems = Object.values(FormatRegistryID); + for (const registryItem of registryItems) { + if (registryItem === FormatRegistryID.queryParam) { + continue; + } + + const firstTarget = _templateSrv.replace(`\${adhoc:${registryItem}}`, {}); + expect(firstTarget).toBe(''); + + const secondTarget = _templateSrv.replace('${adhoc}', {}, registryItem); + expect(secondTarget).toBe(''); + } + }); + }); + describe('queryparam', () => { beforeEach(() => { _templateSrv = initTemplateSrv([ @@ -675,11 +716,29 @@ describe('templateSrv', () => { current: { value: ['value1', 'value2'] }, options: [{ value: 'value1' }, { value: 'value2' }], }, + { + type: 'adhoc', + name: 'adhoc', + filters: [ + { + condition: '', + key: 'alertstate', + operator: '=', + value: 'firing', + }, + { + condition: '', + key: 'alertname', + operator: '=', + value: 'ExampleAlertAlwaysFiring', + }, + ], + }, ]); }); it('query variable with single value with queryparam format should return correct queryparam', () => { - const target = _templateSrv.replace('${single:queryparam}', {}); + const target = _templateSrv.replace(`\${single:queryparam}`, {}); expect(target).toBe('var-single=value1'); }); @@ -689,7 +748,7 @@ describe('templateSrv', () => { }); it('query variable with multi value with queryparam format should return correct queryparam', () => { - const target = _templateSrv.replace('${multi:queryparam}', {}); + const target = _templateSrv.replace(`\${multi:queryparam}`, {}); expect(target).toBe('var-multi=value1&var-multi=value2'); }); @@ -697,5 +756,15 @@ describe('templateSrv', () => { const target = _templateSrv.replace('${multi}', {}, 'queryparam'); expect(target).toBe('var-multi=value1&var-multi=value2'); }); + + it('query variable with adhoc value with queryparam format should return correct queryparam', () => { + const target = _templateSrv.replace(`\${adhoc:queryparam}`, {}); + expect(target).toBe('var-adhoc=alertstate%7C%3D%7Cfiring&var-adhoc=alertname%7C%3D%7CExampleAlertAlwaysFiring'); + }); + + it('query variable with multi value and queryparam format should return correct queryparam', () => { + const target = _templateSrv.replace('${adhoc}', {}, 'queryparam'); + expect(target).toBe('var-adhoc=alertstate%7C%3D%7Cfiring&var-adhoc=alertname%7C%3D%7CExampleAlertAlwaysFiring'); + }); }); }); diff --git a/public/app/features/templating/template_srv.ts b/public/app/features/templating/template_srv.ts index 0a5ceabd8c8..a642954d7eb 100644 --- a/public/app/features/templating/template_srv.ts +++ b/public/app/features/templating/template_srv.ts @@ -1,12 +1,13 @@ -import { isString, property, escape } from 'lodash'; +import { escape, isString, property } from 'lodash'; import { deprecationWarning, ScopedVars, TimeRange } from '@grafana/data'; import { getFilteredVariables, getVariables, getVariableWithName } from '../variables/state/selectors'; import { variableRegex } from '../variables/utils'; import { isAdHoc } from '../variables/guard'; import { VariableModel } from '../variables/types'; import { setTemplateSrv, TemplateSrv as BaseTemplateSrv } from '@grafana/runtime'; -import { FormatOptions, formatRegistry } from './formatRegistry'; +import { FormatOptions, formatRegistry, FormatRegistryID } from './formatRegistry'; import { ALL_VARIABLE_TEXT, ALL_VARIABLE_VALUE } from '../variables/state/types'; +import { safeStringifyValue } from '../../core/utils/explore'; interface FieldAccessorCache { [key: string]: (obj: any) => any; @@ -115,6 +116,10 @@ export class TemplateSrv implements BaseTemplateSrv { return ''; } + if (isAdHoc(variable) && format !== FormatRegistryID.queryParam) { + return ''; + } + // if it's an object transform value to string if (!Array.isArray(value) && typeof value === 'object') { value = `${value}`; @@ -125,7 +130,7 @@ export class TemplateSrv implements BaseTemplateSrv { } if (!format) { - format = 'glob'; + format = FormatRegistryID.glob; } // some formats have arguments that come after ':' character @@ -141,7 +146,7 @@ export class TemplateSrv implements BaseTemplateSrv { if (!formatItem) { console.error(`Variable format ${format} not found. Using glob format as fallback.`); - formatItem = formatRegistry.get('glob'); + formatItem = formatRegistry.get(FormatRegistryID.glob); } const options: FormatOptions = { value, args, text: text ?? value }; @@ -270,6 +275,13 @@ export class TemplateSrv implements BaseTemplateSrv { return match; } + if (isAdHoc(variable)) { + const value = safeStringifyValue(variable.filters); + const text = variable.id; + + return this.formatValue(value, fmt, variable, text); + } + const systemValue = this.grafanaVariables[variable.current.value]; if (systemValue) { return this.formatValue(systemValue, fmt, variable); @@ -282,7 +294,7 @@ export class TemplateSrv implements BaseTemplateSrv { value = this.getAllValue(variable); text = ALL_VARIABLE_TEXT; // skip formatting of custom all values - if (variable.allValue && fmt !== 'text' && fmt !== 'queryparam') { + if (variable.allValue && fmt !== FormatRegistryID.text && fmt !== FormatRegistryID.queryParam) { return this.replace(value); } }