-
Notifications
You must be signed in to change notification settings - Fork 545
Fix regression matching templated McpServerResources #897
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -162,6 +162,14 @@ protected McpServerResource() | |
/// </remarks> | ||
public abstract IReadOnlyList<object> Metadata { get; } | ||
|
||
/// <summary> | ||
/// Evaluates whether the <paramref name="uri"/> matches the <see cref="ProtocolResourceTemplate"/> | ||
/// and can be used as the <see cref="ReadResourceRequestParams.Uri"/> passed to <see cref="ReadAsync"/>. | ||
/// </summary> | ||
/// <param name="uri">The URI being evaluated for this resource.</param> | ||
/// <returns><see langword="true"/> if the <paramref name="uri"/> matches the <see cref="ProtocolResourceTemplate"/>; otherwise, <see langword="false"/>.</returns> | ||
public abstract bool CanReadUri(string uri); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't like the name There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Matches is much better than CanRead for this, imo. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm fine with both. Others like HandlesUri would be fine, too. |
||
|
||
/// <summary> | ||
/// Gets the resource, rendering it with the provided request parameters and returning the resource result. | ||
/// </summary> | ||
|
@@ -174,12 +182,14 @@ protected McpServerResource() | |
/// </param> | ||
/// <returns> | ||
/// A <see cref="ValueTask{ReadResourceResult}"/> representing the asynchronous operation, containing a <see cref="ReadResourceResult"/> with | ||
/// the resource content and messages. If and only if this <see cref="McpServerResource"/> doesn't match the <see cref="ReadResourceRequestParams.Uri"/>, | ||
/// the method returns <see langword="null"/>. | ||
/// the resource content and messages. | ||
/// </returns> | ||
/// <exception cref="ArgumentNullException"><paramref name="request"/> is <see langword="null"/>.</exception> | ||
/// <exception cref="InvalidOperationException">The resource implementation returned <see langword="null"/> or an unsupported result type.</exception> | ||
public abstract ValueTask<ReadResourceResult?> ReadAsync( | ||
/// <exception cref="InvalidOperationException"> | ||
/// The <see cref="ReadResourceRequestParams.Uri"/> did not match the <see cref="ProtocolResourceTemplate"/> for this resource, | ||
/// the resource implementation returned <see langword="null"/>, or the resource implementation returned an unsupported result type. | ||
/// </exception> | ||
public abstract ValueTask<ReadResourceResult> ReadAsync( | ||
RequestContext<ReadResourceRequestParams> request, | ||
CancellationToken cancellationToken = default); | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
using Microsoft.Extensions.DependencyInjection; | ||
using ModelContextProtocol.Client; | ||
using ModelContextProtocol.Protocol; | ||
using ModelContextProtocol.Server; | ||
|
||
namespace ModelContextProtocol.Tests.Configuration; | ||
|
||
public sealed class McpServerResourceRoutingTests(ITestOutputHelper testOutputHelper) : ClientServerTestBase(testOutputHelper) | ||
{ | ||
protected override void ConfigureServices(ServiceCollection services, IMcpServerBuilder mcpServerBuilder) | ||
{ | ||
mcpServerBuilder.WithResources([ | ||
McpServerResource.Create(options: new() { UriTemplate = "test://resource/non-templated" } , method: () => "static"), | ||
McpServerResource.Create(options: new() { UriTemplate = "test://resource/{id}" }, method: (string id) => $"template: {id}"), | ||
McpServerResource.Create(options: new() { UriTemplate = "test://params{?a1,a2,a3}" }, method: (string a1, string a2, string a3) => $"params: {a1}, {a2}, {a3}"), | ||
]); | ||
} | ||
|
||
[Fact] | ||
public async Task MultipleTemplatedResources_MatchesCorrectResource() | ||
{ | ||
// Verify that when multiple templated resources exist, the correct one is matched based on the URI pattern, not just the first one. | ||
// Regression test for https://github.com/modelcontextprotocol/csharp-sdk/issues/821. | ||
await using McpClient client = await CreateMcpClientForServer(); | ||
|
||
var nonTemplatedResult = await client.ReadResourceAsync("test://resource/non-templated", TestContext.Current.CancellationToken); | ||
Assert.Equal("static", ((TextResourceContents)nonTemplatedResult.Contents[0]).Text); | ||
|
||
var templatedResult = await client.ReadResourceAsync("test://resource/12345", TestContext.Current.CancellationToken); | ||
Assert.Equal("template: 12345", ((TextResourceContents)templatedResult.Contents[0]).Text); | ||
|
||
var paramsResult = await client.ReadResourceAsync("test://params?a1=a&a2=b&a3=c", TestContext.Current.CancellationToken); | ||
Assert.Equal("params: a, b, c", ((TextResourceContents)paramsResult.Contents[0]).Text); | ||
|
||
var mcpEx = await Assert.ThrowsAsync<McpProtocolException>(async () => await client.ReadResourceAsync("test://params{?a1,a2,a3}", TestContext.Current.CancellationToken)); | ||
Assert.Equal(McpErrorCode.InvalidParams, mcpEx.ErrorCode); | ||
Assert.Equal("Request failed (remote): Unknown resource URI: 'test://params{?a1,a2,a3}'", mcpEx.Message); | ||
} | ||
} |
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.
This is going to create the Match object if it does match. It'd be better if the Can path used IsMatch instead