Improved JSON accuracy (#1666)

9 files changed, 165 insertions, 76 deletions

diff --git a/redis/commands/helpers.py b/redis/commands/helpers.py
index a92c025..48ee556 100644
--- a/redis/commands/helpers.py
+++ b/redis/commands/helpers.py
@@ -22,6 +22,8 @@ def nativestr(x):
 
 def delist(x):
     """Given a list of binaries, return the stringified version."""
+    if x is None:
+        return x
     return [nativestr(obj) for obj in x]
 
 
diff --git a/redis/commands/json/__init__.py b/redis/commands/json/__init__.py
index 9783705..3149bb8 100644
--- a/redis/commands/json/__init__.py
+++ b/redis/commands/json/__init__.py
@@ -1,5 +1,9 @@
 from json import JSONDecoder, JSONEncoder
 
+from .decoders import (
+    int_or_list,
+    int_or_none
+)
 from .helpers import bulk_of_jsons
 from ..helpers import nativestr, delist
 from .commands import JSONCommands
@@ -48,13 +52,13 @@ class JSON(JSONCommands):
             "JSON.ARRAPPEND": int,
             "JSON.ARRINDEX": int,
             "JSON.ARRINSERT": int,
-            "JSON.ARRLEN": int,
+            "JSON.ARRLEN": int_or_none,
             "JSON.ARRPOP": self._decode,
             "JSON.ARRTRIM": int,
             "JSON.OBJLEN": int,
             "JSON.OBJKEYS": delist,
             # "JSON.RESP": delist,
-            "JSON.DEBUG": int,
+            "JSON.DEBUG": int_or_list,
         }
 
         self.client = client
diff --git a/redis/commands/json/commands.py b/redis/commands/json/commands.py
index 2f8039f..fb00e22 100644
--- a/redis/commands/json/commands.py
+++ b/redis/commands/json/commands.py
@@ -1,5 +1,7 @@
-from .path import Path, str_path
+from .path import Path
 from .helpers import decode_dict_keys
+from deprecated import deprecated
+from redis.exceptions import DataError
 
 
 class JSONCommands:
@@ -9,7 +11,7 @@ class JSONCommands:
         """Append the objects ``args`` to the array under the
         ``path` in key ``name``.
         """
-        pieces = [name, str_path(path)]
+        pieces = [name, str(path)]
         for o in args:
             pieces.append(self._encode(o))
         return self.execute_command("JSON.ARRAPPEND", *pieces)
