-
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
Changes from all commits
789d875
7d5fcb3
04b562d
f2efd30
991aa4a
a8de52a
03057e4
f88a697
00b092d
527643e
add21cd
a5786c3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -198,7 +198,9 @@ public Mono<Void> sendNotification(String method, Object params) { | |
* @return a Mono that completes when the message is processed | ||
*/ | ||
public Mono<Void> handle(McpSchema.JSONRPCMessage message) { | ||
return Mono.defer(() -> { | ||
return Mono.deferContextual(ctx -> { | ||
McpTransportContext transportContext = ctx.getOrDefault(McpTransportContext.KEY, McpTransportContext.EMPTY); | ||
|
||
// TODO handle errors for communication to without initialization happening | ||
// first | ||
if (message instanceof McpSchema.JSONRPCResponse response) { | ||
|
@@ -214,7 +216,7 @@ public Mono<Void> handle(McpSchema.JSONRPCMessage message) { | |
} | ||
else if (message instanceof McpSchema.JSONRPCRequest request) { | ||
logger.debug("Received request: {}", request); | ||
return handleIncomingRequest(request).onErrorResume(error -> { | ||
return handleIncomingRequest(request, transportContext).onErrorResume(error -> { | ||
var errorResponse = new McpSchema.JSONRPCResponse(McpSchema.JSONRPC_VERSION, request.id(), null, | ||
new McpSchema.JSONRPCResponse.JSONRPCError(McpSchema.ErrorCodes.INTERNAL_ERROR, | ||
error.getMessage(), null)); | ||
|
@@ -227,7 +229,7 @@ else if (message instanceof McpSchema.JSONRPCNotification notification) { | |
// happening first | ||
logger.debug("Received notification: {}", notification); | ||
// TODO: in case of error, should the POST request be signalled? | ||
return handleIncomingNotification(notification) | ||
return handleIncomingNotification(notification, transportContext) | ||
.doOnError(error -> logger.error("Error handling notification: {}", error.getMessage())); | ||
} | ||
else { | ||
|
@@ -240,9 +242,11 @@ else if (message instanceof McpSchema.JSONRPCNotification notification) { | |
/** | ||
* Handles an incoming JSON-RPC request by routing it to the appropriate handler. | ||
* @param request The incoming JSON-RPC request | ||
* @param transportContext | ||
* @return A Mono containing the JSON-RPC response | ||
*/ | ||
private Mono<McpSchema.JSONRPCResponse> handleIncomingRequest(McpSchema.JSONRPCRequest request) { | ||
private Mono<McpSchema.JSONRPCResponse> handleIncomingRequest(McpSchema.JSONRPCRequest request, | ||
McpTransportContext transportContext) { | ||
return Mono.defer(() -> { | ||
Mono<?> resultMono; | ||
if (McpSchema.METHOD_INITIALIZE.equals(request.method())) { | ||
|
@@ -266,7 +270,17 @@ private Mono<McpSchema.JSONRPCResponse> handleIncomingRequest(McpSchema.JSONRPCR | |
error.message(), error.data()))); | ||
} | ||
|
||
resultMono = this.exchangeSink.asMono().flatMap(exchange -> handler.handle(exchange, request.params())); | ||
resultMono = this.exchangeSink.asMono().flatMap(exchange -> { | ||
// 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); | ||
return handler.handle(newExchange, request.params()); | ||
}); | ||
} | ||
return resultMono | ||
.map(result -> new McpSchema.JSONRPCResponse(McpSchema.JSONRPC_VERSION, request.id(), result, null)) | ||
|
@@ -280,24 +294,36 @@ private Mono<McpSchema.JSONRPCResponse> handleIncomingRequest(McpSchema.JSONRPCR | |
/** | ||
* Handles an incoming JSON-RPC notification by routing it to the appropriate handler. | ||
* @param notification The incoming JSON-RPC notification | ||
* @param transportContext | ||
* @return A Mono that completes when the notification is processed | ||
*/ | ||
private Mono<Void> handleIncomingNotification(McpSchema.JSONRPCNotification notification) { | ||
private Mono<Void> handleIncomingNotification(McpSchema.JSONRPCNotification notification, | ||
McpTransportContext transportContext) { | ||
return Mono.defer(() -> { | ||
if (McpSchema.METHOD_NOTIFICATION_INITIALIZED.equals(notification.method())) { | ||
this.state.lazySet(STATE_INITIALIZED); | ||
// FIXME: The session ID passed here is not the same as the one in the | ||
// legacy SSE transport. | ||
exchangeSink.tryEmitValue(new McpAsyncServerExchange(this.id, this, clientCapabilities.get(), | ||
clientInfo.get(), McpTransportContext.EMPTY)); | ||
clientInfo.get(), transportContext)); | ||
} | ||
|
||
var handler = notificationHandlers.get(notification.method()); | ||
if (handler == null) { | ||
logger.warn("No handler registered for notification method: {}", notification); | ||
return Mono.empty(); | ||
} | ||
return this.exchangeSink.asMono().flatMap(exchange -> handler.handle(exchange, notification.params())); | ||
return this.exchangeSink.asMono().flatMap(exchange -> { | ||
// 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); | ||
Comment on lines
+317
to
+324
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
return handler.handle(newExchange, notification.params()); | ||
}); | ||
}); | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,6 +10,7 @@ | |
import static org.assertj.core.api.Assertions.assertThatExceptionOfType; | ||
import static org.assertj.core.api.Assertions.assertWith; | ||
import static org.awaitility.Awaitility.await; | ||
import static org.junit.jupiter.api.Assertions.assertFalse; | ||
import static org.mockito.Mockito.mock; | ||
|
||
import java.net.URI; | ||
|
@@ -23,11 +24,14 @@ | |
import java.util.concurrent.CopyOnWriteArrayList; | ||
import java.util.concurrent.CountDownLatch; | ||
import java.util.concurrent.TimeUnit; | ||
import java.util.concurrent.atomic.AtomicBoolean; | ||
import java.util.concurrent.atomic.AtomicReference; | ||
import java.util.function.BiFunction; | ||
import java.util.function.Function; | ||
import java.util.stream.Collectors; | ||
|
||
import jakarta.servlet.http.HttpServletRequest; | ||
import org.assertj.core.util.Strings; | ||
import org.junit.jupiter.params.ParameterizedTest; | ||
import org.junit.jupiter.params.provider.ValueSource; | ||
|
||
|
@@ -746,6 +750,7 @@ void testToolCallSuccess(String clientType) { | |
|
||
var clientBuilder = clientBuilders.get(clientType); | ||
|
||
var responseBodyIsNullOrBlank = new AtomicBoolean(false); | ||
var callResponse = new McpSchema.CallToolResult(List.of(new McpSchema.TextContent("CALL RESPONSE")), null); | ||
McpServerFeatures.SyncToolSpecification tool1 = McpServerFeatures.SyncToolSpecification.builder() | ||
.tool(Tool.builder().name("tool1").description("tool1 description").inputSchema(emptyJsonSchema).build()) | ||
|
@@ -759,7 +764,7 @@ void testToolCallSuccess(String clientType) { | |
.GET() | ||
.build(), HttpResponse.BodyHandlers.ofString()); | ||
String responseBody = response.body(); | ||
assertThat(responseBody).isNotBlank(); | ||
responseBodyIsNullOrBlank.set(Strings.isNullOrEmpty(responseBody)); | ||
} | ||
catch (Exception e) { | ||
e.printStackTrace(); | ||
|
@@ -782,6 +787,7 @@ void testToolCallSuccess(String clientType) { | |
|
||
CallToolResult response = mcpClient.callTool(new McpSchema.CallToolRequest("tool1", Map.of())); | ||
|
||
assertFalse(responseBodyIsNullOrBlank.get(), "Response body should not be blank"); | ||
assertThat(response).isNotNull().isEqualTo(callResponse); | ||
} | ||
|
||
|
@@ -825,6 +831,68 @@ void testThrowingToolCallIsCaughtBeforeTimeout(String clientType) { | |
mcpServer.close(); | ||
} | ||
|
||
@ParameterizedTest(name = "{0} : {displayName} ") | ||
@ValueSource(strings = { "httpclient" }) | ||
void testToolCallSuccessWithTranportContextExtraction(String clientType) { | ||
|
||
var clientBuilder = clientBuilders.get(clientType); | ||
|
||
var transportContextIsNull = new AtomicBoolean(false); | ||
var transportContextIsEmpty = new AtomicBoolean(false); | ||
var responseBodyIsNullOrBlank = new AtomicBoolean(false); | ||
|
||
var expectedCallResponse = new McpSchema.CallToolResult( | ||
List.of(new McpSchema.TextContent("CALL RESPONSE; ctx=value")), null); | ||
McpServerFeatures.SyncToolSpecification tool1 = McpServerFeatures.SyncToolSpecification.builder() | ||
.tool(Tool.builder().name("tool1").description("tool1 description").inputSchema(emptyJsonSchema).build()) | ||
.callHandler((exchange, request) -> { | ||
|
||
McpTransportContext transportContext = exchange.transportContext(); | ||
transportContextIsNull.set(transportContext == null); | ||
transportContextIsEmpty.set(transportContext.equals(McpTransportContext.EMPTY)); | ||
String ctxValue = (String) transportContext.get("important"); | ||
|
||
try { | ||
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 commentThe 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. |
||
.GET() | ||
.build(), HttpResponse.BodyHandlers.ofString()); | ||
String responseBody = response.body(); | ||
responseBodyIsNullOrBlank.set(Strings.isNullOrEmpty(responseBody)); | ||
} | ||
catch (Exception e) { | ||
e.printStackTrace(); | ||
} | ||
|
||
return new McpSchema.CallToolResult( | ||
List.of(new McpSchema.TextContent("CALL RESPONSE; ctx=" + ctxValue)), null); | ||
}) | ||
.build(); | ||
|
||
var mcpServer = prepareSyncServerBuilder().capabilities(ServerCapabilities.builder().tools(true).build()) | ||
.tools(tool1) | ||
.build(); | ||
|
||
try (var mcpClient = clientBuilder.build()) { | ||
|
||
InitializeResult initResult = mcpClient.initialize(); | ||
assertThat(initResult).isNotNull(); | ||
|
||
assertThat(mcpClient.listTools().tools()).contains(tool1.tool()); | ||
|
||
CallToolResult response = mcpClient.callTool(new McpSchema.CallToolRequest("tool1", Map.of())); | ||
|
||
assertFalse(transportContextIsNull.get(), "transportContext should not be null"); | ||
assertFalse(transportContextIsEmpty.get(), "transportContext should not be empty"); | ||
assertFalse(responseBodyIsNullOrBlank.get(), "Response body should not be blank"); | ||
assertThat(response).isNotNull().isEqualTo(expectedCallResponse); | ||
} | ||
|
||
mcpServer.close(); | ||
} | ||
|
||
@ParameterizedTest(name = "{0} : {displayName} ") | ||
@ValueSource(strings = { "httpclient" }) | ||
void testToolListChangeHandlingSuccess(String clientType) { | ||
|
@@ -1531,4 +1599,9 @@ private double evaluateExpression(String expression) { | |
}; | ||
} | ||
|
||
protected static McpTransportContextExtractor<HttpServletRequest> TEST_CONTEXT_EXTRACTOR = (r, tc) -> { | ||
tc.put("important", "value"); | ||
return tc; | ||
}; | ||
Comment on lines
+1602
to
+1605
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could have a copy of this in both 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 ( |
||
|
||
} |
Uh oh!
There was an error while loading. Please reload this page.