Add JSON.MSET #944

gkorland · 2023-03-21T15:59:12Z

No description provided.

src/commands.rs

codecov · 2023-03-21T16:48:03Z

Codecov Report

Patch coverage: 93.54% and project coverage change: +0.07 🎉

Comparison is base (7361ad4) 81.45% compared to head (a8fc4e4) 81.53%.

❗ Current head a8fc4e4 differs from pull request most recent head 879c295. Consider uploading reports for the commit 879c295 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #944      +/-   ##
==========================================
+ Coverage   81.45%   81.53%   +0.07%     
==========================================
  Files          15       15              
  Lines        3894     3954      +60     
==========================================
+ Hits         3172     3224      +52     
- Misses        722      730       +8

Impacted Files	Coverage Δ
src/lib.rs	`86.53% <ø> (ø)`
src/commands.rs	`95.68% <93.54%> (+<0.01%)`	⬆️

... and 2 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

src/commands.rs

MeirShpilraien

Some small comments.

src/commands.rs

MeirShpilraien · 2023-04-20T07:18:50Z

src/commands.rs

+    REDIS_OK
+}
+
+fn apply_updates<M: Manager>(


Just to make sure, you just extracted the code above into a function right?

one change, the for ui in update_info now doesn't stop on the first error

@gkorland

the for ui in update_info now doesn't stop on the first error

Consider testing that a specific change, which cannot be done, does not prevent apply_changes for previously done changes, e.g., once first change is done, second cannot be done, but third should be done as well.

json.set a $ '{"x": {"y":[10,20], "z":[30,40]}}' json.set b $ '{"x": 60}' json.mset a $.x '{}' a '$.x.z[1]' '50' b $.x '70'

Maybe also on the same key

json.mset a $.x '{}' a '$.x.z[1]' '50' a $.u '70'

Since it is also fixed for json_set - now it may notify on a similar scenario with multi-value (using wildcards, etc.) , while it did not notify before - could affect RediSearch results for example, but this would be a progression and is not considered a breaking change.
Agree?

An example for the problem with a single json.set (using RediSearch as a subscriber for keyspace notifications)

127.0.0.1:6379> json.set k1 $ '{"x":{"x":{"y":[4,5,6]}}}' OK 127.0.0.1:6379> ft.create idx on json schema '$..y[*]' as y numeric OK 127.0.0.1:6379> ft.search idx '@y:[4 5]' 1) (integer) 1 2) "k1" 3) 1) "$" 2) "{\"x\":{\"x\":{\"y\":[4,5,6]}}}"

The following json.set was failing in the middle and not notifying on changes, so the index was not being updated causing search to return a stale result:

127.0.0.1:6379> json.set k1 '$..x' '1' (nil) 127.0.0.1:6379> json.get k1 $ "[{\"x\":1}]" 127.0.0.1:6379> ft.search idx '@y:[4 5]' 1) (integer) 1 2) "k1" 3) 1) "$" 2) "{\"x\":1}"

With this PR it should be fixed ☝🏼 (search should return an empty result)
And once RediSearch/RediSearch#3507 is merged, it should also work with json.mset, e.g.,

json.mset k1 $ '{"x":{"x":{"y":[4,5,6]}}}' k1 '$..x' '1'

And then ft.search idx '@y:[4 5]' should return an empty result
And with

json.mset k1 $ '{"x":{"x":{"y":[4,5,6]}}}' k1 '$..x' '1' k1 '$.y' '[4, 5, 6]'

The same search should return k1.

I suppose such tests should be added to RediSearch/RediSearch#3507

@gkorland Keyspace notification can also be tested in this PR (without RediSearch) as in

RedisJSON/tests/pytest/test_keyspace_notifications.py

Line 14 in 0b898be

def test_keyspace_set(env):

MeirShpilraien · 2023-04-20T07:22:30Z

src/commands.rs

