Skip to content

Commit c2efae1

Browse files
Aias00Copilot478320
authored
fix: improve MCP server plugin path handling and backward compatibility for argsPosition (#6293)
* fix: improve MCP server plugin path handling and backward compatibility for argsPosition * fix: enhance MCP server plugin path validation and improve handling of blank paths * Update shenyu-plugin/shenyu-plugin-mcp-server/src/main/java/org/apache/shenyu/plugin/mcp/server/handler/McpServerPluginDataHandler.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Update shenyu-plugin/shenyu-plugin-mcp-server/src/main/java/org/apache/shenyu/plugin/mcp/server/manager/ShenyuMcpServerManager.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * fix: streamline MCP server plugin path handling by removing redundant checks * fix: remove unused imports in ShenyuMcpServerManager --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Yu Siheng <2874602095@qq.com>
1 parent 7ff5ba3 commit c2efae1

11 files changed

Lines changed: 257 additions & 77 deletions

File tree

‎shenyu-client/shenyu-client-mcp/shenyu-client-mcp-common/src/main/java/org/apache/shenyu/client/mcp/generator/McpRequestConfigGenerator.java‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -60,11 +60,10 @@ public static JsonObject generateRequestConfig(final JsonObject openApiJson, fin
6060
requestTemplate.addProperty(RequestTemplateConstants.METHOD_KEY, methodType);
6161

6262
// argsPosition
63+
JsonObject argsPosition = new JsonObject();
6364
JsonObject methodTypeJson = method.getAsJsonObject(methodType);
6465
JsonArray parameters = methodTypeJson.getAsJsonArray(OpenApiConstants.OPEN_API_PATH_OPERATION_METHOD_PARAMETERS_KEY);
6566
if (Objects.nonNull(parameters)) {
66-
JsonObject argsPosition = new JsonObject();
67-
6867
for (JsonElement parameter : parameters) {
6968
JsonObject paramObj = parameter.getAsJsonObject();
7069

@@ -77,8 +76,11 @@ public static JsonObject generateRequestConfig(final JsonObject openApiJson, fin
7776
argsPosition.addProperty(name, inValue);
7877
}
7978
}
80-
requestTemplate.add(RequestTemplateConstants.ARGS_POSITION_KEY, argsPosition);
8179
}
80+
// Keep root-level argsPosition as canonical format used by gateway parser.
81+
root.add(RequestTemplateConstants.ARGS_POSITION_KEY, argsPosition.deepCopy());
82+
// Keep requestTemplate-level argsPosition for backward compatibility.
83+
requestTemplate.add(RequestTemplateConstants.ARGS_POSITION_KEY, argsPosition);
8284

8385
// argsToJsonBody
8486
requestTemplate.addProperty(RequestTemplateConstants.BODY_JSON_KEY, shenyuMcpRequestConfig.getBodyToJson());

‎shenyu-client/shenyu-client-mcp/shenyu-client-mcp-register/src/main/java/org/apache/shenyu/client/mcp/McpServiceEventListener.java‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -304,6 +304,9 @@ private static String concatPaths(final String path1, final String path2) {
304304
@Override
305305
protected String buildApiSuperPath(final Class<?> clazz, final ShenyuMcpTool beanShenyuClient) {
306306
Server[] servers = beanShenyuClient.definition().servers();
307+
if (servers.length == 0) {
308+
return "";
309+
}
307310
if (servers.length != 1) {
308311
log.warn("The shenyuMcp service supports only a single server entry. Please ensure that only one server is configured");
309312
}
@@ -363,7 +366,7 @@ private McpToolsRegisterDTO buildMcpToolsRegisterDTO(final Object bean, final Cl
363366
validateClientConfig(shenyuMcpTool, url);
364367
JsonObject openApiJson = McpOpenApiGenerator.generateOpenApiJson(classShenyuClient, shenyuMcpTool, url);
365368
McpToolsRegisterDTO mcpToolsRegisterDTO = McpToolsRegisterDTOGenerator.generateRegisterDTO(shenyuMcpTool, openApiJson, url, namespaceId);
366-
MetaDataRegisterDTO metaDataRegisterDTO = buildMetaDataDTO(bean, classShenyuClient, superPath, clazz, method, namespaceId);
369+
MetaDataRegisterDTO metaDataRegisterDTO = buildMetaDataDTO(bean, classShenyuClient, url, clazz, method, namespaceId);
367370
metaDataRegisterDTO.setEnabled(shenyuMcpTool.getEnable());
368371
mcpToolsRegisterDTO.setMetaDataRegisterDTO(metaDataRegisterDTO);
369372
return mcpToolsRegisterDTO;

‎shenyu-plugin/shenyu-plugin-mcp-server/src/main/java/org/apache/shenyu/plugin/mcp/server/handler/McpServerPluginDataHandler.java‎

Lines changed: 46 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -78,24 +78,23 @@ public void handlerSelector(final SelectorData selectorData) {
7878
return;
7979
}
8080

81-
// Get the URI from selector data
82-
String uri = selectorData.getConditionList().stream()
83-
.filter(condition -> Constants.URI.equals(condition.getParamType()))
84-
.map(ConditionData::getParamValue)
85-
.findFirst()
86-
.orElse(null);
87-
88-
String path = StringUtils.removeEnd(uri, SLASH);
89-
path = StringUtils.removeEnd(path, STAR);
81+
String uri = extractSelectorUri(selectorData);
82+
if (StringUtils.isBlank(uri)) {
83+
return;
84+
}
85+
String path = normalizeSelectorPath(uri);
86+
if (StringUtils.isBlank(path)) {
87+
return;
88+
}
9089
ShenyuMcpServer shenyuMcpServer = GsonUtils.getInstance().fromJson(StringUtils.isBlank(selectorData.getHandle()) ? DEFAULT_MESSAGE_ENDPOINT : selectorData.getHandle(), ShenyuMcpServer.class);
9190
shenyuMcpServer.setPath(path);
9291
CACHED_SERVER.get().cachedHandle(
9392
selectorData.getId(),
9493
shenyuMcpServer);
9594
String messageEndpoint = shenyuMcpServer.getMessageEndpoint();
9695
// Get or create McpServer for this URI
97-
if (StringUtils.isNotBlank(uri) && !shenyuMcpServerManager.hasMcpServer(uri)) {
98-
shenyuMcpServerManager.getOrCreateMcpServerTransport(uri, messageEndpoint);
96+
if (StringUtils.isNotBlank(path) && !shenyuMcpServerManager.hasMcpServer(path)) {
97+
shenyuMcpServerManager.getOrCreateMcpServerTransport(path, messageEndpoint);
9998
}
10099
if (StringUtils.isNotBlank(path)) {
101100
shenyuMcpServerManager.getOrCreateStreamableHttpTransport(path + STREAMABLE_HTTP_PATH);
@@ -108,31 +107,27 @@ public void handlerSelector(final SelectorData selectorData) {
108107

109108
@Override
110109
public void removeSelector(final SelectorData selectorData) {
110+
if (Objects.isNull(selectorData) || Objects.isNull(selectorData.getId())) {
111+
return;
112+
}
111113
UpstreamCacheManager.getInstance().removeByKey(selectorData.getId());
112114
MetaDataCache.getInstance().clean();
113115
CACHED_TOOL.get().removeHandle(CacheKeyUtils.INST.getKey(selectorData.getId(), Constants.DEFAULT_RULE));
114116

115-
// Remove the McpServer for this URI
116-
// First try to get URI from handle, then from condition list
117-
String uri = selectorData.getHandle();
118-
if (StringUtils.isBlank(uri)) {
119-
// Try to get URI from condition list
120-
uri = selectorData.getConditionList().stream()
121-
.filter(condition -> Constants.URI.equals(condition.getParamType()))
122-
.map(ConditionData::getParamValue)
123-
.findFirst()
124-
.orElse(null);
125-
}
117+
String path = normalizeSelectorPath(extractSelectorUri(selectorData));
126118

127119
CACHED_SERVER.get().removeHandle(selectorData.getId());
128120

129-
if (StringUtils.isNotBlank(uri) && shenyuMcpServerManager.hasMcpServer(uri)) {
130-
shenyuMcpServerManager.removeMcpServer(uri);
121+
if (StringUtils.isNotBlank(path) && shenyuMcpServerManager.hasMcpServer(path)) {
122+
shenyuMcpServerManager.removeMcpServer(path);
131123
}
132124
}
133125

134126
@Override
135127
public void handlerRule(final RuleData ruleData) {
128+
if (Objects.isNull(ruleData)) {
129+
return;
130+
}
136131
Optional.ofNullable(ruleData.getHandle()).ifPresent(s -> {
137132
ShenyuMcpServerTool mcpServerTool = GsonUtils.getInstance().fromJson(s, ShenyuMcpServerTool.class);
138133
CACHED_TOOL.get().cachedHandle(CacheKeyUtils.INST.getKey(ruleData), mcpServerTool);
@@ -145,7 +140,7 @@ public void handlerRule(final RuleData ruleData) {
145140
// Create JSON schema from parameters
146141
String inputSchema = JsonSchemaUtil.createParameterSchema(parameters);
147142
ShenyuMcpServer server = CACHED_SERVER.get().obtainHandle(ruleData.getSelectorId());
148-
if (Objects.nonNull(server)) {
143+
if (Objects.nonNull(server) && StringUtils.isNotBlank(server.getPath())) {
149144
shenyuMcpServerManager.addTool(server.getPath(),
150145
StringUtils.isBlank(mcpServerTool.getName()) ? ruleData.getName()
151146
: mcpServerTool.getName(),
@@ -158,10 +153,15 @@ public void handlerRule(final RuleData ruleData) {
158153

159154
@Override
160155
public void removeRule(final RuleData ruleData) {
156+
if (Objects.isNull(ruleData)) {
157+
return;
158+
}
161159
Optional.ofNullable(ruleData.getHandle()).ifPresent(s -> {
162160
CACHED_TOOL.get().removeHandle(CacheKeyUtils.INST.getKey(ruleData));
163161
ShenyuMcpServer server = CACHED_SERVER.get().obtainHandle(ruleData.getSelectorId());
164-
shenyuMcpServerManager.removeTool(server.getPath(), ruleData.getName());
162+
if (Objects.nonNull(server) && StringUtils.isNotBlank(server.getPath())) {
163+
shenyuMcpServerManager.removeTool(server.getPath(), ruleData.getName());
164+
}
165165
});
166166
MetaDataCache.getInstance().clean();
167167
}
@@ -171,4 +171,24 @@ public String pluginNamed() {
171171
return PluginEnum.MCP_SERVER.getName();
172172
}
173173

174+
private String extractSelectorUri(final SelectorData selectorData) {
175+
if (Objects.isNull(selectorData) || CollectionUtils.isEmpty(selectorData.getConditionList())) {
176+
return null;
177+
}
178+
return selectorData.getConditionList().stream()
179+
.filter(condition -> Constants.URI.equals(condition.getParamType()))
180+
.map(ConditionData::getParamValue)
181+
.findFirst()
182+
.orElse(null);
183+
}
184+
185+
private String normalizeSelectorPath(final String selectorUri) {
186+
if (StringUtils.isBlank(selectorUri)) {
187+
return selectorUri;
188+
}
189+
String path = StringUtils.removeEnd(selectorUri, STAR);
190+
path = StringUtils.removeEnd(path, SLASH);
191+
return StringUtils.defaultIfBlank(path, SLASH);
192+
}
193+
174194
}

‎shenyu-plugin/shenyu-plugin-mcp-server/src/main/java/org/apache/shenyu/plugin/mcp/server/manager/ShenyuMcpServerManager.java‎

Lines changed: 119 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@
4242
import java.util.Map;
4343
import java.util.Objects;
4444
import java.util.Set;
45+
import java.net.URI;
4546
import java.util.concurrent.ConcurrentHashMap;
4647

4748
/**
@@ -172,7 +173,7 @@ private <T> T getOrCreateTransport(final String normalizedPath, final String pro
172173
* @return normalized path
173174
*/
174175
private String processPath(final String uri) {
175-
return normalizeServerPath(extractBasePath(uri));
176+
return normalizeServerPath(uri);
176177
}
177178

178179
/**
@@ -244,7 +245,7 @@ private McpAsyncServer getOrCreateSharedServer(final String normalizedPath) {
244245
* Creates SSE transport provider.
245246
*/
246247
private ShenyuSseServerTransportProvider createSseTransport(final String normalizedPath, final String messageEndPoint) {
247-
String messageEndpoint = normalizedPath + messageEndPoint;
248+
String messageEndpoint = joinPath(normalizedPath, messageEndPoint);
248249
ShenyuSseServerTransportProvider transportProvider = ShenyuSseServerTransportProvider.builder()
249250
.objectMapper(objectMapper)
250251
.sseEndpoint(normalizedPath)
@@ -285,39 +286,17 @@ private ShenyuStreamableHttpServerTransportProvider createStreamableHttpTranspor
285286
*/
286287
private void registerRoutes(final String primaryPath, final String secondaryPath,
287288
final HandlerFunction<?> primaryHandler, final HandlerFunction<?> secondaryHandler) {
288-
routeMap.put(primaryPath, primaryHandler);
289-
routeMap.put(primaryPath + "/**", primaryHandler);
289+
String normalizedPrimaryPath = normalizeRoutePath(primaryPath);
290+
routeMap.put(normalizedPrimaryPath, primaryHandler);
291+
routeMap.put(normalizedPrimaryPath + "/**", primaryHandler);
290292

291293
if (Objects.nonNull(secondaryPath) && Objects.nonNull(secondaryHandler)) {
292-
routeMap.put(secondaryPath, secondaryHandler);
293-
routeMap.put(secondaryPath + "/**", secondaryHandler);
294+
String normalizedSecondaryPath = normalizeRoutePath(secondaryPath);
295+
routeMap.put(normalizedSecondaryPath, secondaryHandler);
296+
routeMap.put(normalizedSecondaryPath + "/**", secondaryHandler);
294297
}
295298
}
296299

297-
/**
298-
* Extract the base path from a URI by removing the /message suffix and any sub-paths.
299-
*
300-
* @param uri The URI to extract base path from
301-
* @return The base path
302-
*/
303-
private String extractBasePath(final String uri) {
304-
String basePath = uri;
305-
306-
// Remove /message suffix if present
307-
if (basePath.endsWith("/message")) {
308-
basePath = basePath.substring(0, basePath.length() - "/message".length());
309-
}
310-
311-
// For sub-paths, extract the main MCP server path
312-
String[] pathSegments = basePath.split("/");
313-
if (pathSegments.length >= 2) {
314-
// Keep only the first two segments (empty + server-name)
315-
basePath = "/" + pathSegments[1];
316-
}
317-
318-
return basePath;
319-
}
320-
321300
/**
322301
* Check if a McpServer exists for the given URI.
323302
*
@@ -390,7 +369,7 @@ public void removeMcpServer(final String uri) {
390369
*/
391370
public synchronized void addTool(final String serverPath, final String name, final String description,
392371
final String requestTemplate, final String inputSchema) {
393-
String normalizedPath = normalizeServerPath(extractBasePath(serverPath));
372+
String normalizedPath = processPath(serverPath);
394373

395374
// Remove existing tool first
396375
try {
@@ -442,7 +421,7 @@ public synchronized void addTool(final String serverPath, final String name, fin
442421
* @param name the tool name
443422
*/
444423
public void removeTool(final String serverPath, final String name) {
445-
String normalizedPath = normalizeServerPath(serverPath);
424+
String normalizedPath = processPath(serverPath);
446425
LOG.debug("Removing tool from shared server - name: {}, path: {}", name, normalizedPath);
447426

448427
McpAsyncServer sharedServer = sharedServerMap.get(normalizedPath);
@@ -485,7 +464,7 @@ private boolean isToolNotFoundError(final Throwable error) {
485464
* @return Set of supported protocols
486465
*/
487466
public Set<String> getSupportedProtocols(final String serverPath) {
488-
String normalizedPath = normalizeServerPath(serverPath);
467+
String normalizedPath = processPath(serverPath);
489468
CompositeTransportProvider compositeTransport = compositeTransportMap.get(normalizedPath);
490469
return Objects.nonNull(compositeTransport) ? compositeTransport.getSupportedProtocols() : new HashSet<>();
491470
}
@@ -501,17 +480,119 @@ private String normalizeServerPath(final String path) {
501480
return null;
502481
}
503482

504-
String normalizedPath = path;
483+
String normalizedPath = path.trim();
484+
if (normalizedPath.isEmpty()) {
485+
return "/";
486+
}
505487

506-
// Remove /streamablehttp suffix
507-
if (normalizedPath.endsWith("/streamablehttp")) {
508-
normalizedPath = normalizedPath.substring(0, normalizedPath.length() - "/streamablehttp".length());
509-
LOG.debug("Normalized Streamable HTTP path from '{}' to '{}' for shared server", path, normalizedPath);
488+
try {
489+
URI uri = URI.create(normalizedPath);
490+
if (Objects.nonNull(uri.getScheme())) {
491+
normalizedPath = uri.getRawPath();
492+
}
493+
} catch (IllegalArgumentException ignored) {
494+
// Keep original input when it's not a full URI.
495+
}
496+
497+
if (Objects.isNull(normalizedPath) || normalizedPath.isEmpty()) {
498+
normalizedPath = "/";
499+
}
500+
if (!normalizedPath.startsWith("/")) {
501+
normalizedPath = "/" + normalizedPath;
502+
}
503+
int queryStart = normalizedPath.indexOf('?');
504+
if (queryStart >= 0) {
505+
normalizedPath = normalizedPath.substring(0, queryStart);
506+
}
507+
int fragmentStart = normalizedPath.indexOf('#');
508+
if (fragmentStart >= 0) {
509+
normalizedPath = normalizedPath.substring(0, fragmentStart);
510510
}
511511

512+
normalizedPath = normalizedPath.replaceAll("/{2,}", "/");
513+
if (normalizedPath.endsWith("/**")) {
514+
normalizedPath = normalizedPath.substring(0, normalizedPath.length() - "/**".length());
515+
}
516+
normalizedPath = removeSuffix(normalizedPath, "/message");
517+
normalizedPath = removeSuffix(normalizedPath, "/sse");
518+
normalizedPath = removeSuffix(normalizedPath, "/streamablehttp");
519+
520+
if (normalizedPath.length() > 1 && normalizedPath.endsWith("/")) {
521+
normalizedPath = normalizedPath.substring(0, normalizedPath.length() - 1);
522+
}
523+
if (normalizedPath.isEmpty()) {
524+
return "/";
525+
}
512526
return normalizedPath;
513527
}
514528

529+
private String normalizeRoutePath(final String path) {
530+
String routePath = Objects.isNull(path) ? "/" : path;
531+
routePath = routePath.trim();
532+
if (routePath.isEmpty()) {
533+
return "/";
534+
}
535+
536+
try {
537+
URI uri = URI.create(routePath);
538+
if (Objects.nonNull(uri.getScheme())) {
539+
routePath = uri.getRawPath();
540+
}
541+
} catch (IllegalArgumentException ignored) {
542+
// Keep original input when it's not a full URI.
543+
}
544+
545+
if (Objects.isNull(routePath) || routePath.isEmpty()) {
546+
routePath = "/";
547+
}
548+
if (!routePath.startsWith("/")) {
549+
routePath = "/" + routePath;
550+
}
551+
552+
int queryStart = routePath.indexOf('?');
553+
if (queryStart >= 0) {
554+
routePath = routePath.substring(0, queryStart);
555+
}
556+
int fragmentStart = routePath.indexOf('#');
557+
if (fragmentStart >= 0) {
558+
routePath = routePath.substring(0, fragmentStart);
559+
}
560+
routePath = routePath.replaceAll("/{2,}", "/");
561+
if (routePath.length() > 1 && routePath.endsWith("/")) {
562+
routePath = routePath.substring(0, routePath.length() - 1);
563+
}
564+
if (routePath.isEmpty()) {
565+
return "/";
566+
}
567+
return routePath;
568+
}
569+
570+
private String joinPath(final String basePath, final String subPath) {
571+
String safeBase = normalizeRoutePath(basePath);
572+
if (Objects.isNull(subPath) || subPath.trim().isEmpty()) {
573+
return safeBase;
574+
}
575+
String safeSub = subPath.trim();
576+
if (safeBase.endsWith("/") && safeSub.startsWith("/")) {
577+
return safeBase + safeSub.substring(1);
578+
}
579+
if (!safeBase.endsWith("/") && !safeSub.startsWith("/")) {
580+
return safeBase + "/" + safeSub;
581+
}
582+
return safeBase + safeSub;
583+
}
584+
585+
private String removeSuffix(final String value, final String suffix) {
586+
if (Objects.isNull(value) || Objects.isNull(suffix) || suffix.isEmpty()) {
587+
return value;
588+
}
589+
if (value.endsWith(suffix)) {
590+
String result = value.substring(0, value.length() - suffix.length());
591+
return result.isEmpty() ? "/" : result;
592+
}
593+
return value;
594+
}
595+
515596
/**
516597
* Composite transport provider that delegates to multiple transport implementations.
517598
* Enhanced with protocol-aware session management and improved error handling.

0 commit comments

Comments
 (0)