-
Notifications
You must be signed in to change notification settings - Fork 4
Support StrictRedis.eval for Lua scripts #9
Changes from 47 commits
b8d997d
da8a820
0845484
7ea39cd
74389b7
58d0763
77685dc
0dd661f
507a125
06e02b5
0fd83e6
f2086fd
6e0c1b3
73cc1d7
87c2686
e0e7718
121de8c
9de8754
9e6b32f
15232fa
9bf4173
bfa38c9
63949ba
975f88c
71a2c1e
1def833
dd23fb1
c91f0b1
ee53358
a30fdef
b426f0f
6153787
7fb69f4
49cf337
392206c
d5e4ce2
fe9065d
296c2ff
95762e2
fde2b75
773f0f4
b1a0402
856a031
acc44b1
23a43af
525a29b
267e338
6377669
244056b
8f869b4
b2b6786
fa252bc
f917498
82e7e65
fdf9ecb
a6a7cd6
9f4d910
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -203,7 +203,6 @@ scripting | |
* script kill | ||
* script load | ||
* evalsha | ||
* eval | ||
* script exists | ||
|
||
|
||
|
@@ -266,6 +265,11 @@ they have all been tagged as 'slow' so you can skip them by running:: | |
Revision history | ||
================ | ||
|
||
0.9.5 | ||
----- | ||
Add support for StrictRedis.eval for Lua scripts | ||
- `#9 <https://github.com/ska-sa/fakenewsredis/pull/9>`_ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you put the description into the bullet point (to make the format of the other changelog entries). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
|
||
0.9.4 | ||
----- | ||
This is a minor bugfix and optimization release: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,6 +14,7 @@ | |
import types | ||
import re | ||
import functools | ||
from itertools import count | ||
|
||
import redis | ||
from redis.exceptions import ResponseError | ||
|
@@ -701,6 +702,138 @@ def sort(self, name, start=None, num=None, by=None, get=None, desc=False, | |
except KeyError: | ||
return [] | ||
|
||
def eval(self, script, numkeys, *keys_and_args): | ||
""" | ||
Execute the Lua ``script``, specifying the ``numkeys`` the script | ||
will touch and the key names and argument values in ``keys_and_args``. | ||
Returns the result of the script. | ||
In practice, use the object returned by ``register_script``. This | ||
function exists purely for Redis API completion. | ||
""" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can delete the docstring. It's assumed that people will refer to redis-py for docs. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
from lupa import LuaRuntime, LuaSyntaxError | ||
|
||
lua_runtime = LuaRuntime(unpack_returned_tuples=True) | ||
|
||
set_globals = lua_runtime.eval( | ||
""" | ||
function(keys, argv, redis_call, redis_pcall) | ||
redis = {} | ||
redis.call = redis_call | ||
redis.pcall = redis_pcall | ||
redis.error_reply = function(msg) return {err=msg} end | ||
redis.status_reply = function(msg) return {ok=msg} end | ||
KEYS = keys | ||
ARGV = argv | ||
end | ||
""" | ||
) | ||
expected_globals = set() | ||
set_globals( | ||
(None,) + keys_and_args[:numkeys], | ||
(None,) + keys_and_args[numkeys:], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I imagine this will go wrong if numkeys is out of range; it should probably raise a ResponseError. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Indeed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed, along with a bunch of other edge cases. |
||
functools.partial(self._lua_redis_call, lua_runtime, expected_globals), | ||
functools.partial(self._lua_redis_pcall, lua_runtime, expected_globals) | ||
) | ||
expected_globals.update(lua_runtime.globals().keys()) | ||
|
||
try: | ||
result = lua_runtime.execute(script) | ||
except LuaSyntaxError as ex: | ||
raise ResponseError(ex) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you need to catch more than just LuaSyntaxError. The following test fails with a LuaError. def test_eval_runtime_error(self):
with self.assertRaises(ResponseError):
self.redis.eval('error("CRASH")', 0) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed. |
||
|
||
self._check_for_lua_globals(lua_runtime, expected_globals) | ||
|
||
return self._convert_lua_result(result, nested=False) | ||
|
||
def _convert_redis_result(self, result): | ||
if isinstance(result, dict): | ||
return [ | ||
i | ||
for item in result.items() | ||
for i in item | ||
] | ||
return result | ||
|
||
def _convert_lua_result(self, result, nested=True): | ||
from lupa import lua_type | ||
if lua_type(result) == 'table': | ||
for key in ('ok', 'err'): | ||
if key in result: | ||
msg = self._convert_lua_result(result[key]) | ||
if not isinstance(msg, bytes): | ||
raise ResponseError("wrong number or type of arguments") | ||
if key == 'ok': | ||
return msg | ||
elif nested: | ||
return ResponseError(msg) | ||
else: | ||
raise ResponseError(msg) | ||
# Convert Lua tables into lists, starting from index 1, mimicking the behavior of StrictRedis. | ||
result_list = [] | ||
for index in count(1): | ||
if index not in result: | ||
break | ||
item = result[index] | ||
result_list.append(self._convert_lua_result(item)) | ||
return result_list | ||
elif isinstance(result, type(u'')): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
return to_bytes(result) | ||
elif isinstance(result, float): | ||
return int(result) | ||
elif isinstance(result, bool): | ||
return result and 1 or None | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use the newer (Python 2.5+, so actually 11 years old) conditional syntax There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
return result | ||
|
||
def _check_for_lua_globals(self, lua_runtime, expected_globals): | ||
actual_globals = set(lua_runtime.globals().keys()) | ||
if actual_globals != expected_globals: | ||
raise ResponseError( | ||
"Script attempted to set a global variables: %s" % ", ".join( | ||
actual_globals - expected_globals | ||
) | ||
) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this is a good approach to preventing global variables being created: it doesn't make the script itself error out, and it doesn't actually prevent the globals from polluting the namespace. Here is the code in redis itself that implements the protection. It may need some tweaks to adapt it - unfortunately I've never programmed in Lua so I'm not sure. See also the function above the code in that link - it appears to disable There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is actually safe, because if we try to set a global variable and error out, the LuaRuntime instance will never be used again; if we try to run another Lua script, we'll get a new LuaRuntime, which will not have that global variable set. Unless there's something I'm missing. I do think this way is a bit more readable, but perhaps that's because I don't know Lua. I could add a few more lines to the unit test asserting that trying to set a global variable can't have a side effect... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I hadn't spotted that the runtime gets discarded each time, so it's safer than I thought. I think it will still be different to real redis if a script sets a global variable, then modifies the database: in real redis it would error out as soon as it tries to set the global, whereas in this implementation it will get to modify the database before the error. Another case that will probably behave differently is a script that creates a global and then deletes it again, before your check. We're starting to get to the point of diminishing returns. If you've got the time and energy to test it and fix things up, then go for it, but I realise that I've made you do a lot more work than you probably expected when you started. If you're running out of steam then this is something that can be left for another day. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 I don't think it should be possible to modify the database either after setting a global, because _lua_redis_call calls _check_for_lua_globals before executing any command that could change the database. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Aha, I'd missed that subtlety too. So then probably the only case that'll behave differently is if the script creates a global and deletes it again. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep. I'll leave that alone for now if you're ok with it. |
||
|
||
def _lua_redis_pcall(self, lua_runtime, expected_globals, op, *args): | ||
try: | ||
return self._lua_redis_call(lua_runtime, expected_globals, op, *args) | ||
except Exception as ex: | ||
return lua_runtime.table_from( | ||
{"err": str(ex)} | ||
) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This can go on one line. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
|
||
def _lua_redis_call(self, lua_runtime, expected_globals, op, *args): | ||
# Check if we've set any global variables before making any change. | ||
self._check_for_lua_globals(lua_runtime, expected_globals) | ||
# These commands aren't necessarily all implemented, but if op is not one of these commands, we expect a | ||
# ResponseError for consistency with Redis | ||
commands = [ | ||
'APPEND', 'AUTH', 'BITCOUNT', 'BITFIELD', 'BITOP', 'BITPOS', 'BLPOP', 'BRPOP', 'BRPOPLPUSH', 'DECR', | ||
'DECRBY', 'DEL', 'DUMP', 'ECHO', 'EVAL', 'EVALSHA', 'EXISTS', 'EXPIRE', 'EXPIREAT', 'FLUSHALL', 'FLUSHDB', | ||
'GEOADD', 'GEODIST', 'GEOHASH', 'GEOPOS', 'GEORADIUS', 'GEORADIUSBYMEMBER', 'GET', 'GETBIT', 'GETRANGE', | ||
'GETSET', 'HDEL', 'HEXISTS', 'HGET', 'HGETALL', 'HINCRBY', 'HINCRBYFLOAT', 'HKEYS', 'HLEN', 'HMGET', | ||
'HMSET', 'HSCAN', 'HSET', 'HSETNX', 'HSTRLEN', 'HVALS', 'INCR', 'INCRBY', 'INCRBYFLOAT', 'INFO', 'KEYS', | ||
'LINDEX', 'LINSERT', 'LLEN', 'LPOP', 'LPUSH', 'LPUSHX', 'LRANGE', 'LREM', 'LSET', 'LTRIM', 'MGET', | ||
'MIGRATE', 'MOVE', 'MSET', 'MSETNX', 'OBJECT', 'PERSIST', 'PEXPIRE', 'PEXPIREAT', 'PFADD', 'PFCOUNT', | ||
'PFMERGE', 'PING', 'PSETEX', 'PSUBSCRIBE', 'PTTL', 'PUBLISH', 'PUBSUB', 'PUNSUBSCRIBE', 'RENAME', | ||
'RENAMENX', 'RESTORE', 'RPOP', 'RPOPLPUSH', 'RPUSH', 'RPUSHX', 'SADD', 'SCAN', 'SCARD', 'SDIFF', | ||
'SDIFFSTORE', 'SELECT', 'SET', 'SETBIT', 'SETEX', 'SETNX', 'SETRANGE', 'SHUTDOWN', 'SINTER', 'SINTERSTORE', | ||
'SISMEMBER', 'SLAVEOF', 'SLOWLOG', 'SMEMBERS', 'SMOVE', 'SORT', 'SPOP', 'SRANDMEMBER', 'SREM', 'SSCAN', | ||
'STRLEN', 'SUBSCRIBE', 'SUNION', 'SUNIONSTORE', 'SWAPDB', 'TOUCH', 'TTL', 'TYPE', 'UNLINK', 'UNSUBSCRIBE', | ||
'WAIT', 'WATCH', 'ZADD', 'ZCARD', 'ZCOUNT', 'ZINCRBY', 'ZINTERSTORE', 'ZLEXCOUNT', 'ZRANGE', 'ZRANGEBYLEX', | ||
'ZRANGEBYSCORE', 'ZRANK', 'ZREM', 'ZREMRANGEBYLEX', 'ZREMRANGEBYRANK', 'ZREMRANGEBYSCORE', 'ZREVRANGE', | ||
'ZREVRANGEBYLEX', 'ZREVRANGEBYSCORE', 'ZREVRANK', 'ZSCAN', 'ZSCORE', 'ZUNIONSTORE' | ||
] | ||
if op.upper() not in commands: | ||
raise ResponseError("Unknown Redis command called from Lua script") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems a bit odd to convert to uppercase for this check and then to lowercase to actually make the call. How about just expressing the whitelist in lowercase? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Heh, the commands are uppercase because that's the way they were written in the Redis documentation I copied this from. I'll change it to lowercase. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
special_cases = { | ||
'del': FakeStrictRedis.delete, | ||
'decrby': FakeStrictRedis.decr, | ||
'incrby': FakeStrictRedis.incr | ||
} | ||
op = op.lower() | ||
func = special_cases[op] if op in special_cases else getattr(FakeStrictRedis, op) | ||
return self._convert_redis_result(func(self, *args)) | ||
|
||
def _retrive_data_from_sort(self, data, get): | ||
if get is not None: | ||
if isinstance(get, string_types): | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,3 +2,4 @@ | |
flake8<3.0.0 | ||
nose==1.3.4 | ||
redis==2.10.6 | ||
lupa==1.6 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,7 +5,7 @@ | |
|
||
setup( | ||
name='fakenewsredis', | ||
version='0.9.4', | ||
version='0.9.5', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't update the version yet; I'll do it when I make a new release. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
description="Fake implementation of redis API for testing purposes.", | ||
long_description=open(os.path.join(os.path.dirname(__file__), | ||
'README.rst')).read(), | ||
|
@@ -30,5 +30,8 @@ | |
], | ||
install_requires=[ | ||
'redis', | ||
] | ||
], | ||
extras_require={ | ||
"lua": ['lupa'] | ||
} | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you change this to "Development version". There is some possibility of re-merging with fakeredis, so I don't want to make assumptions about version numbers yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.