Armeria: HttpClientBuilder() should accept strings without serialization format

[anuraaga]

Currently, there are two versions of HttpClientBuilder(), one that takes a String and one that takes URI. The latter automatically prepends none+ to the URI but the former will just reject URL strings without a serialization format. It's surprising for these two similar methods to have different accepted input - I think it's better of the String version also automatically adds none+ when no format is specified. HttpClient.of already does this, so this is also confusing distinction between the builder and static factories.

[imasahiro]

Both HttpClient.of(String) and HttpClient.of(URI) finally use same HttpClientBuilder constructor (HttpClientBuilder(URI)). So HttpClient.of(String) accepts string without serialization format, right?

https://github.com/line/armeria/blob/54e124d73178609b4f0af6d833ea562a1ce15bd9/core/src/main/java/com/linecorp/armeria/client/HttpClientBuilder.java#L47-L49

https://github.com/line/armeria/blob/54e124d73178609b4f0af6d833ea562a1ce15bd9/core/src/main/java/com/linecorp/armeria/client/HttpClientBuilder.java#L57-L59

[anuraaga]

Oops - I thought the issue also affected HttpClient.of, sorry for not checking, I was working with HttpClientBuilder. I would expect HttpClientBuilder to have identical behavior as the static methods so changed issue for that.

[anuraaga]

Actually looking at your snippet, I see what you're saying. Hmm, let me find the exception I saw.

[anuraaga]

Caused by: java.lang.IllegalArgumentException: URI with unknown scheme: http://169.254.169.254/
    at com.linecorp.armeria.client.AbstractClientFactory.validateScheme(AbstractClientFactory.java:74)
    at com.linecorp.armeria.client.DefaultClientFactory.newClient(DefaultClientFactory.java:117)
    at com.linecorp.armeria.client.ClientBuilder.build(ClientBuilder.java:88)

[minwoox]

I think HttpClientBuilder() only accepts the uri without serialization format because it automatically adds it. From the logs, do you use the ClientBuilder with a uri which does not have a serialization format?
Do you mean we have to support a case when a user wants to create a client using ClientBuiler() without any serialization format by adding none+ in front of it?

[imasahiro]

Just checked current (Http)?ClientBuilder behavior.

// Pass
new HttpClientBuilder("http://localhost");
// IllegalArgumentException: scheme : none+http 
new HttpClientBuilder("none+http://localhost");
// Pass
new HttpClientBuilder(URI.create("http://localhost"));
// IllegalArgumentException: scheme : none+http 
new HttpClientBuilder(URI.create("none+http://localhost"));

// Pass
new ClientBuilder("none+http://localhost").build(HttpClient.class);
// IllegalArgumentException: URI with unknown scheme
new ClientBuilder("http://localhost").build(HttpClient.class);
// Pass
new ClientBuilder(URI.create("none+http://localhost")).build(HttpClient.class);
// IllegalArgumentException: URI with unknown scheme
new ClientBuilder(URI.create("http://localhost")).build(HttpClient.class);

[anuraaga]

Oops! Got the classes mixed up in my head :( I went from HttpClient to ClientBuilder instead of HttpClientBuilder. I guess ClientBuilder requiring format but not HttpClientBuilder makes sense. Though it might be worth thinking about the many ways of doing the same thing causing confusion in this case.

[anuraaga]

I'll go ahead and close this issue since the reported behavior seems reasonable, though possibly too easy to get mixed up.

[trustin]

Not directly related but perhaps we could tolerate the case where the scheme starts with none+?