Copilot commented on code in PR #6293:
URL: https://github.com/apache/shenyu/pull/6293#discussion_r2782286648
##########
shenyu-plugin/shenyu-plugin-mcp-server/src/main/java/org/apache/shenyu/plugin/mcp/server/manager/ShenyuMcpServerManager.java:
##########
@@ -479,17 +458,94 @@ private String normalizeServerPath(final String path) {
return null;
}
- String normalizedPath = path;
+ String normalizedPath = path.trim();
+ if (normalizedPath.isEmpty()) {
+ return "/";
+ }
+
+ try {
+ URI uri = URI.create(normalizedPath);
+ if (Objects.nonNull(uri.getScheme())) {
+ normalizedPath = uri.getRawPath();
+ }
+ } catch (IllegalArgumentException ignored) {
+ // Keep original input when it's not a full URI.
+ }
+
+ if (Objects.isNull(normalizedPath) || normalizedPath.isEmpty()) {
+ normalizedPath = "/";
+ }
+ if (!normalizedPath.startsWith("/")) {
+ normalizedPath = "/" + normalizedPath;
+ }
+ int queryStart = normalizedPath.indexOf('?');
+ if (queryStart >= 0) {
+ normalizedPath = normalizedPath.substring(0, queryStart);
+ }
+ int fragmentStart = normalizedPath.indexOf('#');
+ if (fragmentStart >= 0) {
+ normalizedPath = normalizedPath.substring(0, fragmentStart);
+ }
- // Remove /streamablehttp suffix
- if (normalizedPath.endsWith("/streamablehttp")) {
- normalizedPath = normalizedPath.substring(0,
normalizedPath.length() - "/streamablehttp".length());
- LOG.debug("Normalized Streamable HTTP path from '{}' to '{}' for
shared server", path, normalizedPath);
+ normalizedPath = normalizedPath.replaceAll("/{2,}", "/");
+ if (normalizedPath.endsWith("/**")) {
+ normalizedPath = normalizedPath.substring(0,
normalizedPath.length() - "/**".length());
}
+ normalizedPath = removeSuffix(normalizedPath, "/message");
+ normalizedPath = removeSuffix(normalizedPath, "/sse");
+ normalizedPath = removeSuffix(normalizedPath, "/streamablehttp");
+ if (normalizedPath.length() > 1 && normalizedPath.endsWith("/")) {
+ normalizedPath = normalizedPath.substring(0,
normalizedPath.length() - 1);
+ }
+ if (normalizedPath.isEmpty()) {
+ return "/";
+ }
return normalizedPath;
}
+ private String normalizeRoutePath(final String path) {
+ String routePath = Objects.isNull(path) ? "/" : path;
+ routePath = routePath.trim();
+ if (routePath.isEmpty()) {
+ return "/";
+ }
+ if (!routePath.startsWith("/")) {
+ routePath = "/" + routePath;
+ }
Review Comment:
`normalizeRoutePath` doesn’t strip query/fragment components and doesn’t
handle full URIs (scheme/host) the way `normalizeServerPath` does. Since routes
are registered using `originalUri` (e.g., Streamable HTTP) this can register
keys containing `?`/`#` or an entire URL, which will not match incoming request
paths. Consider extracting `URI.getRawPath()` when a scheme is present and
removing any `?`/`#` portions here too.
```suggestion
}
// Handle full URIs similarly to normalizeServerPath.
try {
URI uri = URI.create(routePath);
if (Objects.nonNull(uri.getScheme())) {
routePath = uri.getRawPath();
}
} catch (IllegalArgumentException ignored) {
// Keep original input when it's not a full URI.
}
if (Objects.isNull(routePath) || routePath.isEmpty()) {
routePath = "/";
}
if (!routePath.startsWith("/")) {
routePath = "/" + routePath;
}
int queryStart = routePath.indexOf('?');
if (queryStart >= 0) {
routePath = routePath.substring(0, queryStart);
}
int fragmentStart = routePath.indexOf('#');
if (fragmentStart >= 0) {
routePath = routePath.substring(0, fragmentStart);
}
```
##########
shenyu-plugin/shenyu-plugin-mcp-server/src/main/java/org/apache/shenyu/plugin/mcp/server/handler/McpServerPluginDataHandler.java:
##########
@@ -108,31 +101,27 @@ public void handlerSelector(final SelectorData
selectorData) {
@Override
public void removeSelector(final SelectorData selectorData) {
+ if (Objects.isNull(selectorData) ||
Objects.isNull(selectorData.getId())) {
+ return;
+ }
UpstreamCacheManager.getInstance().removeByKey(selectorData.getId());
MetaDataCache.getInstance().clean();
CACHED_TOOL.get().removeHandle(CacheKeyUtils.INST.getKey(selectorData.getId(),
Constants.DEFAULT_RULE));
- // Remove the McpServer for this URI
- // First try to get URI from handle, then from condition list
- String uri = selectorData.getHandle();
- if (StringUtils.isBlank(uri)) {
- // Try to get URI from condition list
- uri = selectorData.getConditionList().stream()
- .filter(condition ->
Constants.URI.equals(condition.getParamType()))
- .map(ConditionData::getParamValue)
- .findFirst()
- .orElse(null);
- }
+ String path = normalizeSelectorPath(extractSelectorUri(selectorData));
CACHED_SERVER.get().removeHandle(selectorData.getId());
- if (StringUtils.isNotBlank(uri) &&
shenyuMcpServerManager.hasMcpServer(uri)) {
- shenyuMcpServerManager.removeMcpServer(uri);
+ if (StringUtils.isNotBlank(path) &&
shenyuMcpServerManager.hasMcpServer(path)) {
+ shenyuMcpServerManager.removeMcpServer(path);
}
}
@Override
public void handlerRule(final RuleData ruleData) {
+ if (Objects.isNull(ruleData)) {
+ return;
+ }
Optional.ofNullable(ruleData.getHandle()).ifPresent(s -> {
ShenyuMcpServerTool mcpServerTool =
GsonUtils.getInstance().fromJson(s, ShenyuMcpServerTool.class);
CACHED_TOOL.get().cachedHandle(CacheKeyUtils.INST.getKey(ruleData),
mcpServerTool);
Review Comment:
`handlerRule` now guards against `ruleData == null`, but it can still call
`addTool(server.getPath(), ...)` later with a null/blank `server.getPath()`
(e.g., selector had no URI condition or cached server path is empty). That can
throw inside `ShenyuMcpServerManager` (ConcurrentHashMap null key). Consider
also gating tool registration on `server != null &&
StringUtils.isNotBlank(server.getPath())` (consistent with the `removeRule`
check).
##########
shenyu-plugin/shenyu-plugin-mcp-server/src/main/java/org/apache/shenyu/plugin/mcp/server/handler/McpServerPluginDataHandler.java:
##########
@@ -78,24 +78,17 @@ public void handlerSelector(final SelectorData
selectorData) {
return;
}
- // Get the URI from selector data
- String uri = selectorData.getConditionList().stream()
- .filter(condition ->
Constants.URI.equals(condition.getParamType()))
- .map(ConditionData::getParamValue)
- .findFirst()
- .orElse(null);
-
- String path = StringUtils.removeEnd(uri, SLASH);
- path = StringUtils.removeEnd(path, STAR);
+ String uri = extractSelectorUri(selectorData);
+ String path = normalizeSelectorPath(uri);
Review Comment:
`handlerSelector` continues even when no URI condition is found
(extractSelectorUri returns null), which can cache a server with a null path
and later lead to failures when tools are added/removed. Consider returning
early when the extracted URI/path is blank, and only caching/creating
transports once you have a valid normalized path.
```suggestion
String uri = extractSelectorUri(selectorData);
if (StringUtils.isBlank(uri)) {
// No valid URI condition found; do not cache or create
transports.
return;
}
String path = normalizeSelectorPath(uri);
if (StringUtils.isBlank(path)) {
// Normalization did not yield a usable path; abort handling.
return;
}
```
--
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]
