feat(spi): adds implementation-neutral SPI module #178
Conversation
Aliaksie
force-pushed
the
feat/mcp-spi
branch
2 times, most recently
from
efa638e to
9d7ed54
Compare
Aliaksie
changed the title
|
Hey @tzolov, hope you're doing well! Just wanted to check in — have you had a chance to review this PR? I’d appreciate any feedback when you get a moment. Looking forward to your thoughts! |
Aliaksie
force-pushed
the
feat/mcp-spi
branch
2 times, most recently
from
9432f3c to
58173e5
Compare
Aliaksie
force-pushed
the
feat/mcp-spi
branch
2 times, most recently
from
2dac2b3 to
62b6f8a
Compare
|
Hi @tzolov👋 Just checking in to see if there’s been a chance to review this PR I’d love to get your thoughts on whether this is a direction the project is open to, or if there’s anything I should revise to help move it forward. Happy to discuss further or break it into smaller pieces if that helps with review. |
| <version>${project.version}</version> | ||
| </dependency> | ||
|
|
||
| <!-- MCP Jacson Schema --> |
There was a problem hiding this comment.
| <!-- MCP Jacson Schema --> | |
| <!-- MCP Jackson Schema --> |
A nit typo!
|
|
||
| byte[] encode(McpSchema.JSONRPCMessage message); | ||
|
|
||
| String encodeAsString(Object message) throws IOException; |
There was a problem hiding this comment.
This method is only ever used for McpSchema.JSONRPCMessage and McpError instances. It may make more sense to change the signature to String encodeAsString(McpSchea.JSONRPCMessage message) (which would match decodeFromString) and add a separate method or overload for encoding McpError instances.
There was a problem hiding this comment.
Actually, on second thought, I'm not sure why this is used with McpError at all; maybe those lines could just avoid going through the codec.
| /** | ||
| * @author Aliaksei Darafeyeu | ||
| */ | ||
| public interface McpType<T> { |
There was a problem hiding this comment.
This interface doesn't quite make sense. It's always used to wrap a Class instance, never a ParameterizedType or GenericArray. Unlike Jackson TypeReference, this class doesn't support obtaining full type information by sub-classing, so I don't really see the value in having this wrapper. It seems like it would make more sense to just use Class<T> in it's place.
| /** | ||
| * @author Aliaksei Darafeyeu | ||
| */ | ||
| public interface McpLogger { |
There was a problem hiding this comment.
This logging interface doesn't seem ideal; it feels like all logging levels should support attaching a Throwable cause, the number of methods seems excessive, and there's no way to lazily evaluate string parameters if the message is omitted. Maybe a single void log(McpSchema.LoggingLevel level, Supplier<String> messageSupplier, @Nullable Throwable cause) method would make more sense.
| * | ||
| * @author Aliaksei Darafeyeu | ||
| */ | ||
| public interface McpSchemaCodec { |
There was a problem hiding this comment.
The byte[] base encode and decode as well as decodeBytes never seem to be used. The actual encoded type of the data is likely dependent on the specific transport being used. I wonder if it might make more sense to change this to something like:
public interface McpSchemaCodec<T> {
T encode(McpSchema.JSONRPCMessage message) throws IOException;
McpSchema.JSONRPCMessage decode(T message) throws IOException;
<U> U unmarshalFrom(Object data, Class<T> clazz);
}Then, each transport would be capable of using the codecs for the encoded data types it understands instead of each codec needing to support all possible encodings.
This PR introduces the first version of the
mcp-spimodule to define a transport-agnostic, reactive-compatible interface for the Model Context Protocol Java SDK.Refactor mcp to optionally log via a pluggable logger interface (McpTransportLogger interface in
mcp-spiinstead of hard SLF4J dependency).Key changes:
mcp-spi(transport interfaces, Jackson-free schema types, McpSchemaCodec abstraction, McpClient/McpServer abstraction, McpLogger pluggable logger interface)mcp-schema-jacksonJackson-based schema + McpSchemaCodec implementationmcp-logging-slf4jslf4j-based implementation of McpLoggermcptomcp-reactormcp-reactorto usemcp-spiandmcp-schema-jackson(as default codec implementation) andmcp-logging-slf4j(as default McpLogger implementation)mcp-reactorMotivation and Context
The motivation behind this change is to make the SDK modular, implementation-agnostic, and easier to integrate with diverse runtime environments.
How Has This Been Tested?
Breaking Changes
Types of changes
Checklist
Additional context
Possible Future Work:
mcp-spi-tck).mcp-vertx), Java 11+ Flow.Publisher HTTP client(mcp-flow)mcp-schema-gson,mcp-schema-cbor,mcp-schema-protobuf).p.s. if these changes don't make sense no problem just close this pr :-)