Support StrictRedis.eval for Lua scripts #9

blfoster · 2018-02-06T21:18:19Z

This reverts commit 06e02b5.

coveralls · 2018-02-06T21:55:11Z

Coverage decreased (-0.04%) to 97.949% when pulling 507a125 on amplifylitco:master into 9f881a8 on ska-sa:master.

coveralls · 2018-02-06T21:55:11Z

Coverage increased (+0.04%) to 98.024% when pulling 9f4d910 on amplifylitco:master into 9f881a8 on ska-sa:master.

blfoster · 2018-02-06T21:58:26Z

Coveralls report shows that new code has 100% coverage. I'm not sure why it reports a net decrease. The uncovered line seems unrelated to this PR.

bmerry · 2018-02-07T04:49:55Z

Thanks, I'll take a look soon. What happens if the user doesn't have Lua installed? Ideally it should gracefully fall back to not supporting the eval command, rather than being a hard requirement.

blfoster · 2018-02-07T06:23:52Z

Thanks. I'm testing it now on a clean install of Linux Mint without Lua, and it works fine. I don't have a great explanation for how that is possible at the moment. I would need to dig into the lupa source a bit more. I can look at that in the morning.

bmerry · 2018-02-07T06:44:52Z

Is it possible that lupa is bundling a local copy of Lua in a binary wheel? That might be something that will work only for people using platforms for which lupa ships a wheel.

bmerry · 2018-02-07T06:52:29Z

fakenewsredis.py

+            'incrby': self.incr
+        }
+        op = op.lower()
+        func = special_cases[op] if op in special_cases else getattr(self, op)


There are presumably attributes of self that shouldn't be visible to Lua - for a start, anything with an underscore prefix, plus things like delete and from_url. Also, when self is a FakeRedis rather than a FakeStrictRedis, it presumably should use the overload from the FakeStrictRedis base class.

Probably yes. This is a bit of a hack, but I figure the tradeoff is that it doesn't have a huge mapping from Redis commands to Python functions that needs to be maintained as more Redis functions get added. I'll make it a bit more selective.

blfoster · 2018-02-07T13:47:54Z

It looks like Lupa includes the Lua source, which it builds as a fallback in the event that it can't find Lua installed locally.

bmerry · 2018-02-07T14:11:33Z

It looks like Lupa includes the Lua source, which it builds as a fallback in the event that it can't find Lua installed locally.

I'd prefer not to make a compiler (or an already-installed Lua) a hard requirement. I would suggest

importing lupa locally in the eval function; and
declaring it as an extras_require in setup.py.

blfoster · 2018-02-07T16:40:35Z

Code review fallout is complete.

bmerry · 2018-02-08T18:59:49Z

I still need to go through the docs to see if I missed anything.

Okay, let me know when you think it's all covered and I'll take another look.

blfoster · 2018-02-09T15:36:37Z

@bmerry I've added a bunch more tests and fixed some more issues. It seems that you're more familiar with Redis than I, so you may find that I've missed something, but it looks pretty good to me.

bmerry · 2018-02-09T19:11:06Z

I've added a bunch more tests and fixed some more issues. It seems that you're more familiar with Redis than I, so you may find that I've missed something, but it looks pretty good to me.

Great. I'll look over it again next week. Coveralls seems to indicate that there is an exception handler that isn't being covered by the tests - can you take a look?

blfoster · 2018-02-09T19:36:42Z

Oops, I don't think that exception handler is necessary. I took it out.

bmerry · 2018-02-12T10:40:08Z

README.rst

@@ -266,6 +265,11 @@ they have all been tagged as 'slow' so you can skip them by running::
 Revision history
 ================

+0.9.5


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.

bmerry · 2018-02-12T10:40:39Z

README.rst

+0.9.5
+-----
+Add support for StrictRedis.eval for Lua scripts
+- `#9 <https://github.com/ska-sa/fakenewsredis/pull/9>`_ 


Can you put the description into the bullet point (to make the format of the other changelog entries).

bmerry · 2018-02-12T10:44:09Z

fakenewsredis.py

