From: Brett Smith <brett.smith@curii.com>
Date: Tue, 21 Feb 2023 21:24:17 +0000 (-0500)
Subject: 19980: System properties only require arv: prefix
X-Git-Tag: 2.6.0~37^2
X-Git-Url: https://git.arvados.org/arvados.git/commitdiff_plain/3429718113f11e999dd745e94ff981a4f3b01c57?hp=f41bb69fdd60627f6e4880f51ed48f69e79af760

19980: System properties only require arv: prefix

The decision to require a following alphabetic character was
well-intentioned but misplaced: it doesn't stop these properties from
existing, just changes the layer they would be defined at.

It might make sense to add some name rules to properties, but that would
be better done across the board as a separate story, not as a specific
corner case of system properties.

Arvados-DCO-1.1-Signed-off-by: Brett Smith <brett.smith@curii.com>
---

diff --git a/sdk/go/arvados/vocabulary.go b/sdk/go/arvados/vocabulary.go
index caab6e8fbe..1df43b5fb8 100644
--- a/sdk/go/arvados/vocabulary.go
+++ b/sdk/go/arvados/vocabulary.go
@@ -10,13 +10,10 @@ import (
 	"errors"
 	"fmt"
 	"reflect"
-	"regexp"
 	"strconv"
 	"strings"
 )
 
-const systemKeyPattern = `^arv:[a-zA-Z]`
-
 type Vocabulary struct {
 	reservedTagKeys map[string]bool          `json:"-"`
 	StrictTags      bool                     `json:"strict_tags"`
@@ -30,8 +27,8 @@ type VocabularyTag struct {
 }
 
 // Cannot have a constant map in Go, so we have to use a function.
-// If you are adding a new system property, it SHOULD match `systemKeyPattern`
-// above, and Check will allow it. This map is for historical exceptions that
+// If you are adding a new system property, it SHOULD start with `arv:`,
+// and Check will allow it. This map is for historical exceptions that
 // predate standardizing on this prefix.
 func (v *Vocabulary) systemTagKeys() map[string]bool {
 	return map[string]bool{
@@ -271,13 +268,9 @@ func (v *Vocabulary) Check(data map[string]interface{}) error {
 	if v == nil {
 		return nil
 	}
-	systemKeyRegexp, err := regexp.Compile(systemKeyPattern)
-	if err != nil {
-		return err
-	}
 	for key, val := range data {
 		// Checks for key validity
-		if systemKeyRegexp.MatchString(key) || v.reservedTagKeys[key] {
+		if strings.HasPrefix(key, "arv:") || v.reservedTagKeys[key] {
 			// Allow reserved keys to be used even if they are not defined in
 			// the vocabulary no matter its strictness.
 			continue
diff --git a/sdk/go/arvados/vocabulary_test.go b/sdk/go/arvados/vocabulary_test.go
index c06efda3e0..af62833a31 100644
--- a/sdk/go/arvados/vocabulary_test.go
+++ b/sdk/go/arvados/vocabulary_test.go
@@ -6,7 +6,6 @@ package arvados
 
 import (
 	"encoding/json"
-	"fmt"
 	"regexp"
 	"strings"
 
@@ -312,28 +311,21 @@ func (s *VocabularySuite) TestValidSystemProperties(c *check.C) {
 	c.Check(s.testVoc.Check(properties), check.IsNil)
 }
 
-func (s *VocabularySuite) TestSystemPropertiesFirstCharacterAlphabetic(c *check.C) {
-	s.testVoc.StrictTags = true
-	properties := map[string]interface{}{"arv:": "value"}
-	c.Check(s.testVoc.Check(properties), check.NotNil)
-	// If we expand the list of allowed characters in the future, these lists
-	// may need adjustment to match.
-	for _, prefix := range []string{" ", ".", "_", "-", "1"} {
-		for _, suffix := range []string{"", "invalid"} {
-			key := fmt.Sprintf("arv:%s%s", prefix, suffix)
-			properties := map[string]interface{}{key: "value"}
-			c.Check(s.testVoc.Check(properties), check.NotNil)
-		}
-	}
-}
-
 func (s *VocabularySuite) TestSystemPropertiesPrefixTypo(c *check.C) {
 	s.testVoc.StrictTags = true
 	for _, key := range []string{
+		// Extra characters in prefix
 		"arv :foo",
+		" arv:foo",
+		// Wrong punctuation
+		"arv.foo",
+		"arv-foo",
+		"arv_foo",
+		// Wrong case
+		"Arv:foo",
+		// Wrong word
 		"arvados",
 		"arvados:foo",
-		"Arv:foo",
 	} {
 		properties := map[string]interface{}{key: "value"}
 		c.Check(s.testVoc.Check(properties), check.NotNil)