Copilot commented on code in PR #12605:
URL: https://github.com/apache/apisix/pull/12605#discussion_r2917010321
##########
apisix/plugins/limit-req/util.lua:
##########
@@ -14,61 +14,103 @@
-- See the License for the specific language governing permissions and
-- limitations under the License.
--
-local math = require "math"
-local abs = math.abs
-local max = math.max
local ngx_now = ngx.now
-local ngx_null = ngx.null
local tonumber = tonumber
+local core = require("apisix.core")
local _M = {version = 0.1}
+local redis_incoming_script = core.string.compress_script([=[
+ local state_key = KEYS[1] -- state_key (hash), fields:
"excess", "last"
+ local rate = tonumber(ARGV[1]) -- req/s
+ local now = tonumber(ARGV[2]) -- ms
+ local burst = tonumber(ARGV[3]) -- req/s
Review Comment:
The Lua script comments label `rate`/`burst` as "req/s", but in this limiter
implementation they are passed around in the same scaled units used by the
original algorithm (`rate = configured_rate * 1000`, `burst = configured_burst
* 1000`). Updating these comments to reflect the actual units would reduce the
risk of future logic changes introducing subtle math/TTL bugs.
```suggestion
local rate = tonumber(ARGV[1]) -- scaled request rate
(configured_rate * 1000)
local now = tonumber(ARGV[2]) -- ms
local burst = tonumber(ARGV[3]) -- scaled burst (configured_burst *
1000)
```
##########
t/plugin/limit-req-redis.t:
##########
@@ -660,3 +660,114 @@ passed
GET /t
--- response_body eval
qr/property \"rate\" validation failed: expected 0 to be greater than 0/
+
+
+
+=== TEST 25: verify atomic Redis operations with hash key structure
+--- config
+ location /t {
+ content_by_lua_block {
+ local redis = require "resty.redis"
+ local red = redis:new()
+ red:set_timeout(1000)
+
+ local ok, err = red:connect("127.0.0.1", 6379)
+ if not ok then
+ ngx.say("failed to connect: ", err)
+ return
+ end
+
+ -- Clean up any existing keys
+ red:del("limit_req:{test_key}:state")
+
+ -- Test the new hash-based key structure
+ local util = require("apisix.plugins.limit-req.util")
+ local limiter = {
+ rate = 10, -- 10 req/s
+ burst = 1000 -- 1000 req/s burst
+ }
+
+ -- First request should succeed
+ local delay, excess = util.incoming(limiter, red, "test_key", true)
+ if delay then
+ ngx.say("first request: delay=", delay, " excess=", excess)
+ else
+ ngx.say("first request failed: ", excess)
+ end
+
+ -- Verify the Redis hash was created with correct key format
+ local vals = red:hmget("limit_req:{test_key}:state", "excess",
"last")
+ if vals[1] and vals[2] then
Review Comment:
`resty.redis:hmget` returns `(vals, err)`. This test only captures the first
return value and then unconditionally indexes `vals[1]`/`vals[2]`, which will
throw a Lua runtime error if `hmget` fails and returns `nil`. Capture `err` and
guard for `not vals` (and ideally print the error) to make the test robust and
failures diagnosable.
```suggestion
local vals, err = red:hmget("limit_req:{test_key}:state",
"excess", "last")
if not vals then
ngx.say("failed to hmget: ", err)
elseif vals[1] and vals[2] then
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
