Merge pull request #5760 from influxdata/5757/variable_clash

fix: enforce unique dashboard variable names

CHANGELOG.md

@@ -41,6 +41,8 @@
 1. [#5747](https://github.com/influxdata/chronograf/pull/5747): Manage individual execution status per query.
 1. [#5754](https://github.com/influxdata/chronograf/pull/5754): Add macOS ARM64 builds.
 1. [#5758](https://github.com/influxdata/chronograf/pull/5758): Parse exported dashboard in a resources directory.
+1. [#5757](https://github.com/influxdata/chronograf/pull/5757): Enforce unique dashboard variable names.
+
 
 ### Other
 

server/dashboards.go

@@ -221,7 +221,7 @@ func (s *Service) UpdateDashboard(w http.ResponseWriter, r *http.Request) {
 		}
 		orig.Cells = req.Cells
 	} else {
-		invalidData(w, fmt.Errorf("Update must include either name or cells"), s.Logger)
+		invalidData(w, fmt.Errorf("update must include either name or cells"), s.Logger)
 		return
 	}
 
@@ -246,6 +246,10 @@ func ValidDashboardRequest(d *chronograf.Dashboard, defaultOrgID string) error {
 		}
 		d.Cells[i] = c
 	}
+
+	if err := ValidUniqueTemplateVariables(d); err != nil {
+		return err
+	}
 	for _, t := range d.Templates {
 		if err := ValidTemplateRequest(&t); err != nil {
 			return err

server/dashboards_test.go

@@ -1,6 +1,7 @@
 package server
 
 import (
+	"fmt"
 	"reflect"
 	"testing"
 
@@ -196,12 +197,59 @@ func TestValidDashboardRequest(t *testing.T) {
 				},
 			},
 		},
+		{
+			name: "Checks unique dashboard variables",
+			d: chronograf.Dashboard{
+				Organization: "1337",
+				Cells: []chronograf.DashboardCell{
+					{
+						W: 0,
+						H: 0,
+						Queries: []chronograf.DashboardQuery{
+							{
+								Command: "SELECT donors from hill_valley_preservation_society where time > 1985-10-25T08:00:00",
+								Type:    "influxql",
+							},
+						},
+					},
+					{
+						W: 2,
+						H: 2,
+						Queries: []chronograf.DashboardQuery{
+							{
+								Command: "SELECT winning_horses from grays_sports_alamanc where time > 1955-11-1T00:00:00",
+								Type:    "influxql",
+							},
+						},
+					},
+				},
+				Templates: []chronograf.Template{
+					{
+						TemplateVar: chronograf.TemplateVar{Var: "a"},
+						ID:          "1",
+						Type:        "databases",
+						Label:       "a",
+						Query:       &chronograf.TemplateQuery{Command: "SHOW DATABASES"},
+					},
+					{
+						TemplateVar: chronograf.TemplateVar{Var: "a"},
+						ID:          "1",
+						Type:        "databases",
+						Label:       "a",
+						Query:       &chronograf.TemplateQuery{Command: "SHOW DATABASES"},
+					},
+				},
+			},
+			wantErr: true,
+		},
 	}
 	for _, tt := range tests {
-		// TODO(desa): this Okay?
 		err := ValidDashboardRequest(&tt.d, "0")
-		if (err != nil) != tt.wantErr {
-			t.Errorf("%q. ValidDashboardRequest() error = %v, wantErr %v", tt.name, err, tt.wantErr)
+		fmt.Println(err)
+		if tt.wantErr {
+			if err == nil {
+				t.Errorf("%q. ValidDashboardRequest() error = nil, wantErr %v", tt.name, tt.wantErr)
+			}
 			continue
 		}
 		if diff := cmp.Diff(tt.d, tt.want); diff != "" {
@@ -285,7 +333,7 @@ func Test_newDashboardResponse(t *testing.T) {
 										GroupBy: chronograf.GroupBy{
 											Tags: []string{},
 										},
-										Tags:            make(map[string][]string, 0),
+										Tags:            make(map[string][]string),
 										AreTagsAccepted: false,
 										Shifts: []chronograf.TimeShift{
 											{
@@ -349,7 +397,7 @@ func Test_newDashboardResponse(t *testing.T) {
 										GroupBy: chronograf.GroupBy{
 											Tags: []string{},
 										},
-										Tags:            make(map[string][]string, 0),
+										Tags:            make(map[string][]string),
 										AreTagsAccepted: false,
 										Range: &chronograf.DurationRange{
 											Lower: "now() - 15m",

server/templates.go

@@ -14,14 +14,14 @@ import (
 func ValidTemplateRequest(template *chronograf.Template) error {
 	switch template.Type {
 	default:
-		return fmt.Errorf("Unknown template type %s", template.Type)
+		return fmt.Errorf("unknown template type %s", template.Type)
 	case "constant", "csv", "fieldKeys", "tagKeys", "tagValues", "measurements", "databases", "map", "influxql", "text", "flux":
 	}
 
 	for _, v := range template.Values {
 		switch v.Type {
 		default:
-			return fmt.Errorf("Unknown template variable type %s", v.Type)
+			return fmt.Errorf("unknown template variable type %s", v.Type)
 		case "csv", "map", "fieldKey", "tagKey", "tagValue", "measurement", "database", "constant", "influxql", "flux":
 		}
 
@@ -31,12 +31,24 @@ func ValidTemplateRequest(template *chronograf.Template) error {
 	}
 
 	if (template.Type == "influxql" || template.Type == "flux") && template.Query == nil {
-		return fmt.Errorf("No query set for template of type '%s'", template.Type)
+		return fmt.Errorf("no query set for template of type '%s'", template.Type)
 	}
 
 	return nil
 }
 
+// ValidUniqueTemplateVariables validates that a template defines variable at most once
+func ValidUniqueTemplateVariables(dashboard *chronograf.Dashboard) error {
+	variableNames := map[string]struct{}{}
+	for _, t := range dashboard.Templates {
+		if _, duplicate := variableNames[t.Var]; duplicate {
+			return fmt.Errorf("duplicate variable name %s", t.Var)
+		}
+		variableNames[t.Var] = struct{}{}
+	}
+	return nil
+}
+
 type templateLinks struct {
 	Self string `json:"self"` // Self link mapping to this resource
 }
@@ -125,6 +137,10 @@ func (s *Service) NewTemplate(w http.ResponseWriter, r *http.Request) {
 	template.ID = chronograf.TemplateID(tid)
 
 	dash.Templates = append(dash.Templates, template)
+	if err := ValidUniqueTemplateVariables(&dash); err != nil {
+		invalidData(w, err, s.Logger)
+		return
+	}
 	if err := s.Store.Dashboards(ctx).Update(ctx, dash); err != nil {
 		msg := fmt.Sprintf("Error adding template %s to dashboard %d: %v", tid, id, err)
 		Error(w, http.StatusInternalServerError, msg, s.Logger)
@@ -241,6 +257,10 @@ func (s *Service) ReplaceTemplate(w http.ResponseWriter, r *http.Request) {
 	template.ID = chronograf.TemplateID(tid)
 
 	dash.Templates[pos] = template
+	if err := ValidUniqueTemplateVariables(&dash); err != nil {
+		invalidData(w, err, s.Logger)
+		return
+	}
 	if err := s.Store.Dashboards(ctx).Update(ctx, dash); err != nil {
 		msg := fmt.Sprintf("Error updating template %s in dashboard %d: %v", tid, id, err)
 		Error(w, http.StatusInternalServerError, msg, s.Logger)

ui/src/tempVars/components/TemplateVariableEditor.tsx

@@ -407,19 +407,40 @@ class TemplateVariableEditor extends PureComponent<Props, State> {
   private get canSave(): boolean {
     const {
       nextTemplate: {tempVar, type, values},
+      isNew,
     } = this.state
 
-    let canSaveValues = true
-    if (type === TemplateType.CSV && isEmpty(values)) {
-      canSaveValues = false
+    const templates = this.props.templates || []
+    if (tempVar === '') {
+      return false
     }
+    if (type === TemplateType.CSV && isEmpty(values)) {
+      return false
+    }
+    const variable = formatTempVar(tempVar)
+    if (RESERVED_TEMPLATE_NAMES.includes(variable)) {
+      return false
+    }
+    if (isNew && templates.some(t => t.tempVar === variable)) {
+      // duplicate variable on create
+      return false
+    }
+    const template = this.props.template
 
-    return (
-      tempVar !== '' &&
-      canSaveValues &&
-      !RESERVED_TEMPLATE_NAMES.includes(formatTempVar(tempVar)) &&
-      !this.isSaving
-    )
+    if (
+      !isNew &&
+      template &&
+      templates.reduce((acc, t) => {
+        if (t.tempVar === variable && template.id !== t.id) {
+          return acc + 1
+        }
+        return acc
+      }, 0) > 0
+    ) {
+      // duplicate variable on update
+      return false
+    }
+    return !this.isSaving
   }
 
   private get dropdownSelection(): string {