Feats: use the types of gjson,the error expection,the invert and the key missing process of the condition - #1605
Conversation
WalkthroughThe PR updates relay/common/override.go to introduce generic, multi-mode condition evaluation for JSON overrides, add inversion and missing-key handling, propagate errors during evaluation, change ApplyParamOverride to return errors directly, and adjust operation parsing to support the new fields and value types. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant ApplyParamOverride
participant tryParseOperations
participant applyOperations
participant checkConditions
participant compare
Caller->>ApplyParamOverride: ApplyParamOverride(params, overrides)
ApplyParamOverride->>tryParseOperations: parse operations (values/invert/pass_missing_key)
tryParseOperations-->>ApplyParamOverride: operations or error
ApplyParamOverride->>applyOperations: apply with condition checks
applyOperations->>checkConditions: evaluate AND/OR conditions
checkConditions->>compare: multi-mode comparisons (full/prefix/.../lte)
compare-->>checkConditions: result or error
checkConditions-->>applyOperations: bool or error
applyOperations-->>ApplyParamOverride: applied ops or error
ApplyParamOverride-->>Caller: modified params or error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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: 2
🔭 Outside diff range comments (1)
relay/common/override.go (1)
380-384: Handle json.Marshal error and avoid discarding it.Discarding the error can hide issues when value contains unsupported types. Propagate it.
Apply this diff:
- default: - jsonBytes, _ := json.Marshal(v) - if err := json.Unmarshal(jsonBytes, &newMap); err != nil { - return "", err - } + default: + jsonBytes, err := json.Marshal(v) + if err != nil { + return "", err + } + if err := json.Unmarshal(jsonBytes, &newMap); err != nil { + return "", err + }
🧹 Nitpick comments (4)
relay/common/override.go (4)
39-39: Avoid returning partial/empty payloads when applyOperations fails.When applyOperations returns an error, return nil for the byte slice for clarity.
Apply this diff:
- result, err := applyOperations(string(jsonData), operations) - return []byte(result), err + result, err := applyOperations(string(jsonData), operations) + if err != nil { + return nil, err + } + return []byte(result), nil
96-104: Default condition mode to "full" when absent; improves robustness.If a condition omits mode, compareGjsonValues currently errors with “unsupported comparison mode: ”. Defaulting to full matches typical expectations and avoids surprising failures.
Apply this diff:
- if mode, ok := condMap["mode"].(string); ok { - condition.Mode = mode - } + if mode, ok := condMap["mode"].(string); ok && mode != "" { + condition.Mode = mode + } else { + condition.Mode = "full" + }Optional: consider validating that condMap["path"] is a non-empty string to avoid evaluating the whole document accidentally.
123-151: Short-circuit condition evaluation and validate unknown logic operators.Current implementation evaluates all conditions and treats any non-AND logic as OR. Short-circuiting improves performance and validating logic reduces surprises.
Apply this diff:
-func checkConditions(jsonStr string, conditions []ConditionOperation, logic string) (bool, error) { - if len(conditions) == 0 { - return true, nil // 没有条件，直接通过 - } - results := make([]bool, len(conditions)) - for i, condition := range conditions { - result, err := checkSingleCondition(jsonStr, condition) - if err != nil { - return false, err - } - results[i] = result - } - - if strings.ToUpper(logic) == "AND" { - for _, result := range results { - if !result { - return false, nil - } - } - return true, nil - } else { - for _, result := range results { - if result { - return true, nil - } - } - return false, nil - } -} +func checkConditions(jsonStr string, conditions []ConditionOperation, logic string) (bool, error) { + if len(conditions) == 0 { + return true, nil // 没有条件，直接通过 + } + logicUpper := strings.ToUpper(strings.TrimSpace(logic)) + if logicUpper == "" { + logicUpper = "OR" + } + switch logicUpper { + case "AND": + for _, condition := range conditions { + ok, err := checkSingleCondition(jsonStr, condition) + if err != nil { + return false, err + } + if !ok { + return false, nil + } + } + return true, nil + case "OR": + for _, condition := range conditions { + ok, err := checkSingleCondition(jsonStr, condition) + if err != nil { + return false, err + } + if ok { + return true, nil + } + } + return false, nil + default: + return false, fmt.Errorf("unsupported logic operator: %s", logic) + } +}
180-202: Clarify non-string handling for prefix/suffix/contains.These branches coerce values to strings; “contains” on arrays/objects will check substring of their JSON serialization, which can be surprising. Either:
- constrain these modes to string types and return an error otherwise, or
- explicitly document that non-strings are stringified.
If you prefer strict typing, I can propose a guarded implementation that errors on non-string types.
📜 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(5 hunks)
🔇 Additional comments (3)
relay/common/override.go (3)
12-16: Good expansion of condition expressiveness (types, invert, missing-key).Struct changes look sensible and align with the PR goals. The generic Value plus Invert and PassMissingKey flags enable richer condition handling.
228-249: Numeric comparisons look correct.Type checks and operators are handled cleanly with clear errors on invalid types.
270-274: Error propagation on condition evaluation is correct.Returning the error directly here is the right choice given the PR’s “error expectation” goals.
| func checkSingleCondition(jsonStr string, condition ConditionOperation) (bool, error) { | ||
| value := gjson.Get(jsonStr, condition.Path) | ||
| if !value.Exists() { | ||
| return false | ||
| if condition.PassMissingKey { | ||
| return true, nil | ||
| } | ||
| return false, nil | ||
| } | ||
|
|
||
| valueStr := value.String() | ||
| targetStr := condition.Value | ||
| // 利用gjson的类型解析 | ||
| targetBytes, err := json.Marshal(condition.Value) | ||
| if err != nil { | ||
| return false, fmt.Errorf("failed to marshal condition value: %v", err) | ||
| } | ||
| targetValue := gjson.ParseBytes(targetBytes) | ||
|
|
||
| result, err := compareGjsonValues(value, targetValue, strings.ToLower(condition.Mode)) | ||
| if err != nil { | ||
| return false, fmt.Errorf("comparison failed for path %s: %v", condition.Path, err) | ||
| } | ||
|
|
||
| if condition.Invert { | ||
| result = !result | ||
| } | ||
| return result, nil | ||
| } |
There was a problem hiding this comment.
Invert is not applied when the key is missing.
If the key is missing and PassMissingKey is true, the function returns early and skips Invert, which makes invert semantics inconsistent across branches.
Apply this diff to unify result computation and always apply invert:
func checkSingleCondition(jsonStr string, condition ConditionOperation) (bool, error) {
- value := gjson.Get(jsonStr, condition.Path)
- if !value.Exists() {
- if condition.PassMissingKey {
- return true, nil
- }
- return false, nil
- }
-
- // 利用gjson的类型解析
- targetBytes, err := json.Marshal(condition.Value)
- if err != nil {
- return false, fmt.Errorf("failed to marshal condition value: %v", err)
- }
- targetValue := gjson.ParseBytes(targetBytes)
-
- result, err := compareGjsonValues(value, targetValue, strings.ToLower(condition.Mode))
- if err != nil {
- return false, fmt.Errorf("comparison failed for path %s: %v", condition.Path, err)
- }
-
- if condition.Invert {
- result = !result
- }
- return result, nil
+ value := gjson.Get(jsonStr, condition.Path)
+ mode := strings.ToLower(condition.Mode)
+ if mode == "" {
+ mode = "full"
+ }
+
+ var result bool
+ if !value.Exists() {
+ result = condition.PassMissingKey
+ } else {
+ // 利用gjson的类型解析
+ targetBytes, err := json.Marshal(condition.Value)
+ if err != nil {
+ return false, fmt.Errorf("failed to marshal condition value: %v", err)
+ }
+ targetValue := gjson.ParseBytes(targetBytes)
+
+ r, err := compareGjsonValues(value, targetValue, mode)
+ if err != nil {
+ return false, fmt.Errorf("comparison failed for path %s: %v", condition.Path, err)
+ }
+ result = r
+ }
+
+ if condition.Invert {
+ result = !result
+ }
+ return result, nil
}🤖 Prompt for AI Agents
In relay/common/override.go around lines 153 to 178, the function returns early
when the key is missing which skips applying condition.Invert; change the flow
so you compute a boolean result variable in all branches (when key missing: set
result = true if condition.PassMissingKey else result = false; when key present:
compute result via compareGjsonValues and handle errors as before), then after
all branches apply if condition.Invert { result = !result } and finally return
result, nil (or return false,error if compare returned an error). Ensure no
early returns bypass the invert logic.
| func compareEqual(jsonValue, targetValue gjson.Result) (bool, error) { | ||
| // 对布尔值特殊处理 | ||
| if (jsonValue.Type == gjson.True || jsonValue.Type == gjson.False) && | ||
| (targetValue.Type == gjson.True || targetValue.Type == gjson.False) { | ||
| return jsonValue.Bool() == targetValue.Bool(), nil | ||
| } | ||
|
|
||
| // 如果类型不同，报错 | ||
| if jsonValue.Type != targetValue.Type { | ||
| return false, fmt.Errorf("compare for different types, got %v and %v", jsonValue.Type, targetValue.Type) | ||
| } | ||
|
|
||
| switch jsonValue.Type { | ||
| case gjson.True, gjson.False: | ||
| return jsonValue.Bool() == targetValue.Bool(), nil | ||
| case gjson.Number: | ||
| return jsonValue.Num == targetValue.Num, nil | ||
| case gjson.String: | ||
| return jsonValue.String() == targetValue.String(), nil | ||
| default: | ||
| return jsonValue.String() == targetValue.String(), nil | ||
| } | ||
| } |
There was a problem hiding this comment.
Array/object equality via string comparison is order-dependent; use deep structural equality.
Using String() for JSON types makes equality sensitive to key order and whitespace. This can cause false negatives for semantically equal objects.
Apply this diff:
func compareEqual(jsonValue, targetValue gjson.Result) (bool, error) {
@@
// 如果类型不同，报错
if jsonValue.Type != targetValue.Type {
return false, fmt.Errorf("compare for different types, got %v and %v", jsonValue.Type, targetValue.Type)
}
switch jsonValue.Type {
case gjson.True, gjson.False:
return jsonValue.Bool() == targetValue.Bool(), nil
case gjson.Number:
return jsonValue.Num == targetValue.Num, nil
case gjson.String:
return jsonValue.String() == targetValue.String(), nil
+ case gjson.JSON:
+ // 对数组/对象进行结构化比较（与键顺序无关）
+ var left, right interface{}
+ if err := json.Unmarshal([]byte(jsonValue.Raw), &left); err != nil {
+ return false, err
+ }
+ if err := json.Unmarshal([]byte(targetValue.Raw), &right); err != nil {
+ return false, err
+ }
+ return reflect.DeepEqual(left, right), nil
default:
return jsonValue.String() == targetValue.String(), nil
}
}And add the missing import:
import "reflect"🤖 Prompt for AI Agents
In relay/common/override.go around lines 204 to 226, the current equality
function falls back to jsonValue.String() which makes object/array comparisons
order-dependent; replace string fallback with structural comparison using
reflect.DeepEqual on jsonValue.Value() and targetValue.Value() for compound
types (arrays and objects), keep the existing precise checks for booleans,
numbers and strings, and add the missing import "reflect". Ensure you only use
reflect.DeepEqual for non-primitive JSON types (or as the default for types
other than True/False/Number/String) so semantically equal objects/arrays
compare equal regardless of key order.
…rams-override Feats: use the types of gjson,the error expection,the invert and the key missing process of the condition
2 participants
Summary by CodeRabbit