-
Notifications
You must be signed in to change notification settings - Fork 556
DRAFT - HttpRequest.Builder customizer #388
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
base: main
Are you sure you want to change the base?
DRAFT - HttpRequest.Builder customizer #388
Conversation
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.
@Kehrlann I think this would work.
What about the webflux clients. would we need to expose a similar customizer abstractions there as well?
/** | ||
* In reactive, DONT USE THIS. Use AsyncHttpRequestCustomizer. | ||
*/ | ||
public Builder httpRequestCustomizer(SyncHttpRequestCustomizer syncHttpRequestCustomizer) { |
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.
This feels awkward. Perhaps we should leave only the async method and let the user convert the sync customizer outside
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.
I see your point.
I’m trying to ensure sync-based users don’t have to think about async versions of this API at all. My goal is for the user to make a “sync customizer” and only have to worry about that. Making the transformation outside is one more layer they’d have to think about (and that we would have to explain in the docs).
Also, we’ve had feedback from people absolutely NOT wanting to deal with async.
let me know what you think; happy to make the change to “only async” if you think it’s more confusing than helpful.
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.
I completely understand the need for this. Would this work better:
public Builder httpRequestCustomizer(SyncHttpRequestCustomizer syncHttpRequestCustomizer)
public Builder asyncHttpRequestCustomizer(AsyncHttpRequestCustomizer asyncHttpRequestCustomizer)
The user is now forced to understand there are two ways to go about it and if they are:
- on a Servlet/MVC Thread,
- not waiting for any async computation
- on a non-blocking Thread, but are not blocking
they can freely use the sync customizer. It would actually be the default choice.
For advanced cases, e.g. when the customizer needs to wait for something to happen - e.g. authorization token to be provided via a Publisher
before it can provide the HttpRequest.Builder
, it should go via the async route. We can document this.
Nice! I’ll tidy up the PR then. for WebClient-based transport, the WebClient has transformation primitives already (for ex ExchangeFilterFunction), so no need to add an MCP-specific customizer. |
Provide a way to customize HTTP requests before executing them, for both
HttpClientSseClientTransport
andHttpClientStreamableHttpTransport
.We introduce
AsyncHttpRequestCustomizer
, which is the core class that does the heavy lifting. For blocking contexts, we provide a simplerSyncHttpRequestCustomizer
API, which ultimately gets wrapped. The details of the implementation in both transports is not relevant (yet) ; we are focused on both customizers and also on the transpots, specifically:AsyncHttpRequestCustomizer
SyncHttpRequestCustomizer
HttpClientSseClientTransport.Builder#requestCustomizer
HttpClientStreamableHttpTransport.Builder#requestCustomizer
Motivation and Context
These are transport-level hooks, to allow users to modify request, e.g. to add security in headers (OAuth2 tokens, API keys, etc).
How Has This Been Tested?
Tested with a Servlet Spring app, using
SyncHttpRequestCustomizer
.Breaking Changes
No.
Types of changes
Checklist