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

Provide a way to create a client with an Endpoint rather than with a URI #1743

Open
wants to merge 10 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@SeungukHan
Copy link

commented Apr 25, 2019

Motivation:

  • Current HttpClient only allow string or URI object for argument,
    but Endpoint is commonly use for handling URI. For convenience,
    HttpClient can accept Endpoint as argument.

Modification:

  • Add Endpoint argument to HttpClient creation methods.

Result:

  • HttpClient can be created with Endpoint instead of string or URI.
@CLAassistant

This comment has been minimized.

Copy link

commented Apr 25, 2019

CLA assistant check
All committers have signed the CLA.

@codecov

This comment has been minimized.

Copy link

commented Apr 25, 2019

Codecov Report

Merging #1743 into master will decrease coverage by 0.03%.
The diff coverage is 50.78%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1743      +/-   ##
============================================
- Coverage     71.81%   71.77%   -0.04%     
- Complexity     8436     8488      +52     
============================================
  Files           761      762       +1     
  Lines         33871    34048     +177     
  Branches       4168     4180      +12     
============================================
+ Hits          24323    24439     +116     
- Misses         7439     7486      +47     
- Partials       2109     2123      +14
Impacted Files Coverage Δ Complexity Δ
...ava/com/linecorp/armeria/client/ClientFactory.java 71.42% <ø> (ø) 2 <0> (ø) ⬇️
...necorp/armeria/client/DecoratingClientFactory.java 50% <0%> (-3.34%) 5 <0> (ø)
...n/java/com/linecorp/armeria/client/HttpClient.java 31.57% <0%> (-8.43%) 12 <0> (ø)
...inecorp/armeria/client/grpc/GrpcClientFactory.java 68.75% <100%> (+1.53%) 14 <1> (+2) ⬆️
.../linecorp/armeria/client/DefaultClientFactory.java 72% <100%> (+1.78%) 15 <1> (+1) ⬆️
...ain/java/com/linecorp/armeria/client/Endpoint.java 97.77% <100%> (+0.29%) 92 <8> (+8) ⬆️
...corp/armeria/client/thrift/THttpClientFactory.java 89.74% <100%> (+1.17%) 11 <1> (+2) ⬆️
...linecorp/armeria/client/AbstractClientFactory.java 26.82% <16.66%> (-7.96%) 4 <1> (+1)
...com/linecorp/armeria/client/HttpClientBuilder.java 46.87% <27.77%> (-33.13%) 5 <1> (-1)
...main/java/com/linecorp/armeria/client/Clients.java 64.78% <37.5%> (-7.94%) 23 <6> (+6)
... and 30 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8b58e26...caf03f3. Read the comment docs.

@trustin trustin added the new-feature label Apr 25, 2019

@minwoox
Copy link
Member

left a comment

Could you also add the methods which take an Endpoint to HttpClientBuilder and ClientBuilder?

@SeungukHan SeungukHan force-pushed the SeungukHan:endpoint branch from d951158 to 41b5980 Apr 27, 2019

@minwoox

This comment has been minimized.

Copy link
Member

commented Apr 29, 2019

Could you also add the methods to Clients as well, please? ^^*

@trustin
Copy link
Member

left a comment

Nice work! Asked for some additional touches for perfection.

