Skip to content

Commit a6c0484

Browse files
a-hilalya-hilaly
committed
fix: correct expression unwrapping to preserve trailing braces
Fix bug where `strings.Trim(expr, "${}")` treated `"${}"` as a cutset, removing any `$`, `{`, or `}` characters from both ends of expressions. This corrupted expressions ending with braces like `"condition ? value : {}"`, commonly used in ternary operators with empty map literals.
1 parent 92d096b commit a6c0484

File tree

7 files changed

+289
-6
lines changed

7 files changed

+289
-6
lines changed

‎pkg/graph/parser/cel_test.go‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -167,6 +167,30 @@ func TestExtractExpressions(t *testing.T) {
167167
want: []string{},
168168
wantErr: true,
169169
},
170+
{
171+
name: "Ternary with empty map literal",
172+
input: "${condition ? value : {}}",
173+
want: []string{"condition ? value : {}"},
174+
wantErr: false,
175+
},
176+
{
177+
name: "Ternary with empty map on both sides",
178+
input: "${condition ? {} : {}}",
179+
want: []string{"condition ? {} : {}"},
180+
wantErr: false,
181+
},
182+
{
183+
name: "Complex ternary with empty map (real world example)",
184+
input: "${schema.spec.deployment.includeAnnotations ? schema.spec.deployment.annotations : {}}",
185+
want: []string{"schema.spec.deployment.includeAnnotations ? schema.spec.deployment.annotations : {}"},
186+
wantErr: false,
187+
},
188+
{
189+
name: "Ternary with has() and empty map",
190+
input: "${has(schema.annotations) && includeAnnotations ? schema.annotations : {}}",
191+
want: []string{"has(schema.annotations) && includeAnnotations ? schema.annotations : {}"},
192+
wantErr: false,
193+
},
170194
}
171195

