-
Notifications
You must be signed in to change notification settings - Fork 566
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
Webclient implementation (#1205) #1431
Conversation
5eb31cc
to
f82d12e
Compare
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.
The names ClientXXX feel too generic and not specific to WebClient or HTTP client. Maybe this is something that should be discussed with the team.
...t/standalone/src/main/java/io/helidon/rest/client/example/basic/StandaloneClientExample.java
Outdated
Show resolved
Hide resolved
webclient/webclient/src/main/java/io/helidon/webclient/ClientRequestHeadersImpl.java
Outdated
Show resolved
Hide resolved
return services; | ||
} | ||
|
||
Set<MessageBodyReader<?>> requestReaders() { |
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.
Thinking-out-loud.
I don't know if having a configuration for request/response specific readers/writers is really useful.
Instead you could expose the context from request/response and let the user add these manually for each request/response.
If we think there is a need to have HTTP client specific readers/writers for all client request/response, then maybe we should consider updating MediaSupport
, MessageBodyReaderContext
and MessageBodyWriterContext
to support creating a parented MediaSupport
.
This way you would only need to pass-in a MediaSupport
instance to RequestConfiguration
.
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.
Ah this is great you are mentioning this. I have a note to create issue for that. I think that having builder on MediaSupport
which allows you to insert explicit parent, would be great for my usecase. I didn't added yet, because I wanted to talk to you about it before.
In case of the exposing of context, I do not think that is what we should do.
The reason why I opted for specific readers/writers for each request/response is, that it allows you to have control over entity handling. Basically you can have two or more ways how to handle specific entity type in your code and this helps you to achieve this without interfering with other request where it is not desirable.
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.
There should be a parented context dedicated to each request, that would be symmetrical to how it's done in the webserver currently. We can talk more about this when you have some time.
webclient/webclient/src/main/java/io/helidon/webclient/ClientException.java
Outdated
Show resolved
Hide resolved
webclient/webclient/src/main/java/io/helidon/webclient/ClientConfiguration.java
Outdated
Show resolved
Hide resolved
webclient/tracing/src/main/java/io/helidon/webclient/tracing/ClientTracing.java
Outdated
Show resolved
Hide resolved
Signed-off-by: David Kral <david.k.kral@oracle.com>
Signed-off-by: David Kral <david.k.kral@oracle.com>
Signed-off-by: David Kral <david.k.kral@oracle.com>
Signed-off-by: David Kral <david.k.kral@oracle.com>
Signed-off-by: Tomas Langer <tomas.langer@oracle.com>
Signed-off-by: David Kral <david.k.kral@oracle.com>
Signed-off-by: David Kral <david.k.kral@oracle.com>
Signed-off-by: David Kral <david.k.kral@oracle.com>
Signed-off-by: David Kral <david.k.kral@oracle.com>
Signed-off-by: David Kral <david.k.kral@oracle.com>
Signed-off-by: David Kral <david.k.kral@oracle.com>
Signed-off-by: David Kral <david.k.kral@oracle.com>
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.
LGTM
More tests and updated examples will be added after the merge probably.