Improving lua script with args and values - #6561
Conversation
Mzack9999
added
the
Type: Enhancement
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughExtended the ChangesRedis Lua Script API Support
Headless Test Timing Stabilization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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: 3
🧹 Nitpick comments (1)
pkg/js/libs/redis/redis.go (1)
223-243: Consider optimizing the args conversion to avoid redundant processing.The current implementation converts args twice:
[]interface{}→[]string→[]interface{}. When args is already[]interface{}, you could skip the intermediate[]stringconversion and directly prepare it for the Eval call.Refactor to eliminate redundant conversion:
// Convert interface{} args directly to []interface{} for Eval argsInterface := []interface{}{} if args != nil { switch v := args.(type) { case []string: argsInterface = make([]interface{}, len(v)) for i, arg := range v { argsInterface[i] = arg } case []interface{}: // Filter to only strings (or handle type conversion) for _, item := range v { if s, ok := item.(string); ok { argsInterface = append(argsInterface, s) } } } }This eliminates the intermediate
argsSlicevariable and avoids double conversion for[]interface{}inputs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
integration_tests/protocols/javascript/redis-lua-script.yamlis excluded by!**/*.yamlpkg/js/generated/ts/redis.tsis excluded by!**/generated/**
📒 Files selected for processing (2)
cmd/integration-test/javascript.go(2 hunks)pkg/js/libs/redis/redis.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Format Go code using go fmt
Run static analysis with go vet
Files:
pkg/js/libs/redis/redis.gocmd/integration-test/javascript.go
🧬 Code graph analysis (2)
pkg/js/libs/redis/redis.go (1)
pkg/js/generated/ts/redis.ts (1)
RunLuaScript(70-72)
cmd/integration-test/javascript.go (1)
pkg/testutils/integration.go (2)
TestCase(247-250)RunNucleiTemplateAndGetResults(30-32)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Tests (macOS-latest)
- GitHub Check: Tests (windows-latest)
- GitHub Check: Tests (ubuntu-latest)
🔇 Additional comments (4)
cmd/integration-test/javascript.go (1)
15-15: LGTM!The test case registration is consistent with other JavaScript test cases and properly disables on Windows/OSX platforms.
pkg/js/libs/redis/redis.go (3)
178-182: LGTM!The documentation clearly demonstrates both backward-compatible usage (without keys/args) and the new signature with keys and args parameters.
183-183: LGTM!The function signature correctly uses
interface{}types for keys and args to support backward compatibility (nil values) and flexible type handling from JavaScript.
246-246: LGTM!The Eval call correctly uses the converted
keysSliceandargsInterfaceparameters to execute the Lua script with the provided keys and arguments.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
pkg/js/libs/redis/redis.go (3)
205-217: Consider validating key types explicitly.While
fmt.Sprintf("%v", item)prevents silent data loss, it may still produce unexpected string representations for complex types (e.g., maps, structs). For Redis keys, only strings and basic types are typically meaningful.Consider adding a default case to handle unexpected types:
// Convert interface{} to []string for keys (handle backwards compatibility) keysSlice := []string{} if keys != nil { switch v := keys.(type) { case []string: keysSlice = v case []interface{}: keysSlice = make([]string, 0, len(v)) for _, item := range v { keysSlice = append(keysSlice, fmt.Sprintf("%v", item)) } + default: + return nil, fmt.Errorf("keys must be []string or []interface{}, got %T", keys) } }
236-240: Eliminate unnecessary conversion by directly building []interface{}.The current flow converts args through multiple types:
[]interface{}(from JS) →[]string→[]interface{}(for Eval). This is inefficient and adds complexity.Consider refactoring to build
[]interface{}directly:- // Convert interface{} to []string for args (handle backwards compatibility) - argsSlice := []string{} + // Convert interface{} to []interface{} for args (handle backwards compatibility) + argsInterface := []interface{}{} if args != nil { switch v := args.(type) { case []string: - argsSlice = v + argsInterface = make([]interface{}, len(v)) + for i, s := range v { + argsInterface[i] = s + } case []interface{}: - // Convert []interface{} to []string (from JavaScript arrays) - argsSlice = make([]string, 0, len(v)) - for _, item := range v { - argsSlice = append(argsSlice, fmt.Sprintf("%v", item)) - } + argsInterface = v default: return nil, fmt.Errorf("args must be []string or []interface{}, got %T", args) } } - - // Convert []string args to []interface{} for Eval - argsInterface := make([]interface{}, len(argsSlice)) - for i, arg := range argsSlice { - argsInterface[i] = arg - }This preserves the original types from JavaScript (numbers, booleans, etc.) without unnecessary string conversions, which is more correct for Redis Lua scripts.
243-243: Consider using the provided context parameter.The Eval call uses
context.Background()instead of thectxparameter passed to the function. This bypasses any timeout or cancellation configured by the caller.Apply this diff to respect the caller's context:
- infoCmd := client.Eval(context.Background(), script, keysSlice, argsInterface...) + infoCmd := client.Eval(ctx, script, keysSlice, argsInterface...)Note: This pattern is already used in other functions in this file (e.g., line 43, 49, 84).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/integration-test/javascript.go(5 hunks)pkg/js/libs/redis/redis.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/integration-test/javascript.go
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Format Go code usinggo fmt ./...
Run static analysis usinggo vet ./...
Files:
pkg/js/libs/redis/redis.go
pkg/js/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
JavaScript runtime custom implementations for code protocol templates should use auto-generated bindings from pkg/js/generated/
Files:
pkg/js/libs/redis/redis.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Lint
🔇 Additional comments (1)
pkg/js/libs/redis/redis.go (1)
174-183: LGTM! Clear documentation of backward compatibility.The function signature and documentation clearly explain both the old and new usage patterns, making it easy for users to understand the changes.
|
This pull request has been automatically marked as stale due to inactivity. It will be closed in 7 days if no further activity occurs. Please update if you wish to keep it open. |
github-actions
Bot
added
the
Status: Stale
|
This pull request has been automatically closed due to inactivity. If you think this is a mistake or would like to continue working on it, please comment or feel free to reopen it. |
github-actions
Bot
added
the
Status: Abandoned
github-actions
Bot
removed
Status: Abandoned
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@internal/tests/integration/javascript_test.go`:
- Around line 115-117: The test currently sets the container image with
Repository: "redis" and Tag: "latest", which makes the integration runs
non-deterministic; update the test to pin the Redis image to a specific stable
version or digest (e.g., replace Tag: "latest" with a concrete semver like
"7.0.11" or use an immutable digest) for both the Redis password brute and Redis
Lua script cases so test environments are reproducible; locate the occurrences
of Repository: "redis", Tag: "latest" in the test definitions
(integration/javascript_test.go) and change them to the chosen pinned tag or
digest and update any related test documentation/comments if present.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7ac038ea-2bbd-47ae-83c6-1af8a4f2ab43
⛔ Files ignored due to path filters (1)
internal/tests/integration/testdata/protocols/javascript/redis-lua-script.yamlis excluded by!**/*.yaml
📒 Files selected for processing (1)
internal/tests/integration/javascript_test.go
The two log() calls each emitted a [JS] stdout line that the integration harness counted as a result, yielding 3 results vs the expected 1. The matcher reads `response` from the final expression value (not stdout), so returning the get RunLuaScript call directly keeps the assertion intact with a single result.
|
Test failed, |
Avoid shared templates-dir update races across parallel go test packages and ignore known leveldb/ratelimit/memguardian goroutines in goleak.
Keep goleak ignores, drop DisableUpdateCheck so CI can install templates, and serialize UpdateIfOutdated across processes.
2 participants
Proposed changes
Closes #4790
Checklist
Summary by CodeRabbit