From 732ea8eea5cc739ab6d7201e2f35727c65210dc9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Torkel=20=C3=96degaard?= Date: Fri, 22 Mar 2019 12:26:26 +0100 Subject: [PATCH] Graphite: fixed variable quoting when variable value is nummeric, fixes #2078 --- .../app/plugins/datasource/graphite/gfunc.ts | 13 ++++-- .../datasource/graphite/graphite_query.ts | 10 ++-- .../datasource/graphite/specs/gfunc.test.ts | 46 +++++++++++++++---- .../graphite/specs/graphite_query.test.ts | 3 +- .../graphite/specs/query_ctrl.test.ts | 5 +- public/test/specs/helpers.ts | 2 +- 6 files changed, 57 insertions(+), 22 deletions(-) diff --git a/public/app/plugins/datasource/graphite/gfunc.ts b/public/app/plugins/datasource/graphite/gfunc.ts index 20cb884d617..15cb79f00b5 100644 --- a/public/app/plugins/datasource/graphite/gfunc.ts +++ b/public/app/plugins/datasource/graphite/gfunc.ts @@ -1,5 +1,6 @@ import _ from 'lodash'; import { isVersionGtOrEq } from 'app/core/utils/version'; +import { InterpolateFunction } from '@grafana/ui'; const index = {}; @@ -961,24 +962,30 @@ export class FuncInstance { this.updateText(); } - render(metricExp) { + render(metricExp: string, replaceVariables: InterpolateFunction): string { const str = this.def.name + '('; const parameters = _.map(this.params, (value, index) => { + const valueInterpolated = replaceVariables(value); let paramType; + if (index < this.def.params.length) { paramType = this.def.params[index].type; } else if (_.get(_.last(this.def.params), 'multiple')) { paramType = _.get(_.last(this.def.params), 'type'); } + // param types that should never be quoted if (_.includes(['value_or_series', 'boolean', 'int', 'float', 'node'], paramType)) { return value; } + // param types that might be quoted - if (_.includes(['int_or_interval', 'node_or_tag'], paramType) && _.isFinite(+value)) { - return _.toString(+value); + // To quote variables correctly we need to interpolate it to check if it contains a numeric or string value + if (_.includes(['int_or_interval', 'node_or_tag'], paramType) && _.isFinite(+valueInterpolated)) { + return _.toString(value); } + return "'" + value + "'"; }); diff --git a/public/app/plugins/datasource/graphite/graphite_query.ts b/public/app/plugins/datasource/graphite/graphite_query.ts index 6e863958b78..adbcde69ad7 100644 --- a/public/app/plugins/datasource/graphite/graphite_query.ts +++ b/public/app/plugins/datasource/graphite/graphite_query.ts @@ -18,6 +18,7 @@ export default class GraphiteQuery { constructor(datasource, target, templateSrv?, scopedVars?) { this.datasource = datasource; this.target = target; + this.templateSrv = templateSrv; this.parseTarget(); this.removeTagValue = '-- remove tag --'; @@ -160,7 +161,10 @@ export default class GraphiteQuery { } updateModelTarget(targets) { - // render query + const wrapFunction = (target: string, func: any) => { + return func.render(target, this.templateSrv.replace); + }; + if (!this.target.textEditor) { const metricPath = this.getSegmentPathUpTo(this.segments.length).replace(/\.select metric$/, ''); this.target.target = _.reduce(this.functions, wrapFunction, metricPath); @@ -302,10 +306,6 @@ export default class GraphiteQuery { } } -function wrapFunction(target, func) { - return func.render(target); -} - function renderTagString(tag) { return tag.key + tag.operator + tag.value; } diff --git a/public/app/plugins/datasource/graphite/specs/gfunc.test.ts b/public/app/plugins/datasource/graphite/specs/gfunc.test.ts index 1809adc0940..a1d888cea33 100644 --- a/public/app/plugins/datasource/graphite/specs/gfunc.test.ts +++ b/public/app/plugins/datasource/graphite/specs/gfunc.test.ts @@ -30,66 +30,92 @@ describe('when creating func instance from func names', () => { }); }); +function replaceVariablesDummy(str: string) { + return str; +} + describe('when rendering func instance', () => { it('should handle single metric param', () => { const func = gfunc.createFuncInstance('sumSeries'); - expect(func.render('hello.metric')).toEqual('sumSeries(hello.metric)'); + expect(func.render('hello.metric', replaceVariablesDummy)).toEqual('sumSeries(hello.metric)'); }); it('should include default params if options enable it', () => { const func = gfunc.createFuncInstance('scaleToSeconds', { withDefaultParams: true, }); - expect(func.render('hello')).toEqual('scaleToSeconds(hello, 1)'); + expect(func.render('hello', replaceVariablesDummy)).toEqual('scaleToSeconds(hello, 1)'); }); it('should handle int or interval params with number', () => { const func = gfunc.createFuncInstance('movingMedian'); func.params[0] = '5'; - expect(func.render('hello')).toEqual('movingMedian(hello, 5)'); + expect(func.render('hello', replaceVariablesDummy)).toEqual('movingMedian(hello, 5)'); }); it('should handle int or interval params with interval string', () => { const func = gfunc.createFuncInstance('movingMedian'); func.params[0] = '5min'; - expect(func.render('hello')).toEqual("movingMedian(hello, '5min')"); + expect(func.render('hello', replaceVariablesDummy)).toEqual("movingMedian(hello, '5min')"); }); it('should never quote boolean paramater', () => { const func = gfunc.createFuncInstance('sortByName'); func.params[0] = '$natural'; - expect(func.render('hello')).toEqual('sortByName(hello, $natural)'); + expect(func.render('hello', replaceVariablesDummy)).toEqual('sortByName(hello, $natural)'); }); it('should never quote int paramater', () => { const func = gfunc.createFuncInstance('maximumAbove'); func.params[0] = '$value'; - expect(func.render('hello')).toEqual('maximumAbove(hello, $value)'); + expect(func.render('hello', replaceVariablesDummy)).toEqual('maximumAbove(hello, $value)'); }); it('should never quote node paramater', () => { const func = gfunc.createFuncInstance('aliasByNode'); func.params[0] = '$node'; - expect(func.render('hello')).toEqual('aliasByNode(hello, $node)'); + expect(func.render('hello', replaceVariablesDummy)).toEqual('aliasByNode(hello, $node)'); }); it('should handle metric param and int param and string param', () => { const func = gfunc.createFuncInstance('groupByNode'); func.params[0] = 5; func.params[1] = 'avg'; - expect(func.render('hello.metric')).toEqual("groupByNode(hello.metric, 5, 'avg')"); + expect(func.render('hello.metric', replaceVariablesDummy)).toEqual("groupByNode(hello.metric, 5, 'avg')"); }); it('should handle function with no metric param', () => { const func = gfunc.createFuncInstance('randomWalk'); func.params[0] = 'test'; - expect(func.render(undefined)).toEqual("randomWalk('test')"); + expect(func.render(undefined, replaceVariablesDummy)).toEqual("randomWalk('test')"); }); it('should handle function multiple series params', () => { const func = gfunc.createFuncInstance('asPercent'); func.params[0] = '#B'; - expect(func.render('#A')).toEqual('asPercent(#A, #B)'); + expect(func.render('#A', replaceVariablesDummy)).toEqual('asPercent(#A, #B)'); + }); + + it('should not quote variables that have numeric value', () => { + const func = gfunc.createFuncInstance('movingAverage'); + func.params[0] = '$variable'; + + const replaceVariables = (str: string) => { + return str.replace('$variable', '60'); + }; + + expect(func.render('metric', replaceVariables)).toBe('movingAverage(metric, $variable)'); + }); + + it('should quote variables that have string value', () => { + const func = gfunc.createFuncInstance('movingAverage'); + func.params[0] = '$variable'; + + const replaceVariables = (str: string) => { + return str.replace('$variable', '10min'); + }; + + expect(func.render('metric', replaceVariables)).toBe("movingAverage(metric, '$variable')"); }); }); diff --git a/public/app/plugins/datasource/graphite/specs/graphite_query.test.ts b/public/app/plugins/datasource/graphite/specs/graphite_query.test.ts index 311dfec333a..241a21ab40d 100644 --- a/public/app/plugins/datasource/graphite/specs/graphite_query.test.ts +++ b/public/app/plugins/datasource/graphite/specs/graphite_query.test.ts @@ -1,5 +1,6 @@ import gfunc from '../gfunc'; import GraphiteQuery from '../graphite_query'; +import { TemplateSrvStub } from 'test/specs/helpers'; describe('Graphite query model', () => { const ctx: any = { @@ -9,7 +10,7 @@ describe('Graphite query model', () => { waitForFuncDefsLoaded: jest.fn().mockReturnValue(Promise.resolve(null)), createFuncInstance: gfunc.createFuncInstance, }, - templateSrv: {}, + templateSrv: new TemplateSrvStub(), targets: [], }; diff --git a/public/app/plugins/datasource/graphite/specs/query_ctrl.test.ts b/public/app/plugins/datasource/graphite/specs/query_ctrl.test.ts index d69f5235832..cb88c697d7d 100644 --- a/public/app/plugins/datasource/graphite/specs/query_ctrl.test.ts +++ b/public/app/plugins/datasource/graphite/specs/query_ctrl.test.ts @@ -1,6 +1,7 @@ import { uiSegmentSrv } from 'app/core/services/segment_srv'; import gfunc from '../gfunc'; import { GraphiteQueryCtrl } from '../query_ctrl'; +import { TemplateSrvStub } from 'test/specs/helpers'; describe('GraphiteQueryCtrl', () => { const ctx = { @@ -30,7 +31,7 @@ describe('GraphiteQueryCtrl', () => { {}, {}, new uiSegmentSrv({ trustAsHtml: html => html }, { highlightVariablesAsHtml: () => {} }), - {}, + new TemplateSrvStub(), {} ); }); @@ -291,7 +292,7 @@ describe('GraphiteQueryCtrl', () => { ctx.ctrl.target.target = "seriesByTag('tag1=value1', 'tag2!=~value2')"; ctx.ctrl.datasource.metricFindQuery = () => Promise.resolve([{ expandable: false }]); ctx.ctrl.parseTarget(); - ctx.ctrl.removeTag(0); + ctx.ctrl.tagChanged({ key: ctx.ctrl.removeTagValue }); }); it('should update tags', () => { diff --git a/public/test/specs/helpers.ts b/public/test/specs/helpers.ts index f9124773c97..8710754f04e 100644 --- a/public/test/specs/helpers.ts +++ b/public/test/specs/helpers.ts @@ -172,7 +172,7 @@ export function TemplateSrvStub(this: any) { this.variables = []; this.templateSettings = { interpolate: /\[\[([\s\S]+?)\]\]/g }; this.data = {}; - this.replace = function(text: string) { + this.replace = (text: string) => { return _.template(text, this.templateSettings)(this.data); }; this.init = () => {};