[GitHub] [apisix] membphis commented on a change in pull request #1982: [WIP]feat: script distribute and run

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

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



##########
File path: apisix/init.lua
##########
@@ -451,19 +452,25 @@ function _M.http_access_phase()
         api_ctx.var.upstream_connection = api_ctx.var.http_connection
     end
 
-    local plugins = core.tablepool.fetch("plugins", 32, 0)
-    api_ctx.plugins = plugin.filter(route, plugins)
-
-    run_plugin("rewrite", plugins, api_ctx)
-    if api_ctx.consumer then
-        local changed
-        route, changed = plugin.merge_consumer_route(route, api_ctx.consumer)
-        if changed then
-            core.table.clear(api_ctx.plugins)
-            api_ctx.plugins = plugin.filter(route, api_ctx.plugins)
+    script.load_script(route, api_ctx)
+    if api_ctx.script_obj then
+        script.run_script("access", api_ctx)

Review comment:
       Is it possible move `script.load_script(route, api_ctx)` to here?

##########
File path: apisix/script.lua
##########
@@ -0,0 +1,40 @@
+local require    = require
+local core       = require("apisix.core")
+local pcall      = pcall
+local loadstring = loadstring
+
+local _M = {
+}
+
+
+function _M.load_script(route, api_ctx)
+	local script = route.value.script
+	if script == nil or script == "" then
+		return nil
+	end
+
+	local loadfun = loadstring(script)

Review comment:
       the second value is useful, eg: `"=route#id"` or `"=service#id"`

##########
File path: apisix/init.lua
##########
@@ -451,19 +452,25 @@ function _M.http_access_phase()
         api_ctx.var.upstream_connection = api_ctx.var.http_connection
     end
 
-    local plugins = core.tablepool.fetch("plugins", 32, 0)
-    api_ctx.plugins = plugin.filter(route, plugins)
-
-    run_plugin("rewrite", plugins, api_ctx)
-    if api_ctx.consumer then
-        local changed
-        route, changed = plugin.merge_consumer_route(route, api_ctx.consumer)
-        if changed then
-            core.table.clear(api_ctx.plugins)
-            api_ctx.plugins = plugin.filter(route, api_ctx.plugins)
+    script.load_script(route, api_ctx)

Review comment:
       name style: `script.load`, `script.run` is enough and clearer

##########
File path: apisix/script.lua
##########
@@ -0,0 +1,40 @@
+local require    = require
+local core       = require("apisix.core")
+local pcall      = pcall
+local loadstring = loadstring
+
+local _M = {
+}
+
+
+function _M.load_script(route, api_ctx)
+	local script = route.value.script
+	if script == nil or script == "" then
+		return nil
+	end
+
+	local loadfun = loadstring(script)

Review comment:
       we should catch the return value, it may be failing. we need to record the error message.

##########
File path: apisix/script.lua
##########
@@ -0,0 +1,40 @@
+local require    = require
+local core       = require("apisix.core")
+local pcall      = pcall
+local loadstring = loadstring
+
+local _M = {
+}
+
+
+function _M.load_script(route, api_ctx)
+	local script = route.value.script
+	if script == nil or script == "" then
+		return nil
+	end
+
+	local loadfun = loadstring(script)
+
+	api_ctx.script_obj = loadfun()
+end
+
+
+function _M.run_script(phase, api_ctx)
+    local obj = api_ctx and api_ctx.script_obj or nil
+
+    if not obj then
+        return api_ctx
+    end
+
+    core.log.info("script_obj", core.json.delay_encode(obj, true))
+
+    local phase_fun = obj[phase]
+    if phase_fun then
+    	pcall(phase_fun, api_ctx)

Review comment:
       we should catch the return value, it may be failing. we need to record the error message.




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