Copilot commented on code in PR #16267:
URL: https://github.com/apache/dubbo/pull/16267#discussion_r3223631533
##########
dubbo-plugin/dubbo-mcp/src/main/java/org/apache/dubbo/mcp/tool/DubboServiceToolRegistry.java:
##########
@@ -369,32 +370,19 @@ private String generateDefaultDescription(Method method,
ProviderModel providerM
providerModel.getServiceModel().getInterfaceName());
}
- private String generateToolSchema(Method method) {
- Map<String, Object> schemaMap = new HashMap<>();
- schemaMap.put(McpConstant.SCHEMA_PROPERTY_TYPE,
JsonSchemaType.OBJECT_SCHEMA.getJsonSchemaType());
-
+ private McpSchema.JsonSchema generateToolSchema(Method method) {
Map<String, Object> properties = new HashMap<>();
List<String> requiredParams = new ArrayList<>();
generateSchemaFromMethodSignature(method, properties, requiredParams);
- schemaMap.put(McpConstant.SCHEMA_PROPERTY_PROPERTIES, properties);
-
- if (!requiredParams.isEmpty()) {
- schemaMap.put(McpConstant.SCHEMA_PROPERTY_REQUIRED,
requiredParams);
- }
-
- try {
- return objectMapper.writeValueAsString(schemaMap);
- } catch (Exception e) {
- logger.error(
- LoggerCodeConstants.COMMON_UNEXPECTED_EXCEPTION,
- "",
- "",
- "Failed to generate tool schema for method " +
method.getName() + ": " + e.getMessage(),
- e);
- return "{\"type\":\"object\",\"properties\":{}}";
- }
+ return new McpSchema.JsonSchema(
+ JsonSchemaType.OBJECT_SCHEMA.getJsonSchemaType(),
+ properties,
+ requiredParams.isEmpty() ? null : requiredParams,
Review Comment:
JSON Schema’s `required` must be an array when present; passing `null` risks
emitting `\"required\": null` depending on mapper configuration, which can be
an invalid schema for downstream clients. Prefer omitting the field via an API
that doesn’t serialize nulls (or explicitly ensure nulls are excluded), or pass
an empty list only if the serializer omits empties—otherwise keep the field
absent.
##########
dubbo-plugin/dubbo-mcp/src/main/java/org/apache/dubbo/mcp/transport/DubboMcpStreamableTransportProvider.java:
##########
@@ -63,7 +63,7 @@ public class DubboMcpStreamableTransportProvider implements
McpStreamableServerT
private Factory sessionFactory;
- private final ObjectMapper objectMapper;
+ private final McpJsonMapper objectMapper;
Review Comment:
These identifiers are now misleading: `objectMapper` and `JSON` suggest
Jackson `ObjectMapper`, but the type is `McpJsonMapper`. Renaming to something
like `jsonMapper`/`mcpJsonMapper` (and `json` for the inner field) would reduce
confusion and make future MCP SDK upgrades/refactors less error-prone.
##########
dubbo-plugin/dubbo-mcp/src/main/java/org/apache/dubbo/mcp/transport/DubboMcpStreamableTransportProvider.java:
##########
@@ -528,12 +528,12 @@ private void
refreshSessionExpire(McpStreamableServerSession session) {
private static class DubboMcpSessionTransport implements
McpStreamableServerTransport {
- private final ObjectMapper JSON;
+ private final McpJsonMapper JSON;
Review Comment:
These identifiers are now misleading: `objectMapper` and `JSON` suggest
Jackson `ObjectMapper`, but the type is `McpJsonMapper`. Renaming to something
like `jsonMapper`/`mcpJsonMapper` (and `json` for the inner field) would reduce
confusion and make future MCP SDK upgrades/refactors less error-prone.
##########
dubbo-plugin/dubbo-mcp/src/main/java/org/apache/dubbo/mcp/core/McpApplicationDeployListener.java:
##########
@@ -104,14 +106,15 @@ public void onStarted(ApplicationModel applicationModel) {
Integer sessionTimeout =
globalConf.getInt(McpConstant.SETTINGS_MCP_SESSION_TIMEOUT,
McpConstant.DEFAULT_SESSION_TIMEOUT);
+ McpJsonMapper mcpJsonMapper = new JacksonMcpJsonMapper(new
ObjectMapper());
if ("streamable".equals(protocol)) {
dubboMcpStreamableTransportProvider =
- new DubboMcpStreamableTransportProvider(new
ObjectMapper(), sessionTimeout);
+ new DubboMcpStreamableTransportProvider(mcpJsonMapper,
sessionTimeout);
mcpAsyncServer =
McpServer.async(getDubboMcpStreamableTransportProvider())
.capabilities(serverCapabilities)
.build();
} else if ("sse".equals(protocol)) {
- dubboMcpSseTransportProvider = new
DubboMcpSseTransportProvider(new ObjectMapper(), sessionTimeout);
+ dubboMcpSseTransportProvider = new
DubboMcpSseTransportProvider(mcpJsonMapper, sessionTimeout);
Review Comment:
Creating a brand-new default Jackson `ObjectMapper` here can lead to
inconsistent JSON behavior (modules/config, null handling, naming strategies)
versus the rest of the app, and can also affect schema validity if nulls are
serialized. Consider reusing a centrally configured `ObjectMapper` (or a
singleton `McpJsonMapper`) and explicitly setting null/empty serialization
rules needed by MCP payloads/schemas.
##########
dubbo-plugin/dubbo-mcp/src/main/java/org/apache/dubbo/mcp/tool/DubboOpenApiToolConverter.java:
##########
@@ -313,8 +301,8 @@ private Map<String, Object>
extractParameterSchema(Operation op) {
}
});
}
- schema.put(McpConstant.SCHEMA_PROPERTY_PROPERTIES, props);
- return schema;
+ return new McpSchema.JsonSchema(
+ JsonSchemaType.OBJECT_SCHEMA.getJsonSchemaType(), props, null,
null, null, null);
Review Comment:
Same schema-validity concern as in the service tool registry: if
`JsonSchema` serializes nulls, clients could receive `required: null`/other
null fields, which can violate JSON Schema expectations. Consider constructing
`JsonSchema` in a way that guarantees nulls are omitted (or explicitly
configure the `McpJsonMapper`/`ObjectMapper` used for MCP to exclude nulls for
schema payloads).
##########
dubbo-plugin/dubbo-mcp/src/main/java/org/apache/dubbo/mcp/core/McpApplicationDeployListener.java:
##########
@@ -104,14 +106,15 @@ public void onStarted(ApplicationModel applicationModel) {
Integer sessionTimeout =
globalConf.getInt(McpConstant.SETTINGS_MCP_SESSION_TIMEOUT,
McpConstant.DEFAULT_SESSION_TIMEOUT);
+ McpJsonMapper mcpJsonMapper = new JacksonMcpJsonMapper(new
ObjectMapper());
if ("streamable".equals(protocol)) {
dubboMcpStreamableTransportProvider =
- new DubboMcpStreamableTransportProvider(new
ObjectMapper(), sessionTimeout);
+ new DubboMcpStreamableTransportProvider(mcpJsonMapper,
sessionTimeout);
Review Comment:
Creating a brand-new default Jackson `ObjectMapper` here can lead to
inconsistent JSON behavior (modules/config, null handling, naming strategies)
versus the rest of the app, and can also affect schema validity if nulls are
serialized. Consider reusing a centrally configured `ObjectMapper` (or a
singleton `McpJsonMapper`) and explicitly setting null/empty serialization
rules needed by MCP payloads/schemas.
--
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]
