Copilot commented on code in PR #8035: URL: https://github.com/apache/incubator-seata/pull/8035#discussion_r3014422338
########## changes/en-us/2.x.md: ########## @@ -35,6 +35,9 @@ Add changes here for all PR submitted to the 2.x branch. - [[#7956](https://github.com/apache/incubator-seata/pull/7956)] fix empty jacoco report on local when jdk above 17 - [[#7965](https://github.com/apache/incubator-seata/pull/7965)] fix the issue where different element order in two lists causes failure to set the index as the primary key - [[#7992](https://github.com/apache/incubator-seata/pull/7992)] fix report branch transaction status without setting branch type +- [[#8035](https://github.com/apache/incubator-seata/pull/8035)] fix IllegalArgumentException when GET request has request body + + Review Comment: There are multiple consecutive blank lines added after the new bugfix entry. Please keep section spacing consistent with the rest of this changelog (typically a single blank line between sections) to reduce noise in the rendered markdown. ```suggestion ``` ########## namingserver/src/main/java/org/apache/seata/namingserver/filter/ConsoleRemotingFilter.java: ########## @@ -108,33 +109,49 @@ 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; 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; } + + // 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<>(new byte[0], headers) + : new HttpEntity<>(body, headers); Review Comment: For non-GET/HEAD requests, this still sends a request body even when the incoming body is empty (`new byte[0]`). With the OkHttp request factory (see WebConfig), a non-null body can trigger `IllegalArgumentException: method <X> must not have a request body` for HTTP methods that do not permit bodies (e.g., TRACE), and it also changes semantics vs forwarding a body-less request. Consider using a headers-only HttpEntity when `request.getCachedBody().length == 0` (and removing Content-Length/Transfer-Encoding in that case as well), and only attaching a body when there are actual bytes to forward. ```suggestion if (body == null || body.length == 0) { headers.remove(HttpHeaders.CONTENT_LENGTH); headers.remove(HttpHeaders.TRANSFER_ENCODING); httpEntity = new HttpEntity<>(headers); // headers-only for empty body } else { httpEntity = new HttpEntity<>(body, headers); } ``` -- 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]
