terrymanu commented on PR #39732:
URL: https://github.com/apache/shardingsphere/pull/39732#issuecomment-5549347504
### Result
**Review Result: Mergeable**
The minimal-scope fix has been completed. It preserves existing
functionality, compatibility, and normal-request performance. All 10 listed
`java:S1989` alerts should be resolved after the next remote SonarCloud scan.
### Evidence
- `StreamableHttpMCPServlet.java`
- Catches `IOException` and `ServletException` at the centralized
`service()` boundary.
- Resets uncommitted responses and returns an HTTP 500 response without
exposing internal details.
- Rethrows exceptions from committed responses, preserving Tomcat's
connection-termination semantics.
- Suppresses `java:S1989` only on `doGet`, `doPost`, and `doDelete`; no
global suppression is used.
- Does not modify existing status codes, error messages, the MCP protocol,
method signatures, dependencies, or Tomcat configuration.
- `StreamableHttpMCPServletTest.java`
- Covers uncommitted and committed exception paths for GET, POST, and
DELETE.
- Covers POST session rollback when an exception occurs.
- Covers DELETE session retention when an exception occurs.
- Performance: The normal request path introduces no additional object
allocation, I/O, locking, serialization, or method calls. It only adds JVM
exception table entries, so no performance benchmark is required.
- Verification:
- Focused servlet tests: 38 tests, 0 failures, exit code 0.
- `mcp/bootstrap` module tests: 302 tests, 0 failures, exit code 0.
- Exact-file Spotless check: 2 files clean, exit code 0.
- Exact-file Checkstyle check: 0 violations, exit code 0.
- References: [Jakarta ServletResponse
contract](https://jakarta.ee/specifications/servlet/6.1/apidocs/jakarta.servlet/jakarta/servlet/servletresponse),
[Tomcat 11.0.25
ErrorReportValve](https://github.com/apache/tomcat/blob/11.0.25/java/org/apache/catalina/valves/ErrorReportValve.java),
and [Sonar S1989
rule](https://github.com/SonarSource/sonar-java/blob/39579b909f7a42d0648cbbf2258f71e1a4beda76/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S1989.html).
### Coverage
- Candidate type: Standalone local change.
- Baseline: `e8d5a1558e17db9646398d21fbec1aaa3032775c`.
- The task delta contains only:
-
`mcp/bootstrap/src/main/java/org/apache/shardingsphere/mcp/bootstrap/transport/server/http/StreamableHttpMCPServlet.java`
-
`mcp/bootstrap/src/test/java/org/apache/shardingsphere/mcp/bootstrap/transport/server/http/StreamableHttpMCPServletTest.java`
- Root-cause, blast-radius, test-validity, security, session-lifecycle,
compatibility, performance, and convergence reviews have been completed.
- Three unrelated untracked Python cache directories were left untouched.
- This result covers code correctness only; remote CI and SonarCloud scans
were not reviewed.
--
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]
