Support getting bound local port from Application and avoid using fixed port in unit tests

Author: ewencp

core/src/main/java/io/confluent/rest/Application.java

@@ -19,6 +19,8 @@
 import com.fasterxml.jackson.jaxrs.base.JsonParseExceptionMapper;
 
 import io.confluent.common.config.ConfigException;
+
+import org.eclipse.jetty.server.Connector;
 import org.eclipse.jetty.server.Handler;
 import org.eclipse.jetty.server.NetworkTrafficServerConnector;
 import org.eclipse.jetty.server.Server;
@@ -121,6 +123,12 @@ public Server createServer() throws RestConfigException {
     ServletContainer servletContainer = new ServletContainer(resourceConfig);
     ServletHolder servletHolder = new ServletHolder(servletContainer);
     server = new Server() {
+      @Override
+      protected void doStart() throws Exception {
+        super.doStart();
+        Application.this.onStarted();
+      }
+
       @Override
       protected void doStop() throws Exception {
         super.doStop();
@@ -239,6 +247,26 @@ protected void doStop() throws Exception {
     return server;
   }
 
+  /**
+   * Get the local ports that were bound for the listeners.
+   * @return an List containing the local ports
+   */
+  public List<Integer> localPorts() {
+    List<Integer> ports = new ArrayList<>();
+    for(Connector connector : server.getConnectors()) {
+      if (!(connector instanceof NetworkTrafficServerConnector)) {
+        throw new RuntimeException(
+            String.format(
+                "Expected NetworkTrafficServerConnector for listener but found %s",
+                connector.getClass()
+            )
+        );
+      }
+      ports.add(((NetworkTrafficServerConnector) connector).getLocalPort());
+    }
+    return ports;
+  }
+
   // TODO: delete deprecatedPort parameter when `PORT_CONFIG` is deprecated. It's only used to support the deprecated
   //       configuration.
   public static List<URI> parseListeners(List<String> listenersConfig, int deprecatedPort,
@@ -352,6 +380,10 @@ public void stop() throws Exception {
     server.stop();
   }
 
+  public void onStarted() {
+
+  }
+
   /**
    * Shutdown hook that is invoked after the Jetty server has processed the shutdown request,
    * stopped accepting new connections, and tried to gracefully finish existing requests. At this

core/src/test/java/io/confluent/rest/ApplicationTest.java

@@ -16,15 +16,21 @@
 
 package io.confluent.rest;
 
+import io.confluent.common.config.ConfigDef;
 import io.confluent.common.config.ConfigException;
 import org.junit.Test;
 
 import java.net.URI;
 import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.Collections;
 import java.util.List;
+import java.util.Map;
+
+import javax.ws.rs.core.Configurable;
 
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
 
 public class ApplicationTest {
   @Test
@@ -79,6 +85,48 @@ public void testParseListenersNoPort() {
     List<URI> listeners = Application.parseListeners(listenersConfig, -1, Arrays.asList("http", "https"), "http");
   }
 
+  @Test
+  public void zeroForPortShouldHaveNonZeroLocalPort() throws Exception {
+    TestRestConfig config = new TestRestConfig(
+        Collections.singletonMap(RestConfig.PORT_CONFIG, "0")
+    );
+    TestApplication testApp = new TestApplication(config);
+    testApp.start();
+    List<Integer> localPorts = testApp.localPorts();
+    assertEquals(1, localPorts.size());
+    // Validate not only that it isn't zero, but also a valid value
+    assertTrue("Should have a valid local port value", localPorts.get(0) > 0);
+    testApp.stop();
+    testApp.join();
+  }
+
+  private class TestApplication extends Application<TestRestConfig> {
+    TestApplication(TestRestConfig config) {
+      super(config);
+    }
+
+    @Override
+    public void setupResources(Configurable<?> config, TestRestConfig appConfig) {
+      // intentionally left blank
+    }
+  }
+
+  private static class TestRestConfig extends RestConfig {
+    private static ConfigDef config;
+
+    static {
+      config = baseConfigDef();
+    }
+
+    public TestRestConfig() {
+      super(config);
+    }
+
+    public TestRestConfig(Map<?, ?> props) {
+      super(config, props);
+    }
+  }
+
   private void assertExpectedUri(URI uri, String scheme, String host, int port) {
     assertEquals("Scheme should be " + scheme, scheme, uri.getScheme());
     assertEquals("Host should be " + host, host, uri.getHost());

core/src/test/java/io/confluent/rest/ExceptionHandlingTest.java

@@ -51,6 +51,7 @@ public class ExceptionHandlingTest {
   public void setUp() throws Exception {
     Properties props = new Properties();
     props.setProperty("debug", "false");
+    props.setProperty(RestConfig.PORT_CONFIG, "0");
     config = new TestRestConfig(props);
     app = new ExceptionApplication(config);
     app.start();
@@ -65,7 +66,7 @@ public void tearDown() throws Exception {
   private void testGetException(String path, int expectedStatus, int expectedErrorCode,
       String expectedMessage) {
     Response response = ClientBuilder.newClient(app.resourceConfig.getConfiguration())
-        .target("http://localhost:" + config.getInt(RestConfig.PORT_CONFIG))
+        .target("http://localhost:" + app.localPorts().get(0))
         .path(path)
         .request()
         .get();
@@ -79,7 +80,7 @@ private void testGetException(String path, int expectedStatus, int expectedError
   private void testPostException(String path, Entity entity, int expectedStatus, int expectedErrorCode,
       String expectedMessage) {
     Response response = ClientBuilder.newClient(app.resourceConfig.getConfiguration())
-      .target("http://localhost:" + config.getInt(RestConfig.PORT_CONFIG))
+      .target("http://localhost:" + app.localPorts().get(0))
       .path(path)
       .request()
       .post(entity);

core/src/test/java/io/confluent/rest/ShutdownTest.java

@@ -35,6 +35,7 @@ public class ShutdownTest {
   public void testShutdownHook() throws Exception {
     Properties props = new Properties();
     props.put("shutdown.graceful.ms", "50");
+    props.put(RestConfig.PORT_CONFIG, "0");
     ShutdownApplication app = new ShutdownApplication(new TestRestConfig(props));
     app.start();
 
@@ -49,11 +50,12 @@ public void testShutdownHook() throws Exception {
   public void testGracefulShutdown() throws Exception {
     Properties props = new Properties();
     props.put("shutdown.graceful.ms", "50");
+    props.put(RestConfig.PORT_CONFIG, "0");
     final TestRestConfig config = new TestRestConfig(props);
     ShutdownApplication app = new ShutdownApplication(config);
     app.start();
 
-    RequestThread req = new RequestThread(config);
+    RequestThread req = new RequestThread(app.localPorts().get(0));
     req.start();
     app.resource.requestProcessingStarted.await();
 
@@ -115,12 +117,12 @@ public void run() {
   };
 
   private static class RequestThread extends Thread {
-    RestConfig config;
+    int port;
     boolean finished = false;
     String response = null;
 
-    RequestThread(RestConfig config) {
-      this.config = config;
+    RequestThread(int port) {
+      this.port = port;
     }
     @Override
     public void run() {
@@ -131,7 +133,7 @@ public void run() {
         try {
           Client client = ClientBuilder.newClient();
           response = client
-              .target("http://localhost:" + config.getInt(RestConfig.PORT_CONFIG))
+              .target("http://localhost:" + port)
               .path("/")
               .request()
               .get(String.class);