* Creates a new {@link ClientBuilder} that builds the client that connects to
* the specified {@link Endpoint} with {@code scheme}.
*/
public ClientBuilder(String scheme, Endpoint endpoint) {

This comment has been minimized.

Copy link
@trustin

trustin May 2, 2019

Member

Could you also add the following variants:

  • ClientBuilder(Scheme scheme, Endpoint endpoint)
  • ClientBuilder(SessionProtocol protocol, SerializationFormat format, Endpoint endpoint)
    • Shortcut of ClientBuilder(Scheme.of(format, protocol), endpoint)
  • ClientBuilder(SessionProtocol protocol, Endpoint endpoint)
    • Shortcut of ClientBuilder(protocol, SerializationFormat.NONE, endpoint)
  • ClientBuilder(String scheme, Endpoint endpoint) could also be a shortcut of ClientBuilder(Scheme.parse(scheme), endpoint).
*
* @return the URI
*/
public URI toURI(String scheme) {

This comment has been minimized.

Copy link
@trustin

trustin May 2, 2019

Member
  • toUri()
  • Could you also add the variants similar to those mentioned in ClientBuilder?
    • One thing I'm not sure is whether Endpoint.toUri(SessionProtocol) should return a URI that starts with none+. Perhaps we should not? 🤔
  • When a scheme is a String, we need to URI-encode it or make sure it does not contain anything weird, e.g. what would happen if scheme is http://www.badguys.com/?
}

if (port != 0) {
host = host + ':' + port;

This comment has been minimized.

Copy link
@trustin

trustin May 2, 2019

Member

Could you optimize this a little bit so that concatenation is not done in two-fold? e.g.

if (port != 0) {
    if (isValidIpV6Address(host)) {
        return '[' + host + "]:" + port;
    } else {
        return host + ':' + port
    }
else {
    ...
}
* @param endpoint the server {@link Endpoint}
* @param options the {@link ClientOptionValue}s
*/
static HttpClient of(String scheme, Endpoint endpoint, ClientOptionValue<?>... options) {

This comment has been minimized.

Copy link
@trustin

trustin May 2, 2019

Member

String scheme should be replaced with SessionProtocol protocol, because the serialization format is always none for HttpClient.

* @param endpoint the server {@link Endpoint}
* @param options the {@link ClientOptions}
*/
static HttpClient of(String scheme, Endpoint endpoint, ClientOptions options) {

This comment has been minimized.

Copy link
@trustin

trustin May 2, 2019

Member

String scheme should be replaced with SessionProtocol protocol, because the serialization format is always none for HttpClient.

* @param endpoint the server {@link Endpoint}
* @param options the {@link ClientOptionValue}s
*/
static HttpClient of(ClientFactory factory, String scheme, Endpoint endpoint,

This comment has been minimized.

Copy link
@trustin

trustin May 2, 2019

Member

String scheme should be replaced with SessionProtocol protocol, because the serialization format is always none for HttpClient.

* @param endpoint the server {@link Endpoint}
* @param options the {@link ClientOptions}
*/
static HttpClient of(ClientFactory factory, String scheme, Endpoint endpoint, ClientOptions options) {

This comment has been minimized.

Copy link
@trustin

trustin May 2, 2019

Member

String scheme should be replaced with SessionProtocol protocol, because the serialization format is always none for HttpClient.

* @throws IllegalArgumentException if the {@code scheme} is not one of the fields
* in {@link SessionProtocol}
*/
public HttpClientBuilder(String scheme, Endpoint endpoint) {

This comment has been minimized.

Copy link
@trustin

trustin May 2, 2019

Member

String scheme should be replaced with SessionProtocol protocol, because the serialization format is always none for HttpClient.

@trustin

This comment has been minimized.

Copy link
Member

commented May 8, 2019

@SeungukHan Gentle ping 😉

@SeungukHan

This comment has been minimized.

Copy link
Author

commented May 8, 2019

Oh! I am on the job!
I think the next update can be pushed until tomorrow!
Sorry for late TT

@trustin

This comment has been minimized.

Copy link
Member

commented May 8, 2019

No pressure. Please take your time. 😄

@SeungukHan SeungukHan requested a review from anuraaga as a code owner May 10, 2019

SeungukHan added some commits Apr 25, 2019

Provide a way to create a client with an Endpoint rather than with a URI
   Motivation:
   - Current HttpClient only allow string or URI object for argument,
     but Endpoint is commonly use for handling URI. For convenience,
     HttpClient can accept Endpoint as argument.

   Modification:
   - Add Endpoint argument to HttpClient creation methods.

   Result:
   - HttpClient can be created with Endpoint instead of string or URI.
Provide a way to create a client with an Endpoint rather than with an
URI

 - Make URI can be built with a specific scheme
 - Make toURI method in Endpoint create URI with ipAddr when it is set
Provide a way to create a client with an Endpoint rather than with an
URI

 - Add Endpoint argument for ClientBuilder
Provide a way to create a client with an Endpoint rather than with an…
… URI

 - Use Scheme, SessionProtocol, SerializationFormat for making Client
 Object

@SeungukHan SeungukHan force-pushed the SeungukHan:endpoint branch from e56ccba to 8ebe77a May 12, 2019

@minwoox
Copy link
Member

left a comment

Just few nits! Great job!

@@ -84,6 +110,26 @@ protected final Scheme validateScheme(URI uri) {
return parsedScheme;
}

/**
* Makes sure the scheme of the specified {@link Scheme} is supported by this {@link ClientFactory}.

This comment has been minimized.

Copy link
@minwoox

minwoox May 14, 2019

Member

the scheme of the specified {@link Scheme} -> the specified {@link Scheme}

* Makes sure the scheme of the specified {@link Scheme} is supported by this {@link ClientFactory}.
*
* @param scheme the {@link Scheme} of the server endpoint
* @return the supported {@link Scheme}

This comment has been minimized.

Copy link
@minwoox

minwoox May 14, 2019

Member

the specified {@link Scheme}? or I think we can remove this clause.

}

/**
* Creates a new {@link ClientBuilder} that builds the client that connects to the specified

This comment has been minimized.

Copy link
@minwoox

minwoox May 14, 2019

Member

that builds the HTTP client because the SerializationFormat is SerializationFormat.NONE?

@Nullable
private Scheme scheme;
@Nullable
private Endpoint endpoint;

This comment has been minimized.

Copy link
@minwoox

minwoox May 14, 2019

Member

These three fields should be final.

private ClientBuilder(URI uri, Scheme scheme, Endpoint endpoint) {
    ...
}
* @param options the {@link ClientOptionValue}s
*
* @throws IllegalArgumentException if the scheme of the specified {@link SessionProtocol} and
* {@link SerializationFormat}, or the specified {@code clientType} is

This comment has been minimized.

Copy link
@minwoox

minwoox May 14, 2019

Member

{@link ... should be underneath if.

return URI.create(scheme.uriText() + "://" + authority);
}

private static String buildAuthority(String host, int port) {

This comment has been minimized.

Copy link
@minwoox

minwoox May 14, 2019

Member

Question: Is there any reason that you didn't use Endpoint.authority() method?

This comment has been minimized.

Copy link
@SeungukHan

SeungukHan May 15, 2019

Author

Thanks, now I understand the existing authority method is enough for creating URI object.

* @throws IllegalArgumentException if the {@link Scheme} is not one of the fields
* in {@link SessionProtocol}
*/
public HttpClientBuilder(Scheme scheme, Endpoint endpoint) {

This comment has been minimized.

Copy link
@minwoox

minwoox May 14, 2019

Member

scheme should be replaced with SessionProtocol protocol, because the serialization format is always none for HttpClient.

* in {@link SessionProtocol}
*/
public HttpClientBuilder(Scheme scheme, Endpoint endpoint) {
this.scheme = requireNonNull(scheme, "scheme");

This comment has been minimized.

Copy link
@minwoox

minwoox May 14, 2019

Member

After converting a SessionProtocol to a Scheme by adding none+, we still should validate the scheme.

@@ -33,14 +33,16 @@ public void parse() {
assertThat(foo.ipAddr()).isNull();
assertThat(foo.ipFamily()).isNull();
assertThat(foo.hasIpAddr()).isFalse();
assertThat(foo.toUri("none+http").toString()).isEqualTo("none+http://foo");

This comment has been minimized.

Copy link
@minwoox

minwoox May 14, 2019

Member

Could you add a test case that @trustin mentioned which takes http://www.badguys.com/ as a parameter?

@minwoox minwoox added this to the 0.86.0 milestone May 15, 2019


/**
* Creates a new client that connects to the specified {@link Endpoint} with the {@link Scheme} using
* an alternative {@link ClientFactory}.

This comment has been minimized.

Copy link
@hyangtack

hyangtack May 15, 2019

Contributor

nit: How about an alternative -> the specified?


/**
* Creates a new HTTP client that connects to the specified {@link Endpoint} with {@link SessionProtocol}
* using an alternative {@link ClientFactory}.

This comment has been minimized.

Copy link
@hyangtack

hyangtack May 15, 2019

Contributor

nit: How about an alternative -> the specified?

@@ -32,7 +35,12 @@
*/
public final class HttpClientBuilder extends AbstractClientOptionsBuilder<HttpClientBuilder> {

private final URI uri;
@Nullable
private URI uri;

This comment has been minimized.

Copy link
@hyangtack

hyangtack May 15, 2019

Contributor

We may make uri, scheme and endpoint as final variables.

}

/**
* Creates a new HTTP client that connects to the specified {@link Endpoint} with {@link SessionProtocol}

This comment has been minimized.

Copy link
@hyangtack

hyangtack May 15, 2019

Contributor

nit: with {@link SessionProtocol} -> with the {@link SessionProtocol}?

if (isGroup()) {
authority = "group:" + groupName;
} else {
authority = buildAuthority(hasIpAddr() ? ipAddr() : host(), port);

This comment has been minimized.

Copy link
@hyangtack

hyangtack May 15, 2019

Contributor

Is it okay to use ipAddr instead of host from here? What if the scheme is https?

This comment has been minimized.

Copy link
@SeungukHan

SeungukHan May 16, 2019

Author

I change to use authority() method when creating URI.

SeungukHan added some commits May 15, 2019

Provide a way to create a client with an Endpoint rather than with an…
… URI

 - Replace Scheme to SessionProtocol in HttpClientBuilder
 - Edit javadoc about client create method
 - Make fields in builder be final
 - Add test case for failure
Provide a way to create a client with an Endpoint rather than with an…
… URI

 - Change the comments
 - Use the existing authority method while making URI in Endpoint
* {@link ClientFactory}.
*
* @param factory an alternative {@link ClientFactory}
* @param factory the specified {@link ClientFactory}

This comment has been minimized.

Copy link
@hyangtack

hyangtack May 16, 2019

Contributor

I'm sorry but I think an alternative {@link ClientFactory} would be better from here. I wanted to rewrite the method description only. :-)

Provide a way to create a client with an Endpoint rather than with an…
… URI

 - Change comments to "an alternative factory" for factory parameter
@SeungukHan

This comment has been minimized.

Copy link
Author

commented May 16, 2019

I am going to add some test codes for fill code coverage.

Provide a way to create a client with an Endpoint rather than with an…
… URI

 - Add test code for HttpClient creation
@Test
void givenHttpClient_thenBuildClient() throws Exception {
final URI uri = URI.create(server.httpUri("/"));
final Endpoint endpoint = Endpoint.of(uri.getHost()).withDefaultPort(uri.getPort());

This comment has been minimized.

Copy link
@hyangtack

hyangtack May 16, 2019

Contributor

nit: How about extracting this as a new method, like newEndpoint()?

@hyangtack hyangtack removed this from the 0.86.0 milestone May 17, 2019

@minwoox minwoox added this to the 0.87.0 milestone May 17, 2019

Provide a way to create a client with an Endpoint rather than with an…
… URI

 - Add path paramter for creating Client
    - At Clients, HttpClient, HttpClientBuilder, ClientBuilder,
    ClientFactory, HttpClientFactory
 - Add path paramter for toUri at Endpoint
 - Add test case for Thrift and gRPC client
@SeungukHan

This comment has been minimized.

Copy link
Author

commented May 21, 2019

The URI which is used for creating Client contains path value too.
For Client having path can be generated with Endpoint and related paramters, I add path parameter for that kind of factory method.

@minwoox

This comment has been minimized.

Copy link
Member

commented May 22, 2019

The URI which is used for creating Client contains path value too.
For Client having path can be generated with Endpoint and related paramters, I add path parameter for that kind of factory method.

I think we don't have to have the variants which take path as a parameter.
It makes API more complicated and a user can insert a path to the HttpRequest.
So it's probably better not to provide those APIs. What do you think?

Provide a way to create a client with an Endpoint rather than with an…
… URI

 - Add test case for path parameters
@SeungukHan

This comment has been minimized.

Copy link
Author

commented May 22, 2019

I totally agree to a path parameter makes APIs more complicated.
But, there are some case where a client need base path.

sb.service("/hello", THttpService.of(HELLO_SERVICE))
.service("/hellobinaryonly", THttpService.ofFormats(HELLO_SERVICE, BINARY))
.service("/hellotextonly", THttpService.ofFormats(HELLO_SERVICE, TEXT));

If there is a workaround solution please let me know. I really hope to remove the path parameter.

@minwoox

This comment has been minimized.

Copy link
Member

commented May 22, 2019

But, there are some case where a client need base path.

Then, we should let the user use uri? Any thought on this @hyangtack?
Anyway, we can still remove the path in HttpClientBuilder.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.