[confmap] Remove original representation if invalid (#10795)

<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description

<!-- Issue number if applicable -->

Removes original string representation if invalid.

#### Link to tracking issue

Fixes #10787

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

<!--Describe the documentation added.-->

Added e2e test cases

.chloggen/mx-psi_remove-has-original.yaml

@@ -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: []

confmap/confmap.go

@@ -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).

confmap/confmap_test.go

@@ -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())

confmap/expand.go

@@ -58,25 +58,19 @@ 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
 		}
 
 		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,
 			}, changed || originalChanged, nil
 		}
 
-		result := expandedValue{
-			Value:       expanded,
-			Original:    v.Original,
-			HasOriginal: v.HasOriginal,
-		}
-
-		return result, changed || originalChanged, nil
+		return expanded, changed, nil
 	case string:
 		if !strings.Contains(v, "${") || !strings.Contains(v, "}") {
 			// No URIs to expand.
@@ -158,10 +152,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
 }
 
@@ -182,18 +173,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 {

confmap/internal/e2e/testdata/issue-10787-main.yaml

@@ -0,0 +1,22 @@
+receivers:
+  otlp:
+    protocols:
+      grpc:
+        endpoint: 0.0.0.0:4317
+      http:
+        endpoint: 0.0.0.0:4318
+processors:
+  batch:
+
+exporters:
+  ${file:testdata/issue-10787-snippet.yaml}
+
+service:
+  telemetry:
+    metrics:
+      level: detailed
+  pipelines:
+    traces:
+      receivers: [otlp]
+      processors: [batch]
+      exporters: [logging]

confmap/internal/e2e/testdata/issue-10787-snippet.yaml

@@ -0,0 +1,3 @@
+# ${hello.world}
+logging:
+  verbosity: detailed

confmap/internal/e2e/types_test.go

@@ -8,6 +8,7 @@ import (
 	"path/filepath"
 	"testing"
 
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
 	"go.opentelemetry.io/collector/confmap"
@@ -24,6 +25,7 @@ const (
 	TargetFieldString       TargetField = "string_field"
 	TargetFieldBool         TargetField = "bool_field"
 	TargetFieldInlineString TargetField = "inline_string_field"
+	TargetFieldSlice        TargetField = "slice_field"
 )
 
 type Test struct {
@@ -78,6 +80,9 @@ func AssertResolvesTo(t *testing.T, resolver *confmap.Resolver, tt Test) {
 	case TargetFieldBool:
 		var cfg TargetConfig[bool]
 		AssertExpectedMatch(t, tt, conf, &cfg)
+	case TargetFieldSlice:
+		var cfg TargetConfig[[]any]
+		AssertExpectedMatch(t, tt, conf, &cfg)
 	default:
 		t.Fatalf("unexpected target field %q", tt.targetField)
 	}
@@ -345,6 +350,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`,
+		},
 		// issue 10759
 		{
 			value:       `["a",`,
@@ -356,6 +377,12 @@ func TestStrictTypeCasting(t *testing.T) {
 			targetField: TargetFieldInlineString,
 			expected:    `inline field with ["a", expansion`,
 		},
+		// issue 10799
+		{
+			value:       `[filelog,windowseventlog/application]`,
+			targetField: TargetFieldSlice,
+			expected:    []any{"filelog", "windowseventlog/application"},
+		},
 	}
 
 	previousValue := globalgates.StrictlyTypedInputGate.IsEnabled()
@@ -494,3 +521,106 @@ func TestRecursiveMaps(t *testing.T) {
 		cfgStr.Field,
 	)
 }
+
+// Test that comments with invalid ${env:...} references do not prevent configuration from loading.
+func TestIssue10787(t *testing.T) {
+	previousValue := globalgates.StrictlyTypedInputGate.IsEnabled()
+	err := featuregate.GlobalRegistry().Set(globalgates.StrictlyTypedInputID, true)
+	require.NoError(t, err)
+	defer func() {
+		seterr := featuregate.GlobalRegistry().Set(globalgates.StrictlyTypedInputID, previousValue)
+		require.NoError(t, seterr)
+	}()
+
+	resolver := NewResolver(t, "issue-10787-main.yaml")
+	conf, err := resolver.Resolve(context.Background())
+	require.NoError(t, err)
+	assert.Equal(t, conf.ToStringMap(),
+		map[string]any{
+			"exporters": map[string]any{
+				"logging": map[string]any{
+					"verbosity": "detailed",
+				},
+			},
+			"processors": map[string]any{
+				"batch": nil,
+			},
+			"receivers": map[string]any{
+				"otlp": map[string]any{
+					"protocols": map[string]any{
+						"grpc": map[string]any{
+							"endpoint": "0.0.0.0:4317",
+						},
+						"http": map[string]any{
+							"endpoint": "0.0.0.0:4318",
+						},
+					},
+				},
+			},
+			"service": map[string]any{
+				"pipelines": map[string]any{
+					"traces": map[string]any{
+						"exporters":  []any{"logging"},
+						"processors": []any{"batch"},
+						"receivers":  []any{"otlp"},
+					},
+				},
+				"telemetry": map[string]any{
+					"metrics": map[string]any{
+						"level": "detailed",
+					},
+				},
+			},
+		},
+	)
+}
+
+func TestStructMappingIssue10787(t *testing.T) {
+	previousValue := globalgates.StrictlyTypedInputGate.IsEnabled()
+	err := featuregate.GlobalRegistry().Set(globalgates.StrictlyTypedInputID, true)
+	require.NoError(t, err)
+	defer func() {
+		seterr := featuregate.GlobalRegistry().Set(globalgates.StrictlyTypedInputID, previousValue)
+		require.NoError(t, seterr)
+	}()
+
+	resolver := NewResolver(t, "types_expand.yaml")
+	t.Setenv("ENV", `# ${hello.world}
+logging:
+  verbosity: detailed`)
+	conf, err := resolver.Resolve(context.Background())
+	require.NoError(t, err)
+
+	type Logging struct {
+		Verbosity string `mapstructure:"verbosity"`
+	}
+	type Exporters struct {
+		Logging Logging `mapstructure:"logging"`
+	}
+	type Target struct {
+		Field Exporters `mapstructure:"field"`
+	}
+
+	var cfg Target
+	err = conf.Unmarshal(&cfg)
+	require.NoError(t, err)
+	require.Equal(t,
+		Target{Field: Exporters{
+			Logging: Logging{
+				Verbosity: "detailed",
+			},
+		}},
+		cfg,
+	)
+
+	confStr, err := resolver.Resolve(context.Background())
+	require.NoError(t, err)
+	var cfgStr TargetConfig[string]
+	err = confStr.Unmarshal(&cfgStr)
+	require.NoError(t, err)
+	require.Equal(t, `# ${hello.world}
+logging:
+  verbosity: detailed`,
+		cfgStr.Field,
+	)
+}