fix: every RedisList operation on a wrong-type key is a TypeError; bytes-like elements (#15)

redisdol/base.py

@@ -46,11 +46,29 @@ def from_sourced_object(cls, obj):
 
 
 def _wrapped_class_of(cls):
-    """``(wrapped class, wrapper class)`` of a ``dol`` class-wrapper (``wrap_kvs``,
-    ``KeyCodecs...``, ``filt_iter``), read from the closure of the ``__init__`` dol
-    generates for it (``None`` if it has none, or delegates through another attribute
-    than ``store``)."""
+    """``(wrapped class, dol's generated wrapper class)`` of a ``dol`` class-wrapper
+    (``wrap_kvs``, ``KeyCodecs...``, ``filt_iter``), or ``None`` if ``cls`` is not one,
+    delegates through another attribute than ``store``, or has an ``__init__`` of its
+    own (a subclass of a wrapper: it must be built through that ``__init__``).
+
+    Reads the ``_wrapped_class`` / ``_delegation_attr`` attributes dol sets on its
+    generated wrapper class (dol#131), falling back, for older dol versions, to the
+    closure of the ``__init__`` it generates.
+    """
     init = getattr(cls, "__init__", None)
+    # the generated wrapper class: the one that defines ``__init__`` and, in new dol,
+    # carries the attributes in its own namespace (a subclass only inherits them)
+    wrapper = next(
+        (c for c in cls.__mro__ if "_wrapped_class" in vars(c) and "__init__" in vars(c)),
+        None,
+    )
+    if wrapper is not None:
+        if vars(wrapper)["__init__"] is not init:
+            return None
+        wrapped, attr = wrapper._wrapped_class, wrapper._delegation_attr
+        if isinstance(wrapped, type) and attr == "store":
+            return wrapped, wrapper
+        return None
     code, cells = getattr(init, "__code__", None), getattr(init, "__closure__", None)
     if code is None or not cells:
         return None
@@ -81,8 +99,8 @@ def _new_with_source(cls, source):
             )
         wrapped, wrapper = found
         self = object.__new__(cls)
-        # what dol's generated __init__ does after building the delegate: the wrapper
-        # class's own __init__ (which may set state of its own), not Store's directly
+        # what dol's generated __init__ does after building the delegate: the next
+        # __init__ in the instance's MRO (which may set state of its own), not Store's
         super(wrapper, self).__init__(_new_with_source(wrapped, source))
         return self
     self = object.__new__(cls)
@@ -274,6 +292,49 @@ def _translate_list_response_error(e: ResponseError, what: str):
     return None
 
 
