feat: add parameter coverage for more operations - #2568
Conversation
…rim_suffix, ensure_prefix, ensure_suffix, trim_space, to_lower, to_upper, replace, and regex_replace
WalkthroughThe changes expand the parameter override system by introducing ten new operation modes (copy, trim_prefix, trim_suffix, ensure_prefix, ensure_suffix, trim_space, to_lower, to_upper, replace, regex_replace) with corresponding helper functions for string transformations and value copying, alongside comprehensive error handling and a full test suite. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
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: 0
🧹 Nitpick comments (2)
relay/common/override.go (2)
343-352: Validation for "copy" is inconsistent with "move".The
copyoperation validates thatfromandtoare required (lines 347-349), but themoveoperation (lines 343-345) lacks similar validation. Ifop.Fromorop.Tois empty formove,moveValuewill proceed with empty paths, potentially causing unexpected behavior.🔎 Proposed fix to add validation for move operation
case "move": + if op.From == "" || op.To == "" { + return "", fmt.Errorf("move from/to is required") + } opFrom := processNegativeIndex(result, op.From) opTo := processNegativeIndex(result, op.To) result, err = moveValue(result, opFrom, opTo)
525-538: Consider architectural caching for dynamic regex patterns if performance becomes a concern.The regex is compiled on every call to
regexReplaceStringValue. Since the function accepts user-provided patterns, simple package-level caching isn't feasible—a cache mechanism likesync.Mapwould be required. Only implement caching if you observe repeated patterns in production.For security, the code already validates patterns via
regexp.Compileerror handling, which mitigates basic ReDoS risks. If stricter ReDoS protection is needed for security-sensitive contexts, consider adding a timeout or complexity limit for pattern compilation.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
relay/common/override.gorelay/common/override_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
relay/common/override_test.go (2)
relay/common/override.go (1)
ApplyParamOverride(35-49)common/json.go (1)
Unmarshal(9-11)
🔇 Additional comments (18)
relay/common/override_test.go (11)
1-28: LGTM!Good test structure with clear comments showing the expected operation format. The test correctly validates the trim_prefix operation removes the specified prefix from the model string.
30-70: LGTM!Good coverage for trim_suffix and the no-op scenario where the prefix doesn't exist. These tests properly verify idempotent behavior.
72-89: LGTM!Proper error handling test - validates that trim_prefix requires a value parameter.
91-171: LGTM!Comprehensive tests for replace and regex_replace operations, including validation of required parameters (from for replace, pattern for regex_replace).
173-272: LGTM!Good coverage for core operations: delete, set, set with keep_origin, move, and move with missing source. Tests properly validate both successful operations and error conditions.
274-365: LGTM!Thorough tests for prepend/append operations on both strings and arrays, plus object merge behavior with and without keep_origin. The array flattening behavior on append (line 310) is properly tested.
367-511: LGTM!Excellent coverage for conditional execution including OR/AND logic, invert flag, pass_missing_key behavior, and context-based evaluation. These tests ensure the conditional system works correctly across various scenarios.
513-610: LGTM!Good tests for negative index path handling and copy operation including error cases (missing source, missing from/to fields).
612-713: LGTM!Comprehensive tests for ensure_prefix and ensure_suffix operations, including no-op scenarios when the affix already exists and validation that value is required.
715-773: LGTM!Good coverage for trim_space, to_lower, and to_upper transformations. The trim_space test on line 718 properly tests whitespace including newline character.
775-791: LGTM!Clean helper function using
t.Helper()for proper test failure attribution. Usingreflect.DeepEqualafter unmarshaling is the correct approach for JSON comparison regardless of key ordering.relay/common/override.go (7)
24-33: LGTM!Good documentation of all supported modes in the struct comment. The struct properly supports all the new operations with the existing fields.
357-374: LGTM!Clean dispatch to the new string operation helpers. Good use of the existing
op.Fromandop.Tofields for replace operations rather than introducing new fields.
397-403: LGTM!Clean implementation of
copyValue. Properly checks source existence and returns a contextual error message.
458-476: LGTM!Good implementation with proper type checking and nil value validation. Using
fmt.Sprintf("%v", value)handles various input types gracefully.
478-504: LGTM!Proper implementation with both nil check and empty string validation for ensure operations. The logic correctly handles both prefix and suffix cases with early return when affix already exists.
506-512: LGTM!Elegant use of higher-order function to handle multiple string transformations (TrimSpace, ToLower, ToUpper) with a single implementation.
514-523: LGTM!Proper validation that
fromis required. Good use ofstrings.ReplaceAllfor the replacement.
…rride_trim_prefix
2 participants
Add parameter coverage for the operations: copy, trim_prefix, trim_suffix, ensure_prefix, ensure_suffix, trim_space, to_lower, to_upper, replace, and regex_replace
example: https://github.com/seefs001/new-api/blob/817da8d73c1fbd527bc8405cdbd6b9ac6a338221/relay/common/override_test.go
#2496
Summary by CodeRabbit
Release Notes
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.