From b424e12a5ad4774db9413e838284e947d169dbb5 Mon Sep 17 00:00:00 2001
From: gotjosh <josue@grafana.com>
Date: Wed, 7 Aug 2019 14:50:06 +0100
Subject: [PATCH] Fix: Avoid glob of single-value array variables  (#18420)

* Fix: Avoid glob of single-value array variables

Based on our current implementation of templates, when multi-select
variables are part of a dashboard query the default/fallback formatting option is `glob`.

Some data sources do not support glob (e.g. metrics.{a}.* instead of
metrics.a.*) for single variable queries. This behaviour breaks dashboards.

This commit introduces an alternative formatting option where globing is avoided if it's there is only one value as part of the query variable.

This means, queries previously formatted as `query=metrics.{a}.*.*`, are
now formatted as `query=metrics.a.*.*`. However, queries formatted as
`query=metrics.{a,b}.*.*` continue to be as is.
---
 .../templating/specs/template_srv.test.ts     | 17 +++++++
 .../app/features/templating/template_srv.ts   |  2 +-
 .../graphite/specs/datasource.test.ts         | 46 +++++++++++++++++--
 3 files changed, 60 insertions(+), 5 deletions(-)

diff --git a/public/app/features/templating/specs/template_srv.test.ts b/public/app/features/templating/specs/template_srv.test.ts
index 1535392138c..8f5c5b72212 100644
--- a/public/app/features/templating/specs/template_srv.test.ts
+++ b/public/app/features/templating/specs/template_srv.test.ts
@@ -112,6 +112,23 @@ describe('templateSrv', () => {
       expect(target).toBe('this.{value1,value2}.filters');
     });
 
+    describe('when the globbed variable only has one value', () => {
+      beforeEach(() => {
+        initTemplateSrv([
+          {
+            type: 'query',
+            name: 'test',
+            current: { value: ['value1'] },
+          },
+        ]);
+      });
+
+      it('should not glob the value', () => {
+        const target = _templateSrv.replace('this.$test.filters', {}, 'glob');
+        expect(target).toBe('this.value1.filters');
+      });
+    });
+
     it('should replace ${test} with globbed value', () => {
       const target = _templateSrv.replace('this.${test}.filters', {}, 'glob');
       expect(target).toBe('this.{value1,value2}.filters');
diff --git a/public/app/features/templating/template_srv.ts b/public/app/features/templating/template_srv.ts
index a37be2c4998..f43f6449717 100644
--- a/public/app/features/templating/template_srv.ts
+++ b/public/app/features/templating/template_srv.ts
@@ -172,7 +172,7 @@ export class TemplateSrv {
         return this.encodeURIComponentStrict(value);
       }
       default: {
-        if (_.isArray(value)) {
+        if (_.isArray(value) && value.length > 1) {
           return '{' + value.join(',') + '}';
         }
         return value;
diff --git a/public/app/plugins/datasource/graphite/specs/datasource.test.ts b/public/app/plugins/datasource/graphite/specs/datasource.test.ts
index eb4920b7739..70f086aa433 100644
--- a/public/app/plugins/datasource/graphite/specs/datasource.test.ts
+++ b/public/app/plugins/datasource/graphite/specs/datasource.test.ts
@@ -2,7 +2,7 @@ import { GraphiteDatasource } from '../datasource';
 import _ from 'lodash';
 // @ts-ignore
 import $q from 'q';
-import { TemplateSrvStub } from 'test/specs/helpers';
+import { TemplateSrv } from 'app/features/templating/template_srv';
 import { dateTime } from '@grafana/data';
 
 describe('graphiteDatasource', () => {
@@ -10,7 +10,7 @@ describe('graphiteDatasource', () => {
     backendSrv: {},
     $q,
     // @ts-ignore
-    templateSrv: new TemplateSrvStub(),
+    templateSrv: new TemplateSrv(),
     instanceSettings: { url: 'url', name: 'graphiteProd', jsonData: {} },
   };
 
@@ -218,6 +218,38 @@ describe('graphiteDatasource', () => {
       });
       expect(results.length).toBe(2);
     });
+
+    describe('when formatting targets', () => {
+      it('does not attempt to glob for one variable', () => {
+        ctx.ds.templateSrv.init([
+          {
+            type: 'query',
+            name: 'metric',
+            current: { value: ['b'] },
+          },
+        ]);
+
+        const results = ctx.ds.buildGraphiteParams({
+          targets: [{ target: 'my.$metric.*' }],
+        });
+        expect(results).toStrictEqual(['target=my.b.*', 'format=json']);
+      });
+
+      it('globs for more than one variable', () => {
+        ctx.ds.templateSrv.init([
+          {
+            type: 'query',
+            name: 'metric',
+            current: { value: ['a', 'b'] },
+          },
+        ]);
+
+        const results = ctx.ds.buildGraphiteParams({
+          targets: [{ target: 'my.[[metric]].*' }],
+        });
+        expect(results).toStrictEqual(['target=my.%7Ba%2Cb%7D.*', 'format=json']);
+      });
+    });
   });
 
   describe('querying for template variables', () => {
@@ -308,7 +340,13 @@ describe('graphiteDatasource', () => {
     });
 
     it('/metrics/find should be POST', () => {
-      ctx.templateSrv.setGrafanaVariable('foo', 'bar');
+      ctx.ds.templateSrv.init([
+        {
+          type: 'query',
+          name: 'foo',
+          current: { value: ['bar'] },
+        },
+      ]);
       ctx.ds.metricFindQuery('[[foo]]').then((data: any) => {
         results = data;
       });
@@ -327,7 +365,7 @@ function accessScenario(name: string, url: string, fn: any) {
       backendSrv: {},
       $q,
       // @ts-ignore
-      templateSrv: new TemplateSrvStub(),
+      templateSrv: new TemplateSrv(),
       instanceSettings: { url: 'url', name: 'graphiteProd', jsonData: {} },
     };