Re: [PR] fix: plugins repeats the rewrite phase when consumer configs with a p… [apisix]
hachi029 commented on code in PR #12048: URL: https://github.com/apache/apisix/pull/12048#discussion_r2011414662 ## apisix/plugin.lua: ## @@ -506,6 +506,16 @@ function _M.filter(ctx, conf, plugins, route_conf, phase) if plugin_conf._meta and plugin_conf._meta.priority then custom_sort = true end + Review Comment: @nic-6443 these modifications only affect the `rewrite_in_consumer` phase. -- 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]
Re: [PR] fix: plugins repeats the rewrite phase when consumer configs with a p… [apisix]
hachi029 commented on code in PR #12048: URL: https://github.com/apache/apisix/pull/12048#discussion_r2011414662 ## apisix/plugin.lua: ## @@ -506,6 +506,16 @@ function _M.filter(ctx, conf, plugins, route_conf, phase) if plugin_conf._meta and plugin_conf._meta.priority then custom_sort = true end + Review Comment: these modifications only affect the 'rewrite_in_consumer' phase. -- 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]
Re: [PR] fix: plugins repeats the rewrite phase when consumer configs with a p… [apisix]
nic-6443 commented on code in PR #12048: URL: https://github.com/apache/apisix/pull/12048#discussion_r2011253938 ## apisix/plugin.lua: ## @@ -506,6 +506,16 @@ function _M.filter(ctx, conf, plugins, route_conf, phase) if plugin_conf._meta and plugin_conf._meta.priority then custom_sort = true end + Review Comment: I found that there is a place where the `run_plugin` function is executed without executing `filter` first: https://github.com/apache/apisix/blob/52d8fea3dbc93c4b59ac6682902955c6146af4ad/apisix/init.lua#L437-L451 so I am not sure if moving it to `filter` will cause any issues. -- 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]
Re: [PR] fix: plugins repeats the rewrite phase when consumer configs with a p… [apisix]
hachi029 commented on code in PR #12048: URL: https://github.com/apache/apisix/pull/12048#discussion_r2010185654 ## apisix/plugin.lua: ## @@ -506,6 +506,16 @@ function _M.filter(ctx, conf, plugins, route_conf, phase) if plugin_conf._meta and plugin_conf._meta.priority then custom_sort = true end + Review Comment: Ok. Is it better move ``` if plugins[i].type == "auth" then plugins[i + 1]._skip_rewrite_in_consumer = true end ``` into _M.filter(...) ? Because it is the function _M.filter determines or marks whether a plugin will be executed or not . -- 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]
Re: [PR] fix: plugins repeats the rewrite phase when consumer configs with a p… [apisix]
hachi029 commented on code in PR #12048: URL: https://github.com/apache/apisix/pull/12048#discussion_r2010185654 ## apisix/plugin.lua: ## @@ -506,6 +506,16 @@ function _M.filter(ctx, conf, plugins, route_conf, phase) if plugin_conf._meta and plugin_conf._meta.priority then custom_sort = true end + Review Comment: Ok. Is it better to move ``` if plugins[i].type == "auth" then plugins[i + 1]._skip_rewrite_in_consumer = true end ``` into _M.filter(...) ? Because it is the function _M.filter that determines or marks whether a plugin will be executed or not . -- 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]
Re: [PR] fix: plugins repeats the rewrite phase when consumer configs with a p… [apisix]
hachi029 commented on code in PR #12048: URL: https://github.com/apache/apisix/pull/12048#discussion_r2010185654 ## apisix/plugin.lua: ## @@ -506,6 +506,16 @@ function _M.filter(ctx, conf, plugins, route_conf, phase) if plugin_conf._meta and plugin_conf._meta.priority then custom_sort = true end + Review Comment: Ok. Is it better move ``` if plugins[i].type == "auth" then plugins[i + 1]._skip_rewrite_in_consumer = true end ``` into _M.filter(...) ? because it is the function _M.filter determines or marks whether a plugin will be executed or not . -- 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]
Re: [PR] fix: plugins repeats the rewrite phase when consumer configs with a p… [apisix]
hachi029 commented on code in PR #12048: URL: https://github.com/apache/apisix/pull/12048#discussion_r2010185654 ## apisix/plugin.lua: ## @@ -506,6 +506,16 @@ function _M.filter(ctx, conf, plugins, route_conf, phase) if plugin_conf._meta and plugin_conf._meta.priority then custom_sort = true end + Review Comment: Ok. Is it better move ``` if plugins[i].type == "auth" then plugins[i + 1]._skip_rewrite_in_consumer = true end ``` into _M.filter(...) ? because it is the function _M.filter determines or marks whether a plugin will be executed or not . -- 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]
Re: [PR] fix: plugins repeats the rewrite phase when consumer configs with a p… [apisix]
hachi029 commented on code in PR #12048: URL: https://github.com/apache/apisix/pull/12048#discussion_r2010185654 ## apisix/plugin.lua: ## @@ -506,6 +506,16 @@ function _M.filter(ctx, conf, plugins, route_conf, phase) if plugin_conf._meta and plugin_conf._meta.priority then custom_sort = true end + Review Comment: Ok. Is it better move ``` if plugins[i].type == "auth" then plugins[i + 1]._skip_rewrite_in_consumer = true end ``` into _M.filter(...) ? because it is the function _M.filter determines or marks whether a plugin will be executed or not . -- 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]
Re: [PR] fix: plugins repeats the rewrite phase when consumer configs with a p… [apisix]
nic-6443 commented on code in PR #12048: URL: https://github.com/apache/apisix/pull/12048#discussion_r2009812263 ## apisix/plugin.lua: ## @@ -506,6 +506,16 @@ function _M.filter(ctx, conf, plugins, route_conf, phase) if plugin_conf._meta and plugin_conf._meta.priority then custom_sort = true end + Review Comment: how about move this check to https://github.com/apache/apisix/blob/52d8fea3dbc93c4b59ac6682902955c6146af4ad/apisix/plugin.lua#L1184-L1186 as ``` if plugins[i].type == "auth" or (not plugins[i + 1]._from_consumer) then plugins[i + 1]._skip_rewrite_in_consumer = true end ``` so that we can keep skip logic in one place. -- 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]