+def _wrongtype_as_type_error(what):
+    """Decorator factory: a ``WRONGTYPE`` ``ResponseError`` raised by the method becomes the
+    ``TypeError`` that :func:`_translate_list_response_error` maps it to, whichever
+    operation met a key that holds something other than a list. Any other error passes
+    through untouched."""
+
+    def decorator(method):
+        @wraps(method)
+        def wrapper(self, *args, **kwargs):
+            try:
+                return method(self, *args, **kwargs)
+            except ResponseError as e:
+                translated = _translate_list_response_error(e, what)
+                if isinstance(translated, TypeError):
+                    raise translated from e
+                raise
+
+        return wrapper
+
+    return decorator
+
+
+def _normalising_bytes_likes(encode):
+    """``encode`` (or the identity) followed by ``bytes(...)`` for a bytes-like result.
+
+    redis-py would send a ``memoryview`` cast to ``'I'`` by its *length* rather than its
+    bytes; a bytes-like element is a redis string, as a top-level value is.
+    """
+    encode = encode or _identity
+
+    def encode_element(v):
+        v = encode(v)
+        return bytes(v) if isinstance(v, (bytearray, memoryview)) else v
+
+    return encode_element
+
+
+def _type_name(source, name):
+    """The redis type of ``name`` as bytes (``b'none'`` when it doesn't exist)."""
+    t = source.type(name)
+    return t.encode() if isinstance(t, str) else t
+
+
 # TODO: Redis list type is more of a linked-list than a list. Better subclass deque?
 class RedisList(MutableSequence):
     """
@@ -299,12 +360,14 @@ class RedisList(MutableSequence):
     def __init__(self, source, name, *, encode=None, decode=None):
         self._source = source
         self._name = name
-        self._encode = encode or _identity
+        self._encode = _normalising_bytes_likes(encode)
         self._decode = decode or _identity
 
+    @_wrongtype_as_type_error("RedisList length")
     def __len__(self):
         return self._source.llen(self._name)
 
+    @_wrongtype_as_type_error("RedisList access")
     def __getitem__(self, i):
         if isinstance(i, slice):
             return self._get_slice(i)
@@ -329,7 +392,13 @@ def __iter__(self):
         n = max(1, int(self.iter_batch_size))
         start = 0
         while True:
-            batch = self._source.lrange(self._name, start, start + n - 1)
+            try:
+                batch = self._source.lrange(self._name, start, start + n - 1)
+            except ResponseError as e:  # a generator: the decorator can't wrap it
+                translated = _translate_list_response_error(e, "RedisList iteration")
+                if isinstance(translated, TypeError):
+                    raise translated from e
+                raise
             for v in batch:
                 yield self._decode(v)
             if len(batch) < n:
@@ -370,19 +439,35 @@ def __delitem__(self, i):
                 raise
             raise translated from e
 
+    @_wrongtype_as_type_error("RedisList append")
     def append(self, v):
         # rpush (not rpushx): appending to a not-yet-existing list creates it,
         # consistently with `extend`.
         self._source.rpush(self._name, self._encode(v))
 
+    @_wrongtype_as_type_error("RedisList extend")
     def extend(self, iterable):
         values = [self._encode(v) for v in iterable]
         if values:  # rpush with no values is a redis error
             self._source.rpush(self._name, *values)
 
     def clear(self):
-        self._source.delete(self._name)
+        """Delete the list. Like the other operations, refuses (``TypeError``) a key that
+        holds something else, which this handle must not destroy; the check and the
+        deletion are one WATCH/MULTI transaction."""
+        name = self._name
+
+        def _clear(pipe):
+            if _type_name(pipe, name) not in (RedisType.list, b"none"):
+                raise TypeError(
+                    "RedisList clear: the key holds a Redis value that is not a list"
+                )
+            pipe.multi()
+            pipe.delete(name)
+
+        self._source.transaction(_clear, name)
 
+    @_wrongtype_as_type_error("RedisList insert")
     def insert(self, i, v):
         """Insert ``v`` before position ``i`` (``list.insert`` semantics: ``i`` is clamped).
 
@@ -459,7 +544,7 @@ def __setitem__(self, k, v):
         if isinstance(v, (bytearray, memoryview)):
             v = bytes(v)  # bytes-like: a redis string, not a list of ints
         if isinstance(v, Iterable) and not isinstance(v, (str, bytes)):
-            encode = self._list_codec("list_encode") or _identity
+            encode = _normalising_bytes_likes(self._list_codec("list_encode"))
             values = [encode(x) for x in v]  # also consumes one-shot iterables
             if not values:
                 # Redis has no empty list (a list key with no elements doesn't exist).

tests/test_wrapping_fakeredis.py

@@ -2,7 +2,7 @@
 
 import pytest
 from fakeredis import FakeRedis
