Propagate WebClient attributes into underlying HTTP client request where possible #29958
Conversation
Allows applying the attributes of the Http request to the underlying http-client request closes spring-projectsgh-26208
spring-projects-issues
added
the
status: waiting-for-triage
| @Override | ||
| protected void applyAttributes() { | ||
| // TODO | ||
| throw new RuntimeException(String.format("Using attributes is not available for %s", HttpRequest.class.getName())); |
There was a problem hiding this comment.
Can somebody please verify if there is an option to set request attributes for the java.net.http.HttpRequest?
If there is not, what should be the proper behaviour, should an Exception be thrown to the user if applyAttributes is set to true? Or should it simply always default to false when using the JdkClientHttpConnector?
There was a problem hiding this comment.
If the boolean flag is removed as suggested above, this should become a no-op.
| assertThat(nativeReq.getAttributes()).doesNotContainEntry("foo", "bar"); | ||
| } | ||
| } else if (nativeRequest.get() instanceof org.apache.hc.core5.http.HttpRequest nativeReq) { | ||
| // TODO get attributes from HttpClientContext |
There was a problem hiding this comment.
I could not find out how to verify the request attributes were added to the org.apache.hc.client5.http.protocol.HttpClientContext in the Test here, can somebody help out with this?
rstoyanchev
added
the
in: web
|
@rstoyanchev would love some feedback if you have the time. |
There was a problem hiding this comment.
Thanks for the pull request!
On second thought I don't think we need to make this opt-in and can drop the config option from WebClient.Builder and from ClientHttpConnectors. It does make sense to make attributes go further where we can, and treat them as any other part of the request. For Netty we are also adding the full map as a single attribute, so not likely to be a performance issue.
| @@ -45,6 +48,8 @@ | |||
| */ | |||
| class ReactorClientHttpRequest extends AbstractClientHttpRequest implements ZeroCopyHttpOutputMessage { | |||
|
|
|||
| public static final String ATTRIBUTES_CHANNEL_KEY = "attributes"; | |||
There was a problem hiding this comment.
The attribute name should be a little more qualified. Typically we would use something like:
ReactorClientHttpRequest.class.getName() + ".attributes"| @Override | ||
| protected void applyAttributes() { | ||
| // TODO | ||
| throw new RuntimeException(String.format("Using attributes is not available for %s", HttpRequest.class.getName())); |
There was a problem hiding this comment.
If the boolean flag is removed as suggested above, this should become a no-op.
rstoyanchev
added
type: enhancement
rstoyanchev
changed the title
This PR Closes #26208
As proposed by @tstavinoha (#26208 (comment)) and @rstoyanchev (#26208 (comment)), this adds the option
applyAttributesto theWebClient.Builder. If it is set totrue, theClientRequest.attributes()will be copied over to the underlying http-client library HttpRequest. For this there are different implementations for the variousClientHttpConnectorimplementations:JettyClientHttpRequest: Attributes are set with theorg.eclipse.jetty.client.api.Request#attribute(String, Object)method one by oneReactorClientHttpRequest: A singleMap<String, Object>containing all request attributes is set to the Attributeattributesin thereactor.netty.channel.ChannelOperations#channel()(as proposed by Passing attributes fromWebClientto underlying HTTP library #26208 (comment))ReactorNetty2ClientHttpRequest: Same as inReactorClientHttpRequest, but with thenetty5classesHttpComponentsClientHttpRequest: Attributes are set with theorg.apache.hc.client5.http.protocol.HttpClientContext#setAttribute(String, Object)method one by oneJdkClientHttpRequest: I still need some input on this one. As far as I can see there is no possibility to set request attributes to thejava.net.http.HttpRequest?As suggested by #26208 (comment),
applyAttributesdefaults totrueand the user can opt-out when using the Builder to create the Connector.