From 0c8390cea26b55126bfae9b0a48b738c8bd46ebc Mon Sep 17 00:00:00 2001 From: Marcus Andersson Date: Mon, 14 Sep 2020 08:05:51 +0200 Subject: [PATCH] Templating: global/system variables should be properly replaced in templated values. (#27394) * Fixed so we try to use the variables in the redux store to replace values in template variables. * First draft of working version. * Including fieldPath when adding :text format. * cleaned up code by introducing helper function. * some minor refactoring. * Added tests and support for multi variables. * added test and code to handle the All scenario of a multivariable. * fixed according to feedback. * added docs. * added text format to gdev dashboard. * updated e2e tests. * make sure we use the same function for formatting och variable lable. * increased the number to 22. * changed label for tests to be All. * existing format should be respected. --- .../global-variables-and-interpolation.json | 2 +- .../advanced-variable-format-options.md | 10 +++ e2e/suite1/specs/dashboard-templating.spec.ts | 3 +- .../app/features/templating/formatRegistry.ts | 55 +++++++++++----- .../features/templating/template_srv.test.ts | 50 +++++++++++++- .../app/features/templating/template_srv.ts | 65 +++++++++---------- .../pickers/OptionsPicker/OptionsPicker.tsx | 41 +----------- .../variables/shared/formatVariable.ts | 51 +++++++++++++++ 8 files changed, 186 insertions(+), 91 deletions(-) create mode 100644 public/app/features/variables/shared/formatVariable.ts diff --git a/devenv/dev-dashboards/feature-templating/global-variables-and-interpolation.json b/devenv/dev-dashboards/feature-templating/global-variables-and-interpolation.json index 1a8dd87b76a..86aae9c58bf 100644 --- a/devenv/dev-dashboards/feature-templating/global-variables-and-interpolation.json +++ b/devenv/dev-dashboards/feature-templating/global-variables-and-interpolation.json @@ -34,7 +34,7 @@ }, "id": 11, "options": { - "content": "## Global variables\n\n* `__dashboard` = `${__dashboard}`\n* `__dashboard.name` = `${__dashboard.name}`\n* `__dashboard.uid` = `${__dashboard.uid}`\n* `__org.name` = `${__org.name}`\n* `__org.id` = `${__org.id}`\n* `__user.id` = `${__user.id}`\n* `__user.login` = `${__user.login}`\n \n## Formats\n\n* `Server:raw` = `${Server:raw}`\n* `Server:regex` = `${Server:regex}`\n* `Server:lucene` = `${Server:lucene}`\n* `Server:glob` = `${Server:glob}`\n* `Server:pipe` = `${Server:pipe}`\n* `Server:distributed` = `${Server:distributed}`\n* `Server:csv` = `${Server:csv}`\n* `Server:html` = `${Server:html}`\n* `Server:json` = `${Server:json}`\n* `Server:percentencode` = `${Server:percentencode}`\n* `Server:singlequote` = `${Server:singlequote}`\n* `Server:doublequote` = `${Server:doublequote}`\n* `Server:sqlstring` = `${Server:sqlstring}`\n* `Server:date` = `${Server:date}`\n\n", + "content": "## Global variables\n\n* `__dashboard` = `${__dashboard}`\n* `__dashboard.name` = `${__dashboard.name}`\n* `__dashboard.uid` = `${__dashboard.uid}`\n* `__org.name` = `${__org.name}`\n* `__org.id` = `${__org.id}`\n* `__user.id` = `${__user.id}`\n* `__user.login` = `${__user.login}`\n \n## Formats\n\n* `Server:raw` = `${Server:raw}`\n* `Server:regex` = `${Server:regex}`\n* `Server:lucene` = `${Server:lucene}`\n* `Server:glob` = `${Server:glob}`\n* `Server:pipe` = `${Server:pipe}`\n* `Server:distributed` = `${Server:distributed}`\n* `Server:csv` = `${Server:csv}`\n* `Server:html` = `${Server:html}`\n* `Server:json` = `${Server:json}`\n* `Server:percentencode` = `${Server:percentencode}`\n* `Server:singlequote` = `${Server:singlequote}`\n* `Server:doublequote` = `${Server:doublequote}`\n* `Server:sqlstring` = `${Server:sqlstring}`\n* `Server:date` = `${Server:date}`\n* `Server:text` = `${Server:text}`\n\n", "mode": "markdown" }, "pluginVersion": "7.1.0", diff --git a/docs/sources/variables/advanced-variable-format-options.md b/docs/sources/variables/advanced-variable-format-options.md index 9429120d9a4..8c8d14f7d0e 100644 --- a/docs/sources/variables/advanced-variable-format-options.md +++ b/docs/sources/variables/advanced-variable-format-options.md @@ -143,3 +143,13 @@ servers = ["test'1", "test2"] String to interpolate: '${servers:sqlstring}' Interpolation result: "'test''1','test2'" ``` + +## Text + +Formats single- and multi-valued variables into their text representation. For a single variable it will just return the text representation. For multi-valued variables it will return the text representation combined with `+`. + +```bash +servers = ["test1", "test2"] +String to interpolate: '${servers:text}' +Interpolation result: "test1 + test2" +``` \ No newline at end of file diff --git a/e2e/suite1/specs/dashboard-templating.spec.ts b/e2e/suite1/specs/dashboard-templating.spec.ts index 91fd9c372a4..87e30ba2677 100644 --- a/e2e/suite1/specs/dashboard-templating.spec.ts +++ b/e2e/suite1/specs/dashboard-templating.spec.ts @@ -33,11 +33,12 @@ e2e.scenario({ `Server:doublequote = "A'A\\"A","BB\\B","CCC"`, `Server:sqlstring = 'A''A"A','BB\\\B','CCC'`, `Server:date = null`, + `Server:text = All`, ]; e2e() .get('.markdown-html li') - .should('have.length', 21) + .should('have.length', 22) .each(element => { items.push(element.text()); }) diff --git a/public/app/features/templating/formatRegistry.ts b/public/app/features/templating/formatRegistry.ts index abf9566e936..40b0690aca7 100644 --- a/public/app/features/templating/formatRegistry.ts +++ b/public/app/features/templating/formatRegistry.ts @@ -1,9 +1,16 @@ import kbn from 'app/core/utils/kbn'; import { Registry, RegistryItem, VariableModel, textUtil, dateTime } from '@grafana/data'; import { map, isArray, replace } from 'lodash'; +import { formatVariableLabel } from '../variables/shared/formatVariable'; + +export interface FormatOptions { + value: any; + text: string; + args: string[]; +} export interface FormatRegistryItem extends RegistryItem { - formatter(value: any, args: string[], variable: VariableModel): string; + formatter(options: FormatOptions, variable: VariableModel): string; } export const formatRegistry = new Registry(() => { @@ -12,7 +19,7 @@ export const formatRegistry = new Registry(() => { id: 'lucene', name: 'Lucene', description: 'Values are lucene escaped and multi-valued variables generate an OR expression', - formatter: value => { + formatter: ({ value }) => { if (typeof value === 'string') { return luceneEscape(value); } @@ -32,13 +39,13 @@ export const formatRegistry = new Registry(() => { id: 'raw', name: 'raw', description: 'Keep value as is', - formatter: value => value, + formatter: ({ value }) => value, }, { id: 'regex', name: 'Regex', description: 'Values are regex escaped and multi-valued variables generate a (|) expression', - formatter: value => { + formatter: ({ value }) => { if (typeof value === 'string') { return kbn.regexEscape(value); } @@ -54,7 +61,7 @@ export const formatRegistry = new Registry(() => { id: 'pipe', name: 'Pipe', description: 'Values are seperated by | character', - formatter: value => { + formatter: ({ value }) => { if (typeof value === 'string') { return value; } @@ -65,7 +72,7 @@ export const formatRegistry = new Registry(() => { id: 'distributed', name: 'Distributed', description: 'Multiple values are formatted like variable=value', - formatter: (value, args, variable) => { + formatter: ({ value }, variable) => { if (typeof value === 'string') { return value; } @@ -84,7 +91,7 @@ export const formatRegistry = new Registry(() => { id: 'csv', name: 'Csv', description: 'Comma seperated values', - formatter: (value, args, variable) => { + formatter: ({ value }) => { if (isArray(value)) { return value.join(','); } @@ -95,7 +102,7 @@ export const formatRegistry = new Registry(() => { id: 'html', name: 'HTML', description: 'HTML escaping of values', - formatter: (value, args, variable) => { + formatter: ({ value }) => { if (isArray(value)) { return textUtil.escapeHtml(value.join(', ')); } @@ -106,7 +113,7 @@ export const formatRegistry = new Registry(() => { id: 'json', name: 'JSON', description: 'JSON stringify valu', - formatter: (value, args, variable) => { + formatter: ({ value }) => { return JSON.stringify(value); }, }, @@ -114,7 +121,7 @@ export const formatRegistry = new Registry(() => { id: 'percentencode', name: 'Percent encode', description: 'Useful for url escaping values', - formatter: (value, args, variable) => { + formatter: ({ value }) => { // like glob, but url escaped if (isArray(value)) { return encodeURIComponentStrict('{' + value.join(',') + '}'); @@ -126,7 +133,7 @@ export const formatRegistry = new Registry(() => { id: 'singlequote', name: 'Single quote', description: 'Single quoted values', - formatter: (value, args, variable) => { + formatter: ({ value }) => { // escape single quotes with backslash const regExp = new RegExp(`'`, 'g'); if (isArray(value)) { @@ -139,7 +146,7 @@ export const formatRegistry = new Registry(() => { id: 'doublequote', name: 'Double quote', description: 'Double quoted values', - formatter: (value, args, variable) => { + formatter: ({ value }) => { // escape double quotes with backslash const regExp = new RegExp('"', 'g'); if (isArray(value)) { @@ -152,7 +159,7 @@ export const formatRegistry = new Registry(() => { id: 'sqlstring', name: 'SQL string', description: 'SQL string quoting and commas for use in IN statements and other scenarios', - formatter: (value, args, variable) => { + formatter: ({ value }) => { // escape single quotes by pairing them const regExp = new RegExp(`'`, 'g'); if (isArray(value)) { @@ -165,7 +172,7 @@ export const formatRegistry = new Registry(() => { id: 'date', name: 'Date', description: 'Format date in different ways', - formatter: (value, args, variable) => { + formatter: ({ value, args }) => { const arg = args[0] ?? 'iso'; switch (arg) { @@ -184,13 +191,31 @@ export const formatRegistry = new Registry(() => { id: 'glob', name: 'Glob', description: 'Format multi valued variables using glob syntax, example {value1,value2}', - formatter: (value, args, variable) => { + formatter: ({ value }) => { if (isArray(value) && value.length > 1) { return '{' + value.join(',') + '}'; } return value; }, }, + { + id: 'text', + name: 'Text', + description: 'Format variables in their text representation. Example in multi variable scenario A + B + C.', + formatter: (options, variable) => { + if (typeof options.text === 'string') { + return options.text; + } + + const current = (variable as any)?.current; + + if (!current) { + return options.value; + } + + return formatVariableLabel(variable); + }, + }, ]; return formats; diff --git a/public/app/features/templating/template_srv.test.ts b/public/app/features/templating/template_srv.test.ts index d1e7a98c22a..d4acb2608a4 100644 --- a/public/app/features/templating/template_srv.test.ts +++ b/public/app/features/templating/template_srv.test.ts @@ -95,11 +95,18 @@ describe('templateSrv', () => { expect(target).toBe('this.asd.filters'); }); - it('should replace ${test:glob} with scoped text', () => { + it('should replace ${test.name} with scoped text', () => { + const target = _templateSrv.replaceWithText('this.${test.name}.filters', { + test: { value: { name: 'mupp' }, text: 'asd' }, + }); + expect(target).toBe('this.mupp.filters'); + }); + + it('should not replace ${test:glob} with scoped text', () => { const target = _templateSrv.replaceWithText('this.${test:glob}.filters', { test: { value: 'mupp', text: 'asd' }, }); - expect(target).toBe('this.asd.filters'); + expect(target).toBe('this.mupp.filters'); }); }); @@ -595,6 +602,45 @@ describe('templateSrv', () => { }); }); + describe('replaceWithText can pass all / multi value', () => { + beforeEach(() => { + initTemplateSrv([ + { + type: 'query', + name: 'server', + current: { value: ['server1', 'server2'], text: ['Server 1', 'Server 2'] }, + }, + { + type: 'textbox', + name: 'empty_on_init', + current: { value: '', text: '' }, + }, + { + type: 'query', + name: 'databases', + current: { value: '$__all', text: '' }, + options: [{ value: '$__all' }, { value: 'db1', text: 'Database 1' }, { value: 'db2', text: 'Database 2' }], + }, + ]); + _templateSrv.updateIndex(); + }); + + it('should replace with text with variable label', () => { + const target = _templateSrv.replaceWithText('Server: $server'); + expect(target).toBe('Server: Server 1 + Server 2'); + }); + + it('should replace empty string-values with an empty string', () => { + const target = _templateSrv.replaceWithText('Hello $empty_on_init'); + expect(target).toBe('Hello '); + }); + + it('should replace $__all with All', () => { + const target = _templateSrv.replaceWithText('Db: $databases'); + expect(target).toBe('Db: All'); + }); + }); + describe('built in interval variables', () => { beforeEach(() => { initTemplateSrv([]); diff --git a/public/app/features/templating/template_srv.ts b/public/app/features/templating/template_srv.ts index 379b2952370..74f1cb4bc92 100644 --- a/public/app/features/templating/template_srv.ts +++ b/public/app/features/templating/template_srv.ts @@ -6,7 +6,8 @@ import { isAdHoc } from '../variables/guard'; import { VariableModel } from '../variables/types'; import { setTemplateSrv, TemplateSrv as BaseTemplateSrv } from '@grafana/runtime'; import { variableAdapters } from '../variables/adapters'; -import { formatRegistry } from './formatRegistry'; +import { formatRegistry, FormatOptions } from './formatRegistry'; +import { ALL_VARIABLE_TEXT } from '../variables/state/types'; interface FieldAccessorCache { [key: string]: (obj: any) => any; @@ -107,7 +108,7 @@ export class TemplateSrv implements BaseTemplateSrv { return filters; } - formatValue(value: any, format: any, variable: any) { + formatValue(value: any, format: any, variable: any, text?: string) { // for some scopedVars there is no variable variable = variable || {}; @@ -133,7 +134,8 @@ export class TemplateSrv implements BaseTemplateSrv { throw new Error(`Variable format ${format} not found`); } - return formatItem.formatter(value, args, variable); + const options: FormatOptions = { value, args, text: text ?? value }; + return formatItem.formatter(options, variable); } setGrafanaVariable(name: string, value: any) { @@ -197,7 +199,7 @@ export class TemplateSrv implements BaseTemplateSrv { return values; } - getFieldAccessor(fieldPath: string) { + private getFieldAccessor(fieldPath: string) { const accessor = this.fieldAccessorCache[fieldPath]; if (accessor) { return accessor; @@ -206,7 +208,7 @@ export class TemplateSrv implements BaseTemplateSrv { return (this.fieldAccessorCache[fieldPath] = _.property(fieldPath)); } - getVariableValue(variableName: string, fieldPath: string | undefined, scopedVars: ScopedVars) { + private getVariableValue(variableName: string, fieldPath: string | undefined, scopedVars: ScopedVars) { const scopedVar = scopedVars[variableName]; if (!scopedVar) { return null; @@ -219,6 +221,20 @@ export class TemplateSrv implements BaseTemplateSrv { return scopedVar.value; } + private getVariableText(variableName: string, value: any, scopedVars: ScopedVars) { + const scopedVar = scopedVars[variableName]; + + if (!scopedVar) { + return null; + } + + if (scopedVar.value === value || typeof value !== 'string') { + return scopedVar.text; + } + + return value; + } + replace(target?: string, scopedVars?: ScopedVars, format?: string | Function): string { if (!target) { return target ?? ''; @@ -233,8 +249,10 @@ export class TemplateSrv implements BaseTemplateSrv { if (scopedVars) { const value = this.getVariableValue(variableName, fieldPath, scopedVars); + const text = this.getVariableText(variableName, value, scopedVars); + if (value !== null && value !== undefined) { - return this.formatValue(value, fmt, variable); + return this.formatValue(value, fmt, variable, text); } } @@ -248,8 +266,11 @@ export class TemplateSrv implements BaseTemplateSrv { } let value = variable.current.value; + let text = variable.current.text; + if (this.isAllValue(value)) { value = this.getAllValue(variable); + text = ALL_VARIABLE_TEXT; // skip formatting of custom all values if (variable.allValue) { return this.replace(value); @@ -258,14 +279,14 @@ export class TemplateSrv implements BaseTemplateSrv { if (fieldPath) { const fieldValue = this.getVariableValue(variableName, fieldPath, { - [variableName]: { value: value, text: '' }, + [variableName]: { value, text }, }); if (fieldValue !== null && fieldValue !== undefined) { - return this.formatValue(fieldValue, fmt, variable); + return this.formatValue(fieldValue, fmt, variable, text); } } - const res = this.formatValue(value, fmt, variable); + const res = this.formatValue(value, fmt, variable, text); return res; }); } @@ -275,30 +296,8 @@ export class TemplateSrv implements BaseTemplateSrv { } replaceWithText(target: string, scopedVars?: ScopedVars) { - if (!target) { - return target; - } - - let variable; - this.regex.lastIndex = 0; - - return target.replace(this.regex, (match: any, var1: any, var2: any, fmt2: any, var3: any) => { - if (scopedVars) { - const option = scopedVars[var1 || var2 || var3]; - if (option) { - return option.text; - } - } - - variable = this.getVariableAtIndex(var1 || var2 || var3); - if (!variable) { - return match; - } - - const value = this.grafanaVariables[variable.current.value]; - - return typeof value === 'string' ? value : variable.current.text; - }); + deprecationWarning('template_srv.ts', 'replaceWithText()', 'replace(), and specify the :text format'); + return this.replace(target, scopedVars, 'text'); } fillVariableValuesForUrl = (params: any, scopedVars?: ScopedVars) => { diff --git a/public/app/features/variables/pickers/OptionsPicker/OptionsPicker.tsx b/public/app/features/variables/pickers/OptionsPicker/OptionsPicker.tsx index 7709b303166..a5901e0ace6 100644 --- a/public/app/features/variables/pickers/OptionsPicker/OptionsPicker.tsx +++ b/public/app/features/variables/pickers/OptionsPicker/OptionsPicker.tsx @@ -10,6 +10,7 @@ import { VariableOption, VariableTag, VariableWithMultiSupport, VariableWithOpti import { VariableOptions } from '../shared/VariableOptions'; import { isQuery } from '../../guard'; import { VariablePickerProps } from '../types'; +import { formatVariableLabel } from '../../shared/formatVariable'; interface OwnProps extends VariablePickerProps {} @@ -64,7 +65,7 @@ export class OptionsPickerUnconnected extends PureComponent { return null; } - const linkText = getLinkText(variable); + const linkText = formatVariableLabel(variable); const tags = getSelectedTags(variable); return ; @@ -104,44 +105,6 @@ const getSelectedTags = (variable: VariableWithOptions): VariableTag[] => { return variable.tags.filter(t => t.selected); }; -const getLinkText = (variable: VariableWithOptions) => { - const { current, options } = variable; - - if (!current.tags || current.tags.length === 0) { - if (Array.isArray(current.text)) { - return current.text.join(' + '); - } - return current.text; - } - - // filer out values that are in selected tags - const selectedAndNotInTag = options.filter(option => { - if (!option.selected) { - return false; - } - - if (!current || !current.tags || !current.tags.length) { - return false; - } - - for (let i = 0; i < current.tags.length; i++) { - const tag = current.tags[i]; - const foundIndex = tag?.values?.findIndex(v => v === option.value); - if (foundIndex && foundIndex !== -1) { - return false; - } - } - return true; - }); - - // convert values to text - const currentTexts = selectedAndNotInTag.map(s => s.text); - - // join texts - const newLinkText = currentTexts.join(' + '); - return newLinkText.length > 0 ? `${newLinkText} + ` : newLinkText; -}; - const mapDispatchToProps: MapDispatchToProps = { showOptions, commitChangesToVariable, diff --git a/public/app/features/variables/shared/formatVariable.ts b/public/app/features/variables/shared/formatVariable.ts new file mode 100644 index 00000000000..c0daaf98d51 --- /dev/null +++ b/public/app/features/variables/shared/formatVariable.ts @@ -0,0 +1,51 @@ +import { VariableModel } from '@grafana/data'; +import { VariableWithOptions } from '../types'; + +export const formatVariableLabel = (variable: VariableModel) => { + if (!isVariableWithOptions(variable)) { + return variable.name; + } + + const { current, options = [] } = variable; + + if (!current.tags || current.tags.length === 0) { + if (Array.isArray(current.text)) { + return current.text.join(' + '); + } + return current.text; + } + + // filer out values that are in selected tags + const selectedAndNotInTag = options.filter(option => { + if (!option.selected) { + return false; + } + + if (!current || !current.tags || !current.tags.length) { + return false; + } + + for (let i = 0; i < current.tags.length; i++) { + const tag = current.tags[i]; + const foundIndex = tag?.values?.findIndex(v => v === option.value); + if (foundIndex && foundIndex !== -1) { + return false; + } + } + return true; + }); + + // convert values to text + const currentTexts = selectedAndNotInTag.map(s => s.text); + + // join texts + const newLinkText = currentTexts.join(' + '); + return newLinkText.length > 0 ? `${newLinkText} + ` : newLinkText; +}; + +const isVariableWithOptions = (variable: VariableModel): variable is VariableWithOptions => { + return ( + Array.isArray((variable as VariableWithOptions)?.options) || + typeof (variable as VariableWithOptions)?.current === 'object' + ); +};