172196
for _, tt := range tests {

‎pkg/graph/parser/conditions.go‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,9 @@ func ParseConditionExpressions(conditions []string) ([]string, error) {
3838
if !ok {
3939
return nil, fmt.Errorf("only standalone expressions are allowed")
4040
}
41-
expressions = append(expressions, strings.Trim(e, "${}"))
41+
expr := strings.TrimPrefix(e, "${")
42+
expr = strings.TrimSuffix(expr, "}")
43+
expressions = append(expressions, expr)
4244
}
4345

4446
return expressions, nil

‎pkg/graph/parser/parser.go‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -278,8 +278,10 @@ func parseString(field string, schema *spec.Schema, path string, expectedTypes [
278278
if ok {
279279
// For CEL expressions, get the CEL type from the schema
280280
expectedType := getCelType(schema)
281+
expr := strings.TrimPrefix(field, "${")
282+
expr = strings.TrimSuffix(expr, "}")
281283
return []variable.FieldDescriptor{{
282-
Expressions: []string{strings.Trim(field, "${}")},
284+
Expressions: []string{expr},
283285
ExpectedType: expectedType,
284286
Path: path,
285287
StandaloneExpression: true,

‎pkg/graph/parser/parser_test.go‎

Lines changed: 125 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1645,3 +1645,128 @@ func TestCollectTypesFromSubSchemas(t *testing.T) {
16451645
})
16461646
}
16471647
}
1648+
1649+
// TestEmptyBracesInExpressions tests the regression where strings.Trim() was
1650+
// incorrectly stripping {} from expressions. This bug affected ternary expressions
1651+
// with empty map literals like: condition ? value : {}
1652+
func TestEmptyBracesInExpressions(t *testing.T) {
1653+
testCases := []struct {
1654+
name string
1655+
resource map[string]interface{}
1656+
schema *spec.Schema
1657+
expectedExprPath string // Path where we expect to find the expression
1658+
expectedExpr string // The exact expression we expect (without ${})
1659+
}{
1660+
{
1661+
name: "Ternary with empty map literal",
1662+
resource: map[string]interface{}{
1663+
"annotations": "${includeAnnotations ? annotations : {}}",
1664+
},
1665+
schema: &spec.Schema{
1666+
SchemaProps: spec.SchemaProps{
1667+
Type: []string{"object"},
1668+
Properties: map[string]spec.Schema{
1669+
"annotations": {
1670+
SchemaProps: spec.SchemaProps{
1671+
Type: []string{"object"},
1672+
AdditionalProperties: &spec.SchemaOrBool{
1673+
Allows: true,
1674+
Schema: &spec.Schema{
1675+
SchemaProps: spec.SchemaProps{Type: []string{"string"}},
1676+
},
1677+
},
1678+
},
1679+
},
1680+
},
1681+
},
1682+
},
1683+
expectedExprPath: "annotations",
1684+
expectedExpr: "includeAnnotations ? annotations : {}",
1685+
},
1686+
{
1687+
name: "Complex ternary with has() and empty map",
1688+
resource: map[string]interface{}{
1689+
"metadata": map[string]interface{}{
1690+
"annotations": "${has(schema.annotations) && includeAnnotations ? schema.annotations : {}}",
1691+
},
1692+
},
1693+
schema: &spec.Schema{
1694+
SchemaProps: spec.SchemaProps{
1695+
Type: []string{"object"},
1696+
Properties: map[string]spec.Schema{
1697+
"metadata": {
1698+
SchemaProps: spec.SchemaProps{
1699+
Type: []string{"object"},
1700+
Properties: map[string]spec.Schema{
1701+
"annotations": {
1702+
SchemaProps: spec.SchemaProps{
1703+
Type: []string{"object"},
1704+
AdditionalProperties: &spec.SchemaOrBool{
1705+
Allows: true,
1706+
Schema: &spec.Schema{
1707+
SchemaProps: spec.SchemaProps{Type: []string{"string"}},
1708+
},
1709+
},
1710+
},
1711+
},
1712+
},
1713+
},
1714+
},
1715+
},
1716+
},
1717+
},
1718+
expectedExprPath: "metadata.annotations",
1719+
expectedExpr: "has(schema.annotations) && includeAnnotations ? schema.annotations : {}",
1720+
},
1721+
{
1722+
name: "Ternary with empty maps on both sides",
1723+
resource: map[string]interface{}{
1724+
"config": "${condition ? {} : {}}",
1725+
},
1726+
schema: &spec.Schema{
1727+
SchemaProps: spec.SchemaProps{
1728+
Type: []string{"object"},
1729+
Properties: map[string]spec.Schema{
1730+
"config": {
1731+
SchemaProps: spec.SchemaProps{
1732+
Type: []string{"object"},
1733+
},
1734+
},
1735+
},
1736+
},
1737+
},
1738+
expectedExprPath: "config",
1739+
expectedExpr: "condition ? {} : {}",
1740+
},
1741+
}
1742+
1743+
for _, tc := range testCases {
1744+
t.Run(tc.name, func(t *testing.T) {
1745+
fields, err := ParseResource(tc.resource, tc.schema)
1746+
if err != nil {
1747+
t.Fatalf("ParseResource() error = %v", err)
1748+
}
1749+
1750+
// Find the field descriptor for the expected path
1751+
found := false
1752+
for _, field := range fields {
1753+
if field.Path == tc.expectedExprPath {
1754+
found = true
1755+
if len(field.Expressions) != 1 {
1756+
t.Errorf("Expected 1 expression, got %d", len(field.Expressions))
1757+
continue
1758+
}
1759+
if field.Expressions[0] != tc.expectedExpr {
1760+
t.Errorf("Expression mismatch:\ngot: %q\nwant: %q", field.Expressions[0], tc.expectedExpr)
1761+
}
1762+
if !field.StandaloneExpression {
1763+
t.Error("Expected StandaloneExpression to be true")
1764+
}
1765+
}
1766+
}
1767+
if !found {
1768+
t.Errorf("Expected to find field descriptor for path %q", tc.expectedExprPath)
1769+
}
1770+
})
1771+
}
1772+
}

‎pkg/graph/parser/schemaless.go‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -58,8 +58,10 @@ func parseSchemalessResource(resource interface{}, path string) ([]variable.Fiel
5858
return nil, err
5959
}
6060
if ok {
61+
expr := strings.TrimPrefix(field, "${")
62+
expr = strings.TrimSuffix(expr, "}")
6163
expressionsFields = append(expressionsFields, variable.FieldDescriptor{
62-
Expressions: []string{strings.Trim(field, "${}")},
64+
Expressions: []string{expr},
6365
ExpectedType: cel.DynType, // No schema, so we use dynamic type
6466
Path: path,
6567
StandaloneExpression: true,

‎pkg/runtime/resolver/resolver.go‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -104,7 +104,7 @@ func (r *Resolver) resolveField(field variable.FieldDescriptor) ResolutionResult
104104
}
105105

106106
if field.StandaloneExpression {
107-
resolvedValue, ok := r.data[strings.Trim(field.Expressions[0], "${}")]
107+
resolvedValue, ok := r.data[field.Expressions[0]]
108108
if !ok {
109109
result.Error = fmt.Errorf("no data provided for expression: %s", field.Expressions[0])
110110
return result
@@ -125,8 +125,7 @@ func (r *Resolver) resolveField(field variable.FieldDescriptor) ResolutionResult
125125

126126
replaced := strValue
127127
for _, expr := range field.Expressions {
128-
key := strings.Trim(expr, "${}")
129-
replacement, ok := r.data[key]
128+
replacement, ok := r.data[expr]
130129
if !ok {
131130
result.Error = fmt.Errorf("no data provided for expression: %s", expr)
132131
return result

‎pkg/runtime/resolver/resolver_test.go‎

Lines changed: 129 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -663,3 +663,132 @@ func TestResolver(t *testing.T) {
663663
assert.Equal(t, summary.ResolvedExpressions, 1)
664664
assert.Equal(t, "resolved-done", summary.Results[0].Replaced)
665665
}
666+
667+
// TestResolveFieldWithEmptyBraces tests the regression where strings.Trim() was
668+
// incorrectly stripping {} from expressions. This affected ternary CEL expressions
669+
// that end with empty maps like: condition ? value : {}
670+
func TestResolveFieldWithEmptyBraces(t *testing.T) {
671+
tests := []struct {
672+
name string
673+
resource map[string]interface{}
674+
data map[string]interface{}
675+
field variable.FieldDescriptor
676+
want ResolutionResult
677+
}{
678+
{
679+
name: "standalone expression ending with empty braces",
680+
resource: map[string]interface{}{
681+
"metadata": map[string]interface{}{
682+
"annotations": "${includeAnnotations ? annotations : {}}",
683+
},
684+
},
685+
data: map[string]interface{}{
686+
"includeAnnotations ? annotations : {}": map[string]interface{}{},
687+
},
688+
field: variable.FieldDescriptor{
689+
Path: "metadata.annotations",
690+
Expressions: []string{"includeAnnotations ? annotations : {}"},
691+
StandaloneExpression: true,
692+
},
693+
want: ResolutionResult{
694+
Path: "metadata.annotations",
695+
Original: "[includeAnnotations ? annotations : {}]",
696+
Resolved: true,
697+
Replaced: map[string]interface{}{},
698+
},
699+
},
700+
{
701+
name: "complex expression with has() and empty braces",
702+
resource: map[string]interface{}{
703+
"spec": map[string]interface{}{
704+
"config": "${has(schema.config) && includeConfig ? schema.config : {}}",
705+
},
706+
},
707+
data: map[string]interface{}{
708+
"has(schema.config) && includeConfig ? schema.config : {}": map[string]interface{}{
709+
"key": "value",
710+
},
711+
},
712+
field: variable.FieldDescriptor{
713+
Path: "spec.config",
714+
Expressions: []string{"has(schema.config) && includeConfig ? schema.config : {}"},
715+
StandaloneExpression: true,
716+
},
717+
want: ResolutionResult{
718+
Path: "spec.config",
719+
Original: "[has(schema.config) && includeConfig ? schema.config : {}]",
720+
Resolved: true,
721+
Replaced: map[string]interface{}{
722+
"key": "value",
723+
},
724+
},
725+
},
726+
{
727+
name: "expression with empty braces on both sides",
728+
resource: map[string]interface{}{
729+
"data": map[string]interface{}{
730+
"field": "${condition ? {} : {}}",
731+
},
732+
},
733+
data: map[string]interface{}{
734+
"condition ? {} : {}": map[string]interface{}{},
735+
},
736+
field: variable.FieldDescriptor{
737+
Path: "data.field",
738+
Expressions: []string{"condition ? {} : {}"},
739+
StandaloneExpression: true,
740+
},
741+
want: ResolutionResult{
742+
Path: "data.field",
743+
Original: "[condition ? {} : {}]",
744+
Resolved: true,
745+
Replaced: map[string]interface{}{},
746+
},
747+
},
748+
{
749+
name: "string template with expression ending in braces",
750+
resource: map[string]interface{}{
751+
"spec": map[string]interface{}{
752+
"value": "prefix-${expr ? value : {}}-suffix",
753+
},
754+
},
755+
data: map[string]interface{}{
756+
"expr ? value : {}": "resolved",
757+
},
758+
field: variable.FieldDescriptor{
759+
Path: "spec.value",
760+
Expressions: []string{"expr ? value : {}"},
761+
},
762+
want: ResolutionResult{
763+
Path: "spec.value",
764+
Original: "[expr ? value : {}]",
765+
Resolved: true,
766+
Replaced: "prefix-resolved-suffix",
767+
},
768+
},
769+
}
770+
771+
for _, tt := range tests {
772+
t.Run(tt.name, func(t *testing.T) {
773+
r := NewResolver(tt.resource, tt.data)
774+
got := r.resolveField(tt.field)
775+
776+
assert.Equal(t, tt.want.Path, got.Path)
777+
assert.Equal(t, tt.want.Original, got.Original)
778+
assert.Equal(t, tt.want.Resolved, got.Resolved)
779+
assert.Equal(t, tt.want.Replaced, got.Replaced)
780+
781+
if tt.want.Error != nil {
782+
assert.EqualError(t, got.Error, tt.want.Error.Error())
783+
} else {
784+
assert.NoError(t, got.Error)
785+
}
786+
787+
if tt.want.Resolved {
788+
value, err := r.getValueFromPath(tt.field.Path)
789+
assert.NoError(t, err)
790+
assert.Equal(t, tt.want.Replaced, value)
791+
}
792+
})
793+
}
794+
}

0 commit comments

Comments
 (0)