Skip to content

Commit

Permalink
Make 'none+' serialization format as optional. (line#2241)
Browse files Browse the repository at this point in the history
Motivation:

Improve API UX, by not forcing application to provide none+ serialization format.

Modifications:

- Update `Scheme.parse()` and `tryParse()` to default to `none+`, if no serialization format is passed.
- Update `DefaultClientFactory` to normalize URI by removing `none+` from URI scheme.

Result:

- Fixes line#2219
  • Loading branch information
rmohta authored and trustin committed Nov 12, 2019
1 parent 1507205 commit b44cd8e
Show file tree
Hide file tree
Showing 6 changed files with 146 additions and 15 deletions.
Expand Up @@ -96,18 +96,17 @@ public final <T> T newClient(Scheme scheme, Endpoint endpoint, @Nullable String
protected final Scheme validateScheme(URI uri) {
requireNonNull(uri, "uri");

final String scheme = uri.getScheme();
if (scheme == null) {
throw new IllegalArgumentException("URI with missing scheme: " + uri);
}

if (uri.getAuthority() == null) {
throw new IllegalArgumentException("URI with missing authority: " + uri);
}

final String scheme = uri.getScheme();
if (scheme == null) {
throw new IllegalArgumentException("URI with missing scheme: " + uri);
}
final Optional<Scheme> parsedSchemeOpt = Scheme.tryParse(scheme);
if (!parsedSchemeOpt.isPresent()) {
throw new IllegalArgumentException("URI with unknown scheme: " + uri);
throw new IllegalArgumentException("URI with undefined scheme: " + uri);
}

final Scheme parsedScheme = parsedSchemeOpt.get();
Expand Down
Expand Up @@ -16,6 +16,8 @@

package com.linecorp.armeria.client;

import static com.linecorp.armeria.client.HttpClientBuilder.isUndefinedUri;

import java.net.URI;
import java.util.ArrayList;
import java.util.List;
Expand All @@ -35,6 +37,7 @@
import com.google.common.collect.Streams;

import com.linecorp.armeria.common.Scheme;
import com.linecorp.armeria.common.SerializationFormat;
import com.linecorp.armeria.common.SessionProtocol;
import com.linecorp.armeria.common.util.ReleasableHolder;

Expand Down Expand Up @@ -128,6 +131,7 @@ public void setMeterRegistry(MeterRegistry meterRegistry) {
@Override
public <T> T newClient(URI uri, Class<T> clientType, ClientOptions options) {
final Scheme scheme = validateScheme(uri);
uri = normalizeUri(uri, scheme);
return clientFactories.get(scheme).newClient(uri, clientType, options);
}

Expand Down Expand Up @@ -171,4 +175,20 @@ public void close() {
void doClose() {
clientFactoriesToClose.forEach(ClientFactory::close);
}

private URI normalizeUri(URI uri, Scheme scheme) {
if (isUndefinedUri(uri)) {
// We use a special singleton marker URI for clients that do not explicitly define a
// host or scheme at construction time.
// As this isn't created by users, we don't need to normalize it.
return uri;
}

if (scheme.serializationFormat() != SerializationFormat.NONE) {
return uri;
}

return URI.create(scheme.sessionProtocol().uriText() +
uri.toString().substring(uri.getScheme().length()));
}
}
Expand Up @@ -38,7 +38,7 @@ public final class HttpClientBuilder extends AbstractClientOptionsBuilder<HttpCl
/**
* An undefined {@link URI} to create {@link HttpClient} without specifying {@link URI}.
*/
private static final URI UNDEFINED_URI = URI.create("none+http://undefined");
private static final URI UNDEFINED_URI = URI.create("http://undefined");

/**
* Returns {@code true} if the specified {@code uri} is an undefined {@link URI}.
Expand Down
16 changes: 8 additions & 8 deletions core/src/main/java/com/linecorp/armeria/common/Scheme.java
Expand Up @@ -78,8 +78,12 @@ public static Optional<Scheme> tryParse(@Nullable String scheme) {
if (scheme == null) {
return Optional.empty();
}

return Optional.ofNullable(SCHEMES.get(Ascii.toLowerCase(scheme)));
final String lowercaseScheme = Ascii.toLowerCase(scheme);
final Scheme parsedScheme = SCHEMES.get(lowercaseScheme);
if (parsedScheme != null) {
return Optional.of(parsedScheme);
}
return Optional.ofNullable(SCHEMES.get(SerializationFormat.NONE.uriText() + '+' + lowercaseScheme));
}

/**
Expand All @@ -90,12 +94,8 @@ public static Optional<Scheme> tryParse(@Nullable String scheme) {
* there is no such {@link Scheme} available
*/
public static Scheme parse(String scheme) {
final Scheme res = SCHEMES.get(Ascii.toLowerCase(requireNonNull(scheme, "scheme")));
if (res == null) {
throw new IllegalArgumentException("scheme: " + scheme);
}

return res;
final Optional<Scheme> parsedScheme = tryParse(requireNonNull(scheme, "scheme"));
return parsedScheme.orElseThrow(() -> new IllegalArgumentException("scheme: " + scheme));
}

/**
Expand Down
@@ -0,0 +1,48 @@
/*
* Copyright 2019 LINE Corporation
*
* LINE Corporation licenses this file to you under the Apache License,
* version 2.0 (the "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at:
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
* License for the specific language governing permissions and limitations
* under the License.
*/

package com.linecorp.armeria.client;

import static org.assertj.core.api.Assertions.assertThat;

import java.net.MalformedURLException;
import java.net.URI;

import org.junit.jupiter.api.Test;

/**
* Test for {@link ClientBuilder}.
*/
class ClientBuilderTest {

@Test
void nonePlusSchemeProvided() {
final HttpClient client = new ClientBuilder("none+https://google.com/").build(HttpClient.class);
assertThat(client.uri().toString()).isEqualTo("https://google.com/");
}

@Test
void nonePlusSchemeUriToUrl() throws MalformedURLException {
final HttpClient client = new ClientBuilder("none+https://google.com/").build(HttpClient.class);
assertThat(client.uri().toURL()).isEqualTo(URI.create("https://google.com/").toURL());
}

@Test
void noSchemeShouldDefaultToNone() {
final HttpClient client = new ClientBuilder("https://google.com/").build(HttpClient.class);
assertThat(client.uri().toString()).isEqualTo("https://google.com/");
}
}
64 changes: 64 additions & 0 deletions core/src/test/java/com/linecorp/armeria/common/SchemeTest.java
@@ -0,0 +1,64 @@
/*
* Copyright 2019 LINE Corporation
*
* LINE Corporation licenses this file to you under the Apache License,
* version 2.0 (the "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at:
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
* License for the specific language governing permissions and limitations
* under the License.
*/

package com.linecorp.armeria.common;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatCode;

import java.util.Optional;

import org.junit.jupiter.api.Test;

/**
* Test for {@link Scheme}.
*/
class SchemeTest {

@Test
public void tryParse_null() {
assertThat(Scheme.tryParse(null)).isEmpty();
}

@Test
void tryParse_add_none() {
final Optional<Scheme> got = Scheme.tryParse("http");
assertThat(got).hasValueSatisfying(scheme -> {
assertThat(scheme.sessionProtocol()).isEqualTo(SessionProtocol.HTTP);
assertThat(scheme.serializationFormat()).isEqualTo(SerializationFormat.NONE);
});
}

@Test
void tryParse_with_none() {
final Optional<Scheme> got = Scheme.tryParse("http+none");
assertThat(got).hasValueSatisfying(scheme -> {
assertThat(scheme.sessionProtocol()).isEqualTo(SessionProtocol.HTTP);
assertThat(scheme.serializationFormat()).isEqualTo(SerializationFormat.NONE);
});
}

@Test
void parse_null() {
assertThatCode(() -> Scheme.parse(null)).isInstanceOf(NullPointerException.class);
}

@Test
void parse_exception() {
assertThatCode(() -> Scheme.parse("http+blah")).isInstanceOf(IllegalArgumentException.class)
.hasMessageContaining("scheme: http+blah");
}
}

0 comments on commit b44cd8e

Please sign in to comment.