Skip to content
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

Merged
merged 12 commits into from
Mar 11, 2020
Merged

Conversation

Verdent
Copy link
Member

@Verdent Verdent commented Feb 21, 2020

More tests and updated examples will be added after the merge probably.

Copy link
Contributor

@romain-grecourt romain-grecourt left a 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.

examples/webclient/pom.xml Outdated Show resolved Hide resolved
return services;
}

Set<MessageBodyReader<?>> requestReaders() {
Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Contributor

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.

Verdent and others added 8 commits March 9, 2020 16:14
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>
Copy link
Member

@tomas-langer tomas-langer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Verdent Verdent merged commit 6ca7d5e into helidon-io:master Mar 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants