Validates the reference to metrics from the report config.
Also, validates that metric parts are not nil.
Change-Id: Ic172e4ebfa2189382fe78c8def7090a39d938d66
diff --git a/config/config_parser/src/config_parser/project_config.go b/config/config_parser/src/config_parser/project_config.go
index bc4a088..c5f803f 100644
--- a/config/config_parser/src/config_parser/project_config.go
+++ b/config/config_parser/src/config_parser/project_config.go
@@ -21,6 +21,7 @@
import (
"config"
"fmt"
+ "github.com/golang/glog"
"yamlpb"
)
@@ -41,9 +42,13 @@
return fmt.Errorf("Error while parsing yaml: %v", err)
}
- // TODO(azani): Validate that metrics and encodings referred to exist.
- metricIds := map[uint32]bool{}
+ // Maps metric ids to the metric's index in c.projectConfig.MetricConfigs.
+ metrics := map[uint32]uint32{}
+
+ // Set of encoding ids. Used to detect duplicates.
encodingIds := map[uint32]bool{}
+
+ // Set of report ids. Used to detect duplicates.
reportIds := map[uint32]bool{}
for i, e := range c.projectConfig.EncodingConfigs {
if encodingIds[e.Id] {
@@ -55,10 +60,14 @@
}
for i, e := range c.projectConfig.MetricConfigs {
- if metricIds[e.Id] {
+ if _, ok := metrics[e.Id]; ok {
return fmt.Errorf("Metric id '%v' is repeated in metric config entry number %v. Metric ids must be unique.", e.Id, i)
}
- metricIds[e.Id] = true
+ metrics[e.Id] = uint32(i)
+
+ if err := validateMetric(*e); err != nil {
+ return fmt.Errorf("Error validating metric %v (%v): %v", e.Name, e.Id, err)
+ }
e.CustomerId = c.customerId
e.ProjectId = c.projectId
}
@@ -70,6 +79,63 @@
reportIds[e.Id] = true
e.CustomerId = c.customerId
e.ProjectId = c.projectId
+
+ if _, ok := metrics[e.MetricId]; !ok {
+ return fmt.Errorf("Error validating report %v (%v): There is no metric id %v.", e.Name, e.Id, e.MetricId)
+ }
+
+ m := c.projectConfig.MetricConfigs[metrics[e.MetricId]]
+ if err := validateReportVariables(*e, *m); err != nil {
+ return fmt.Errorf("Error validating report %v (%v): %v", e.Name, e.Id, err)
+ }
+ }
+
+ return nil
+}
+
+func validateMetric(m config.Metric) (err error) {
+ for k, v := range m.Parts {
+ if v == nil {
+ return fmt.Errorf("Metric part '%v' is null. This is not allowed.", k)
+ }
+ }
+
+ return nil
+}
+
+// Checks that the report variables are compatible with the specific metric.
+func validateReportVariables(c config.ReportConfig, m config.Metric) (err error) {
+ if len(c.Variable) == 0 {
+ glog.Warningf("Report '%v' (Customer %v, Project %v Id %v) does not have any report variables.", c.Name, c.CustomerId, c.ProjectId, c.Id)
+ return nil
+ }
+
+ for i, v := range c.Variable {
+ if v == nil {
+ return fmt.Errorf("Report Variable in position %v is null. This is not allowed.", i)
+ }
+
+ // Check that the metric part being referenced can be found.
+ p, ok := m.Parts[v.MetricPart]
+ if !ok {
+ return fmt.Errorf("Metric part '%v' is not present in metric '%v'.", v.MetricPart, m.Name)
+ }
+
+ // Checks that if index labels are found, the metric part referred to is an index.
+ if v.IndexLabels != nil && len(v.IndexLabels.Labels) > 0 && p.DataType != config.MetricPart_INDEX {
+ return fmt.Errorf("Report variable %v has index labels specified "+
+ "which implies referring to an index metric part. But metric part '%v'"+
+ "of metric '%v' (%v) is of type %v.",
+ i, v.MetricPart, m.Name, m.Id, config.MetricPart_DataType_name[int32(p.DataType)])
+ }
+
+ // Checks that if RAPPOR candidates are found, the metric part referred to is a string.
+ if v.RapporCandidates != nil && len(v.RapporCandidates.Candidates) > 0 && p.DataType != config.MetricPart_STRING {
+ return fmt.Errorf("Report variable %v has RAPPOR candidates specified "+
+ "which implies referring to a string metric part. But metric part '%v'"+
+ "of metric '%v' (%v) is of type %v.",
+ i, v.MetricPart, m.Name, m.Id, config.MetricPart_DataType_name[int32(p.DataType)])
+ }
}
return nil
diff --git a/config/config_parser/src/config_parser/project_config_test.go b/config/config_parser/src/config_parser/project_config_test.go
index d79e133..6da6b0c 100644
--- a/config/config_parser/src/config_parser/project_config_test.go
+++ b/config/config_parser/src/config_parser/project_config_test.go
@@ -39,8 +39,9 @@
- id: 2
report_configs:
- id: 1
- metric_id: 5
+ metric_id: 1
- id: 2
+ metric_id: 1
`
c := projectConfig{
customerId: 1,
@@ -91,12 +92,13 @@
CustomerId: 1,
ProjectId: 10,
Id: 1,
- MetricId: 5,
+ MetricId: 1,
},
&config.ReportConfig{
CustomerId: 1,
ProjectId: 10,
Id: 2,
+ MetricId: 1,
},
},
}
@@ -201,3 +203,122 @@
t.Error("Accepted non-unique report id.")
}
}
+
+// Test that a report config with an unknown metric id gets rejected.
+func TestParseProjectConfigUnknownMetricIdInReportConfig(t *testing.T) {
+ y := `
+report_configs:
+- id: 1
+ metric_id: 10
+`
+ c := projectConfig{}
+
+ if err := parseProjectConfig(y, &c); err == nil {
+ t.Error("Accepted report config with unknown metric id.")
+ }
+}
+
+// Check that valid report variables are accepted.
+func TestValidateReportVariables(t *testing.T) {
+ m := config.Metric{
+ Parts: map[string]*config.MetricPart{
+ "int_part": &config.MetricPart{DataType: config.MetricPart_INT},
+ "string_part": &config.MetricPart{DataType: config.MetricPart_STRING},
+ "blob_part": &config.MetricPart{DataType: config.MetricPart_BLOB},
+ "index_part": &config.MetricPart{DataType: config.MetricPart_INDEX},
+ },
+ }
+
+ c := config.ReportConfig{
+ Variable: []*config.ReportVariable{
+ &config.ReportVariable{
+ MetricPart: "int_part",
+ },
+ &config.ReportVariable{
+ MetricPart: "index_part",
+ IndexLabels: &config.IndexLabels{Labels: map[uint32]string{0: "zero"}},
+ },
+ &config.ReportVariable{
+ MetricPart: "string_part",
+ RapporCandidates: &config.RapporCandidateList{Candidates: []string{"hello"}},
+ },
+ },
+ }
+
+ if err := validateReportVariables(c, m); err != nil {
+ t.Error(err)
+ }
+}
+
+// Test that a report variable referring to an unknown metric part will be rejected.
+func TestValidateReportVariablesUnknownMetricPart(t *testing.T) {
+ m := config.Metric{}
+
+ c := config.ReportConfig{
+ Variable: []*config.ReportVariable{
+ &config.ReportVariable{
+ MetricPart: "int_part",
+ },
+ },
+ }
+
+ if err := validateReportVariables(c, m); err == nil {
+ t.Error("Report with unknown metric part was accepted.")
+ }
+}
+
+// Test that if a report variable specifies index labels, the metric part it
+// refers to must be of type index.
+func TestValidateReportVariablesIndexLablesNonIndexMetric(t *testing.T) {
+ m := config.Metric{
+ Parts: map[string]*config.MetricPart{
+ "int_part": &config.MetricPart{DataType: config.MetricPart_INT},
+ },
+ }
+
+ c := config.ReportConfig{
+ Variable: []*config.ReportVariable{
+ &config.ReportVariable{
+ MetricPart: "int_part",
+ IndexLabels: &config.IndexLabels{Labels: map[uint32]string{0: "zero"}},
+ },
+ },
+ }
+
+ if err := validateReportVariables(c, m); err == nil {
+ t.Error("Report with with index labels specified for a non-index metric part accepted.")
+ }
+}
+
+// Test that if a report variable specifies rappor candidates, the metric part
+// it refers to must be of type string.
+func TestValidateReportVarialesRapporCandidatesNonStringMetric(t *testing.T) {
+ m := config.Metric{
+ Parts: map[string]*config.MetricPart{
+ "int_part": &config.MetricPart{DataType: config.MetricPart_INT},
+ },
+ }
+
+ c := config.ReportConfig{
+ Variable: []*config.ReportVariable{
+ &config.ReportVariable{
+ MetricPart: "int_part",
+ RapporCandidates: &config.RapporCandidateList{Candidates: []string{"alpha"}},
+ },
+ },
+ }
+
+ if err := validateReportVariables(c, m); err == nil {
+ t.Error("Report with with rappor candidates specified for a non-string metric part accepted.")
+ }
+}
+
+func TestValidateMetricNoNilMetricPart(t *testing.T) {
+ m := config.Metric{
+ Parts: map[string]*config.MetricPart{"int_part": nil},
+ }
+
+ if err := validateMetric(m); err == nil {
+ t.Error("Metric with nil metric part was accepted.")
+ }
+}
