[ARVADOS] created: 2.1.0-1836-g435363035

Git user git at public.arvados.org
Tue Jan 18 20:24:04 UTC 2022


        at  43536303547784d11d190e2cfdadda954005ae5d (commit)


commit 43536303547784d11d190e2cfdadda954005ae5d
Author: Lucas Di Pentima <lucas.dipentima at curii.com>
Date:   Tue Jan 18 12:54:59 2022 -0300

    18487: Fixes error checking on nonexistant vocabulary file. Adds tests.
    
    Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <lucas.dipentima at curii.com>

diff --git a/lib/config/cmd.go b/lib/config/cmd.go
index e532a7e32..2f90bc80d 100644
--- a/lib/config/cmd.go
+++ b/lib/config/cmd.go
@@ -148,7 +148,7 @@ func (checkCommand) RunCommand(prog string, args []string, stdin io.Reader, stdo
 		if cc.API.VocabularyPath == "" {
 			continue
 		}
-		_, err = os.Stat(cc.API.VocabularyPath)
+		vd, err := os.ReadFile(cc.API.VocabularyPath)
 		if err != nil {
 			if errors.Is(err, os.ErrNotExist) {
 				// If the vocabulary path doesn't exist, it might mean that
@@ -156,11 +156,6 @@ func (checkCommand) RunCommand(prog string, args []string, stdin io.Reader, stdo
 				// error.
 				continue
 			}
-			fmt.Fprintf(stderr, "Error checking vocabulary path %q for cluster %s: %s\n", cc.API.VocabularyPath, id, err)
-			return 1
-		}
-		vd, err := os.ReadFile(cc.API.VocabularyPath)
-		if err != nil {
 			fmt.Fprintf(stderr, "Error reading vocabulary file %q for cluster %s: %s\n", cc.API.VocabularyPath, id, err)
 			return 1
 		}
diff --git a/lib/config/cmd_test.go b/lib/config/cmd_test.go
index a5cc28b80..6b5ca4e57 100644
--- a/lib/config/cmd_test.go
+++ b/lib/config/cmd_test.go
@@ -53,6 +53,7 @@ Clusters:
   SystemRootToken: aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
   API:
     MaxItemsPerResponse: 1234
+    VocabularyPath: /this/path/does/not/exist
   Collections:
     BlobSigningKey: aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
   PostgreSQL:
@@ -77,6 +78,60 @@ Clusters:
 	c.Check(stderr.String(), check.Equals, "")
 }
 
+func (s *CommandSuite) TestCheck_VocabularyErrors(c *check.C) {
+	tmpFile, err := ioutil.TempFile("", "")
+	c.Assert(err, check.IsNil)
+	defer os.Remove(tmpFile.Name())
+	_, err = tmpFile.WriteString(`
+{
+ "tags": {
+  "IDfoo": {
+   "labels": [
+    {"label": "foo"}
+   ]
+  },
+  "IDfoo": {
+   "labels": [
+    {"label": "baz"}
+   ]
+  }
+ }
+}`)
+	c.Assert(err, check.IsNil)
+	tmpFile.Close()
+	vocPath := tmpFile.Name()
+	var stdout, stderr bytes.Buffer
+	in := `
+Clusters:
+ z1234:
+  ManagementToken: aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
+  SystemRootToken: aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
+  API:
+    MaxItemsPerResponse: 1234
+    VocabularyPath: ` + vocPath + `
+  Collections:
+    BlobSigningKey: aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
+  PostgreSQL:
+    Connection:
+      sslmode: require
+  Services:
+    RailsAPI:
+      InternalURLs:
+        "http://0.0.0.0:8000": {}
+  Workbench:
+    UserProfileFormFields:
+      color:
+        Type: select
+        Options:
+          fuchsia: {}
+    ApplicationMimetypesWithViewIcon:
+      whitespace: {}
+`
+	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\).*`)
+}
+
 func (s *CommandSuite) TestCheck_DeprecatedKeys(c *check.C) {
 	var stdout, stderr bytes.Buffer
 	in := `

commit af44b96bc1f843c1b7878049e161602fef839d1d
Author: Lucas Di Pentima <lucas.dipentima at curii.com>
Date:   Tue Jan 18 11:29:49 2022 -0300

    18487: Improves vocabulary error checking, with tests.
    
    * Adds JSON duplicated keys checking (json.Unmarshall silently accepts them).
    * Adds line & column numbers to JSON syntax errors.
    * Reports all detected non-syntax errors at once instead of returning on the
    first one. (so that the site admin doesn't have to do multiple edit+check
    cycles)
    
    Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <lucas.dipentima at curii.com>

diff --git a/lib/controller/localdb/conn.go b/lib/controller/localdb/conn.go
index 323e660c6..104cfe28f 100644
--- a/lib/controller/localdb/conn.go
+++ b/lib/controller/localdb/conn.go
@@ -99,7 +99,7 @@ func (conn *Conn) maybeRefreshVocabularyCache(logger logrus.FieldLogger) error {
 func (conn *Conn) loadVocabularyFile() error {
 	vf, err := os.ReadFile(conn.cluster.API.VocabularyPath)
 	if err != nil {
-		return fmt.Errorf("couldn't reading the vocabulary file: %v", err)
+		return fmt.Errorf("while reading the vocabulary file: %v", err)
 	}
 	mk := make([]string, 0, len(conn.cluster.Collections.ManagedProperties))
 	for k := range conn.cluster.Collections.ManagedProperties {
diff --git a/sdk/go/arvados/vocabulary.go b/sdk/go/arvados/vocabulary.go
index 150091b30..4e155f34a 100644
--- a/sdk/go/arvados/vocabulary.go
+++ b/sdk/go/arvados/vocabulary.go
@@ -7,8 +7,10 @@ package arvados
 import (
 	"bytes"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"reflect"
+	"strconv"
 	"strings"
 )
 
@@ -55,7 +57,20 @@ func NewVocabulary(data []byte, managedTagKeys []string) (voc *Vocabulary, err e
 	}
 	err = json.Unmarshal(data, &voc)
 	if err != nil {
-		return nil, fmt.Errorf("invalid JSON format error: %q", err)
+		var serr *json.SyntaxError
+		if errors.As(err, &serr) {
+			offset := serr.Offset
+			errorMsg := string(data[:offset])
+			line := 1 + strings.Count(errorMsg, "\n")
+			column := offset - int64(strings.LastIndex(errorMsg, "\n")+len("\n"))
+			return nil, fmt.Errorf("invalid JSON format: %q (line %d, column %d)", err, line, column)
+		}
+		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)
@@ -74,6 +89,57 @@ func NewVocabulary(data []byte, managedTagKeys []string) (voc *Vocabulary, err e
 	return voc, nil
 }
 
+func checkJSONDupedKeys(d *json.Decoder, path []string, errors *[]string) error {
+	t, err := d.Token()
+	if err != nil {
+		return err
+	}
+	delim, ok := t.(json.Delim)
+	if !ok {
+		return nil
+	}
+	switch delim {
+	case '{':
+		keys := make(map[string]bool)
+		for d.More() {
+			t, err := d.Token()
+			if err != nil {
+				return err
+			}
+			key := t.(string)
+
+			if keys[key] {
+				*errors = append(*errors, strings.Join(append(path, key), "."))
+			}
+			keys[key] = true
+
+			if err := checkJSONDupedKeys(d, append(path, key), errors); err != nil {
+				return err
+			}
+		}
+		// consume closing '}'
+		if _, err := d.Token(); err != nil {
+			return err
+		}
+	case '[':
+		i := 0
+		for d.More() {
+			if err := checkJSONDupedKeys(d, append(path, strconv.Itoa(i)), errors); err != nil {
+				return err
+			}
+			i++
+		}
+		// consume closing ']'
+		if _, err := d.Token(); err != nil {
+			return err
+		}
+	}
+	if len(path) == 0 && len(*errors) > 0 {
+		return fmt.Errorf("duplicate JSON key(s):\n%s", strings.Join(*errors, "\n"))
+	}
+	return nil
+}
+
 func (v *Vocabulary) validate() error {
 	if v == nil {
 		return nil
@@ -85,44 +151,48 @@ func (v *Vocabulary) validate() error {
 	}
 	// Checks for collisions between tag keys, reserved tag keys
 	// and tag key labels.
+	errors := []string{}
 	for key := range v.Tags {
 		if v.reservedTagKeys[key] {
-			return fmt.Errorf("tag key %q is reserved", key)
+			errors = append(errors, fmt.Sprintf("tag key %q is reserved", key))
 		}
 		lcKey := strings.ToLower(key)
 		if tagKeys[lcKey] != "" {
-			return fmt.Errorf("duplicate tag key %q", key)
+			errors = append(errors, fmt.Sprintf("duplicate tag key %q", key))
 		}
 		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", lbl.Label, key)
+				errors = append(errors, fmt.Sprintf("tag label %q for key %q already seen as a tag key or label", lbl.Label, key))
 			}
 			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)
+			errors = append(errors, fmt.Sprintf("tag key %q is configured as strict but doesn't provide values", key))
 		}
 		// Checks for collisions between tag values and tag value labels.
 		tagValues := map[string]string{}
 		for val := range v.Tags[key].Values {
 			lcVal := strings.ToLower(val)
 			if tagValues[lcVal] != "" {
-				return fmt.Errorf("duplicate tag value %q for tag %q", val, key)
+				errors = append(errors, fmt.Sprintf("duplicate tag value %q for tag %q", val, key))
 			}
 			// Checks for collisions between labels from different values.
 			tagValues[lcVal] = val
 			for _, tagLbl := range v.Tags[key].Values[val].Labels {
 				label := strings.ToLower(tagLbl.Label)
 				if tagValues[label] != "" && tagValues[label] != val {
-					return fmt.Errorf("tag value label %q for pair (%q:%q) already seen on value %q", tagLbl.Label, key, val, tagValues[label])
+					errors = append(errors, fmt.Sprintf("tag value label %q for pair (%q:%q) already seen on value %q", tagLbl.Label, key, val, tagValues[label]))
 				}
 				tagValues[label] = val
 			}
 		}
 	}
+	if len(errors) > 0 {
+		return fmt.Errorf("invalid vocabulary:\n%s", strings.Join(errors, "\n"))
+	}
 	return nil
 }
 
diff --git a/sdk/go/arvados/vocabulary_test.go b/sdk/go/arvados/vocabulary_test.go
index 5a5189de2..da62d5c25 100644
--- a/sdk/go/arvados/vocabulary_test.go
+++ b/sdk/go/arvados/vocabulary_test.go
@@ -257,15 +257,37 @@ func (s *VocabularySuite) TestNewVocabulary(c *check.C) {
 				},
 			},
 		},
