syncbase: Relax restrictions on wire.Id Name.

Relaxed restrictions on wire.Id Name component to allow up to 1024 byte
(previously 64 byte), arbitrary Unicode (previously C identifier) names,
excluding only Unicode control characters (notably including \0 and \n).

Added restriction on blessings (in general, not just Id blessings) and
row keys to also exclude Unicode control characters.

Also cleaned up key splitting helper usage.

MultiPart: 2/3
Change-Id: If7074d1a8553566f557dcc15b20508fae3605709
diff --git a/services/syncbase/common/key_util.go b/services/syncbase/common/key_util.go
index 976052e..0bb1171 100644
--- a/services/syncbase/common/key_util.go
+++ b/services/syncbase/common/key_util.go
@@ -15,6 +15,9 @@
 	"v.io/x/lib/vlog"
 )
 
+// TODO(ivanpi): Switch helpers here and in v23 to verror and eliminate wrapping
+// into verror.ErrInternal at callsites.
+
 // JoinKeyParts builds keys for accessing data in the storage engine.
 func JoinKeyParts(parts ...string) string {
 	return strings.Join(parts, KeyPartSep)
@@ -69,16 +72,6 @@
 	return cxId, parts[2], nil
 }
 
-// ParseRowKeyOrDie calls ParseRowKey and panics on error.
-func ParseRowKeyOrDie(key string) (collection wire.Id, row string) {
-	collection, row, err := ParseRowKey(key)
-	if err != nil {
-		// TODO(ivanpi): Get rid of *OrDie() functions. Always log internal errors.
-		vlog.Fatal(err)
-	}
-	return collection, row
-}
-
 // ParseCollectionPermsKey extracts the collection id from the given storage
 // engine key for a collection perms entry. Returns an error if the given key
 // is not a storage engine key for a collection perms entry.