+                !update_info.is_empty() && apply_updates::<M>(&mut redis_key, value, update_info)
+            } else {
+                // In case it is a root path
+                redis_key.set_value(Vec::new(), value).unwrap()


Suggested change

redis_key.set_value(Vec::new(), value).unwrap()

redis_key.set_value(Vec::new(), value).expect("<Some informative message>")

I replaced it with a fold so if one fails an error will be returned

tests/pytest/test.py

sonatype-lift · 2023-04-23T12:44:15Z

tests/pytest/test_keyspace_notifications.py

@@ -30,6 +30,12 @@ def test_keyspace_set(env):
        assert_msg(env, pubsub.get_message(timeout=1), 'pmessage', 'json.set')
        assert_msg(env, pubsub.get_message(timeout=1), 'pmessage', 'test_key')

+        env.assertEqual('OK', r.execute_command('JSON.MSET', 'test_key', '$.foo', '"gogo"', 'test_key{test_key}', '$', '{"a":"fufu"}'))


E501: line too long (135 > 79 characters)

ℹ️ Expand to see all @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage

@sonatype-lift ignore Leave out the above finding from this PR

@sonatype-lift ignoreall Leave out all the existing findings from this PR

@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
_{Click here to add LiftBot to another repo.}

sonatype-lift · 2023-04-23T12:44:17Z

tests/pytest/test_keyspace_notifications.py

+        assert_msg(env, pubsub.get_message(timeout=1), 'pmessage', 'json.mset')
+        assert_msg(env, pubsub.get_message(timeout=1), 'pmessage', 'test_key')
+        assert_msg(env, pubsub.get_message(timeout=1), 'pmessage', 'json.mset')
+        assert_msg(env, pubsub.get_message(timeout=1), 'pmessage', 'test_key{test_key}')


E501: line too long (88 > 79 characters)

ℹ️ Expand to see all @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage

@sonatype-lift ignore Leave out the above finding from this PR

@sonatype-lift ignoreall Leave out all the existing findings from this PR

@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
_{Click here to add LiftBot to another repo.}

add json.mset

2e2c95e

gkorland requested a review from oshadmi March 21, 2023 15:59

add tests

ff5d831

gkorland requested a review from MeirShpilraien March 21, 2023 16:41

MeirShpilraien reviewed Mar 21, 2023

View reviewed changes

src/commands.rs Outdated Show resolved Hide resolved

MeirShpilraien reviewed Mar 21, 2023

View reviewed changes

src/commands.rs Outdated Show resolved Hide resolved

MeirShpilraien reviewed Mar 21, 2023

View reviewed changes

src/commands.rs Show resolved Hide resolved

MeirShpilraien reviewed Mar 21, 2023

View reviewed changes

oshadmi reviewed Mar 21, 2023

View reviewed changes

src/commands.rs Show resolved Hide resolved

oshadmi reviewed Mar 21, 2023

View reviewed changes

src/commands.rs Outdated Show resolved Hide resolved

oshadmi reviewed Mar 21, 2023

View reviewed changes

src/commands.rs Outdated Show resolved Hide resolved

oshadmi reviewed Mar 21, 2023

View reviewed changes

src/commands.rs Outdated Show resolved Hide resolved

gkorland added 3 commits March 22, 2023 09:59

fix key steps and applay changes on root update too

55a1be3

Merge branch 'master' into gkorland-mset

e786297

don't fail in the middle of an update

9480263

gkorland linked an issue Mar 22, 2023 that may be closed by this pull request

Review the code in fn json_set when updating a doc - can apply_changes be missed? #821

Closed

fix update dynamic path

a8fc4e4

gkorland requested review from MeirShpilraien and oshadmi March 23, 2023 08:44

gkorland added the feature label Mar 23, 2023

gkorland marked this pull request as ready for review March 27, 2023 13:38

gkorland mentioned this pull request Mar 28, 2023

Add support for json.merge and json.mset RediSearch/RediSearch#3507

Merged

MeirShpilraien reviewed Apr 20, 2023

View reviewed changes

return an error if set_value or apply_changes fail

512e74a

sonatype-lift bot reviewed Apr 20, 2023

View reviewed changes

tests/pytest/test.py Show resolved Hide resolved

sonatype-lift bot reviewed Apr 20, 2023

View reviewed changes

tests/pytest/test.py Show resolved Hide resolved

sonatype-lift bot reviewed Apr 20, 2023

View reviewed changes

tests/pytest/test.py Outdated Show resolved Hide resolved

sonatype-lift bot reviewed Apr 20, 2023

View reviewed changes

tests/pytest/test.py Outdated Show resolved Hide resolved

sonatype-lift bot reviewed Apr 20, 2023

View reviewed changes

tests/pytest/test.py Outdated Show resolved Hide resolved

gkorland added 2 commits April 23, 2023 10:15

add tests for MSET

7ef9422

Merge branch 'master' into gkorland-mset

4491b42

sonatype-lift bot reviewed Apr 23, 2023

View reviewed changes

tests/pytest/test.py Show resolved Hide resolved

sonatype-lift bot reviewed Apr 23, 2023

View reviewed changes

tests/pytest/test.py Show resolved Hide resolved

fix tests

a3e7f78

sonatype-lift bot reviewed Apr 23, 2023

View reviewed changes

tests/pytest/test.py Show resolved Hide resolved

oshadmi previously approved these changes Apr 23, 2023

View reviewed changes

fix multi key test for cluster

879c295

gkorland dismissed oshadmi’s stale review via 879c295 April 23, 2023 12:38

sonatype-lift bot reviewed Apr 23, 2023

View reviewed changes

gkorland merged commit 47e05e4 into master Apr 24, 2023
11 of 12 checks passed

gkorland deleted the gkorland-mset branch April 24, 2023 06:14

gkorland mentioned this pull request Apr 25, 2023

last key index should be -1 #973

Merged

gkorland changed the title ~~add json.mset~~ Add JSON.MSET May 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add JSON.MSET #944

Add JSON.MSET #944

gkorland commented Mar 21, 2023

codecov bot commented Mar 21, 2023 •

edited

MeirShpilraien left a comment

MeirShpilraien Apr 20, 2023

gkorland Apr 20, 2023

oshadmi Apr 20, 2023

oshadmi Apr 20, 2023

oshadmi Apr 20, 2023

oshadmi Apr 20, 2023

oshadmi Apr 20, 2023

oshadmi Apr 20, 2023

MeirShpilraien Apr 20, 2023

gkorland Apr 20, 2023 •

edited

sonatype-lift bot Apr 23, 2023

sonatype-lift bot Apr 23, 2023

	redis_key.set_value(Vec::new(), value).unwrap()
	redis_key.set_value(Vec::new(), value).expect("<Some informative message>")

Command	Usage
`@sonatype-lift ignore`	Leave out the above finding from this PR
`@sonatype-lift ignoreall`	Leave out all the existing findings from this PR
`@sonatype-lift exclude <file\|issue\|path\|tool>`	Exclude specified `file\|issue\|path\|tool` from Lift findings by updating your config.toml file

Add JSON.MSET #944

Add JSON.MSET #944

Conversation

gkorland commented Mar 21, 2023

codecov bot commented Mar 21, 2023 • edited

Codecov Report

MeirShpilraien left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkorland Apr 20, 2023 • edited

Choose a reason for hiding this comment

sonatype-lift bot Apr 23, 2023

Choose a reason for hiding this comment

sonatype-lift bot Apr 23, 2023

Choose a reason for hiding this comment

codecov bot commented Mar 21, 2023 •

edited

gkorland Apr 20, 2023 •

edited