fix: bundle test panic#2944
Conversation
📝 WalkthroughWalkthroughThe PR updates ChangesTemplate Missing Key Configuration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/bundle/bundle.go (1)
5-5:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSwitch from
html/templatetotext/template
html/templateapplies HTML escaping to argument values, which corrupts the structured format expected bytuple.Tuple()andattribute.Attribute(). If entity IDs or other argument values contain characters like&,<,>, or", they will be HTML-escaped (e.g.,org&co→org&co), breaking the tuple/attribute string format before parsing.Fix
import ( "bytes" - "html/template" + "text/template" "github.com/Permify/permify/pkg/attribute" "github.com/Permify/permify/pkg/database"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/bundle/bundle.go` at line 5, Replace the html/template import with text/template to avoid HTML auto-escaping that corrupts tuple.Tuple()/attribute.Attribute() inputs; update the import line from "html/template" to "text/template" and ensure any uses of template.New, template.Parse, template.Execute (or template.Must) continue to reference the same symbols from the text/template package so rendered strings are not HTML-escaped before being passed to tuple.Tuple() and attribute.Attribute().
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/bundle/bundle.go`:
- Line 22: The template parsing currently uses
template.New("template").Option("missingkey=zero").Parse(w) which silently
substitutes missing keys; change the Option to "missingkey=error" so
template.New("template").Option("missingkey=error").Parse(w) to surface
missing-key errors; update this replacement in all four occurrences (the
template.New(...).Option(...).Parse(w) calls found in the four loops in
pkg/bundle/bundle.go).
---
Outside diff comments:
In `@pkg/bundle/bundle.go`:
- Line 5: Replace the html/template import with text/template to avoid HTML
auto-escaping that corrupts tuple.Tuple()/attribute.Attribute() inputs; update
the import line from "html/template" to "text/template" and ensure any uses of
template.New, template.Parse, template.Execute (or template.Must) continue to
reference the same symbols from the text/template package so rendered strings
are not HTML-escaped before being passed to tuple.Tuple() and
attribute.Attribute().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| for _, w := range rWrites { | ||
| // Parse the write operation string into a template. | ||
| tmpl, err := template.New("template").Parse(w) | ||
| tmpl, err := template.New("template").Option("missingkey=zero").Parse(w) |
There was a problem hiding this comment.
missingkey=zero silently produces malformed tuples/attributes — prefer missingkey=error
missingkey=zero returns the zero value (empty string for map[string]string) for missing keys, while missingkey=error stops execution immediately with an error. With missingkey=zero, a template like user:{{.user_id}}#member@group:{{.group_id}} with a missing group_id silently renders as user:alice#member@group:, which gets passed to tuple.Tuple() / attribute.Attribute(). The downstream parser may return an error, but it will describe a malformed string rather than the actual root cause (missing argument key), making debugging harder. If the downstream parser happens to accept a truncated string, data corruption occurs silently.
missingkey=error surfaces the missing key immediately with a clear error, which is the correct behaviour for a required-argument template substitution.
🐛 Proposed fix — use `missingkey=error` in all four loops
- tmpl, err := template.New("template").Option("missingkey=zero").Parse(w)
+ tmpl, err := template.New("template").Option("missingkey=error").Parse(w)Apply the same change to lines 58, 94, and 130.
Also applies to: 58-58, 94-94, 130-130
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/bundle/bundle.go` at line 22, The template parsing currently uses
template.New("template").Option("missingkey=zero").Parse(w) which silently
substitutes missing keys; change the Option to "missingkey=error" so
template.New("template").Option("missingkey=error").Parse(w) to surface
missing-key errors; update this replacement in all four occurrences (the
template.New(...).Option(...).Parse(w) calls found in the four loops in
pkg/bundle/bundle.go).
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2944 +/- ##
==========================================
- Coverage 82.61% 82.58% -0.02%
==========================================
Files 74 74
Lines 8300 8300
==========================================
- Hits 6856 6854 -2
- Misses 909 910 +1
- Partials 535 536 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1 participant
Summary by CodeRabbit