feat(server): auto-install ContentNegotiation with McpJson (#664)#665
feat(server): auto-install ContentNegotiation with McpJson (#664)#665
Conversation
38b5ee0 to
29c5946
Compare
mcp(), mcpStreamableHttp(), and mcpStatelessStreamableHttp() now install ContentNegotiation configured with McpJson automatically, removing the need for users to do it manually. If ContentNegotiation was already installed by user code, a warning is logged instead of crashing with DuplicatePluginException.
- adjust formatting in StreamableHttpClientTest for consistency - ci: run ktlint first and fail fast on CI
- Replaced captureStderr with a Logback ListAppender-based LogCapture utility - Swapped slf4j-simple for logback-classic in kotlin-sdk-server's test dependencies - Added logback-test.xml (matching the previous simplelogger.properties levels) - Tests now assert directly on captured log events instead of relying on stderr output
27849ca to
787211b
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
devcrocod
left a comment
There was a problem hiding this comment.
Thank you for the PR, this indeed needs to be fixed
However, it is still unclear to me what the user is supposed to do if they want to provide their own JSON configuration
for example, enable pretty printing or set other options in addition to explicitNulls and the rest
Also, in addition to the comments I already left, the AI review pointed out that KotlinTestBase was missed as well, and it also uses install(ContentNegotiation)
| - os: ubuntu-latest | ||
| job-name: "Linux Tests" | ||
| gradle-tasks: "build" | ||
| gradle-tasks: "ktlintCheck build" |
There was a problem hiding this comment.
How is it related to PR?
and ktlintCheck runs with check, which means it's already included in build task
There was a problem hiding this comment.
build runs check after running tests. Build should fail earlier if it can not be merged.
There was a problem hiding this comment.
build runs check after running tests. Build should fail earlier if it can not be merged.
That's not correct. test is part of check
There was a problem hiding this comment.
ktlintCheck doesn't depend on the tests. When running build, the tests and ktlintCheck are executed in parallel. So it is not entirely clear in which scenario "Build should fail earlier" would actually make the build fail faster
| mokksy = { group = "dev.mokksy", name = "mokksy", version.ref = "mokksy" } | ||
| netty-bom = { group = "io.netty", name = "netty-bom", version.ref = "netty" } | ||
| slf4j-simple = { group = "org.slf4j", name = "slf4j-simple", version.ref = "slf4j" } | ||
| logback-classic = { group = "ch.qos.logback", name = "logback-classic", version.ref = "logback" } |
There was a problem hiding this comment.
Why do we need another logger?
There was a problem hiding this comment.
For testing, please check the tests.
| "Remove your install(ContentNegotiation) { … } block and let " + | ||
| "mcp() / mcpStreamableHttp() / mcpStatelessStreamableHttp() configure it automatically." |
There was a problem hiding this comment.
To be honest, that’s a strange suggestion for me
| * Automatically installs [ContentNegotiation][io.ktor.server.plugins.contentnegotiation.ContentNegotiation] | ||
| * with [McpJson][io.modelcontextprotocol.kotlin.sdk.types.McpJson] and [SSE]. |
There was a problem hiding this comment.
Why hasn’t kdoc been updated for the other methods?
| configuration: StreamableHttpServerTransport.Configuration, | ||
| block: RoutingContext.() -> Server, | ||
| ) { | ||
| installMcpContentNegotiation() |
There was a problem hiding this comment.
If the user is configured to use multiple extensions, they will see a warning saying that ContentNegotiation is already installed
There was a problem hiding this comment.
?
I mean our extensions: mcp and mcpStreamable
There was a problem hiding this comment.
In this case, we must store an application attribute and make installMcpContentNegotiation() completely idempotent. Done
| internal class LogCapture(loggerName: String) : AutoCloseable { | ||
| private val logger: Logger = LoggerFactory.getLogger(loggerName) as Logger | ||
| private val appender: ListAppender<ILoggingEvent> = ListAppender() | ||
|
|
||
| init { | ||
| appender.start() | ||
| logger.addAppender(appender) | ||
| } | ||
|
|
||
| val messages: List<String> | ||
| get() = appender.list.map { it.formattedMessage } | ||
|
|
||
| override fun close() { | ||
| logger.detachAppender(appender) | ||
| appender.stop() | ||
| } | ||
| } |
There was a problem hiding this comment.
Wouldn't it be enough to just verify that no DuplicatePluginException is thrown?
The existing integration tests already prove that this works, since they did not break after the manual installation of ContentNegotiation was removed
Verifying the exact log output does not seem quite right because it may change at any moment. On top of that, it adds another dependency, extra configuration, and new tests that verify log output. It feels unnecessary
There was a problem hiding this comment.
DuplicatePluginException should not be thrown, otherwise it will break current users. Also, users should still be able to install their own ContentNegotiation configuration. That's why it is warniong, not an error.
The log message that should be verified, this is expected behavior and to verify. When extra dependency and configuration is needed - it is needed
There was a problem hiding this comment.
It's the log message that should be verified. When extra dependency and configuration is needed - it is needed
Why is that? Your comment doesn’t make it clear
| **CORS for browser-based clients (e.g. MCP Inspector):** if you connect from a browser-based | ||
| client you need to install the Ktor CORS plugin so that MCP-specific headers are allowed and exposed: | ||
|
|
||
| ```kotlin | ||
| install(CORS) { | ||
| anyHost() // restrict to specific origins in production | ||
| allowMethod(HttpMethod.Options) | ||
| allowMethod(HttpMethod.Get) | ||
| allowMethod(HttpMethod.Post) | ||
| allowMethod(HttpMethod.Delete) | ||
| allowNonSimpleContentTypes = true | ||
| allowHeader("Mcp-Session-Id") | ||
| allowHeader("Mcp-Protocol-Version") | ||
| exposeHeader("Mcp-Session-Id") | ||
| exposeHeader("Mcp-Protocol-Version") | ||
| } | ||
| ``` | ||
|
|
There was a problem hiding this comment.
This is a finding from the troubleshooting session. The MCP Inspector does not function with the default configuration without this.
There was a problem hiding this comment.
Didn't you suggest not mixing unrelated changes in the same PR?
There was a problem hiding this comment.
I agree, but this often occurs in this project. This note is related to the user’s issue. So "boy scout's rule" apply.
README.md
Outdated
| // mcpStreamableHttp() automatically installs ContentNegotiation with McpJson. | ||
| // Do NOT install ContentNegotiation yourself — the SDK handles it. |
There was a problem hiding this comment.
Why need this comment in the code snippet?
Use Application.attributes to track whether the SDK has already handled ContentNegotiation installation. Subsequent calls return immediately without re-installing or logging spurious warnings. Also softens the warning message for user-pre-installed ContentNegotiation to suggest ensuring json(McpJson) is included rather than demanding removal, and adds auto-install KDoc to mcpStreamableHttp/mcpStatelessStreamableHttp. Remove comment in code snippet in README.md
e5l
left a comment
There was a problem hiding this comment.
Two issues found — see inline comments.
| "Ensure your ContentNegotiation configuration includes json(McpJson)." | ||
| } | ||
| } else { | ||
| install(ContentNegotiation) { |
There was a problem hiding this comment.
When the user has pre-installed ContentNegotiation with wrong config (e.g. default json() instead of json(McpJson)), this logs a warning but doesn't actually fix anything — the runtime serialization failures still happen silently.
This means the most dangerous case (user installed CN incorrectly) isn't addressed. Consider either:
- Registering
McpJsonas an additional converter even when CN is already installed - Failing fast with an exception instead of a warning
As written, the fix only helps users who forgot to install CN entirely, not those who installed it with the wrong config.
| api(libs.ktor.server.core) | ||
| api(libs.ktor.server.sse) | ||
| implementation(libs.ktor.server.content.negotiation) | ||
| implementation(libs.ktor.server.websockets) |
There was a problem hiding this comment.
ktor-serialization should already come in transitively via ktor-server-content-negotiation. Is there a specific multiplatform reason this is needed as a direct dependency? If not, it can be removed.
There was a problem hiding this comment.
Unfortunately, both ktor-server-content-negotiation and ktor-serialization are required
e5l
left a comment
There was a problem hiding this comment.
agreed to fix it as separate pr
|
Added follow-up issue #668 |
Users currently must manually
install(ContentNegotiation) { json(McpJson) }before callingmcp(),mcpStreamableHttp(), ormcpStatelessStreamableHttp(). Forgetting this or using the defaultjson()instead of
json(McpJson)causes silent serialization failures at runtime (e.g., explicit null fields in JSON-RPCresponses). This PR makes the SDK install it automatically.
Fixes #664
Changes
Core feature —
mcp(),mcpStreamableHttp(), andmcpStatelessStreamableHttp()now auto-installContentNegotiationconfigured withMcpJson. If the user already installedContentNegotiation, a warning is logged instead of crashing withDuplicatePluginException.KtorServerHelpers.kt(new) —installMcpContentNegotiation()internal helper withpluginOrNullguard and warningKtorServer.kt— calls the helper from all three entry pointsbuild.gradle.kts— addsktor-server-content-negotiationandktor-serializationasimplementationdeps tokotlin-sdk-serverRemoved manual installs from
ConformanceServer.ktandAbstractStreamableHttpIntegrationTest.kt.Test infrastructure — replaced
slf4j-simplewith Logback and useListAppender(LogCaptureutility) for deterministic log assertion. Addedlogback-test.xmlmatching previous log levels.StreamableHttpServerConfigurationTest.kt(new) — verifies the warning for all three entry points:mcpStreamableHttp,mcpStatelessStreamableHttp, andmcp(SSE).README.md — removed manual
ContentNegotiationboilerplate from examples, documented automatic installation, added CORS configuration snippet for browser-based clients.CI — added
ktlintCheckto the Linux build matrix job.How Has This Been Tested?
Unit/integration tests. Conformance tests updated
Breaking Changes
No
Types of changes
Checklist