Feats negative number for override.go - #1749
Conversation
WalkthroughImplements negative index support in JSON path handling within relay/common/override.go by preprocessing paths (including condition checks and operations) via a new unexported helper. Updates delete/set/move/prepend/append to use processed paths and adds unified error propagation for failed operations. Imports regexp and strconv for parsing. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant Override as OverrideLogic
participant NegIdx as processNegativeIndex
participant JSON as JSONStore
Caller->>Override: apply(operation, path, from, to, json)
rect rgba(231, 245, 254, 0.6)
note right of Override: Preprocess paths (supports negative indices)
Override->>NegIdx: process(path)
NegIdx-->>Override: opPath
Override->>NegIdx: process(from)
NegIdx-->>Override: opFrom
Override->>NegIdx: process(to)
NegIdx-->>Override: opTo
end
alt delete/set/prepend/append
Override->>JSON: mutate using opPath (or opTo)
JSON-->>Override: result or error
else move
Override->>JSON: read from opFrom
JSON-->>Override: value or error
Override->>JSON: write to opTo
JSON-->>Override: result or error
end
alt error
Override-->>Caller: error "operation <mode> failed: <err>"
else success
Override-->>Caller: success
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
relay/common/override.go (2)
8-9: Precompile the regex to avoid per-call allocations.Move the regex to a package-level var and reuse it in processNegativeIndex.
Add near the top-level:
var negativeIndexRe = regexp.MustCompile(`\.-(\d+)`)
156-158: Good call: preprocess paths for conditions. Add the processed path to error context.When compareGjsonValues fails, include both processed and original paths for quicker debugging.
Change the error return in Line 175 to:
return false, fmt.Errorf("comparison failed for path %s (orig: %s): %v", path, condition.Path, err)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
relay/common/override.go(4 hunks)
🔇 Additional comments (3)
relay/common/override.go (3)
319-319: Delete with processed path — LGTM.
321-325: Set with KeepOrigin using processed path — LGTM.
328-330: Prepend/append with processed path — LGTM.
| func processNegativeIndex(jsonStr string, path string) string { | ||
| re := regexp.MustCompile(`\.(-\d+)`) | ||
| matches := re.FindAllStringSubmatch(path, -1) | ||
|
|
||
| if len(matches) == 0 { | ||
| return path | ||
| } | ||
|
|
||
| result := path | ||
| for _, match := range matches { | ||
| negIndex := match[1] | ||
| index, _ := strconv.Atoi(negIndex) | ||
|
|
||
| arrayPath := strings.Split(path, negIndex)[0] | ||
| if strings.HasSuffix(arrayPath, ".") { | ||
| arrayPath = arrayPath[:len(arrayPath)-1] | ||
| } | ||
|
|
||
| array := gjson.Get(jsonStr, arrayPath) | ||
| if array.IsArray() { | ||
| length := len(array.Array()) | ||
| actualIndex := length + index | ||
| if actualIndex >= 0 && actualIndex < length { | ||
| result = strings.Replace(result, match[0], "."+strconv.Itoa(actualIndex), 1) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return result | ||
| } |
There was a problem hiding this comment.
Bug: nested/multiple negative indices can resolve incorrectly.
arrayPath is computed from the original path and may still contain unresolved negative indices, breaking lookups for subsequent matches (e.g., a.-1.b.-2). Iterate left-to-right on the progressively updated result and compute arrayPath via match indices.
Apply this diff:
-func processNegativeIndex(jsonStr string, path string) string {
- re := regexp.MustCompile(`\.(-\d+)`)
- matches := re.FindAllStringSubmatch(path, -1)
-
- if len(matches) == 0 {
- return path
- }
-
- result := path
- for _, match := range matches {
- negIndex := match[1]
- index, _ := strconv.Atoi(negIndex)
-
- arrayPath := strings.Split(path, negIndex)[0]
- if strings.HasSuffix(arrayPath, ".") {
- arrayPath = arrayPath[:len(arrayPath)-1]
- }
-
- array := gjson.Get(jsonStr, arrayPath)
- if array.IsArray() {
- length := len(array.Array())
- actualIndex := length + index
- if actualIndex >= 0 && actualIndex < length {
- result = strings.Replace(result, match[0], "."+strconv.Itoa(actualIndex), 1)
- }
- }
- }
-
- return result
-}
+// processNegativeIndex walks the path and resolves ".-N" against the array length at that point.
+func processNegativeIndex(jsonStr string, path string) string {
+ if path == "" {
+ return ""
+ }
+ re := regexp.MustCompile(`\.-(\d+)`) // or use package-level negativeIndexRe
+ result := path
+ offset := 0
+ for {
+ loc := re.FindStringSubmatchIndex(result[offset:])
+ if loc == nil {
+ break
+ }
+ start := offset + loc[0]
+ end := offset + loc[1]
+ digitsStart := offset + loc[2]
+ digitsEnd := offset + loc[3]
+ idx, err := strconv.Atoi("-" + result[digitsStart:digitsEnd])
+ if err != nil {
+ offset = end
+ continue
+ }
+ arrayPath := strings.TrimSuffix(result[:start], ".")
+ arr := gjson.Get(jsonStr, arrayPath)
+ if arr.IsArray() {
+ length := len(arr.Array())
+ actual := length + idx
+ if actual >= 0 && actual < length {
+ repl := "." + strconv.Itoa(actual)
+ result = result[:start] + repl + result[end:]
+ offset = start + len(repl)
+ continue
+ }
+ }
+ offset = end
+ }
+ return result
+}| // 处理路径中的负数索引 | ||
| opPath := processNegativeIndex(result, op.Path) | ||
| opFrom := processNegativeIndex(result, op.From) | ||
| opTo := processNegativeIndex(result, op.To) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate required fields after preprocessing.
Prevent operations with empty paths (e.g., missing from/to for move) to avoid surprises like querying the root with empty path.
opPath := processNegativeIndex(result, op.Path)
opFrom := processNegativeIndex(result, op.From)
opTo := processNegativeIndex(result, op.To)
+ // basic validation
+ switch op.Mode {
+ case "move":
+ if opFrom == "" || opTo == "" {
+ return "", fmt.Errorf("move requires both 'from' and 'to'")
+ }
+ default:
+ if opPath == "" {
+ return "", fmt.Errorf("%s requires non-empty 'path'", op.Mode)
+ }
+ }📝 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.
| // 处理路径中的负数索引 | |
| opPath := processNegativeIndex(result, op.Path) | |
| opFrom := processNegativeIndex(result, op.From) | |
| opTo := processNegativeIndex(result, op.To) | |
| // 处理路径中的负数索引 | |
| opPath := processNegativeIndex(result, op.Path) | |
| opFrom := processNegativeIndex(result, op.From) | |
| opTo := processNegativeIndex(result, op.To) | |
| // basic validation | |
| switch op.Mode { | |
| case "move": | |
| if opFrom == "" || opTo == "" { | |
| return "", fmt.Errorf("move requires both 'from' and 'to'") | |
| } | |
| default: | |
| if opPath == "" { | |
| return "", fmt.Errorf("%s requires non-empty 'path'", op.Mode) | |
| } | |
| } |
🤖 Prompt for AI Agents
In relay/common/override.go around lines 312 to 316, after calling
processNegativeIndex for op.Path, op.From, and op.To, validate that required
path fields are non-empty (e.g., op.Path for most ops, and both op.From and
op.To for move operations); if any required path is empty, return an explicit
error (or skip the operation) instead of continuing and potentially querying the
root — ensure validation happens after preprocessing so negative-index
resolution is considered.
| result, err = sjson.Set(result, opPath, op.Value) | ||
| case "move": | ||
| result, err = moveValue(result, op.From, op.To) | ||
| result, err = moveValue(result, opFrom, opTo) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Moving into a descendant of source will delete the newly set value.
Current order (set then delete) removes the destination when toPath is under fromPath. Handle descendant case by deleting first, then setting with the captured value.
Proposed change in moveValue (outside this hunk):
func moveValue(jsonStr, fromPath, toPath string) (string, error) {
sourceValue := gjson.Get(jsonStr, fromPath)
if !sourceValue.Exists() {
return jsonStr, fmt.Errorf("source path does not exist: %s", fromPath)
}
- result, err := sjson.Set(jsonStr, toPath, sourceValue.Value())
- if err != nil {
- return "", err
- }
- return sjson.Delete(result, fromPath)
+ // Delete-first if destination is a descendant of source.
+ if toPath == fromPath || strings.HasPrefix(toPath, fromPath+".") {
+ tmp, err := sjson.Delete(jsonStr, fromPath)
+ if err != nil {
+ return "", err
+ }
+ return sjson.Set(tmp, toPath, sourceValue.Value())
+ }
+ result, err := sjson.Set(jsonStr, toPath, sourceValue.Value())
+ if err != nil {
+ return "", err
+ }
+ return sjson.Delete(result, fromPath)
}📝 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.
| result, err = moveValue(result, opFrom, opTo) | |
| func moveValue(jsonStr, fromPath, toPath string) (string, error) { | |
| sourceValue := gjson.Get(jsonStr, fromPath) | |
| if !sourceValue.Exists() { | |
| return jsonStr, fmt.Errorf("source path does not exist: %s", fromPath) | |
| } | |
| // Delete-first if destination is a descendant of source. | |
| if toPath == fromPath || strings.HasPrefix(toPath, fromPath+".") { | |
| tmp, err := sjson.Delete(jsonStr, fromPath) | |
| if err != nil { | |
| return "", err | |
| } | |
| return sjson.Set(tmp, toPath, sourceValue.Value()) | |
| } | |
| // Default: set then delete | |
| result, err := sjson.Set(jsonStr, toPath, sourceValue.Value()) | |
| if err != nil { | |
| return "", err | |
| } | |
| return sjson.Delete(result, fromPath) | |
| } |
🤖 Prompt for AI Agents
In relay/common/override.go around line 326, the call currently does set then
delete which causes the destination to be removed when toPath is a descendant of
fromPath; modify moveValue so it first detects if toPath is a descendant of
fromPath, capture the source value into a temporary variable, perform the delete
on fromPath first, then set the captured value at toPath (otherwise keep the
existing set-then-delete flow). Ensure the descendant check is robust (path
prefix or tree-structure comparison) and preserve existing error handling and
return values.
…ng-effort fix: support xhigh reasoning effort in usage records
2 participants
Summary by CodeRabbit