refactor(client): improve validation and remove server methods #14
Conversation
tzolov
force-pushed
the
improve-client-validation
branch
from
6bb7fbd to
193b7b4
Compare
mcp/src/test/java/io/modelcontextprotocol/MockMcpTransport.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| public McpSchema.JSONRPCMessage getLastSentMessage() { | ||
| try { | ||
| latch.await(200, TimeUnit.MILLISECONDS); |
There was a problem hiding this comment.
This shouldn't be necessary since you are already dealing with a Sink which provides synchronization. The issue lies elsewhere.
| @@ -33,30 +34,40 @@ public class MockMcpTransport implements ClientMcpTransport, ServerMcpTransport | |||
|
|
|||
| private final Flux<McpSchema.JSONRPCMessage> outboundView = outgoing.asFlux().cache(1); | |||
|
|
|||
| java.util.concurrent.CountDownLatch latch = new java.util.concurrent.CountDownLatch(1); | |||
There was a problem hiding this comment.
The Flux above provides the means to synchronize actions. Something else is at fault.
There was a problem hiding this comment.
The role of the latch is to ensure that the expected number of output messages are produced in result of the (simulated) incoming message.
I've added additional public void simulateIncomingMessage(McpSchema.JSONRPCMessage message, int expectedResponseMessagesCount) signature that would allow the test to specify the expected output message (could be 0 for some tests).
| // Run in a separate reactive context to avoid blocking the main subscription | ||
| Mono.fromRunnable(() -> { | ||
| McpSchema.JSONRPCRequest initRequest = transport.getLastSentMessageAsRequest(); | ||
| assertThat(initRequest.method()).isEqualTo(McpSchema.METHOD_INITIALIZE); |
There was a problem hiding this comment.
I believe this assertion failure would be swallowed and would not fail the test. Best to set an AtomicReference on the outer context and verify assumptions in the main test thread.
| McpSchema.JSONRPCResponse initResponse = new McpSchema.JSONRPCResponse(McpSchema.JSONRPC_VERSION, | ||
| initRequest.id(), mockInitResult, null); | ||
| transport.simulateIncomingMessage(initResponse); | ||
| }).subscribeOn(reactor.core.scheduler.Schedulers.boundedElastic()).subscribe(); |
There was a problem hiding this comment.
This shouldn't be required. I think it's a source of the synchronization issues you are facing. Let me think about the way to address this.
| // Start initialization with reactive handling | ||
| InitializeResult result = asyncMcpClient.initialize().doOnSubscribe(subscription -> { | ||
| // Run in a separate reactive context to avoid blocking the main subscription | ||
| Mono.fromRunnable(() -> { | ||
| McpSchema.JSONRPCRequest initRequest = transport.getLastSentMessageAsRequest(); | ||
| assertThat(initRequest.method()).isEqualTo(McpSchema.METHOD_INITIALIZE); | ||
|
|
||
| // Send mock server response | ||
| McpSchema.JSONRPCResponse initResponse = new McpSchema.JSONRPCResponse(McpSchema.JSONRPC_VERSION, | ||
| initRequest.id(), mockInitResult, null); | ||
| transport.simulateIncomingMessage(initResponse); | ||
| // latch.countDown(); | ||
| }).subscribeOn(reactor.core.scheduler.Schedulers.boundedElastic()).subscribe(); | ||
| }).block(); |
There was a problem hiding this comment.
Is this code a duplication of the initialization method?
mcp/src/test/java/io/modelcontextprotocol/client/McpAsyncClientResponseHandlerTests.java
Show resolved
Hide resolved
|
@chemicL latest commit, address some of the comments. |
There was a problem hiding this comment.
Spotted a few copy-pastes here and there and added suggestions for error messages fixes.
Most importantly, please add a FIXME note to
- the
MockMcpTransportthat the latch is a temporary workaround and needs to be revisited - the use of
boundedElasticin the test - swallowed assertion failures
Aside from that, the production code checks improve the situation a bit, but make it hard to act upon. In general, it would be useful to not even allow acting upon an uninitialized instance and coordinate calls properly reducing the friction in usage, let's perhaps open an issue for redesigning the API a bit.
mcp/src/main/java/io/modelcontextprotocol/client/McpAsyncClient.java
Outdated
Show resolved
Hide resolved
mcp/src/main/java/io/modelcontextprotocol/client/McpAsyncClient.java
Outdated
Show resolved
Hide resolved
mcp/src/main/java/io/modelcontextprotocol/client/McpAsyncClient.java
Outdated
Show resolved
Hide resolved
mcp/src/main/java/io/modelcontextprotocol/client/McpAsyncClient.java
Outdated
Show resolved
Hide resolved
chemicL
force-pushed
the
improve-client-validation
branch
from
de6790a to
1e3e37e
Compare
tzolov
force-pushed
the
improve-client-validation
branch
from
57fb3f4 to
c3532da
Compare
Add client initialization and capability validation checks - New isInitialized() method to check client state - Validate server capabilities before tool/resource operations - Add clear error messages for common failure cases - Remove server-side notification methods from client: sendResourcesListChanged(), promptListChangedNotification() - Improve protocol version handling - Testing improvements and new initialization tests Resolves #13 Signed-off-by: Christian Tzolov <christian.tzolov@broadcom.com>
Signed-off-by: Christian Tzolov <christian.tzolov@broadcom.com>
Signed-off-by: Christian Tzolov <christian.tzolov@broadcom.com>
Signed-off-by: Christian Tzolov <christian.tzolov@broadcom.com>
Signed-off-by: Dariusz Jędrzejczyk <dariusz.jedrzejczyk@broadcom.com>
Signed-off-by: Christian Tzolov <christian.tzolov@broadcom.com>
tzolov
force-pushed
the
improve-client-validation
branch
from
c3532da to
4bdd734
Compare
Add client initialization and capability validation checks
Resolves #13