Skip to content

Commit

Permalink
[confmap] Encode string-like map keys (open-telemetry#10137)
Browse files Browse the repository at this point in the history
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description

Our check for determining whether a map key can be encoded as a string
is too restrictive and doesn't into account types which are essentially
aliases for the string type and don't implement `UnmarshalText`.

I encountered this while trying to call
`(confmap.Conf).Marshal(*otelcol.Config)` when the config includes a
Prometheus receiver, which includes the [LabelName
type](https://github.com/prometheus/common/blob/main/model/labels.go#L98)
that does not implement an unmarshaling method. We can't guarantee that
all types will implement this, and [Go's print formatters
check](https://github.com/golang/go/blob/master/src/fmt/print.go#L803)
whether `(reflect.Value).Kind()` equals `reflect.String`, so I think
this will be an overall more robust approach.

<!--Describe what testing was performed and which tests were added.-->
#### Testing

I added two test cases to demonstrate when we will hit this case.

---------

Co-authored-by: Pablo Baeyens <pbaeyens31+github@gmail.com>
  • Loading branch information
2 people authored and andrzej-stencel committed May 27, 2024
1 parent 54d6b6b commit 6044416
Show file tree
Hide file tree
Showing 3 changed files with 54 additions and 4 deletions.
25 changes: 25 additions & 0 deletions .chloggen/string-encoding.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
# Use this changelog template to create an entry for release notes.

# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
change_type: bug_fix

# The name of the component, or a single word describing the area of concern, (e.g. otlpreceiver)
component: confmap

# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
note: Support string-like types as map keys when marshaling

# One or more tracking issues or pull requests related to the change
issues: [10137]

# (Optional) One or more lines of additional information to render under the primary note.
# These lines will be padded with 2 spaces and then inserted directly into the document.
# Use pipe (|) for multiline entries.
subtext:

# Optional: The change log or logs in which this entry should be included.
# e.g. '[user]' or '[user, api]'
# Include 'user' if the change is relevant to end users.
# Include 'api' if there is a change to a library API.
# Default: '[user]'
change_logs: [api]
15 changes: 11 additions & 4 deletions confmap/internal/mapstructure/encoder.go
Original file line number Diff line number Diff line change
Expand Up @@ -168,11 +168,18 @@ func (e *Encoder) encodeMap(value reflect.Value) (any, error) {
if err != nil {
return nil, fmt.Errorf("error encoding key: %w", err)
}
key, ok := encoded.(string)
if !ok {
return nil, fmt.Errorf("%w key %q, kind: %v", errNonStringEncodedKey, iterator.Key().Interface(), iterator.Key().Kind())

v := reflect.ValueOf(encoded)
var key string

switch v.Kind() {
case reflect.String:
key = v.String()
default:
return nil, fmt.Errorf("%w, key: %q, kind: %v, type: %T", errNonStringEncodedKey, iterator.Key().Interface(), iterator.Key().Kind(), encoded)
}
if _, ok = result[key]; ok {

if _, ok := result[key]; ok {
return nil, fmt.Errorf("duplicate key %q while encoding", key)
}
if result[key], err = e.encode(iterator.Value()); err != nil {
Expand Down
18 changes: 18 additions & 0 deletions confmap/internal/mapstructure/encoder_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,8 @@ func (tID TestID) MarshalText() (text []byte, err error) {
return []byte(out), nil
}

type TestStringLike string

func TestEncode(t *testing.T) {
enc := New(&EncoderConfig{
EncodeHook: TextMarshalerHookFunc(),
Expand All @@ -63,6 +65,22 @@ func TestEncode(t *testing.T) {
input: TestID("type"),
want: "type_",
},
"MapWithTextMarshalerKey": {
input: map[TestID]TestSimpleStruct{
TestID("type"): {Value: "value"},
},
want: map[string]any{
"type_": map[string]any{"value": "value"},
},
},
"MapWithoutTextMarshalerKey": {
input: map[TestStringLike]TestSimpleStruct{
TestStringLike("key"): {Value: "value"},
},
want: map[string]any{
"key": map[string]any{"value": "value"},
},
},
"WithSlice": {
input: []TestID{
TestID("nop"),
Expand Down

0 comments on commit 6044416

Please sign in to comment.