feature: auto create path for json.set - #1611
Conversation
|
|
|
@emreyalvac, thank you. This is a very welcome addition, but there are some important points to consider:
The best way to address 1. is probably to add a new configuration parameter (RedisJSON currently has none) that users can explicitly enable. We also need a clear formal definition of the new behavior. JSONPath is tricky. Consider the following examples:
|
Thank you for your time. Working on it.. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 3 potential issues.
Reviewed by Cursor Bugbot for commit e42bf47. Configure here.
| let mut current_node = &mut root_obj; | ||
| for key in clean_path.split('.').filter(|s| !s.is_empty()) { | ||
| current_node = &mut current_node[key]; | ||
| } |
There was a problem hiding this comment.
Nested path indexing panics
High Severity
When creating a new key at a multi-segment path, the loop walks every segment with current_node[key]. serde_json inserts Null for missing keys, then the next segment indexes into that non-object value and the module panics. The added test path $.foo.bar hits this path.
Reviewed by Cursor Bugbot for commit e42bf47. Configure here.
There was a problem hiding this comment.
I don't want to work with AI..
| let root_str = serde_json::to_string(&root_obj) | ||
| .map_err(|_| RedisError::Str("ERR JSON serialization error"))?; | ||
|
|
||
| let final_val = manager.from_str(&root_str, Format::JSON, true, fpha_type)?; |
There was a problem hiding this comment.
Ignores FORMAT on new key
Medium Severity
For a missing key and non-root path, the value is parsed again with serde_json::from_str and manager.from_str(..., Format::JSON, ...), even though json_set_command_impl already parsed value with the caller’s format into val for the root case. Non-JSON FORMAT (e.g. BSON) is ignored on this branch.
Reviewed by Cursor Bugbot for commit e42bf47. Configure here.
There was a problem hiding this comment.
I don't want to work with AI..
| let mut current_node = &mut root_obj; | ||
| for key in clean_path.split('.').filter(|s| !s.is_empty()) { | ||
| current_node = &mut current_node[key]; | ||
| } |
There was a problem hiding this comment.
Dot split breaks JSONPath
Medium Severity
Missing-key path handling strips $/. and splits on . instead of using the compiled JSONPath used elsewhere (find_add_paths / compile). Array indices, bracket notation, and static-path validation are not applied, so some paths create wrong shapes or succeed where the existing API would error.
Reviewed by Cursor Bugbot for commit e42bf47. Configure here.
There was a problem hiding this comment.
I don't want to work with AI..
3 participants
This PR adds an auto-create path feature to JSON.SET to handle missing nested paths automatically. Instead of throwing an error, it now creates the intermediate structures on the fly.
#27 #1388
Note
Medium Risk
Behavior change for clients that relied on the old error when setting deep paths on absent keys; the dot-path bootstrap differs from JSONPath-based updates on existing documents.
Overview
JSON.SET on a missing key with a non-root path no longer returns
ERR new objects must be created at the root. It now materializes a new document by walking dot-separated path segments under an empty root object, assigns the parsed JSON at the leaf, and stores the result like a normal root set (including keyspace notification).Example:
JSON.SET key $.foo.bar '"baz"'on a deleted key yields{"foo":{"bar":"baz"}}, covered by a new pytest.Scope note: This applies only to the new-key branch of
JSON.SET. JSON.MERGE / JSON.MSET still require creating new keys at the root. Path handling here is simple dot splitting after stripping$/., not full JSONPath semantics for all path forms.Reviewed by Cursor Bugbot for commit e42bf47. Bugbot is set up for automated code reviews on this repo. Configure here.