Copilot commented on code in PR #6312:
URL: https://github.com/apache/shenyu/pull/6312#discussion_r3013631849
##########
shenyu-plugin/shenyu-plugin-mcp-server/pom.xml:
##########
@@ -55,43 +51,47 @@
<artifactId>shenyu-loadbalancer</artifactId>
<version>${project.version}</version>
</dependency>
+
+ <!--
+ Spring AI and MCP SDK Dependencies
+
+ SDK Compatibility Notes:
+ - Current tested version: MCP SDK 0.17.0, Spring AI 1.1.2
+ - McpSessionHelper uses reflection to access
McpSyncServerExchange.exchange and
+ McpAsyncServerExchange.session fields for session ID extraction
+ - ShenyuStreamableHttpServerTransportProvider uses reflection to
access
+ McpServerSession.initialized and McpServerSession.state fields
for session verification
+ - When upgrading SDK versions, verify that these internal fields
still exist and
+ update McpSessionHelper.SUPPORTED_SDK_VERSION and
+
ShenyuStreamableHttpServerTransportProvider.SUPPORTED_SDK_VERSION accordingly
Review Comment:
The comment says to update
`ShenyuStreamableHttpServerTransportProvider.SUPPORTED_SDK_VERSION`, but that
constant doesn’t exist in `ShenyuStreamableHttpServerTransportProvider` (the
class currently has no `SUPPORTED_SDK_VERSION`). Either add the constant (if
intended) or update the comment to reference the actual version source to avoid
misleading future upgrades.
```suggestion
any related version constants in
ShenyuStreamableHttpServerTransportProvider accordingly
```
##########
shenyu-plugin/shenyu-plugin-mcp-server/src/main/java/org/apache/shenyu/plugin/mcp/server/callback/ShenyuToolCallback.java:
##########
@@ -761,20 +761,24 @@ private McpSyncServerExchange extractMcpExchange(final
ToolContext toolContext)
*
* @param mcpExchange the MCP sync server exchange
* @return the session ID
- * @throws IllegalStateException if session ID cannot be extracted
+ * @throws IllegalStateException if session ID cannot be extracted (SDK
compatibility issue)
*/
private String extractSessionId(final McpSyncServerExchange mcpExchange) {
- final String sessionId;
try {
- sessionId = McpSessionHelper.getSessionId(mcpExchange);
- } catch (NoSuchFieldException | IllegalAccessException e) {
- throw new RuntimeException(e);
- }
- if (StringUtils.hasText(sessionId)) {
- LOG.debug("Extracted session ID: {}", sessionId);
- return sessionId;
+ final String sessionId =
McpSessionHelper.getSessionId(mcpExchange);
+ if (StringUtils.hasText(sessionId)) {
+ LOG.debug("Extracted session ID: {}", sessionId);
+ return sessionId;
+ }
+ throw new IllegalStateException("Session ID is empty – it should
have been set earlier by handleMessageEndpoint");
+ } catch (IllegalStateException e) {
+ // Re-throw SDK compatibility errors with additional context
+ throw new IllegalStateException(
+ "Failed to extract session ID from MCP exchange. "
+ + "This may indicate an SDK compatibility issue. "
+ + "Tested SDK version: " +
McpSessionHelper.getSupportedSdkVersion() + ". "
+ + "Original error: " + e.getMessage(), e);
}
Review Comment:
There are existing unit tests for `ShenyuToolCallback`, but none cover the
new error-wrapping behavior that adds SDK version context. Consider adding a
test where `McpSessionHelper.getSessionId()` throws an `IllegalStateException`
(reflection failure) and asserting the propagated exception message includes
the supported SDK version, and another where sessionId is empty to ensure it is
not misclassified as a compatibility issue.
##########
shenyu-plugin/shenyu-plugin-mcp-server/src/main/java/org/apache/shenyu/plugin/mcp/server/session/McpSessionHelper.java:
##########
@@ -28,9 +30,86 @@
/**
* Helper class for handling McpSession related operations.
+ *
+ * <p>SDK Compatibility Note: This class uses reflection to access internal
SDK fields
+ * that may change between versions. The following SDK versions are tested and
supported:
+ * <ul>
+ * <li>MCP SDK 0.17.0 (current)</li>
+ * <li>Spring AI 1.1.2 (current)</li>
+ * </ul>
+ *
+ * <p>If reflection fails (e.g., due to SDK API changes), this class will throw
+ * IllegalStateException with clear error messages indicating SDK
compatibility issues.
+ *
+ * @since 2.7.0.2
*/
public class McpSessionHelper {
-
+
+ private static final Logger LOG =
LoggerFactory.getLogger(McpSessionHelper.class);
+
+ /**
+ * SDK version for compatibility checking.
+ * This should be updated when testing with new SDK versions.
+ */
+ private static final String SUPPORTED_SDK_VERSION = "0.17.0";
+
+ /**
+ * Cached field reference for McpSyncServerExchange.exchange field.
+ * Null if not yet resolved or resolution failed.
+ */
+ private static volatile Field asyncExchangeFieldCache;
+
+ /**
+ * Cached field reference for McpAsyncServerExchange.session field.
+ * Null if not yet resolved or resolution failed.
+ */
+ private static volatile Field sessionFieldCache;
+
+ /**
+ * Flag indicating whether reflection fields have been resolved.
+ */
+ private static volatile boolean fieldsResolved;
+
+ /**
+ * Lock for resolving reflection fields.
+ */
+ private static final Object FIELD_RESOLVE_LOCK = new Object();
+
+ static {
+ resolveReflectionFields();
+ }
+
+ /**
+ * Resolves reflection fields for accessing internal SDK state.
+ * This method is called during class initialization and logs any
compatibility issues.
+ */
+ private static void resolveReflectionFields() {
+ try {
+ // Resolve asyncExchange field from McpSyncServerExchange
+ asyncExchangeFieldCache =
McpSyncServerExchange.class.getDeclaredField("exchange");
+ asyncExchangeFieldCache.setAccessible(true);
+ LOG.info("Successfully resolved McpSyncServerExchange.exchange
field for SDK compatibility");
+
+ // Resolve session field from McpAsyncServerExchange
+ sessionFieldCache =
McpAsyncServerExchange.class.getDeclaredField("session");
+ sessionFieldCache.setAccessible(true);
+ LOG.info("Successfully resolved McpAsyncServerExchange.session
field for SDK compatibility");
+
+ fieldsResolved = true;
+ LOG.info("MCP SDK reflection fields resolved successfully. Tested
with SDK version: {}", SUPPORTED_SDK_VERSION);
+ } catch (NoSuchFieldException e) {
+ LOG.error("SDK COMPATIBILITY ERROR: Failed to resolve reflection
fields. "
+ + "This indicates the MCP SDK API has changed. "
+ + "Tested version: {}, Current SDK may be incompatible. "
+ + "Missing field: {}", SUPPORTED_SDK_VERSION,
e.getMessage());
+ fieldsResolved = false;
+ } catch (SecurityException e) {
+ LOG.error("SDK COMPATIBILITY ERROR: Security manager blocked
reflection access. "
+ + "Field resolution failed: {}", e.getMessage());
+ fieldsResolved = false;
Review Comment:
`resolveReflectionFields()` runs during class initialization and calls
`Field#setAccessible(true)`. On Java 9+ this can throw unchecked runtime
exceptions (e.g., `InaccessibleObjectException`) in addition to
`SecurityException`; currently those will escape the static initializer and can
fail class loading with `ExceptionInInitializerError`. Consider catching
`RuntimeException` around the reflective access, logging, and leaving
`fieldsResolved=false` so the helper fails gracefully via
`checkReflectionAvailability()` instead of crashing at class-load time.
```suggestion
fieldsResolved = false;
} catch (RuntimeException e) {
LOG.error("SDK COMPATIBILITY ERROR: Unexpected runtime exception
during reflection field resolution. "
+ "This may indicate module access restrictions or an
incompatible SDK version (tested: {}). "
+ "Error: {}", SUPPORTED_SDK_VERSION, e.getMessage(), e);
fieldsResolved = false;
```
##########
shenyu-plugin/shenyu-plugin-mcp-server/src/main/java/org/apache/shenyu/plugin/mcp/server/callback/ShenyuToolCallback.java:
##########
@@ -761,20 +761,24 @@ private McpSyncServerExchange extractMcpExchange(final
ToolContext toolContext)
*
* @param mcpExchange the MCP sync server exchange
* @return the session ID
- * @throws IllegalStateException if session ID cannot be extracted
+ * @throws IllegalStateException if session ID cannot be extracted (SDK
compatibility issue)
*/
private String extractSessionId(final McpSyncServerExchange mcpExchange) {
- final String sessionId;
try {
- sessionId = McpSessionHelper.getSessionId(mcpExchange);
- } catch (NoSuchFieldException | IllegalAccessException e) {
- throw new RuntimeException(e);
- }
- if (StringUtils.hasText(sessionId)) {
- LOG.debug("Extracted session ID: {}", sessionId);
- return sessionId;
+ final String sessionId =
McpSessionHelper.getSessionId(mcpExchange);
+ if (StringUtils.hasText(sessionId)) {
+ LOG.debug("Extracted session ID: {}", sessionId);
+ return sessionId;
+ }
+ throw new IllegalStateException("Session ID is empty – it should
have been set earlier by handleMessageEndpoint");
+ } catch (IllegalStateException e) {
+ // Re-throw SDK compatibility errors with additional context
+ throw new IllegalStateException(
+ "Failed to extract session ID from MCP exchange. "
+ + "This may indicate an SDK compatibility issue. "
+ + "Tested SDK version: " +
McpSessionHelper.getSupportedSdkVersion() + ". "
+ + "Original error: " + e.getMessage(), e);
Review Comment:
The `try` block throws an `IllegalStateException` when `sessionId` is empty,
but the `catch (IllegalStateException e)` immediately wraps it into a new
exception that suggests an SDK compatibility issue. This makes non-SDK failures
(like an empty/missing session) look like compatibility problems; consider only
wrapping exceptions that originate from `McpSessionHelper` reflection failures
(e.g., messages starting with "SDK COMPATIBILITY ERROR"), and rethrow other
`IllegalStateException`s unchanged.
```suggestion
// Only wrap SDK compatibility-related errors; rethrow others
unchanged
final String originalMessage = e.getMessage();
if (originalMessage != null && originalMessage.startsWith("SDK
COMPATIBILITY ERROR")) {
throw new IllegalStateException(
"Failed to extract session ID from MCP exchange. "
+ "This may indicate an SDK compatibility issue. "
+ "Tested SDK version: " +
McpSessionHelper.getSupportedSdkVersion() + ". "
+ "Original error: " + originalMessage, e);
}
throw e;
```
##########
shenyu-plugin/shenyu-plugin-mcp-server/MCP_TOOL_EXAMPLES_EN.md:
##########
@@ -2,6 +2,27 @@
This document provides comprehensive examples of tool configurations for the
Shenyu MCP Server Plugin based on **real interfaces from the
shenyu-examples-http project**, covering different HTTP request methods,
parameter types, and configuration patterns.
+## SDK Version Compatibility
+
+This plugin is developed and tested with the following SDK versions:
+
+| Dependency | Version |
+|------------|---------|
+| MCP SDK (io.modelcontextprotocol.sdk:mcp-bom) | 0.17.0 |
+| Spring AI (org.springframework.ai:spring-ai-bom) | 1.1.2 |
+| Spring Boot | 3.3.1 |
+
+### Supported Protocols
+
+- **SSE (Server-Sent Events)**: `/sse` endpoint with long-polling session
support
+- **Streamable HTTP**: Unified endpoint supporting GET (SSE stream) and POST
(message) requests
Review Comment:
SSE is a streaming (server-push) mechanism, not long-polling. The
description "`/sse` endpoint with long-polling session support" is inaccurate;
consider changing it to describe a long-lived streaming connection (and, if
relevant, how session IDs are managed).
```suggestion
- **SSE (Server-Sent Events)**: `/sse` endpoint providing a long-lived,
one-way streaming (server-push) connection, with optional session ID–based
correlation
- **Streamable HTTP**: Unified endpoint where GET establishes an SSE stream
and POST sends messages correlated to that stream
```
##########
shenyu-plugin/shenyu-plugin-mcp-server/src/main/java/org/apache/shenyu/plugin/mcp/server/session/McpSessionHelper.java:
##########
@@ -51,54 +130,98 @@ public static McpSyncServerExchange
getMcpSyncServerExchange(final ToolContext t
}
return mcpSyncServerExchange;
}
-
+
/**
* Get sessionId from McpSyncServerExchange.
*
+ * <p>Uses reflection to access internal SDK fields. If reflection fails,
+ * an IllegalStateException is thrown with SDK compatibility information.
+ *
* @param mcpSyncServerExchange the McpSyncServerExchange instance
* @return the session id string
- * @throws NoSuchFieldException if field not found
- * @throws IllegalAccessException if field not accessible
+ * @throws IllegalStateException if SDK reflection fails (API
incompatibility)
*/
- public static String getSessionId(final McpSyncServerExchange
mcpSyncServerExchange)
- throws NoSuchFieldException, IllegalAccessException {
- Field asyncExchangeField =
mcpSyncServerExchange.getClass().getDeclaredField("exchange");
- asyncExchangeField.setAccessible(true);
- Object session = getSession(mcpSyncServerExchange, asyncExchangeField);
+ public static String getSessionId(final McpSyncServerExchange
mcpSyncServerExchange) {
+ McpServerSession session = getSession(mcpSyncServerExchange);
if (Objects.isNull(session)) {
throw new IllegalArgumentException("Session is required in
McpAsyncServerExchange");
}
Review Comment:
`getSessionId()` checks `Objects.isNull(session)` after calling
`getSession()`, but `getSession()` never returns null (it either returns a
non-null session or throws). This branch is effectively dead code and can be
removed to avoid misleading future readers about possible return values.
##########
shenyu-plugin/shenyu-plugin-mcp-server/src/main/java/org/apache/shenyu/plugin/mcp/server/callback/ShenyuToolCallback.java:
##########
@@ -761,20 +761,24 @@ private McpSyncServerExchange extractMcpExchange(final
ToolContext toolContext)
*
* @param mcpExchange the MCP sync server exchange
* @return the session ID
- * @throws IllegalStateException if session ID cannot be extracted
+ * @throws IllegalStateException if session ID cannot be extracted (SDK
compatibility issue)
*/
private String extractSessionId(final McpSyncServerExchange mcpExchange) {
- final String sessionId;
try {
- sessionId = McpSessionHelper.getSessionId(mcpExchange);
- } catch (NoSuchFieldException | IllegalAccessException e) {
- throw new RuntimeException(e);
- }
- if (StringUtils.hasText(sessionId)) {
- LOG.debug("Extracted session ID: {}", sessionId);
- return sessionId;
+ final String sessionId =
McpSessionHelper.getSessionId(mcpExchange);
+ if (StringUtils.hasText(sessionId)) {
+ LOG.debug("Extracted session ID: {}", sessionId);
+ return sessionId;
+ }
+ throw new IllegalStateException("Session ID is empty – it should
have been set earlier by handleMessageEndpoint");
+ } catch (IllegalStateException e) {
+ // Re-throw SDK compatibility errors with additional context
Review Comment:
`extractSessionId()` only catches `IllegalStateException`, but
`McpSessionHelper.getSessionId()` can also throw `IllegalArgumentException`
(e.g., missing async exchange/session). That will currently propagate without
the added context and contradict the method’s Javadoc (`@throws
IllegalStateException`). Consider catching/wrapping the helper’s
`IllegalArgumentException` as well (or changing the helper to throw
`IllegalStateException` consistently for extraction failures).
```suggestion
} catch (IllegalStateException | IllegalArgumentException e) {
// Re-throw errors with additional context (including potential
SDK compatibility issues)
```
--
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]