+        Returns the result of the script.
+        In practice, use the object returned by ``register_script``. This
+        function exists purely for Redis API completion.
+        """


You can delete the docstring. It's assumed that people will refer to redis-py for docs.

bmerry · 2018-02-12T10:47:51Z

fakenewsredis.py

+        expected_globals = set()
+        set_globals(
+            (None,) + keys_and_args[:numkeys],
+            (None,) + keys_and_args[numkeys:],


I imagine this will go wrong if numkeys is out of range; it should probably raise a ResponseError.

Fixed, along with a bunch of other edge cases.

bmerry · 2018-02-12T11:05:55Z

fakenewsredis.py

+                "Script attempted to set a global variables: %s" % ", ".join(
+                    actual_globals - expected_globals
+                )
+            )


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 readfile and dofile, presumably for security reasons.

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...

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.

👍 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.

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.

Yep. I'll leave that alone for now if you're ok with it.

bmerry · 2018-02-12T11:20:11Z

test_fakenewsredis.py

+            [
+                val[:2],
+                val[2:]
+            ]


This whole statement can go on one line.

bmerry · 2018-02-12T11:26:59Z

test_fakenewsredis.py

+
+    def test_eval_convert_nil_to_false(self):
+        val = self.redis.eval('return ARGV[1] == false', 0, None)
+        self.assertFalse(val)


I don't think this test is testing what is implied by the name. redis-py converts all arguments through its to_bytes, so None as an argument becomes the string "None" in the script, rather than false. The following test currently fails (but passes for real redis):

val = self.redis.eval('return ARGV[1] == "None"', 0, None) self.assertTrue(val)

Huh, something went wrong here. I'll sort it out...

bmerry · 2018-02-12T11:29:28Z

fakenewsredis.py

+        try:
+            result = lua_runtime.execute(script)
+        except LuaSyntaxError as ex:
+            raise ResponseError(ex)


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)

bmerry · 2018-02-12T11:32:04Z

fakenewsredis.py

+            'ZREVRANGEBYLEX', 'ZREVRANGEBYSCORE', 'ZREVRANK', 'ZSCAN', 'ZSCORE', 'ZUNIONSTORE'
+        ]
+        if op.upper() not in commands:
+            raise ResponseError("Unknown Redis command called from Lua script")


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?

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.

bmerry · 2018-02-12T11:32:30Z

fakenewsredis.py

+        except Exception as ex:
+            return lua_runtime.table_from(
+                {"err": str(ex)}
+            )


This can go on one line.

blfoster · 2018-02-12T14:56:07Z

There are some more subtleties to the numkeys argument (in addition to not being negative), which I've added. I'll push those in a few minutes when I have tests.

blfoster · 2018-02-12T15:28:14Z

Ready for review again.

bmerry · 2018-02-13T09:37:52Z

I'm going to merge this because I think it's at a point where it is a solid useful contribution - thanks! There are still some other things that could be done if you're feeling keen to work on them in a new PR. If not I can file them as bugs to be worked on later:

The other script related commands (EVALSHA, SCRIPT LOAD etc)
The Script convenience class provided by redis-py on top of the raw commands
Loading modules like bitop, cjson etc
Some environmental differences e.g. corner cases in global variable handling we've discussed, blocking database modifications after calling non-deterministic functions, removal of dofile and readfile. This is probably a rabbit-hole though.

blfoster added 13 commits February 5, 2018 12:54

Limited Lua support

b8d997d

Bump patch version

da8a820

Remove nightly Python build

0845484

Return result from Lua

7ea39cd

Fix table handling

74389b7

Fix unit tests

58d0763

Fix test

77685dc

Decode non-byte strings

0dd661f

Add eval

507a125

Restore nightly build

06e02b5

Revert "Restore nightly build"

0fd83e6

This reverts commit 06e02b5.

Update readme

f2086fd

Remove eval from unimplemented functions

6e0c1b3

bmerry reviewed Feb 7, 2018

View reviewed changes

blfoster added 3 commits February 7, 2018 11:10

Code review fallout

73cc1d7

pep8:

87c2686

Install lupa for CI

e0e7718

blfoster added 4 commits February 7, 2018 11:43

Code review fallout

121de8c

pep8:

9de8754

Install lupa for CI

9e6b32f

Merge branch 'master' of ../fakeredis

15232fa

blfoster added 5 commits February 8, 2018 13:19

Merge branch 'master' of ../fakeredis

95762e2

Mess with string types some more

fde2b75

Merge branch 'master' of ../fakeredis

773f0f4

Fix test

b1a0402

Merge branch 'master' of ../fakeredis

856a031

blfoster added 3 commits February 8, 2018 17:20

Test for nil in table

acc44b1

More tests

23a43af

Fix test

525a29b

Remove unnecessary exception handler

267e338

bmerry suggested changes Feb 12, 2018

View reviewed changes

blfoster added 6 commits February 12, 2018 09:11

WIP: fallout

6377669

Remove docstring

244056b

More fallout

8f869b4

Catch LuaError

b2b6786

Check number of keys

fa252bc

More fallout

f917498

blfoster added 4 commits February 12, 2018 10:04

More edge cases for numkeys

82e7e65

Remove braces

fdf9ecb

flake8

a6a7cd6

to_bytes

9f4d910

bmerry approved these changes Feb 13, 2018

View reviewed changes

bmerry merged commit 4bfcda2 into ska-sa:master Feb 13, 2018

bmerry mentioned this pull request Feb 19, 2018

Flesh out the scripting support #16

Closed

Support StrictRedis.eval for Lua scripts #9

Support StrictRedis.eval for Lua scripts #9

Conversation

blfoster commented Feb 6, 2018

coveralls commented Feb 6, 2018 • edited

coveralls commented Feb 6, 2018 • edited

blfoster commented Feb 6, 2018 • edited

bmerry commented Feb 7, 2018

blfoster commented Feb 7, 2018 • edited

bmerry commented Feb 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blfoster commented Feb 7, 2018

bmerry commented Feb 7, 2018

blfoster commented Feb 7, 2018

bmerry commented Feb 8, 2018

blfoster commented Feb 9, 2018

bmerry commented Feb 9, 2018

blfoster commented Feb 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blfoster commented Feb 12, 2018

blfoster commented Feb 12, 2018

bmerry commented Feb 13, 2018

coveralls commented Feb 6, 2018 •

edited

coveralls commented Feb 6, 2018 •

edited

blfoster commented Feb 6, 2018 •

edited

blfoster commented Feb 7, 2018 •

edited