-
Notifications
You must be signed in to change notification settings - Fork 495
Per RouteValue Tools Sample #724
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?
Conversation
Co-authored-by: PederHP <[email protected]>
Co-authored-by: PederHP <[email protected]>
Co-authored-by: PederHP <[email protected]>
…b-c4b4779d276e Add ASP.NET Core MCP server sample showcasing per-user tool filtering
…s with route-based filtering Co-authored-by: PederHP <[email protected]>
Co-authored-by: PederHP <[email protected]>
…a-1d92ba914d7b Rename AspNetCoreMcpServerPerUserTools to AspNetCoreMcpPerSessionTools with route-based filtering
I've updated the sample as per @halter73 's suggestion to use route as the criteria instead of a header - to avoid any confusion with the mechanisms provided by the upcoming authorization convenience. The sample should show how to use arbitrary criteria to modify the Tools collection in the ConfigureSessionOptions delegate. |
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 looks great!
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.
Alternatively, we could filter out the tools, but starting from an empty list does seem simpler. If we wanted to improve this further, we could have a ConcurrentDictionary<string, McpServerTool[]>
to look up the tool types more efficiently so we don't have to do a bunch of reflection at the start of each session.
Applied your suggested changes to start from an empty list without clearing, but hadn't noticed that this means the capability isn't initialized when configuring the session options. To make the sample reasonably simply, I added this back rather than manually configure the capability. I could change it to add a basic tool that's always there, but that might add more confusion than benefit. |
I think the better bet is to manually initialize the capabilities. That way, the ConfigureSessionOptions callback doesn't need to null-check it either making it slightly shorter while also getting rid of all the // Configure per-session options to filter tools based on route category
options.ConfigureSessionOptions = async (httpContext, mcpOptions, cancellationToken) =>
{
var toolCategory = httpContext.Request.RouteValues["toolCategory"]?.ToString()?.ToLower() ?? "all";
// Get pre-populated tools for the requested category
if (toolDictionary.TryGetValue(toolCategory, out var tools))
{
mcpOptions.Capabilities = new();
mcpOptions.Capabilities.Tools = new();
var toolCollection = mcpOptions.Capabilities.Tools.ToolCollection = new();
foreach (var tool in tools)
{
toolCollection.Add(tool);
}
}
}; @jozkee Is looking at moving where we put the [JsonIgnore]'d properties on the capabilities types like the collections and handlers so they aren't so deeply nested which could make this look even nicer. Another thing that might help tighten this up is just removing the OpenTelemetry dependencies from this sample. I think it's enough to leave that in the AspNetCoreMcpServer sample and keep this one a little more focused on the per-session options. |
Had Copilot help create a sample of how to use ConfigureSessionOptions to make tools dynamic and client/user specific. See #714.