Skip to content

Commit

Permalink
Allow specifying no default port when building PropertiesEndpointGroup
Browse files Browse the repository at this point in the history 
Motivation:

A user does not always want to specify the default port number when
building a PropertiesEndpointGroup, especially for HTTP and HTTPS
endpoints that run on their default ports.

Modifications:

- Add two overloaded factory methods that do not require 'defaultPort'
- Fix off-by-one validation bug with 'defaultPort'
- Overall clean-up on Javadoc and exception messages

Result:

- A user can build PropertiesEndpointGroup without defaultPort
- Specifying 0 as defaultPort will trigger an exception at earlier
  point.
  • Loading branch information
@trustin
trustin committed Feb 6, 2018
1 parent 4042934 commit 5d758c4
Show file tree
Hide file tree
Showing 3 changed files with 106 additions and 19 deletions.
8 changes: 5 additions & 3 deletions core/src/main/java/com/linecorp/armeria/client/Endpoint.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,11 @@

package com.linecorp.armeria.client;

import static com.google.common.base.Preconditions.checkArgument;
import static java.util.Objects.requireNonNull;

import javax.annotation.Nullable;

import com.google.common.net.HostAndPort;

import com.linecorp.armeria.client.endpoint.EndpointGroupRegistry;
Expand Down Expand Up @@ -93,6 +96,7 @@ public static Endpoint of(String host, int port, int weight) {
private final String host;
private final int port;
private final int weight;
@Nullable
private String authority;

private Endpoint(String groupName) {
Expand Down Expand Up @@ -250,9 +254,7 @@ private void ensureSingle() {
}

private static void validatePort(String name, int port) {
if (port <= 0 || port > 65535) {
throw new IllegalArgumentException(name + ": " + port + " (expected: 1-65535)");
}
checkArgument(port > 0 && port <= 65535, "%s: %s (expected: 1-65535)", name, port);
}

private static void validateWeight(int weight) {
Expand Down
87 changes: 75 additions & 12 deletions core/src/main/java/com/linecorp/armeria/client/endpoint/PropertiesEndpointGroup.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,11 @@
/**
* A {@link Properties} backed {@link EndpointGroup}. The list of {@link Endpoint}s are loaded from the
* {@link Properties}.
* TODO(ide) Reload the endpoint list if the file is updated.
*/
public final class PropertiesEndpointGroup implements EndpointGroup {

// TODO(ide) Reload the endpoint list if the file is updated.

/**
* Creates a new {@link EndpointGroup} instance that loads the host names (or IP address) and the port
* numbers of the {@link Endpoint} from the {@code resourceName} resource file. The resource file must
Expand All @@ -49,13 +51,40 @@ public final class PropertiesEndpointGroup implements EndpointGroup {
* example.hosts.2=example3.com:36462
* }</pre>
*
* @param resourceName the resource file name that the list of {@link Endpoint}s loaded from
* @param resourceName the name of the resource where the list of {@link Endpoint}s is loaded from
* @param endpointKeyPrefix the property name prefix
*
* @throws IllegalArgumentException if failed to load any hosts from the specified resource file
*/
public static PropertiesEndpointGroup of(ClassLoader classLoader, String resourceName,
String endpointKeyPrefix) {
return new PropertiesEndpointGroup(loadEndpoints(
requireNonNull(classLoader, "classLoader"),
requireNonNull(resourceName, "resourceName"),
requireNonNull(endpointKeyPrefix, "endpointKeyPrefix"),
0));
}

/**
* Creates a new {@link EndpointGroup} instance that loads the host names (or IP address) and the port
* numbers of the {@link Endpoint} from the {@code resourceName} resource file. The resource file must
* contain at least one property whose name starts with {@code endpointKeyPrefix}:
*
* <pre>{@code
* example.hosts.0=example1.com:36462
* example.hosts.1=example2.com:36462
* example.hosts.2=example3.com:36462
* }</pre>
*
* @param resourceName the name of the resource where the list of {@link Endpoint}s is loaded from
* @param endpointKeyPrefix the property name prefix
* @param defaultPort the default port number to use
*
* @throws IllegalArgumentException if failed to load any hosts from the specified resource file
*/
public static PropertiesEndpointGroup of(ClassLoader classLoader, String resourceName,
String endpointKeyPrefix, int defaultPort) {
checkArgument(defaultPort >= 0 && defaultPort <= 65535,
"defaultPort(%s) must be between 0 and 65535", defaultPort);
validateDefaultPort(defaultPort);
return new PropertiesEndpointGroup(loadEndpoints(
requireNonNull(classLoader, "classLoader"),
requireNonNull(resourceName, "resourceName"),
Expand All @@ -74,20 +103,48 @@ public static PropertiesEndpointGroup of(ClassLoader classLoader, String resourc
* example.hosts.2=example3.com:36462
* }</pre>
*
* @param properties the {@link Properties} that the list of {@link Endpoint}s loaded from
* @param properties the {@link Properties} where the list of {@link Endpoint}s is loaded from
* @param endpointKeyPrefix the property name prefix
*
* @throws IllegalArgumentException if failed to load any hosts from the specified {@link Properties}
*/
public static PropertiesEndpointGroup of(Properties properties, String endpointKeyPrefix) {
return new PropertiesEndpointGroup(loadEndpoints(
requireNonNull(properties, "properties"),
requireNonNull(endpointKeyPrefix, "endpointKeyPrefix"),
0));
}

/**
* Creates a new {@link EndpointGroup} instance that loads the host names (or IP address) and the port
* numbers of the {@link Endpoint} from the {@link Properties}. The {@link Properties} must contain at
* least one property whose name starts with {@code endpointKeyPrefix}:
*
* <pre>{@code
* example.hosts.0=example1.com:36462
* example.hosts.1=example2.com:36462
* example.hosts.2=example3.com:36462
* }</pre>
*
* @param properties the {@link Properties} where the list of {@link Endpoint}s is loaded from
* @param endpointKeyPrefix the property name prefix
* @param defaultPort the default port number to use
*
* @throws IllegalArgumentException if failed to load any hosts from the specified {@link Properties}
*/
public static PropertiesEndpointGroup of(Properties properties, String endpointKeyPrefix,
int defaultPort) {
return new PropertiesEndpointGroup(loadEndpoints(properties, endpointKeyPrefix, defaultPort));
validateDefaultPort(defaultPort);
return new PropertiesEndpointGroup(loadEndpoints(
requireNonNull(properties, "properties"),
requireNonNull(endpointKeyPrefix, "endpointKeyPrefix"),
defaultPort));
}

private static List<Endpoint> loadEndpoints(ClassLoader classLoader, String resourceName,
String endpointKeyPrefix, int defaultPort) {
final URL resourceUrl = classLoader.getResource(resourceName);
if (resourceUrl == null) {
throw new IllegalArgumentException(resourceName + " not found");
}
checkArgument(resourceUrl != null, "resource not found: %s", resourceName);
if (endpointKeyPrefix.endsWith(".")) {
endpointKeyPrefix += ".";
}
Expand All @@ -110,14 +167,20 @@ private static List<Endpoint> loadEndpoints(Properties properties, String endpoi
if (key.startsWith(endpointKeyPrefix)) {
final Endpoint endpoint = Endpoint.parse(value);
checkState(!endpoint.isGroup(),
"%s contains an endpoint group which is not allowed: %s", properties, value);
newEndpoints.add(endpoint.withDefaultPort(defaultPort));
"properties contains an endpoint group which is not allowed: %s in %s",
value, properties);
newEndpoints.add(defaultPort == 0 ? endpoint : endpoint.withDefaultPort(defaultPort));
}
}
checkArgument(!newEndpoints.isEmpty(), "%s contains no hosts.", properties);
checkArgument(!newEndpoints.isEmpty(), "properties contains no hosts: %s", properties);
return ImmutableList.copyOf(newEndpoints);
}

private static void validateDefaultPort(int defaultPort) {
checkArgument(defaultPort > 0 && defaultPort <= 65535,
"defaultPort: %s (expected: 1-65535)", defaultPort);
}

private final List<Endpoint> endpoints;

private PropertiesEndpointGroup(List<Endpoint> endpoints) {
Expand Down
30 changes: 26 additions & 4 deletions core/src/test/java/com/linecorp/armeria/client/endpoint/PropertiesEndpointGroupTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,20 +25,42 @@

public class PropertiesEndpointGroupTest {
@Test
public void test() {
public void testWithoutDefaultPort() {
PropertiesEndpointGroup endpointGroup = PropertiesEndpointGroup.of(
getClass().getClassLoader(), "server-list.properties", "serverA.hosts");

assertThat(endpointGroup.endpoints()).containsOnly(Endpoint.of("127.0.0.1:8080"),
Endpoint.of("127.0.0.1:8081"),
Endpoint.of("127.0.0.1"));
}

@Test
public void testWithDefaultPort() {
PropertiesEndpointGroup endpointGroupA = PropertiesEndpointGroup.of(
this.getClass().getClassLoader(), "server-list.properties", "serverA.hosts", 80);
getClass().getClassLoader(), "server-list.properties", "serverA.hosts", 80);
PropertiesEndpointGroup endpointGroupB = PropertiesEndpointGroup.of(
this.getClass().getClassLoader(), "server-list.properties", "serverB.hosts", 8080);
getClass().getClassLoader(), "server-list.properties", "serverB.hosts", 8080);

assertThat(endpointGroupA.endpoints()).containsOnly(Endpoint.of("127.0.0.1:8080"),
Endpoint.of("127.0.0.1:8081"),
Endpoint.of("127.0.0.1:80"));
assertThat(endpointGroupB.endpoints()).containsOnly(Endpoint.of("127.0.0.1:8082"),
Endpoint.of("127.0.0.1:8083"));
}

@Test
public void containsNoHosts() {
assertThatThrownBy(() -> PropertiesEndpointGroup.of(
this.getClass().getClassLoader(), "server-list.properties", "serverC.hosts", 8080))
getClass().getClassLoader(), "server-list.properties", "serverC.hosts", 8080))
.isInstanceOf(IllegalArgumentException.class)
.hasMessageContaining("contains no hosts");
}

@Test
public void illegalDefaultPort() {
assertThatThrownBy(() -> PropertiesEndpointGroup.of(
getClass().getClassLoader(), "server-list.properties", "serverA.hosts", 0))
.isInstanceOf(IllegalArgumentException.class)
.hasMessageContaining("defaultPort");
}
}

0 comments on commit 5d758c4

Please sign in to comment.