-from dol import KeyCodecs, wrap_kvs
+from dol import KeyCodecs, Store, wrap_kvs
 
 from redisdol import (
     RedisBytesPersister,
@@ -453,3 +453,143 @@ def execute(s):
     with pytest.raises(ResponseError, match="READONLY") as info:
         del RedisList(ReadOnlyPipe(), "no such key")[0]
     assert info.value.__cause__ is None  # not its own cause
+
+
+# ---- every list operation on a wrong-type key; bytes-like elements (i2mint/redisdol#15) --
+
+
+def _wrong_type_lists():
+    r = FakeRedis()
+    r.set("s", "text")
+    r.hset("h", "f", "v")
+    return r, [RedisList(r, "s"), RedisList(r, "h")]
+
+
+_READ_OPS = {
+    "getitem": lambda l: l[0],
+    "negative": lambda l: l[-1],
+    "slice": lambda l: l[0:2],
+    "stepped slice": lambda l: l[::2],
+    "len": len,
+    "iter": list,
+    "contains": lambda l: "x" in l,
+    "index": lambda l: l.index("x"),
+    "count": lambda l: l.count("x"),
+    "reversed": lambda l: list(reversed(l)),
+    "append": lambda l: l.append("x"),
+    "extend": lambda l: l.extend(["x", "y"]),
+    "insert": lambda l: l.insert(0, "x"),
+    "pop": lambda l: l.pop(),
+    "pop0": lambda l: l.pop(0),
+    "remove": lambda l: l.remove("x"),
+    "clear": lambda l: l.clear(),
+    "iadd": lambda l: l.__iadd__(["x"]),
+    "setitem": lambda l: l.__setitem__(0, "x"),
+    "delitem": lambda l: l.__delitem__(0),
+}
+
+
+@pytest.mark.parametrize("op", list(_READ_OPS.values()), ids=list(_READ_OPS))
+def test_every_list_operation_on_a_wrong_type_key_is_a_type_error(op):
+    r, lists = _wrong_type_lists()
+    for lst in lists:
+        with pytest.raises(TypeError, match="not a list") as info:
+            op(lst)
+        assert type(info.value.__cause__).__name__ == "ResponseError" or (
+            info.value.__cause__ is None  # raised by our own type check (clear)
+        )
+    assert r.get("s") == b"text"  # nothing was destroyed
+    assert r.hget("h", "f") == b"v"
+
+
+def test_clear_still_deletes_lists_and_tolerates_a_missing_key():
+    r = FakeRedis()
+    r.rpush("l", "a", "b")
+    RedisList(r, "l").clear()
+    assert not r.exists("l")
+    RedisList(r, "nope").clear()  # like list.clear on an empty list: no error
+
+
+def test_decode_responses_connection_still_gets_type_errors():
+    r = FakeRedis(decode_responses=True)
+    r.set("s", "text")
+    with pytest.raises(TypeError, match="not a list"):
+        RedisList(r, "s").clear()
+    assert r.get("s") == "text"
+
+
+def test_missing_key_reads_are_unchanged():
+    lst = RedisList(FakeRedis(), "nope")
+    assert len(lst) == 0 and list(lst) == [] and lst[0:3] == []
+    with pytest.raises(IndexError):
+        lst[0]
+
+
+@pytest.mark.parametrize("make", [bytearray, memoryview])
+def test_bytes_like_elements_are_stored_as_their_bytes(make):
+    r = FakeRedis()
+    lst = RedisList(r, "l")
+    lst.append(make(b"ab"))
+    lst.extend([make(b"cd")])
+    lst.insert(0, make(b"zz"))
+    assert r.lrange("l", 0, -1) == [b"zz", b"ab", b"cd"]
+    lst[0] = make(b"yy")
+    assert lst[0] == b"yy"
+
+
+def test_cast_memoryview_element_is_sent_by_its_bytes():
+    import array
+
+    mv = memoryview(array.array("I", [1, 2]))
+    assert len(mv) == 2 and mv.nbytes == 8  # redis-py would send len(mv)
+    r = FakeRedis()
+    RedisList(r, "l").append(mv)
+    assert r.lindex("l", 0) == bytes(mv)
+    s = _store()
+    s["k"] = [mv, b"x"]  # via the store's list path as well
+    assert s._source.lindex("k", 0) == bytes(mv)
+
+
+def test_element_codec_output_that_is_bytes_like_is_normalised():
+    r = FakeRedis()
+    lst = RedisList(r, "l", encode=lambda s: bytearray(s.encode()))
+    lst.append("hi")
+    assert r.lindex("l", 0) == b"hi"
+
+
+def test_wrapped_class_of_falls_back_to_the_init_closure(monkeypatch):
+    """Older dol versions don't set ``_wrapped_class`` etc.: the closure is read instead."""
+    from redisdol.base import _wrapped_class_of
+
+    W = KeyCodecs.prefixed("p:")(RedisPersister)
+    assert _wrapped_class_of(W)[0] is RedisPersister
+    wrap = next((c for c in W.__mro__ if "_wrapped_class" in vars(c)), None)
+    for attr in ("_wrapped_class", "_wrapper_class", "_delegation_attr"):
+        if wrap is not None:  # newer dol sets them; with older dol this is already the fallback
+            monkeypatch.delattr(wrap, attr)
+    assert _wrapped_class_of(W)[0] is RedisPersister
+    s = store_from_source(FakeRedis(), W)
+    s["a"] = "1"
+    assert s["a"] == b"1"
+
+
+def test_subclass_with_its_own_init_is_refused_and_a_mixin_in_the_mro_still_runs():
+    W = KeyCodecs.prefixed("p:")(RedisPersister)
+
+    class Y(W):  # built through its own __init__, which we can't call without a Redis(**kw)
+        def __init__(self, *a, **k):
+            super().__init__(*a, **k)
+            self.extra = 1
+
+    with pytest.raises(TypeError, match="can't be determined"):
+        store_from_source(FakeRedis(), Y)
+
+    class Mixin(Store):
+        def __init__(self, store):
+            super().__init__(store)
+            self.mixin_ran = True
+
+    class X(W, Mixin):
+        pass
+
+    assert store_from_source(FakeRedis(), X).mixin_ran is True