17944: Improves keys & value collision validation against aliases. Adds tests.

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

diff --git a/sdk/go/arvados/vocabulary.go b/sdk/go/arvados/vocabulary.go
index 17b9441a6bdd0898e2495e26ca85a73fce942f64..4edc9e83d5a5fcd31adab69b1b59bf450946ab34 100644 (file)
--- a/sdk/go/arvados/vocabulary.go
+++ b/sdk/go/arvados/vocabulary.go
@@ -46,6 +46,9 @@ type VocabularyTagValue struct {
        Labels []VocabularyLabel `json:"labels"`
 }
 
+// NewVocabulary creates a new Vocabulary from a JSON definition and a list
+// of reserved tag keys that will get special treatment when strict mode is
+// enabled.
 func NewVocabulary(data []byte, managedTagKeys []string) (voc *Vocabulary, err error) {
        if r := bytes.Compare(data, []byte("")); r == 0 {
                return &Vocabulary{}, nil
@@ -64,55 +67,58 @@ func NewVocabulary(data []byte, managedTagKeys []string) (voc *Vocabulary, err e
        for systemKey := range voc.systemTagKeys() {
                voc.reservedTagKeys[systemKey] = true
        }
-       err = voc.Validate()
+       err = voc.validate()
        if err != nil {
                return nil, err
        }
        return voc, nil
 }
 
-func (v *Vocabulary) Validate() error {
+func (v *Vocabulary) validate() error {
        if v == nil {
                return nil
        }
-       tagKeys := map[string]bool{}
+       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")
        }
-       // Checks for duplicate tag keys
+       // Checks for collisions between tag keys, reserved tag keys
+       // and tag key labels.
        for key := range v.Tags {
                if v.reservedTagKeys[key] {
                        return fmt.Errorf("tag key %q is reserved", key)
                }
-               if tagKeys[key] {
+               lcKey := strings.ToLower(key)
+               if tagKeys[lcKey] != "" {
                        return fmt.Errorf("duplicate tag key %q", key)
                }
-               tagKeys[key] = true
+               tagKeys[lcKey] = key
                for _, lbl := range v.Tags[key].Labels {
                        label := strings.ToLower(lbl.Label)
-                       if tagKeys[label] {
-                               return fmt.Errorf("tag label %q for key %q already seen as a tag key or label", label, key)
+                       if tagKeys[label] != "" {
+                               return fmt.Errorf("tag label %q for key %q already seen as a tag key or label", lbl.Label, key)
                        }
-                       tagKeys[label] = true
+                       tagKeys[label] = lbl.Label
                }
                // Checks for value strictness
                if v.Tags[key].Strict && len(v.Tags[key].Values) == 0 {
                        return fmt.Errorf("tag key %q is configured as strict but doesn't provide values", key)
                }
-               // Checks for value duplication within a key
-               tagValues := map[string]bool{}
+               // Checks for collisions between tag values and tag value labels.
+               tagValues := map[string]string{}
                for val := range v.Tags[key].Values {
-                       if tagValues[val] {
+                       lcVal := strings.ToLower(val)
+                       if tagValues[lcVal] != "" {
                                return fmt.Errorf("duplicate tag value %q for tag %q", val, key)
                        }
-                       tagValues[val] = true
+                       tagValues[lcVal] = val
                        for _, tagLbl := range v.Tags[key].Values[val].Labels {
                                label := strings.ToLower(tagLbl.Label)
-                               if tagValues[label] {
-                                       return fmt.Errorf("tag value label %q for pair (%q:%q) already seen as a value key or label", label, key, val)
+                               if tagValues[label] != "" {
+                                       return fmt.Errorf("tag value label %q for pair (%q:%q) already seen as a value key or label", tagLbl.Label, key, val)
                                }
-                               tagValues[label] = true
+                               tagValues[label] = tagLbl.Label
                        }
                }
        }

diff --git a/sdk/go/arvados/vocabulary_test.go b/sdk/go/arvados/vocabulary_test.go
index 7986a8252268ac7a20ea7b35d3fd0a8387565e0b..4756e720e3e56e956e5b4d57452577266835b6e1 100644 (file)
--- a/sdk/go/arvados/vocabulary_test.go
+++ b/sdk/go/arvados/vocabulary_test.go
@@ -56,7 +56,7 @@ func (s *VocabularySuite) SetUpTest(c *check.C) {
                        },
                },
        }
-       err := s.testVoc.Validate()
+       err := s.testVoc.validate()
        c.Assert(err, check.IsNil)
 }
 
@@ -193,7 +193,41 @@ func (s *VocabularySuite) TestValidationErrors(c *check.C) {
                        "vocabulary is strict but no tags are defined",
                },
                {
-                       "Duplicated tag keys",
+                       "Collision between tag key and tag key label",
+                       &Vocabulary{
+                               StrictTags: false,
+                               Tags: map[string]VocabularyTag{
+                                       "IDTAGANIMALS": {
+                                               Strict: false,
+                                               Labels: []VocabularyLabel{{Label: "Animal"}, {Label: "Creature"}},
+                                       },
+                                       "IDTAGCOMMENT": {
+                                               Strict: false,
+                                               Labels: []VocabularyLabel{{Label: "Comment"}, {Label: "IDTAGANIMALS"}},
+                                       },
+                               },
+                       },
+                       "", // Depending on how the map is sorted, this could be one of two errors
+               },
+               {
+                       "Collision between tag key and tag key label (case-insensitive)",
+                       &Vocabulary{
+                               StrictTags: false,
+                               Tags: map[string]VocabularyTag{
+                                       "IDTAGANIMALS": {
+                                               Strict: false,
+                                               Labels: []VocabularyLabel{{Label: "Animal"}, {Label: "Creature"}},
+                                       },
+                                       "IDTAGCOMMENT": {
+                                               Strict: false,
+                                               Labels: []VocabularyLabel{{Label: "Comment"}, {Label: "IdTagAnimals"}},
+                                       },
+                               },
+                       },
+                       "", // Depending on how the map is sorted, this could be one of two errors
+               },
+               {
+                       "Collision between tag key labels",
                        &Vocabulary{
                                StrictTags: false,
                                Tags: map[string]VocabularyTag{
@@ -210,7 +244,49 @@ func (s *VocabularySuite) TestValidationErrors(c *check.C) {
                        "tag label.*for key.*already seen.*",
                },
                {
-                       "Duplicated tag values",
+                       "Collision between tag value and tag value label",
+                       &Vocabulary{
+                               StrictTags: false,
+                               Tags: map[string]VocabularyTag{
+                                       "IDTAGANIMALS": {
+                                               Strict: false,
+                                               Labels: []VocabularyLabel{{Label: "Animal"}, {Label: "Creature"}},
+                                               Values: map[string]VocabularyTagValue{
+                                                       "IDVALANIMAL1": {
+                                                               Labels: []VocabularyLabel{{Label: "Human"}, {Label: "Mammal"}},
+                                                       },
+                                                       "IDVALANIMAL2": {
+                                                               Labels: []VocabularyLabel{{Label: "Elephant"}, {Label: "IDVALANIMAL1"}},
+                                                       },
+                                               },
+                                       },
+                               },
+                       },
+                       "", // Depending on how the map is sorted, this could be one of two errors
+               },
+               {
+                       "Collision between tag value and tag value label (case-insensitive)",
+                       &Vocabulary{
+                               StrictTags: false,
+                               Tags: map[string]VocabularyTag{
+                                       "IDTAGANIMALS": {
+                                               Strict: false,
+                                               Labels: []VocabularyLabel{{Label: "Animal"}, {Label: "Creature"}},
+                                               Values: map[string]VocabularyTagValue{
+                                                       "IDVALANIMAL1": {
+                                                               Labels: []VocabularyLabel{{Label: "Human"}, {Label: "Mammal"}},
+                                                       },
+                                                       "IDVALANIMAL2": {
+                                                               Labels: []VocabularyLabel{{Label: "Elephant"}, {Label: "IDValAnimal1"}},
+                                                       },
+                                               },
+                                       },
+                               },
+                       },
+                       "", // Depending on how the map is sorted, this could be one of two errors
+               },
+               {
+                       "Collision between tag value labels",
                        &Vocabulary{
                                StrictTags: false,
                                Tags: map[string]VocabularyTag{
@@ -231,7 +307,7 @@ func (s *VocabularySuite) TestValidationErrors(c *check.C) {
                        "tag value label.*for pair.*already seen.*",
                },
                {
-                       "Strict key, no values",
+                       "Strict tag key, with no values",
                        &Vocabulary{
                                StrictTags: false,
                                Tags: map[string]VocabularyTag{
@@ -246,8 +322,10 @@ func (s *VocabularySuite) TestValidationErrors(c *check.C) {
        }
        for _, tt := range tests {
                c.Log(c.TestName()+" ", tt.name)
-               err := tt.voc.Validate()
+               err := tt.voc.validate()
                c.Assert(err, check.NotNil)
-               c.Assert(err, check.ErrorMatches, tt.errMatches)
+               if tt.errMatches != "" {
+                       c.Assert(err, check.ErrorMatches, tt.errMatches)
+               }
        }
 }