Use lowercase host in resource URI template derivation. #530
Conversation
src/ModelContextProtocol.Core/Server/AIFunctionMcpServerResource.cs
Outdated
Show resolved
Hide resolved
|
|
||
| /// <summary> | ||
| /// Initializes a new instance of the <see cref="McpServerPrimitiveCollection{T}"/> class. | ||
| /// </summary> | ||
| public McpServerPrimitiveCollection() | ||
| { | ||
| _primitives = typeof(T) == typeof(McpServerResource) |
There was a problem hiding this comment.
@stephentoub I did this to address uri lookups. Let me know if this approach passes your smell test.
| /// | ||
| /// We do this because non-templated resources are looked up directly from the resource dictionary | ||
| /// and we need to make sure equality is implemented correctly. Templated Uris are resolved using | ||
| /// linear traversal so there's no need for equality comparison to be fully accurate. |
There was a problem hiding this comment.
I'm not understanding how this statement relates to using this comparer with the ConcurrentDictionary in the collection, as that's not a linear search... ?
There was a problem hiding this comment.
It relates to this fallback, which is where any URI derived via template expansion is meant to be found:
csharp-sdk/src/ModelContextProtocol.Core/Server/McpServer.cs
Lines 293 to 302 in 283a052
| } | ||
| else | ||
| { | ||
| return StringComparer.Ordinal.GetHashCode(uriTemplate); |
There was a problem hiding this comment.
Doesn't this mean that two equal templates could end up with different hashcodes? I guess I'm not understanding the loose comparison rules being employed and why they're ok.
There was a problem hiding this comment.
If you mean uri://host/{Id} not being equal to uri://Host/{Id} it's something I've done deliberately to avoid getting lost in the weeds of defining URI template equality. I believe this is ok because
- Inbound resource requests ought to be non-templated URIs and should therefore always not be equal to templated keys and
- The downside of letting servers still declare duplicate templates up to case insensitivity is relatively minor and pre-existing.
…sure built-in resources work with the VS Code client.
Makes the following changes:
This should mitigate some of the issues described in #516.