Add StreamableHttpHandler and WithHttpTransport #291
Conversation
Yes, please. |
…tOptions - Add proper AdditionalHeaders support
Done. |
| @@ -24,136 +12,20 @@ public static class McpEndpointRouteBuilderExtensions | |||
| { | |||
| /// <summary> | |||
| /// Sets up endpoints for handling MCP HTTP Streaming transport. | |||
There was a problem hiding this comment.
And we don't want it to automatically do WithHttpTransport? Could/should it do it but then if a user calls it themselves it overrides whatever default MapMcp would have used?
There was a problem hiding this comment.
It's typical for things that need to configure routes like Mvc and SignalR to require that you call two methods: one to configure service, and then another to add the routes. For MVC, it's AddMvc and MapControllers/MapControllerRoutes. For SignalR, it's AddSignalR and MapHub. Even for minimal APIs, it's AddRouting, UseRouting, and MapGet/Post, but AddRouting and UseRouting get added explicitly for WebApplicationBuilder.
This is why I was so hesitant to add WithHttpTransport. The MapMcp call is the one thing we cannot easily get rid of. We could get rid of WithHttpTransport if we newed up the StreamableHttpHandler and any other services we need when they were missing, but it would be inconvenient for us as MapMcp implementers. And it is very conventional to need to call both an "Add" and a "Map" method, even if I'm personally not a fan of requiring the extra code.
| else | ||
| { | ||
| context.Response.StatusCode = StatusCodes.Status405MethodNotAllowed; | ||
| await context.Response.WriteAsync("Method Not Allowed"); |
There was a problem hiding this comment.
Is this common practice, to write the status description into the response body as well?
There was a problem hiding this comment.
No. Copilot did this, and I hit tab 😄 If this were routing producing the response, it'd be an empty. Fortunately, it's not too big of a deal, and as of now, this part is unreachable. I'll probably switch this to throw as a sort of assert later.
| context.Features.GetRequiredFeature<IHttpResponseBodyFeature>().DisableBuffering(); | ||
|
|
||
| var sessionId = MakeNewSessionId(); | ||
| await using var transport = new SseResponseStreamTransport(response.Body, $"message?sessionId={sessionId}"); |
There was a problem hiding this comment.
Were you saying something about wanting to data protect session IDs?
There was a problem hiding this comment.
I think I still might have to do this for stateless requests similar to what we do for antiforgery, since we won't be able to rely on the ConcurrentDictionary to track if things like the user ID changed if the client sends requests to multiple servers, but I haven't got there yet.
src/ModelContextProtocol/Protocol/Transport/SseClientSessionTransport.cs
Show resolved
Hide resolved
|
I'm going to merge your change in order to rebase my logging change on top of it. My comments are all minor and can be addressed subsequently. |
This is still not an implementation of the "Streamable HTTP" transport, but it gets us a little bit closer.
This is a breaking change that now requires WithHttpTransport() to be called for MapMcp to work. This change also adds testing for IHttpContextAcessor and ensure that requests from different users for the same session are rejected. It also cleans up a few test namespaces like removing ModelContextProtocol.Test.Utils in favor of Tests.Utils.
I
plan to removealso removed SseClientTransportOptions.MaxReconnectAttempts and ReconnectDelay. I don't think we want to be in the business of handling retry logic. That's better left to something like Polly.