[GitHub] [apisix] membphis commented on a change in pull request #2121: fix: limit count config not effective

[GitBox <gi...@apache.org>]

membphis commented on a change in pull request #2121:
URL: https://github.com/apache/apisix/pull/2121#discussion_r479862432



##########
File path: apisix/plugin.lua
##########
@@ -297,6 +297,7 @@ local function merge_service_route(service_conf, route_conf)
     local new_conf = core.table.deepcopy(service_conf)
     new_conf.value.service_id = new_conf.value.id
     new_conf.value.id = route_conf.value.id
+    new_conf.modifiedIndex=route_conf.modifiedIndex

Review comment:
       `new_conf.modifiedIndex=route_conf.modifiedIndex`
   ->
   `new_conf.modifiedIndex = route_conf.modifiedIndex`

##########
File path: apisix/plugin.lua
##########
@@ -332,7 +333,8 @@ function _M.merge_service_route(service_conf, route_conf)
     core.log.info("service conf: ", core.json.delay_encode(service_conf))
     core.log.info("  route conf: ", core.json.delay_encode(route_conf))
 
-    return merged_route(route_conf, service_conf,
+    local rout_service_key = route_conf.modifiedIndex .. service_conf.modifiedIndex

Review comment:
       add a separator: `modifiedIndex .. "#" .. modifiedIndex`

##########
File path: t/plugin/limit-count.t
##########
@@ -637,3 +637,54 @@ GET /t
 passed
 --- no_error_log
 [error]
+

Review comment:
       three blank lines between different test cases

##########
File path: apisix/init.lua
##########
@@ -361,21 +361,12 @@ function _M.http_access_phase()
             return core.response.exit(404)
         end
 
-        local changed
-        route, changed = plugin.merge_service_route(service, route)
+        route= plugin.merge_service_route(service, route)
         api_ctx.matched_route = route
-
-        if changed then
-            api_ctx.conf_type = "route&service"
-            api_ctx.conf_version = route.modifiedIndex .. "&"
-                                   .. service.modifiedIndex
-            api_ctx.conf_id = route.value.id .. "&"
-                              .. service.value.id
-        else
-            api_ctx.conf_type = "service"
-            api_ctx.conf_version = service.modifiedIndex
-            api_ctx.conf_id = service.value.id
-        end
+        api_ctx.conf_type = "route&service"
+        api_ctx.conf_version = route.modifiedIndex .. "&" .. service.modifiedIndex
+        api_ctx.conf_id = route.value.id .. "&"
+                .. service.value.id

Review comment:
       bad indentation

##########
File path: t/plugin/limit-count.t
##########
@@ -637,3 +637,54 @@ GET /t
 passed
 --- no_error_log
 [error]
+
+=== TEST 21: when the count is changed, check  the limit is correct
+--- config
+    location /t1 {
+        content_by_lua_block {
+            local t = require("lib.test_admin").test
+            local code, body = t('/apisix/admin/routes/1',
+                 ngx.HTTP_PUT,
+                 [[{
+                        "methods": ["GET"],
+                        "plugins": {
+                            "limit-count": {
+                                "count": 1,
+                                "time_window": 60,
+                                "key": "remote_addr"
+                            }
+                        },
+                        "upstream": {
+                            "nodes": {
+                                "127.0.0.1:1980": 1
+                            },
+                            "type": "roundrobin"
+                        },
+                        "uri": "/hello"
+                }]],
+                [[{
+                    "node": {
+                        "value": {
+                            "plugins": {
+                                "limit-count": {
+                                    "rejected_code": 503
+                                }
+                            }
+                        }
+                    }
+                }]]
+                )
+
+            if code >= 300 then
+                ngx.status = code
+            end
+            ngx.say(body)
+        }
+    }   
+    

Review comment:
       remove this line




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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org