+		{
+			"Invalid JSON error with line & column numbers",
+			`{"tags":{
+				"aKey":{
+					"labels": [,{"label": "A label"}]
+				}
+			}}`,
+			false, `invalid JSON format:.*\(line \d+, column \d+\)`, nil,
+		},
+		{
+			"Invalid JSON with duplicate keys",
+			`{"tags":{
+				"type":{
+					"strict": false,
+					"labels": [{"label": "Class", "label": "Type"}]
+				},
+				"type":{
+					"labels": []
+				}
+			}}`,
+			false, "(?s).*tags.type.labels.0.label\ntags.type", nil,
+		},
 		{
 			"Valid data, but uses reserved key",
 			`{"tags":{
 				"type":{
 					"strict": false,
-					"labels": [{"label": "Type"}]
+					"labels": [{"label": "Class"}]
 				}
 			}}`,
-			false, "tag key.*is reserved", nil,
+			false, "(?s).*tag key.*is reserved", nil,
 		},
 	}
 
@@ -288,14 +310,14 @@ func (s *VocabularySuite) TestValidationErrors(c *check.C) {
 	tests := []struct {
 		name       string
 		voc        *Vocabulary
-		errMatches string
+		errMatches []string
 	}{
 		{
 			"Strict vocabulary, no keys",
 			&Vocabulary{
 				StrictTags: true,
 			},
-			"vocabulary is strict but no tags are defined",
+			[]string{"vocabulary is strict but no tags are defined"},
 		},
 		{
 			"Collision between tag key and tag key label",
@@ -312,7 +334,7 @@ func (s *VocabularySuite) TestValidationErrors(c *check.C) {
 					},
 				},
 			},
-			"", // Depending on how the map is sorted, this could be one of two errors
+			nil, // Depending on how the map is sorted, this could be one of two errors
 		},
 		{
 			"Collision between tag key and tag key label (case-insensitive)",
@@ -329,7 +351,7 @@ func (s *VocabularySuite) TestValidationErrors(c *check.C) {
 					},
 				},
 			},
