Merge pull request #16149 from grafana/graphite-variable-quoting

Graphite: fixed variable quoting when variable value is nummeric

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 + "'";
     });
 

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

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')");
   });
 });
 

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: [],
   };
 

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', () => {

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 = () => {};