Conversation
Change-Id: I49c8b1cf7a0a6f357b68b58bd352efc5981b76b2 Co-developed-by: OpenCode <noreply@opencode.ai>
Change-Id: Ieacaf949222f5871f934d2f0aa3bd6d73d1a087b
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| @PostMapping("/users") | ||
| public String createUser(@RequestBody String user) { | ||
| return "Created: " + user; |
Check warning
Code scanning / CodeQL
Cross-site scripting Medium test
Change-Id: I2df54be42732b1c1786b4b9189639913b4598fd9 Co-developed-by: OpenCode <noreply@opencode.ai>
Change-Id: Ia9dceb36c2ef3a9a86a61110446c6aec4de331a9 Co-developed-by: OpenCode <noreply@opencode.ai>
Change-Id: I10f1752fa921aa8980e640c4fe7726d3c1764f37 Co-developed-by: OpenCode <noreply@opencode.ai>
| } catch (BlockException ex) { | ||
| return handleBlockException(request, body, execution, ex); | ||
| } catch (IOException ex) { | ||
| Tracer.traceEntry(ex, hostEntry); |
There was a problem hiding this comment.
pathEntry should also record exceptions.
|
|
||
| if (response.getStatusCode().is5xxServerError()) { | ||
| RuntimeException ex = new RuntimeException("Server error: " + response.getStatusCode().value()); | ||
| Tracer.trace(ex); |
There was a problem hiding this comment.
Currently, only pathEntry records exceptions. hostEntry should also record the ex.
| public ClientHttpResponse handle(HttpRequest request, byte[] body, | ||
| ClientHttpRequestExecution execution, BlockException ex) { | ||
| return new SentinelClientHttpResponse("RestClient request blocked by Sentinel: " | ||
| + ex.getClass().getSimpleName()); |
There was a problem hiding this comment.
For consistency, the client framework should default to throwing exceptions.
| public HttpHeaders getHeaders() { | ||
| Map<String, List<String>> headers = new HashMap<>(); | ||
| headers.put(HttpHeaders.CONTENT_TYPE, | ||
| Arrays.asList(MediaType.APPLICATION_JSON_VALUE)); |
There was a problem hiding this comment.
If the body isn't JSON, wouldn't it be more appropriate to use text/plain?
There was a problem hiding this comment.
Pull request overview
Adds a new Sentinel adapter module to integrate with Spring Framework 6+ RestClient, providing resource extraction, flow-control/circuit-breaking interception, and fallback handling.
Changes:
- Introduces
SentinelRestClientInterceptorwith configurable resource naming and fallback behavior. - Adds default implementations for resource extraction and blocked-request fallback response.
- Adds a new Maven module with documentation and a set of unit/integration-style tests.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| sentinel-adapter/pom.xml | Registers the new sentinel-spring-restclient-adapter module in the parent build. |
| sentinel-adapter/sentinel-spring-restclient-adapter/pom.xml | Adds module dependencies (Spring Web/Boot/Test) and Surefire configuration. |
| sentinel-adapter/sentinel-spring-restclient-adapter/README.md | Documents adapter usage, configuration, and resource naming. |
| sentinel-adapter/sentinel-spring-restclient-adapter/src/main/java/com/alibaba/csp/sentinel/adapter/spring/restclient/SentinelRestClientInterceptor.java | Implements the RestClient request interceptor with dual-level resource entries and fallback handling. |
| sentinel-adapter/sentinel-spring-restclient-adapter/src/main/java/com/alibaba/csp/sentinel/adapter/spring/restclient/SentinelRestClientConfig.java | Provides configuration (prefix, extractor, fallback) for the interceptor. |
| sentinel-adapter/sentinel-spring-restclient-adapter/src/main/java/com/alibaba/csp/sentinel/adapter/spring/restclient/SentinelClientHttpResponse.java | Defines the default blocked-response ClientHttpResponse implementation. |
| sentinel-adapter/sentinel-spring-restclient-adapter/src/main/java/com/alibaba/csp/sentinel/adapter/spring/restclient/extractor/RestClientResourceExtractor.java | Introduces extractor SPI for resource naming. |
| sentinel-adapter/sentinel-spring-restclient-adapter/src/main/java/com/alibaba/csp/sentinel/adapter/spring/restclient/extractor/DefaultRestClientResourceExtractor.java | Implements default resource naming from method + URL components. |
| sentinel-adapter/sentinel-spring-restclient-adapter/src/main/java/com/alibaba/csp/sentinel/adapter/spring/restclient/fallback/RestClientFallback.java | Introduces fallback SPI for blocked requests. |
| sentinel-adapter/sentinel-spring-restclient-adapter/src/main/java/com/alibaba/csp/sentinel/adapter/spring/restclient/fallback/DefaultRestClientFallback.java | Provides default blocked-request fallback response. |
| sentinel-adapter/sentinel-spring-restclient-adapter/src/test/java/com/alibaba/csp/sentinel/adapter/spring/restclient/SentinelRestClientInterceptorSimpleTest.java | Adds integration-style tests around flow control/degrade with an embedded Spring Boot app. |
| sentinel-adapter/sentinel-spring-restclient-adapter/src/test/java/com/alibaba/csp/sentinel/adapter/spring/restclient/ManualTest.java | Adds a manual runnable test/demo for the adapter. |
| sentinel-adapter/sentinel-spring-restclient-adapter/src/test/java/com/alibaba/csp/sentinel/adapter/spring/restclient/SentinelRestClientConfigTest.java | Adds config validation tests (null extractor/fallback). |
| sentinel-adapter/sentinel-spring-restclient-adapter/src/test/java/com/alibaba/csp/sentinel/adapter/spring/restclient/SentinelClientHttpResponseTest.java | Adds tests for blocked-response behavior. |
| sentinel-adapter/sentinel-spring-restclient-adapter/src/test/java/com/alibaba/csp/sentinel/adapter/spring/restclient/extractor/DefaultRestClientResourceExtractorTest.java | Adds tests for default resource name formatting. |
| sentinel-adapter/sentinel-spring-restclient-adapter/src/test/java/com/alibaba/csp/sentinel/adapter/spring/restclient/fallback/DefaultRestClientFallbackTest.java | Adds tests for default fallback handling. |
| sentinel-adapter/sentinel-spring-restclient-adapter/src/test/java/com/alibaba/csp/sentinel/adapter/spring/restclient/app/TestApplication.java | Provides Spring Boot test application used by integration tests. |
| sentinel-adapter/sentinel-spring-restclient-adapter/src/test/java/com/alibaba/csp/sentinel/adapter/spring/restclient/app/TestController.java | Provides endpoints used by integration tests (hello/users/error/delay). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| */ | ||
| package com.alibaba.csp.sentinel.adapter.spring.restclient.fallback; | ||
|
|
||
| import com.alibaba.csp.sentinel.adapter.spring.restclient.SentinelClientHttpResponse; |
There was a problem hiding this comment.
Unused import: SentinelClientHttpResponse is not referenced in this interface. Please remove it to avoid checkstyle/compilation warnings in stricter builds.
| import com.alibaba.csp.sentinel.adapter.spring.restclient.SentinelClientHttpResponse; |
| import java.io.IOException; | ||
| import java.net.URI; | ||
|
|
||
| import static org.junit.Assert.assertEquals; |
There was a problem hiding this comment.
Unused static import: assertEquals is not used in this test. Removing it keeps the test clean and avoids warnings.
| import static org.junit.Assert.assertEquals; |
| Tracer.trace(ex); | ||
| } | ||
|
|
||
| return response; | ||
| } catch (BlockException ex) { | ||
| return handleBlockException(request, body, execution, ex); | ||
| } catch (IOException ex) { | ||
| Tracer.traceEntry(ex, hostEntry); |
There was a problem hiding this comment.
Exception tracing is currently attributed only to hostEntry for IOException, and 5xx tracing uses Tracer.trace(ex) without binding to the active entry. This can lead to inaccurate Sentinel exception metrics at the path-level (and possibly no attribution depending on context). Consider tracing against pathEntry when it exists (otherwise hostEntry), and similarly trace 5xx against the active entry (e.g., via Tracer.traceEntry(...)).
| Tracer.trace(ex); | |
| } | |
| return response; | |
| } catch (BlockException ex) { | |
| return handleBlockException(request, body, execution, ex); | |
| } catch (IOException ex) { | |
| Tracer.traceEntry(ex, hostEntry); | |
| Entry currentEntry = (pathEntry != null) ? pathEntry : hostEntry; | |
| if (currentEntry != null) { | |
| Tracer.traceEntry(ex, currentEntry); | |
| } else { | |
| Tracer.trace(ex); | |
| } | |
| } | |
| return response; | |
| } catch (BlockException ex) { | |
| return handleBlockException(request, body, execution, ex); | |
| } catch (IOException ex) { | |
| Entry currentEntry = (pathEntry != null) ? pathEntry : hostEntry; | |
| if (currentEntry != null) { | |
| Tracer.traceEntry(ex, currentEntry); | |
| } else { | |
| Tracer.trace(ex); | |
| } |
| public InputStream getBody() throws IOException { | ||
| return new ByteArrayInputStream(blockResponse.getBytes()); | ||
| } | ||
|
|
||
| @Override | ||
| public HttpHeaders getHeaders() { | ||
| Map<String, List<String>> headers = new HashMap<>(); | ||
| headers.put(HttpHeaders.CONTENT_TYPE, | ||
| Arrays.asList(MediaType.APPLICATION_JSON_VALUE)); | ||
| HttpHeaders httpHeaders = new HttpHeaders(); | ||
| httpHeaders.putAll(headers); | ||
| return httpHeaders; | ||
| } |
There was a problem hiding this comment.
The response body is plain text but the Content-Type is set to application/json, and blockResponse.getBytes() uses the platform default charset. Please align the content type with the actual payload (e.g., text/plain; charset=UTF-8), and encode bytes with a fixed charset (e.g., UTF-8) to avoid inconsistent behavior across environments.
| return HttpStatus.OK; | ||
| } | ||
|
|
||
| @Override | ||
| public int getRawStatusCode() throws IOException { | ||
| return HttpStatus.OK.value(); |
There was a problem hiding this comment.
Returning 200 OK for a request that was blocked by flow control/circuit breaking can mask failures and confuse client-side error handling. Consider using a more appropriate status code (commonly 429 TOO_MANY_REQUESTS for flow control, or 503 SERVICE_UNAVAILABLE for degrade) to make the blocked state explicit to API consumers.
| return HttpStatus.OK; | |
| } | |
| @Override | |
| public int getRawStatusCode() throws IOException { | |
| return HttpStatus.OK.value(); | |
| return HttpStatus.TOO_MANY_REQUESTS; | |
| } | |
| @Override | |
| public int getRawStatusCode() throws IOException { | |
| return HttpStatus.TOO_MANY_REQUESTS.value(); |
| import java.util.concurrent.CountDownLatch; | ||
| import java.util.concurrent.TimeUnit; | ||
| import java.util.concurrent.atomic.AtomicBoolean; | ||
| import java.util.concurrent.atomic.AtomicReference; |
There was a problem hiding this comment.
These imports (CountDownLatch, TimeUnit, AtomicBoolean, AtomicReference) are not used in the test class as shown. Please remove unused imports to avoid warnings and keep the test file focused.
| import java.util.concurrent.CountDownLatch; | |
| import java.util.concurrent.TimeUnit; | |
| import java.util.concurrent.atomic.AtomicBoolean; | |
| import java.util.concurrent.atomic.AtomicReference; |
| @SpringBootTest(classes = TestApplication.class, | ||
| webEnvironment = SpringBootTest.WebEnvironment.DEFINED_PORT, | ||
| properties = { | ||
| "server.port=8087" | ||
| }) |
There was a problem hiding this comment.
Using DEFINED_PORT + hard-coded 8087 can make CI flaky due to port conflicts (parallel test runs, local dev environment, etc.). Prefer RANDOM_PORT with injected port (e.g., @LocalServerPort) so the test suite is more reliable.
| public static void main(String[] args) { | ||
| System.out.println("=== Sentinel RestClient Adapter Manual Test ===\n"); | ||
|
|
||
| testBasicUsage(); | ||
| testWithFlowControl(); | ||
| testWithCustomExtractor(); | ||
|
|
||
| System.out.println("\n=== All manual tests completed! ==="); | ||
| } |
There was a problem hiding this comment.
This is a runnable demo/manual test living under src/test/java. While it won’t be picked up by Surefire as a JUnit test, it still becomes part of the test artifact and may confuse automated test expectations. Consider moving it to documentation/examples (or clearly marking it under a dedicated .../manual/... package and documenting that it performs real network calls).
| ## Usage | ||
|
|
||
| ### 1. Add Dependency | ||
|
|
There was a problem hiding this comment.
The Maven dependency snippet is missing the closing </dependency> tag indentation/structure is fine, but the opening tag block should be properly closed to be copy-pastable.
| <properties> | ||
| <spring-web.version>6.1.0</spring-web.version> | ||
| <spring-boot.version>3.2.0</spring-boot.version> | ||
| <spring-test.version>6.1.0</spring-test.version> | ||
|
|
||
| <skip.spring.v6x.test>false</skip.spring.v6x.test> | ||
| </properties> |
There was a problem hiding this comment.
Hard-coding Spring/Spring Boot versions in this module POM can cause version skew against the repository’s parent dependency management (and across other modules). If the repo already manages Spring versions via a BOM/parent, prefer relying on that (or using a shared property from the parent) to keep dependency alignment consistent.
Describe what this PR does / why we need it
Closes #3601
Does this pull request fix one issue?
Closes #3601
Describe how you did it
add Sentinel RestClient Interceptor
Describe how to verify it
Run com.alibaba.csp.sentinel.adapter.spring.restclient.SentinelRestClientInterceptorSimpleTest to confirm the flow control behavior of Sentinel.
Special notes for reviews
None