From fc2f2adc017d21aef067b7ef30d6ec6e9c63a0a4 Mon Sep 17 00:00:00 2001 From: Pavel Zavora Date: Wed, 2 Jun 2021 10:34:39 +0200 Subject: [PATCH 01/10] fix(server): validate unique template variable names --- server/dashboards.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/server/dashboards.go b/server/dashboards.go index 7e259e751..4e5e70b0b 100644 --- a/server/dashboards.go +++ b/server/dashboards.go @@ -246,7 +246,13 @@ func ValidDashboardRequest(d *chronograf.Dashboard, defaultOrgID string) error { } d.Cells[i] = c } + + variableNames := map[string]struct{}{} for _, t := range d.Templates { + if _, duplicate := variableNames[t.Var]; duplicate { + return fmt.Errorf("duplicate variable name %s", t.Var) + } + variableNames[t.Var] = struct{}{} if err := ValidTemplateRequest(&t); err != nil { return err } From 97ed58bcbe5159a009bd8220311f4cec7ca9a7eb Mon Sep 17 00:00:00 2001 From: Pavel Zavora Date: Wed, 2 Jun 2021 10:38:58 +0200 Subject: [PATCH 02/10] feat(server): introduce ValidUniqueTemplateVariables --- server/templates.go | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/server/templates.go b/server/templates.go index 721f33119..a2d23000d 100644 --- a/server/templates.go +++ b/server/templates.go @@ -37,6 +37,21 @@ func ValidTemplateRequest(template *chronograf.Template) error { 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{}{} + if err := ValidTemplateRequest(&t); err != nil { + return err + } + } + return nil +} + type templateLinks struct { Self string `json:"self"` // Self link mapping to this resource } From c830f244b06ed04149b29a5ae1f37849f56e4660 Mon Sep 17 00:00:00 2001 From: Pavel Zavora Date: Wed, 2 Jun 2021 10:39:38 +0200 Subject: [PATCH 03/10] fix(server): repair error messages --- server/dashboards.go | 2 +- server/templates.go | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/server/dashboards.go b/server/dashboards.go index 4e5e70b0b..183bc65cb 100644 --- a/server/dashboards.go +++ b/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 } diff --git a/server/templates.go b/server/templates.go index a2d23000d..8db16e28b 100644 --- a/server/templates.go +++ b/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,7 +31,7 @@ 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 From 65f15ed88604381c16e38628801b35d09e874ca6 Mon Sep 17 00:00:00 2001 From: Pavel Zavora Date: Wed, 2 Jun 2021 11:05:37 +0200 Subject: [PATCH 04/10] fix(server): use ValidUniqueTemplateVariables fn --- server/dashboards.go | 8 +++--- server/dashboards_test.go | 54 ++++++++++++++++++++++++++++++++++++--- 2 files changed, 54 insertions(+), 8 deletions(-) diff --git a/server/dashboards.go b/server/dashboards.go index 183bc65cb..59ae10749 100644 --- a/server/dashboards.go +++ b/server/dashboards.go @@ -247,12 +247,10 @@ func ValidDashboardRequest(d *chronograf.Dashboard, defaultOrgID string) error { d.Cells[i] = c } - variableNames := map[string]struct{}{} + if err := ValidUniqueTemplateVariables(d); err != nil { + return err + } for _, t := range d.Templates { - if _, duplicate := variableNames[t.Var]; duplicate { - return fmt.Errorf("duplicate variable name %s", t.Var) - } - variableNames[t.Var] = struct{}{} if err := ValidTemplateRequest(&t); err != nil { return err } diff --git a/server/dashboards_test.go b/server/dashboards_test.go index 161d9ef63..72dbfff58 100644 --- a/server/dashboards_test.go +++ b/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 != "" { From dedcf00e5ac5298a87a7b04732b062fc12c2148a Mon Sep 17 00:00:00 2001 From: Pavel Zavora Date: Wed, 2 Jun 2021 11:06:56 +0200 Subject: [PATCH 05/10] fix(server): better scope ValidUniqueTemplateVariables --- server/templates.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/server/templates.go b/server/templates.go index 8db16e28b..c654b4af6 100644 --- a/server/templates.go +++ b/server/templates.go @@ -45,9 +45,6 @@ func ValidUniqueTemplateVariables(dashboard *chronograf.Dashboard) error { return fmt.Errorf("duplicate variable name %s", t.Var) } variableNames[t.Var] = struct{}{} - if err := ValidTemplateRequest(&t); err != nil { - return err - } } return nil } From f8054fbbb394aa351948b33805d047cfbf34d3ff Mon Sep 17 00:00:00 2001 From: Pavel Zavora Date: Wed, 2 Jun 2021 11:14:16 +0200 Subject: [PATCH 06/10] fix(server): use ValidUniqueTemplateVariables in templates API --- server/templates.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/server/templates.go b/server/templates.go index c654b4af6..173be9438 100644 --- a/server/templates.go +++ b/server/templates.go @@ -137,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) @@ -253,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) From 1aa80724c82f4867515cc347312b6d4ad789bde1 Mon Sep 17 00:00:00 2001 From: Pavel Zavora Date: Wed, 2 Jun 2021 11:14:59 +0200 Subject: [PATCH 07/10] chore: fix staticcheck errors --- server/dashboards_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/server/dashboards_test.go b/server/dashboards_test.go index 72dbfff58..0d85b4056 100644 --- a/server/dashboards_test.go +++ b/server/dashboards_test.go @@ -333,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{ { @@ -397,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", From 4eaca3e43689f06954a3d0a2ba07bacb0135b78b Mon Sep 17 00:00:00 2001 From: Pavel Zavora Date: Wed, 2 Jun 2021 12:10:20 +0200 Subject: [PATCH 08/10] fix(ui): disallow to create or update variable --- .../components/TemplateVariableEditor.tsx | 39 ++++++++++++++----- 1 file changed, 30 insertions(+), 9 deletions(-) diff --git a/ui/src/tempVars/components/TemplateVariableEditor.tsx b/ui/src/tempVars/components/TemplateVariableEditor.tsx index 81c2fb059..f3d7fa542 100644 --- a/ui/src/tempVars/components/TemplateVariableEditor.tsx +++ b/ui/src/tempVars/components/TemplateVariableEditor.tsx @@ -407,19 +407,40 @@ class TemplateVariableEditor extends PureComponent { 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 { From efca22a26b49d7aff89e2cc6619c3cb3d78c805e Mon Sep 17 00:00:00 2001 From: Pavel Zavora Date: Wed, 2 Jun 2021 15:32:11 +0200 Subject: [PATCH 09/10] chore: update changelog --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8248b0110..9a40bd4c1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -41,6 +41,9 @@ 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. [#5754](https://github.com/influxdata/chronograf/pull/5754): Add macOS arm64 builds. +1. [#5757](https://github.com/influxdata/chronograf/pull/5757): Enforce unique dashboard variable names. + ### Other From bbdc775dd36cdd6be0fb355d47b4696c599023fb Mon Sep 17 00:00:00 2001 From: Pavel Zavora Date: Sat, 5 Jun 2021 08:21:56 +0200 Subject: [PATCH 10/10] chore: repair changelog --- CHANGELOG.md | 1 - 1 file changed, 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9a40bd4c1..c3c9da22b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -41,7 +41,6 @@ 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. [#5754](https://github.com/influxdata/chronograf/pull/5754): Add macOS arm64 builds. 1. [#5757](https://github.com/influxdata/chronograf/pull/5757): Enforce unique dashboard variable names.