Copilot commented on code in PR #7870:
URL: https://github.com/apache/incubator-seata/pull/7870#discussion_r2630202117
##########
namingserver/src/main/java/org/apache/seata/namingserver/filter/ConsoleRemotingFilter.java:
##########
@@ -113,46 +109,34 @@ public void doFilter(ServletRequest servletRequest,
ServletResponse servletRespo
// Create the HttpEntity with headers and body
HttpEntity<byte[]> httpEntity = new
HttpEntity<>(request.getCachedBody(), headers);
-
- // Forward the request
- AsyncContext asyncContext =
servletRequest.startAsync();
- asyncContext.setTimeout(5000L);
- ListenableFuture<ResponseEntity<byte[]>>
responseEntityFuture = asyncRestTemplate.exchange(
- URI.create(targetUrl),
-
Objects.requireNonNull(HttpMethod.resolve(request.getMethod())),
- httpEntity,
- byte[].class);
- responseEntityFuture.addCallback(new
ListenableFutureCallback<ResponseEntity<byte[]>>() {
- @Override
- public void onFailure(Throwable ex) {
- try {
- logger.error(ex.getMessage(), ex);
-
response.setStatus(HttpServletResponse.SC_INTERNAL_SERVER_ERROR);
- } finally {
- asyncContext.complete();
+ HttpMethod httpMethod;
+ try {
+ httpMethod =
HttpMethod.valueOf(request.getMethod());
+ } catch (IllegalArgumentException 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.getHeaders().forEach((key,
value) -> {
+ value.forEach(v -> response.addHeader(key,
v));
+ });
+ response.setStatus(
+
responseEntity.getStatusCode().value());
+
Optional.ofNullable(responseEntity.getBody()).ifPresent(body -> {
+ try (ServletOutputStream outputStream =
response.getOutputStream()) {
+ outputStream.write(body);
+ outputStream.flush();
+ } catch (IOException e) {
+ logger.error(e.getMessage(), e);
}
- }
-
- @Override
- public void onSuccess(ResponseEntity<byte[]>
responseEntity) {
- // Copy response headers and status code
- responseEntity.getHeaders().forEach((key,
value) -> {
- value.forEach(v ->
response.addHeader(key, v));
- });
-
response.setStatus(responseEntity.getStatusCodeValue());
- // Write response body
-
Optional.ofNullable(responseEntity.getBody())
- .ifPresent(body -> {
- try (ServletOutputStream
outputStream = response.getOutputStream()) {
- outputStream.write(body);
- outputStream.flush();
- } catch (IOException e) {
-
logger.error(e.getMessage(), e);
- }
- });
- asyncContext.complete();
- }
- });
+ });
+ } catch (Exception ex) {
+ logger.error(ex.getMessage(), ex);
+
response.sendError(HttpServletResponse.SC_INTERNAL_SERVER_ERROR);
+ }
Review Comment:
The significant refactoring of ConsoleRemotingFilter from AsyncRestTemplate
to RestTemplate changes the core request forwarding mechanism. This change
lacks test coverage to verify that the synchronous RestTemplate with virtual
threads provides equivalent functionality to the previous asynchronous
implementation. Consider adding tests to verify proper request forwarding,
error handling, and timeout behavior.
##########
console/pom.xml:
##########
@@ -121,7 +108,7 @@
<exclusions>
<exclusion>
<groupId>org.apache.tomcat.embed</groupId>
- <artifactId>tomcat-embed-core</artifactId>
+ <artifactId>*</artifactId>
</exclusion>
<exclusion>
Review Comment:
The wildcard exclusion pattern for tomcat-embed artifacts may be too broad.
While excluding all tomcat-embed artifacts (using *) ensures no conflicts,
verify that this doesn't accidentally exclude required embedded Tomcat
components that might be needed by spring-boot-starter-web. Consider being more
specific about which artifacts to exclude if issues arise.
```suggestion
<artifactId>tomcat-embed-core</artifactId>
</exclusion>
<exclusion>
<groupId>org.apache.tomcat.embed</groupId>
<artifactId>tomcat-embed-el</artifactId>
</exclusion>
<exclusion>
<groupId>org.apache.tomcat.embed</groupId>
<artifactId>tomcat-embed-websocket</artifactId>
</exclusion>
<exclusion>
```
##########
namingserver/src/main/java/org/apache/seata/namingserver/config/WebConfig.java:
##########
@@ -16,22 +16,19 @@
*/
package org.apache.seata.namingserver.config;
-import org.apache.http.impl.client.CloseableHttpClient;
-import org.apache.http.impl.client.HttpClients;
-import org.apache.http.impl.conn.PoolingHttpClientConnectionManager;
+import jakarta.servlet.Filter;
+import org.apache.hc.client5.http.impl.classic.CloseableHttpClient;
+import org.apache.hc.client5.http.impl.classic.HttpClients;
+import org.apache.hc.client5.http.impl.io.PoolingHttpClientConnectionManager;
Review Comment:
The migration from org.apache.httpcomponents:httpclient (HttpClient 4.x) to
org.apache.httpcomponents.client5:httpclient5 (HttpClient 5.x) involves
significant API changes. The imports have been correctly updated to use
org.apache.hc.client5 packages. However, ensure that the
HttpComponentsClientHttpRequestFactory from Spring is compatible with
HttpClient 5.x at runtime, as Spring's HttpComponentsClientHttpRequestFactory
may have different configuration methods for HttpClient 5.
##########
namingserver/src/main/java/org/apache/seata/namingserver/config/WebConfig.java:
##########
@@ -56,21 +53,10 @@ public RestTemplate restTemplate() {
return new RestTemplate(requestFactory);
}
- @Bean
- public AsyncRestTemplate asyncRestTemplate(RestTemplate restTemplate) {
- HttpComponentsAsyncClientHttpRequestFactory
asyncClientHttpRequestFactory =
- new HttpComponentsAsyncClientHttpRequestFactory();
- asyncClientHttpRequestFactory.setConnectionRequestTimeout(
- DEFAULT_REQUEST_TIMEOUT); // Connection request timeout in
milliseconds
-
asyncClientHttpRequestFactory.setConnectTimeout(DEFAULT_REQUEST_TIMEOUT); //
Connection timeout in milliseconds
- asyncClientHttpRequestFactory.setReadTimeout(DEFAULT_REQUEST_TIMEOUT);
// Read timeout in milliseconds
- return new AsyncRestTemplate(asyncClientHttpRequestFactory,
restTemplate);
- }
-
@Bean
public FilterRegistrationBean<Filter> consoleRemotingFilter(
- NamingManager namingManager, AsyncRestTemplate asyncRestTemplate) {
- ConsoleRemotingFilter consoleRemotingFilter = new
ConsoleRemotingFilter(namingManager, asyncRestTemplate);
+ NamingManager namingManager, RestTemplate restTemplate) {
+ ConsoleRemotingFilter consoleRemotingFilter = new
ConsoleRemotingFilter(namingManager, restTemplate);
FilterRegistrationBean<Filter> registration = new
FilterRegistrationBean<>();
registration.setFilter(consoleRemotingFilter);
registration.addUrlPatterns("/*");
Review Comment:
The removal of AsyncRestTemplate and the corresponding asyncRestTemplate
bean removes the original asynchronous request handling capability. While the
PR description mentions using virtual threads for lightweight blocking, the
synchronous RestTemplate call in ConsoleRemotingFilter will now block the
request thread even with virtual threads enabled. Consider if this blocking
behavior is acceptable for the use case, as it changes from a truly
asynchronous callback model to a blocking model (even if on virtual threads).
```suggestion
registration.addUrlPatterns("/console/*");
```
##########
namingserver/src/main/java/org/apache/seata/namingserver/filter/ConsoleRemotingFilter.java:
##########
@@ -113,46 +109,34 @@ public void doFilter(ServletRequest servletRequest,
ServletResponse servletRespo
// Create the HttpEntity with headers and body
HttpEntity<byte[]> httpEntity = new
HttpEntity<>(request.getCachedBody(), headers);
-
- // Forward the request
- AsyncContext asyncContext =
servletRequest.startAsync();
- asyncContext.setTimeout(5000L);
- ListenableFuture<ResponseEntity<byte[]>>
responseEntityFuture = asyncRestTemplate.exchange(
- URI.create(targetUrl),
-
Objects.requireNonNull(HttpMethod.resolve(request.getMethod())),
- httpEntity,
- byte[].class);
- responseEntityFuture.addCallback(new
ListenableFutureCallback<ResponseEntity<byte[]>>() {
- @Override
- public void onFailure(Throwable ex) {
- try {
- logger.error(ex.getMessage(), ex);
-
response.setStatus(HttpServletResponse.SC_INTERNAL_SERVER_ERROR);
- } finally {
- asyncContext.complete();
+ HttpMethod httpMethod;
+ try {
+ httpMethod =
HttpMethod.valueOf(request.getMethod());
+ } catch (IllegalArgumentException 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.getHeaders().forEach((key,
value) -> {
+ value.forEach(v -> response.addHeader(key,
v));
+ });
+ response.setStatus(
+
responseEntity.getStatusCode().value());
+
Optional.ofNullable(responseEntity.getBody()).ifPresent(body -> {
+ try (ServletOutputStream outputStream =
response.getOutputStream()) {
+ outputStream.write(body);
+ outputStream.flush();
+ } catch (IOException e) {
+ logger.error(e.getMessage(), e);
}
- }
-
- @Override
- public void onSuccess(ResponseEntity<byte[]>
responseEntity) {
- // Copy response headers and status code
- responseEntity.getHeaders().forEach((key,
value) -> {
- value.forEach(v ->
response.addHeader(key, v));
- });
-
response.setStatus(responseEntity.getStatusCodeValue());
- // Write response body
-
Optional.ofNullable(responseEntity.getBody())
- .ifPresent(body -> {
- try (ServletOutputStream
outputStream = response.getOutputStream()) {
- outputStream.write(body);
- outputStream.flush();
- } catch (IOException e) {
-
logger.error(e.getMessage(), e);
- }
- });
- asyncContext.complete();
- }
- });
+ });
+ } catch (Exception ex) {
+ logger.error(ex.getMessage(), ex);
+
response.sendError(HttpServletResponse.SC_INTERNAL_SERVER_ERROR);
+ }
Review Comment:
The migration from AsyncRestTemplate with callback-based asynchronous
processing to synchronous RestTemplate removes the async context management
(startAsync/complete). The original code used asyncContext.setTimeout(5000L)
and properly completed the async context. With the new synchronous approach,
the servlet filter chain will be blocked during the entire HTTP
request/response cycle. While virtual threads help mitigate this, the behavior
change from non-blocking asynchronous to blocking (even on virtual threads) is
significant and should be carefully validated in production scenarios with high
concurrency.
##########
namingserver/src/main/resources/application.yml:
##########
@@ -18,6 +18,12 @@
server:
port: 8081
spring:
+ threads:
+ virtual:
+ enabled: true
+ mvc:
+ pathmatch:
Review Comment:
The PathMatchingStrategy configuration using 'ant_path_matcher' forces
Spring MVC to use the legacy AntPathMatcher instead of the newer
PathPatternParser (default in Spring Boot 3.x). While this maintains backward
compatibility with patterns like /**/*.css, be aware that PathPatternParser is
more efficient. Consider migrating path patterns to be compatible with
PathPatternParser in the future for better performance, or document why the
legacy matcher is required.
```suggestion
pathmatch:
# Use legacy AntPathMatcher for compatibility with existing patterns
like /**/*.css, /**/*.js, etc.
# Switching to PathPatternParser (Spring Boot 3.x default) would
require updating these patterns.
```
##########
console/src/main/java/org/apache/seata/console/config/WebSecurityConfig.java:
##########
@@ -79,60 +84,60 @@ public class WebSecurityConfig extends
WebSecurityConfigurerAdapter {
@Autowired
private Environment env;
- @Bean(name = BeanIds.AUTHENTICATION_MANAGER)
- @Override
- public AuthenticationManager authenticationManagerBean() throws Exception {
- return super.authenticationManagerBean();
+ @Bean
+ public PasswordEncoder passwordEncoder() {
+ return new BCryptPasswordEncoder();
}
- @Override
- protected void configure(AuthenticationManagerBuilder auth) throws
Exception {
-
auth.userDetailsService(userDetailsService).passwordEncoder(passwordEncoder());
+ @Bean
+ public AuthenticationManager authenticationManager() {
+ DaoAuthenticationProvider authenticationProvider = new
DaoAuthenticationProvider(userDetailsService);
+ authenticationProvider.setPasswordEncoder(passwordEncoder());
+ return new ProviderManager(authenticationProvider);
}
- @Override
- public void configure(WebSecurity web) {
- String ignoreURLs = env.getProperty("seata.security.ignore.urls",
"/**");
- for (String ignoreURL :
ignoreURLs.trim().split(SECURITY_IGNORE_URLS_SPILT_CHAR)) {
- web.ignoring().antMatchers(ignoreURL.trim());
- }
+ @Bean
+ public WebSecurityCustomizer webSecurityCustomizer() {
+ RequestMatcher[] ignoredMatchers =
buildAntMatchers(env.getProperty("seata.security.ignore.urls", "/**"));
+ return web -> {
+ if (ignoredMatchers.length > 0) {
+ web.ignoring().requestMatchers(ignoredMatchers);
+ }
+ };
}
- @Override
- protected void configure(HttpSecurity http) throws Exception {
- String csrfIgnoreUrls =
env.getProperty("seata.security.csrf-ignore-urls");
- CsrfConfigurer<HttpSecurity> csrf = http.authorizeRequests()
- .anyRequest()
- .authenticated()
- .and()
- // custom token authorize exception handler
- .exceptionHandling()
- .authenticationEntryPoint(unauthorizedHandler)
- .and()
- // since we use jwt, session is not necessary
- .sessionManagement()
- .sessionCreationPolicy(SessionCreationPolicy.STATELESS)
- .disable()
- .csrf();
- if (StringUtils.isNotBlank(csrfIgnoreUrls)) {
-
csrf.ignoringAntMatchers(csrfIgnoreUrls.trim().split(SECURITY_IGNORE_URLS_SPILT_CHAR));
- }
-
csrf.csrfTokenRepository(CookieCsrfTokenRepository.withHttpOnlyFalse());
- // don't disable csrf, jwt may be implemented based on cookies
- http.addFilterBefore(
- new JwtAuthenticationTokenFilter(tokenProvider),
UsernamePasswordAuthenticationFilter.class);
-
- // disable cache
- http.headers().cacheControl();
+ @Bean
+ public SecurityFilterChain securityFilterChain(HttpSecurity http,
AuthenticationManager authenticationManager)
+ throws Exception {
+ RequestMatcher[] csrfIgnored =
buildAntMatchers(env.getProperty("seata.security.csrf-ignore-urls"));
+
+ http.authenticationManager(authenticationManager)
+ .authorizeHttpRequests(authz ->
authz.anyRequest().authenticated())
+ .exceptionHandling(exception ->
exception.authenticationEntryPoint(unauthorizedHandler))
+ .sessionManagement(session ->
session.sessionCreationPolicy(SessionCreationPolicy.STATELESS))
+ .csrf(csrf -> {
+ if (csrfIgnored.length > 0) {
+ csrf.ignoringRequestMatchers(csrfIgnored);
+ }
+
csrf.csrfTokenRepository(CookieCsrfTokenRepository.withHttpOnlyFalse());
+ })
+ .addFilterBefore(
+ new JwtAuthenticationTokenFilter(tokenProvider),
UsernamePasswordAuthenticationFilter.class)
+ .headers(headers -> headers.cacheControl(cache -> {}));
+
+ return http.build();
}
- /**
- * Password encoder password encoder.
- *
- * @return the password encoder
- */
- @Bean
- public PasswordEncoder passwordEncoder() {
- return new BCryptPasswordEncoder();
+ private RequestMatcher[] buildAntMatchers(String patterns) {
+ if (StringUtils.isBlank(patterns)) {
+ return new RequestMatcher[0];
+ }
+ return
Arrays.stream(patterns.trim().split(SECURITY_IGNORE_URLS_SPILT_CHAR))
+ .map(String::trim)
+ .filter(StringUtils::isNotBlank)
+ // PathPatternParser using the new version of Security cannot
directly achieve the same matching effect
+ // as the deprecated Ant style mode /**/*.css
Review Comment:
The comment explaining why AntPathRequestMatcher is used is helpful.
However, the explanation could be clearer. The issue is that Spring Security
6.x uses PathPatternParser by default, which has stricter matching rules than
the legacy Ant style matcher. Patterns like /**/*.css work differently between
the two. Consider adding a reference or example to make this clearer for future
maintainers.
```suggestion
// In Spring Security 6.x, requestMatchers(String...) uses
PathPatternParser by default, which has
// stricter matching rules than the legacy Ant-style
matcher. For example, the Ant pattern "/**/*.css"
// (often used for static resources) is not interpreted the
same way by PathPatternParser and may no
// longer match all nested paths as expected. We therefore
use AntPathRequestMatcher explicitly so
// that ignore / CSRF patterns coming from configuration
keep their original Ant-style semantics.
// See: Spring Security Reference - "Request Matchers"
section.
```
--
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]
