merge prerelease branch

Author: eli-darkly

ldattr/errors.go

@@ -5,6 +5,8 @@ import (
 )
 
 var (
-	errAttributeEmpty      = errors.New("attribute reference cannot be empty")
-	errAttributeExtraSlash = errors.New("attribute reference contained a double slash or a trailing slash")
+	errAttributeEmpty         = errors.New("attribute reference cannot be empty")
+	errAttributeExtraSlash    = errors.New("attribute reference contained a double slash or a trailing slash")
+	errAttributeInvalidEscape = errors.New(
+		"attribute reference contained an escape character (~) that was not followed by 0 or 1")
 )

ldattr/package.go

@@ -3,4 +3,7 @@
 // This includes the ldattr.Ref type, which provides a syntax similar to JSON Pointer for
 // referencing values either of a top-level context attribute, or of a value within a JSON object
 // or JSON array. It also includes constants for the names of some built-in attributes.
+//
+// These types and constants are mainly intended to be used internally by LaunchDarkly SDK and
+// service code. Applications are unlikely to need to use them directly.
 package ldattr

ldattr/ref.go

@@ -12,17 +12,55 @@ import (
 
 // Ref is an attribute name or path expression identifying a value within a Context.
 //
-// This can be used to retrieve a value with Context.GetValueForRef(), or to identify an attribute or
+// This type is mainly intended to be used internally by LaunchDarkly SDK and service code, where
+// efficiency is a major concern so it's desirable to do any parsing or preprocessing just once.
+// Applications are unlikely to need to use the Ref type directly.
+//
+// It can be used to retrieve a value with Context.GetValueForRef(), or to identify an attribute or
 // nested value that should be considered private with Builder.Private() (the SDK configuration can also
 // have a list of private attribute references).
 //
-// This is represented as a separate type, rather than just a string, so that validation and parsing can
-// be done ahead of time if an attribute reference will be used repeatedly later (such as in flag
-// evaluations).
+// Parsing and validation are done at the time that the NewRef or NewLiteralRef constructor is called.
+// If a Ref instance was created from an invalid string, or if it is an uninitialized Ref{}, it is
+// considered invalid and its Err() method will return a non-nil error.
+//
+// Syntax
+//
+// The string representation of an attribute reference in LaunchDarkly JSON data uses the following
+// syntax:
+//
+// If the first character is not a slash, the string is interpreted literally as an attribute name.
+// An attribute name can contain any characters, but must not be empty.
+//
+// If the first character is a slash, the string is interpreted as a slash-delimited path where the
+// first path component is an attribute name, and each subsequent path component is either the name of
+// a property in a JSON object, or a decimal numeric string that is the index of an element in a JSON
+// array. Any instances of the characters "/" or "~" in a path component are escaped as "~1" or "~0"
+// respectively. This syntax deliberately resembles JSON Pointer, but no JSON Pointer behaviors other
+// than those mentioned here are supported.
 //
-// Call NewRef() to create an Ref.  An uninitialized ldattr.Ref{} struct is not valid for use in any
-// SDK operations. Also, an Ref can be in an error state if it was built from an invalid string. See
-// Ref.Err().
+// Examples
+//
+// Suppose there is a context whose JSON implementation looks like this:
+//
+//     {
+//       "kind": "user",
+//       "key": "value1",
+//       "address": {
+//         "street": "value2",
+//         "city": "value3"
+//       },
+//       "groups": [ "value4", "value5" ],
+//       "good/bad": "value6"
+//     }
+//
+// The attribute references "key" and "/key" would both point to "value1".
+//
+// The attribute reference "/address/street" would point to "value2".
+//
+// The attribute reference "/groups/0" would point to "value4".
+//
+// The attribute references "good/bad" and "/good~1bad" would both point to "value6".
 type Ref struct {
 	err                 error
 	rawPath             string
@@ -35,38 +73,12 @@ type attrRefComponent struct {
 	intValue ldvalue.OptionalInt
 }
 
-// NewRef creates a Ref from a string.
-//
-// If the string starts with '/', then this is treated as a slash-delimited path reference where the
-// first component is the name of an attribute, and subsequent components are the names of nested JSON
-// object properties (or, if they are numeric, the indices of JSON array elements). In this syntax, the
-// escape sequences "~0" and "~1" represent '~' and '/' respectively within a path component.
-//
-// If the string does not start with '/', then it is treated as the literal name of an attribute (the
-// same as NewNameRef).
-//
-// For instance, if the JSON representation of a context is as follows--
-//
-//     {
-//       "kind": "user",
-//       "key": "123",
-//       "name": "xyz",
-//       "address": {
-//         "street": "99 Main St.",
-//         "city": "Westview"
-//       },
-//       "groups": [ "p", "q" ],
-//       "a/b": "ok"
-//     }
-//
-// --then NewRef("name") or NewRef("/name") would refer to the value "xyz"; NewRef("/address/street")
-// would refer to the value "99 Main St."; NewRef("/groups/0") would refer to the value "p"; and
-// NewRef("a/b") or NewRef("/a~1b") would refer to the value "ok".
+// NewRef creates a Ref from a string. For the supported syntax and examples, see comments on the Ref type.
 //
 // This constructor always returns a Ref that preserves the original string, even if validation fails,
 // so that calling String() (or serializing the Ref to JSON) will produce the original string. If
-// validation fails, Err() will return nil and any SDK method that takes this Ref as a parameter will
-// consider it invalid.
+// validation fails, Err() will return a non-nil error and any SDK method that takes this Ref as a
+// parameter will consider it invalid.
 func NewRef(referenceString string) Ref {
 	if referenceString == "" || referenceString == "/" {
 		return Ref{err: errAttributeEmpty, rawPath: referenceString}
@@ -79,7 +91,10 @@ func NewRef(referenceString string) Ref {
 	if !strings.Contains(path, "/") {
 		// There's only one segment, so this is still a simple attribute reference. However, we still may
 		// need to unescape special characters.
-		return Ref{singlePathComponent: unescapePath(path), rawPath: referenceString}
+		if unescaped, ok := unescapePath(path); ok {
+			return Ref{singlePathComponent: unescaped, rawPath: referenceString}
+		}
+		return Ref{err: errAttributeInvalidEscape, rawPath: referenceString}
 	}
 	parts := strings.Split(path, "/")
 	ret := Ref{rawPath: referenceString, components: make([]attrRefComponent, 0, len(parts))}
@@ -88,7 +103,11 @@ func NewRef(referenceString string) Ref {
 			ret.err = errAttributeExtraSlash
 			return ret
 		}
-		component := attrRefComponent{name: unescapePath(p)}
+		unescaped, ok := unescapePath(p)
+		if !ok {
+			return Ref{err: errAttributeInvalidEscape, rawPath: referenceString}
+		}
+		component := attrRefComponent{name: unescaped}
 		if p[0] >= '0' && p[0] <= '9' {
 			if n, err := strconv.Atoi(p); err == nil {
 				component.intValue = ldvalue.NewOptionalInt(n)
@@ -99,11 +118,16 @@ func NewRef(referenceString string) Ref {
 	return ret
 }
 
-// NewNameRef is similar to NewRef except that it always interprets the string as a single
+// NewLiteralRef is similar to NewRef except that it always interprets the string as a literal
 // attribute name, never as a slash-delimited path expression. There is no escaping or unescaping,
-// even if the name contains literal '/' or '~' characters. This method always returns a valid Ref
-// unless the name is empty.
-func NewNameRef(attrName string) Ref {
+// even if the name contains literal '/' or '~' characters. Since an attribute name can contain
+// any characters, this method always returns a valid Ref unless the name is empty.
+//
+// For example: ldattr.NewLiteralRef("name") is exactly equivalent to ldattr.NewRef("name").
+// ldattr.NewLiteralRef("a/b") is exactly equivalent to ldattr.NewRef("a/b") (since the syntax
+// used by NewRef treats the whole string as a literal as long as it does not start with a slash),
+// or to ldattr.NewRef("/a~1b").
+func NewLiteralRef(attrName string) Ref {
 	if attrName == "" {
 		return Ref{err: errAttributeEmpty, rawPath: attrName}
 	}
@@ -140,13 +164,17 @@ func (a Ref) Equal(other Ref) bool {
 //
 // A Ref can only be invalid for the following reasons:
 //
-// - The input string was empty, or consisted only of "/".
+// 1. The input string was empty, or consisted only of "/".
+//
+// 2. A slash-delimited string had a double slash causing one component to be empty, such as "/a//b".
 //
-// - A slash-delimited string had a double slash causing one component to be empty, such as "/a//b".
+// 3. A slash-delimited string contained a "~" character that was not followed by "0" or "1".
 //
 // Otherwise, the Ref is valid, but that does not guarantee that such an attribute exists in any
 // given Context. For instance, NewRef("name") is a valid Ref, but a specific Context might or might
 // not have a name.
+//
+// See comments on the Ref type for more details of the attribute reference syntax.
 func (a Ref) Err() error {
 	if a.err == nil && a.rawPath == "" {
 		return errAttributeEmpty
@@ -237,10 +265,39 @@ func (a *Ref) UnmarshalJSON(data []byte) error {
 	return nil
 }
 
-func unescapePath(path string) string {
+// Performs unescaping of attribute reference path components:
+//
+// "~1" becomes "/"
+// "~0" becomes "~"
+// "~" followed by any character other than "0" or "1" is invalid
+//
+// The second return value is true if successful, or false if there was an invalid escape sequence.
+func unescapePath(path string) (string, bool) {
 	// If there are no tildes then there's definitely nothing to do
 	if !strings.Contains(path, "~") {
-		return path
+		return path, true
+	}
+	out := make([]byte, 0, 100) // arbitrary preallocated size - path components will almost always be shorter than this
+	for i := 0; i < len(path); i++ {
+		ch := path[i]
+		if ch != '~' {
+			out = append(out, ch)
+			continue
+		}
+		i++
+		if i >= len(path) {
+			return "", false
+		}
+		var unescaped byte
+		switch path[i] {
+		case '0':
+			unescaped = '~'
+		case '1':
+			unescaped = '/'
+		default:
+			return "", false
+		}
+		out = append(out, unescaped)
 	}
-	return strings.ReplaceAll(strings.ReplaceAll(path, "~1", "/"), "~0", "~")
+	return string(out), true
 }

ldattr/ref_test.go

@@ -20,6 +20,10 @@ func TestRefInvalid(t *testing.T) {
 		{"//", errAttributeExtraSlash},
 		{"/a//b", errAttributeExtraSlash},
 		{"/a/b/", errAttributeExtraSlash},
+		{"/a~x", errAttributeInvalidEscape},
+		{"/a~", errAttributeInvalidEscape},
+		{"/a/b~x", errAttributeInvalidEscape},
+		{"/a/b~", errAttributeInvalidEscape},
 	} {
 		t.Run(fmt.Sprintf("input string %q", p.input), func(t *testing.T) {
 			a := NewRef(p.input)
@@ -69,7 +73,7 @@ func TestRefSimpleWithLeadingSlash(t *testing.T) {
 		{"/name", "name"},
 		{"/custom", "custom"},
 		{"/0", "0"},
-		{"/name~1with~1slashes~0and~0tildes~2~x~~", "name/with/slashes~and~tildes~2~x~~"},
+		{"/name~1with~1slashes~0and~0tildes", "name/with/slashes~and~tildes"},
 	} {
 		t.Run(fmt.Sprintf("input string %q", params.input), func(t *testing.T) {
 			a := NewRef(params.input)
@@ -83,22 +87,22 @@ func TestRefSimpleWithLeadingSlash(t *testing.T) {
 	}
 }
 
-func TestNewNameRef(t *testing.T) {
-	a0 := NewNameRef("name")
+func TestNewLiteralRef(t *testing.T) {
+	a0 := NewLiteralRef("name")
 	assert.Equal(t, NewRef("name"), a0)
 
-	a1 := NewNameRef("a/b")
+	a1 := NewLiteralRef("a/b")
 	assert.Equal(t, NewRef("a/b"), a1)
 
-	a2 := NewNameRef("/a/b~c")
+	a2 := NewLiteralRef("/a/b~c")
 	assert.Equal(t, NewRef("/~1a~1b~0c"), a2)
 	assert.Equal(t, 1, a2.Depth())
 
-	a3 := NewNameRef("/")
+	a3 := NewLiteralRef("/")
 	assert.Equal(t, NewRef("/~1"), a3)
 	assert.Equal(t, 1, a3.Depth())
 
-	a4 := NewNameRef("")
+	a4 := NewLiteralRef("")
 	assert.Equal(t, errAttributeEmpty, a4.Err())
 }
 
@@ -117,6 +121,7 @@ func TestRefComponents(t *testing.T) {
 		{"/a/b", 2, 0, "a", undefined},
 		{"/a/b", 2, 1, "b", undefined},
 		{"/a~1b/c", 2, 0, "a/b", undefined},
+		{"/a~0b/c", 2, 0, "a~b", undefined},
 		{"/a/10/20/30x", 4, 1, "10", 10},
 		{"/a/10/20/30x", 4, 2, "20", 20},
 		{"/a/10/20/30x", 4, 3, "30x", undefined},

ldcontext/builder_multi.go

@@ -48,11 +48,23 @@ func NewMultiBuilder() *MultiBuilder {
 // to see if it has an error. See Context.Err() for more information about invalid Context
 // conditions. Using a single-return-value syntax is more convenient for application code, since
 // in normal usage an application will never build an invalid Context.
+//
+// If only one context kind was added to the builder, Build returns a single-kind context rather
+// than a multi-kind context.
 func (m *MultiBuilder) Build() Context {
 	if len(m.contexts) == 0 {
 		return Context{err: errContextKindMultiWithNoKinds}
 	}
 
+	if len(m.contexts) == 1 {
+		// Never return a multi-kind context with just one kind; instead return the individual one
+		c := m.contexts[0]
+		if c.Multiple() {
+			return Context{err: errContextKindMultiWithinMulti}
+		}
+		return c
+	}
+
 	m.contextsCopyOnWrite = true // see note on ___CopyOnWrite in Builder.Build()
 
 	// Sort the list by kind - this makes our output deterministic and will also be important when we
@@ -97,18 +109,14 @@ func (m *MultiBuilder) Build() Context {
 		multiContexts: m.contexts,
 	}
 
-	if len(m.contexts) == 1 && m.contexts[0].kind != DefaultKind {
-		ret.fullyQualifiedKey = m.contexts[0].fullyQualifiedKey
-	} else {
-		// Fully-qualified key for multi-kind is defined as "kind1:key1:kind2:key2" etc., where kinds are in
-		// alphabetical order (we have already sorted them above) and keys are URL-encoded. In this case we
-		// do _not_ omit a default kind of "user".
-		for _, c := range m.contexts {
-			if ret.fullyQualifiedKey != "" {
-				ret.fullyQualifiedKey += ":"
-			}
-			ret.fullyQualifiedKey += makeFullyQualifiedKeySingleKind(c.kind, c.key, false)
+	// Fully-qualified key for multi-kind is defined as "kind1:key1:kind2:key2" etc., where kinds are in
+	// alphabetical order (we have already sorted them above) and keys are URL-encoded. In this case we
+	// do _not_ omit a default kind of "user".
+	for _, c := range m.contexts {
+		if ret.fullyQualifiedKey != "" {
+			ret.fullyQualifiedKey += ":"
 		}
+		ret.fullyQualifiedKey += makeFullyQualifiedKeySingleKind(c.kind, c.key, false)
 	}
 
 	return ret