Skip to content

Comments

Fix for #793 - public (or protected) constructors to allow subclassing for extensions#794

Closed
scottslewis wants to merge 12 commits intomodelcontextprotocol:mainfrom
scottslewis:issue_792
Closed

Fix for #793 - public (or protected) constructors to allow subclassing for extensions#794
scottslewis wants to merge 12 commits intomodelcontextprotocol:mainfrom
scottslewis:issue_792

Conversation

@scottslewis
Copy link
Contributor

@scottslewis scottslewis commented Feb 14, 2026

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.

@Kehrlann
Copy link
Contributor

Kehrlann commented Feb 20, 2026

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.

@scottslewis
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants