-
Notifications
You must be signed in to change notification settings - Fork 701
Feature: McpTransportContext for HttpServletSseServerTransportProvider #477
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
Conversation
…rovider and HttpServletStreamableServerTransportProvider
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.
Thanks, I think this is useful and seems you found a way around the singleton behaviour of the exchange object :) Left some comments to improve a few things, but I agree we should merge this once fixed. Also, would you consider following up with the Spring-related SSE transport providers to maintain feature parity?
...ain/java/io/modelcontextprotocol/server/transport/HttpServletSseServerTransportProvider.java
Outdated
Show resolved
Hide resolved
...ain/java/io/modelcontextprotocol/server/transport/HttpServletSseServerTransportProvider.java
Outdated
Show resolved
Hide resolved
...ain/java/io/modelcontextprotocol/server/transport/HttpServletSseServerTransportProvider.java
Outdated
Show resolved
Hide resolved
mcp/src/test/java/io/modelcontextprotocol/server/AbstractMcpClientServerIntegrationTests.java
Outdated
Show resolved
Hide resolved
mcp/src/test/java/io/modelcontextprotocol/server/AbstractMcpClientServerIntegrationTests.java
Outdated
Show resolved
Hide resolved
@chemicL thanks for the quick review! All your suggestions make sense, I'll get those updates made. RE: Spring support, I've never used Spring but I can take a pass at it, probably this weekend. If I hit some snags would you be open to merging the Servlet version in first? I just don't want to stay on our internal fork for too long as additional capabilities and bug fixes get merged in. |
…; add notNull assertions
…ch request in handleIncomingRequest and handleIncomingNotification
…ody to peform assertions within JUnit test thread; since callHandler may run on a different thread
…ientServerIntegrationTests.java Co-authored-by: Dariusz Jędrzejczyk <[email protected]>
@chemicL thanks again for the review! Updates
Testing
Spring WebMvc and WebFluxI took a quick look through the code, it all looks pretty similar so I should be able to follow up in a subsequent PR adding |
// This legacy implementation assumes an exchange is established upon | ||
// the initialization phase see: exchangeSink.tryEmitValue(...), | ||
// which creates a cached immutable exchange. | ||
// Here, we create a new exchange and copy over everything from that | ||
// cached exchange, and use it for a single HTTP request, with the | ||
// transport context passed in. | ||
McpAsyncServerExchange newExchange = new McpAsyncServerExchange(exchange.sessionId(), this, | ||
exchange.getClientCapabilities(), exchange.getClientInfo(), transportContext); |
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 can be extracted to a utility method to avoid the copy of the comment.
HttpResponse<String> response = HttpClient.newHttpClient() | ||
.send(HttpRequest.newBuilder() | ||
.uri(URI.create( | ||
"https://raw.githubusercontent.com/modelcontextprotocol/java-sdk/refs/heads/main/README.md")) |
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.
I suppose there is no need to call an external API in this test. I'd say we don't need to call anything, just return the contents of the context from the tool.
protected static McpTransportContextExtractor<HttpServletRequest> TEST_CONTEXT_EXTRACTOR = (r, tc) -> { | ||
tc.put("important", "value"); | ||
return tc; | ||
}; |
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.
We could have a copy of this in both HttpServletSseIntegrationTests
and HttpServletStreamableIntegrationTests
to avoid a reference to a servlet type in the base class.
BTW, I thought this abstract test can be used outside of the servlet context, but it seems the variant in mcp-test module is not really a copy.. @tzolov do we have a strategy for aligning these? There's no comment like we have in other files of this sort (// KEEP IN SYNC with the class in mcp-test module
)...
- Add McpTransportContextExtractor to WebFlux/WebMVC SSE and Streamable transport providers - Enable extraction of HTTP transport metadata (headers, etc.) for use during request processing - Pass transport context through reactive context chain using McpTransportContext.KEY - Add contextExtractor() builder methods for configuring custom extractors - Update HttpServlet transport providers with same context extraction capability - Modify McpServerSession to properly propagate transport context to handlers - Add test coverage with TEST_CONTEXT_EXTRACTOR in integration tests This allows MCP feature implementations to access HTTP transport level metadata that was present at request time, enabling use cases like authentication, request tracing, and custom header processing. Signed-off-by: Christian Tzolov <[email protected]> Co-authored-by: Christian Tzolov <[email protected]> Co-authored-by: Dariusz Jędrzejczyk <[email protected]>
- Add McpTransportContextExtractor to WebFlux/WebMVC SSE and Streamable transport providers - Enable extraction of HTTP transport metadata (headers, etc.) for use during request processing - Pass transport context through reactive context chain using McpTransportContext.KEY - Add contextExtractor() builder methods for configuring custom extractors - Update HttpServlet transport providers with same context extraction capability - Modify McpServerSession to properly propagate transport context to handlers - Add test coverage with TEST_CONTEXT_EXTRACTOR in integration tests This allows MCP feature implementations to access HTTP transport level metadata that was present at request time, enabling use cases like authentication, request tracing, and custom header processing. Signed-off-by: Christian Tzolov <[email protected]> Co-authored-by: Christian Tzolov <[email protected]> Co-authored-by: Dariusz Jędrzejczyk <[email protected]>
rebased, extended, squashed and merged at b4fef52 Additionally:
|
Implements #476.
Motivation and Context
#420 introduced McpTransportContext.
McpTransportContext
should be supported inHttpServletSseServerTransportProvider
to enable those who are still on that transport to leverageMcpTransportContext
to set attributes that Tools, etc. have access to. This is crucial for implementing Authentication & Authorization at the Tool level.Current Behavior
McpTransportContext
is absent fromHttpServletSseServerTransportProvider
Context
How has this issue affected you?
We're not able to switch to
HttpServletStreamableServerTransportProvider
yet, but wish to useMcpTransportContext
instead of our custom built similar capability.What are you trying to accomplish?
Authn/z at Tool level.
How Has This Been Tested?
HttpServletSseServerTransportProvider
HttpServletSseServerTransportProvider
Breaking Changes
No, use of the
McpTransportContext
is optional.Types of changes
Checklist
Additional context
None