Copilot commented on code in PR #7818:
URL: https://github.com/apache/incubator-seata/pull/7818#discussion_r2570849476
##########
server/src/main/java/org/apache/seata/server/filter/RaftRequestFilter.java:
##########
@@ -101,4 +123,26 @@ private boolean isPass(String group) {
// Non-raft mode always allows requests
return Optional.ofNullable(GROUP_PREVENT.get(group)).orElse(false);
}
+
+ private String getUri(HttpFilterContext<?> context) {
+ // Assuming HttpRequest has uri, but for SimpleHttp2Request, uri is in
request.getUri()
+ // For simplicity, since it's HTTP, we can cast or handle differently
+ // But to keep it simple, perhaps add a method to get uri
+ // For now, assume it's HttpRequest
+ if (context.getRequest() instanceof
io.netty.handler.codec.http.HttpRequest) {
+ io.netty.handler.codec.http.HttpRequest httpRequest =
(io.netty.handler.codec.http.HttpRequest) context.getRequest();
+ return httpRequest.uri();
+ } else if (context.getRequest() instanceof SimpleHttp2Request) {
+ return ((SimpleHttp2Request) context.getRequest()).getPath();
+ }
+ // For HTTP/2, similar
Review Comment:
[nitpick] Similar to `getGroup()`, this method contains verbose
implementation notes that should be cleaned up. The comments on lines 128-131
and 138 appear to be leftover implementation notes. Consider simplifying or
removing them.
```suggestion
if (context.getRequest() instanceof
io.netty.handler.codec.http.HttpRequest) {
io.netty.handler.codec.http.HttpRequest httpRequest =
(io.netty.handler.codec.http.HttpRequest) context.getRequest();
return httpRequest.uri();
} else if (context.getRequest() instanceof SimpleHttp2Request) {
return ((SimpleHttp2Request) context.getRequest()).getPath();
}
```
##########
server/src/main/java/org/apache/seata/server/filter/RaftRequestFilter.java:
##########
@@ -16,80 +16,102 @@
*/
package org.apache.seata.server.filter;
+import io.netty.handler.codec.http.HttpRequest;
+import io.netty.handler.codec.http2.Http2Headers;
+import org.apache.seata.common.loader.LoadLevel;
import org.apache.seata.common.store.SessionMode;
import org.apache.seata.common.util.StringUtils;
-import org.apache.seata.core.exception.TransactionException;
-import org.apache.seata.core.exception.TransactionExceptionCode;
+import org.apache.seata.core.exception.HttpRequestFilterException;
+import org.apache.seata.core.rpc.netty.http.SimpleHttp2Request;
+import org.apache.seata.core.rpc.netty.http.filter.HttpFilterContext;
+import org.apache.seata.core.rpc.netty.http.filter.HttpRequestFilter;
+import org.apache.seata.core.rpc.netty.http.filter.HttpRequestFilterChain;
import org.apache.seata.server.cluster.listener.ClusterChangeEvent;
import org.apache.seata.server.cluster.raft.context.SeataClusterContext;
-import org.apache.seata.server.console.exception.ConsoleException;
import org.apache.seata.server.store.StoreConfig;
import org.springframework.context.ApplicationListener;
-import org.springframework.context.annotation.Conditional;
-import org.springframework.http.HttpMethod;
import org.springframework.stereotype.Component;
-import javax.servlet.Filter;
-import javax.servlet.FilterChain;
-import javax.servlet.FilterConfig;
-import javax.servlet.ServletException;
-import javax.servlet.ServletRequest;
-import javax.servlet.ServletResponse;
-import javax.servlet.http.HttpServletRequest;
-import java.io.IOException;
+import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.concurrent.ConcurrentHashMap;
import static org.apache.seata.common.Constants.RAFT_GROUP_HEADER;
/**
- * Raft Leader Write Filter
+ * Raft Leader Write Filter for HTTP requests
*/
+@LoadLevel(name = "RaftRequest", order = 1)
@Component
-@Conditional(RaftCondition.class)
-public class RaftRequestFilter implements Filter,
ApplicationListener<ClusterChangeEvent> {
+public class RaftRequestFilter implements HttpRequestFilter,
ApplicationListener<ClusterChangeEvent> {
private static final Map<String, Boolean> GROUP_PREVENT = new
ConcurrentHashMap<>();
@Override
- public void init(FilterConfig filterConfig) throws ServletException {}
-
- @Override
- public void doFilter(ServletRequest servletRequest, ServletResponse
servletResponse, FilterChain filterChain)
- throws IOException, ServletException {
- HttpServletRequest httpRequest = (HttpServletRequest) servletRequest;
- String group = httpRequest.getParameter("unit");
- if (StringUtils.isBlank(group)) {
- group = httpRequest.getHeader(RAFT_GROUP_HEADER);
+ public void doFilter(HttpFilterContext<?> context, HttpRequestFilterChain
chain) throws HttpRequestFilterException {
+ String uri = getUri(context);
+ if (!isTargetUri(uri)) {
+ chain.doFilter(context);
+ return;
}
+ String group = getGroup(context);
if (group != null) {
SeataClusterContext.bindGroup(group);
}
try {
- String method = httpRequest.getMethod();
- if (!HttpMethod.GET.name().equalsIgnoreCase(method)) {
+ String method;
+ if (context.getRequest() instanceof SimpleHttp2Request) {
+ SimpleHttp2Request request =
(SimpleHttp2Request)context.getRequest();
+ method = request.getMethod().name();
+ } else {
+ HttpRequest request = (HttpRequest)context.getRequest();
+ method = request.method().name();
+ }
+ if (!"GET".equalsIgnoreCase(method)) {
if (!isPass(group)) {
- throw new ConsoleException(
- new TransactionException(
- TransactionExceptionCode.NotRaftLeader,
- " The current TC is not a leader node,
interrupt processing of transactions!"),
- " The current TC is not a leader node, interrupt
processing of transactions!");
+ throw new HttpRequestFilterException(
+ "The current TC is not a leader node, interrupt
processing of transactions!");
}
}
- filterChain.doFilter(servletRequest, servletResponse);
+ chain.doFilter(context);
} finally {
SeataClusterContext.unbindGroup();
}
}
+ private String getGroup(HttpFilterContext<?> context) {
+ // Try to get from query params
+ Map<String, List<String>> params =
context.getParamWrapper().getAllParamsAsMultiMap();
+ List<String> unitParams = params.get("unit");
+ if (unitParams != null && !unitParams.isEmpty()) {
+ return unitParams.get(0);
+ }
+ // Try to get from headers
+ // Assuming HttpRequest has headers, but for SimpleHttp2Request,
headers are in request.getHeaders()
+ // For simplicity, since it's HTTP, we can cast or handle differently
+ // But to keep it simple, perhaps add a method to get header
+ // For now, assume it's HttpRequest
+ if (context.getRequest() instanceof
io.netty.handler.codec.http.HttpRequest) {
+ io.netty.handler.codec.http.HttpRequest httpRequest =
(io.netty.handler.codec.http.HttpRequest) context.getRequest();
+ return httpRequest.headers().get(RAFT_GROUP_HEADER);
+ } else if (context.getRequest() instanceof SimpleHttp2Request) {
+ Http2Headers http2Headers = ((SimpleHttp2Request)
context.getRequest()).getHeaders();
+ return http2Headers.get(RAFT_GROUP_HEADER).toString();
+ }
+ // For HTTP/2, similar
Review Comment:
[nitpick] The comments in this method are verbose and contain implementation
notes that should be cleaned up. Consider simplifying to:
```java
// Try to get from query params first
// ...
// Try to get from headers
```
The comments on lines 91-94 and 102 appear to be leftover implementation
notes rather than helpful documentation.
##########
core/src/main/java/org/apache/seata/core/rpc/netty/http/filter/HttpRequestFilterChain.java:
##########
@@ -19,17 +19,29 @@
import org.apache.seata.core.exception.HttpRequestFilterException;
import java.util.List;
+import java.util.function.Consumer;
public class HttpRequestFilterChain {
private final List<HttpRequestFilter> filters;
+ private final Consumer<HttpFilterContext<?>> finalAction;
+ private int currentIndex = 0;
Review Comment:
The `currentIndex` field creates a thread-safety issue. When
`HttpRequestFilterChain` instances are shared across multiple concurrent
requests (as happens with the cached instance in `HttpRequestFilterManager`),
the mutable `currentIndex` can cause race conditions. Different threads could
interfere with each other's progress through the filter chain.
Consider making `currentIndex` part of the filter context or pass it as a
parameter through the chain, rather than storing it as mutable instance state.
##########
server/src/main/java/org/apache/seata/server/filter/RaftRequestFilter.java:
##########
@@ -16,80 +16,102 @@
*/
package org.apache.seata.server.filter;
+import io.netty.handler.codec.http.HttpRequest;
+import io.netty.handler.codec.http2.Http2Headers;
+import org.apache.seata.common.loader.LoadLevel;
import org.apache.seata.common.store.SessionMode;
import org.apache.seata.common.util.StringUtils;
-import org.apache.seata.core.exception.TransactionException;
-import org.apache.seata.core.exception.TransactionExceptionCode;
+import org.apache.seata.core.exception.HttpRequestFilterException;
+import org.apache.seata.core.rpc.netty.http.SimpleHttp2Request;
+import org.apache.seata.core.rpc.netty.http.filter.HttpFilterContext;
+import org.apache.seata.core.rpc.netty.http.filter.HttpRequestFilter;
+import org.apache.seata.core.rpc.netty.http.filter.HttpRequestFilterChain;
import org.apache.seata.server.cluster.listener.ClusterChangeEvent;
import org.apache.seata.server.cluster.raft.context.SeataClusterContext;
-import org.apache.seata.server.console.exception.ConsoleException;
import org.apache.seata.server.store.StoreConfig;
import org.springframework.context.ApplicationListener;
-import org.springframework.context.annotation.Conditional;
-import org.springframework.http.HttpMethod;
import org.springframework.stereotype.Component;
-import javax.servlet.Filter;
-import javax.servlet.FilterChain;
-import javax.servlet.FilterConfig;
-import javax.servlet.ServletException;
-import javax.servlet.ServletRequest;
-import javax.servlet.ServletResponse;
-import javax.servlet.http.HttpServletRequest;
-import java.io.IOException;
+import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.concurrent.ConcurrentHashMap;
import static org.apache.seata.common.Constants.RAFT_GROUP_HEADER;
/**
- * Raft Leader Write Filter
+ * Raft Leader Write Filter for HTTP requests
*/
+@LoadLevel(name = "RaftRequest", order = 1)
@Component
-@Conditional(RaftCondition.class)
-public class RaftRequestFilter implements Filter,
ApplicationListener<ClusterChangeEvent> {
+public class RaftRequestFilter implements HttpRequestFilter,
ApplicationListener<ClusterChangeEvent> {
private static final Map<String, Boolean> GROUP_PREVENT = new
ConcurrentHashMap<>();
Review Comment:
The variable name `GROUP_PREVENT` is misleading. Based on line 113 where
`setPrevent(event.getGroup(), event.isLeader())` is called, this map actually
stores whether a group's node IS a leader (true = leader, false = not leader),
not whether to prevent requests.
Consider renaming to `GROUP_IS_LEADER` or `GROUP_LEADER_STATUS` to better
reflect its actual meaning. This would also make the logic in `isPass()` more
intuitive.
##########
core/src/main/java/org/apache/seata/core/rpc/netty/http/HttpDispatchHandler.java:
##########
@@ -52,85 +52,91 @@ public class HttpDispatchHandler extends
BaseHttpChannelHandler<HttpRequest> {
@Override
protected void channelRead0(ChannelHandlerContext ctx, HttpRequest
httpRequest) {
- try {
- HttpFilterContext<HttpRequest> context =
- new HttpFilterContext<>(httpRequest, () -> new
HttpRequestParamWrapper(httpRequest));
- doFilterInternal(context);
- } catch (HttpRequestFilterException e) {
- LOGGER.warn("Request blocked by filter: {}", e.getMessage());
- sendErrorResponse(ctx, HttpResponseStatus.BAD_REQUEST, false);
- return;
- } catch (Exception e) {
- LOGGER.error("Unexpected error during filter execution: {}",
e.getMessage(), e);
- sendErrorResponse(ctx, HttpResponseStatus.INTERNAL_SERVER_ERROR,
false);
+ HttpFilterContext<HttpRequest> context = new HttpFilterContext<>(
+ httpRequest, ctx, HttpUtil.isKeepAlive(httpRequest) &&
httpRequest.protocolVersion().isKeepAliveDefault(),
+ HttpContext.HTTP_1_1, () -> new
HttpRequestParamWrapper(httpRequest));
+
+ // Parse request
+ QueryStringDecoder queryStringDecoder = new
QueryStringDecoder(httpRequest.uri());
+ String path = queryStringDecoder.path();
+ HttpInvocation httpInvocation =
ControllerManager.getHttpInvocation(path);
+
+ if (httpInvocation == null) {
+ sendErrorResponse(ctx, HttpResponseStatus.NOT_FOUND, false);
return;
}
- try {
- boolean keepAlive = HttpUtil.isKeepAlive(httpRequest)
- && httpRequest.protocolVersion().isKeepAliveDefault();
- QueryStringDecoder queryStringDecoder = new
QueryStringDecoder(httpRequest.uri());
- String path = queryStringDecoder.path();
- HttpInvocation httpInvocation =
ControllerManager.getHttpInvocation(path);
-
- if (httpInvocation == null) {
- sendErrorResponse(ctx, HttpResponseStatus.NOT_FOUND, false);
- return;
- }
+ context.setAttribute("httpInvocation", httpInvocation);
+ context.setAttribute("httpController", httpInvocation.getController());
+ context.setAttribute("handleMethod", httpInvocation.getMethod());
- HttpContext<HttpRequest> httpContext = new
HttpContext<>(httpRequest, ctx, keepAlive, HttpContext.HTTP_1_1);
- ObjectNode requestDataNode = OBJECT_MAPPER.createObjectNode();
- requestDataNode.set("param",
ParameterParser.convertParamMap(queryStringDecoder.parameters()));
-
- if (httpRequest.method() == HttpMethod.POST) {
- HttpPostRequestDecoder httpPostRequestDecoder = null;
- try {
- httpPostRequestDecoder = new
HttpPostRequestDecoder(httpRequest);
- ObjectNode bodyDataNode = OBJECT_MAPPER.createObjectNode();
- for (InterfaceHttpData interfaceHttpData :
httpPostRequestDecoder.getBodyHttpDatas()) {
- if (interfaceHttpData.getHttpDataType() !=
InterfaceHttpData.HttpDataType.Attribute) {
- continue;
- }
- Attribute attribute = (Attribute) interfaceHttpData;
- bodyDataNode.put(attribute.getName(),
attribute.getValue());
- }
- requestDataNode.putIfAbsent("body", bodyDataNode);
- } finally {
- if (httpPostRequestDecoder != null) {
- httpPostRequestDecoder.destroy();
+ ObjectNode requestDataNode = OBJECT_MAPPER.createObjectNode();
+ requestDataNode.set("param",
ParameterParser.convertParamMap(queryStringDecoder.parameters()));
+
+ if (httpRequest.method() == HttpMethod.POST) {
+ try {
+ HttpPostRequestDecoder httpPostRequestDecoder = new
HttpPostRequestDecoder(httpRequest);
+ ObjectNode bodyDataNode = OBJECT_MAPPER.createObjectNode();
+ for (InterfaceHttpData interfaceHttpData :
httpPostRequestDecoder.getBodyHttpDatas()) {
+ if (interfaceHttpData.getHttpDataType() !=
InterfaceHttpData.HttpDataType.Attribute) {
+ continue;
}
+ Attribute attribute = (Attribute) interfaceHttpData;
+ bodyDataNode.put(attribute.getName(),
attribute.getValue());
}
+ requestDataNode.putIfAbsent("body", bodyDataNode);
+ httpPostRequestDecoder.destroy();
+ } catch (Exception e) {
+ LOGGER.warn("Failed to parse POST body: {}", e.getMessage());
+ // Continue without body, no error response needed
}
Review Comment:
Potential resource leak: If an exception occurs between lines 78-87 (e.g.,
when reading body data or parsing attributes), the `HttpPostRequestDecoder`
won't be properly destroyed because line 88 will be skipped.
The original code had this in a try-finally block. Consider restructuring:
```java
HttpPostRequestDecoder httpPostRequestDecoder = null;
try {
httpPostRequestDecoder = new HttpPostRequestDecoder(httpRequest);
// ... processing ...
} catch (Exception e) {
LOGGER.warn("Failed to parse POST body: {}", e.getMessage());
} finally {
if (httpPostRequestDecoder != null) {
httpPostRequestDecoder.destroy();
}
}
```
##########
server/src/main/java/org/apache/seata/server/filter/RaftRequestFilter.java:
##########
@@ -16,80 +16,102 @@
*/
package org.apache.seata.server.filter;
+import io.netty.handler.codec.http.HttpRequest;
+import io.netty.handler.codec.http2.Http2Headers;
+import org.apache.seata.common.loader.LoadLevel;
import org.apache.seata.common.store.SessionMode;
import org.apache.seata.common.util.StringUtils;
-import org.apache.seata.core.exception.TransactionException;
-import org.apache.seata.core.exception.TransactionExceptionCode;
+import org.apache.seata.core.exception.HttpRequestFilterException;
+import org.apache.seata.core.rpc.netty.http.SimpleHttp2Request;
+import org.apache.seata.core.rpc.netty.http.filter.HttpFilterContext;
+import org.apache.seata.core.rpc.netty.http.filter.HttpRequestFilter;
+import org.apache.seata.core.rpc.netty.http.filter.HttpRequestFilterChain;
import org.apache.seata.server.cluster.listener.ClusterChangeEvent;
import org.apache.seata.server.cluster.raft.context.SeataClusterContext;
-import org.apache.seata.server.console.exception.ConsoleException;
import org.apache.seata.server.store.StoreConfig;
import org.springframework.context.ApplicationListener;
-import org.springframework.context.annotation.Conditional;
-import org.springframework.http.HttpMethod;
import org.springframework.stereotype.Component;
-import javax.servlet.Filter;
-import javax.servlet.FilterChain;
-import javax.servlet.FilterConfig;
-import javax.servlet.ServletException;
-import javax.servlet.ServletRequest;
-import javax.servlet.ServletResponse;
-import javax.servlet.http.HttpServletRequest;
-import java.io.IOException;
+import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.concurrent.ConcurrentHashMap;
import static org.apache.seata.common.Constants.RAFT_GROUP_HEADER;
/**
- * Raft Leader Write Filter
+ * Raft Leader Write Filter for HTTP requests
*/
+@LoadLevel(name = "RaftRequest", order = 1)
@Component
-@Conditional(RaftCondition.class)
-public class RaftRequestFilter implements Filter,
ApplicationListener<ClusterChangeEvent> {
+public class RaftRequestFilter implements HttpRequestFilter,
ApplicationListener<ClusterChangeEvent> {
private static final Map<String, Boolean> GROUP_PREVENT = new
ConcurrentHashMap<>();
@Override
- public void init(FilterConfig filterConfig) throws ServletException {}
-
- @Override
- public void doFilter(ServletRequest servletRequest, ServletResponse
servletResponse, FilterChain filterChain)
- throws IOException, ServletException {
- HttpServletRequest httpRequest = (HttpServletRequest) servletRequest;
- String group = httpRequest.getParameter("unit");
- if (StringUtils.isBlank(group)) {
- group = httpRequest.getHeader(RAFT_GROUP_HEADER);
+ public void doFilter(HttpFilterContext<?> context, HttpRequestFilterChain
chain) throws HttpRequestFilterException {
+ String uri = getUri(context);
+ if (!isTargetUri(uri)) {
+ chain.doFilter(context);
+ return;
}
+ String group = getGroup(context);
if (group != null) {
SeataClusterContext.bindGroup(group);
}
try {
- String method = httpRequest.getMethod();
- if (!HttpMethod.GET.name().equalsIgnoreCase(method)) {
+ String method;
+ if (context.getRequest() instanceof SimpleHttp2Request) {
+ SimpleHttp2Request request =
(SimpleHttp2Request)context.getRequest();
+ method = request.getMethod().name();
+ } else {
+ HttpRequest request = (HttpRequest)context.getRequest();
+ method = request.method().name();
+ }
+ if (!"GET".equalsIgnoreCase(method)) {
if (!isPass(group)) {
- throw new ConsoleException(
- new TransactionException(
- TransactionExceptionCode.NotRaftLeader,
- " The current TC is not a leader node,
interrupt processing of transactions!"),
- " The current TC is not a leader node, interrupt
processing of transactions!");
+ throw new HttpRequestFilterException(
+ "The current TC is not a leader node, interrupt
processing of transactions!");
}
}
- filterChain.doFilter(servletRequest, servletResponse);
+ chain.doFilter(context);
} finally {
SeataClusterContext.unbindGroup();
}
}
+ private String getGroup(HttpFilterContext<?> context) {
+ // Try to get from query params
+ Map<String, List<String>> params =
context.getParamWrapper().getAllParamsAsMultiMap();
+ List<String> unitParams = params.get("unit");
+ if (unitParams != null && !unitParams.isEmpty()) {
+ return unitParams.get(0);
+ }
+ // Try to get from headers
+ // Assuming HttpRequest has headers, but for SimpleHttp2Request,
headers are in request.getHeaders()
+ // For simplicity, since it's HTTP, we can cast or handle differently
+ // But to keep it simple, perhaps add a method to get header
+ // For now, assume it's HttpRequest
+ if (context.getRequest() instanceof
io.netty.handler.codec.http.HttpRequest) {
+ io.netty.handler.codec.http.HttpRequest httpRequest =
(io.netty.handler.codec.http.HttpRequest) context.getRequest();
+ return httpRequest.headers().get(RAFT_GROUP_HEADER);
+ } else if (context.getRequest() instanceof SimpleHttp2Request) {
+ Http2Headers http2Headers = ((SimpleHttp2Request)
context.getRequest()).getHeaders();
+ return http2Headers.get(RAFT_GROUP_HEADER).toString();
Review Comment:
Potential NullPointerException: Calling `.toString()` on the result of
`http2Headers.get(RAFT_GROUP_HEADER)` without null-checking can cause a NPE if
the header is not present.
Add a null check before calling `.toString()`:
```java
CharSequence headerValue = http2Headers.get(RAFT_GROUP_HEADER);
return headerValue != null ? headerValue.toString() : null;
```
```suggestion
CharSequence headerValue = http2Headers.get(RAFT_GROUP_HEADER);
return headerValue != null ? headerValue.toString() : null;
```
##########
server/src/main/java/org/apache/seata/server/filter/XSSHttpRequestFilter.java:
##########
@@ -43,7 +44,7 @@
/**
* Filter to detect and block potential XSS attack vectors in HTTP request
parameters.
*/
-@LoadLevel(name = "XSS", order = 1)
+@LoadLevel(name = "XSS", order = Integer.MIN_VALUE)
Review Comment:
Inconsistent ordering configuration: `@LoadLevel(order = Integer.MIN_VALUE)`
specifies one order value, but the `getOrder()` method at line 98 returns `1`.
The `HttpRequestFilterManager` uses `getOrder()` for sorting filters (not
the `@LoadLevel` order), so this filter will actually execute with order 1, not
Integer.MIN_VALUE as the annotation suggests. This inconsistency is confusing
and could lead to unexpected filter execution order.
Either:
1. Remove the `getOrder()` override and rely on the interface default (0), or
2. Change `getOrder()` to return `Integer.MIN_VALUE` to match the
annotation, or
3. Update the annotation to `@LoadLevel(order = 1)` to match `getOrder()`
--
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]
