feat: introduce StreamableHttpServerTransport.Configuration#560
feat: introduce StreamableHttpServerTransport.Configuration#560
Conversation
| assertEquals(testResourceContent, content.text, "Resource content should match") | ||
| } | ||
|
|
||
| @Ignore("Blocked by https://github.com/modelcontextprotocol/kotlin-sdk/issues/249") |
There was a problem hiding this comment.
This test is passing now
kpavlov
force-pushed
the
kpavlov/streaming-http-config
branch
from
ef8ba32 to
6ab4187
Compare
kpavlov
force-pushed
the
kpavlov/streaming-http-config
branch
2 times, most recently
from
8c63aa8 to
f649ff0
Compare
devcrocod
left a comment
devcrocod
left a comment
There was a problem hiding this comment.
Except for one place where I left a comment, it looks good
| public companion object { | ||
| public val Default: Configuration = Configuration() | ||
| } |
There was a problem hiding this comment.
Why is a companion object needed?
In my view, it’s redundant, considering that Configuration already has all the parameters with default values
There was a problem hiding this comment.
It serves as a constant avoid creating Configuration instances every time. Does it make sense?
There was a problem hiding this comment.
Not really.
Is it actually expected that users will be creating thousands of Configuration instances?
Also, this adds it to the public Companion
There was a problem hiding this comment.
Is it actually expected that users will be creating thousands of Configuration instances?
Thank you, @devcrocod.
When I took a closer look at the code, I noticed a more important problem here: the custom config object is being created on every POST request, anda one-time StreamableHttpServerTransport is also being created.
The Configuration object would have been reused if this method had accepted it as a parameter.
Given this, let’s remove the default parameter value from the StreamableHttpServerTransport constructor completely. So, StreamableHttpServerTransport.Default is also not needed.
Changing Application.mcpStatelessStreamableHttp(...) parameters from plain parameter list to configuration object should be separate PR.
kpavlov
changed the title
|
Please, have a look at #563 after this one |
| */ | ||
| @OptIn(ExperimentalUuidApi::class, ExperimentalAtomicApi::class) | ||
| @Suppress("TooManyFunctions") | ||
| public class StreamableHttpServerTransport( |
There was a problem hiding this comment.
Should we add a method for backward source compatibility?
kpavlov
force-pushed
the
kpavlov/streaming-http-config
branch
from
0df06f5 to
41eb756
Compare
Replace the six-parameter flat constructor of StreamableHttpServerTransport with a typed Configuration class. This improves API ergonomics and provides a stable extension point for future options without further breaking the constructor signature.
Changes:
- Add `Configuration` as a public class nested directly on `StreamableHttpServerTransport`, with `enableJsonResponse` as the first
parameter (most commonly set)
- Change the primary constructor to accept `Configuration`
- Rename `retryIntervalMillis: Long?` to `retryInterval: Duration?` in Configuration, aligning with Kotlin's type-safe time API
- Deprecate the old flat constructor with a compatibility bridge
- Update KotlinTestBase integration test to use the new constructor
…simplify `StreamableHttpServerTransport` constructor
…uctor - Revert default constructor and mark it as deprecated - Add ReplaceWith expression for multi-parameter constructor
kpavlov
force-pushed
the
kpavlov/streaming-http-config
branch
from
41eb756 to
7da7675
Compare
4 participants
Introduce Configuration class for StreamableHttpServerTransport
Replace the six-parameter flat constructor of
StreamableHttpServerTransportwith a typedConfigurationclass. This improves API ergonomics, enables structural equality and copy(), and provides a stable extension point for future options without further breaking the constructor signature.Changes:
Configurationas a public class nested directly onStreamableHttpServerTransport, withenableJsonResponseas the first parameter (most commonly set)Configuration.retryIntervalMillis: Long?toretryInterval: Duration?in Configuration, aligning with Kotlin's type-safe time APIMotivation and Context
The current StreamableHttpServerTransport API cannot be easily extended: adding more parameters would be a breaking change. But this is already needed for #521.
This PR is a prerequisite for #535
How Has This Been Tested?
Regression tests
Breaking Changes
No. Current StreamableHttpServerTransport constructor was deprecated
Types of changes
Checklist