fix(fieldpath): do not recurse forever on a field named * - #1095
fix(fieldpath): do not recurse forever on a field named *#1095arpitjain099 wants to merge 1 commit into
Conversation
expandWildcards substitutes each map key back into the segment list and re-expands from the top. A key that is literally "*" is substituted in as a wildcard segment again, so the same input expands over and over until the goroutine stack is exhausted. That is a fatal error, not a panic, so no recover() upstack can contain it. Segments cannot represent a literal "*" field (Segments.String renders one as [*], which parses back as a wildcard), so return an error for that key instead. Signed-off-by: Arpit Jain <arpitjain099@gmail.com>
📝 WalkthroughWalkthrough
ChangesWildcard ambiguity handling
Estimated code review effort: 2 (Simple) | ~5 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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://cdn.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/fieldpath/paved.go`:
- Around line 219-220: The ambiguity error in the wildcard check within the
field-path parsing logic is not actionable. Update the errors.Errorf message in
the k == wildcard branch to explain that the conflicting field must be renamed
before retrying, while retaining the existing path and field details.
- Around line 216-221: In the field-path expansion logic surrounding the
mapOrArray iteration, scan for and reject a literal "*" key before entering the
for k := range mapOrArray loop, ensuring its ambiguity error takes precedence
regardless of map iteration order. Preserve the existing error details and add a
regression test covering the spec[*][*] path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: eda39cd3-ee88-403d-9f41-659509e2ab43
📒 Files selected for processing (2)
pkg/fieldpath/paved.gopkg/fieldpath/paved_test.go
| // A field literally named "*" would be substituted back | ||
| // in as a wildcard segment, so the expansion below would | ||
| // recurse on the same input forever. | ||
| if k == wildcard { | ||
| return nil, errors.Errorf("%q: object has a field named %q, which cannot be distinguished from a wildcard", segments[:i], wildcard) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Check for the literal * before expanding other keys.
Go map iteration order is not defined. If the path contains a later wildcard, another key can be expanded first and return a different error before this code reaches the literal * key. For example, spec[*][*] can report unexpected wildcard usage instead of the ambiguity error.
Move the literal-key check before the for k := range mapOrArray loop and add a regression case for this path.
Proposed fix
case map[string]any:
+ if _, ok := mapOrArray[wildcard]; ok {
+ return nil, errors.Errorf("%q: object has a field named %q, which cannot be distinguished from a wildcard", segments[:i], wildcard)
+ }
for k := range mapOrArray {
- if k == wildcard {
- return nil, errors.Errorf("%q: object has a field named %q, which cannot be distinguished from a wildcard", segments[:i], wildcard)
- }
-
expanded := make(Segments, len(segments))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // A field literally named "*" would be substituted back | |
| // in as a wildcard segment, so the expansion below would | |
| // recurse on the same input forever. | |
| if k == wildcard { | |
| return nil, errors.Errorf("%q: object has a field named %q, which cannot be distinguished from a wildcard", segments[:i], wildcard) | |
| } | |
| case map[string]any: | |
| if _, ok := mapOrArray[wildcard]; ok { | |
| return nil, errors.Errorf("%q: object has a field named %q, which cannot be distinguished from a wildcard", segments[:i], wildcard) | |
| } | |
| for k := range mapOrArray { | |
| expanded := make(Segments, len(segments)) |
🤖 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/fieldpath/paved.go` around lines 216 - 221, In the field-path expansion
logic surrounding the mapOrArray iteration, scan for and reject a literal "*"
key before entering the for k := range mapOrArray loop, ensuring its ambiguity
error takes precedence regardless of map iteration order. Preserve the existing
error details and add a regression test covering the spec[*][*] path.
| if k == wildcard { | ||
| return nil, errors.Errorf("%q: object has a field named %q, which cannot be distinguished from a wildcard", segments[:i], wildcard) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the ambiguity error actionable.
The message explains the conflict but does not tell the caller how to correct the input. State that the field must be renamed before retrying, because field-path syntax cannot represent a literal * separately from a wildcard.
As per path instructions, Go errors must provide user context and suggest next steps.
🤖 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/fieldpath/paved.go` around lines 219 - 220, The ambiguity error in the
wildcard check within the field-path parsing logic is not actionable. Update the
errors.Errorf message in the k == wildcard branch to explain that the
conflicting field must be renamed before retrying, while retaining the existing
path and field details.
Source: Path instructions
1 participant
Description of your changes
expandWildcardshandles a wildcard segment over a map by substituting each key back into the segment list and re-expanding from the top:If one of those keys is literally
*, the substituted segment is a wildcard again, so the call re-expands the identical input and never makes progress. The goroutine stack fills and Go reportsfatal error: stack overflow, which is not recoverable, so arecover()further up the stack cannot contain it.Reproduced against
mainbefore the fix, withExpandWildcards("spec[*]")on{"spec":{"*":"star"}}:A literal
*field cannot be represented inSegmentsat all:Segments.Stringrenders such a field as[*], and parsing that back yields a wildcard. So rather than trying to expand it, this returns an error for that key. Objects without a*key are unaffected, and the array branch is untouched.Test case
LiteralWildcardKeyadded toTestExpandWildcards. It overflows the stack on the unmodified tree and passes with the change; the rest of the package passes either way.Fixes #
I have:
Run(ran./nix.sh flake checkto ensure this PR is ready for review.go test ./pkg/fieldpath/...,go vetandgofmtlocally; happy to run the full flake check if you want it)Linked a PR or a docs tracking issue to document this change.No user-facing docs change.AddedLet me know if you want this backported.backport release-x.ylabels to auto-backport this PR.