Copilot commented on code in PR #6288:
URL: https://github.com/apache/shenyu/pull/6288#discussion_r2762386536
##########
shenyu-plugin/shenyu-plugin-mcp-server/src/main/java/org/apache/shenyu/plugin/mcp/server/callback/ShenyuToolCallback.java:
##########
@@ -233,7 +239,12 @@ private String executeToolCall(final ServerWebExchange
originExchange,
responseFuture.completeExceptionally(e);
}
})
- .doOnSuccess(v -> LOG.debug("Plugin chain completed
successfully for session: {}", sessionId))
+ .doOnSuccess(v -> {
+ LOG.debug("Plugin chain completed successfully for
session: {}", sessionId);
+ if (!responseFuture.isDone()) {
+ responseFuture.complete("");
+ }
+ })
Review Comment:
The new fallback logic for request body handling (lines 366-370) and the
doOnSuccess completion logic (lines 242-247) lack test coverage. Given that the
repository has comprehensive tests for the ShenyuToolCallback class, these new
code paths should have corresponding test cases to verify correct behavior,
especially for edge cases like when argsToJsonBody is false but inputJson has
content, and when responseFuture is completed by multiple code paths.
##########
shenyu-plugin/shenyu-plugin-mcp-server/src/main/java/org/apache/shenyu/plugin/mcp/server/callback/ShenyuToolCallback.java:
##########
@@ -603,8 +618,11 @@ private ServerHttpResponseDecorator
createResponseDecorator(final ServerWebExcha
*/
private ServerWebExchange handleRequestBody(final ServerWebExchange
decoratedExchange,
final RequestConfig
requestConfig) {
- if (isRequestBodyMethod(requestConfig.getMethod()) &&
requestConfig.getBodyJson().size() > 0) {
- return new BodyWriterExchange(decoratedExchange,
requestConfig.getBodyJson().toString());
+ if (isRequestBodyMethod(requestConfig.getMethod())) {
+ String body = requestConfig.getBodyJson().size() > 0
+ ? requestConfig.getBodyJson().toString()
+ : "{}";
+ return new BodyWriterExchange(decoratedExchange, body);
}
Review Comment:
The change to always wrap request bodies in a BodyWriterExchange for body
methods, even when the body is empty (defaulting to "{}"), could have
unintended consequences. For some endpoints, an empty body might be
semantically different from a body containing an empty JSON object. Consider
whether this should be configurable or if there are cases where no body should
be sent at all for POST/PUT/PATCH requests.
```suggestion
if (isRequestBodyMethod(requestConfig.getMethod())
&& requestConfig.getBodyJson() != null
&& requestConfig.getBodyJson().size() > 0) {
String body = requestConfig.getBodyJson().toString();
return new BodyWriterExchange(decoratedExchange, body);
}
// For methods without a request body or when no body is configured,
// return the original exchange to avoid sending an unintended "{}"
body.
```
##########
shenyu-plugin/shenyu-plugin-httpclient/src/main/java/org/apache/shenyu/plugin/httpclient/AbstractHttpClientPlugin.java:
##########
@@ -74,7 +74,10 @@ public final Mono<Void> execute(final ServerWebExchange
exchange, final ShenyuPl
final int retryTimes = (int)
Optional.ofNullable(exchange.getAttribute(Constants.HTTP_RETRY)).orElse(0);
final String retryStrategy = (String)
Optional.ofNullable(exchange.getAttribute(Constants.RETRY_STRATEGY)).orElseGet(RetryEnum.CURRENT::getName);
LogUtils.debug(LOG, () -> String.format("The request urlPath is: %s,
retryTimes is : %s, retryStrategy is : %s", uri, retryTimes, retryStrategy));
- final Mono<R> response = doRequest(exchange,
exchange.getRequest().getMethod().name(), uri, exchange.getRequest().getBody())
+ final Mono<R> response = doRequest(exchange,
+ Objects.nonNull(exchange.getRequest().getMethod()) ?
exchange.getRequest().getMethod().name() : "UNKNOWN",
+ uri,
+ exchange.getRequest().getBody())
Review Comment:
The null check for the request method is a good defensive addition. However,
this same null safety pattern is not applied in DefaultRetryStrategy.java at
line 131, which also calls `exchange.getRequest().getMethod().name()` without a
null check. This creates an inconsistency where the initial request is
protected but retry attempts are not. Consider applying the same null safety
pattern to DefaultRetryStrategy to ensure consistent error handling.
##########
shenyu-plugin/shenyu-plugin-httpclient/src/main/java/org/apache/shenyu/plugin/httpclient/NettyHttpClientPlugin.java:
##########
@@ -89,7 +99,11 @@ protected Mono<HttpClientResponse> doRequest(final
ServerWebExchange exchange, f
} else {
throw new IllegalStateException("Unable to set status
code on response: " + res.status().code() + ", " + response.getClass());
}
- response.getHeaders().putAll(headers);
+ try {
+ response.getHeaders().putAll(headers);
+ } catch (UnsupportedOperationException ex) {
+ LOG.debug("Skip setting response headers because they
are read-only: {}", ex.getMessage());
Review Comment:
Catching UnsupportedOperationException when setting response headers is a
good defensive measure for handling read-only headers. However, the log level
is set to DEBUG, which means these errors might be silently ignored in
production. If headers are read-only in certain scenarios (e.g., when using the
NonCommittingMcpResponseDecorator), this could indicate a configuration issue
or unexpected decorator usage. Consider logging at WARN level or adding context
about which scenarios are expected to have read-only headers.
```suggestion
LOG.warn("Failed to set response headers because
they are read-only. "
+ "This may indicate unexpected response
decorator usage. "
+ "responseClass={}, statusCode={},
message={}",
response.getClass().getName(),
res.status().code(), ex.getMessage());
```
##########
shenyu-plugin/shenyu-plugin-mcp-server/src/main/java/org/apache/shenyu/plugin/mcp/server/callback/ShenyuToolCallback.java:
##########
@@ -233,7 +239,12 @@ private String executeToolCall(final ServerWebExchange
originExchange,
responseFuture.completeExceptionally(e);
}
})
- .doOnSuccess(v -> LOG.debug("Plugin chain completed
successfully for session: {}", sessionId))
+ .doOnSuccess(v -> {
+ LOG.debug("Plugin chain completed successfully for
session: {}", sessionId);
+ if (!responseFuture.isDone()) {
+ responseFuture.complete("");
+ }
Review Comment:
The doOnSuccess handler now completes the responseFuture with an empty
string if not already done. This creates a race condition: if the response
decorator's setComplete() method completes the future with processed response
data (either empty or with template processing), and this doOnSuccess also
tries to complete it, the behavior depends on which completes first. While the
isDone() check prevents an exception, this could lead to inconsistent response
handling. Consider whether the completion should only happen in the decorator's
setComplete() method, or add a comment explaining why both paths are necessary.
```suggestion
// Successful completion of the plugin chain.
// The actual response body (if any) must be propagated
via the response decorator,
// which is responsible for completing responseFuture.
LOG.debug("Plugin chain completed successfully for
session: {}", sessionId);
```
##########
shenyu-plugin/shenyu-plugin-mcp-server/src/main/java/org/apache/shenyu/plugin/mcp/server/callback/ShenyuToolCallback.java:
##########
@@ -215,11 +215,17 @@ private String executeToolCall(final ServerWebExchange
originExchange,
final String configStr,
final String input) {
+ final RequestConfigHelper configHelper = new
RequestConfigHelper(configStr);
+ final String toolMethod = configHelper.getMethod();
+ final String toolUrl = configHelper.getUrlTemplate();
+ final JsonObject requestTemplate = configHelper.getRequestTemplate();
+ final String toolTimeout = requestTemplate.has("timeout") ?
requestTemplate.get("timeout").getAsString() : "default";
final CompletableFuture<String> responseFuture = new
CompletableFuture<>();
final ServerWebExchange decoratedExchange = buildDecoratedExchange(
originExchange, responseFuture, sessionId, configStr, input);
- LOG.debug("Executing plugin chain for session: {}", sessionId);
+ LOG.debug("Executing plugin chain for session: {} (method: {}, url:
{}, timeoutMs: {})",
Review Comment:
The logging enhancement extracts timeout from the requestTemplate as a
string but labels it as "timeoutMs" in the log message. This could be
misleading if the timeout value in the template is not in milliseconds or is a
non-numeric value like "default". Consider either validating the timeout format
or using a more generic label like "timeout" instead of "timeoutMs".
```suggestion
LOG.debug("Executing plugin chain for session: {} (method: {}, url:
{}, timeout: {})",
```
##########
shenyu-plugin/shenyu-plugin-httpclient/src/main/java/org/apache/shenyu/plugin/httpclient/NettyHttpClientPlugin.java:
##########
@@ -89,7 +99,11 @@ protected Mono<HttpClientResponse> doRequest(final
ServerWebExchange exchange, f
} else {
throw new IllegalStateException("Unable to set status
code on response: " + res.status().code() + ", " + response.getClass());
}
- response.getHeaders().putAll(headers);
+ try {
+ response.getHeaders().putAll(headers);
+ } catch (UnsupportedOperationException ex) {
+ LOG.debug("Skip setting response headers because they
are read-only: {}", ex.getMessage());
+ }
Review Comment:
The new try-catch block for handling UnsupportedOperationException when
setting response headers (lines 102-106) lacks test coverage. Given that the
repository has comprehensive tests for NettyHttpClientPlugin, there should be a
test case verifying that the plugin handles read-only headers gracefully
without throwing exceptions.
##########
shenyu-plugin/shenyu-plugin-mcp-server/src/main/java/org/apache/shenyu/plugin/mcp/server/callback/ShenyuToolCallback.java:
##########
@@ -352,7 +363,11 @@ private RequestConfig buildRequestConfig(final String
configStr, final JsonObjec
final String path = RequestConfigHelper.buildPath(urlTemplate,
argsPosition, inputJson);
// Build body with parameter formatting (only format body parameters
that need it)
- final JsonObject bodyJson = buildFormattedBodyJson(argsToJsonBody,
argsPosition, inputJson);
+ JsonObject bodyJson = buildFormattedBodyJson(argsToJsonBody,
argsPosition, inputJson);
+ // Fallback: if argsToJsonBody is false but input has content for body
methods, use the raw input as body
+ if (!argsToJsonBody && bodyJson.size() == 0 &&
isRequestBodyMethod(method) && inputJson.size() > 0) {
Review Comment:
The fallback logic added here creates potential ambiguity. If
`argsToJsonBody` is false but `inputJson` has content, the code assumes the
entire input should be used as the body. However, this may not be the intended
behavior if the parameters are supposed to be in query strings or path
parameters instead. Consider adding a log warning when this fallback is
triggered to help with debugging, or verify that this is the correct
interpretation of the configuration.
```suggestion
if (!argsToJsonBody && bodyJson.size() == 0 &&
isRequestBodyMethod(method) && inputJson.size() > 0) {
LOG.warn("Using fallback body mapping: argsToJsonBody=false and
no body-mapped args, "
+ "but method {} expects a request body and inputJson
has content. "
+ "Using full inputJson as request body. Check tool
configuration (urlTemplate={}, argsPosition={}).",
method, urlTemplate, argsPosition);
```
--
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]
