Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix baggage.NewMember to decode the accepted value #3226

Merged
merged 16 commits into from Oct 18, 2022
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Expand Up @@ -24,6 +24,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm

- Updated go.mods to point to valid versions of the sdk. (#3216)
- Set the `MeterProvider` resource on all exported metric data. (#3218)
- Fix function `baggage.NewMember` to decode the `value` parameter instead of directly use it according to the W3C specification. (#3144)
MrAlias marked this conversation as resolved.
Show resolved Hide resolved

## [0.32.0] Revised Metric SDK (Alpha) - 2022-09-18

Expand Down
10 changes: 9 additions & 1 deletion baggage/baggage.go
Expand Up @@ -263,7 +263,15 @@ func NewMember(key, value string, props ...Property) (Member, error) {
if err := m.validate(); err != nil {
MrAlias marked this conversation as resolved.
Show resolved Hide resolved
return newInvalidMember(), err
}

// the accepted value of this function should be decoded
// according to the W3C Baggage value specification
// e.g. value "%3B" should be treated as its decoded form ";"
decodedValue, err := url.QueryUnescape(value)
if err != nil {
return newInvalidMember(),
fmt.Errorf("%w: %q", errInvalidValue, value)
MrAlias marked this conversation as resolved.
Show resolved Hide resolved
}
m.value = decodedValue
return m, nil
}

Expand Down
28 changes: 28 additions & 0 deletions baggage/baggage_test.go
Expand Up @@ -768,6 +768,23 @@ func TestNewMember(t *testing.T) {
}
assert.Equal(t, expected, m)

// wrong value with wrong decoding
val = "%zzzzz"
_, err = NewMember(key, val, p)
assert.ErrorIs(t, err, errInvalidValue)

// value should be decoded
val = "%3B"
m, err = NewMember(key, val, p)
expected = Member{
key: key,
value: ";",
properties: properties{{key: "foo", hasData: true}},
hasData: true,
}
assert.NoError(t, err)
assert.Equal(t, expected, m)

// Ensure new member is immutable.
p.key = "bar"
assert.Equal(t, expected, m)
Expand All @@ -784,6 +801,17 @@ func TestPropertiesValidate(t *testing.T) {
assert.NoError(t, p.validate())
}

func TestMemberString(t *testing.T) {
// normal key value pair
member, _ := NewMember("key", "value")
memberStr := member.String()
assert.Equal(t, memberStr, "key=value")
// encoded key
member, _ = NewMember("key", "%3B")
memberStr = member.String()
assert.Equal(t, memberStr, "key=%3B")
}

var benchBaggage Baggage

func BenchmarkNew(b *testing.B) {
Expand Down
5 changes: 3 additions & 2 deletions propagation/baggage_test.go
Expand Up @@ -17,6 +17,7 @@ package propagation_test
import (
"context"
"net/http"
"net/url"
"strings"
"testing"

Expand Down Expand Up @@ -46,8 +47,8 @@ func (m member) Member(t *testing.T) baggage.Member {
}
props = append(props, p)
}

bMember, err := baggage.NewMember(m.Key, m.Value, props...)
// when creating a new Member, NewMember only accepted encoded string as value
MrAlias marked this conversation as resolved.
Show resolved Hide resolved
bMember, err := baggage.NewMember(m.Key, url.QueryEscape(m.Value), props...)
if err != nil {
t.Fatal(err)
}
Expand Down