Adding an overload of McpClientExtensions::CallToolAsync taking JsonElement tool argument #641
Conversation
| public static ValueTask<CallToolResult> CallToolAsync( | ||
| this IMcpClient client, | ||
| string toolName, | ||
| IReadOnlyDictionary<string, JsonElement> arguments, |
There was a problem hiding this comment.
There are many ways in which you could represent tool arguments in STJ: IDictionary<string, JsonElement>, IDictionary<string, object?>, JsonElement, JsonObject, IReadOnlyDictionary<string, JsonNode?>, so singling out an overload for this particular representation feels oddly specific. Rather than exposing overloads for each and every one of them, I would be inclined to just ask users to populate the one representation picked by the library (or author extension methods that perform the conversion under wraps).
There was a problem hiding this comment.
We could also consider just having an overload that takes a JsonElement for the arguments. Then someone that has json wouldn't need to first parse it into a dictionary.
There was a problem hiding this comment.
thanks @eiriktsarpalis and @stephentoub for taking a look.
We could also consider just having an overload that takes a JsonElement for the arguments. Then someone that has json wouldn't need to first parse it into a dictionary.
This is great feedback @stephentoub. Yes, this helps optimize the call sites even better, often user may obtain immutable JsonElement with little overhead. I’ve updated the pr. Could you please review this and let me know if there are any areas for improvement?
There was a problem hiding this comment.
It would still need to be converted under the hood by the overload, since AIFunction expects dictionaries. Which is fine I suppose if we only care about this being an accelerator.
There was a problem hiding this comment.
It would still need to be converted under the hood by the overload, since AIFunction expects dictionaries. Which is fine I suppose if we only care about this being an accelerator.
That is correct. I was not sure, for this one use case, if it is appropriate to define a variant of CallToolRequestParams that uses an Arguments property of type JsonElement, as well as to add a corresponding context.
There was a problem hiding this comment.
It would still need to be converted under the hood by the overload, since AIFunction expects dictionaries.
This doesn't get passed to AIFunction; it's written into a json rpc request.
There was a problem hiding this comment.
The underlying request model is using Dictionary, which necessitates a conversion from JsonElement. In the future we might want to consider using a different type so that we avoid doing that.
anuchandy
changed the title
| new() | ||
| { | ||
| Name = toolName, | ||
| Arguments = arguments.EnumerateObject().ToDictionary(prop => prop.Name, prop => prop.Value) ?? [] |
There was a problem hiding this comment.
This will throw an exception if the JsonElement isn't representing an object. Perhaps it makes sense to add a check at the start of the method so that a bespoke error message can be raised?
There was a problem hiding this comment.
Great point, Just updated the pr with the check as suggested.
There was a problem hiding this comment.
This will throw an exception if the JsonElement isn't representing an object. Perhaps it makes sense to add a check at the start of the method so that a bespoke error message can be raised?
What if it's a no-parameter tool? Would JsonValueKind.Undefined be an acceptable value then, or must it be a property-less JsonElement object? I would think default(JsonElement) would an intuitive choice for someone who is passing no arguments, and that's undefined afair?
There was a problem hiding this comment.
Hmm, good point. Maybe we should make the parameter nullable for that reason, needing to create a null JsonElement or empty object seems somewhat overbearing.
|
|
||
| [Theory] | ||
| [MemberData(nameof(GetClients))] | ||
| public async Task CallTool_Stdio_EchoServer_WithJsonElementArguments_ThrowsForNonObject(string clientId) |
There was a problem hiding this comment.
For completeness, you could try turning this into a theory that accepts all types of invalid JSON.
Adding
McpClientExtensions::CallToolAsyncoverload that takes tool parameters asDictionary<string, JsonElement>JsonElement.Motivation and Context
Please see #640
How Has This Been Tested?
yes, and added the unit tests.
Breaking Changes
this is a non-breaking change.
Types of changes
Checklist
Additional context