Copilot commented on code in PR #16267:
URL: https://github.com/apache/dubbo/pull/16267#discussion_r3224399698
##########
dubbo-plugin/dubbo-mcp/src/main/java/org/apache/dubbo/mcp/transport/DubboMcpStreamableTransportProvider.java:
##########
@@ -74,11 +74,11 @@ public class DubboMcpStreamableTransportProvider implements
McpStreamableServerT
*/
private final ExpiringMap<String, McpStreamableServerSession> sessions;
- public DubboMcpStreamableTransportProvider(ObjectMapper objectMapper) {
+ public DubboMcpStreamableTransportProvider(McpJsonMapper objectMapper) {
this(objectMapper, McpConstant.DEFAULT_SESSION_TIMEOUT);
}
- public DubboMcpStreamableTransportProvider(ObjectMapper objectMapper,
Integer expireSeconds) {
+ public DubboMcpStreamableTransportProvider(McpJsonMapper objectMapper,
Integer expireSeconds) {
// Minimum expiration time is 60 seconds
Review Comment:
In the constructors, the parameter is named `objectMapper` but its type is
`McpJsonMapper` (and the field is `mcpJsonMapper`). Renaming the parameter to
something like `jsonMapper`/`mcpJsonMapper` would avoid confusion with
Jackson's `ObjectMapper` and better reflect the actual dependency type.
##########
dubbo-plugin/dubbo-mcp/src/main/java/org/apache/dubbo/mcp/transport/DubboMcpStreamableTransportProvider.java:
##########
@@ -528,14 +528,14 @@ private void
refreshSessionExpire(McpStreamableServerSession session) {
private static class DubboMcpSessionTransport implements
McpStreamableServerTransport {
- private final ObjectMapper JSON;
+ private final McpJsonMapper mcpJsonMapper1;
private final StreamObserver<ServerSentEvent<byte[]>> responseObserver;
public DubboMcpSessionTransport(
- StreamObserver<ServerSentEvent<byte[]>> responseObserver,
ObjectMapper objectMapper) {
+ StreamObserver<ServerSentEvent<byte[]>> responseObserver,
McpJsonMapper objectMapper) {
this.responseObserver = responseObserver;
- this.JSON = objectMapper;
+ this.mcpJsonMapper1 = objectMapper;
Review Comment:
`mcpJsonMapper1` is an unclear field name for the session transport and
reads like an auto-generated temporary variable. Consider renaming it to
`mcpJsonMapper`/`jsonMapper` (and similarly rename the constructor parameter)
to improve readability.
##########
dubbo-plugin/dubbo-mcp/src/main/java/org/apache/dubbo/mcp/transport/DubboMcpSseTransportProvider.java:
##########
@@ -179,12 +179,12 @@ private void
sendEvent(StreamObserver<ServerSentEvent<String>> responseObserver,
private static class DubboMcpSessionTransport implements
McpServerTransport {
- private final ObjectMapper JSON;
+ private final McpJsonMapper JSON;
private final StreamObserver<ServerSentEvent<String>> responseObserver;
public DubboMcpSessionTransport(
- StreamObserver<ServerSentEvent<String>> responseObserver,
ObjectMapper objectMapper) {
+ StreamObserver<ServerSentEvent<String>> responseObserver,
McpJsonMapper objectMapper) {
this.responseObserver = responseObserver;
this.JSON = objectMapper;
}
Review Comment:
The instance field name `JSON` is all-caps but it’s not a constant (`static
final`). Renaming it to `jsonMapper`/`mcpJsonMapper` would better match typical
Java naming conventions and avoid implying constant semantics.
--
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]