-			"", // Depending on how the map is sorted, this could be one of two errors
+			nil, // Depending on how the map is sorted, this could be one of two errors
 		},
 		{
 			"Collision between tag key labels",
@@ -346,7 +368,7 @@ func (s *VocabularySuite) TestValidationErrors(c *check.C) {
 					},
 				},
 			},
-			"tag label.*for key.*already seen.*",
+			[]string{"(?s).*tag label.*for key.*already seen.*"},
 		},
 		{
 			"Collision between tag value and tag value label",
@@ -367,7 +389,7 @@ func (s *VocabularySuite) TestValidationErrors(c *check.C) {
 					},
 				},
 			},
-			"", // Depending on how the map is sorted, this could be one of two errors
+			nil, // Depending on how the map is sorted, this could be one of two errors
 		},
 		{
 			"Collision between tag value and tag value label (case-insensitive)",
@@ -388,7 +410,7 @@ func (s *VocabularySuite) TestValidationErrors(c *check.C) {
 					},
 				},
 			},
-			"", // Depending on how the map is sorted, this could be one of two errors
+			nil, // Depending on how the map is sorted, this could be one of two errors
 		},
 		{
 			"Collision between tag value labels",
@@ -409,7 +431,7 @@ func (s *VocabularySuite) TestValidationErrors(c *check.C) {
 					},
 				},
 			},
