Error if reserved message keys are mixed with unreserved keys (#320)

Makes it an error to provide data that mixes reserved message keys with
unreserved ones. Previously the unreserved keys would be ignored, but
this caused surprising behavior where if someone added a message that
had a key with a reserved word, it would silently break the whole
translation file. This change turns that situation into an error.

Resolves #209

i18n/bundle_test.go

@@ -131,7 +131,7 @@ simple: simple translation
 
 # Comment
 detail:
-  description: detail description 
+  description: detail description
   other: detail translation
 
 # Comment
@@ -150,6 +150,47 @@ everything:
 	expectMessage(t, bundle, language.AmericanEnglish, "everything", everythingMessage)
 }
 
+func TestInvalidYAML(t *testing.T) {
+	bundle := NewBundle(language.English)
+	bundle.RegisterUnmarshalFunc("yaml", yaml.Unmarshal)
+	_, err := bundle.ParseMessageFileBytes([]byte(`
+# Comment
+simple: simple translation
+
+# Comment
+detail:
+  description: detail description
+  other: detail translation
+
+# Comment
+everything:
+  description: everything description
+  zero: zero translation
+  one: one translation
+  two: two translation
+  few: few translation
+  many: many translation
+  other: other translation
+  garbage: something
+
+description: translation
+`), "en-US.yaml")
+
+	expectedErr := &mixedKeysError{
+		reservedKeys:   []string{"description"},
+		unreservedKeys: []string{"detail", "everything", "simple"},
+	}
+	if err == nil {
+		t.Fatalf("expected error %#v; got nil", expectedErr)
+	}
+	if err.Error() != expectedErr.Error() {
+		t.Fatalf("expected error %q; got %q", expectedErr, err)
+	}
+	if c := len(bundle.messageTemplates); c > 0 {
+		t.Fatalf("expected no message templates in bundle; got %d", c)
+	}
+}
+
 func TestTOML(t *testing.T) {
 	bundle := NewBundle(language.English)
 	bundle.RegisterUnmarshalFunc("toml", toml.Unmarshal)

i18n/message.go

@@ -2,6 +2,7 @@ package i18n
 
 import (
 	"fmt"
+	"sort"
 	"strings"
 )
 
@@ -175,47 +176,111 @@ func stringSubmap(k string, v interface{}, strdata map[string]string) error {
 	}
 }
 
-// isMessage tells whether the given data is a message, or a map containing
-// nested messages.
-// A map is assumed to be a message if it contains any of the "reserved" keys:
-// "id", "description", "hash", "leftdelim", "rightdelim", "zero", "one", "two", "few", "many", "other"
-// with a string value.
-// e.g.,
+var reservedKeys = map[string]struct{}{
+	"id":          {},
+	"description": {},
+	"hash":        {},
+	"leftdelim":   {},
+	"rightdelim":  {},
+	"zero":        {},
+	"one":         {},
+	"two":         {},
+	"few":         {},
+	"many":        {},
+	"other":       {},
+	"translation": {},
+}
+
+func isReserved(key string, val any) bool {
+	if _, ok := reservedKeys[key]; ok {
+		if key == "translation" {
+			return true
+		}
+		if _, ok := val.(string); ok {
+			return true
+		}
+	}
+	return false
+}
+
+// isMessage returns true if v contains only message keys and false if it contains no message keys.
+// It returns an error if v contains both message and non-message keys.
 // - {"message": {"description": "world"}} is a message
