Copilot commented on code in PR #6350:
URL: https://github.com/apache/shenyu/pull/6350#discussion_r3377444057
##########
shenyu-plugin/shenyu-plugin-httpclient/src/main/java/org/apache/shenyu/plugin/httpclient/AbstractHttpClientPlugin.java:
##########
@@ -117,6 +117,10 @@ public final Mono<Void> execute(final ServerWebExchange
exchange, final ShenyuPl
protected abstract Mono<R> doRequest(ServerWebExchange exchange, String
httpMethod,
URI uri, Flux<DataBuffer> body);
+ protected boolean isRequestBodyRequired(final String httpMethod) {
+ return !"GET".equals(httpMethod) && !"HEAD".equals(httpMethod);
+ }
Review Comment:
`isRequestBodyRequired` uses case-sensitive comparisons. Since callers may
provide method names in different cases (or with leading/trailing whitespace),
this can incorrectly treat GET/HEAD as body-eligible. Using a case-insensitive
check makes the shared helper more robust.
##########
shenyu-plugin/shenyu-plugin-httpclient/src/main/java/org/apache/shenyu/plugin/httpclient/NettyHttpClientPlugin.java:
##########
@@ -76,7 +76,16 @@ protected Mono<HttpClientResponse> doRequest(final
ServerWebExchange exchange, f
headers.add(HttpHeaders.HOST,
request.getHeaders().getFirst(HttpHeaders.HOST));
}
}).request(HttpMethod.valueOf(httpMethod)).uri(uri.toASCIIString())
- .send((req, nettyOutbound) ->
nettyOutbound.send(body.map(dataBuffer -> ((NettyDataBuffer)
dataBuffer).getNativeBuffer())))
+ .send((req, nettyOutbound) -> {
+ // Do not send a request body for GET/HEAD. Otherwise
Reactor Netty may add
+ // Transfer-Encoding: chunked and cause compatibility
issues with some upstream servers.
+ if (isRequestBodyRequired(httpMethod)) {
Review Comment:
For GET/HEAD, the request body is skipped, but the request is still built
via `.send(...)` and the original body-related headers are still forwarded. To
ensure no-body semantics (and avoid chunked transfer encoding/content-length
inconsistencies), strip body-related headers and avoid invoking `.send(...)`
when the method is GET/HEAD.
##########
shenyu-plugin/shenyu-plugin-httpclient/src/main/java/org/apache/shenyu/plugin/httpclient/AbstractHttpClientPlugin.java:
##########
@@ -117,6 +117,10 @@ public final Mono<Void> execute(final ServerWebExchange
exchange, final ShenyuPl
protected abstract Mono<R> doRequest(ServerWebExchange exchange, String
httpMethod,
URI uri, Flux<DataBuffer> body);
+ protected boolean isRequestBodyRequired(final String httpMethod) {
+ return !"GET".equals(httpMethod) && !"HEAD".equals(httpMethod);
+ }
Review Comment:
The new GET/HEAD body-skipping behavior is not covered by tests. There are
existing unit tests for the HTTP client plugins, but they don’t assert the
outgoing request method/headers/body. Please add coverage that verifies GET and
HEAD do not forward a request body and do not propagate body-related headers
(Content-Length / Transfer-Encoding), while non-GET/HEAD methods keep existing
body forwarding behavior.
--
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]
