Copilot commented on code in PR #13251:
URL: https://github.com/apache/apisix/pull/13251#discussion_r3101301128
##########
apisix/plugins/ai-providers/base.lua:
##########
@@ -181,6 +182,16 @@ function _M.build_request(self, ctx, conf, request_body,
opts)
request_body[opt] = val
end
end
+
+ -- Inject per-target-protocol request body override (deep merge)
+ if opts.request_body_override_map then
+ local patch = opts.request_body_override_map[ctx.ai_target_protocol]
+ if patch then
+ core.log.info("applying request_body override for target protocol
'",
+ ctx.ai_target_protocol, "'")
+ deep_merge(request_body, patch)
Review Comment:
`deep_merge` can return a replacement value when the target is not
considered a “plain object” (e.g., when `request_body` is an empty table or
otherwise non-object). Since the return value is ignored here, the override may
be silently skipped in those cases. Assign the result back (e.g., rebind
`request_body`) so wholesale replacement semantics work as intended.
```suggestion
request_body = deep_merge(request_body, patch)
```
##########
apisix/plugins/ai-proxy/schema.lua:
##########
@@ -72,6 +74,40 @@ local model_options_schema = {
additionalProperties = true,
}
+-- Build per-target-protocol request body override schema.
+-- Each registered protocol gets an optional "any-shape object" entry.
+-- Values are applied via deep-merge after the model_options flat overwrite.
+local request_body_override_properties = {}
+for _, proto_name in pairs(protocols.names()) do
+ request_body_override_properties[proto_name] = {
Review Comment:
`protocols.names()` returns an array-like table. Iterating it with `pairs()`
works but is non-idiomatic and can produce surprising results if the table ever
becomes sparse; using `ipairs()` makes the intent explicit and keeps ordering
stable if the implementation changes.
##########
apisix/plugins/ai-providers/base.lua:
##########
@@ -36,6 +36,7 @@ local transport_http =
require("apisix.plugins.ai-transport.http")
local transport_auth = require("apisix.plugins.ai-transport.auth")
local log_sanitize = require("apisix.utils.log-sanitize")
local protocols = require("apisix.plugins.ai-protocols")
+local deep_merge = require("apisix.plugins.ai-proxy.merge").deep_merge
local ngx = ngx
Review Comment:
This introduces a dependency from the provider base layer
(`apisix.plugins.ai-providers.*`) back into the ai-proxy plugin
(`apisix.plugins.ai-proxy.merge`). That coupling makes it harder to reuse
providers in other plugins and increases the risk of future circular
dependencies. Consider moving `deep_merge` to a shared AI utility module (e.g.,
under `apisix/plugins/ai-*` common area) or into `ai-providers` itself.
--
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]
