-
Notifications
You must be signed in to change notification settings - Fork 619
Add McpTransportContext to McpSyncClient #522
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?
Add McpTransportContext to McpSyncClient #522
Conversation
15bb3f9
to
703d363
Compare
703d363
to
4f7a937
Compare
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.
Great progress! Left some comments after our sync discussion :)
|
||
import io.modelcontextprotocol.server.McpTransportContext; |
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.
Let's move McpTransportContext
up a level into a new common
package. It will be a breaking change, but it's a super recent type.
@@ -183,6 +185,8 @@ class SyncSpec { | |||
|
|||
private Function<ElicitRequest, ElicitResult> elicitationHandler; | |||
|
|||
private Supplier<McpTransportContext> contextProvider = McpTransportContext.EMPTY::copy; |
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 don't need to copy here. We should ensure that the copy is made whenever there is an intention for the user to modify the context, e.g. when creating an empty one that is being filled in and used as the provided result.
A couple of ideas:
- Hide
DefaultMcpTransportContext
(currently it's public, let's make it package private) - Add factory methods to
McpTransportContext
:empty()
which returns the immutable instance; andcreate()
which creates an empty but mutable container.
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.
Another note on the McpTransportContext
:
We use it already on the server side, where the extractor expects a mutable instance that it can fill in.
With the client we have the opposite where the provider creates an instance and expects to mutate it.
On the read side, however we have the user handlers in server specification that are only supposed to read contents and we should ensure that mutations are not allowed because there is no use of mutating the contents.
On the client end, the customizer is the reader also so it should be disallowed to write something to the context.
Therefore, perhaps let's break the API a little bit:
McpTransportContext#put
is removed! TheMcpTransportContext
is just a container of immutable contextMcpTransportExtractor
- deprecateMcpTransportContext extract(T request, McpTransportContext transportContext)
or even remove it (BREAKING) and just haveMcpTransportContext extract(T request)
that allows to callMcpTransportContext.create(Map)
which would accept aMap
that is already populated and now theMcpTransportContext
is only the view.- Client's transport context provider is also creating an instance like the extractor.
- Customizer is just supposed to read values using
get(K)
.
* Streamable HTTP transport. | ||
* Streamable HTTP transport. Do not rely on thread-locals in this implementation, instead | ||
* use {@link SyncSpec#transportContextProvider} to extract context, and then consume it | ||
* through {@link McpTransportContext}. | ||
* | ||
* @author Daniel Garnier-Moiroux | ||
*/ | ||
public interface McpSyncHttpRequestCustomizer { |
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.
Why not rename it to McpSyncHttpClientRequestCustomizer
to reflect that it is tied to JDK's HttpClient
.
* @param contextProvider A supplier to create a context | ||
* @return This builder for method chaining | ||
*/ | ||
public SyncSpec transportContextProvider(Supplier<McpTransportContext> contextProvider) { |
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.
Let's perhaps add a comment so users are not too confused by the lack of this method on the AsyncSpec
to clarify that the means to achieve the same is to simply append a contextWrite(McpTransportContext.KEY, context)
to any McpAsyncClient
API call.
@@ -177,36 +185,43 @@ public boolean closeGracefully() { | |||
public McpSchema.InitializeResult initialize() { | |||
// TODO: block takes no argument here as we assume the async client is | |||
// configured with a requestTimeout at all times | |||
return this.delegate.initialize().block(); | |||
var context = this.contextProvider.get(); | |||
return this.delegate.initialize().contextWrite(ctx -> ctx.put(McpTransportContext.KEY, context)).block(); |
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.
How about adding a method like Mono<?> withProvidedContext(...)
that wraps all the calls and attaches .contextWrite(ctx -> ctx.put(McpTransportContext.KEY, this.contextProvider.get())
@@ -247,7 +250,9 @@ private Mono<Disposable> reconnect(McpTransportStream<Disposable> stream) { | |||
.header("Cache-Control", "no-cache") | |||
.header(HttpHeaders.PROTOCOL_VERSION, MCP_PROTOCOL_VERSION) | |||
.GET(); | |||
return Mono.from(this.httpRequestCustomizer.customize(builder, "GET", uri, null)); | |||
var transportContext = ctx.getOrDefault(McpTransportContext.KEY, McpTransportContext.EMPTY); |
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.
Let's perhaps use the innermost Context
to be super certain that if at any point we alter the inner chain we do still grab the correct reference here. Replacing Mono.defer
on line 236 with Mono.deferContextual(connectionCtx -> {
should do the trick.
mcpSyncClient -> { | ||
mcpSyncClient.initialize(); | ||
|
||
var transportCaptor = ArgumentCaptor.forClass(McpTransportContext.class); |
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.
Hmm, perhaps we can add equality overrides to DefaultMcpTransportContext
? :)
4f7a937
to
477fbd6
Compare
- McpSyncClient should be considered thread-agnostic, and therefore consumers cannot rely on thread locals to propagate "context", e.g. pass down the Servlet request reference in a server context. - This PR introduces a mechanism for populating an McpTransportContext before executing client operations, and reworks the HTTP request customizers to leverage that McpTransportContext. - This introduces a breaking change to the Sync/Async request customizers. Signed-off-by: Daniel Garnier-Moiroux <[email protected]>
Signed-off-by: Daniel Garnier-Moiroux <[email protected]>
Signed-off-by: Daniel Garnier-Moiroux <[email protected]>
477fbd6
to
98eb323
Compare
This PR adds
McpTransportContext
to MCP Clients, and makes it available to(Async|Sync)HttpRequestCustomizer
s.Motivation and Context
The motivation is twofold:
HttpRequestCustomizer
, or through the reactor context in WebClient-based transports.How Has This Been Tested?
Test with Spring AI + Spring Security for passing JWT tokens to MCP Sync clients.
Breaking Changes
Following the move of
(Async|Sync)HttpRequestCustomizer
toMcp(Async|Sync)HttpRequestCustomizer
, itself a breaking change, I also changed the signatures of those classes.Types of changes
Checklist