Python - Implement JSON.MGET command #2507

BoazBD · 2024-10-23T07:15:17Z

This Pull Request is linked to issue (URL): $645

Checklist

Before submitting the PR make sure the following are checked:

This Pull Request is related to one issue.
Commit message has a detailed description of what changed and why.
Tests are added or updated.
CHANGELOG.md and documentation files are updated.
Destination branch is correct - main or release
Commits will be squashed upon merging.

Yury-Fridlyand

You need to update glide core to send this command to multiple nodes if multiple keys given and then aggregate the response (same as for mget command).
Please do that or wait for my implementation of json.mget for java with that change (coming soon)

python/python/glide/async_commands/server_modules/json.py

… into python/json.mget

Signed-off-by: BoazBD <boazbd@amazon.com>

python/python/glide/async_commands/server_modules/json.py

python/python/tests/tests_server_modules/test_json.py

Co-authored-by: Shoham Elias <116083498+shohamazon@users.noreply.github.com> Signed-off-by: BoazBD <50696333+BoazBD@users.noreply.github.com>

Signed-off-by: BoazBD <boazbd@amazon.com>

python/python/glide/async_commands/server_modules/json.py

python/python/tests/tests_server_modules/test_json.py

Signed-off-by: BoazBD <boazbd@amazon.com>

jamesx-improving · 2024-11-12T20:43:19Z

python/python/tests/tests_server_modules/test_json.py

+            glide_client,
+            [key1, key2],
+        )
+        assert result == [None]


path (Optional[TEncodable]): The path within the JSON documents. Default is root `$`.

If path really defaults to root, wouldn't the expected result of this test be the same as the one with $ as path, on linie 191?

If this test is indeed passing, maybe it is the doc that need to be fixed?

After investigation, this is straight up wrong. path should be required, not optional. Please see comment on parameter path of the mget function for detail.

youre right, lets stick with Defaults to None.

Yury-Fridlyand

Please update tests too

python/python/glide/async_commands/server_modules/json.py

jamesx-improving · 2024-11-12T21:45:24Z

python/python/tests/tests_server_modules/test_json.py

+            glide_client,
+            [key1, key2],
+        )
+        assert result == [None]


After investigation, this is straight up wrong. path should be required, not optional. Please see comment on parameter path of the mget function for detail.

python/python/glide/async_commands/server_modules/json.py

Signed-off-by: BoazBD <boazbd@amazon.com>

BoazBD changed the base branch from main to release-1.2 October 23, 2024 07:15

BoazBD force-pushed the python/json.mget branch from 5afa929 to f3019e1 Compare October 23, 2024 07:34

BoazBD had a problem deploying to AWS_ACTIONS October 23, 2024 07:34 — with GitHub Actions Failure

BoazBD had a problem deploying to AWS_ACTIONS October 23, 2024 07:34 — with GitHub Actions Error

BoazBD had a problem deploying to AWS_ACTIONS October 23, 2024 07:34 — with GitHub Actions Failure

BoazBD force-pushed the python/json.mget branch from f3019e1 to 25b6586 Compare October 23, 2024 08:11

BoazBD had a problem deploying to AWS_ACTIONS October 23, 2024 08:11 — with GitHub Actions Error

BoazBD force-pushed the python/json.mget branch from 25b6586 to 3b35adc Compare October 23, 2024 08:15

BoazBD had a problem deploying to AWS_ACTIONS October 23, 2024 08:16 — with GitHub Actions Error

BoazBD force-pushed the python/json.mget branch from 3b35adc to da6dbf9 Compare October 23, 2024 08:17

BoazBD had a problem deploying to AWS_ACTIONS October 23, 2024 08:18 — with GitHub Actions Error

BoazBD had a problem deploying to AWS_ACTIONS October 23, 2024 08:25 — with GitHub Actions Error

BoazBD force-pushed the python/json.mget branch from 844ffc3 to 6fbf923 Compare October 23, 2024 08:34

BoazBD had a problem deploying to AWS_ACTIONS October 23, 2024 08:34 — with GitHub Actions Error

BoazBD force-pushed the python/json.mget branch from 6fbf923 to 2107955 Compare October 23, 2024 09:26

BoazBD had a problem deploying to AWS_ACTIONS October 23, 2024 09:26 — with GitHub Actions Error

BoazBD force-pushed the python/json.mget branch from 2107955 to 5c14340 Compare October 23, 2024 09:27

BoazBD had a problem deploying to AWS_ACTIONS October 23, 2024 09:28 — with GitHub Actions Error

BoazBD force-pushed the python/json.mget branch from 5c14340 to cb65830 Compare October 23, 2024 09:29

BoazBD had a problem deploying to AWS_ACTIONS October 23, 2024 09:30 — with GitHub Actions Error

BoazBD marked this pull request as ready for review October 23, 2024 09:41

BoazBD requested a review from a team as a code owner October 23, 2024 09:41

BoazBD requested a review from shohamazon October 23, 2024 09:41

Yury-Fridlyand requested changes Oct 24, 2024

View reviewed changes

python/python/glide/async_commands/server_modules/json.py Outdated Show resolved Hide resolved

python/python/glide/async_commands/server_modules/json.py Show resolved Hide resolved

Yury-Fridlyand reviewed Oct 24, 2024

