[config_parser] Improve config_validator errors
Make them more structured and easier to parse.
Also, support more than one error from a particular step
Change-Id: Idafc4984a3055e6f107dea7fa5890123adb8bfa2
Reviewed-on: https://fuchsia-review.googlesource.com/c/cobalt/+/483577
Reviewed-by: Alexandre Zani <azani@google.com>
Commit-Queue: Zach Bush <zmbush@google.com>
Fuchsia-Auto-Submit: Zach Bush <zmbush@google.com>
diff --git a/src/bin/config_parser/src/BUILD.gn b/src/bin/config_parser/src/BUILD.gn
index 93a38f7..2c38db6 100644
--- a/src/bin/config_parser/src/BUILD.gn
+++ b/src/bin/config_parser/src/BUILD.gn
@@ -79,6 +79,7 @@
"int_buckets_test.go",
"metric_definitions_test.go",
"report_definitions_test.go",
+ "validator_test.go",
]
deps = [ ":config_parser" ]
@@ -89,6 +90,7 @@
deps = [
":config_validator",
"//third_party/golibs:github.com/golang/glog",
+ "//third_party/golibs:github.com/google/go-cmp",
]
}
diff --git a/src/bin/config_parser/src/config_parser_main.go b/src/bin/config_parser/src/config_parser_main.go
index f85edcc..8811641 100644
--- a/src/bin/config_parser/src/config_parser_main.go
+++ b/src/bin/config_parser/src/config_parser_main.go
@@ -25,6 +25,11 @@
forClient = flag.Bool("for_client", false, "Filters out the hide_on_client tagged fields")
)
+func fail(step string, err error) {
+ glog.Errorf("\n\nFailed to %s:\n%v\n\n", step, err)
+ os.Exit(1)
+}
+
func main() {
flag.Parse()
@@ -32,30 +37,30 @@
if *depFile != "" {
configFiles, err := config_parser.GetConfigFilesListFromFlags()
if err != nil {
- glog.Exit(err)
+ fail("get config files", err)
}
if err := source_generator.WriteDepFileFromFlags(configFiles, *depFile); err != nil {
- glog.Exit(err)
+ fail("write dep file", err)
}
}
// First, we parse the configuration from the specified location.
configs, err := config_parser.ParseConfigFromFlags()
if err != nil {
- glog.Exit(err)
+ fail("parse registry", err)
}
// Unless otherwise specified, validate the registry.
if !*skipValidation {
if err = config_validator.ValidateProjectConfigs(configs); err != nil {
- glog.Exit(err)
+ fail("validate registry", err)
}
}
// Compute and write the |prob_bit_flip| and |num_index_points| fields for each Cobalt 1.1 ReportDefinition
// in |configs|.
if err := config_parser.PopulatePrivacyParams(configs); err != nil {
- glog.Exit(err)
+ fail("populate privacy parameters", err)
}
// Merge the list of project configs in a single config.CobaltRegistry.
@@ -69,6 +74,7 @@
// Write the registry depending upon the specified flags.
if err := source_generator.WriteConfigFromFlags(&c, filtered); err != nil {
+ fail("write generated registry files", err)
glog.Exit(err)
}
diff --git a/src/bin/config_parser/src/config_validator/metric_definitions.go b/src/bin/config_parser/src/config_validator/metric_definitions.go
index 5a03107..a34bba6 100644
--- a/src/bin/config_parser/src/config_validator/metric_definitions.go
+++ b/src/bin/config_parser/src/config_validator/metric_definitions.go
@@ -33,95 +33,110 @@
// Validate a list of MetricDefinitions.
func validateConfiguredMetricDefinitions(metrics []*config.MetricDefinition) (err error) {
+ metricErrors := newValidationErrors("metric")
+
metricIds := map[uint32]int{}
metricNames := map[string]bool{}
metricLookup := map[uint32]*config.MetricDefinition{}
for i, metric := range metrics {
if _, ok := metricIds[metric.Id]; ok {
- return fmt.Errorf("There are two metrics with id=%v.", metric.Id)
+ metricErrors.addError(fmt.Sprintf("Metric %d", metric.Id), fmt.Errorf("there are two metrics with id=%v", metric.Id))
+ continue
}
metricIds[metric.Id] = i
if _, ok := metricNames[metric.MetricName]; ok {
- return fmt.Errorf("There are two metrics with name=%v.", metric.MetricName)
+ metricErrors.addError(metric.MetricName, fmt.Errorf("there are two metrics with name=%v", metric.MetricName))
+ continue
}
metricNames[metric.MetricName] = true
metricLookup[metric.Id] = metric
if err = validateMetricDefinition(*metric); err != nil {
- return fmt.Errorf("Error validating metric '%s': %v", metric.MetricName, err)
+ metricErrors.addError(metric.MetricName, err)
}
}
+ replacementIds := map[uint32]string{}
for _, metric := range metrics {
- if err = validateReplacementMetricId(*metric, metricLookup); err != nil {
- return fmt.Errorf("Error validating metric '%s': %v", metric.MetricName, err)
+ if metric.ReplacementMetricId != 0 {
+ if s, ok := replacementIds[metric.ReplacementMetricId]; ok {
+ metricErrors.addError(metric.MetricName, fmt.Errorf("found metric with duplicate replacement_metric_id %s", s))
+ }
+ replacementIds[metric.ReplacementMetricId] = metric.MetricName
+ }
+ if err = validateReplacementMetricID(*metric, metricLookup); err != nil {
+ metricErrors.addError(metric.MetricName, err)
}
}
- return nil
+ return metricErrors.err()
}
// Validate a single MetricDefinition.
func validateMetricDefinition(m config.MetricDefinition) (err error) {
+ validationErrors := newValidationErrors("field")
if !validNameRegexp.MatchString(m.MetricName) {
- return fmt.Errorf("Invalid metric name. Metric names must match the regular expression '%v'.", validNameRegexp)
+ validationErrors.addError("metric_name", fmt.Errorf("metric names must match the regular expression '%v'", validNameRegexp))
}
if m.Id == 0 {
- return fmt.Errorf("Metric ID of zero is not allowed. Please specify a positive metric ID.")
+ validationErrors.addError("id", fmt.Errorf("metric ID of zero is not allowed. Please specify a positive metric ID"))
}
if m.MetricType == config.MetricDefinition_UNSET {
- return fmt.Errorf("metric_type is not set.")
+ validationErrors.addError("metric_type", fmt.Errorf("is not set"))
}
if m.MetaData == nil {
- return fmt.Errorf("meta_data is not set, additionally, in meta_data: %v", validateMetadata(config.MetricDefinition_Metadata{}))
- }
-
- if err := validateMetadata(*m.MetaData); err != nil {
- return fmt.Errorf("in meta_data: %v", err)
+ validationErrors.addError("meta_data", fmt.Errorf("meta_data is not set, additionally, in meta_data: %v", validateMetadata(config.MetricDefinition_Metadata{})))
+ } else if err := validateMetadata(*m.MetaData); err != nil {
+ validationErrors.addError("meta_data", err)
}
if err := validateMetricUnits(m); err != nil {
- return err
+ validationErrors.addError("metric_units", err)
}
if m.IntBuckets != nil && m.MetricType != config.MetricDefinition_INT_HISTOGRAM && m.MetricType != config.MetricDefinition_INTEGER_HISTOGRAM {
- return fmt.Errorf("Metric %s has int_buckets set. int_buckets can only be set for metrics for metric type INT_HISTOGRAM and INTEGER_HISTOGRAM.", m.MetricName)
+ validationErrors.addError("int_buckets", fmt.Errorf("int_buckets can only be set for metrics for metric type INT_HISTOGRAM and INTEGER_HISTOGRAM"))
}
if m.ProtoName != "" && m.MetricType != config.MetricDefinition_CUSTOM {
- return fmt.Errorf("Metric %s has proto_name set. proto_name can only be set for metrics for metric type CUSTOM.", m.MetricName)
+ validationErrors.addError("proto_name", fmt.Errorf("proto_name can only be set for metrics for metric type CUSTOM"))
}
if config_parser.Cobalt11MetricTypesSet[m.MetricType] {
if m.MetricSemantics == nil || len(m.MetricSemantics) == 0 {
- return fmt.Errorf("Metric %s does not have metric_semantics. metric_semantics is required for this metric.", m.MetricName)
+ validationErrors.addError("metric_semantics", fmt.Errorf("metric_semantics is required for this metric"))
}
} else {
if len(m.MetricSemantics) > 0 {
- return fmt.Errorf("Metric %s has metric_semantics set. metric_semantics can only be set for Cobalt 1.1 metrics.", m.MetricName)
+ validationErrors.addError("metric_semantics", fmt.Errorf("metric_semantics can only be set for Cobalt 1.1 metrics"))
}
}
if err := validateMetricDefinitionForType(m); err != nil {
- return fmt.Errorf("Metric %s: %v", m.MetricName, err)
+ validationErrors.addError("other", err)
}
if m.MetricType != config.MetricDefinition_CUSTOM {
if err := validateMetricDimensions(m); err != nil {
- return fmt.Errorf("Metric %s: %v", m.MetricName, err)
+ validationErrors.addError("metric_dimensions", err)
}
}
+ err = validationErrors.err()
+ if err != nil {
+ return err
+ }
+
return validateReportDefinitions(m)
}
-func validateReplacementMetricId(m config.MetricDefinition, mapping map[uint32]*config.MetricDefinition) (err error) {
+func validateReplacementMetricID(m config.MetricDefinition, mapping map[uint32]*config.MetricDefinition) (err error) {
if m.ReplacementMetricId == 0 {
return nil
}
@@ -159,7 +174,7 @@
if replacement.MetricType == config.MetricDefinition_STRING {
for _, report := range m.Reports {
if len(report.CandidateList) == 0 && len(report.CandidateFile) == 0 {
- return fmt.Errorf("Error in report %v: metrics mapped to STRING must contain either candidate_list or candidate_file in all reports.", report.Id)
+ return fmt.Errorf("report `%v`: metrics mapped to STRING must contain either candidate_list or candidate_file in all reports", report.Id)
}
}
}
@@ -281,17 +296,17 @@
}
if len(md.EventCodes) == 0 && md.MaxEventCode == 0 {
- return fmt.Errorf("For metric dimension %v, you must either define max_event_code or explicitly define at least one event code.", name)
+ return fmt.Errorf("for metric dimension %v, you must either define max_event_code or explicitly define at least one event code", name)
}
if md.MaxEventCode != 0 {
- for j, _ := range md.EventCodes {
+ for j := range md.EventCodes {
if j > md.MaxEventCode {
return fmt.Errorf("event index %v in metric_dimension %v is greater than max_event_code %v", j, name, md.MaxEventCode)
}
}
numOptions *= int(md.MaxEventCode + 1)
} else {
- for j, _ := range md.EventCodes {
+ for j := range md.EventCodes {
if cobalt10MetricType && j >= 1024 && len(m.MetricDimensions) > 4 {
return fmt.Errorf("event index %v in metric_dimension %v is greater than 1023, which means that this metric cannot support more than 4 metric_dimensions", j, name)
}
@@ -360,16 +375,16 @@
func validateEventOccurred(m config.MetricDefinition) error {
if len(m.MetricDimensions) == 0 || m.MetricDimensions[0].MaxEventCode == 0 {
- return fmt.Errorf("No max_event_code specified for metric of type EVENT_OCCURRED.")
+ return fmt.Errorf("no max_event_code specified for metric of type EVENT_OCCURRED")
}
if len(m.MetricDimensions) > 1 {
- return fmt.Errorf("Too many metric dimensions specified for metric of type EVENT_OCCURRED.")
+ return fmt.Errorf("too many metric dimensions specified for metric of type EVENT_OCCURRED")
}
for i, md := range m.MetricDimensions {
if md.MaxEventCode == 0 {
- return fmt.Errorf("No max_event_code specified for metric_dimension %v.", i)
+ return fmt.Errorf("no max_event_code specified for metric_dimension %v", i)
}
}
@@ -378,7 +393,7 @@
func validateIntHistogram(m config.MetricDefinition) error {
if m.IntBuckets == nil {
- return fmt.Errorf("No int_buckets specified for metric of type INT_HISTOGRAM.")
+ return fmt.Errorf("no int_buckets specified for metric of type INT_HISTOGRAM")
}
// TODO(azani): Validate bucket definition.
@@ -394,10 +409,10 @@
func validateString(m config.MetricDefinition) error {
if m.StringCandidateFile == "" {
- return fmt.Errorf("No string_candidate_file specified for metric of type STRING.")
+ return fmt.Errorf("no string_candidate_file specified for metric of type STRING")
}
if m.StringBufferMax == 0 {
- return fmt.Errorf("No string_buffer_max specified for metric of type STRING.")
+ return fmt.Errorf("no string_buffer_max specified for metric of type STRING")
}
return nil
@@ -409,11 +424,11 @@
}
if m.ProtoName == "" {
- return fmt.Errorf("Missing proto_name. Proto names are required for metrics of type CUSTOM.")
+ return fmt.Errorf("missing proto_name. Proto names are required for metrics of type CUSTOM")
}
if !validProtoNameRegexp.MatchString(m.ProtoName) {
- return fmt.Errorf("Invalid proto_name. Proto names must match the regular expression '%v'.", validProtoNameRegexp)
+ return fmt.Errorf("Invalid proto_name. Proto names must match the regular expression '%v'", validProtoNameRegexp)
}
return nil
diff --git a/src/bin/config_parser/src/config_validator/report_definitions.go b/src/bin/config_parser/src/config_validator/report_definitions.go
index b525992..af2d5d7 100644
--- a/src/bin/config_parser/src/config_validator/report_definitions.go
+++ b/src/bin/config_parser/src/config_validator/report_definitions.go
@@ -88,25 +88,30 @@
}
func validateReportDefinitions(m config.MetricDefinition) error {
+ reportErrors := newValidationErrors("report")
+
reportIds := map[uint32]int{}
reportNames := map[string]bool{}
for i, r := range m.Reports {
if _, ok := reportIds[r.Id]; ok {
- return fmt.Errorf("There are two reports with id=%v.", r.Id)
+ reportErrors.addError(fmt.Sprintf("Report %d", r.Id), fmt.Errorf("there are two reports with id=%v", r.Id))
+ continue
}
reportIds[r.Id] = i
if _, ok := reportNames[r.ReportName]; ok {
- return fmt.Errorf("There are two reports with name=%v.", r.ReportName)
+ reportErrors.addError(r.ReportName, fmt.Errorf("there are two reports with name=%v", r.ReportName))
+ continue
}
reportNames[r.ReportName] = true
if err := validateReportDefinitionForMetric(m, *r); err != nil {
- return fmt.Errorf("Error validating report '%s': %v", r.ReportName, err)
+ reportErrors.addError(r.ReportName, err)
+ continue
}
}
- return nil
+ return reportErrors.err()
}
// Validate a single instance of a ReportDefinition with its associated metric.
@@ -129,7 +134,7 @@
// Validate a single instance of a ReportDefinition.
func validateReportDefinition(m config.MetricDefinition, r config.ReportDefinition) error {
if !validNameRegexp.MatchString(r.ReportName) {
- return fmt.Errorf("Invalid report name. Report names must match the regular expression '%v'.", validNameRegexp)
+ return fmt.Errorf("Invalid report name. Report names must match the regular expression '%v'", validNameRegexp)
}
if r.Id == 0 {
@@ -137,7 +142,7 @@
}
if err := validateReportDefinitionForType(m, r); err != nil {
- return fmt.Errorf("Report %s: %v", r.ReportName, err)
+ return err
}
return nil
@@ -146,16 +151,16 @@
// Validates that the MetricType and ReportType provided are compatible.
func validateReportType(mt config.MetricDefinition_MetricType, rt config.ReportDefinition_ReportType) error {
if rt == config.ReportDefinition_REPORT_TYPE_UNSET {
- return fmt.Errorf("report_type is not set.")
+ return fmt.Errorf("report_type is not set")
}
rts, ok := allowedReportTypes[mt]
if !ok {
- return fmt.Errorf("Unknown metric type %s.", mt)
+ return fmt.Errorf("unknown metric type %s", mt)
}
if _, ok = rts[rt]; !ok {
- return fmt.Errorf("Reports of type %s cannot be used with metrics of type %s", rt, mt)
+ return fmt.Errorf("reports of type %s cannot be used with metrics of type %s", rt, mt)
}
return nil
diff --git a/src/bin/config_parser/src/config_validator/validator.go b/src/bin/config_parser/src/config_validator/validator.go
index 5d8bb0c..7e76732 100644
--- a/src/bin/config_parser/src/config_validator/validator.go
+++ b/src/bin/config_parser/src/config_validator/validator.go
@@ -7,17 +7,84 @@
import (
"config"
"config_parser"
+ "errors"
"fmt"
+ "sort"
+ "strings"
)
+// validationErrors is used to package up nested validation errors in a structured way
+//
+// The method validationErrors::err() converts this into an error type (or nil if no entries have been added to errors)
+type validationErrors struct {
+ ty string
+ errors map[string][]error
+}
+
+func (ve validationErrors) addError(key string, value error) {
+ ve.errors[key] = append(ve.errors[key], value)
+}
+
+func (ve *validationErrors) Error() string {
+ if ve == nil {
+ return ""
+ }
+
+ projects := []string{}
+ for project := range ve.errors {
+ projects = append(projects, project)
+ }
+ sort.Strings(projects)
+ result := []string{}
+ for _, project := range projects {
+ errs := ve.errors[project]
+ if len(errs) == 1 {
+ var innerErrs *validationErrors
+ err := errs[0]
+ if errors.As(err, &innerErrs) {
+ // Indent the nested validationErrors
+ errStr := strings.Join(strings.Split(err.Error(), "\n"), "\n ")
+ result = append(result, fmt.Sprintf("%s `%s`:\n %v", ve.ty, project, errStr))
+ } else {
+ result = append(result, fmt.Sprintf("%s `%s`: %v", ve.ty, project, err.Error()))
+ }
+ } else {
+ errStrs := []string{}
+ for _, err := range errs {
+ errStrs = append(errStrs, err.Error())
+ }
+ errStr := strings.Join(errStrs, "\n")
+ errStr = strings.Join(strings.Split(errStr, "\n"), "\n ")
+ result = append(result, fmt.Sprintf("%s `%s`:\n %v", ve.ty, project, errStr))
+ }
+ }
+ return strings.Join(result, "\n")
+}
+
+func newValidationErrors(ty string) validationErrors {
+ return validationErrors{
+ ty: ty,
+ errors: map[string][]error{},
+ }
+}
+
+func (ve validationErrors) err() error {
+ if len(ve.errors) > 0 {
+ return &ve
+ }
+ return nil
+}
+
func ValidateProjectConfigs(configs []config_parser.ProjectConfig) error {
+ projectErrors := newValidationErrors("customer/project")
+
for _, c := range configs {
if err := validateProjectConfig(&c); err != nil {
- return err
+ projectErrors.addError(fmt.Sprintf("%s/%s", c.CustomerName, c.ProjectName), err)
}
}
- return nil
+ return projectErrors.err()
}
func validateProjectConfig(c *config_parser.ProjectConfig) (err error) {
@@ -26,11 +93,11 @@
}
if err = validateConfiguredMetricDefinitions(c.ProjectConfig.MetricDefinitions); err != nil {
- return fmt.Errorf("Error in configuration for project %s: %v", c.ProjectName, err)
+ return err
}
if err = validateReplacementMetricSaturation(c.ProjectConfig.MetricDefinitions); err != nil {
- return fmt.Errorf("Error in configuration for project %s: %v", c.ProjectName, err)
+ return err
}
return nil
}
diff --git a/src/bin/config_parser/src/config_validator/validator_test.go b/src/bin/config_parser/src/config_validator/validator_test.go
new file mode 100644
index 0000000..eb3fe74
--- /dev/null
+++ b/src/bin/config_parser/src/config_validator/validator_test.go
@@ -0,0 +1,66 @@
+package config_validator
+
+import (
+ "fmt"
+ "testing"
+
+ "github.com/google/go-cmp/cmp"
+)
+
+func TestEmptyValidationErrors(t *testing.T) {
+ if newValidationErrors("err").err() != nil {
+ t.Errorf("Empty validation errors .err() should return nil")
+ }
+}
+
+func TestSingleValidationErrors(t *testing.T) {
+ errs := newValidationErrors("err")
+ errs.addError("an", fmt.Errorf("error"))
+ expectedError := "err `an`: error"
+ if diff := cmp.Diff(errs.err().Error(), expectedError); diff != "" {
+ t.Errorf("Output error does not match expected error: %s", diff)
+ }
+}
+
+func TestMultipleValidationErrors(t *testing.T) {
+ errs := newValidationErrors("err")
+ errs.addError("an", fmt.Errorf("error"))
+ errs.addError("another", fmt.Errorf("error"))
+ expectedError := "err `an`: error\nerr `another`: error"
+ if diff := cmp.Diff(errs.err().Error(), expectedError); diff != "" {
+ t.Errorf("Output error does not match expected error: %s", diff)
+ }
+}
+
+func TestNestedValidationErrors(t *testing.T) {
+ innerErr := newValidationErrors("inner")
+ innerErr.addError("an", fmt.Errorf("error"))
+ errs := newValidationErrors("err")
+ errs.addError("nested", innerErr.err())
+ expectedError := "err `nested`:\n inner `an`: error"
+ if diff := cmp.Diff(errs.err().Error(), expectedError); diff != "" {
+ t.Errorf("Output error does not match expected error: %s", diff)
+ }
+}
+
+func TestMultipleNestedValidationErrors(t *testing.T) {
+ innerErr := newValidationErrors("inner")
+ innerErr.addError("an", fmt.Errorf("error"))
+ errs := newValidationErrors("err")
+ errs.addError("nested2", innerErr.err())
+ errs.addError("nested1", innerErr.err())
+ expectedError := "err `nested1`:\n inner `an`: error\nerr `nested2`:\n inner `an`: error"
+ if diff := cmp.Diff(errs.err().Error(), expectedError); diff != "" {
+ t.Errorf("Output error does not match expected error: %s", diff)
+ }
+}
+
+func TestMultipleErrorsForLevel(t *testing.T) {
+ errs := newValidationErrors("err")
+ errs.addError("an", fmt.Errorf("error"))
+ errs.addError("an", fmt.Errorf("error"))
+ expectedError := "err `an`:\n error\n error"
+ if diff := cmp.Diff(errs.err().Error(), expectedError); diff != "" {
+ t.Errorf("Output error does not match expected error: %s", diff)
+ }
+}
