Copilot commented on code in PR #16267:
URL: https://github.com/apache/dubbo/pull/16267#discussion_r3225573724
##########
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) {
- this(objectMapper, McpConstant.DEFAULT_SESSION_TIMEOUT);
+ public DubboMcpStreamableTransportProvider(McpJsonMapper mcpJsonMapper) {
+ this(mcpJsonMapper, McpConstant.DEFAULT_SESSION_TIMEOUT);
}
- public DubboMcpStreamableTransportProvider(ObjectMapper objectMapper,
Integer expireSeconds) {
+ public DubboMcpStreamableTransportProvider(McpJsonMapper mcpJsonMapper,
Integer expireSeconds) {
Review Comment:
The public constructors now require `McpJsonMapper`, which is a
source/binary breaking change for any external code instantiating this
transport provider. If backward compatibility is a goal for the 3.3 line,
consider keeping an overload that accepts a Jackson `ObjectMapper` (deprecated)
and wraps it with `JacksonMcpJsonMapper` internally.
##########
dubbo-plugin/dubbo-mcp/src/main/java/org/apache/dubbo/mcp/tool/DubboServiceToolRegistry.java:
##########
@@ -369,32 +370,14 @@ 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, null, null, null);
Review Comment:
`generateToolSchema` always passes a (potentially empty) `requiredParams`
list into `McpSchema.JsonSchema`. Previously, the `required` field was omitted
when there were no required params; now it may serialize as `"required":[]`,
which is a behavior change and also inconsistent with
`DubboOpenApiToolConverter` (it passes `null` when no required list). Consider
passing `null` when `requiredParams` is empty to preserve prior output and keep
schema generation consistent.
--
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]
