Copilot commented on code in PR #8035:
URL: https://github.com/apache/incubator-seata/pull/8035#discussion_r3013712987
##########
namingserver/src/main/java/org/apache/seata/namingserver/filter/ConsoleRemotingFilter.java:
##########
@@ -108,33 +108,48 @@ public void doFilter(ServletRequest servletRequest,
ServletResponse servletRespo
.forEach(headerName ->
headers.add(headerName, request.getHeader(headerName)));
// Create the HttpEntity with headers and body
- HttpEntity<byte[]> httpEntity = new
HttpEntity<>(request.getCachedBody(), headers);
- HttpMethod httpMethod;
+ HttpMethod httpMethod =
HttpMethod.valueOf(request.getMethod());
+
+ // GET/HEAD methods should not have a body; other
methods may include a body as needed.
+ HttpEntity<byte[]> httpEntity;
+ if (HttpMethod.GET.equals(httpMethod) ||
HttpMethod.HEAD.equals(httpMethod)) {
+ headers.remove(HttpHeaders.CONTENT_LENGTH);
+ headers.remove(HttpHeaders.TRANSFER_ENCODING);
+ httpEntity = new HttpEntity<>(headers); //
headers-only
+ } else {
+ byte[] body = request.getCachedBody();
+ httpEntity = (body == null || body.length == 0)
+ ? new HttpEntity<>(headers)
Review Comment:
For non-GET/HEAD methods, converting an empty body into a `null` entity
(`new HttpEntity<>(headers)`) can break OkHttp for methods that require a body
(e.g., POST/PUT/PATCH), potentially throwing `IllegalArgumentException`.
Consider still sending an empty body (byte[0]) for such methods, and only force
body-less requests for GET/HEAD.
```suggestion
? new HttpEntity<>(new byte[0],
headers)
```
##########
namingserver/src/main/java/org/apache/seata/namingserver/filter/ConsoleRemotingFilter.java:
##########
@@ -57,7 +57,7 @@ public class ConsoleRemotingFilter implements Filter {
private final Pattern urlPattern = Pattern.compile(CONSOLE_PATTERN);
- private final Logger logger =
LoggerFactory.getLogger(ConsoleRemotingFilter.class);
+ private final Logger LOGGER =
LoggerFactory.getLogger(ConsoleRemotingFilter.class);
Review Comment:
`LOGGER` is an instance field (`private final`) but is named like a
constant; this is inconsistent with the common `private static final Logger
LOGGER` pattern in namingserver. Consider either making it `static final` or
renaming back to `logger` to avoid implying immutability/static usage.
```suggestion
private static final Logger LOGGER =
LoggerFactory.getLogger(ConsoleRemotingFilter.class);
```
##########
namingserver/src/main/java/org/apache/seata/namingserver/filter/ConsoleRemotingFilter.java:
##########
@@ -108,33 +108,48 @@ public void doFilter(ServletRequest servletRequest,
ServletResponse servletRespo
.forEach(headerName ->
headers.add(headerName, request.getHeader(headerName)));
// Create the HttpEntity with headers and body
- HttpEntity<byte[]> httpEntity = new
HttpEntity<>(request.getCachedBody(), headers);
- HttpMethod httpMethod;
+ HttpMethod httpMethod =
HttpMethod.valueOf(request.getMethod());
+
Review Comment:
`HttpMethod.valueOf(request.getMethod())` is executed before the
`try/catch`, so an unsupported method will still throw
`IllegalArgumentException` and bypass the intended 405 handling. Parse/validate
the method once inside the `try` block and reuse it (also removes the current
double-parsing).
##########
namingserver/src/main/java/org/apache/seata/namingserver/filter/ConsoleRemotingFilter.java:
##########
@@ -108,33 +108,48 @@ public void doFilter(ServletRequest servletRequest,
ServletResponse servletRespo
.forEach(headerName ->
headers.add(headerName, request.getHeader(headerName)));
// Create the HttpEntity with headers and body
- HttpEntity<byte[]> httpEntity = new
HttpEntity<>(request.getCachedBody(), headers);
- HttpMethod httpMethod;
+ HttpMethod httpMethod =
HttpMethod.valueOf(request.getMethod());
+
+ // GET/HEAD methods should not have a body; other
methods may include a body as needed.
+ HttpEntity<byte[]> httpEntity;
+ if (HttpMethod.GET.equals(httpMethod) ||
HttpMethod.HEAD.equals(httpMethod)) {
+ headers.remove(HttpHeaders.CONTENT_LENGTH);
+ headers.remove(HttpHeaders.TRANSFER_ENCODING);
+ httpEntity = new HttpEntity<>(headers); //
headers-only
+ } else {
+ byte[] body = request.getCachedBody();
+ httpEntity = (body == null || body.length == 0)
+ ? new HttpEntity<>(headers)
+ : new HttpEntity<>(body, headers);
+ }
+
try {
httpMethod =
HttpMethod.valueOf(request.getMethod());
} catch (IllegalArgumentException ex) {
- logger.error("Unsupported HTTP method: {}",
request.getMethod(), ex);
+ LOGGER.error("Unsupported HTTP method: {}",
request.getMethod(), ex);
response.setStatus(HttpServletResponse.SC_METHOD_NOT_ALLOWED);
return;
}
try {
- ResponseEntity<byte[]> responseEntity =
restTemplate.exchange(
- URI.create(targetUrl), httpMethod,
httpEntity, byte[].class);
+ ResponseEntity<byte[]> responseEntity =
restTemplate.exchange(URI.create(targetUrl), httpMethod, httpEntity,
byte[].class);
responseEntity.getHeaders().forEach((key,
value) -> {
- value.forEach(v -> response.addHeader(key,
v));
+ if (!"Content-Type".equalsIgnoreCase(key))
{
+ value.forEach(v ->
response.addHeader(key, v));
+ }
});
- response.setStatus(
-
responseEntity.getStatusCode().value());
+
response.setContentType("application/json;charset=UTF-8");
Review Comment:
The proxy response unconditionally sets `application/json;charset=UTF-8`,
which can corrupt non-JSON console responses (the URL pattern is
`/api/.*/console/.*`). Prefer preserving the upstream `Content-Type` (or only
defaulting when it is missing) instead of forcing JSON for all responses.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