-			"tag value label.*for pair.*already seen.*on value.*",
+			[]string{"(?s).*tag value label.*for pair.*already seen.*on value.*"},
 		},
 		{
 			"Collision between tag value labels (case-insensitive)",
@@ -430,7 +452,7 @@ func (s *VocabularySuite) TestValidationErrors(c *check.C) {
 					},
 				},
 			},
-			"tag value label.*for pair.*already seen.*on value.*",
+			[]string{"(?s).*tag value label.*for pair.*already seen.*on value.*"},
 		},
 		{
 			"Strict tag key, with no values",
@@ -443,15 +465,34 @@ func (s *VocabularySuite) TestValidationErrors(c *check.C) {
 					},
 				},
 			},
-			"tag key.*is configured as strict but doesn't provide values",
+			[]string{"(?s).*tag key.*is configured as strict but doesn't provide values"},
+		},
+		{
+			"Multiple errors reported",
+			&Vocabulary{
+				StrictTags: false,
+				Tags: map[string]VocabularyTag{
+					"IDTAGANIMALS": {
+						Strict: true,
+						Labels: []VocabularyLabel{{Label: "Animal"}, {Label: "Creature"}},
+					},
+					"IDTAGSIZES": {
+						Labels: []VocabularyLabel{{Label: "Animal"}, {Label: "Size"}},
+					},
+				},
+			},
+			[]string{
+				"(?s).*tag key.*is configured as strict but doesn't provide values.*",
+				"(?s).*tag label.*for key.*already seen.*",
+			},
 		},
 	}
 	for _, tt := range tests {
 		c.Log(c.TestName()+" ", tt.name)
 		err := tt.voc.validate()
 		c.Assert(err, check.NotNil)
-		if tt.errMatches != "" {
-			c.Assert(err, check.ErrorMatches, tt.errMatches)
+		for _, errMatch := range tt.errMatches {
+			c.Assert(err, check.ErrorMatches, errMatch)
 		}
 	}
 }

commit adee1c1a7902de81df8cfd5064c3fa9f377faa47
Author: Lucas Di Pentima <lucas.dipentima at curii.com>
Date:   Mon Jan 17 16:20:01 2022 -0300

    18487: Adds vocabulary check to 'arvados-server config-check'
    
    Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <lucas.dipentima at curii.com>

diff --git a/lib/config/cmd.go b/lib/config/cmd.go
index eeab6ac8c..e532a7e32 100644
--- a/lib/config/cmd.go
+++ b/lib/config/cmd.go
@@ -6,6 +6,7 @@ package config
 
 import (
 	"bytes"
+	"errors"
 	"flag"
 	"fmt"
 	"io"
@@ -13,6 +14,7 @@ import (
 	"os/exec"
 
 	"git.arvados.org/arvados.git/lib/cmd"
+	"git.arvados.org/arvados.git/sdk/go/arvados"
 	"git.arvados.org/arvados.git/sdk/go/ctxlog"
 	"github.com/ghodss/yaml"
 	"github.com/sirupsen/logrus"
@@ -142,6 +144,36 @@ func (checkCommand) RunCommand(prog string, args []string, stdin io.Reader, stdo
 			return 1
 		}
 	}
+	for id, cc := range withDepr.Clusters {
+		if cc.API.VocabularyPath == "" {
+			continue
+		}
+		_, err = os.Stat(cc.API.VocabularyPath)
+		if err != nil {
+			if errors.Is(err, os.ErrNotExist) {
+				// If the vocabulary path doesn't exist, it might mean that
+				// the current node isn't the controller; so it's not an
+				// error.
+				continue
+			}
+			fmt.Fprintf(stderr, "Error checking vocabulary path %q for cluster %s: %s\n", cc.API.VocabularyPath, id, err)
+			return 1
+		}
+		vd, err := os.ReadFile(cc.API.VocabularyPath)
+		if err != nil {
+			fmt.Fprintf(stderr, "Error reading vocabulary file %q for cluster %s: %s\n", cc.API.VocabularyPath, id, err)
+			return 1
+		}
+		mk := make([]string, 0, len(cc.Collections.ManagedProperties))
+		for k := range cc.Collections.ManagedProperties {
+			mk = append(mk, k)
+		}
+		_, 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)
+			return 1
+		}
+	}
 	return 0
 }
 

-----------------------------------------------------------------------


hooks/post-receive
-- 




More information about the arvados-commits mailing list