baggage: Add NewMemberRaw and NewKeyValuePropertyRaw

CHANGELOG.md

@@ -21,6 +21,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 - Add `WithResourceAsConstantLabels` option to apply resource attributes for every metric emitted by the Prometheus exporter. (#4733)
 - Experimental cardinality limiting is added to the metric SDK.
   See [metric documentation](./sdk/metric/EXPERIMENTAL.md#cardinality-limit) for more information about this feature and how to enable it. (#4457)
+- Add `NewMemberRaw` and `NewKeyValuePropertyRaw` in `go.opentelemetry.io/otel/baggage`. (#4804)
 
 ### Changed
 
@@ -31,12 +32,14 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 - Improve `go.opentelemetry.io/otel/baggage` performance. (#4743)
 - Improve performance of the `(*Set).Filter` method in `go.opentelemetry.io/otel/attribute` when the passed filter does not filter out any attributes from the set. (#4774)
 - `Member.String` in `go.opentelemetry.io/otel/baggage` percent-encodes only when necessary. (#4775)
+- `Property.Value` in `go.opentelemetry.io/otel/baggage` now returns a vanilla string instead of a percent-encoded value. (#4804)
 
 ### Fixed
 
 - Fix `Parse` in `go.opentelemetry.io/otel/baggage` to validate member value before percent-decoding. (#4755)
 - Fix whitespace encoding of `Member.String` in `go.opentelemetry.io/otel/baggage`. (#4756)
 - Fix baggage item key so that it is not canonicalized in `go.opentelemetry.io/otel/bridge/opentracing`. (#4776)
+- Fix `go.opentelemetry.io/otel/bridge/opentracing` to properly handle baggage values that requires escaping during propagation. (#4804)
 
 ## [1.21.0/0.44.0] 2023-11-16
 

baggage/baggage.go

@@ -66,14 +66,29 @@
 
 // NewKeyValueProperty returns a new Property for key with value.
 //
-// If key or value are invalid, an error will be returned.
+// The passed key must be compliant with W3C Baggage specification.
+// The passed value must be precent-encoded as defined in W3C Baggage specification.
+//
+// Notice: Consider using [NewKeyValuePropertyRaw] instead
+// that does not require precent-encoding of the value.
 func NewKeyValueProperty(key, value string) (Property, error) {
-	if !validateKey(key) {
-		return newInvalidProperty(), fmt.Errorf("%w: %q", errInvalidKey, key)
-	}
 	if !validateValue(value) {
 		return newInvalidProperty(), fmt.Errorf("%w: %q", errInvalidValue, value)
 	}
+	decodedValue, err := url.PathUnescape(value)
+	if err != nil {
+		return newInvalidProperty(), fmt.Errorf("%w: %q", errInvalidValue, value)
+	}
+	return NewKeyValuePropertyRaw(key, decodedValue)
+}
+
+// NewKeyValuePropertyRaw returns a new Property for key with value.
+//
+// The passed key must be compliant with W3C Baggage specification.
+func NewKeyValuePropertyRaw(key, value string) (Property, error) {
+	if !validateKey(key) {
+		return newInvalidProperty(), fmt.Errorf("%w: %q", errInvalidKey, key)
+	}
 
 	p := Property{
 		key:      key,
@@ -113,9 +128,6 @@
 	if !validateKey(p.key) {
 		return errFunc(fmt.Errorf("%w: %q", errInvalidKey, p.key))
 	}
-	if p.hasValue && !validateValue(p.value) {
-		return errFunc(fmt.Errorf("%w: %q", errInvalidValue, p.value))
-	}
 	if !p.hasValue && p.value != "" {
 		return errFunc(errors.New("inconsistent value"))
 	}
@@ -134,11 +146,11 @@
 	return p.value, p.hasValue
 }
 
-// String encodes Property into a string compliant with the W3C Baggage
+// String encodes Property into a header string compliant with the W3C Baggage
 // specification.
 func (p Property) String() string {
 	if p.hasValue {
-		return fmt.Sprintf("%s%s%v", p.key, keyValueDelimiter, p.value)
+		return fmt.Sprintf("%s%s%v", p.key, keyValueDelimiter, valueEscape(p.value))
 	}
 	return p.key
 }
@@ -198,7 +210,7 @@
 	return nil
 }
 
-// String encodes properties into a string compliant with the W3C Baggage
+// String encodes properties into a header string compliant with the W3C Baggage
 // specification.
 func (p properties) String() string {
 	props := make([]string, len(p))
@@ -220,11 +232,28 @@
 	hasData bool
 }
 
-// NewMember returns a new Member from the passed arguments. The key will be
-// used directly while the value will be url decoded after validation. An error
-// is returned if the created Member would be invalid according to the W3C
-// Baggage specification.
+// NewMemberRaw returns a new Member from the passed arguments.
+//
+// The passed key must be compliant with W3C Baggage specification.
+// The passed value must be precent-encoded as defined in W3C Baggage specification.
+//
+// Notice: Consider using [NewMemberRaw] instead
+// that does not require precent-encoding of the value.
 func NewMember(key, value string, props ...Property) (Member, error) {
+	if !validateValue(value) {
+		return newInvalidMember(), fmt.Errorf("%w: %q", errInvalidValue, value)
+	}
+	decodedValue, err := url.PathUnescape(value)
+	if err != nil {
+		return newInvalidMember(), fmt.Errorf("%w: %q", errInvalidValue, value)
+	}
+	return NewMemberRaw(key, decodedValue, props...)
+}
+
+// NewMemberRaw returns a new Member from the passed arguments.
+//
+// The passed key must be compliant with W3C Baggage specification.
+func NewMemberRaw(key, value string, props ...Property) (Member, error) {
 	m := Member{
 		key:        key,
 		value:      value,
@@ -234,11 +263,6 @@
 	if err := m.validate(); err != nil {
 		return newInvalidMember(), err
 	}
-	decodedValue, err := url.PathUnescape(value)
-	if err != nil {
-		return newInvalidMember(), fmt.Errorf("%w: %q", errInvalidValue, value)
-	}
-	m.value = decodedValue
 	return m, nil
 }
 
@@ -284,6 +308,8 @@
 	if !validateValue(val) {
 		return newInvalidMember(), fmt.Errorf("%w: %q", errInvalidValue, v)
 	}
+
+	// Decode a precent-encoded value.
 	value, err := url.PathUnescape(val)
 	if err != nil {
 		return newInvalidMember(), fmt.Errorf("%w: %v", errInvalidValue, err)
@@ -292,8 +318,7 @@
 }
 
 // validate ensures m conforms to the W3C Baggage specification.
-// A key is just an ASCII string, but a value must be URL encoded UTF-8,
-// returning an error otherwise.
+// A key must be an ASCII string, returning an error otherwise.
 func (m Member) validate() error {
 	if !m.hasData {
 		return fmt.Errorf("%w: %q", errInvalidMember, m)
@@ -302,9 +327,6 @@
 	if !validateKey(m.key) {
 		return fmt.Errorf("%w: %q", errInvalidKey, m.key)
 	}
-	if !validateValue(m.value) {
-		return fmt.Errorf("%w: %q", errInvalidValue, m.value)
-	}
 	return m.properties.validate()
 }
 
@@ -317,7 +339,7 @@
 // Properties returns a copy of the Member properties.
 func (m Member) Properties() []Property { return m.properties.Copy() }
 
-// String encodes Member into a string compliant with the W3C Baggage
+// String encodes Member into a header string compliant with the W3C Baggage
 // specification.
 func (m Member) String() string {
 	// A key is just an ASCII string. A value is restricted to be
@@ -514,9 +536,8 @@
 	return len(b.list)
 }
 
-// String encodes Baggage into a string compliant with the W3C Baggage
-// specification. The returned string will be invalid if the Baggage contains
-// any invalid list-members.
+// String encodes Baggage into a header string compliant with the W3C Baggage
+// specification.
 func (b Baggage) String() string {
 	members := make([]string, 0, len(b.list))
 	for k, v := range b.list {
@@ -595,10 +616,17 @@
 		return
 	}
 
+	// Decode a precent-encoded value.
+	value, err := url.PathUnescape(s[valueStart:valueEnd])
+	if err != nil {
+		return
+	}
+
 	ok = true
 	p.key = s[keyStart:keyEnd]
 	p.hasValue = true
-	p.value = s[valueStart:valueEnd]
+
+	p.value = value
 	return
 }
 

baggage/baggage_test.go

@@ -143,7 +143,7 @@ func TestNewKeyProperty(t *testing.T) {
 }
 
 func TestNewKeyValueProperty(t *testing.T) {
-	p, err := NewKeyValueProperty(" ", "")
+	p, err := NewKeyValueProperty(" ", "value")
 	assert.ErrorIs(t, err, errInvalidKey)
 	assert.Equal(t, Property{}, p)
 
@@ -156,20 +156,27 @@ func TestNewKeyValueProperty(t *testing.T) {
 	assert.Equal(t, Property{key: "key", value: "value", hasValue: true}, p)
 }
 
+func TestNewKeyValuePropertyRaw(t *testing.T) {
+	p, err := NewKeyValuePropertyRaw(" ", "")
+	assert.ErrorIs(t, err, errInvalidKey)
+	assert.Equal(t, Property{}, p)
+
+	p, err = NewKeyValuePropertyRaw("key", "Witaj Świecie!")
+	assert.NoError(t, err)
+	assert.Equal(t, Property{key: "key", value: "Witaj Świecie!", hasValue: true}, p)
+}
+
 func TestPropertyValidate(t *testing.T) {
 	p := Property{}
 	assert.ErrorIs(t, p.validate(), errInvalidKey)
 
 	p.key = "k"
 	assert.NoError(t, p.validate())
 
-	p.value = ";"
+	p.value = "v"
 	assert.EqualError(t, p.validate(), "invalid property: inconsistent value")
 
 	p.hasValue = true
-	assert.ErrorIs(t, p.validate(), errInvalidValue)
-
-	p.value = "v"
 	assert.NoError(t, p.validate())
 }
 
@@ -397,6 +404,15 @@ func TestBaggageParse(t *testing.T) {
 				"key1": {Value: "val%2,"},
 			},
 		},
+		{
+			name: "encoded property",
+			in:   "key1=;bar=val%252%2C",
+			want: baggage.List{
+				"key1": {
+					Properties: []baggage.Property{{Key: "bar", HasValue: true, Value: "val%2,"}},
+				},
+			},
+		},
 		{
 			name: "encoded UTF-8 string",
 			in:   "foo=%C4%85%C5%9B%C4%87",
@@ -528,6 +544,17 @@ func TestBaggageString(t *testing.T) {
 				"foo": {Value: "ąść"},
 			},
 		},
+		{
+			name: "Encoded property value",
+			out:  "foo=;bar=%20",
+			baggage: baggage.List{
+				"foo": {
+					Properties: []baggage.Property{
+						{Key: "bar", Value: " ", HasValue: true},
+					},
+				},
+			},
+		},
 		{
 			name: "plus",
 			out:  "foo=1+1",
@@ -846,9 +873,6 @@ func TestMemberValidation(t *testing.T) {
 	assert.ErrorIs(t, m.validate(), errInvalidKey)
 
 	m.key, m.value = "k", "\\"
-	assert.ErrorIs(t, m.validate(), errInvalidValue)
-
-	m.value = "v"
 	assert.NoError(t, m.validate())
 }
 
@@ -891,6 +915,28 @@ func TestNewMember(t *testing.T) {
 	assert.Equal(t, expected, m)
 }
 
+func TestNewMemberRaw(t *testing.T) {
+	m, err := NewMemberRaw("", "")
+	assert.ErrorIs(t, err, errInvalidKey)
+	assert.Equal(t, Member{hasData: false}, m)
+
+	key, val := "k", "v"
+	p := Property{key: "foo"}
+	m, err = NewMemberRaw(key, val, p)
+	assert.NoError(t, err)
+	expected := Member{
+		key:        key,
+		value:      val,
+		properties: properties{{key: "foo"}},
+		hasData:    true,
+	}
+	assert.Equal(t, expected, m)
+
+	// Ensure new member is immutable.
+	p.key = "bar"
+	assert.Equal(t, expected, m)
+}
+
 func TestPropertiesValidate(t *testing.T) {
 	p := properties{{}}
 	assert.ErrorIs(t, p.validate(), errInvalidKey)
@@ -904,22 +950,23 @@ func TestPropertiesValidate(t *testing.T) {
 
 func TestMemberString(t *testing.T) {
 	// normal key value pair
-	member, _ := NewMember("key", "value")
+	member, _ := NewMemberRaw("key", "value")
 	memberStr := member.String()
 	assert.Equal(t, memberStr, "key=value")
-	// encoded key
-	member, _ = NewMember("key", "%3B%20")
+
+	// encoded value
+	member, _ = NewMemberRaw("key", "; ")
 	memberStr = member.String()
 	assert.Equal(t, memberStr, "key=%3B%20")
 }
 
 var benchBaggage Baggage
 
 func BenchmarkNew(b *testing.B) {
-	mem1, _ := NewMember("key1", "val1")
-	mem2, _ := NewMember("key2", "val2")
-	mem3, _ := NewMember("key3", "val3")
-	mem4, _ := NewMember("key4", "val4")
+	mem1, _ := NewMemberRaw("key1", "val1")
+	mem2, _ := NewMemberRaw("key2", "val2")
+	mem3, _ := NewMemberRaw("key3", "val3")
+	mem4, _ := NewMemberRaw("key4", "val4")
 
 	b.ReportAllocs()
 	b.ResetTimer()
@@ -931,11 +978,11 @@ func BenchmarkNew(b *testing.B) {
 
 var benchMember Member
 
-func BenchmarkNewMember(b *testing.B) {
+func BenchmarkNewMemberRaw(b *testing.B) {
 	b.ReportAllocs()
 
 	for i := 0; i < b.N; i++ {
-		benchMember, _ = NewMember("key", "value")
+		benchMember, _ = NewMemberRaw("key", "value")
 	}
 }
 
@@ -950,7 +997,7 @@ func BenchmarkParse(b *testing.B) {
 func BenchmarkString(b *testing.B) {
 	var members []Member
 	addMember := func(k, v string) {
-		m, err := NewMember(k, valueEscape(v))
+		m, err := NewMemberRaw(k, valueEscape(v))
 		require.NoError(b, err)
 		members = append(members, m)
 	}

bridge/opentracing/bridge.go

@@ -73,7 +73,7 @@ func (c *bridgeSpanContext) ForeachBaggageItem(handler func(k, v string) bool) {
 }
 
 func (c *bridgeSpanContext) setBaggageItem(restrictedKey, value string) {
-	m, err := baggage.NewMember(restrictedKey, value)
+	m, err := baggage.NewMemberRaw(restrictedKey, value)
 	if err != nil {
 		return
 	}