Skip to content
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

Python: adds JSON.ARRPOP command #2407

Merged
merged 6 commits into from
Nov 3, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@
* Python: Add `JSON.ARRTRIM` command ([#2457](https://github.com/valkey-io/valkey-glide/pull/2457))
* Python: Add `JSON.ARRAPPEND` command ([#2382](https://github.com/valkey-io/valkey-glide/pull/2382))
* Python: Add `JSON.RESP` command ([#2451](https://github.com/valkey-io/valkey-glide/pull/2451))
* Python: Add `JSON.ARRPOP` command ([#2407](https://github.com/valkey-io/valkey-glide/pull/2407))
* Node: Add `JSON.STRLEN` and `JSON.STRAPPEND` command ([#2537](https://github.com/valkey-io/valkey-glide/pull/2537))

#### Breaking Changes
Expand Down
9 changes: 8 additions & 1 deletion python/python/glide/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,11 @@
FtSearchLimit,
ReturnField,
)
from glide.async_commands.server_modules.json import JsonArrIndexOptions, JsonGetOptions
from glide.async_commands.server_modules.json import (
JsonArrIndexOptions,
JsonArrPopOptions,
JsonGetOptions,
)
from glide.async_commands.sorted_set import (
AggregationType,
GeoSearchByBox,
Expand Down Expand Up @@ -254,6 +258,9 @@
"json",
"JsonGetOptions",
"JsonArrIndexOptions",
"JsonArrPopOptions",
# Search
"ft",
shohamazon marked this conversation as resolved.
Show resolved Hide resolved
# Logger
"Logger",
"LogLevel",
Expand Down
89 changes: 89 additions & 0 deletions python/python/glide/async_commands/server_modules/json.py
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,33 @@ def to_args(self) -> List[str]:
return args


class JsonArrPopOptions:
"""
Options for the JSON.ARRPOP command.

Args:
path (TEncodable): The path within the JSON document.
index (Optional[int]): The index of the element to pop. If not specified, will pop the last element.
Out of boundary indexes are rounded to their respective array boundaries. Defaults to None.
ikolomi marked this conversation as resolved.
Show resolved Hide resolved
"""

def __init__(self, path: TEncodable, index: Optional[int] = None):
ikolomi marked this conversation as resolved.
Show resolved Hide resolved
self.path = path
self.index = index

def to_args(self) -> List[TEncodable]:
"""
Get the options as a list of arguments for the JSON.ARRPOP command.

Returns:
List[TEncodable]: A list containing the path and, if specified, the index.
"""
args = [self.path]
if self.index is not None:
args.append(str(self.index))
return args


async def set(
client: TGlideClient,
key: TEncodable,
Expand Down Expand Up @@ -403,6 +430,68 @@ async def arrlen(
)


async def arrpop(
client: TGlideClient,
key: TEncodable,
options: Optional[JsonArrPopOptions] = None,
) -> Optional[TJsonResponse[bytes]]:
"""
shohamazon marked this conversation as resolved.
Show resolved Hide resolved
Pops an element from the array located at the specified path within the JSON document stored at `key`.
If `options.index` is provided, it pops the element at that index instead of the last element.

Args:
client (TGlideClient): The client to execute the command.
key (TEncodable): The key of the JSON document.
options (Optional[JsonArrPopOptions]): Options including the path and optional index. See `JsonArrPopOptions`. Default to None.
If not specified, attempts to pop the last element from the root value if it's an array.
If the root value is not an array, an error will be raised.

Returns:
Optional[TJsonResponse[bytes]]:
For JSONPath (`options.path` starts with `$`):
Returns a list of bytes string replies for every possible path, representing the popped JSON values,
or None for JSON values matching the path that are not an array or are an empty array.
If `options.path` doesn't exist, an empty list will be returned.
For legacy path (`options.path` doesn't starts with `$`):
Returns a bytes string representing the popped JSON value, or None if the array at `options.path` is empty.
If multiple paths match, the value from the first matching array that is not empty is returned.
If the JSON value at `options.path` is not a array or if `options.path` doesn't exist, an error is raised.
shohamazon marked this conversation as resolved.
Show resolved Hide resolved
If `key` doesn't exist, an error is raised.
shohamazon marked this conversation as resolved.
Show resolved Hide resolved

Examples:
>>> from glide import json
>>> await json.set(client, "doc", "$", '{"a": [1, 2, true], "b": {"a": [3, 4, ["value", 3, false], 5], "c": {"a": 42}}}')
b'OK'
>>> await json.arrpop(client, "doc", JsonArrPopOptions(path="$.a", index=1))
[b'2'] # Pop second element from array at path $.a
>>> await json.arrpop(client, "doc", JsonArrPopOptions(path="$..a"))
[b'true', b'5', None] # Pop last elements from all arrays matching path `..a`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
[b'true', b'5', None] # Pop last elements from all arrays matching path `..a`
[b'true', b'5', None] # Pop last elements from all arrays matching path `$..a`


#### Using a legacy path (..) to pop the first matching array
>>> await json.arrpop(client, "doc", JsonArrPopOptions(path="..a"))
b"1" # First match popped (from array at path ..a)

#### Even though only one value is returned from `..a`, subsequent arrays are also affected
>>> await json.get(client, "doc", "$..a")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
>>> await json.get(client, "doc", "$..a")
>>> await json.get(client, "doc", "..a")

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No I wanted to show that even if we used ..a and just one value was returned, the rest of the matching paths were popped as well

b"[[], [3, 4], 42]" # Remaining elements after pop show the changes

>>> await json.set(client, "doc", "$", '[[], ["a"], ["a", "b", "c"]]')
b'OK' # JSON is successfully set
>>> await json.arrpop(client, "doc", JsonArrPopOptions(path=".", index=-1))
b'["a","b","c"]' # Pop last elements at path `.`
>>> await json.arrpop(client, "doc")
b'["a"]' # Pop last elements at path `.`
"""
args = ["JSON.ARRPOP", key]
if options:
args.extend(options.to_args())

return cast(
Optional[TJsonResponse[bytes]],
await client.custom_command(args),
)


async def arrtrim(
client: TGlideClient,
key: TEncodable,
Expand Down
80 changes: 79 additions & 1 deletion python/python/tests/tests_server_modules/test_json.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,11 @@
import pytest
from glide.async_commands.core import ConditionalChange, InfoSection
from glide.async_commands.server_modules import json
from glide.async_commands.server_modules.json import JsonArrIndexOptions, JsonGetOptions
from glide.async_commands.server_modules.json import (
JsonArrIndexOptions,
JsonArrPopOptions,
JsonGetOptions,
)
from glide.config import ProtocolVersion
from glide.constants import OK
from glide.exceptions import RequestError
Expand Down Expand Up @@ -1840,3 +1844,77 @@ async def test_json_resp(self, glide_client: TGlideClient):
assert await json.resp(glide_client, "nonexistent_key", "$") is None
assert await json.resp(glide_client, "nonexistent_key", ".") is None
assert await json.resp(glide_client, "nonexistent_key") is None

@pytest.mark.parametrize("cluster_mode", [True, False])
@pytest.mark.parametrize("protocol", [ProtocolVersion.RESP2, ProtocolVersion.RESP3])
async def test_json_arrpop(self, glide_client: TGlideClient):
key = get_random_string(5)
key2 = get_random_string(5)

json_value = '{"a": [1, 2, true], "b": {"a": [3, 4, ["value", 3, false] ,5], "c": {"a": 42}}}'
assert await json.set(glide_client, key, "$", json_value) == OK

assert await json.arrpop(
glide_client, key, JsonArrPopOptions(path="$.a", index=1)
shohamazon marked this conversation as resolved.
Show resolved Hide resolved
) == [b"2"]
assert (
await json.arrpop(glide_client, key, JsonArrPopOptions(path="$..a"))
) == [b"true", b"5", None]

assert (
await json.arrpop(glide_client, key, JsonArrPopOptions(path="..a")) == b"1"
)
# Even if only one array element was returned, ensure second array at `..a` was popped
assert await json.get(glide_client, key, "$..a") == b"[[],[3,4],42]"

# Out of index
assert await json.arrpop(
glide_client, key, JsonArrPopOptions(path="$..a", index=10)
) == [None, b"4", None]

assert (
await json.arrpop(
glide_client, key, JsonArrPopOptions(path="..a", index=-10)
)
== b"3"
)

# Path is not an array
assert await json.arrpop(glide_client, key, JsonArrPopOptions(path="$")) == [
None
]
with pytest.raises(RequestError):
assert await json.arrpop(glide_client, key, JsonArrPopOptions(path="."))
with pytest.raises(RequestError):
assert await json.arrpop(glide_client, key)

# Non existing path
assert (
await json.arrpop(
glide_client, key, JsonArrPopOptions(path="$.non_existing_path")
)
== []
)
with pytest.raises(RequestError):
assert await json.arrpop(
glide_client, key, JsonArrPopOptions(path="non_existing_path")
)

with pytest.raises(RequestError):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should make sure to include more comments for tests. it's not always obvious to readers what you're doing here.

await json.arrpop(
glide_client, "non_existing_key", JsonArrPopOptions(path="$.a")
)
with pytest.raises(RequestError):
await json.arrpop(
glide_client, "non_existing_key", JsonArrPopOptions(path=".a")
)

assert (
await json.set(glide_client, key2, "$", '[[], ["a"], ["a", "b", "c"]]')
== OK
)
assert (
await json.arrpop(glide_client, key2, JsonArrPopOptions(path=".", index=-1))
== b'["a","b","c"]'
)
assert await json.arrpop(glide_client, key2) == b'["a"]'
Loading