Skip to content

Commit 6a0184b

Browse files
chmouelclaude
chmouel
and
claude
committed
fix: resolve templating placeholder evaluation bug
Fix overly strict condition in ReplacePlaceHoldersVariables that prevented correct evaluation of placeholders when rawEvent or headers were nil. Changes: - body.* placeholders now work when rawEvent != nil (regardless of headers) - headers.* placeholders now work when headers != nil (regardless of rawEvent) - files.* placeholders now work independently of rawEvent/headers Add comprehensive unit tests improving coverage from 49.0% to 88.9%: - Nil parameter handling scenarios - CEL type conversions (numeric, boolean, array, object, double, int, bytes) - Error paths for invalid keys and expressions - Edge cases for missing dictionary placeholders - Complex nested object and array access - Mixed scenarios combining dico, body, and header lookups Split tests into focused functions: - TestReplacePlaceHoldersVariables: exact-match tests for deterministic cases - TestReplacePlaceHoldersVariablesJSONOutput: flexible JSON serialization tests - TestReplacePlaceHoldersVariablesEdgeCases: complex nested access patterns 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Chmouel Boudjnah <chmouel@redhat.com>
1 parent f8c4367 commit 6a0184b

File tree

2 files changed

+287
-1
lines changed

2 files changed

+287
-1
lines changed

