Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add better support for LUA scripts #304

Closed
JCHacking opened this issue Mar 25, 2024 · 16 comments
Closed

Add better support for LUA scripts #304

JCHacking opened this issue Mar 25, 2024 · 16 comments
Labels
enhancement New feature or request

Comments

@JCHacking
Copy link

JCHacking commented Mar 25, 2024

Is your feature request related to a problem? Please describe.
I am trying to mock the redi backend for asgi-ratelimit and I am encountering the following error:

Traceback (most recent call last):
  File "C:\Users\jcmencia\PycharmProjects\NGCSQALIBS\py_api\venv\Lib\site-packages\redis\commands\core.py", line 5172, in __call__
    return await client.evalsha(self.sha, len(keys), *args)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\Users\jcmencia\PycharmProjects\NGCSQALIBS\py_api\venv\Lib\site-packages\redis\asyncio\client.py", line 610, in execute_command
    return await conn.retry.call_with_retry(
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\Users\jcmencia\PycharmProjects\NGCSQALIBS\py_api\venv\Lib\site-packages\redis\asyncio\retry.py", line 59, in call_with_retry
    return await do()
           ^^^^^^^^^^
  File "C:\Users\jcmencia\PycharmProjects\NGCSQALIBS\py_api\venv\Lib\site-packages\redis\asyncio\client.py", line 584, in _send_command_parse_response
    return await self.parse_response(conn, command_name, **options)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\Users\jcmencia\PycharmProjects\NGCSQALIBS\py_api\venv\Lib\site-packages\redis\asyncio\client.py", line 631, in parse_response
    response = await connection.read_response()
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\Users\jcmencia\PycharmProjects\NGCSQALIBS\py_api\venv\Lib\site-packages\fakeredis\aioredis.py", line 171, in read_response
    raise response
redis.exceptions.NoScriptError: No matching script. Please use EVAL.

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "C:\Users\jcmencia\PycharmProjects\NGCSQALIBS\py_api\venv\Lib\site-packages\starlette\middleware\errors.py", line 164, in __call__
    await self.app(scope, receive, _send)
  File "C:\Users\jcmencia\PycharmProjects\NGCSQALIBS\py_api\venv\Lib\site-packages\ratelimit\core.py", line 94, in __call__
    retry_after = await self.backend.retry_after(path, user, rule)
                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\Users\jcmencia\PycharmProjects\NGCSQALIBS\py_api\venv\Lib\site-packages\ratelimit\backends\redis.py", line 51, in retry_after
    await self.lua_script(keys=list(ruleset.keys()), args=[json.dumps(ruleset)])
  File "C:\Users\jcmencia\PycharmProjects\NGCSQALIBS\py_api\venv\Lib\site-packages\redis\commands\core.py", line 5178, in __call__
    return await client.evalsha(self.sha, len(keys), *args)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\Users\jcmencia\PycharmProjects\NGCSQALIBS\py_api\venv\Lib\site-packages\redis\asyncio\client.py", line 610, in execute_command
    return await conn.retry.call_with_retry(
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\Users\jcmencia\PycharmProjects\NGCSQALIBS\py_api\venv\Lib\site-packages\redis\asyncio\retry.py", line 59, in call_with_retry
    return await do()
           ^^^^^^^^^^
  File "C:\Users\jcmencia\PycharmProjects\NGCSQALIBS\py_api\venv\Lib\site-packages\redis\asyncio\client.py", line 584, in _send_command_parse_response
    return await self.parse_response(conn, command_name, **options)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\Users\jcmencia\PycharmProjects\NGCSQALIBS\py_api\venv\Lib\site-packages\redis\asyncio\client.py", line 631, in parse_response
    response = await connection.read_response()
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\Users\jcmencia\PycharmProjects\NGCSQALIBS\py_api\venv\Lib\site-packages\fakeredis\aioredis.py", line 171, in read_response
    raise response
redis.exceptions.ResponseError: Error running script (call to f_283a893bdb53a688600eff30001e6fb4e4ebd4f5): @user_script:?: [string "<python>"]:2: attempt to index a nil value (global 'cjson')
stack traceback:
	[string "<python>"]:2: in main chunk

Describe the solution you'd like
Add support for cjson? I don't know if this is only the problem.
I don't know exactly if this would be for lupa or for fakeredis, if you can give me some guidance I would appreciate it.

Related issues:

Additional context
I have tried this code:

from fakeredis.aioredis import FakeRedis
from fastapi import FastAPI
from ratelimit import RateLimitMiddleware, Rule
from ratelimit.backends.redis import RedisBackend
from ratelimit.types import Scope

app = FastAPI(debug=True)


async def authenticate(scope: Scope) -> tuple[str, str]:
    return str(scope["client"]), "default"


app.add_middleware(
    RateLimitMiddleware,
    authenticate=authenticate,
    backend=RedisBackend(
        FakeRedis.from_url("redis://redis")
    ),
    config={
        "": [Rule(minute=1)]
    }
)


@app.get("/")
async def test():
    return "A"

Upvote & Fund

  • We're using Polar.sh so you can upvote and help fund this issue.
  • We receive the funding once the issue is completed & confirmed by you.
  • Thank you in advance for helping prioritize & fund our backlog.
Fund with Polar
@JCHacking JCHacking added the enhancement New feature or request label Mar 25, 2024
@cunla
Copy link
Owner

cunla commented Mar 31, 2024

Did you install fakeredis with lua support?
https://fakeredis.readthedocs.io/en/latest/#installation

@JCHacking
Copy link
Author

Did you install fakeredis with lua support?
https://fakeredis.readthedocs.io/en/latest/#installation

Yes

@cunla
Copy link
Owner

cunla commented Mar 31, 2024

I checked, to load cjson, it should be compiled for the machine where fakeredis is running.

I might add an environment variable LUA_MODULES stating which modules should be loaded.

@JCHacking
Copy link
Author

JCHacking commented Mar 31, 2024

So from what you say, to avoid this error I have to compile it on the machine that is going to run it with the environment variable LUA_MODULES=cjson right?

Or could this be a bug in the lua script?
It looks like this is the lua script running underneath. I understand that the failure is that it does not find the cjson module.

local ruleset = cjson.decode(ARGV[1])

-- Set limits
for i, key in pairs(KEYS) do
    redis.call('SET', key, ruleset[key][1], 'EX', ruleset[key][2], 'NX')
end

-- Check limits
for i = 1, #KEYS do
    local value = redis.call('GET', KEYS[i])
    if value and tonumber(value) < 1 then
        return ruleset[KEYS[i]][2]
    end
end

-- Decrease limits
for i, key in pairs(KEYS) do
    redis.call('DECR', key)
end
return 0

@cunla
Copy link
Owner

cunla commented Mar 31, 2024

Well, I would need to implement this, but yeah, eventually, that would be the way it works.

@JCHacking
Copy link
Author

JCHacking commented Mar 31, 2024

I use poetry to manage dependencies, so when I get a whl from pypi I will download that one. I understand that in that case I will not use anything from LUA_MODULES because I will download it already compiled.

@cunla
Copy link
Owner

cunla commented Mar 31, 2024

does cjson have a pypi package?

@JCHacking
Copy link
Author

It seems that there are packages like ujson that give this functionality (I have not tested them).

@cunla
Copy link
Owner

cunla commented Mar 31, 2024

We need something where this would run:

import lupa

lupa.LuaRuntime().execute("require('cjson')")

I don't think ujson meets this requirement

@JCHacking
Copy link
Author

JCHacking commented Mar 31, 2024

Trying with this code I get this error:

lupa.lua54.LuaError: [string "<python>"]:1: module 'cjson' not found:
	no field package.preload['cjson']
	no file 'C:\Users\jcmencia\AppData\Local\Programs\Python\Python312\lua\cjson.lua'
	no file 'C:\Users\jcmencia\AppData\Local\Programs\Python\Python312\lua\cjson\init.lua'
	no file 'C:\Users\jcmencia\AppData\Local\Programs\Python\Python312\cjson.lua'
	no file 'C:\Users\jcmencia\AppData\Local\Programs\Python\Python312\cjson\init.lua'
	no file 'C:\Users\jcmencia\AppData\Local\Programs\Python\Python312\..\share\lua\5.4\cjson.lua'
	no file 'C:\Users\jcmencia\AppData\Local\Programs\Python\Python312\..\share\lua\5.4\cjson\init.lua'
	no file '.\cjson.lua'
	no file '.\cjson\init.lua'
	no file 'C:\Users\jcmencia\AppData\Local\Programs\Python\Python312\cjson.dll'
	no file 'C:\Users\jcmencia\AppData\Local\Programs\Python\Python312\..\lib\lua\5.4\cjson.dll'
	no file 'C:\Users\jcmencia\AppData\Local\Programs\Python\Python312\loadall.dll'
	no file '.\cjson.dll'
stack traceback:
	[string "<python>"]:1: in main chunk
	[C]: in function 'require'

A little closer to making it work we are!

@JCHacking
Copy link
Author

This is probably something that should be implemented in the lupa package.

@cunla
Copy link
Owner

cunla commented Apr 1, 2024

lupa supports importing modules, it simply does not have cjson ootb.

I managed to install cjson:

  • First, install luarocks (I am using mac so brew install luarocks, but there is an installation for windows).
  • Then, install cjson using luarocks install lua-cjson
  • Then, copy cjson.so (or in your case I assume cjson.dll) to the project homedir.
  • Then, the code I wrote should work.

cunla added a commit that referenced this issue Apr 1, 2024
cunla added a commit that referenced this issue Apr 1, 2024
@JCHacking
Copy link
Author

JCHacking commented Apr 1, 2024

I have been researching and there is a "cleaner" way to do this:

import lupa

lupa.LuaRuntime().require("cjson")

@cunla
Copy link
Owner

cunla commented Apr 1, 2024

Does it work without installing cjson first?

@JCHacking
Copy link
Author

JCHacking commented Apr 2, 2024

Does it work without installing cjson first?

No, as you said it seems that lupa does not bring the cjson module with it.

This makes more "complex", in my case, mock redis for asgi-ratelimit since I want for my CI to use python base only, and it doesn't seem right to upload cjson.dll or cjson.so to the repository either.

@cunla
Copy link
Owner

cunla commented Apr 2, 2024

yeah, I am not sure why asgi-ratelimit chose to implement a ratelimit using a script that uses cjson - it seems a bit cumbersome.
But regardless, fakeredis now supports having LUA modules

@cunla cunla closed this as completed Apr 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants