Restructure the way new-style functions are loaded.

This shares more code between the HTTP and Background flavours,
and improves the logic for guessing the flavour when it is not specified.
It also gives a better exception when the named target class does not exist.

Also replace some uses of deprecated methods in one of the test functions.

PiperOrigin-RevId: 293924171

Author: eamonnmcmanus

invoker/core/src/main/java/com/google/cloud/functions/invoker/NewBackgroundFunctionExecutor.java

@@ -53,34 +53,25 @@ private NewBackgroundFunctionExecutor(FunctionExecutor<?> functionExecutor) {
   }
 
   /**
-   * Make a {@link NewBackgroundFunctionExecutor} for the class named by the given {@code target}.
-   * If the class cannot be loaded, we currently assume that this is an old-style function
-   * (specified as package.Class.method instead of package.Class) and return
-   * {@code Optional.empty()}.
+   * Makes a {@link NewHttpFunctionExecutor} for the given class.
    *
-   * @throws RuntimeException if we succeed in loading the class named by {@code target} but then
-   *    either the class does not implement {@link RawBackgroundFunction} or we are unable to
-   *    construct an instance using its no-arg constructor.
+   * @throws RuntimeException if either the class does not implement one of
+   *    {@link BackgroundFunction} or {@link RawBackgroundFunction},
+   *    or we are unable to construct an instance using its no-arg constructor.
    */
