18487: Fixes error reporting to include both JSON & vocabulary errors at once.

Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <lucas.dipentima@curii.com>

diff --git a/lib/config/cmd.go b/lib/config/cmd.go
index 2f90bc80dd850a6fca9db5d3b07a3ed387eb2cd3..56a74a21c2a9f41dc1710c468e162a6f60f10231 100644 (file)
--- 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 6b5ca4e577143849fcca13deb36bccdb213db268..2bb596a05adc1d936597fa2ad3568be7f77f5150 100644 (file)
--- 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 4e155f34aaee7453069ad09f4614600b2c8e4cd9..bb1bec789f7f459a3cd49657c4df5337711cce19 100644 (file)
--- 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 da62d5c25164c1042394432b6fd1172706aae590..84b9bf2295e62e6025e0c6f03847c4d3e666a9eb 100644 (file)
--- 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")))
+                       }
                }
        }
 }