Python: adds json module and JSON.SET JSON.GET commands #1056

shohamazon · 2024-03-03T12:16:44Z

Issue #, if available:

Description of changes:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

python/python/glide/async_commands/redis_modules/json/commands.py

shachlanAmazon · 2024-03-03T15:59:56Z

python/python/glide/async_commands/redis_modules/json/commands.py

+        client: TRedisClient,
+        key: str,
+        path: str,
+        value: TJson,


this is always translated to a string, and always returned as a string, right?
If so, isn't it a bit confusing that you set a dictionary and get a string?

true, but I think it could be nice to be able to pass a dictionary (or another complex object) and also to be able to pass a string representation of the dictionary.

I agree with Shachar, if we expect that the user will use python's json module to convert the response from a string to a complex object, it's relevant to the passed value too. If we except only string as a value, we can avoid importing 'json' module, which might not be in the standard libraries in other languages.

barshaul · 2024-03-04T11:24:57Z

python/pytest.ini

@@ -1,4 +1,4 @@
 [pytest]
 markers =
    smoke_test: mark a test as a build verification testing.
-addopts = -k "not test_redis_modules.py"
+addopts = -k "not test_redis_modules.py and not test_json.py"


instead of adding this for each new module we add, can you check if we can move all module tests into a folder (tests/modules) and skip the whole folder by default?

barshaul · 2024-03-04T12:57:57Z

python/python/glide/async_commands/redis_modules/json/commands.py

+TJson = Union[str, int, float, bool, None, List["TJson"], Dict[str, "TJson"]]
+
+
+class Json:


This class doesn't save any state, not sure what's the gain of using it as a class rather than just a module, e.g.:

json.py

def set(): ... def get(): ...

main.py

import json json.set() json.get()

python/python/glide/async_commands/redis_modules/json/commands.py

Yury-Fridlyand · 2024-03-04T18:57:21Z

python/python/glide/async_commands/redis_modules/json/commands.py

+                The key will be modified only if `value` is added as the last child in the specified `path`, or if the specified `path` acts as the parent of a new child being added.
+            value (TJSON): The value to set at the specific path.
+            set_condition (Optional[ConditionalChange]): Set the value only if the given condition is met (within the key or path).
+                Equivalent to [`XX` | `NX`] in the Redis API. Defaults to None.


Suggested change

Equivalent to [`XX` | `NX`] in the Redis API. Defaults to None.

Equivalent to `XX`/`NX` in the Redis API. Defaults to None.

python/python/glide/async_commands/redis_modules/json/commands.py

Yury-Fridlyand · 2024-03-04T18:58:04Z

python/python/glide/async_commands/redis_modules/json/commands.py

+                "[]"  # Returns an empty array since the path '$.non_existing_path' does not exist in the JSON document stored at 'doc'.
+            >>> import json
+            >>> json.loads(await json_get(client, "doc", "$"))
+                [{"a": 1.0, "b" :2}]  # JSON object retrieved from the key "doc" using json.loads()


Suggested change

[{"a": 1.0, "b" :2}] # JSON object retrieved from the key "doc" using json.loads()

[{"a": 1.0, "b" :2}] # JSON object retrieved from the key "doc" using `json.loads()`

Yury-Fridlyand · 2024-03-04T18:58:47Z

python/python/glide/async_commands/redis_modules/json/commands.py

+        indent: Optional[str] = None,
+        newline: Optional[str] = None,
+        space: Optional[str] = None,


Do you want to merge this into JsonOptions?

Regarding Yuri's proposal, are those options being utilized in other JSON functions as well?

python/python/tests/test_json.py

Yury-Fridlyand · 2024-03-04T19:00:01Z

Transaction test?

barshaul · 2024-03-05T12:59:44Z

python/python/glide/async_commands/redis_modules/json/commands.py

+                If value isn't set because of `set_condition`, return None.
+
+        Examples:
+            >>> await json_set(client, "doc", "$", {'a': 1.0, 'b': 2})


Can you expand json examples to be more informative, for example:

import json value = {'a': 1.0, 'b': 2} json_str = json.dumps(value) await json.set(client, "doc", "$", json_str) 'OK' # Indicates successful setting of the value at path '$' in the key stored at 'doc'.

and for json.get show how you use json to decode the response to python object

(we can have a single example and remove the second you did here)

barshaul · 2024-03-05T13:03:00Z

python/python/glide/async_commands/redis_modules/json/commands.py

+        Examples:
+            >>> await json_set(client, "doc", "$", {'a': 1.0, 'b': 2})
+                'OK'  # Indicates successful setting of the value at path '$' in the key stored at 'doc'.
+            >>> await json_set(client, "doc", "$.a", 1.5 , ConditionalChange.ONLY_IF_DOES_NOT_EXIST)


