Fix for #793 - public (or protected) constructors to allow subclassing for extensions#794
Fix for #793 - public (or protected) constructors to allow subclassing for extensions#794scottslewis wants to merge 12 commits intomodelcontextprotocol:mainfrom
Conversation
Signed-off-by: Scott Lewis <scottslewis@gmail.com>
variable to protected, so that extensions/subclasses can access.
and async servers and clients
for subclassing/extension.
Type directly rather than via the other constructor.
|
As a rule of thumb, we avoid making our core implementations extensible through subclassing. Instead, we prefer composition, and introducing specific APIs. In this case, some sort of handler for customizing the params seems to better fit. I'm going to close this, and we can start again from another issue. |
For context: I'm creating an MCP extension...that extends the behavior of server side notification sending to add params/data to the notification. There is currently no way to do this with the current sdk class structure. I believe this is true for all of McpSyncServer, async, stateless, etc ..as with the client clsses as well (not totally sure about that). I'm a fan of composition also fwiw. More generally, though, composition works only when the API of a class is actually exposed for composition at runtime...i.e. via interfaces and/or abstract super classes....which I would strongly suggest for all of the McpSync/AsyncServer classes (and the client classes as well). I'm willing to contribute to that refactoring if desired. In this case though, it's server notification extension that's required, meaning some way to replace the notification that's done (via mcpTransport member variable) to include data in the form of a non-null params. As well, the existing notify methods access mcpTransport in the current implementation (passing in null params), which is a private member variable currently, so it can't be accessed at all at runtime through inheritance or composition. Here is the extension impl that does that. I'm open to other ways of accomplishing this than inheritance, but for a viable extension a public (or perhaps just protected) constructors for the existing classes would be quick and easy...without compromising maintainability (especially protected). FWIW, I've found that both inheritance and composition are sometimes useful for SDK consumers. Please let me know what you want to do wrt refactoring strategy in both short and longer term to make this extension possible and I will open appropriate issues. |
Fix for issue #793
This pr creates public constructors for McpSyncServer, McpAsyncServer and the stateless sync/async McpServer* types.
Technically the constructors could be protected rather than public, but I think for an SDK that public constructors is likely preferable.
Motivation and Context
The main motivation is to be able to subclass the various sdk server and client types...i.e. McpSyncServer, McpAsyncServer, McpSyncClient, McpAsyncClient, etc. I am creating an extension that has to override/customized notifyToolsListChanged() so that params can be added to the notification.
There are other use cases for extending the various server and client types in an sdk, along with making member variables protected, so that sdk external subsclasses can extend the behavior of these types.
Also, to extend these types requires that it be possible to invoke a subclass constructor, with instances of all the required arguments (e.g. McpServerFeatures.Async for McpAsyncServer). So these types also need to have public constructors.