View reviewed changes

python/python/glide/async_commands/server_modules/json.py Outdated Show resolved Hide resolved

Yury-Fridlyand reviewed Oct 24, 2024

View reviewed changes

python/python/glide/async_commands/server_modules/json.py Outdated Show resolved Hide resolved

Yury-Fridlyand reviewed Oct 24, 2024

View reviewed changes

python/python/glide/async_commands/server_modules/json.py Show resolved Hide resolved

BoazBD had a problem deploying to AWS_ACTIONS October 27, 2024 09:06 — with GitHub Actions Error

BoazBD force-pushed the python/json.mget branch from fbd9911 to 99f6049 Compare October 27, 2024 09:15

shohamazon mentioned this pull request Oct 27, 2024

Json module for Python #645

Closed

22 tasks

BoazBD had a problem deploying to AWS_ACTIONS October 27, 2024 14:08 — with GitHub Actions Failure

BoazBD added 4 commits November 10, 2024 11:27

Merge branch 'release-1.2' of https://github.com/valkey-io/valkey-glide…

d593cb6

… into python/json.mget

Merge branch 'release-1.2' of https://github.com/valkey-io/valkey-glide…

f45fcb5

… into python/json.mget

Merge branch 'release-1.2' of https://github.com/valkey-io/valkey-glide…

e93c0a4

… into python/json.mget

update due to multi-slot requests support

dd5a0b2

Signed-off-by: BoazBD <boazbd@amazon.com>

BoazBD had a problem deploying to AWS_ACTIONS November 12, 2024 07:49 — with GitHub Actions Error

BoazBD requested a review from Yury-Fridlyand November 12, 2024 11:31

shohamazon reviewed Nov 12, 2024

View reviewed changes

Apply suggestions from code review

964fee9

Co-authored-by: Shoham Elias <116083498+shohamazon@users.noreply.github.com> Signed-off-by: BoazBD <50696333+BoazBD@users.noreply.github.com>

BoazBD had a problem deploying to AWS_ACTIONS November 12, 2024 12:51 — with GitHub Actions Error

fix shoham comments

b9b20cd

Signed-off-by: BoazBD <boazbd@amazon.com>

BoazBD had a problem deploying to AWS_ACTIONS November 12, 2024 12:53 — with GitHub Actions Error

shohamazon reviewed Nov 12, 2024

View reviewed changes

python/python/glide/async_commands/server_modules/json.py Outdated Show resolved Hide resolved

shohamazon reviewed Nov 12, 2024

View reviewed changes

python/python/tests/tests_server_modules/test_json.py Show resolved Hide resolved

shohamazon reviewed Nov 12, 2024

View reviewed changes

python/python/tests/tests_server_modules/test_json.py Show resolved Hide resolved

Improve documentation and tests

278673e

Signed-off-by: BoazBD <boazbd@amazon.com>

BoazBD temporarily deployed to AWS_ACTIONS November 12, 2024 14:07 — with GitHub Actions Inactive

Yury-Fridlyand approved these changes Nov 12, 2024

View reviewed changes

jamesx-improving approved these changes Nov 12, 2024

View reviewed changes

Yury-Fridlyand requested changes Nov 12, 2024

View reviewed changes

python/python/glide/async_commands/server_modules/json.py Outdated Show resolved Hide resolved

python/python/glide/async_commands/server_modules/json.py Outdated Show resolved Hide resolved

python/python/glide/async_commands/server_modules/json.py Outdated Show resolved Hide resolved

jamesx-improving requested changes Nov 12, 2024

View reviewed changes

make path mandatory

2f9e62f

Signed-off-by: BoazBD <boazbd@amazon.com>

BoazBD temporarily deployed to AWS_ACTIONS November 13, 2024 07:50 — with GitHub Actions Inactive

BoazBD requested review from jamesx-improving, Yury-Fridlyand and shohamazon November 13, 2024 15:44

jamesx-improving approved these changes Nov 13, 2024

View reviewed changes

Yury-Fridlyand approved these changes Nov 13, 2024

View reviewed changes

BoazBD merged commit 664cc8e into release-1.2 Nov 14, 2024
21 checks passed

BoazBD deleted the python/json.mget branch November 14, 2024 06:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Python - Implement JSON.MGET command #2507

Python - Implement JSON.MGET command #2507

BoazBD commented Oct 23, 2024 •

edited

Loading

Yury-Fridlyand left a comment

jamesx-improving Nov 12, 2024

jamesx-improving Nov 12, 2024

shohamazon Nov 13, 2024

Yury-Fridlyand left a comment

jamesx-improving Nov 12, 2024

Python - Implement JSON.MGET command #2507

Python - Implement JSON.MGET command #2507

Conversation

BoazBD commented Oct 23, 2024 • edited Loading

Checklist

Yury-Fridlyand left a comment

Choose a reason for hiding this comment

jamesx-improving Nov 12, 2024

Choose a reason for hiding this comment

jamesx-improving Nov 12, 2024

Choose a reason for hiding this comment

shohamazon Nov 13, 2024

Choose a reason for hiding this comment

Yury-Fridlyand left a comment

Choose a reason for hiding this comment

jamesx-improving Nov 12, 2024

Choose a reason for hiding this comment

BoazBD commented Oct 23, 2024 •

edited

Loading