From 4ffe3382ff35cebce873668dfdfad2eef2def3d3 Mon Sep 17 00:00:00 2001 From: Lucas Di Pentima Date: Thu, 20 Jan 2022 17:00:17 -0300 Subject: [PATCH] 18487: Fixes error reporting to include both JSON & vocabulary errors at once. Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima --- lib/config/cmd.go | 2 +- lib/config/cmd_test.go | 2 +- sdk/go/arvados/vocabulary.go | 38 ++++++++++++++++++++----------- sdk/go/arvados/vocabulary_test.go | 35 ++++++++++++++++------------ 4 files changed, 47 insertions(+), 30 deletions(-) diff --git a/lib/config/cmd.go b/lib/config/cmd.go index 2f90bc80dd..56a74a21c2 100644 --- a/lib/config/cmd.go +++ b/lib/config/cmd.go @@ -165,7 +165,7 @@ func (checkCommand) RunCommand(prog string, args []string, stdin io.Reader, stdo } _, err = arvados.NewVocabulary(vd, mk) if err != nil { - fmt.Fprintf(stderr, "Error loading vocabulary file %q for cluster %s: %s\n", cc.API.VocabularyPath, id, err) + fmt.Fprintf(stderr, "Error loading vocabulary file %q for cluster %s:\n%s\n", cc.API.VocabularyPath, id, err) return 1 } } diff --git a/lib/config/cmd_test.go b/lib/config/cmd_test.go index 6b5ca4e577..2bb596a05a 100644 --- a/lib/config/cmd_test.go +++ b/lib/config/cmd_test.go @@ -129,7 +129,7 @@ Clusters: ` code := CheckCommand.RunCommand("arvados config-check", []string{"-config", "-"}, bytes.NewBufferString(in), &stdout, &stderr) c.Check(code, check.Equals, 1) - c.Check(stderr.String(), check.Matches, `(?s).*Error loading vocabulary file.*duplicate JSON key\(s\).*`) + c.Check(stderr.String(), check.Matches, `(?s).*Error loading vocabulary file.*for cluster.*\nduplicate JSON key "tags.IDfoo".*`) } func (s *CommandSuite) TestCheck_DeprecatedKeys(c *check.C) { diff --git a/sdk/go/arvados/vocabulary.go b/sdk/go/arvados/vocabulary.go index 4e155f34aa..bb1bec789f 100644 --- a/sdk/go/arvados/vocabulary.go +++ b/sdk/go/arvados/vocabulary.go @@ -67,14 +67,22 @@ func NewVocabulary(data []byte, managedTagKeys []string) (voc *Vocabulary, err e } return nil, fmt.Errorf("invalid JSON format: %q", err) } - // json.Unmarshal() doesn't error out on duplicate keys. - err = checkJSONDupedKeys(json.NewDecoder(bytes.NewReader(data)), nil, &[]string{}) - if err != nil { - return nil, err - } if reflect.DeepEqual(voc, &Vocabulary{}) { return nil, fmt.Errorf("JSON data provided doesn't match Vocabulary format: %q", data) } + + shouldReportErrors := false + errors := []string{} + + // json.Unmarshal() doesn't error out on duplicate keys. + dupedKeys := []string{} + err = checkJSONDupedKeys(json.NewDecoder(bytes.NewReader(data)), nil, &dupedKeys) + if err != nil { + shouldReportErrors = true + for _, dk := range dupedKeys { + errors = append(errors, fmt.Sprintf("duplicate JSON key %q", dk)) + } + } voc.reservedTagKeys = make(map[string]bool) for _, managedKey := range managedTagKeys { voc.reservedTagKeys[managedKey] = true @@ -82,9 +90,13 @@ func NewVocabulary(data []byte, managedTagKeys []string) (voc *Vocabulary, err e for systemKey := range voc.systemTagKeys() { voc.reservedTagKeys[systemKey] = true } - err = voc.validate() + validationErrs, err := voc.validate() if err != nil { - return nil, err + shouldReportErrors = true + errors = append(errors, validationErrs...) + } + if shouldReportErrors { + return nil, fmt.Errorf("%s", strings.Join(errors, "\n")) } return voc, nil } @@ -135,19 +147,19 @@ func checkJSONDupedKeys(d *json.Decoder, path []string, errors *[]string) error } } if len(path) == 0 && len(*errors) > 0 { - return fmt.Errorf("duplicate JSON key(s):\n%s", strings.Join(*errors, "\n")) + return fmt.Errorf("duplicate JSON key(s) found") } return nil } -func (v *Vocabulary) validate() error { +func (v *Vocabulary) validate() ([]string, error) { if v == nil { - return nil + return nil, nil } tagKeys := map[string]string{} // Checks for Vocabulary strictness if v.StrictTags && len(v.Tags) == 0 { - return fmt.Errorf("vocabulary is strict but no tags are defined") + return nil, fmt.Errorf("vocabulary is strict but no tags are defined") } // Checks for collisions between tag keys, reserved tag keys // and tag key labels. @@ -191,9 +203,9 @@ func (v *Vocabulary) validate() error { } } if len(errors) > 0 { - return fmt.Errorf("invalid vocabulary:\n%s", strings.Join(errors, "\n")) + return errors, fmt.Errorf("invalid vocabulary") } - return nil + return nil, nil } func (v *Vocabulary) getLabelsToKeys() (labels map[string]string) { diff --git a/sdk/go/arvados/vocabulary_test.go b/sdk/go/arvados/vocabulary_test.go index da62d5c251..84b9bf2295 100644 --- a/sdk/go/arvados/vocabulary_test.go +++ b/sdk/go/arvados/vocabulary_test.go @@ -6,6 +6,8 @@ package arvados import ( "encoding/json" + "regexp" + "strings" check "gopkg.in/check.v1" ) @@ -56,7 +58,7 @@ func (s *VocabularySuite) SetUpTest(c *check.C) { }, }, } - err := s.testVoc.validate() + _, err := s.testVoc.validate() c.Assert(err, check.IsNil) } @@ -267,7 +269,7 @@ func (s *VocabularySuite) TestNewVocabulary(c *check.C) { false, `invalid JSON format:.*\(line \d+, column \d+\)`, nil, }, { - "Invalid JSON with duplicate keys", + "Invalid JSON with duplicate & reserved keys", `{"tags":{ "type":{ "strict": false, @@ -277,17 +279,7 @@ func (s *VocabularySuite) TestNewVocabulary(c *check.C) { "labels": [] } }}`, - false, "(?s).*tags.type.labels.0.label\ntags.type", nil, - }, - { - "Valid data, but uses reserved key", - `{"tags":{ - "type":{ - "strict": false, - "labels": [{"label": "Class"}] - } - }}`, - false, "(?s).*tag key.*is reserved", nil, + false, "(?s).*duplicate JSON key \"tags.type.labels.0.label\"\nduplicate JSON key \"tags.type\"\ntag key \"type\" is reserved", nil, }, } @@ -489,10 +481,23 @@ func (s *VocabularySuite) TestValidationErrors(c *check.C) { } for _, tt := range tests { c.Log(c.TestName()+" ", tt.name) - err := tt.voc.validate() + validationErrs, err := tt.voc.validate() c.Assert(err, check.NotNil) for _, errMatch := range tt.errMatches { - c.Assert(err, check.ErrorMatches, errMatch) + seen := false + for _, validationErr := range validationErrs { + if regexp.MustCompile(errMatch).MatchString(validationErr) { + seen = true + break + } + } + if len(validationErrs) == 0 { + c.Assert(err, check.ErrorMatches, errMatch) + } else { + c.Assert(seen, check.Equals, true, + check.Commentf("Expected to see error matching %q:\n%s", + errMatch, strings.Join(validationErrs, "\n"))) + } } } } -- 2.30.2