@@ -23,7 +25,7 @@ class JSONCommands:
         and exclusive ``stop`` indices.
         """
         return self.execute_command(
-            "JSON.ARRINDEX", name, str_path(path), self._encode(scalar),
+            "JSON.ARRINDEX", name, str(path), self._encode(scalar),
             start, stop
         )
 
@@ -31,67 +33,64 @@ class JSONCommands:
         """Insert the objects ``args`` to the array at index ``index``
         under the ``path` in key ``name``.
         """
-        pieces = [name, str_path(path), index]
+        pieces = [name, str(path), index]
         for o in args:
             pieces.append(self._encode(o))
         return self.execute_command("JSON.ARRINSERT", *pieces)
 
-    def forget(self, name, path=Path.rootPath()):
-        """Alias for jsondel (delete the JSON value)."""
-        return self.execute_command("JSON.FORGET", name, str_path(path))
-
     def arrlen(self, name, path=Path.rootPath()):
         """Return the length of the array JSON value under ``path``
         at key``name``.
         """
-        return self.execute_command("JSON.ARRLEN", name, str_path(path))
+        return self.execute_command("JSON.ARRLEN", name, str(path))
 
     def arrpop(self, name, path=Path.rootPath(), index=-1):
         """Pop the element at ``index`` in the array JSON value under
         ``path`` at key ``name``.
         """
-        return self.execute_command("JSON.ARRPOP", name, str_path(path), index)
+        return self.execute_command("JSON.ARRPOP", name, str(path), index)
 
     def arrtrim(self, name, path, start, stop):
         """Trim the array JSON value under ``path`` at key ``name`` to the
         inclusive range given by ``start`` and ``stop``.
         """
-        return self.execute_command("JSON.ARRTRIM", name, str_path(path),
+        return self.execute_command("JSON.ARRTRIM", name, str(path),
                                     start, stop)
 
     def type(self, name, path=Path.rootPath()):
         """Get the type of the JSON value under ``path`` from key ``name``."""
-        return self.execute_command("JSON.TYPE", name, str_path(path))
+        return self.execute_command("JSON.TYPE", name, str(path))
 
     def resp(self, name, path=Path.rootPath()):
         """Return the JSON value under ``path`` at key ``name``."""
-        return self.execute_command("JSON.RESP", name, str_path(path))
+        return self.execute_command("JSON.RESP", name, str(path))
 
     def objkeys(self, name, path=Path.rootPath()):
         """Return the key names in the dictionary JSON value under ``path`` at
         key ``name``."""
-        return self.execute_command("JSON.OBJKEYS", name, str_path(path))
+        return self.execute_command("JSON.OBJKEYS", name, str(path))
 
     def objlen(self, name, path=Path.rootPath()):
         """Return the length of the dictionary JSON value under ``path`` at key
         ``name``.
         """
-        return self.execute_command("JSON.OBJLEN", name, str_path(path))
+        return self.execute_command("JSON.OBJLEN", name, str(path))
 
     def numincrby(self, name, path, number):
         """Increment the numeric (integer or floating point) JSON value under
         ``path`` at key ``name`` by the provided ``number``.
         """
         return self.execute_command(
-            "JSON.NUMINCRBY", name, str_path(path), self._encode(number)
+            "JSON.NUMINCRBY", name, str(path), self._encode(number)
         )
 
+    @deprecated(version='4.0.0', reason='deprecated since redisjson 1.0.0')
     def nummultby(self, name, path, number):
         """Multiply the numeric (integer or floating point) JSON value under
         ``path`` at key ``name`` with the provided ``number``.
         """
         return self.execute_command(
-            "JSON.NUMMULTBY", name, str_path(path), self._encode(number)
+            "JSON.NUMMULTBY", name, str(path), self._encode(number)
         )
 
     def clear(self, name, path=Path.rootPath()):
@@ -102,11 +101,14 @@ class JSONCommands:
         Return the count of cleared paths (ignoring non-array and non-objects
         paths).
         """
-        return self.execute_command("JSON.CLEAR", name, str_path(path))
+        return self.execute_command("JSON.CLEAR", name, str(path))
+
+    def delete(self, key, path=Path.rootPath()):
+        """Delete the JSON value stored at key ``key`` under ``path``."""
+        return self.execute_command("JSON.DEL", key, str(path))
 
-    def delete(self, name, path=Path.rootPath()):
-        """Delete the JSON value stored at key ``name`` under ``path``."""
-        return self.execute_command("JSON.DEL", name, str_path(path))
+    # forget is an alias for delete
+    forget = delete
 
     def get(self, name, *args, no_escape=False):
         """
@@ -125,7 +127,7 @@ class JSONCommands:
 
         else:
             for p in args:
-                pieces.append(str_path(p))
+                pieces.append(str(p))
 
         # Handle case where key doesn't exist. The JSONDecoder would raise a
         # TypeError exception since it can't decode None
@@ -134,13 +136,14 @@ class JSONCommands:
         except TypeError:
             return None
 
-    def mget(self, path, *args):
-        """Get the objects stored as a JSON values under ``path`` from keys
-        ``args``.
+    def mget(self, keys, path):
+        """
+        Get the objects stored as a JSON values under ``path``. ``keys``
+        is a list of one or more keys.
         """
         pieces = []
-        pieces.extend(args)
-        pieces.append(str_path(path))
+        pieces += keys
+        pieces.append(str(path))
         return self.execute_command("JSON.MGET", *pieces)
 
     def set(self, name, path, obj, nx=False, xx=False, decode_keys=False):
@@ -155,7 +158,7 @@ class JSONCommands:
         if decode_keys:
             obj = decode_dict_keys(obj)
 
-        pieces = [name, str_path(path), self._encode(obj)]
+        pieces = [name, str(path), self._encode(obj)]
 
         # Handle existential modifiers
         if nx and xx:
@@ -169,29 +172,43 @@ class JSONCommands:
             pieces.append("XX")
         return self.execute_command("JSON.SET", *pieces)
 
-    def strlen(self, name, path=Path.rootPath()):
+    def strlen(self, name, path=None):
         """Return the length of the string JSON value under ``path`` at key
         ``name``.
         """
-        return self.execute_command("JSON.STRLEN", name, str_path(path))
+        pieces = [name]
+        if path is not None:
+            pieces.append(str(path))
+        return self.execute_command("JSON.STRLEN", *pieces)
 
     def toggle(self, name, path=Path.rootPath()):
         """Toggle boolean value under ``path`` at key ``name``.
         returning the new value.
         """
-        return self.execute_command("JSON.TOGGLE", name, str_path(path))
+        return self.execute_command("JSON.TOGGLE", name, str(path))
 
-    def strappend(self, name, string, path=Path.rootPath()):
-        """Append to the string JSON value under ``path`` at key ``name``
-        the provided ``string``.
+    def strappend(self, name, value, path=Path.rootPath()):
+        """Append to the string JSON value. If two options are specified after
+        the key name, the path is determined to be the first. If a single
+        option is passed, then the rootpath (i.e Path.rootPath()) is used.
         """
+        pieces = [name, str(path), value]
         return self.execute_command(
-            "JSON.STRAPPEND", name, str_path(path), self._encode(string)
+            "JSON.STRAPPEND", *pieces
         )
 
-    def debug(self, name, path=Path.rootPath()):
+    def debug(self, subcommand, key=None, path=Path.rootPath()):
         """Return the memory usage in bytes of a value under ``path`` from
         key ``name``.
         """
-        return self.execute_command("JSON.DEBUG", "MEMORY",
-                                    name, str_path(path))
+        valid_subcommands = ["MEMORY", "HELP"]
+        if subcommand not in valid_subcommands:
+            raise DataError("The only valid subcommands are ",
+                            str(valid_subcommands))
+        pieces = [subcommand]
+        if subcommand == "MEMORY":
+            if key is None:
+                raise DataError("No key specified")
+            pieces.append(key)
+            pieces.append(str(path))
+        return self.execute_command("JSON.DEBUG", *pieces)
diff --git a/redis/commands/json/decoders.py b/redis/commands/json/decoders.py
new file mode 100644
index 0000000..0ee102a
--- /dev/null
+++ b/redis/commands/json/decoders.py
@@ -0,0 +1,12 @@
+def int_or_list(b):
+    if isinstance(b, int):
+        return b
+    else:
+        return b
+
+
+def int_or_none(b):
+    if b is None:
+        return None
+    if isinstance(b, int):
+        return b
diff --git a/redis/commands/json/path.py b/redis/commands/json/path.py
index dff8648..6d87045 100644
--- a/redis/commands/json/path.py
+++ b/redis/commands/json/path.py
@@ -1,11 +1,3 @@
-def str_path(p):
-    """Return the string representation of a path if it is of class Path."""
-    if isinstance(p, Path):
-        return p.strPath
-    else:
-        return p
-
-
 class Path(object):
     """This class represents a path in a JSON value."""
 
@@ -19,3 +11,6 @@ class Path(object):
     def __init__(self, path):
         """Make a new path based on the string representation in `path`."""
         self.strPath = path
+
+    def __repr__(self):
+        return self.strPath
diff --git a/requirements.txt b/requirements.txt
new file mode 100644
index 0000000..9f8d550
--- /dev/null
+++ b/requirements.txt
@@ -0,0 +1 @@
+deprecated
diff --git a/setup.py b/setup.py
index d0c81b4..6c712bd 100644
--- a/setup.py
+++ b/setup.py
@@ -23,6 +23,9 @@ setup(
     author="Redis Inc.",
     author_email="oss@redis.com",
     python_requires=">=3.6",
+    install_requires=[
+        'deprecated'
+    ],
     classifiers=[
         "Development Status :: 5 - Production/Stable",
         "Environment :: Console",
diff --git a/tests/test_json.py b/tests/test_json.py
index 83fbf28..f62346f 100644
--- a/tests/test_json.py
+++ b/tests/test_json.py
@@ -74,27 +74,28 @@ def test_jsonsetexistentialmodifiersshouldsucceed(client):
 def test_mgetshouldsucceed(client):
     client.json().set("1", Path.rootPath(), 1)
     client.json().set("2", Path.rootPath(), 2)
-    r = client.json().mget(Path.rootPath(), "1", "2")
-    e = [1, 2]
-    assert e == r
+    assert client.json().mget(["1"], Path.rootPath()) == [1]
+
+    assert client.json().mget([1, 2], Path.rootPath()) == [1, 2]
 
 
 @pytest.mark.redismod
 @skip_ifmodversion_lt("99.99.99", "ReJSON")  # todo: update after the release
-def test_clearShouldSucceed(client):
+def test_clear(client):
     client.json().set("arr", Path.rootPath(), [0, 1, 2, 3, 4])
     assert 1 == client.json().clear("arr", Path.rootPath())
     assert [] == client.json().get("arr")
 
 
 @pytest.mark.redismod
-def test_typeshouldsucceed(client):
+def test_type(client):
     client.json().set("1", Path.rootPath(), 1)
+    assert b"integer" == client.json().type("1", Path.rootPath())
     assert b"integer" == client.json().type("1")
 
 
 @pytest.mark.redismod
-def test_numincrbyshouldsucceed(client):
+def test_numincrby(client):
     client.json().set("num", Path.rootPath(), 1)
     assert 2 == client.json().numincrby("num", Path.rootPath(), 1)
     assert 2.5 == client.json().numincrby("num", Path.rootPath(), 0.5)
@@ -102,16 +103,18 @@ def test_numincrbyshouldsucceed(client):
 
 
 @pytest.mark.redismod
-def test_nummultbyshouldsucceed(client):
+def test_nummultby(client):
     client.json().set("num", Path.rootPath(), 1)
-    assert 2 == client.json().nummultby("num", Path.rootPath(), 2)
-    assert 5 == client.json().nummultby("num", Path.rootPath(), 2.5)
-    assert 2.5 == client.json().nummultby("num", Path.rootPath(), 0.5)
+
+    with pytest.deprecated_call():
+        assert 2 == client.json().nummultby("num", Path.rootPath(), 2)
+        assert 5 == client.json().nummultby("num", Path.rootPath(), 2.5)
+        assert 2.5 == client.json().nummultby("num", Path.rootPath(), 0.5)
 
 
 @pytest.mark.redismod
 @skip_ifmodversion_lt("99.99.99", "ReJSON")  # todo: update after the release
-def test_toggleShouldSucceed(client):
+def test_toggle(client):
     client.json().set("bool", Path.rootPath(), False)
     assert client.json().toggle("bool", Path.rootPath())
     assert not client.json().toggle("bool", Path.rootPath())
@@ -122,28 +125,37 @@ def test_toggleShouldSucceed(client):
 
 
 @pytest.mark.redismod
-def test_strappendshouldsucceed(client):
-    client.json().set("str", Path.rootPath(), "foo")
-    assert 6 == client.json().strappend("str", "bar", Path.rootPath())
-    assert "foobar" == client.json().get("str", Path.rootPath())
+def test_strappend(client):
+    client.json().set("jsonkey", Path.rootPath(), 'foo')
+    import json
+    assert 6 == client.json().strappend("jsonkey", json.dumps('bar'))
+    with pytest.raises(redis.exceptions.ResponseError):
+        assert 6 == client.json().strappend("jsonkey", 'bar')
+    assert "foobar" == client.json().get("jsonkey", Path.rootPath())
 
 
 @pytest.mark.redismod
 def test_debug(client):
     client.json().set("str", Path.rootPath(), "foo")
-    assert 24 == client.json().debug("str", Path.rootPath())
+    assert 24 == client.json().debug("MEMORY", "str", Path.rootPath())
+    assert 24 == client.json().debug("MEMORY", "str")
+
+    # technically help is valid
+    assert isinstance(client.json().debug("HELP"), list)
 
 
 @pytest.mark.redismod
-def test_strlenshouldsucceed(client):
+def test_strlen(client):
     client.json().set("str", Path.rootPath(), "foo")
     assert 3 == client.json().strlen("str", Path.rootPath())
-    client.json().strappend("str", "bar", Path.rootPath())
+    import json
+    client.json().strappend("str", json.dumps("bar"), Path.rootPath())
     assert 6 == client.json().strlen("str", Path.rootPath())
+    assert 6 == client.json().strlen("str")
 
 
 @pytest.mark.redismod
-def test_arrappendshouldsucceed(client):
+def test_arrappend(client):
     client.json().set("arr", Path.rootPath(), [1])
     assert 2 == client.json().arrappend("arr", Path.rootPath(), 2)
     assert 4 == client.json().arrappend("arr", Path.rootPath(), 3, 4)
@@ -151,14 +163,14 @@ def test_arrappendshouldsucceed(client):
 
 
 @pytest.mark.redismod
-def testArrIndexShouldSucceed(client):
+def test_arrindex(client):
     client.json().set("arr", Path.rootPath(), [0, 1, 2, 3, 4])
     assert 1 == client.json().arrindex("arr", Path.rootPath(), 1)
     assert -1 == client.json().arrindex("arr", Path.rootPath(), 1, 2)
 
 
 @pytest.mark.redismod
-def test_arrinsertshouldsucceed(client):
+def test_arrinsert(client):
     client.json().set("arr", Path.rootPath(), [0, 4])
     assert 5 - -client.json().arrinsert(
         "arr",
@@ -172,15 +184,22 @@ def test_arrinsertshouldsucceed(client):
     )
     assert [0, 1, 2, 3, 4] == client.json().get("arr")
 
+    # test prepends
+    client.json().set("val2", Path.rootPath(), [5, 6, 7, 8, 9])
+    client.json().arrinsert("val2", Path.rootPath(), 0, ['some', 'thing'])
+    assert client.json().get("val2") == [["some", "thing"], 5, 6, 7, 8, 9]
+
 
 @pytest.mark.redismod
-def test_arrlenshouldsucceed(client):
+def test_arrlen(client):
     client.json().set("arr", Path.rootPath(), [0, 1, 2, 3, 4])
     assert 5 == client.json().arrlen("arr", Path.rootPath())
+    assert 5 == client.json().arrlen("arr")
+    assert client.json().arrlen('fakekey') is None
 
 
 @pytest.mark.redismod
-def test_arrpopshouldsucceed(client):
+def test_arrpop(client):
     client.json().set("arr", Path.rootPath(), [0, 1, 2, 3, 4])
     assert 4 == client.json().arrpop("arr", Path.rootPath(), 4)
     assert 3 == client.json().arrpop("arr", Path.rootPath(), -1)
@@ -188,25 +207,50 @@ def test_arrpopshouldsucceed(client):
     assert 0 == client.json().arrpop("arr", Path.rootPath(), 0)
     assert [1] == client.json().get("arr")
 
+    # test out of bounds
+    client.json().set("arr", Path.rootPath(), [0, 1, 2, 3, 4])
+    assert 4 == client.json().arrpop("arr", Path.rootPath(), 99)
+
+    # none test
+    client.json().set("arr", Path.rootPath(), [])
+    assert client.json().arrpop("arr") is None
+
 
 @pytest.mark.redismod
-def test_arrtrimshouldsucceed(client):
+def test_arrtrim(client):
     client.json().set("arr", Path.rootPath(), [0, 1, 2, 3, 4])
     assert 3 == client.json().arrtrim("arr", Path.rootPath(), 1, 3)
     assert [1, 2, 3] == client.json().get("arr")
 
+    # <0 test, should be 0 equivalent
+    client.json().set("arr", Path.rootPath(), [0, 1, 2, 3, 4])
+    assert 0 == client.json().arrtrim("arr", Path.rootPath(), -1, 3)
+
+    # testing stop > end
+    client.json().set("arr", Path.rootPath(), [0, 1, 2, 3, 4])
+    assert 2 == client.json().arrtrim("arr", Path.rootPath(), 3, 99)
+
+    # start > array size and stop
+    client.json().set("arr", Path.rootPath(), [0, 1, 2, 3, 4])
+    assert 0 == client.json().arrtrim("arr", Path.rootPath(), 9, 1)
+
+    # all larger
+    client.json().set("arr", Path.rootPath(), [0, 1, 2, 3, 4])
+    assert 0 == client.json().arrtrim("arr", Path.rootPath(), 9, 11)
+
 
 @pytest.mark.redismod
-def test_respshouldsucceed(client):
+def test_resp(client):
     obj = {"foo": "bar", "baz": 1, "qaz": True}
     client.json().set("obj", Path.rootPath(), obj)
     assert b"bar" == client.json().resp("obj", Path("foo"))
     assert 1 == client.json().resp("obj", Path("baz"))
     assert client.json().resp("obj", Path("qaz"))
+    assert isinstance(client.json().resp("obj"), list)
 
 
 @pytest.mark.redismod
-def test_objkeysshouldsucceed(client):
+def test_objkeys(client):
     obj = {"foo": "bar", "baz": "qaz"}
     client.json().set("obj", Path.rootPath(), obj)
     keys = client.json().objkeys("obj", Path.rootPath())
@@ -215,13 +259,22 @@ def test_objkeysshouldsucceed(client):
     exp.sort()
     assert exp == keys
 
+    client.json().set("obj", Path.rootPath(), obj)
+    keys = client.json().objkeys("obj")
+    assert keys == list(obj.keys())
+
+    assert client.json().objkeys("fakekey") is None
+
 
 @pytest.mark.redismod
-def test_objlenshouldsucceed(client):
+def test_objlen(client):
     obj = {"foo": "bar", "baz": "qaz"}
     client.json().set("obj", Path.rootPath(), obj)
     assert len(obj) == client.json().objlen("obj", Path.rootPath())
 
+    client.json().set("obj", Path.rootPath(), obj)
+    assert len(obj) == client.json().objlen("obj")
+
 
 # @pytest.mark.pipeline
 # @pytest.mark.redismod
diff --git a/tox.ini b/tox.ini
index 211f69e..f09b3a8 100644
--- a/tox.ini
+++ b/tox.ini
@@ -75,7 +75,9 @@ volumes =
     bind:rw:{toxinidir}:/data
 
 [testenv]
-deps = -r {toxinidir}/dev_requirements.txt
+deps =
+    -r {toxinidir}/requirements.txt
+    -r {toxinidir}/dev_requirements.txt
 docker =
     master
     replica