-
Notifications
You must be signed in to change notification settings - Fork 1k
feat(json): add JSON command support #1333
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
ae1c729 to
8333b77
Compare
byroot
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- None of the new methods are documented.
- The feature is only added to standalone redis, not to cluster or distributed.
lib/redis/commands/json.rb
Outdated
| args.concat(paths) unless paths.empty? | ||
| result = parse_json(send_command(args)) | ||
| # Unwrap single-element arrays for JSONPath queries | ||
| result.is_a?(Array) && result.length == 1 ? result.first : result |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how convenient that really is. If I'm working with a list of paths I received, and sometimes it is of length 1, sometimes more, I get inconsistent results.
Might be better to accept either a single path or an array, and return a consistent type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed:
-
Documentation: Added YARD docs to all methods (commit 6dd3596)
-
Distributed support: Implemented all JSON methods in Redis::Distributed using the node_for(key) delegation pattern. Multi-key operations (json_mget, json_mset) properly use ensure_same_node to enforce same-node requirements (commit 7e8906d)
-
Cluster support: Added test suite for Redis::Cluster. Since Cluster inherits from Redis, it automatically has access to all JSON commands without additional implementation (commit 1b1ef5c)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how convenient that really is. If I'm working with a list of paths I received, and sometimes it is of length 1, sometimes more, I get inconsistent results.
Might be better to accept either a single path or an array, and return a consistent type.
Yup. I've removed the unwrapping logic, see (commit 6dd3596).
The methods now always return the raw parsed JSON response from Redis without conditional unwrapping.
lib/redis/commands/json.rb
Outdated
| end | ||
|
|
||
| def json_arrpop(key, path = '$', index = -1) | ||
| parse_json(send_command(['JSON.ARRPOP', key, path, index.to_s])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| parse_json(send_command(['JSON.ARRPOP', key, path, index.to_s])) | |
| parse_json(send_command(['JSON.ARRPOP', key, path, Integer(index).to_s])) |
We should enforce the type of Integer arguments for consistency with the rest of the codebase (e.g. see linsert).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed! Changed json_arrpop to use Integer(index) for proper type enforcement, see (commit 6dd3596).
|
|
||
| private | ||
|
|
||
| def parse_json(value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned before, that helper is a smell. We should know what type to expect in return of the command we emit, instead of having a generic helper that can parse about anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The challenge with JSON commands is that Redis returns JSON-encoded strings that need parsing, and the return type varies by command and path query:
- json_get can return objects, arrays, strings, numbers, booleans, or null
- Array operations can return single values or arrays depending on JSONPath queries
- Some commands like json_arrpop return the actual JSON value, not metadata
I kept the helper with proper error handling (raises Redis::JSONParseError on parse failures) as it centralizes the JSON parsing logic and symbolize_names option. However, I'm open to refactoring this if you have a preferred pattern - perhaps individual parsing lambdas similar to Hashify/Floatify in Commands?
- Add YARD documentation for all JSON methods - Remove inconsistent unwrapping behavior from json_get/json_mget/json_numincrby/json_nummultby - Use Integer() to enforce type for json_arrpop index parameter - Clarify json_forget as Redis command alias Addresses reviewer feedback on PR #1333 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
Implement all JSON commands with complete feature parity to redis-py: - JSON.GET, JSON.SET, JSON.DEL, JSON.MGET, JSON.MSET - JSON.ARRAPPEND, JSON.ARRINDEX, JSON.ARRINSERT, JSON.ARRLEN, JSON.ARRPOP, JSON.ARRTRIM - JSON.OBJKEYS, JSON.OBJLEN, JSON.STRLEN, JSON.STRAPPEND - JSON.NUMINCRBY, JSON.NUMMULTBY - JSON.TOGGLE, JSON.CLEAR, JSON.TYPE - JSON.RESP, JSON.DEBUG - Support for both legacy (.) and modern (\$) JSONPath syntax - Add json_forget alias for json_del Testing: - Add comprehensive test suite with 97 tests covering all commands - All edge cases and error conditions tested - JSONPath query tests for both syntaxes - Array and object manipulation tests - Numeric operation tests Documentation: - Add json_tutorial.rb example demonstrating all features - Add search_with_json.rb example showing JSON with Search - Include practical examples for common use cases
- Add YARD documentation for all JSON methods - Remove inconsistent unwrapping behavior from json_get/json_mget/json_numincrby/json_nummultby - Use Integer() to enforce type for json_arrpop index parameter - Clarify json_forget as Redis command alias
Adds all JSON commands to the distributed Redis implementation, delegating single-key operations to the appropriate node and enforcing same-node requirements for multi-key operations (json_mget, json_mset). Includes comprehensive test suite for distributed JSON operations with tests for both same-node (using key tags) and different-node scenarios. This addresses the reviewer feedback that JSON support was only added to standalone redis, not to cluster or distributed.
Adds comprehensive test suite for JSON operations in cluster mode. Since Redis::Cluster inherits from Redis, it automatically includes all JSON commands from Redis::Commands::JSON without requiring additional implementation. This completes the cluster support requirement from the reviewer feedback.
|
You just copy pasted all the tests for distributed. That's not how it should be done. Please look at the |
Implement all JSON commands with complete feature parity to redis-py:
Testing:
Documentation: