AlinsRan commented on code in PR #13757:
URL: https://github.com/apache/apisix/pull/13757#discussion_r3687005926
##########
apisix/core/table.lua:
##########
@@ -130,7 +131,8 @@ do
copied[orig] = copy
for orig_key, orig_value in pairs(orig) do
local path = parent .. "." .. tostring(orig_key)
- if opts and array_find(opts.shallows, path) then
+ if opts and (array_find(opts.shallows, path) or
+ (opts.shallow_prefix and str_has_prefix(parent,
opts.shallow_prefix))) then
copy[orig_key] = orig_value
Review Comment:
Fixed the boundary in 6eee234: the match now has to stop at a path
separator, so `self.value.plugins` no longer claims a sibling like
`self.value.plugins2`. `t/core/table.t` TEST 14 covers it (it returns `true`
for the sibling before the fix, `false` after). No live caller was affected —
the only `plugins`-ish sibling under `self.value` is `plugin_config_id`, which
diverges before the prefix ends.
On the type tolerance: I would rather keep `has_prefix` raising here.
`opts.shallows` is only tolerant because `array_find` returns `nil` for a
non-table, i.e. a wrong type is silently treated as "not configured" — for
`shallow_prefix` that would silently disable the sharing and bring back the
state loss this PR fixes, which is much harder to notice than an error on the
first request.
##########
apisix/plugin.lua:
##########
@@ -818,7 +818,10 @@ local function merge_consumer_route(route_conf,
consumer_conf, consumer_group_co
return route_conf
end
- local new_route_conf = core.table.deepcopy(route_conf)
+ -- the plugins subtree is fully overwritten below, so there is no need to
+ -- deep-copy it; shallow-copy that subtree to avoid the wasted work
+ local new_route_conf = core.table.deepcopy(route_conf,
+ { shallow_prefix = "self.value.plugins" })
Review Comment:
Right, and the comment was wrong on the facts too — the merge only
overwrites the plugin names carried by the consumer/consumer group, so a
route-only plugin is never overwritten, and those are exactly the confs that
need to keep their identity. Rewritten in 6eee234 to state the real reason
(preserve cached plugin state; the `plugins` container is still a fresh table
so the merge does not mutate the original route conf).
--
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]