(it should be changed after we'll change Json to a module instead of a class - but note that json_set function is incorrect)

barshaul · 2024-03-05T13:06:25Z

python/python/glide/async_commands/redis_modules/json/commands.py

+                "{\n \"$.a\": [\n  1.0\n ],\n \"$.b\": [\n  2\n ]\n}"  # Returns the values at paths '$.a' and '$.b' in the JSON document stored at 'doc', with specified formatting options.
+            >>> await json_get(client, "doc", "$.non_existing_path")
+                "[]"  # Returns an empty array since the path '$.non_existing_path' does not exist in the JSON document stored at 'doc'.
+            >>> import json


lets do the import on top and have one/two examples only that use json.loads

barshaul

Round

shohamazon · 2024-03-05T14:28:16Z

@barshaul round

barshaul

Will continue reviewing tests tomorrow

barshaul · 2024-03-05T16:31:25Z

python/pytest.ini

@@ -1,4 +1,4 @@
 [pytest]
 markers =
    smoke_test: mark a test as a build verification testing.
-addopts = -k "not test_redis_modules.py"
+addopts = --ignore "tests_redis_modules"


Since it's already under tests folder, please rename the folder name to "redis_modules"

What about changing the GitHub action that tests modules accordingly?

no need to change a thing, the GitHub action overrides addopts

( pytest --asyncio-mode=auto --override-ini=addopts= --load-module=$GITHUB_WORKSPACE/redisearch.so --load-module=$GITHUB_WORKSPACE/redisjson.so)

barshaul · 2024-03-05T16:35:57Z

python/python/glide/async_commands/redis_modules/json/json.py

@@ -0,0 +1,104 @@
+# Copyright GLIDE-for-Redis Project Contributors - SPDX Identifier: Apache-2.0


Lets remove the json folder, we can always add it later on if the json module will need to be extended

General note: please add json to the all in glide/init

barshaul · 2024-03-05T16:40:17Z

python/python/glide/async_commands/redis_modules/json/json.py

+            Equivalent to [`XX` | `NX`] in the Redis API. Defaults to None.
+
+    Returns:
+        Optional[TOK]: If the value is successfully set, return OK.


Minor: return-> returns
(In both lines)

barshaul · 2024-03-05T16:41:01Z

python/python/glide/async_commands/redis_modules/json/json.py

+            If value isn't set because of `set_condition`, return None.
+
+    Examples:
+        >>> from glide.async_commands.json import json


After adding it to all, change to '''
from glide import json as redisJson
Import json
...

barshaul · 2024-03-05T16:43:19Z

python/python/glide/async_commands/redis_modules/json/json.py

+
+    Examples:
+        >>> from glide.async_commands.json import json
+        >>> import json as OuterJson


(Same as in the comment above - don't use OuterJson)

barshaul · 2024-03-05T16:46:05Z

python/python/glide/async_commands/redis_modules/json/json.py

+    Args:
+        client (TRedisClient): The Redis client to execute the command.
+        key (str): The key of the JSON document.
+        paths (ptional[Union[str, List[str]]]): The path or list of paths within the JSON document.


Why is it defaults to None? What is the default path?

Minor: ptional-> optional

Default is root $.

barshaul · 2024-03-05T16:47:41Z

python/python/glide/async_commands/redis_modules/json/json.py

+
+    Examples:
+        >>> import json as OuterJson
+        >>> import json


Same as above

barshaul · 2024-03-05T16:49:03Z

python/python/glide/async_commands/redis_modules/json/json.py

+        >>> import json as OuterJson
+        >>> import json
+        >>> OuterJson.loads(await json_get(client, "doc", "$"))
+            [{"a": 1.0, "b" :2}]  # JSON object retrieved from the key "doc" using json.loads()


json_get -> json.get

Let's have it in two steps so you'll explain you're getting a jsonstring and you load it to a python object

barshaul · 2024-03-05T16:52:36Z

python/python/glide/async_commands/redis_modules/json/json.py

+            paths = [paths]
+
+        for path in paths:
+            args.append(path)


Why not using extend?

barshaul · 2024-03-06T12:41:52Z

python/python/glide/async_commands/redis_modules/json.py

+    Args:
+        client (TRedisClient): The Redis client to execute the command.
+        key (str): The key of the JSON document.
+        paths (Optional[Union[str, List[str]]]): The path or list of paths within the JSON document.


lets write that the default is root, it feels not readable enough without this info

barshaul · 2024-03-06T13:32:45Z

python/python/tests/tests_redis_modules/test_json.py

+import json as OuterJson
+
+import pytest
+from glide import json


just to be consistent, please use the full import path

barshaul · 2024-03-06T13:38:39Z

python/python/tests/tests_redis_modules/test_json.py

+class TestJson:
+    @pytest.mark.parametrize("cluster_mode", [True, False])
+    @pytest.mark.parametrize("protocol", [ProtocolVersion.RESP2, ProtocolVersion.RESP3])
+    async def test_json_module_loadded(self, redis_client: TRedisClient):


loadded => is_loaded

barshaul · 2024-03-06T13:44:26Z

python/python/tests/tests_redis_modules/test_json.py

+        key = get_random_string(5)
+
+        assert (
+            await json.set(redis_client, key, "$", OuterJson.dumps({"a": 1.0, "b": 2}))


lets save it in a variable, something like:

json_value = {"a": 1.0, "b": 2} await json.set(redis_client, key, "$", OuterJson.dumps(json_value)) ... result = await json.get(redis_client, key, ".") assert isinstance(result, str) assert OuterJson.loads(result) == json_value

please add test with boolean too

barshaul · 2024-03-06T14:26:27Z

python/python/tests/tests_redis_modules/test_redis_modules.py

Can we remove this file? looks like these tests are a part of the specific module tests

barshaul · 2024-03-06T14:29:05Z

glide-core/src/protobuf/redis_request.proto

@@ -125,6 +125,8 @@ enum RequestType {
    XGroupDestroy = 81;


Add this change to CHANGELOG

barshaul · 2024-03-06T14:29:24Z

python/python/glide/async_commands/redis_modules/json.py

+            'OK'  # Indicates successful setting of the value at path '$' in the key stored at 'doc'.
+        >>> await redisJson.get(client, "doc", "$")
+            "[{\"a\":1.0,\"b\":2}]"  # Returns the value at path '$' in the JSON document stored at 'doc'.
+        >>> json_get = await redisJson.get(client, "doc", "$")


remove duplication of json get

barshaul

fix & merge

shohamazon requested a review from a team as a code owner March 3, 2024 12:16

shohamazon added the python Python wrapper label Mar 3, 2024

shohamazon requested a review from barshaul March 3, 2024 14:11

shachlanAmazon reviewed Mar 3, 2024

View reviewed changes

barshaul reviewed Mar 4, 2024

View reviewed changes

shachlanAmazon approved these changes Mar 4, 2024

View reviewed changes

barshaul reviewed Mar 4, 2024

View reviewed changes

Yury-Fridlyand approved these changes Mar 4, 2024

View reviewed changes

barshaul reviewed Mar 5, 2024

View reviewed changes

barshaul requested changes Mar 5, 2024

View reviewed changes

shohamazon force-pushed the python/json branch from e7e1eaa to f65090b Compare March 5, 2024 19:06

barshaul reviewed Mar 6, 2024

View reviewed changes

barshaul approved these changes Mar 6, 2024

View reviewed changes

shohamazon force-pushed the python/json branch 3 times, most recently from 0ab2c95 to e3633d9 Compare March 11, 2024 12:09

shohamazon added 3 commits March 11, 2024 14:17

adds redis json support

c9dc2ae

Python: adds json module and JSON.SET JSON.GET commands

ab5c4be

change commands.py to json.py module

3e6238e

shohamazon added 3 commits March 11, 2024 14:17

change import

b0a3931

add module doc

4163b51

add changelog and few changes

a17d22f

shohamazon force-pushed the python/json branch from e3633d9 to a17d22f Compare March 11, 2024 14:17

shohamazon merged commit ac1c8d5 into valkey-io:main Mar 11, 2024
41 of 44 checks passed

shohamazon deleted the python/json branch March 11, 2024 15:31

cyip10 pushed a commit to Bit-Quill/valkey-glide that referenced this pull request Jun 24, 2024

Python: adds json module and JSON.SET JSON.GET commands (valkey-io#1056)

1d8e2f6

shohamazon mentioned this pull request Oct 28, 2024

Json module for Python #645

Open

22 tasks

		TJson = Union[str, int, float, bool, None, List["TJson"], Dict[str, "TJson"]]


		class Json:

	Equivalent to [`XX` \| `NX`] in the Redis API. Defaults to None.
	Equivalent to `XX`/`NX` in the Redis API. Defaults to None.

	[{"a": 1.0, "b" :2}] # JSON object retrieved from the key "doc" using json.loads()
	[{"a": 1.0, "b" :2}] # JSON object retrieved from the key "doc" using `json.loads()`

		@@ -0,0 +1,104 @@
		# Copyright GLIDE-for-Redis Project Contributors - SPDX Identifier: Apache-2.0

Python: adds json module and JSON.SET JSON.GET commands #1056

Python: adds json module and JSON.SET JSON.GET commands #1056

Conversation

shohamazon commented Mar 3, 2024

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

json.py

main.py

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

Yury-Fridlyand commented Mar 4, 2024

barshaul Mar 5, 2024 • edited Loading

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

barshaul left a comment

Choose a reason for hiding this comment

shohamazon commented Mar 5, 2024

barshaul left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shohamazon Mar 5, 2024 • edited Loading

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

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

barshaul Mar 6, 2024 • edited Loading

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

barshaul left a comment

Choose a reason for hiding this comment

barshaul Mar 5, 2024 •

edited

Loading

shohamazon Mar 5, 2024 •

edited

Loading

barshaul Mar 6, 2024 •

edited

Loading