Copilot commented on code in PR #7818:
URL: https://github.com/apache/incubator-seata/pull/7818#discussion_r2572895467
##########
server/src/main/java/org/apache/seata/server/filter/RaftRequestFilter.java:
##########
@@ -16,80 +16,104 @@
*/
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
Review Comment:
Outdated or unclear comments: Lines 91-94 contain stream-of-consciousness
comments ("Assuming HttpRequest has headers...", "For simplicity...") that
should be removed or rewritten as clear documentation. Consider replacing with
a concise comment like "// Retrieve header from HttpRequest or
SimpleHttp2Request".
```suggestion
// Try to get the group header from HttpRequest or SimpleHttp2Request
```
##########
server/src/main/java/org/apache/seata/server/filter/RaftRequestFilter.java:
##########
@@ -101,4 +125,27 @@ 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
Review Comment:
Outdated or unclear comments: Lines 129-132 contain stream-of-consciousness
comments ("Assuming HttpRequest has uri...", "For simplicity...") that should
be removed or rewritten as clear documentation. Consider replacing with a
concise comment like "// Extract URI from HttpRequest or SimpleHttp2Request".
```suggestion
// Extract URI from HttpRequest or path from SimpleHttp2Request
```
##########
core/src/test/java/org/apache/seata/core/rpc/netty/http/HttpDispatchHandlerTest.java:
##########
@@ -76,11 +73,22 @@ void testGetRequestWithParameters() throws Exception {
invocation.setParamMetaData(paramMetaDatas);
ControllerManager.addHttpInvocation(invocation);
+ }
+
+ @AfterEach
+ void after() throws Exception {
+ clearControllerManager();
+ Field field2 =
HttpRequestFilterManager.class.getDeclaredField("initialized");
+ field2.setAccessible(true);
+ field2.setAccessible(false);
Review Comment:
Test cleanup issue: Setting `field2.setAccessible(false)` does not reset the
`initialized` flag. The `initialized` field should be reset to `false` using
`field2.set(null, false)` to properly clean up state between tests. The current
code only changes accessibility, not the field value.
```suggestion
field2.set(null, false);
```
##########
server/src/main/java/org/apache/seata/server/filter/RaftRequestFilter.java:
##########
@@ -16,80 +16,104 @@
*/
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();
+ CharSequence headerValue = http2Headers.get(RAFT_GROUP_HEADER);
+ return headerValue != null ? headerValue.toString() : null;
+ }
+ // For HTTP/2, similar
Review Comment:
Redundant comment: The comment "// For HTTP/2, similar" at line 103 is
redundant since the HTTP/2 case is already handled in the else-if block above.
This comment should be removed.
```suggestion
```
##########
server/src/main/java/org/apache/seata/server/filter/RaftRequestFilter.java:
##########
@@ -101,4 +125,27 @@ 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:
Redundant comment: The comment "// For HTTP/2, similar" at line 140 is
redundant since the HTTP/2 case is already handled in the else-if block above.
This comment should be removed.
```suggestion
```
##########
core/src/test/java/org/apache/seata/core/rpc/netty/http/Http2HttpHandlerTest.java:
##########
@@ -373,5 +371,8 @@ void tearDown() throws Exception {
field.setAccessible(true);
Map<String, HttpInvocation> map = (Map<String, HttpInvocation>)
field.get(null);
map.clear();
+ Field field2 =
HttpRequestFilterManager.class.getDeclaredField("initialized");
+ field2.setAccessible(true);
+ field2.setAccessible(false);
Review Comment:
Test cleanup issue: Setting `field2.setAccessible(false)` does not reset the
`initialized` flag. The `initialized` field should be reset to `false` using
`field2.set(null, false)` to properly clean up state between tests. The current
code only changes accessibility, not the field value.
```suggestion
field2.set(null, false);
```
--
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]