diff --git a/services/syncbase/server/collection.go b/services/syncbase/server/collection.go
index a4251a4..b4041ca 100644
--- a/services/syncbase/server/collection.go
+++ b/services/syncbase/server/collection.go
@@ -202,16 +202,18 @@
 		key, value := []byte{}, []byte{}
 		for it.Advance() {
 			key, value = it.Key(key), it.Value(value)
-			// See comment in util/constants.go for why we use SplitNKeyParts.
-			parts := common.SplitNKeyParts(string(key), 3)
-			externalKey := parts[2]
+			_, row, err := common.ParseRowKey(string(key))
+			if err != nil {
+				it.Cancel()
+				return verror.New(verror.ErrInternal, ctx, err)
+			}
 			var rawBytes *vom.RawBytes
 			if err := vom.Decode(value, &rawBytes); err != nil {
 				// TODO(m3b): Is this the correct behaviour on an encoding error here?
 				it.Cancel()
 				return err
 			}
-			if err := sender.Send(wire.KeyValue{Key: externalKey, Value: rawBytes}); err != nil {
+			if err := sender.Send(wire.KeyValue{Key: row, Value: rawBytes}); err != nil {
 				it.Cancel()
 				return err
 			}
diff --git a/services/syncbase/server/database.go b/services/syncbase/server/database.go
index 4333b76..9c74705 100644
--- a/services/syncbase/server/database.go
+++ b/services/syncbase/server/database.go
@@ -717,9 +717,15 @@
 		if s.it[s.curr].Advance() {
 			// key
 			keyBytes := s.it[s.curr].Key(nil)
-			parts := common.SplitNKeyParts(string(keyBytes), 3)
+			_, row, err := common.ParseRowKey(string(keyBytes))
+			if err != nil {
+				s.validRow = false
+				s.err = err
+				s.Cancel() // to cancel iterators after s.curr
+				return false
+			}
 			// TODO(rogulenko): Check access for the key.
-			s.currKey = parts[2]
+			s.currKey = row
 			// value
 			valueBytes := s.it[s.curr].Value(nil)
 			var currValue *vom.RawBytes
diff --git a/services/syncbase/server/database_watch.go b/services/syncbase/server/database_watch.go
index cadf468..82fec12 100644
--- a/services/syncbase/server/database_watch.go
+++ b/services/syncbase/server/database_watch.go
@@ -200,11 +200,12 @@
 	key, value := []byte{}, []byte{}
 	for rIt.Advance() {
 		key, value = rIt.Key(key), rIt.Value(value)
-		// See comment in util/constants.go for why we use SplitNKeyParts.
-		parts := common.SplitNKeyParts(string(key), 3)
-		externalKey := parts[2]
+		_, row, err := common.ParseRowKey(string(key))
+		if err != nil {
+			return verror.New(verror.ErrInternal, ctx, err)
+		}
 		// Filter out unnecessary rows.
-		if !watchFilter.RowMatches(c.id, externalKey) {
+		if !watchFilter.RowMatches(c.id, row) {
 			continue
 		}
 		// Send row.
@@ -213,7 +214,7 @@
 			return verror.New(verror.ErrInternal, ctx, err)
 		}
 		if err := sender.addChange(
-			naming.Join(pubutil.EncodeId(c.id), externalKey),
+			naming.Join(pubutil.EncodeId(c.id), row),
 			watch.Exists,
 			&wire.StoreChange{
 				Value: valueAsRawBytes,
diff --git a/services/syncbase/testutil/constants.go b/services/syncbase/testutil/constants.go
index 0ea5076..dc0579b 100644
--- a/services/syncbase/testutil/constants.go
+++ b/services/syncbase/testutil/constants.go
@@ -9,6 +9,8 @@
 )
 
 var invalidBlessingPatterns = []string{
+	",",
+	"a,b",
 	":",
 	"a:",
 	":b",
@@ -29,34 +31,12 @@
 	"v.io:foo:bar",
 	"v.io:a:admin@myapp.com",
 	"v.io:o:app:user",
-	"\x00",
-	"\xfb",
-	"a\x00",
+	"\xfa",
 	"a\xfb",
 	"안녕하세요",
-	// 16 4-byte runes => 64 bytes
-	strings.Repeat("𠜎", 16),
 }
 
-var invalidIdentifiers = []string{
-	"/",
-	"a/b",
-	":",
-	"a:b",
-	"*",
-	"\x00",
-	"\x01",
-	"\xfa",
-	"\xfb",
-	"@@",
-	"dev.v.io/a/admin@myapp.com",
-	"dev.v.io:a:admin@myapp.com",
-	"안녕하세요",
-	// 16 4-byte runes => 64 bytes
-	strings.Repeat("𠜎", 16),
-}
-
-var validIdentifiers = []string{
+var validNames = []string{
 	"a",
 	"B",
 	"a_",
@@ -68,23 +48,30 @@
 	"foobar",
 	"foo_bar",
 	"BARBAZ",
-	// 64 bytes
-	"abcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcd",
-}
-
-var universallyInvalidNames = []string{
-	"",
-	"\xfc",
-	"\xfd",
-	"\xfe",
-	"\xff",
-	"a\xfcb",
-	"a\xfdb",
-	"a\xfeb",
-	"a\xffb",
+	"/",
+	"a/b",
+	":",
+	"a:b",
+	"*",
+	"\xfa",
+	"a\xfb",
+	"@@",
+	"dev.v.io/a/admin@myapp.com",
+	"dev.v.io:a:admin@myapp.com",
+	",",
+	"a,b",
+	" ",
+	"foo bar",
+	"foo-bar/baz42",
+	"8T:j=re{*(TfF_.U9VG\"{2g:;Z-/~Ibm}&.p%7Zf:I{K~b8;Di\\!rA@k",
+	"안녕하세요",
 }
 
 var longNames = []string{
+	// 64 bytes
+	"abcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcd",
+	// 16 4-byte runes => 64 bytes
+	strings.Repeat("𠜎", 16),
 	// 65 bytes
 	"abcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcde",
 	// 16 4-byte runes + 1 more byte => 65 bytes
@@ -99,6 +86,33 @@
 	strings.Repeat("foobar", 1337),
 }
 
+var universallyInvalidIdParts = []string{
+	// disallow empty
+	"",
+	// disallow Unicode control characters (00-1F, 7F, 80-9F)
+	"\x00",
+	"a\x00b",
+	"\x01",
+	"\x07",
+	"\x1f",
+	"\x7f",
+	"\u0080",
+	"\u0091",
+	"\u009f",
+	"a\x01b",
+	"a\x7fb",
+	"a\u0091b",
+	// disallow bytes FC-FF (invalid Unicode)
+	"\xfc",
+	"\xfd",
+	"\xfe",
+	"\xff",
+	"a\xfcb",
+	"a\xfdb",
+	"a\xfeb",
+	"a\xffb",
+}
+
 func concat(slices ...[]string) []string {
 	var res []string
 	for _, slice := range slices {
@@ -109,13 +123,13 @@
 
 var (
 	OkAppUserBlessings    []string = concat(validBlessingPatterns, longNames)
-	NotOkAppUserBlessings []string = concat(universallyInvalidNames, invalidBlessingPatterns, veryLongNames)
+	NotOkAppUserBlessings []string = concat(universallyInvalidIdParts, invalidBlessingPatterns, veryLongNames)
 
-	OkDbCxNames    []string = validIdentifiers
-	NotOkDbCxNames []string = concat(universallyInvalidNames, invalidIdentifiers, longNames, veryLongNames)
+	OkDbCxNames    []string = concat(validNames, longNames)
+	NotOkDbCxNames []string = concat(universallyInvalidIdParts, veryLongNames)
 )
 
 var (
-	OkRowKeys    []string = concat(validIdentifiers, invalidIdentifiers, longNames, veryLongNames)
-	NotOkRowKeys []string = universallyInvalidNames
+	OkRowKeys    []string = concat(validNames, longNames, veryLongNames)
+	NotOkRowKeys []string = universallyInvalidIdParts
 )
diff --git a/services/syncbase/testutil/layer.go b/services/syncbase/testutil/layer.go
index de79cac..3492e2e 100644
--- a/services/syncbase/testutil/layer.go
+++ b/services/syncbase/testutil/layer.go
@@ -94,9 +94,29 @@
 	assertExists(t, ctx, self3, "self3", false)
 }
 
+func takeEveryNth(strs []string, n int) []string {
+	var res []string
+	for i, s := range strs {
+		if i%n == 0 {
+			res = append(res, s)
+		}
+	}
+	return res
+}
+
 // Tests that Create() checks name validity.
 func TestCreateNameValidation(t *testing.T, ctx *context.T, i interface{}, okNames, notOkNames []string) {
 	parent := makeLayer(i)
+
+	// Race tests run out of memory when all combinations are run.
+	// TODO(ivanpi): Investigate further. It may be necessary to split up the test
+	// or the v23/syncbase package.
+	if testutil.RaceEnabled {
+		// Use 1/4 of the samples for the race test.
+		okNames = takeEveryNth(okNames, 4)
+		notOkNames = takeEveryNth(notOkNames, 4)
+	}
+
 	for _, name := range okNames {
 		if err := parent.Child(name).Create(ctx, nil); err != nil {
 			t.Fatalf("Create(%q) failed: %v", name, err)
@@ -241,6 +261,16 @@
 func TestListChildIds(t *testing.T, ctx *context.T, rootp security.Principal, i interface{}, blessings, names []string) {
 	self := makeLayer(i)
 
+	// Race tests run out of memory when all combinations are run.
+	// TODO(ivanpi): Investigate further. It may be necessary to split up the test
+	// or the v23/syncbase package.
+	if testutil.RaceEnabled {
+		// Use 1/2 of the names and blessings for the race test, giving 1/4 of the
+		// total Id samples.
+		blessings = takeEveryNth(blessings, 2)
+		names = takeEveryNth(names, 2)
+	}
+
 	var got, want []wire.Id
 	var err error
 
diff --git a/services/syncbase/vsync/cr_policy_filter.go b/services/syncbase/vsync/cr_policy_filter.go
index c21021a..2968174 100644
--- a/services/syncbase/vsync/cr_policy_filter.go
+++ b/services/syncbase/vsync/cr_policy_filter.go
@@ -71,7 +71,11 @@
 // applies to the given oid.
 // TODO(jlodhia): Implement Type based matching.
 func isRuleApplicable(oid string, rule *wire.CrRule) bool {
-	collection, rowKey := common.ParseRowKeyOrDie(oid)
+	collection, rowKey, err := common.ParseRowKey(oid)
+	if err != nil {
+		// TODO(ivanpi): Quarantine database instead.
+		vlog.Fatal(err)
+	}
 	if rule.CollectionId != (wire.Id{}) && collection != rule.CollectionId {
 		return false
 	}