-// - {"message": {"description": "world", "foo": "bar"}} is a message ("foo" key is ignored)
-// - {"notmessage": {"description": {"hello": "world"}}} is not
-// - {"notmessage": {"foo": "bar"}} is not
-func isMessage(v interface{}) bool {
-	reservedKeys := []string{"id", "description", "hash", "leftdelim", "rightdelim", "zero", "one", "two", "few", "many", "other"}
+// - {"error": {"description": "world", "foo": "bar"}} is an error
+// - {"notmessage": {"description": {"hello": "world"}}} is not a message
+// - {"notmessage": {"foo": "bar"}} is not a message
+func isMessage(v interface{}) (bool, error) {
 	switch data := v.(type) {
 	case nil, string:
-		return true
+		return true, nil
 	case map[string]interface{}:
-		for _, key := range reservedKeys {
+		reservedKeyCount := 0
+		for key := range reservedKeys {
 			val, ok := data[key]
-			if !ok {
-				continue
+			if ok && isReserved(key, val) {
+				reservedKeyCount++
 			}
-			_, ok = val.(string)
-			if !ok {
-				continue
+		}
+		if reservedKeyCount == 0 {
+			return false, nil
+		}
+		if len(data) > reservedKeyCount {
+			reservedKeys := make([]string, 0, reservedKeyCount)
+			unreservedKeys := make([]string, 0, len(data)-reservedKeyCount)
+			for k, v := range data {
+				if isReserved(k, v) {
+					reservedKeys = append(reservedKeys, k)
+				} else {
+					unreservedKeys = append(unreservedKeys, k)
+				}
+			}
+			return false, &mixedKeysError{
+				reservedKeys:   reservedKeys,
+				unreservedKeys: unreservedKeys,
 			}
-			// v is a message if it contains a "reserved" key holding a string value
-			return true
 		}
+		return true, nil
 	case map[interface{}]interface{}:
-		for _, key := range reservedKeys {
+		reservedKeyCount := 0
+		for key := range reservedKeys {
 			val, ok := data[key]
-			if !ok {
-				continue
+			if ok && isReserved(key, val) {
+				reservedKeyCount++
 			}
-			_, ok = val.(string)
-			if !ok {
-				continue
+		}
+		if reservedKeyCount == 0 {
+			return false, nil
+		}
+		if len(data) > reservedKeyCount {
+			reservedKeys := make([]string, 0, reservedKeyCount)
+			unreservedKeys := make([]string, 0, len(data)-reservedKeyCount)
+			for key, v := range data {
+				k, ok := key.(string)
+				if !ok {
+					unreservedKeys = append(unreservedKeys, fmt.Sprintf("%+v", key))
+				} else if isReserved(k, v) {
+					reservedKeys = append(reservedKeys, k)
+				} else {
+					unreservedKeys = append(unreservedKeys, k)
+				}
+			}
+			return false, &mixedKeysError{
+				reservedKeys:   reservedKeys,
+				unreservedKeys: unreservedKeys,
 			}
-			// v is a message if it contains a "reserved" key holding a string value
-			return true
 		}
+		return true, nil
 	}
-	return false
+	return false, nil
+}
+
+type mixedKeysError struct {
+	reservedKeys   []string
+	unreservedKeys []string
+}
+
+func (e *mixedKeysError) Error() string {
+	sort.Strings(e.reservedKeys)
+	sort.Strings(e.unreservedKeys)
+	return fmt.Sprintf("reserved keys %v mixed with unreserved keys %v", e.reservedKeys, e.unreservedKeys)
 }

i18n/parse.go

@@ -43,7 +43,12 @@ func ParseMessageFileBytes(buf []byte, path string, unmarshalFuncs map[string]Un
 		return nil, err
 	}
 
-	if messageFile.Messages, err = recGetMessages(raw, isMessage(raw), true); err != nil {
+	m, err := isMessage(raw)
+	if err != nil {
+		return nil, err
+	}
+
+	if messageFile.Messages, err = recGetMessages(raw, m, true); err != nil {
 		return nil, err
 	}
 
@@ -105,7 +110,11 @@ func recGetMessages(raw interface{}, isMapMessage, isInitialCall bool) ([]*Messa
 		messages = make([]*Message, 0, len(data))
 		for _, data := range data {
 			// recursively scan slice items
-			childMessages, err := recGetMessages(data, isMessage(data), false)
+			m, err := isMessage(data)
+			if err != nil {
+				return nil, err
+			}
+			childMessages, err := recGetMessages(data, m, false)
 			if err != nil {
 				return nil, err
 			}
@@ -127,7 +136,10 @@ func recGetMessages(raw interface{}, isMapMessage, isInitialCall bool) ([]*Messa
 }
 
 func addChildMessages(id string, data interface{}, messages []*Message) ([]*Message, error) {
-	isChildMessage := isMessage(data)
+	isChildMessage, err := isMessage(data)
+	if err != nil {
+		return nil, err
+	}
 	childMessages, err := recGetMessages(data, isChildMessage, false)
 	if err != nil {
 		return nil, err

i18n/parse_test.go

@@ -33,6 +33,29 @@ func TestParseMessageFileBytes(t *testing.T) {
 				}},
 			},
 		},
+		{
+			name: "nested with reserved key",
+			file: `{"nested": {"description": {"other": "world"}}}`,
+			path: "en.json",
+			messageFile: &MessageFile{
+				Path:   "en.json",
+				Tag:    language.English,
+				Format: "json",
+				Messages: []*Message{{
+					ID:    "nested.description",
+					Other: "world",
+				}},
+			},
+		},
+		{
+			name: "basic test reserved key top level",
+			file: `{"other": "world", "foo": "bar"}`,
+			path: "en.json",
+			err: &mixedKeysError{
+				reservedKeys:   []string{"other"},
+				unreservedKeys: []string{"foo"},
+			},
+		},
 		{
 			name: "basic test with dot separator in key",
 			file: `{"prepended.hello": "world"}`,
@@ -98,14 +121,9 @@ func TestParseMessageFileBytes(t *testing.T) {
 			name: "basic test with description and dummy",
 			file: `{"notnested": {"description": "world", "dummy": "nothing"}}`,
 			path: "en.json",
-			messageFile: &MessageFile{
-				Path:   "en.json",
-				Tag:    language.English,
-				Format: "json",
-				Messages: []*Message{{
-					ID:          "notnested",
-					Description: "world",
-				}},
+			err: &mixedKeysError{
+				reservedKeys:   []string{"description"},
+				unreservedKeys: []string{"dummy"},
 			},
 		},
 		{
@@ -198,6 +216,19 @@ some-keys:
 			unmarshalFuncs: map[string]UnmarshalFunc{"yaml": yaml.Unmarshal},
 			err:            errors.New("expected key to be string but got 2"),
 		},
+		{
+			name: "YAML extra number key test",
+			file: `
+some-keys:
+    other: world
+    2: legit`,
+			path:           "en.yaml",
+			unmarshalFuncs: map[string]UnmarshalFunc{"yaml": yaml.Unmarshal},
+			err: &mixedKeysError{
+				reservedKeys:   []string{"other"},
+				unreservedKeys: []string{"2"},
+			},
+		},
 	}
 	for _, testCase := range testCases {
 		t.Run(testCase.name, func(t *testing.T) {
@@ -221,13 +252,21 @@ some-keys:
 					t.Errorf("expected format %q; got %q", testCase.messageFile.Format, actual.Format)
 				}
 				if !equalMessages(actual.Messages, testCase.messageFile.Messages) {
-					t.Errorf("expected %#v; got %#v", testCase.messageFile.Messages, actual.Messages)
+					t.Errorf("expected %#v; got %#v", deref(testCase.messageFile.Messages), deref(actual.Messages))
 				}
 			}
 		})
 	}
 }
 
+func deref(mptrs []*Message) []Message {
+	messages := make([]Message, len(mptrs))
+	for i, m := range mptrs {
+		messages[i] = *m
+	}
+	return messages
+}
+
 // equalMessages compares two slices of messages, ignoring private fields and order.
 // Sorts both input slices, which are therefore modified by this function.
 func equalMessages(m1, m2 []*Message) bool {