Copilot commented on code in PR #16267:
URL: https://github.com/apache/dubbo/pull/16267#discussion_r3224688547
##########
dubbo-plugin/dubbo-mcp/src/main/java/org/apache/dubbo/mcp/transport/DubboMcpSseTransportProvider.java:
##########
@@ -82,12 +82,12 @@ public DubboMcpSseTransportProvider(ObjectMapper
objectMapper, Integer expireSec
expireSeconds = 60;
}
sessions = new ExpiringMap<>(expireSeconds, 30);
- this.objectMapper = objectMapper;
+ this.mcpJsonMapper = mcpJsonMapper;
Review Comment:
The constructor stores the provided `mcpJsonMapper` without validating it.
If a caller passes null, request parsing/serialization will later fail with an
NPE. Consider enforcing non-null in the constructor (fail fast with a clear
exception) to make the provider API safer.
##########
dubbo-plugin/dubbo-mcp/src/main/java/org/apache/dubbo/mcp/transport/DubboMcpSseTransportProvider.java:
##########
@@ -147,7 +147,7 @@ public void handleMessage() {
refreshSessionExpire(session);
try {
McpSchema.JSONRPCMessage message =
McpSchema.deserializeJsonRpcMessage(
- objectMapper, IOUtils.read(request.inputStream(),
String.valueOf(StandardCharsets.UTF_8)));
+ mcpJsonMapper, IOUtils.read(request.inputStream(),
String.valueOf(StandardCharsets.UTF_8)));
Review Comment:
`IOUtils.read(...)` expects a charset name;
`String.valueOf(StandardCharsets.UTF_8)` happens to produce "UTF-8" but is
indirect and easy to misread. Prefer `StandardCharsets.UTF_8.name()` (or an
overload that accepts `Charset`, if available) for clarity.
##########
dubbo-plugin/dubbo-mcp/src/main/java/org/apache/dubbo/mcp/transport/DubboMcpStreamableTransportProvider.java:
##########
@@ -88,7 +88,7 @@ public DubboMcpStreamableTransportProvider(ObjectMapper
objectMapper, Integer ex
expireSeconds = 60;
}
sessions = new ExpiringMap<>(expireSeconds, 30);
- this.objectMapper = objectMapper;
+ this.mcpJsonMapper = mcpJsonMapper;
sessions.getExpireThread().startExpiryIfNotStarted();
Review Comment:
The constructor stores the provided `mcpJsonMapper` without validating it.
If a caller passes null, this will later NPE during request
handling/serialization. Consider enforcing non-null in the constructor (e.g.,
fail fast with a clear exception) to make the API safer.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
