-
Notifications
You must be signed in to change notification settings - Fork 256
refactor: adds Client/Reqeust customizer for HttpClientSseClientTransport #117
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: adds Client/Reqeust customizer for HttpClientSseClientTransport #117
Conversation
…SseClientTransport
|
||
/** | ||
* Creates a new builder with the specified base URI. | ||
* @param baseUri the base URI of the MCP server | ||
*/ | ||
public Builder(String baseUri) { | ||
Builder(String baseUri) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to deprecate it first in order to avoid breaking the users. It is currently public.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're absolutely right — to avoid breaking existing users, I've marked all currently public constructors as @Deprecated
and added clear Javadoc indicating that they will be removed in a future release.
In addition, I introduced an alternative package-private constructor, which will be used in the new builder-based approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some comments around backwards compatibility, but this is a useful change, thanks!
* @param requestBuilder the HTTP request builder to use | ||
* @param baseUri the base URI of the MCP server | ||
* @param sseEndpoint the SSE endpoint path | ||
* @param objectMapper the object mapper for JSON serialization/deserialization | ||
* @throws IllegalArgumentException if objectMapper, clientBuilder, or headers is null | ||
*/ | ||
public HttpClientSseClientTransport(HttpClient.Builder clientBuilder, HttpRequest.Builder requestBuilder, | ||
String baseUri, String sseEndpoint, ObjectMapper objectMapper) { | ||
HttpClientSseClientTransport(HttpClient httpClient, HttpRequest.Builder requestBuilder, String baseUri, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to deprecate it first in order to avoid breaking the users. It is currently public.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're absolutely right — to avoid breaking existing users, I've marked all currently public constructors as @Deprecated
and added clear Javadoc indicating that they will be removed in a future release.
In addition, I introduced an alternative package-private constructor, which will be used in the new builder-based approach.
…customization API (#117) - Add builder customizeClient() and customizeRequest() methods - Enable HTTP client and request configuration through consumer-based customization - Deprecate direct constructors in favor of the more flexible builder approach - Add test coverage for customization capabilities Co-authored-by: Christian Tzolov <[email protected]> Signed-off-by: Christian Tzolov <[email protected]>
…
This PR improves the construction pattern for HttpClientSseClientTransport by enforcing usage via a builder. It also introduces support for customizing HttpClient.Builder and HttpRequest.Builder before the transport is created. This provides a cleaner, more extensible way to configure HTTP behavior. Related to issue
Motivation and Context
I came across this project and really liked its design and goals, so I wanted to start contributing.
How Has This Been Tested?
Breaking Changes
Yes — this change makes the constructor for HttpClientSseClientTransport protected/private, which is a breaking change for users relying on direct instantiation.
To migrate, users should use the new builder-based approach for constructing HttpClientSseClientTransport.
Types of changes
Checklist
Additional context
let me know if i missed something and need to add or if these changes don't make sense no problem just close this pr :-)