Support for non-required MCP Tool arguments #33706
Conversation
| /** | ||
| * Tool Arguments are NOT required if: | ||
| * | ||
| * ToolArg.required is set to false | ||
| * ToolArg.defaultValue is set, and | ||
| * the argument return type is optional | ||
| */ | ||
| public static boolean isRequiredArgument(ArgumentMetadata argMetadata) { | ||
|
|
||
| if (argMetadata.defaultValue() != null && !argMetadata.defaultValue().isEmpty()) { | ||
| return false; | ||
| } | ||
| if (isOptionalType(argMetadata.type())) { | ||
| return false; | ||
| } | ||
| return argMetadata.required(); | ||
| } |
There was a problem hiding this comment.
I would prefer to do these checks before we create the argument metadata, and then set ArgumentMetadata.required according to these rules.
Then every other part of the code can just call argMetadata.required().
In general, the Metadata objects should reflect our final decisions about an object. For example, when we work out a tool name we look at the @Tool annotation, and if that doesn't set a value we look at the method name. Whatever we decide is what goes into the ToolMetadata so that nothing else has to worry about whether the name came from the annotation or from the method.
| public static Object resolveDefaultValue(ArgumentMetadata argMetadata) { | ||
| if (argMetadata.defaultValue() != null && !argMetadata.defaultValue().isEmpty()) { | ||
| try { | ||
| return convertDefaultValue(argMetadata.defaultValue(), argMetadata.type()); | ||
| } catch (Exception e) { | ||
| throw new IllegalStateException("Failed to convert default value"); //TODO nlsprops | ||
| } | ||
| } |
There was a problem hiding this comment.
Will this fail if the argument type is Optional<Something> and defaultValue is set?
| * @throws IllegalStateException if default value conversion fails | ||
| */ | ||
| public static Object resolveDefaultValue(ArgumentMetadata argMetadata) { | ||
| if (argMetadata.defaultValue() != null && !argMetadata.defaultValue().isEmpty()) { |
There was a problem hiding this comment.
We don't want to do this check like this. If null and empty string should both be interpreted as not having a default value, we should turn them both into null before it's added to ArgumentMetadata and document what null means for defaultValue.
| if (providedArgs.containsKey(argName)) { | ||
| return providedArgs.get(argName); | ||
| } |
There was a problem hiding this comment.
| if (providedArgs.containsKey(argName)) { | |
| return providedArgs.get(argName); | |
| } | |
| Object providedValue = providedArgs.get(argName); | |
| if (providedValue != null) { | |
| return providedValue; | |
| } |
Don't call containsKey and get back to back like this.
| try { | ||
| return convertDefaultValue(argMetadata.defaultValue(), argMetadata.type()); | ||
| } catch (Exception e) { | ||
| throw new IllegalStateException("Failed to convert default value"); //TODO nlsprops |
There was a problem hiding this comment.
| argsArray[i] = t.args().get(name); | ||
| ArgumentMetadata argMetadata = arguments.get(name); | ||
| if (argMetadata != null) { | ||
| argsArray[i] = ArgumentResolver.resolveArgumentValue(argMetadata, name, providedArgs); | ||
| } else { | ||
| argsArray[i] = providedArgs.get(name); | ||
| } |
There was a problem hiding this comment.
We should make ToolArguments contain the resolved argument values.
At the moment it doesn't make difference, but the interface for registering tools programatically allow a default value to be provided:
This only makes sense if the ToolArguments object provided to the handler already has these default values resolved.
2 participants
release buglabel if applicable: https://github.com/OpenLiberty/open-liberty/wiki/Open-Liberty-Conventions).Resolves #33562