merge prerelease branch

Author: eli-darkly

ldcontext/builder_multi.go

@@ -56,12 +56,8 @@ func (m *MultiBuilder) Build() Context {
 	}
 
 	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{defined: true, err: lderrors.ErrContextKindMultiWithinMulti{}}
-		}
-		return c
+		// If only one context was added, the result is just the same as that one
+		return m.contexts[0]
 	}
 
 	m.contextsCopyOnWrite = true // see note on ___CopyOnWrite in Builder.Build()
@@ -72,7 +68,6 @@ func (m *MultiBuilder) Build() Context {
 
 	// Check for conditions that could make a multi-kind context invalid
 	var individualErrors map[string]error
-	nestedMulti := false
 	duplicates := false
 	for i, c := range m.contexts {
 		err := c.Err()
@@ -82,8 +77,6 @@ func (m *MultiBuilder) Build() Context {
 				individualErrors = make(map[string]error)
 			}
 			individualErrors[string(c.Kind())] = err
-		case c.Multiple(): // multi-kind inside multi-kind is not allowed
-			nestedMulti = true
 		default:
 			for j := 0; j < i; j++ {
 				if c.Kind() == m.contexts[j].Kind() { // same kind was seen twice
@@ -95,8 +88,6 @@ func (m *MultiBuilder) Build() Context {
 	}
 	var err error
 	switch {
-	case nestedMulti:
-		err = lderrors.ErrContextKindMultiWithinMulti{}
 	case duplicates:
 		err = lderrors.ErrContextKindMultiDuplicates{}
 	case len(individualErrors) != 0:
@@ -168,11 +159,28 @@ func (m *MultiBuilder) TryBuild() (Context, error) {
 //
 // It is invalid to add more than one context with the same Kind. This error is detected
 // when you call Build() or TryBuild().
+//
+// If the nested context is multi-kind, this is exactly equivalent to adding each of the
+// individual kinds from it separately. For instance, in the following example, "multi1" and
+// "multi2" end up being exactly the same:
+//
+//     c1 := ldcontext.NewWithKind("kind1", "key1")
+//     c2 := ldcontext.NewWithKind("kind2", "key2")
+//     c3 := ldcontext.NewWithKind("kind3", "key3")
+//
+//     multi1 := ldcontext.NewMultiBuilder().Add(c1).Add(c2).Add(c3).Build()
+//
+//     c1plus2 := ldcontext.NewMultiBuilder().Add(c1).Add(c2).Build()
+//     multi2 := ldcontext.NewMultiBuilder().Add(c1plus2).Add(c3).Build()
 func (m *MultiBuilder) Add(context Context) *MultiBuilder {
 	if m.contextsCopyOnWrite {
 		m.contexts = append([]Context(nil), m.contexts...)
 		m.contextsCopyOnWrite = true
 	}
-	m.contexts = append(m.contexts, context)
+	if context.Multiple() {
+		m.contexts = append(m.contexts, context.multiContexts...)
+	} else {
+		m.contexts = append(m.contexts, context)
+	}
 	return m
 }

ldcontext/builder_multi_test.go

@@ -29,6 +29,18 @@ func TestMultiBuilder(t *testing.T) {
 		assert.Equal(t, []Context{sub1, sub2}, c0.GetAllIndividualContexts(nil))
 		// other accessors are tested in context_test.go
 	})
+
+	t.Run("nested multi-kind contexts are flattened", func(t *testing.T) {
+		sub1 := NewWithKind("kind1", "key1")
+		sub2 := NewWithKind("kind2", "key2")
+		sub3 := NewWithKind("kind3", "key3")
+		sub4 := NewWithKind("kind3", "key3")
+		c0 := NewMultiBuilder().Add(sub2).Add(sub3).Build()
+		c1 := NewMultiBuilder().Add(sub1).Add(c0).Add(sub4).Build()
+
+		expected := NewMultiBuilder().Add(sub1).Add(sub2).Add(sub3).Add(sub4).Build()
+		assert.Equal(t, expected.multiContexts, c1.multiContexts)
+	})
 }
 
 func TestMultiBuilderFullyQualifiedKey(t *testing.T) {
@@ -43,6 +55,14 @@ func TestMultiBuilderFullyQualifiedKey(t *testing.T) {
 			Build()
 		assert.Equal(t, "kind-a:key-2:kind-b:key-4:kind-c:key-1:kind-d:key-3", c.FullyQualifiedKey())
 	})
+
+	t.Run("keys are escaped", func(t *testing.T) {
+		c := NewMultiBuilder().
+			Add(NewWithKind("kind-a", "key-1")).
+			Add(NewWithKind("kind-b", "key:2")).
+			Build()
+		assert.Equal(t, "kind-a:key-1:kind-b:key%3A2", c.FullyQualifiedKey())
+	})
 }
 
 func TestMultiBuilderErrors(t *testing.T) {
@@ -61,16 +81,6 @@ func TestMultiBuilderErrors(t *testing.T) {
 		verifyError(t, NewMultiBuilder(), lderrors.ErrContextKindMultiWithNoKinds{})
 	})
 
-	t.Run("nested multi", func(t *testing.T) {
-		sub1 := NewWithKind("org", "my-key")
-		sub2 := NewMulti(New("user-key"), NewWithKind("org", "other"))
-		b0 := NewMultiBuilder().Add(sub1).Add(sub2)
-		verifyError(t, b0, lderrors.ErrContextKindMultiWithinMulti{})
-
-		b1 := NewMultiBuilder().Add(sub2)
-		verifyError(t, b1, lderrors.ErrContextKindMultiWithinMulti{})
-	})
-
 	t.Run("duplicate kind", func(t *testing.T) {
 		sub1 := NewWithKind("org", "my-org-key")
 		sub2 := NewWithKind("user", "my-user-key")

ldcontext/builder_simple.go

@@ -2,7 +2,7 @@ package ldcontext
 
 import (
 	"fmt"
-	"net/url"
+	"strings"
 
 	"github.com/launchdarkly/go-sdk-common/v3/ldattr"
 	"github.com/launchdarkly/go-sdk-common/v3/lderrors"
@@ -279,8 +279,9 @@ func (b *Builder) SetString(attributeName string, value string) *Builder {
 // SetValue sets the value of any attribute for the Context.
 //
 // This includes only attributes that are addressable in evaluations-- not metadata such as
-// Secondary() or Private(). If attributeName is "secondary" or "privateAttributes", it is
-// ignored and no attribute is set.
+// Secondary() or Private(). If attributeName is "secondary" or "privateAttributes", you will be
+// setting an attribute with that name which you can use in evaluations or to record data for
+// your own purposes, but it will be unrelated to Secondary() and Private().
 //
 // This method uses the ldvalue.Value type to represent a value of any JSON type: null, boolean,
 // number, string, array, or object. For all attribute names that do not have special meaning
@@ -297,6 +298,9 @@ func (b *Builder) SetString(attributeName string, value string) *Builder {
 //
 // - "anonymous": Must be a boolean. See Builder.Anonymous().
 //
+// The attribute name "_meta" is not allowed, because it has special meaning in the JSON
+// schema for contexts; any attempt to set an attribute with this name has no effect.
+//
 // Values that are JSON arrays or objects have special behavior when referenced in flag/segment
 // rules.
 //
@@ -341,7 +345,7 @@ func (b *Builder) TrySetValue(attributeName string, value ldvalue.Value) bool {
 			return false
 		}
 		b.Anonymous(value.BoolValue())
-	case jsonPropPrivate, jsonPropSecondary:
+	case jsonPropMeta:
 		return false
 	default:
 		if value.IsNull() {
@@ -360,8 +364,10 @@ func (b *Builder) TrySetValue(attributeName string, value ldvalue.Value) bool {
 // (https://docs.launchdarkly.com/home/flags/targeting-users#targeting-rules-based-on-user-attributes)
 // as follows: if you have chosen to bucket users by a specific attribute, the secondary key (if set)
 // is used to further distinguish between users who are otherwise identical according to that attribute.
-// This value is not addressable as an attribute in evaluations: that is, a rule clause cannot use the
-// attribute name "secondary".
+//
+// This is a metadata property, rather than an attribute that can be addressed in evaluations: that is,
+// a rule clause that references the attribute name "secondary" will not use this value, but instead will
+// use whatever value (if any) you have set for the name "secondary" with a method such as SetString.
 //
 // Setting this value to an empty string is not the same as leaving it unset. If you need to clear this
 // attribute to a "no value" state, use OptSecondary().
@@ -374,9 +380,6 @@ func (b *Builder) Secondary(value string) *Builder {
 // Calling b.OptSecondary(ldvalue.NewOptionalString("x")) is equivalent to b.Secondary("x"), but since it uses
 // the OptionalString type, it also allows clearing a previously set name with
 // b.OptSecondary(ldvalue.OptionalString{}).
-//
-// This value is not addressable as an attribute in evaluations: that is, a rule clause cannot use the
-// attribute name "secondary".
 func (b *Builder) OptSecondary(value ldvalue.OptionalString) *Builder {
 	if b != nil {
 		b.secondary = value
@@ -396,7 +399,7 @@ func (b *Builder) OptSecondary(value ldvalue.OptionalString) *Builder {
 // other attributes may be included (so, for instance, Anonymous does not mean there is no Name).
 //
 // This value is also addressable in evaluations as the attribute name "anonymous". It is always treated as
-// a boolean true or false in evaluations.
+// a boolean true or false in evaluations; it cannot be null/undefined.
 func (b *Builder) Anonymous(value bool) *Builder {
 	if b != nil {
 		b.anonymous = value
@@ -407,9 +410,6 @@ func (b *Builder) Anonymous(value bool) *Builder {
 // Private designates any number of Context attributes, or properties within them, as private: that is,
 // their values will not be sent to LaunchDarkly.
 //
-// (TKTK: possibly move some of this conceptual information to a non-platform-specific docs page and/or
-// have docs team copyedit it here)
-//
 // Each parameter can be a simple attribute name, such as "email". Or, if the first character is a slash,
 // the parameter is interpreted as a slash-delimited path to a property within a JSON object, where the
 // first path component is a Context attribute name and each following component is a nested property name:
@@ -434,6 +434,11 @@ func (b *Builder) Anonymous(value bool) *Builder {
 //         SetString("lastName", "Menard").
 //	       Private("firstName").
 //         Build()
+//
+// This is a metadata property, rather than an attribute that can be addressed in evaluations: that is,
+// a rule clause that references the attribute name "private" (or "privateAttributes", as it appears
+// in JSON representations) will not use this value, but instead will use whatever value (if any) you
+// have set for that name with a method such as SetString.
 func (b *Builder) Private(attrRefStrings ...string) *Builder {
 	refs := make([]ldattr.Ref, 0, 20) // arbitrary capacity that's likely greater than needed, to preallocate on stack
 	for _, s := range attrRefStrings {
@@ -520,10 +525,12 @@ func (b *Builder) copyFrom(fromContext Context) {
 func makeFullyQualifiedKeySingleKind(kind Kind, key string, omitDefaultKind bool) string {
 	// Per the users-to-contexts specification, the fully-qualified key for a single-kind context is:
 	// - equal to the regular "key" property, if the kind is "user" (a.k.a. DefaultKind)
-	// - or, for any other kind, it's the kind plus ":" plus the result of URL-encoding the "key"
-	// property (the URL-encoding is to avoid ambiguity if the key contains colons).
+	// - or, for any other kind, it's the kind plus ":" plus the result of partially URL-encoding the
+	// "key" property ("partially URL-encoding" here means that ':' and '%' are percent-escaped; other
+	// URL-encoding behaviors are inconsistent across platforms, so we do not use a library function).
 	if omitDefaultKind && kind == DefaultKind {
 		return key
 	}
-	return fmt.Sprintf("%s:%s", kind, url.PathEscape(key))
+	escapedKey := strings.ReplaceAll(strings.ReplaceAll(key, "%", "%25"), ":", "%3A")
+	return fmt.Sprintf("%s:%s", kind, escapedKey)
 }

ldcontext/builder_simple_test.go

@@ -87,6 +87,11 @@ func TestBuilderFullyQualifiedKey(t *testing.T) {
 		c := NewWithKind("org", "my-org-key")
 		assert.Equal(t, "org:my-org-key", c.FullyQualifiedKey())
 	})
+
+	t.Run("key is escaped", func(t *testing.T) {
+		c := NewWithKind("org", "my:key%x/y")
+		assert.Equal(t, "org:my%3Akey%25x/y", c.FullyQualifiedKey())
+	})
 }
 
 func TestBuilderBasicSetters(t *testing.T) {
@@ -297,16 +302,25 @@ func TestBuilderSetBuiltInAttributesByName(t *testing.T) {
 }
 
 func TestBuilderSetValueCannotSetMetaProperties(t *testing.T) {
-	t.Run("privateAttributes", func(t *testing.T) {
-		assert.Equal(t,
-			makeBasicBuilder(),
-			makeBasicBuilder().SetValue("privateAttributes", ldvalue.ArrayOf(ldvalue.String("x"))))
-	})
+	for _, p := range []struct {
+		name  string
+		value ldvalue.Value
+	}{
+		{"secondary", ldvalue.String("x")},
+		{"privateAttributes", ldvalue.ArrayOf(ldvalue.String("x"))},
+	} {
+		t.Run(p.name, func(t *testing.T) {
+			c := makeBasicBuilder().SetValue(p.name, p.value).Build()
+			assert.Equal(t, p.value, c.attributes.Get(p.name))
+			assert.Equal(t, ldvalue.OptionalString{}, c.secondary)
+			assert.Len(t, c.privateAttrs, 0)
+		})
+	}
 
-	t.Run("secondary", func(t *testing.T) {
-		assert.Equal(t,
-			makeBasicBuilder(),
-			makeBasicBuilder().SetValue("secondary", ldvalue.String("x")))
+	t.Run("_meta", func(t *testing.T) {
+		b := makeBasicBuilder()
+		assert.False(t, b.TrySetValue("_meta", ldvalue.String("hi")))
+		assert.Equal(t, 0, b.Build().attributes.Count())
 	})
 }
 

ldcontext/constructors.go

@@ -28,11 +28,23 @@ func NewWithKind(kind Kind, key string) Context {
 // To create a single-kind Context, use New(), NewWithKind, or NewBuilder().
 //
 // For the returned Context to be valid, the contexts list must not be empty, and all of its
-// elements must be single-kind Contexts. Otherwise, the returned Context will be invalid as
-// reported by Context.Err().
+// elements must be valid Contexts. Otherwise, the returned Context will be invalid as reported
+// by Context.Err().
 //
-// If only one context parameter is given, NewMulti returns a single-kind context (that is,
-// just that same context) rather than a multi-kind context.
+// If only one context parameter is given, NewMulti returns that same context.
+//
+// If one of the nested contexts is multi-kind, this is exactly equivalent to adding each of the
+// individual kinds from it separately. For instance, in the following example, "multi1" and
+// "multi2" end up being exactly the same:
+//
+//     c1 := ldcontext.NewWithKind("kind1", "key1")
+//     c2 := ldcontext.NewWithKind("kind2", "key2")
+//     c3 := ldcontext.NewWithKind("kind3", "key3")
+//
+//     multi1 := ldcontext.NewMulti(c1, c2, c3)
+//
+//     c1plus2 := ldcontext.NewMulti(c1, c2)
+//     multi2 := ldcontext.NewMulti(c1plus2, c3)
 func NewMulti(contexts ...Context) Context {
 	// Same rationale as for New/NewWithKey of using the builder instead of constructing directly.
 	var m MultiBuilder

ldcontext/constructors_test.go

@@ -39,9 +39,12 @@ func TestNewWithKindErrors(t *testing.T) {
 }
 
 func TestNewMulti(t *testing.T) {
-	c1 := NewWithKind("org", "my-org-key")
-	c2 := NewWithKind("user", "my-user-key")
-	c0 := NewMulti(c1, c2)
-
-	assert.Equal(t, NewMultiBuilder().Add(c1).Add(c2).Build(), c0)
+	sub1 := NewWithKind("kind1", "key1")
+	sub2 := NewWithKind("kind2", "key2")
+	sub3 := NewWithKind("kind3", "key3")
+	sub4 := NewWithKind("kind3", "key3")
+
+	assert.Equal(t, NewMultiBuilder().Add(sub1).Add(sub2).Build(), NewMulti(sub1, sub2))
+	assert.Equal(t, NewMultiBuilder().Add(sub1).Add(sub2).Add(sub3).Add(sub4).Build(),
+		NewMulti(sub1, NewMulti(sub2, sub3), sub4))
 }

ldcontext/context.go

@@ -11,8 +11,6 @@ import (
 
 // Context is a collection of attributes that can be referenced in flag evaluations and analytics events.
 //
-// (TKTK - some conceptual text here, and/or a link to a docs page)
-//
 // To create a Context of a single kind, such as a user, you may use the New() or NewWithKind()
 // constructors. Or, to specify other attributes, use NewBuilder().
 //

ldcontext/context_easyjson.go

@@ -290,7 +290,9 @@ func unmarshalOldUserSchemaEasyJSON(c *Context, in *jlexer.Lexer, usingEventForm
 				} else {
 					var value ldvalue.Value
 					value.UnmarshalEasyJSON(in)
-					attributes.Set(name, value)
+					if isOldUserCustomAttributeNameAllowed(name) {
+						attributes.Set(name, value)
+					}
 				}
 				in.WantComma()
 			}

ldcontext/context_unmarshaling.go

@@ -8,7 +8,7 @@ import (
 	"github.com/launchdarkly/go-jsonstream/v2/jreader"
 )
 
-// See internalAttributeNameIfPossible().
+// See internAttributeNameIfPossible().
 var internCommonAttributeNamesMap = makeInternCommonAttributeNamesMap() //nolint:gochecknoglobals
 
 func makeInternCommonAttributeNamesMap() map[string]string {
@@ -24,9 +24,6 @@ func makeInternCommonAttributeNamesMap() map[string]string {
 // LaunchDarkly's JSON schema for contexts is standardized across SDKs. For unmarshaling, there are
 // three supported formats:
 //
-// (TKTK: consider moving all this content into a non-platform-specific online docs page, since none
-// of this is specific to Go)
-//
 // 1. A single-kind context, identified by a top-level "kind" property that is not "multi".
 //
 // 2. A multi-kind context, identified by a top-level "kind" property of "multi".
@@ -200,7 +197,9 @@ func unmarshalOldUserSchema(c *Context, r *jreader.Reader, usingEventFormat bool
 				name := string(customObj.Name())
 				var value ldvalue.Value
 				value.ReadFromJSONReader(r)
-				b.SetValue(name, value)
+				if isOldUserCustomAttributeNameAllowed(name) {
+					b.SetValue(name, value)
+				}
 			}
 		case jsonPropOldUserPrivate:
 			if usingEventFormat {
@@ -236,6 +235,17 @@ func unmarshalOldUserSchema(c *Context, r *jreader.Reader, usingEventFormat bool
 	return c.Err()
 }
 
+func isOldUserCustomAttributeNameAllowed(name string) bool {
+	// If we see any of these names within the "custom": {} object in old-style user JSON, logically
+	// we can't use it because it would collide with a top-level property.
+	switch name {
+	case ldattr.KindAttr, ldattr.KeyAttr, ldattr.NameAttr, ldattr.AnonymousAttr, jsonPropMeta:
+		return false
+	default:
+		return true
+	}
+}
+
 // internAttributeNameIfPossible takes a byte slice representing a property name, and returns an existing
 // string if we already have a string literal equal to that name; otherwise it converts the bytes to a string.
 //