Skip to content

Commit

Permalink
[confmap] Remove original representation if invalid
Browse files Browse the repository at this point in the history
  • Loading branch information
mx-psi committed Aug 2, 2024
1 parent 2beed98 commit 489bdb5
Show file tree
Hide file tree
Showing 5 changed files with 55 additions and 21 deletions.
25 changes: 25 additions & 0 deletions .chloggen/mx-psi_remove-has-original.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: Remove original string representation if invalid.

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

# (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: []
3 changes: 0 additions & 3 deletions confmap/confmap.go
Original file line number Diff line number Diff line change
Expand Up @@ -240,9 +240,6 @@ func caseSensitiveMatchName(a, b string) bool {
func castTo(exp expandedValue, useOriginal bool) (any, error) {
// If the target field is a string, use `exp.Original` or fail if not available.
if globalgates.StrictlyTypedInputGate.IsEnabled() && useOriginal {
if !exp.HasOriginal {
return nil, fmt.Errorf("cannot expand value to string: original value not set")
}
return exp.Original, nil
}
// Otherwise, use the parsed value (previous behavior).
Expand Down
5 changes: 2 additions & 3 deletions confmap/confmap_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -849,9 +849,8 @@ func TestRecursiveUnmarshaling(t *testing.T) {
func TestExpandedValue(t *testing.T) {
cm := NewFromStringMap(map[string]any{
"key": expandedValue{
Value: 0xdeadbeef,
HasOriginal: true,
Original: "original",
Value: 0xdeadbeef,
Original: "original",
}})
assert.Equal(t, 0xdeadbeef, cm.Get("key"))
assert.Equal(t, map[string]any{"key": 0xdeadbeef}, cm.ToStringMap())
Expand Down
27 changes: 12 additions & 15 deletions confmap/expand.go
Original file line number Diff line number Diff line change
Expand Up @@ -58,22 +58,21 @@ func (mr *Resolver) expandValue(ctx context.Context, value any) (any, bool, erro
// At this point we don't know the target field type, so we need to expand the original representation as well.
originalExpanded, originalChanged, err := mr.expandValue(ctx, v.Original)
if err != nil {
return nil, false, err
// The original representation is not valid, return the expanded value.
return expanded, changed, nil

Check warning on line 62 in confmap/expand.go

View check run for this annotation

Codecov / codecov/patch

confmap/expand.go#L62

Added line #L62 was not covered by tests
}

if originalExpanded, ok := originalExpanded.(string); ok {
// If the original representation is a string, return the expanded value with the original representation.
return expandedValue{
Value: expanded,
Original: originalExpanded,
HasOriginal: true,
Value: expanded,
Original: originalExpanded,

Check warning on line 69 in confmap/expand.go

View check run for this annotation

Codecov / codecov/patch

confmap/expand.go#L68-L69

Added lines #L68 - L69 were not covered by tests
}, changed || originalChanged, nil
}

result := expandedValue{
Value: expanded,
Original: v.Original,
HasOriginal: v.HasOriginal,
Value: expanded,
Original: v.Original,

Check warning on line 75 in confmap/expand.go

View check run for this annotation

Codecov / codecov/patch

confmap/expand.go#L74-L75

Added lines #L74 - L75 were not covered by tests
}

return result, changed || originalChanged, nil
Expand Down Expand Up @@ -158,10 +157,7 @@ func (mr *Resolver) findURI(input string) string {
type expandedValue struct {
// Value is the expanded value.
Value any
// HasOriginal is true if the original representation is set.
HasOriginal bool
// Original is the original representation of the value.
// It is only valid if HasOriginal is true.
Original string
}

Expand All @@ -182,18 +178,19 @@ func (mr *Resolver) findAndExpandURI(ctx context.Context, input string) (any, bo
return input, false, err
}

expanded := expandedValue{}
expanded.Value, err = ret.AsRaw()
val, err := ret.AsRaw()
if err != nil {
return input, false, err
}

if asStr, err2 := ret.AsString(); err2 == nil {
expanded.HasOriginal = true
expanded.Original = asStr
return expandedValue{
Value: val,
Original: asStr,
}, true, nil
}

return expanded, true, err
return val, true, nil
}
expanded, err := mr.expandURI(ctx, uri)
if err != nil {
Expand Down
16 changes: 16 additions & 0 deletions confmap/internal/e2e/types_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -345,6 +345,22 @@ func TestStrictTypeCasting(t *testing.T) {
targetField: TargetFieldInlineString,
expected: "inline field with 2006-01-02T15:04:05Z07:00 expansion",
},
// issue 10787
{
value: "true # comment with a ${env:hello.world} reference",
targetField: TargetFieldBool,
expected: true,
},
{
value: "true # comment with a ${env:hello.world} reference",
targetField: TargetFieldString,
unmarshalErr: `expected type 'string', got unconvertible type 'bool'`,
},
{
value: "true # comment with a ${env:hello.world} reference",
targetField: TargetFieldInlineString,
resolveErr: `environment variable "hello.world" has invalid name`,
},
}

previousValue := globalgates.StrictlyTypedInputGate.IsEnabled()
Expand Down

0 comments on commit 489bdb5

Please sign in to comment.