Resources-enhancements #257

Tyler-R-Kendrick · 2025-04-09T21:29:25Z

Added DI builder enhancements to resources to emulate registration of other MCP list primitives (like tools and prompts).

Motivation and Context

Enhances the api to provide a more consistent way to add resources.

How Has This Been Tested?

I wrote additional tests to cover the cases.

Breaking Changes

Yes. ReadResourceRequest marked its property as nullable instead of required. This was a bug because the spec specified it as not optional. That is changed in the PR.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Documentation update

Checklist

I have read the MCP Documentation
My code follows the repository's style guidelines
New and existing tests pass locally
I have added appropriate error handling
I have added or updated documentation as needed

Additional context

Simplified registration of other MCP primitives for future commits.

…-sdk into resources-enhancements

Tyler-R-Kendrick · 2025-04-10T14:33:02Z

The failing test appears to indicate that it will sometimes fail accidentally. I don't appear to be able to requeue the tests for evaluation. @PederHP or @stephentoub, would you be able to look into this?

The test is in ClientIntegrationTests on line 312.
It reads:

        // wait a bit to validate we don't receive another. this is best effort only;
        // false negatives are possible.
        await Assert.ThrowsAsync<TimeoutException>(() => receivedNotification.Task.WaitAsync(TimeSpan.FromSeconds(1), TestContext.Current.CancellationToken));

I would expect that this test should be broadcasting the notifications it expects to intercept instead of random sampling leading to indeterminate output.

stephentoub · 2025-04-10T14:37:36Z

That's #228

Tyler-R-Kendrick · 2025-04-10T18:40:35Z

This is for addressing #72. I've implemented a portion of the issue that can be extended to support the entire feature request.
I believe this implementation minimizes the necessary public types exposed through the API.

alainkaiser · 2025-04-16T08:38:30Z

Nice stuff. Will be a lot simpler to work with Resources like that!

stephentoub · 2025-04-16T17:10:19Z

src/ModelContextProtocol/Configuration/McpServerBuilderExtensions.cs

+    /// <returns>The <see cref="IMcpServerBuilder"/> instance.</returns>
+    public static IMcpServerBuilder WithResource(
+        this IMcpServerBuilder builder,
+        IFileInfo fileInfo)


Isn't it critically important to have a MIME type for a resource? Otherwise the client doesn't know how to interpret the bytes. And IFileInfo doesn't provide any information about the media type (for a limited set of known file extensions we might be able to infer it, but it would require a hardcoded list and would likely fall off a cliff).

stephentoub · 2025-04-16T17:11:28Z

src/ModelContextProtocol/Configuration/McpServerBuilderExtensions.cs

