[ARVADOS] updated: 2.1.0-1588-gf827088cc
Git user
git at public.arvados.org
Tue Nov 9 21:46:24 UTC 2021
Summary of changes:
lib/controller/handler_test.go | 2 +-
lib/controller/localdb/conn.go | 4 -
sdk/go/arvados/vocabulary.go | 60 ++++++-----
sdk/go/arvados/vocabulary_test.go | 202 ++++++++++++++++++++++++++++++++++----
4 files changed, 220 insertions(+), 48 deletions(-)
via f827088cc812a217bfb46aca66be62b79b7ed973 (commit)
via d1af1ede3314ec5ae9b7dbbe51f8a8a7314ba651 (commit)
via d75bcd1e8bb8c5b312ccd6c86136d0c1e1d7b904 (commit)
via 79870ba994f0606c8ed13806f00cb8b23d9b2c83 (commit)
via 1f7f3f7f49d3e2d44b77472bfc1f204ae0496a70 (commit)
from 7b7de0ba345c02103bbaa9fb981424c59d440d55 (commit)
Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.
commit f827088cc812a217bfb46aca66be62b79b7ed973
Author: Lucas Di Pentima <lucas.dipentima at curii.com>
Date: Tue Nov 9 18:45:31 2021 -0300
17944: Adds a case-insensitive check for key/value against labels.
Also, improves tests.
Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <lucas.dipentima at curii.com>
diff --git a/sdk/go/arvados/vocabulary.go b/sdk/go/arvados/vocabulary.go
index 4edc9e83d..dc31b998f 100644
--- a/sdk/go/arvados/vocabulary.go
+++ b/sdk/go/arvados/vocabulary.go
@@ -146,6 +146,7 @@ func (v *Vocabulary) getLabelsToValues(key string) (labels map[string]string) {
labels = make(map[string]string)
if _, ok := v.Tags[key]; ok {
for val := range v.Tags[key].Values {
+ labels[strings.ToLower(val)] = val
for _, tagLbl := range v.Tags[key].Values[val].Labels {
label := strings.ToLower(tagLbl.Label)
labels[label] = val
diff --git a/sdk/go/arvados/vocabulary_test.go b/sdk/go/arvados/vocabulary_test.go
index 4756e720e..050d5ea76 100644
--- a/sdk/go/arvados/vocabulary_test.go
+++ b/sdk/go/arvados/vocabulary_test.go
@@ -66,22 +66,108 @@ func (s *VocabularySuite) TestCheck(c *check.C) {
strictVoc bool
props string
expectSuccess bool
+ errMatches string
}{
// Check succeeds
- {"Known key, known value", false, `{"IDTAGANIMALS":"IDVALANIMAL1"}`, true},
- {"Unknown non-alias key on non-strict vocabulary", false, `{"foo":"bar"}`, true},
- {"Known non-strict key, unknown non-alias value", false, `{"IDTAGANIMALS":"IDVALANIMAL3"}`, true},
- {"Undefined but reserved key on strict vocabulary", true, `{"reservedKey":"bar"}`, true},
- {"Known key, list of known values", false, `{"IDTAGANIMALS":["IDVALANIMAL1","IDVALANIMAL2"]}`, true},
- {"Known non-strict key, list of unknown non-alias values", false, `{"IDTAGCOMMENT":["hello world","lorem ipsum"]}`, true},
+ {
+ "Known key, known value",
+ false,
+ `{"IDTAGANIMALS":"IDVALANIMAL1"}`,
+ true,
+ "",
+ },
+ {
+ "Unknown non-alias key on non-strict vocabulary",
+ false,
+ `{"foo":"bar"}`,
+ true,
+ "",
+ },
+ {
+ "Known non-strict key, unknown non-alias value",
+ false,
+ `{"IDTAGANIMALS":"IDVALANIMAL3"}`,
+ true,
+ "",
+ },
+ {
+ "Undefined but reserved key on strict vocabulary",
+ true,
+ `{"reservedKey":"bar"}`,
+ true,
+ "",
+ },
+ {
+ "Known key, list of known values",
+ false,
+ `{"IDTAGANIMALS":["IDVALANIMAL1","IDVALANIMAL2"]}`,
+ true,
+ "",
+ },
+ {
+ "Known non-strict key, list of unknown non-alias values",
+ false,
+ `{"IDTAGCOMMENT":["hello world","lorem ipsum"]}`,
+ true,
+ "",
+ },
// Check fails
- {"Known first key & value; known 2nd key, unknown 2nd value", false, `{"IDTAGANIMALS":"IDVALANIMAL1", "IDTAGIMPORTANCE": "blah blah"}`, false},
- {"Unknown non-alias key on strict vocabulary", true, `{"foo":"bar"}`, false},
- {"Known non-strict key, known value alias", false, `{"IDTAGANIMALS":"Loxodonta"}`, false},
- {"Known strict key, unknown non-alias value", false, `{"IDTAGIMPORTANCE":"Unimportant"}`, false},
- {"Known strict key, known value alias", false, `{"IDTAGIMPORTANCE":"High"}`, false},
- {"Known strict key, list of known alias values", false, `{"IDTAGIMPORTANCE":["Unimportant","High"]}`, false},
- {"Known strict key, list of unknown non-alias values", false, `{"IDTAGIMPORTANCE":["foo","bar"]}`, false},
+ {
+ "Known first key & value; known 2nd key, unknown 2nd value",
+ false,
+ `{"IDTAGANIMALS":"IDVALANIMAL1", "IDTAGIMPORTANCE": "blah blah"}`,
+ false,
+ "tag value.*is not valid for key.*",
+ },
+ {
+ "Unknown non-alias key on strict vocabulary",
+ true,
+ `{"foo":"bar"}`,
+ false,
+ "tag key.*is not defined in the vocabulary",
+ },
+ {
+ "Known non-strict key, known value alias",
+ false,
+ `{"IDTAGANIMALS":"Loxodonta"}`,
+ false,
+ "tag value.*for key.* is an alias, must be provided as.*",
+ },
+ {
+ "Known strict key, unknown non-alias value",
+ false,
+ `{"IDTAGIMPORTANCE":"Unimportant"}`,
+ false,
+ "tag value.*is not valid for key.*",
+ },
+ {
+ "Known strict key, lowercase value regarded as alias",
+ false,
+ `{"IDTAGIMPORTANCE":"idval1"}`,
+ false,
+ "tag value.*for key.* is an alias, must be provided as.*",
+ },
+ {
+ "Known strict key, known value alias",
+ false,
+ `{"IDTAGIMPORTANCE":"High"}`,
+ false,
+ "tag value.* for key.*is an alias, must be provided as.*",
+ },
+ {
+ "Known strict key, list of known alias values",
+ false,
+ `{"IDTAGIMPORTANCE":["High", "Low"]}`,
+ false,
+ "tag value.*for key.*is an alias, must be provided as.*",
+ },
+ {
+ "Known strict key, list of unknown non-alias values",
+ false,
+ `{"IDTAGIMPORTANCE":["foo","bar"]}`,
+ false,
+ "tag value.*is not valid for key.*",
+ },
}
for _, tt := range tests {
c.Log(c.TestName()+" ", tt.name)
@@ -95,6 +181,7 @@ func (s *VocabularySuite) TestCheck(c *check.C) {
c.Assert(err, check.IsNil)
} else {
c.Assert(err, check.NotNil)
+ c.Assert(err.Error(), check.Matches, tt.errMatches)
}
}
}
commit d1af1ede3314ec5ae9b7dbbe51f8a8a7314ba651
Author: Lucas Di Pentima <lucas.dipentima at curii.com>
Date: Tue Nov 9 18:01:12 2021 -0300
17944: Improves keys & value collision validation against aliases. Adds tests.
Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <lucas.dipentima at curii.com>
diff --git a/sdk/go/arvados/vocabulary.go b/sdk/go/arvados/vocabulary.go
index 17b9441a6..4edc9e83d 100644
--- 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 7986a8252..4756e720e 100644
--- 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)
+ }
}
}
commit d75bcd1e8bb8c5b312ccd6c86136d0c1e1d7b904
Author: Lucas Di Pentima <lucas.dipentima at curii.com>
Date: Tue Nov 9 16:36:51 2021 -0300
17944: Fixes test. Avoids unnecessary Validate() call.
Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <lucas.dipentima at curii.com>
diff --git a/lib/controller/handler_test.go b/lib/controller/handler_test.go
index ac27b8ea5..f854079f9 100644
--- a/lib/controller/handler_test.go
+++ b/lib/controller/handler_test.go
@@ -183,7 +183,7 @@ func (s *HandlerSuite) TestVocabularyFailedCheckStatus(c *check.C) {
err = json.Unmarshal(resp.Body.Bytes(), &jresp)
c.Check(err, check.IsNil)
c.Assert(len(jresp.Errors), check.Equals, 1)
- c.Check(jresp.Errors[0], check.Matches, `.*tag value.*for key.*is not listed as valid.*`)
+ c.Check(jresp.Errors[0], check.Matches, `.*tag value.*is not valid for key.*`)
}
func (s *HandlerSuite) TestProxyDiscoveryDoc(c *check.C) {
diff --git a/lib/controller/localdb/conn.go b/lib/controller/localdb/conn.go
index f51567315..86e3f3714 100644
--- a/lib/controller/localdb/conn.go
+++ b/lib/controller/localdb/conn.go
@@ -107,10 +107,6 @@ func (conn *Conn) loadVocabularyFile() error {
if err != nil {
return fmt.Errorf("while loading vocabulary file %q: %s", conn.cluster.API.VocabularyPath, err)
}
- err = voc.Validate()
- if err != nil {
- return fmt.Errorf("while validating vocabulary file %q: %s", conn.cluster.API.VocabularyPath, err)
- }
conn.vocabularyCache = voc
return nil
}
commit 79870ba994f0606c8ed13806f00cb8b23d9b2c83
Author: Lucas Di Pentima <lucas.dipentima at curii.com>
Date: Tue Nov 9 16:10:01 2021 -0300
17944: Improves error messages.
Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <lucas.dipentima at curii.com>
diff --git a/sdk/go/arvados/vocabulary.go b/sdk/go/arvados/vocabulary.go
index 5804d4c40..17b9441a6 100644
--- a/sdk/go/arvados/vocabulary.go
+++ b/sdk/go/arvados/vocabulary.go
@@ -152,11 +152,11 @@ func (v *Vocabulary) getLabelsToValues(key string) (labels map[string]string) {
func (v *Vocabulary) checkValue(key, val string) error {
if _, ok := v.Tags[key].Values[val]; !ok {
lcVal := strings.ToLower(val)
- alias, ok := v.getLabelsToValues(key)[lcVal]
+ correctValue, ok := v.getLabelsToValues(key)[lcVal]
if ok {
- return fmt.Errorf("tag value %q for key %q is not defined but is an alias for %q", val, key, alias)
+ return fmt.Errorf("tag value %q for key %q is an alias, must be provided as %q", val, key, correctValue)
} else if v.Tags[key].Strict {
- return fmt.Errorf("tag value %q for key %q is not listed as valid", val, key)
+ return fmt.Errorf("tag value %q is not valid for key %q", val, key)
}
}
return nil
@@ -176,11 +176,11 @@ func (v *Vocabulary) Check(data map[string]interface{}) error {
}
if _, ok := v.Tags[key]; !ok {
lcKey := strings.ToLower(key)
- alias, ok := v.getLabelsToKeys()[lcKey]
+ correctKey, ok := v.getLabelsToKeys()[lcKey]
if ok {
- return fmt.Errorf("tag key %q is not defined but is an alias for %q", key, alias)
+ return fmt.Errorf("tag key %q is an alias, must be provided as %q", key, correctKey)
} else if v.StrictTags {
- return fmt.Errorf("tag key %q is not defined", key)
+ return fmt.Errorf("tag key %q is not defined in the vocabulary", key)
}
// If the key is not defined, we don't need to check the value
continue
@@ -201,11 +201,11 @@ func (v *Vocabulary) Check(data map[string]interface{}) error {
return err
}
default:
- return fmt.Errorf("tag value %q for key %q is not a valid type (%T)", singleVal, key, singleVal)
+ return fmt.Errorf("tag value of type %T for key %q is not a valid", singleVal, key)
}
}
default:
- return fmt.Errorf("tag value %q for key %q is not a valid type (%T)", val, key, val)
+ return fmt.Errorf("tag value of type %T for key %q is not a valid", val, key)
}
}
return nil
commit 1f7f3f7f49d3e2d44b77472bfc1f204ae0496a70
Author: Lucas Di Pentima <lucas.dipentima at curii.com>
Date: Tue Nov 9 15:24:47 2021 -0300
17944: Fixes premature vocabulary check success.
Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <lucas.dipentima at curii.com>
diff --git a/sdk/go/arvados/vocabulary.go b/sdk/go/arvados/vocabulary.go
index cb1106e9b..5804d4c40 100644
--- a/sdk/go/arvados/vocabulary.go
+++ b/sdk/go/arvados/vocabulary.go
@@ -188,7 +188,10 @@ func (v *Vocabulary) Check(data map[string]interface{}) error {
// Checks for value validity -- key is defined
switch val := val.(type) {
case string:
- return v.checkValue(key, val)
+ err := v.checkValue(key, val)
+ if err != nil {
+ return err
+ }
case []interface{}:
for _, singleVal := range val {
switch singleVal := singleVal.(type) {
diff --git a/sdk/go/arvados/vocabulary_test.go b/sdk/go/arvados/vocabulary_test.go
index b2748c7be..7986a8252 100644
--- a/sdk/go/arvados/vocabulary_test.go
+++ b/sdk/go/arvados/vocabulary_test.go
@@ -75,6 +75,7 @@ func (s *VocabularySuite) TestCheck(c *check.C) {
{"Known key, list of known values", false, `{"IDTAGANIMALS":["IDVALANIMAL1","IDVALANIMAL2"]}`, true},
{"Known non-strict key, list of unknown non-alias values", false, `{"IDTAGCOMMENT":["hello world","lorem ipsum"]}`, true},
// Check fails
+ {"Known first key & value; known 2nd key, unknown 2nd value", false, `{"IDTAGANIMALS":"IDVALANIMAL1", "IDTAGIMPORTANCE": "blah blah"}`, false},
{"Unknown non-alias key on strict vocabulary", true, `{"foo":"bar"}`, false},
{"Known non-strict key, known value alias", false, `{"IDTAGANIMALS":"Loxodonta"}`, false},
{"Known strict key, unknown non-alias value", false, `{"IDTAGIMPORTANCE":"Unimportant"}`, false},
-----------------------------------------------------------------------
hooks/post-receive
--
More information about the arvados-commits
mailing list