-
Notifications
You must be signed in to change notification settings - Fork 287
Add McpServer/ClientResource{Template} and friends #391
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
Add McpServer/ClientResource{Template} and friends #391
Conversation
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.
I didn't expect that we'd allow properties for non-templated resources, but I suppose that makes sense given that non-templated resources have their own collection.
I was wondering if for the high-level API we'd treat non-templated resources as a special case of a templated resource with no parameters similar to route patterns in ASP.NET Core. This would mean combining both the attributes, the WithResources methods, collections, etc...
I think this may help discoverability and it would definitely reduce a lot of duplication and probably code size. You'd lose some compile time safety of course. For example, you could no longer allow resource properties (instead of methods) only for non-templated resources, but I think it could still be worth it. ASP.NET Core APIs would be a lot uglier if we had to duplicate everything for templated and non-templated routes, so giving up on some possible built time checks is worth it.
I know resources vs templated resources have separate listing methods, but they also have a lot of similarities including both being read using the "resources/read" method. I think if it were me, I'd merge the concepts, but it's definitely not necessary.
I know @mikekistler updated the EverythingServer sample to include templated resources. It might be interesting to use the new WithResources and WithTemplatedResources methods there. It already uses WithTools and WithPrompts, so it'd be more consistent.
I did skim over the regex code, and I'm not going to claim to have reviewed every line with a fine-toothed comb, but it LGTM.
tests/ModelContextProtocol.Tests/Server/McpServerResourceTemplateTests.cs
Outdated
Show resolved
Hide resolved
This PR copies the same design used by tools and prompts to add McpServerResource, McpServerResourceAttribute, McpServerResourceTypeAttribute, McpClientResource, and then all of those for ResourceTemplate as well. Unlike the others, [McpServerResource] is permitted not only on methods but also on properties. This convenience is afforded because such resources aren't parameterized, and thus can be easily modeled as properties, likely auto-props, e.g. ```C# [McpServerResource] public static BlobResourceContents MyAmazingData { get; } = Create(); ``` Details about resources can be encoded into the attributes or provided via McpServerResource{Template}.Create methods. If not URI is explicitly defined, one is derived from the member name and arguments. For templates, this means manufacturing a URI template that includes variables for all non-DI parameters. This also fixes a few issues discovered along the way: - McpClientExtensions are updated to use `ValueTask<T>` instead of `Task<T>` where relevant. - McpServerPrompt was missing handling for progress notifications; that's been rectified. It was also missing configuring the description on the ProtocolPrompt. - Renamed IMcpServerPrimitive.Name to Id. For resources, names aren't unique. - After going back and forth, updated McpServer to not fail for missing handlers and to instead register default handlers. It simplifies the implementation and makes it more consistent. - Added a bunch of primitive types into the McpJsonUtilities JSON source generator context so that such primitives are usable with tools/prompts/resources without folks being required to add them into their own context for Native AOT. I think we should consider pushing this down to AIJsonUtilities, instead, and that will then flow up to McpJsonUtilities automatically.
This PR copies the same design used by tools and prompts to add McpServerResource, McpServerResourceAttribute, McpServerResourceTypeAttribute, McpClientResource, and then all of those for ResourceTemplate as well.
Unlike the others, [McpServerResource] is permitted not only on methods but also on properties. This convenience is afforded because such resources aren't parameterized, and thus can be easily modeled as properties, likely auto-props, e.g.
Details about resources can be encoded into the attributes or provided via McpServerResource{Template}.Create methods. If not URI is explicitly defined, one is derived from the member name and arguments. For templates, this means manufacturing a URI template that includes variables for all non-DI parameters.
This also fixes a few issues discovered along the way:
ValueTask<T>
instead ofTask<T>
where relevant.Closes #72
Closes #74