-
Notifications
You must be signed in to change notification settings - Fork 577
Helper method to merge additional metadata into requests, results, and notifications #1030
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
@stephentoub @PederHP Is this what you had in mind when you suggested a helper method to merge metadata into an existing request / result / notification? |
Yes, exactly. |
| /// Merges additional metadata into the existing Meta object. | ||
| /// </summary> | ||
| /// <param name="additionalMeta">The additional metadata to merge.</param> | ||
| public void MergeMeta(JsonObject additionalMeta) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're needing to duplicate this method into each place that has Meta. We're also baking in the overwrite semantics of the additionalMeta always winning, as opposed to an existing value winning. If this is common enough to need a helper for, should we instead expose a static merge method?
@eiriktsarpalis, are we missing something here in STJ? What do you think we should do?
|
|
||
| foreach (var kvp in additionalMeta) | ||
| { | ||
| Meta[kvp.Key] = kvp.Value; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we put this logic into an internal shared method?
Also, do we want to do a deep merge? For example,
// Left
"_meta": {
"a": {
"b": {}
}
}
// Right
"_meta": {
"a": {
"c": {}
}
}
// Deep Merge
"_meta": {
"a": {
"b": {}
"c": {}
}
}With the current shallow merge, the right JsonObject would completely overwrite the left one. This might be the best behavior if we don't think it makes sense for multiple components to configure the same top-level _meta keys, but we should at least clarify this is a shallow merge in the doc comments.
|
It was curious to me that we only need this "merging" function on the high-level server-to-client request methods, and not on any of the client-to-server request methods. This exposes a peculiar difference between the high-level client methods and high-level server methods.
Is there a reason these are different? Wouldn't it be better to use a similar pattern for both client and server? |
|
I've noticed that too. The server methods don't exclusively take xxxRequestParams though. There are overloads of SampleAsync and ElicitAsync that construct the RequestParams object for you like the client methods do. The difference is that the server methods all have overloads that do take the RequestParams directly without forcing you to drop all the way down to SendRequestAsync. I think we should do the same for the client and ensure that each set of client request overloads (e.g. CallToolAsync) has an overload that takes the xxxRequestParams (e.g. CallToolRequestParams). This would basically give you the low-level control of calling SendRequestAsync directly, but with much better discoverability. |
|
@halter73 I don't think the server methods are the same as the client methods. As an example, the What I was actually hoping for was to get rid of the server request methods that accept a params. But for now this helps clarify the path forward for the options bag PR. I had added an options bag on the server methods and then got hung up needing these helper methods to merge the meta from the options bag into the request params. Seems like we don't need Meta or options on these since they are passing the full params, so I'll back out the changes to the server methods. If we add server methods that are equivalent to the client methods, we can include an options bag on those. |
|
Closing this as I think we won't need these helper methods, at least for now. |
Motivation and Context
This PR adds a helper method to the RequestParams, NotificationParams, and Result classes, all of which have a
Metaproperty, that will merge additional metadata into thatMetaproperty.This was motivated comments on #970 but is a standalone feature that can be done in its own PR.
How Has This Been Tested?
Will add tests.
Breaking Changes
Not breaking.
Types of changes
Checklist
Additional context