‎pkg/templates/templating.go‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,18 @@ func ReplacePlaceHoldersVariables(template string, dico map[string]string, rawEv
5353
parts := keys.ParamsRe.FindStringSubmatch(s)
5454
key := strings.TrimSpace(parts[1])
5555
if strings.HasPrefix(key, "body") || strings.HasPrefix(key, "headers") || strings.HasPrefix(key, "files") {
56-
if rawEvent != nil && headers != nil {
56+
// Check specific requirements for each prefix
57+
canEvaluate := false
58+
switch {
59+
case strings.HasPrefix(key, "body") && rawEvent != nil:
60+
canEvaluate = true
61+
case strings.HasPrefix(key, "headers") && headers != nil:
62+
canEvaluate = true
63+
case strings.HasPrefix(key, "files"):
64+
canEvaluate = true // files evaluation doesn't depend on rawEvent or headers
65+
}
66+
67+
if canEvaluate {
5768
// convert headers to map[string]string
5869
headerMap := make(map[string]string)
5970
for k, v := range headers {

‎pkg/templates/templating_test.go‎

Lines changed: 275 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,9 +2,11 @@ package templates
22

33
import (
44
"net/http"
5+
"strings"
56
"testing"
67

78
"github.com/google/go-cmp/cmp"
9+
"gotest.tools/v3/assert"
810
)
911

1012
func TestReplacePlaceHoldersVariables(t *testing.T) {
@@ -72,7 +74,280 @@ func TestReplacePlaceHoldersVariables(t *testing.T) {
7274
headers: http.Header{},
7375
rawEvent: map[string]string{},
7476
},
77+
{
78+
name: "Test with nil rawEvent and headers",
79+
template: `body: {{ body.hello }}`,
80+
expected: `body: {{ body.hello }}`,
81+
dicto: map[string]string{},
82+
changedFiles: map[string]any{},
83+
headers: nil,
84+
rawEvent: nil,
85+
},
86+
{
87+
name: "Test with nil headers only",
88+
template: `body: {{ body.hello }}`,
89+
expected: `body: world`,
90+
dicto: map[string]string{},
91+
changedFiles: map[string]any{},
92+
headers: nil,
93+
rawEvent: map[string]string{
94+
"hello": "world",
95+
},
96+
},
97+
{
98+
name: "Test CEL with numeric value",
99+
template: `count: {{ body.count }}`,
100+
expected: `count: 42`,
101+
dicto: map[string]string{},
102+
changedFiles: map[string]any{},
103+
headers: http.Header{},
104+
rawEvent: map[string]any{
105+
"count": 42,
106+
},
107+
},
108+
{
109+
name: "Test CEL with boolean value",
110+
template: `enabled: {{ body.enabled }}`,
111+
expected: `enabled: true`,
112+
dicto: map[string]string{},
113+
changedFiles: map[string]any{},
114+
headers: http.Header{},
115+
rawEvent: map[string]any{
116+
"enabled": true,
117+
},
118+
},
119+
{
120+
name: "Test CEL with invalid key",
121+
template: `invalid: {{ body.nonexistent }}`,
122+
expected: `invalid: {{ body.nonexistent }}`,
123+
dicto: map[string]string{},
124+
changedFiles: map[string]any{},
125+
headers: http.Header{},
126+
rawEvent: map[string]any{
127+
"existing": "value",
128+
},
129+
},
130+
{
131+
name: "Test with file prefix but nil rawEvent",
132+
template: `file: {{ files.modified }}`,
133+
expected: `file: true`,
134+
dicto: map[string]string{},
135+
changedFiles: map[string]any{
136+
"modified": "true",
137+
},
138+
headers: http.Header{},
139+
rawEvent: nil,
140+
},
141+
{
142+
name: "Test with header prefix but nil rawEvent",
143+
template: `header: {{ headers.Accept }}`,
144+
expected: `header: application/json`,
145+
dicto: map[string]string{},
146+
changedFiles: map[string]any{},
147+
headers: http.Header{
148+
"Accept": []string{"application/json"},
149+
},
150+
rawEvent: nil,
151+
},
152+
{
153+
name: "Test placeholder not found in dico",
154+
template: `missing: {{ missing_key }}`,
155+
expected: `missing: {{ missing_key }}`,
156+
dicto: map[string]string{
157+
"existing_key": "value",
158+
},
159+
changedFiles: map[string]any{},
160+
headers: http.Header{},
161+
rawEvent: nil,
162+
},
163+
{
164+
name: "Test multiple placeholders mixed",
165+
template: `dico: {{ revision }}, body: {{ body.hello }}, header: {{ headers.Test }}`,
166+
expected: `dico: main, body: world, header: value`,
167+
dicto: map[string]string{
168+
"revision": "main",
169+
},
170+
changedFiles: map[string]any{},
171+
headers: http.Header{
172+
"Test": []string{"value"},
173+
},
174+
rawEvent: map[string]any{
175+
"hello": "world",
176+
},
177+
},
178+
}
179+
for _, tt := range tests {
180+
t.Run(tt.name, func(t *testing.T) {
181+
got := ReplacePlaceHoldersVariables(tt.template, tt.dicto, tt.rawEvent, tt.headers, tt.changedFiles)
182+
if d := cmp.Diff(got, tt.expected); d != "" {
183+
t.Fatalf("-got, +want: %v", d)
184+
}
185+
})
75186
}
187+
}
188+
189+
func TestReplacePlaceHoldersVariablesJSONOutput(t *testing.T) {
190+
tests := []struct {
191+
name string
192+
template string
193+
dicto map[string]string
194+
headers http.Header
195+
changedFiles map[string]any
196+
rawEvent any
197+
checkFunc func(t *testing.T, result string)
198+
}{
199+
{
200+
name: "CEL array serialization",
201+
template: `items: {{ body.items }}`,
202+
dicto: map[string]string{},
203+
changedFiles: map[string]any{},
204+
headers: http.Header{},
205+
rawEvent: map[string]any{
206+
"items": []string{"item1", "item2"},
207+
},
208+
checkFunc: func(t *testing.T, result string) {
209+
assert.Assert(t, strings.Contains(result, `"item1"`), "should contain item1")
210+
assert.Assert(t, strings.Contains(result, `"item2"`), "should contain item2")
211+
assert.Assert(t, strings.HasPrefix(result, "items: ["), "should start with 'items: ['")
212+
assert.Assert(t, strings.HasSuffix(result, "]"), "should end with ']'")
213+
},
214+
},
215+
{
216+
name: "CEL object serialization",
217+
template: `config: {{ body.config }}`,
218+
dicto: map[string]string{},
219+
changedFiles: map[string]any{},
220+
headers: http.Header{},
221+
rawEvent: map[string]any{
222+
"config": map[string]any{
223+
"name": "test",
224+
"value": "123",
225+
},
226+
},
227+
checkFunc: func(t *testing.T, result string) {
228+
assert.Assert(t, strings.Contains(result, `"name"`), "should contain name key")
229+
assert.Assert(t, strings.Contains(result, `"test"`), "should contain test value")
230+
assert.Assert(t, strings.Contains(result, `"value"`), "should contain value key")
231+
assert.Assert(t, strings.Contains(result, `"123"`), "should contain 123 value")
232+
assert.Assert(t, strings.HasPrefix(result, "config: {"), "should start with 'config: {'")
233+
assert.Assert(t, strings.HasSuffix(result, "}"), "should end with '}'")
234+
},
235+
},
236+
{
237+
name: "CEL double value",
238+
template: `price: {{ body.price }}`,
239+
dicto: map[string]string{},
240+
changedFiles: map[string]any{},
241+
headers: http.Header{},
242+
rawEvent: map[string]any{
243+
"price": 42.5,
244+
},
245+
checkFunc: func(t *testing.T, result string) {
246+
assert.Assert(t, strings.Contains(result, "42.5"), "should contain double value")
247+
assert.Assert(t, strings.HasPrefix(result, "price: "), "should start with 'price: '")
248+
},
249+
},
250+
{
251+
name: "CEL int value",
252+
template: `age: {{ body.age }}`,
253+
dicto: map[string]string{},
254+
changedFiles: map[string]any{},
255+
headers: http.Header{},
256+
rawEvent: map[string]any{
257+
"age": int64(25),
258+
},
259+
checkFunc: func(t *testing.T, result string) {
260+
assert.Assert(t, strings.Contains(result, "25"), "should contain int value")
261+
assert.Assert(t, strings.HasPrefix(result, "age: "), "should start with 'age: '")
262+
},
263+
},
264+
{
265+
name: "CEL bytes value",
266+
template: `data: {{ body.data }}`,
267+
dicto: map[string]string{},
268+
changedFiles: map[string]any{},
269+
headers: http.Header{},
270+
rawEvent: map[string]any{
271+
"data": []byte("hello"),
272+
},
273+
checkFunc: func(t *testing.T, result string) {
274+
assert.Assert(t, strings.HasPrefix(result, "data: "), "should start with 'data: '")
275+
// Bytes might be base64 encoded or handled differently
276+
assert.Assert(t, len(result) > len("data: "), "should have some data content")
277+
},
278+
},
279+
}
280+
281+
for _, tt := range tests {
282+
t.Run(tt.name, func(t *testing.T) {
283+
got := ReplacePlaceHoldersVariables(tt.template, tt.dicto, tt.rawEvent, tt.headers, tt.changedFiles)
284+
tt.checkFunc(t, got)
285+
})
286+
}
287+
}
288+
289+
func TestReplacePlaceHoldersVariablesEdgeCases(t *testing.T) {
290+
tests := []struct {
291+
name string
292+
template string
293+
dicto map[string]string
294+
headers http.Header
295+
changedFiles map[string]any
296+
rawEvent any
297+
expected string
298+
}{
299+
{
300+
name: "CEL expression with complex nested access",
301+
template: `nested: {{ body.deep.nested.value }}`,
302+
dicto: map[string]string{},
303+
changedFiles: map[string]any{},
304+
headers: http.Header{},
305+
rawEvent: map[string]any{
306+
"deep": map[string]any{
307+
"nested": map[string]any{
308+
"value": "found",
309+
},
310+
},
311+
},
312+
expected: `nested: found`,
313+
},
314+
{
315+
name: "CEL with nil in nested structure",
316+
template: `null: {{ body.value }}`,
317+
dicto: map[string]string{},
318+
changedFiles: map[string]any{},
319+
headers: http.Header{},
320+
rawEvent: map[string]any{
321+
"value": nil,
322+
},
323+
expected: `null: `,
324+
},
325+
{
326+
name: "Multi-level headers access",
327+
template: `user-agent: {{ headers["User-Agent"] }}`,
328+
dicto: map[string]string{},
329+
changedFiles: map[string]any{},
330+
headers: http.Header{
331+
"User-Agent": []string{"test-agent/1.0"},
332+
},
333+
rawEvent: map[string]any{},
334+
expected: `user-agent: test-agent/1.0`,
335+
},
336+
{
337+
name: "Files with nested structure",
338+
template: `file-info: {{ files.metadata.size }}`,
339+
dicto: map[string]string{},
340+
changedFiles: map[string]any{
341+
"metadata": map[string]any{
342+
"size": 1024,
343+
},
344+
},
345+
headers: http.Header{},
346+
rawEvent: nil,
347+
expected: `file-info: 1024`,
348+
},
349+
}
350+
76351
for _, tt := range tests {
77352
t.Run(tt.name, func(t *testing.T) {
78353
got := ReplacePlaceHoldersVariables(tt.template, tt.dicto, tt.rawEvent, tt.headers, tt.changedFiles)

0 commit comments

Comments
 (0)