-  public static Optional<NewBackgroundFunctionExecutor> forTarget(
-      String target, ClassLoader loader) {
-    Class<?> c;
-    try {
-      c = loader.loadClass(target);
-    } catch (ClassNotFoundException e) {
-      return Optional.empty();
-    }
-    if (!BackgroundFunction.class.isAssignableFrom(c)
-        && !RawBackgroundFunction.class.isAssignableFrom(c)) {
+  public static NewBackgroundFunctionExecutor forClass(Class<?> functionClass) {
+    if (!BackgroundFunction.class.isAssignableFrom(functionClass)
+        && !RawBackgroundFunction.class.isAssignableFrom(functionClass)) {
       throw new RuntimeException(
-          "Class " + c.getName() + " implements neither " + BackgroundFunction.class
+          "Class " + functionClass.getName() + " implements neither " + BackgroundFunction.class
               .getName() + " nor " + RawBackgroundFunction.class.getName());
     }
     Object instance;
     try {
-      instance = c.getConstructor().newInstance();
+      instance = functionClass.getConstructor().newInstance();
     } catch (ReflectiveOperationException e) {
-      throw new RuntimeException("Could not construct an instance of " + target + ": " + e, e);
+      throw new RuntimeException(
+          "Could not construct an instance of " + functionClass.getName() + ": " + e, e);
     }
     FunctionExecutor<?> executor;
     if (instance instanceof RawBackgroundFunction) {
@@ -99,7 +90,7 @@ public static Optional<NewBackgroundFunctionExecutor> forTarget(
       }
       executor = new TypedFunctionExecutor<>(maybeTargetType.get(), backgroundFunction);
     }
-    return Optional.of(new NewBackgroundFunctionExecutor(executor));
+    return new NewBackgroundFunctionExecutor(executor);
   }
 
   /**

invoker/core/src/main/java/com/google/cloud/functions/invoker/NewHttpFunctionExecutor.java

@@ -18,7 +18,6 @@
 import com.google.cloud.functions.invoker.http.HttpRequestImpl;
 import com.google.cloud.functions.invoker.http.HttpResponseImpl;
 import java.io.IOException;
-import java.util.Optional;
 import java.util.logging.Level;
 import java.util.logging.Logger;
 import javax.servlet.http.HttpServlet;
@@ -36,42 +35,24 @@ private NewHttpFunctionExecutor(HttpFunction function) {
   }
 
   /**
-   * Make a {@link NewHttpFunctionExecutor} for the class named by the given {@code target}.
-   * If the class cannot be loaded, we currently assume that this is an old-style function
-   * (specified as package.Class.method instead of package.Class) and return
-   * {@code Optional.empty()}.
+   * Makes a {@link NewHttpFunctionExecutor} for the given class.
    *
-   * @throws RuntimeException if we succeed in loading the class named by {@code target} but then
-   *    either the class does not implement {@link HttpFunction} or we are unable to construct an
-   *    instance using its no-arg constructor.
+   * @throws RuntimeException if either the given class does not implement {@link HttpFunction}
+   *    or we are unable to construct an instance using its no-arg constructor.
    */
-  public static Optional<NewHttpFunctionExecutor> forTarget(String target, ClassLoader loader) {
-    Class<?> c;
-    while (true) {
-      try {
-        c = loader.loadClass(target);
-        break;
-      } catch (ClassNotFoundException e) {
-        // This might be a nested class like com.example.Foo.Bar. That will actually appear as
-        // com.example.Foo$Bar as far as Class.forName is concerned. So we try to replace every dot
-        // from the last to the first with a $ in the hope of finding a class we can load.
-        int lastDot = target.lastIndexOf('.');
-        if (lastDot < 0) {
-          return Optional.empty();
-        }
-        target = target.substring(0, lastDot) + '$' + target.substring(lastDot + 1);
-      }
-    }
-    if (!HttpFunction.class.isAssignableFrom(c)) {
+  public static NewHttpFunctionExecutor forClass(Class<?> functionClass) {
+    if (!HttpFunction.class.isAssignableFrom(functionClass)) {
       throw new RuntimeException(
-          "Class " + c.getName() + " does not implement " + HttpFunction.class.getName());
+          "Class " + functionClass.getName() + " does not implement "
+              + HttpFunction.class.getName());
     }
-    Class<? extends HttpFunction> httpFunctionClass = c.asSubclass(HttpFunction.class);
+    Class<? extends HttpFunction> httpFunctionClass = functionClass.asSubclass(HttpFunction.class);
     try {
       HttpFunction httpFunction = httpFunctionClass.getConstructor().newInstance();
-      return Optional.of(new NewHttpFunctionExecutor(httpFunction));
+      return new NewHttpFunctionExecutor(httpFunction);
     } catch (ReflectiveOperationException e) {
-      throw new RuntimeException("Could not construct an instance of " + target + ": " + e, e);
+      throw new RuntimeException(
+          "Could not construct an instance of " + functionClass.getName() + ": " + e, e);
     }
   }
 

invoker/core/src/main/java/com/google/cloud/functions/invoker/runner/Invoker.java

@@ -19,6 +19,9 @@
 import com.beust.jcommander.JCommander;
 import com.beust.jcommander.Parameter;
 import com.beust.jcommander.ParameterException;
+import com.google.cloud.functions.BackgroundFunction;
+import com.google.cloud.functions.HttpFunction;
+import com.google.cloud.functions.RawBackgroundFunction;
 import com.google.cloud.functions.invoker.BackgroundCloudFunction;
 import com.google.cloud.functions.invoker.BackgroundFunctionExecutor;
 import com.google.cloud.functions.invoker.BackgroundFunctionSignatureMatcher;
@@ -48,6 +51,7 @@
 import java.util.logging.Level;
 import java.util.logging.LogManager;
 import java.util.logging.Logger;
+import java.util.stream.Stream;
 import javax.servlet.http.HttpServlet;
 import org.eclipse.jetty.server.Server;
 import org.eclipse.jetty.servlet.ServletContextHandler;
@@ -163,12 +167,13 @@ static Optional<Invoker> makeInvoker(Map<String, String> environment, String...
             .stream()
             .filter(Objects::nonNull)
             .findFirst();
+    ClassLoader functionClassLoader = makeClassLoader(functionClasspath);
     Invoker invoker =
         new Invoker(
             port,
             functionTarget,
             environment.get("FUNCTION_SIGNATURE_TYPE"),
-            functionClasspath);
+            functionClassLoader);
     return Optional.of(invoker);
   }
 
@@ -185,20 +190,29 @@ private static boolean isLocalRun() {
     return System.getenv("K_SERVICE") == null;
   }
 
+  private static ClassLoader makeClassLoader(Optional<String> functionClasspath) {
+    ClassLoader runtimeLoader = Invoker.class.getClassLoader();
+    if (functionClasspath.isPresent()) {
+      ClassLoader parent = new OnlyApiClassLoader(runtimeLoader);
+      return new URLClassLoader(classpathToUrls(functionClasspath.get()), parent);
+    }
+    return runtimeLoader;
+  }
+
   private final Integer port;
   private final String functionTarget;
   private final String functionSignatureType;
-  private final Optional<String> functionClasspath;
+  private final ClassLoader functionClassLoader;
 
   public Invoker(
       Integer port,
       String functionTarget,
       String functionSignatureType,
-      Optional<String> functionClasspath) {
+      ClassLoader functionClassLoader) {
     this.port = port;
     this.functionTarget = functionTarget;
     this.functionSignatureType = functionSignatureType;
-    this.functionClasspath = functionClasspath;
+    this.functionClassLoader = functionClassLoader;
   }
 
   Integer getPort() {
@@ -213,8 +227,8 @@ String getFunctionSignatureType() {
     return functionSignatureType;
   }
 
-  Optional<String> getFunctionClasspath() {
-    return functionClasspath;
+  ClassLoader getFunctionClassLoader() {
+    return functionClassLoader;
   }
 
   public void startServer() throws Exception {
@@ -224,58 +238,34 @@ public void startServer() throws Exception {
     context.setContextPath("/");
     server.setHandler(context);
 
-    ClassLoader runtimeLoader = getClass().getClassLoader();
-    ClassLoader classLoader;
-    if (functionClasspath.isPresent()) {
-      ClassLoader parent = new OnlyApiClassLoader(runtimeLoader);
-      classLoader = new URLClassLoader(classpathToUrls(functionClasspath.get()), parent);
-    } else {
-      classLoader = runtimeLoader;
-    }
+    Optional<Class<?>> functionClass = loadFunctionClass();
 
     HttpServlet servlet;
     if ("http".equals(functionSignatureType)) {
-      Optional<NewHttpFunctionExecutor> newExecutor =
-          NewHttpFunctionExecutor.forTarget(functionTarget, classLoader);
-      if (newExecutor.isPresent()) {
-        servlet = newExecutor.get();
+      if (functionClass.isPresent()) {
+        servlet = NewHttpFunctionExecutor.forClass(functionClass.get());
       } else {
-        FunctionLoader<HttpCloudFunction> loader =
-            new FunctionLoader<>(functionTarget, classLoader, new HttpFunctionSignatureMatcher());
+        FunctionLoader<HttpCloudFunction> loader = new FunctionLoader<>(
+            functionTarget, functionClassLoader, new HttpFunctionSignatureMatcher());
         HttpCloudFunction function = loader.loadUserFunction();
         servlet = new HttpFunctionExecutor(function);
       }
     } else if ("event".equals(functionSignatureType)) {
-      Optional<NewBackgroundFunctionExecutor> newExecutor =
-          NewBackgroundFunctionExecutor.forTarget(functionTarget, classLoader);
-      if (newExecutor.isPresent()) {
-        servlet = newExecutor.get();
+      if (functionClass.isPresent()) {
+        servlet = NewBackgroundFunctionExecutor.forClass(functionClass.get());
       } else {
         FunctionLoader<BackgroundCloudFunction> loader =
             new FunctionLoader<>(
-                functionTarget, classLoader, new BackgroundFunctionSignatureMatcher());
+                functionTarget, functionClassLoader, new BackgroundFunctionSignatureMatcher());
         BackgroundCloudFunction function = loader.loadUserFunction();
         servlet = new BackgroundFunctionExecutor(function);
       }
     } else if (functionSignatureType == null) {
-      Optional<NewHttpFunctionExecutor> httpExecutor =
-          NewHttpFunctionExecutor.forTarget(functionTarget, classLoader);
-      if (httpExecutor.isPresent()) {
-        servlet = httpExecutor.get();
+      if (functionClass.isPresent()) {
+        servlet = servletForDeducedSignatureType(functionClass.get());
       } else {
-        Optional<NewBackgroundFunctionExecutor> backgroundExecutor =
-            NewBackgroundFunctionExecutor.forTarget(functionTarget, classLoader);
-        if (backgroundExecutor.isPresent()) {
-          servlet = backgroundExecutor.get();
-        } else {
-          String error = String.format(
-              "Could not determine function signature type from target %s. Either this should be"
-              + " a class implementing one of the interfaces in com.google.cloud.functions, or the"
-              + " environment variable FUNCTION_SIGNATURE_TYPE should be set to \"http\" or"
-              + " \"event\".",
-              functionTarget);
-          throw new RuntimeException(error);
-        }
+        throw new RuntimeException(
+            "Class " + functionTarget + " does not exist or could not be loaded");
       }
     } else {
       String error = String.format(
@@ -290,28 +280,71 @@ public void startServer() throws Exception {
     server.join();
   }
 
-  static URL[] classpathToUrls(String classpath) throws IOException {
+  private Optional<Class<?>> loadFunctionClass() {
+    String target = functionTarget;
+    while (true) {
+      try {
+        return Optional.of(functionClassLoader.loadClass(target));
+      } catch (ClassNotFoundException e) {
+        // This might be a nested class like com.example.Foo.Bar. That will actually appear as
+        // com.example.Foo$Bar as far as Class.forName is concerned. So we try to replace every dot
+        // from the last to the first with a $ in the hope of finding a class we can load.
+        int lastDot = target.lastIndexOf('.');
+        if (lastDot < 0) {
+          return Optional.empty();
+        }
+        target = target.substring(0, lastDot) + '$' + target.substring(lastDot + 1);
+      }
+    }
+  }
+
+  private HttpServlet servletForDeducedSignatureType(Class<?> functionClass) {
+    if (HttpFunction.class.isAssignableFrom(functionClass)) {
+      return NewHttpFunctionExecutor.forClass(functionClass);
+    }
+    if (BackgroundFunction.class.isAssignableFrom(functionClass)
+        || RawBackgroundFunction.class.isAssignableFrom(functionClass)) {
+      return NewBackgroundFunctionExecutor.forClass(functionClass);
+    }
+    String error = String.format(
+        "Could not determine function signature type from target %s. Either this should be"
+            + " a class implementing one of the interfaces in com.google.cloud.functions, or the"
+            + " environment variable FUNCTION_SIGNATURE_TYPE should be set to \"http\" or"
+            + " \"event\".",
+        functionTarget);
+    throw new RuntimeException(error);
+  }
+
+  static URL[] classpathToUrls(String classpath) {
     String[] components = classpath.split(File.pathSeparator);
     List<URL> urls = new ArrayList<>();
     for (String component : components) {
       if (component.endsWith(File.separator + "*")) {
         urls.addAll(jarsIn(component.substring(0, component.length() - 2)));
       } else {
         Path path = Paths.get(component);
-        if (Files.exists(path)) {
+        try {
           urls.add(path.toUri().toURL());
+        } catch (MalformedURLException e) {
+          throw new UncheckedIOException(e);
         }
       }
     }
     return urls.toArray(new URL[0]);
   }
 
-  private static List<URL> jarsIn(String dir) throws IOException {
+  private static List<URL> jarsIn(String dir) {
     Path path = Paths.get(dir);
     if (!Files.isDirectory(path)) {
       return Collections.emptyList();
     }
-    return Files.list(path)
+    Stream<Path> stream;
+    try {
+      stream = Files.list(path);
+    } catch (IOException e) {
+      throw new UncheckedIOException(e);
+    }
+    return stream
         .filter(p -> p.getFileName().toString().endsWith(".jar"))
         .map(p -> {
           try {

invoker/core/src/test/java/com/google/cloud/functions/invoker/runner/InvokerTest.java

@@ -3,17 +3,20 @@
 import static com.google.common.truth.Truth.assertThat;
 import static com.google.common.truth.Truth.assertWithMessage;
 import static com.google.common.truth.Truth8.assertThat;
+import static java.util.stream.Collectors.joining;
 
 import java.io.ByteArrayOutputStream;
 import java.io.File;
 import java.io.IOException;
 import java.io.PrintStream;
 import java.net.URL;
+import java.net.URLClassLoader;
 import java.nio.charset.StandardCharsets;
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.Map;
 import java.util.Optional;
+import java.util.stream.Collectors;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
@@ -71,7 +74,8 @@ public void explicitSignatureType() {
   @Test
   public void defaultClasspath() {
     Optional<Invoker> invoker = Invoker.makeInvoker();
-    assertThat(invoker.get().getFunctionClasspath()).isEmpty();
+    assertThat(invoker.get().getClass().getClassLoader())
+        .isSameInstanceAs(Invoker.class.getClassLoader());
   }
 
   private static final String FAKE_CLASSPATH =
@@ -81,13 +85,20 @@ public void defaultClasspath() {
   public void explicitClasspathViaEnvironment() {
     Map<String, String> env = Collections.singletonMap("FUNCTION_CLASSPATH", FAKE_CLASSPATH);
     Optional<Invoker> invoker = Invoker.makeInvoker(env);
-    assertThat(invoker.get().getFunctionClasspath()).hasValue(FAKE_CLASSPATH);
+    assertThat(invokerClasspath(invoker.get())).isEqualTo(FAKE_CLASSPATH);
   }
 
   @Test
   public void explicitClasspathViaOption() {
     Optional<Invoker> invoker = Invoker.makeInvoker("--classpath", FAKE_CLASSPATH);
-    assertThat(invoker.get().getFunctionClasspath()).hasValue(FAKE_CLASSPATH);
+    assertThat(invokerClasspath(invoker.get())).isEqualTo(FAKE_CLASSPATH);
+  }
+
+  private static String invokerClasspath(Invoker invoker) {
+    URLClassLoader urlClassLoader = (URLClassLoader) invoker.getFunctionClassLoader();
+    return Arrays.stream(urlClassLoader.getURLs())
+        .map(URL::getPath)
+        .collect(joining(File.pathSeparator));
   }
 
   @Test