Repository: ska-sa/fakenewsredis

Issue:
Mutating operations incorrectly clear expiry
A number of operations modify an existing value, then assign back to `self._db`, which has the side-effect of clearing the expiry time. According to the redis documentation, only operations that completely replace a key (rather than modifying it) clear the expiry. I haven't written any tests yet, but looking at the code, it is probably at least
- decr, incr, incrby, incrbyfloat
- setbit
- ltrim
- rpoplpush

Some other operations where I'm not sure what redis will do
- getset
- rename, renamenx

Discussion:

Patch:
diff --git a/README.rst b/README.rst
index 4f29a1f..14388a4 100644
--- a/README.rst
+++ b/README.rst
@@ -1,5 +1,5 @@
 fakenewsredis: A fake version of a redis-py
-===========================================
+==========================================
 
 .. image:: https://secure.travis-ci.org/ska-sa/fakenewsredis.svg?branch=master
    :target: http://travis-ci.org/ska-sa/fakenewsredis
@@ -270,17 +270,17 @@ Revision history
 -----
 This is the first release of fakenewsredis, based on `fakeredis`_ 0.9.0, with the following features and fixes:
 
-- fakeredis `#78 <https://github.com/jamesls/fakeredis/issues/78>`_ Behaviour of transaction() does not match redis-py
-- fakeredis `#79 <https://github.com/jamesls/fakeredis/issues/79>`_ Implement redis-py's .lock()
-- fakeredis `#90 <https://github.com/jamesls/fakeredis/issues/90>`_ HINCRBYFLOAT changes hash value type to float
-- fakeredis `#101 <https://github.com/jamesls/fakeredis/issues/101>`_ Should raise an error when attempting to get a key holding a list)
-- fakeredis `#146 <https://github.com/jamesls/fakeredis/issues/146>`_ Pubsub messages and channel names are forced to be ASCII strings on Python 2
-- fakeredis `#163 <https://github.com/jamesls/fakeredis/issues/163>`_ getset does not to_bytes the value
-- fakeredis `#165 <https://github.com/jamesls/fakeredis/issues/165>`_ linsert implementation is incomplete
-- fakeredis `#128 <https://github.com/jamesls/fakeredis/pull/128>`_ Remove `_ex_keys` mapping
-- fakeredis `#139 <https://github.com/jamesls/fakeredis/pull/139>`_ Fixed all flake8 errors and added flake8 to Travis CI
-- fakeredis `#166 <https://github.com/jamesls/fakeredis/pull/166>`_ Add type checking
-- fakeredis `#168 <https://github.com/jamesls/fakeredis/pull/168>`_ Use repr to encode floats in to_bytes
+- fakeredis [#78](https://github.com/jamesls/fakeredis/issues/78) Behaviour of transaction() does not match redis-py
+- fakeredis [#79](https://github.com/jamesls/fakeredis/issues/79) Implement redis-py's .lock()
+- fakeredis [#90](https://github.com/jamesls/fakeredis/issues/90) HINCRBYFLOAT changes hash value type to float
+- fakeredis [#101](https://github.com/jamesls/fakeredis/issues/101) Should raise an error when attempting to get a key holding a list)
+- fakeredis [#146](https://github.com/jamesls/fakeredis/issues/146) Pubsub messages and channel names are forced to be ASCII strings on Python 2
+- fakeredis [#163](https://github.com/jamesls/fakeredis/issues/163) getset does not to_bytes the value
+- fakeredis [#165](https://github.com/jamesls/fakeredis/issues/165) linsert implementation is incomplete
+- fakeredis [#128](https://github.com/jamesls/fakeredis/pull/128) Remove `_ex_keys` mapping
+- fakeredis [#139](https://github.com/jamesls/fakeredis/pull/139) Fixed all flake8 errors and added flake8 to Travis CI
+- fakeredis [#166](https://github.com/jamesls/fakeredis/pull/166) Add type checking
+- fakeredis [#168](https://github.com/jamesls/fakeredis/pull/168) Use repr to encode floats in to_bytes
 
 .. _fakeredis: https://github.com/jamesls/fakeredis
 .. _redis-py: http://redis-py.readthedocs.org/en/latest/index.html
diff --git a/fakenewsredis.py b/fakenewsredis.py
index 6e8fdcc..79930f1 100644
--- a/fakenewsredis.py
+++ b/fakenewsredis.py
@@ -142,6 +142,18 @@ class _StrKeyDict(MutableMapping):
         value = self._dict[to_bytes(key)][0]
         self._dict[to_bytes(key)] = (value, timestamp)
 
+    def setx(self, key, value, src=None):
+        """Set a value, keeping the existing expiry time if any. If
+        `src` is specified, it is used as the source of the expiry
+        """
+        if src is None:
+            src = key
+        try:
+            _, expiration = self._dict[to_bytes(src)]
+        except KeyError:
+            expiration = None
+        self._dict[to_bytes(key)] = (value, expiration)
+
     def persist(self, key):
         try:
             value, _ = self._dict[to_bytes(key)]
@@ -294,11 +306,12 @@ class FakeStrictRedis(object):
 
     def decr(self, name, amount=1):
         try:
-            self._db[name] = to_bytes(int(self._get_string(name, b'0')) - amount)
+            value = int(self._get_string(name, b'0')) - amount
+            self._db.setx(name, to_bytes(value))
         except (TypeError, ValueError):
             raise redis.ResponseError("value is not an integer or out of "
                                       "range.")
-        return int(self._db[name])
+        return value
 
     def exists(self, name):
         return name in self._db
@@ -381,11 +394,12 @@ class FakeStrictRedis(object):
             if not isinstance(amount, int):
                 raise redis.ResponseError("value is not an integer or out "
                                           "of range.")
-            self._db[name] = to_bytes(int(self._get_string(name, b'0')) + amount)
+            value = int(self._get_string(name, b'0')) + amount
+            self._db.setx(name, to_bytes(value))
         except (TypeError, ValueError):
             raise redis.ResponseError("value is not an integer or out of "
                                       "range.")
-        return int(self._db[name])
+        return value
 
     def incrby(self, name, amount=1):
         """
@@ -395,10 +409,11 @@ class FakeStrictRedis(object):
 
     def incrbyfloat(self, name, amount=1.0):
         try:
-            self._db[name] = to_bytes(float(self._get_string(name, b'0')) + amount)
+            value = float(self._get_string(name, b'0')) + amount
+            self._db.setx(name, to_bytes(value))
         except (TypeError, ValueError):
             raise redis.ResponseError("value is not a valid float.")
-        return float(self._db[name])
+        return value
 
     def keys(self, pattern=None):
         return [key for key in self._db
@@ -457,7 +472,7 @@ class FakeStrictRedis(object):
             value = self._db[src]
         except KeyError:
             raise redis.ResponseError("No such key: %s" % src)
-        self._db[dst] = value
+        self._db.setx(dst, value, src=src)
         del self._db[src]
         return True
 
@@ -512,7 +527,7 @@ class FakeStrictRedis(object):
             new_byte = byte_to_int(val[byte]) ^ (1 << actual_bitoffset)
         reconstructed = bytearray(val)
         reconstructed[byte] = new_byte
-        self._db[name] = bytes(reconstructed)
+        self._db.setx(name, bytes(reconstructed))
 
     def setex(self, name, time, value):
         if isinstance(time, timedelta):
@@ -541,7 +556,7 @@ class FakeStrictRedis(object):
         if len(val) < offset:
             val += b'\x00' * (offset - len(val))
         val = val[0:offset] + to_bytes(value) + val[offset+len(value):]
-        self.set(name, val)
+        self._db.setx(name, val)
         return len(val)
 
     def strlen(self, name):
@@ -800,7 +815,7 @@ class FakeStrictRedis(object):
                 end = None
             else:
                 end += 1
-            self._db[name] = val[start:end]
+            self._db.setx(name, val[start:end])
         return True
 
     def lindex(self, name, index):
@@ -843,7 +858,7 @@ class FakeStrictRedis(object):
         if el is not None:
             el = to_bytes(el)
             dst_list.insert(0, el)
-            self._db[dst] = dst_list
+            self._db.setx(dst, dst_list)
         return el
 
     def blpop(self, keys, timeout=0):


Test patch:
diff --git a/test_fakenewsredis.py b/test_fakenewsredis.py
index d60751f..064d24f 100644
--- a/test_fakenewsredis.py
+++ b/test_fakenewsredis.py
@@ -201,6 +201,11 @@ class TestFakeStrictRedis(unittest.TestCase):
         with self.assertRaises(redis.ResponseError):
             self.redis.setbit('foo', 0, 1)
 
+    def test_setbit_expiry(self):
+        self.redis.set('foo', b'0x00', ex=10)
+        self.redis.setbit('foo', 1, 1)
+        self.assertGreater(self.redis.ttl('foo'), 0)
+
     def test_bitcount(self):
         self.redis.delete('foo')
         self.assertEqual(self.redis.bitcount('foo'), 0)
@@ -296,6 +301,11 @@ class TestFakeStrictRedis(unittest.TestCase):
         self.assertEqual(self.redis.incr('foo', 5), 20)
         self.assertEqual(self.redis.get('foo'), b'20')
 
+    def test_incr_expiry(self):
+        self.redis.set('foo', 15, ex=10)
+        self.redis.incr('foo', 5)
+        self.assertGreater(self.redis.ttl('foo'), 0)
+
     def test_incr_bad_type(self):
         self.redis.set('foo', 'bar')
         with self.assertRaises(redis.ResponseError):
@@ -326,6 +336,11 @@ class TestFakeStrictRedis(unittest.TestCase):
         self.assertEqual(self.redis.incrbyfloat('foo', 1.0), 1.0)
         self.assertEqual(self.redis.incrbyfloat('foo', 1.0), 2.0)
 
+    def test_incrbyfloat_expiry(self):
+        self.redis.set('foo', 1.5, ex=10)
+        self.redis.incrbyfloat('foo', 2.5)
+        self.assertGreater(self.redis.ttl('foo'), 0)
+
     def test_incrbyfloat_bad_type(self):
         self.redis.set('foo', 'bar')
         with self.assertRaisesRegexp(redis.ResponseError, 'not a valid float'):
@@ -348,6 +363,11 @@ class TestFakeStrictRedis(unittest.TestCase):
         self.redis.decr('foo')
         self.assertEqual(self.redis.get('foo'), b'-1')
 
+    def test_decr_expiry(self):
+        self.redis.set('foo', 10, ex=10)
+        self.redis.decr('foo', 5)
+        self.assertGreater(self.redis.ttl('foo'), 0)
+
     def test_decr_badtype(self):
         self.redis.set('foo', 'bar')
         with self.assertRaises(redis.ResponseError):
@@ -389,6 +409,12 @@ class TestFakeStrictRedis(unittest.TestCase):
         self.assertEqual(self.redis.get('foo'), b'unique value')
         self.assertEqual(self.redis.get('bar'), b'unique value2')
 
+    def test_rename_expiry(self):
+        self.redis.set('foo', 'value1', ex=10)
+        self.redis.set('bar', 'value2')
+        self.redis.rename('foo', 'bar')
+        self.assertGreater(self.redis.ttl('bar'), 0)
+
     def test_mget(self):
         self.redis.set('foo', 'one')
         self.redis.set('bar', 'two')
@@ -743,6 +769,12 @@ class TestFakeStrictRedis(unittest.TestCase):
     def test_ltrim_with_non_existent_key(self):
         self.assertTrue(self.redis.ltrim('foo', 0, -1))
 
+    def test_ltrim_expiry(self):
+        self.redis.rpush('foo', 'one', 'two', 'three')
+        self.redis.expire('foo', 10)
+        self.redis.ltrim('foo', 1, 2)
+        self.assertGreater(self.redis.ttl('foo'), 0)
+
     def test_ltrim_wrong_type(self):
         self.redis.set('foo', 'bar')
         with self.assertRaises(redis.ResponseError):
@@ -834,6 +866,13 @@ class TestFakeStrictRedis(unittest.TestCase):
         self.assertEqual(self.redis.rpoplpush('foo', 'bar'), b'one')
         self.assertEqual(self.redis.rpop('bar'), b'one')
 
+    def test_rpoplpush_expiry(self):
+        self.redis.rpush('foo', 'one')
+        self.redis.rpush('bar', 'two')
+        self.redis.expire('bar', 10)
+        self.redis.rpoplpush('foo', 'bar')
+        self.assertGreater(self.redis.ttl('bar'), 0)
+
     def test_rpoplpush_wrong_type(self):
         self.redis.set('foo', 'bar')
         self.redis.rpush('list', 'element')
@@ -1273,6 +1312,11 @@ class TestFakeStrictRedis(unittest.TestCase):
         self.assertEqual(self.redis.setrange('bar', 2, 'test'), 6)
         self.assertEqual(self.redis.get('bar'), b'\x00\x00test')
 
+    def test_setrange_expiry(self):
+        self.redis.set('foo', 'test', ex=10)
+        self.redis.setrange('foo', 1, 'aste')
+        self.assertGreater(self.redis.ttl('foo'), 0)
+
     def test_sinter(self):
         self.redis.sadd('foo', 'member1')
         self.redis.sadd('foo', 'member2')