+    /// <param name="builder">The builder instance.</param>
+    /// <param name="fileInfo">The file info of the resource.</param>
+    /// <returns>The <see cref="IMcpServerBuilder"/> instance.</returns>
+    public static IMcpServerBuilder WithResource(


We don't have singular versions for tools or prompts. I don't think we should start having singular versions for resources, either. We can later add singular versions for all of them if it proves worthwhile, but with collection expressions, you can wrap any instance with [...] and have it work with the plural versions.

stephentoub · 2025-04-16T17:13:56Z

src/ModelContextProtocol/Configuration/McpServerBuilderExtensions.cs

+    /// <returns>The <see cref="IMcpServerBuilder"/> instance.</returns>
+    public static IMcpServerBuilder WithResources(
+        this IMcpServerBuilder builder,
+        params IEnumerable<IFileInfo> fileInfos)


I don't think we should have such IFileInfo overloads, both for the mime type issue, but also because it's not clear exactly when or how we'd be reading the data from the file. Would every request require re-reading the data? If not, what is the caching policy? Would we be required to subscribe to file change notifications in order to raise resource changed notifications? Maybe at some point we'd want to sign up for all that, but I'd prefer if we do, for it to be via creating McpServerResource instances, with Create methods that are configurable with various details.

stephentoub · 2025-04-16T17:21:14Z

src/ModelContextProtocol/Protocol/Types/ToolsCapability.cs

@@ -61,4 +61,9 @@ public class ToolsCapability
    /// </remarks>
    [JsonIgnore]
    public McpServerPrimitiveCollection<McpServerTool>? ToolCollection { get; set; }
+    McpServerPrimitiveCollection<McpServerTool>? IListCapability<McpServerTool>.Collection


Suggested change

McpServerPrimitiveCollection<McpServerTool>? IListCapability<McpServerTool>.Collection

McpServerPrimitiveCollection<McpServerTool>? IListCapability<McpServerTool>.Collection

stephentoub · 2025-04-16T17:24:19Z

src/ModelContextProtocol/Server/McpServer.cs

@@ -64,32 +62,16 @@ public McpServer(ITransport transport, McpServerOptions options, ILoggerFactory?
        SetCompletionHandler(options);
        SetPingHandler();

+        var capabilities = options.Capabilities;


Suggested change

var capabilities = options.Capabilities;

var capabilities = options.Capabilities;

stephentoub · 2025-04-16T17:33:00Z

src/ModelContextProtocol/Server/McpServer.cs

+        var isMissingListResourceHandlers = originalListResourcesHandler is not { } && listResourceTemplatesHandler is not { };
+        if (resourceCollection is not { IsEmpty: false } && (isMissingListResourceHandlers || readResourceHandler is not { }))
+        {
+            throw new McpException("Resources capability was enabled, but ListResources, ListResourceTemplates, and/or ReadResource handlers were not specified.");
+        }


I don't understand this logic. Why is "is missing" true if both originalListResourcesHandler and listResourceTemplatesHandler are non-null?

stephentoub · 2025-04-16T17:34:35Z

src/ModelContextProtocol/Server/McpServer.cs

+        // https://modelcontextprotocol.io/specification/2024-11-05/server/tools#capabilities
+        // Look to spec for guidance on ListChanged over collection existance.


stephentoub · 2025-04-16T17:35:30Z

src/ModelContextProtocol/Server/McpServer.cs

+        // https://modelcontextprotocol.io/specification/2024-11-05/server/tools#capabilities
+        // Look to spec for guidance on ListChanged over collection existance.
+        if (capability?.Collection is { } collection)
+            //&& capability.ListChanged is true)


stephentoub · 2025-04-16T17:37:03Z

src/ModelContextProtocol/Utils/Disposable.cs

+/// </remarks>
+/// <param name="disposeAction">The action to execute when this object is disposed.</param>
+/// <exception cref="ArgumentNullException">Thrown when <paramref name="disposeAction"/> is null.</exception>
+public sealed class Disposable(Action disposeAction) : IDisposable


This is not needed, and it certainly doesn't need to be public.

stephentoub · 2025-04-16T17:37:16Z

src/ModelContextProtocol/Server/McpServer.cs

@@ -32,6 +29,7 @@ internal sealed class McpServer : McpEndpoint, IMcpServer
    /// rather than a nullable to be able to manipulate it atomically.
    /// </remarks>
    private StrongBox<LoggingLevel>? _loggingLevel;
+    private readonly List<Disposable> _disposables = [];


This can just be List<Action>

stephentoub · 2025-04-16T17:37:37Z

src/ModelContextProtocol/Server/McpServer.cs

+            void ChangedDelegate(object? sender, EventArgs e)
+                => _ = this.SendNotificationAsync(methodName);
+            collection.Changed += ChangedDelegate;
+            _disposables.Add(new(() => collection.Changed -= ChangedDelegate));


Suggested change

_disposables.Add(new(() => collection.Changed -= ChangedDelegate));

_disposables.Add(() => collection.Changed -= ChangedDelegate);

stephentoub · 2025-04-16T17:38:31Z

src/ModelContextProtocol/Server/McpServerResource.cs

+/// <summary>
+/// Represents a resource that the server supports.
+/// </summary>
+public class McpServerResource : IMcpServerPrimitive


McpServerTool and McpServerPrompt are both abstract, with abstract members. This should be the same.

stephentoub · 2025-04-16T17:39:14Z

src/ModelContextProtocol/Server/McpServerResource.cs

+
+    /// <inheritdoc />
+    public string Name => ProtocolResource.Name;
+}


This should have members for being invoked and returning the relevant data, as is the case for McpServerPrompt/Tool.

Tyler-R-Kendrick added 6 commits April 9, 2025 20:38

added mcp server resource to mimic tool registration.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

GPG key ID: B5690EEEBB952194

Verified
Learn about vigilant mode

a918739

simplified event unsubscription in primitive registration

4beb291

Added tests

Loading
Loading status checks…

144413b

Merge branch 'main' of https://github.com/Tyler-R-Kendrick/mcp-csharp…

0179cfe

…-sdk into resources-enhancements

fixed tests for resource list changes.

Loading
Loading status checks…

ada39ad

Merge branch 'main' of https://github.com/Tyler-R-Kendrick/mcp-csharp…

Loading
Loading status checks…

c6989de

…-sdk into resources-enhancements

Tyler-R-Kendrick marked this pull request as ready for review April 10, 2025 14:28

stephentoub reviewed Apr 16, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resources-enhancements #257

Resources-enhancements #257

Tyler-R-Kendrick commented Apr 9, 2025 •

edited

Loading

Tyler-R-Kendrick commented Apr 10, 2025

stephentoub commented Apr 10, 2025

Tyler-R-Kendrick commented Apr 10, 2025 •

edited

Loading

alainkaiser commented Apr 16, 2025

stephentoub Apr 16, 2025

stephentoub Apr 16, 2025

stephentoub Apr 16, 2025

stephentoub Apr 16, 2025

stephentoub Apr 16, 2025

stephentoub Apr 16, 2025

stephentoub Apr 16, 2025

stephentoub Apr 16, 2025

stephentoub Apr 16, 2025

stephentoub Apr 16, 2025

stephentoub Apr 16, 2025

stephentoub Apr 16, 2025

stephentoub Apr 16, 2025

	McpServerPrimitiveCollection<McpServerTool>? IListCapability<McpServerTool>.Collection

	McpServerPrimitiveCollection<McpServerTool>? IListCapability<McpServerTool>.Collection

	var capabilities = options.Capabilities;
	var capabilities = options.Capabilities;

		// https://modelcontextprotocol.io/specification/2024-11-05/server/tools#capabilities
		// Look to spec for guidance on ListChanged over collection existance.

	_disposables.Add(new(() => collection.Changed -= ChangedDelegate));
	_disposables.Add(() => collection.Changed -= ChangedDelegate);

Resources-enhancements #257

Are you sure you want to change the base?

Resources-enhancements #257

Conversation

Tyler-R-Kendrick commented Apr 9, 2025 • edited Loading

Motivation and Context

How Has This Been Tested?

Breaking Changes

Types of changes

Checklist

Additional context

Tyler-R-Kendrick commented Apr 10, 2025

stephentoub commented Apr 10, 2025

Tyler-R-Kendrick commented Apr 10, 2025 • edited Loading

alainkaiser commented Apr 16, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tyler-R-Kendrick commented Apr 9, 2025 •

edited

Loading

Tyler-R-Kendrick commented Apr 10, 2025 •

edited

Loading