daemon: Remove unnecessary requirement of HttpServletRequest

Brad pointed out that `java -jar gerrit.war daemon` failed because
HttpServletRequest was not in the CLASSPATH.  This was an unfortunate
dependency caused by GerritGlobalModule setting up the CanonicalWebUrl
with a bytecode reference to an optional Provider<HttpServletRequest>.
Although the Provider could be supplied as null, the bytecode did not
load and verify because the HttpServletRequest was not available in
the CLASSPATH.

By refactoring the global module to require the caller to supply the
binding for the CanonicalWebUrl, we can avoid needing to reference the
HttpServletRequest from code intended to run in the daemon mode.

In the future we should refactor the build process so that pure daemon
code does not have visiblity at compile time to the servlet API, thus
making this sort of error visible at compile time, rather than finding
it late at runtime.  Unfortunately this requires creating several POMs
in the Maven build.

Bug: GERRIT-281
Change-Id: Ibea06bfca8f6b2186ee2f6eff297d25318d6c418
Signed-off-by: Shawn O. Pearce <sop@google.com>
CC: Brad Larson <bklarson@gmail.com>

src/main/java/com/google/gerrit/server/config/CanonicalWebUrlModule.java

@@ -0,0 +1,40 @@
+// Copyright (C) 2009 The Android Open Source Project
+//
+// Licensed 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
+//
+// http://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.google.gerrit.server.config;
+
+import static com.google.inject.Scopes.SINGLETON;
+
+import com.google.inject.AbstractModule;
+import com.google.inject.Provider;
+
+/** Supports binding the {@link CanonicalWebUrl} annotation. */
+public abstract class CanonicalWebUrlModule extends AbstractModule {
+  @Override
+  protected void configure() {
+    // Note that the CanonicalWebUrl itself must not be a singleton, but its
+    // provider must be.
+    //
+    // If the value was not configured in the system configuration data the
+    // provider may try to guess it from the current HTTP request, if we are
+    // running in an HTTP environment.
+    //
+    final Class<? extends Provider<String>> provider = provider();
+    bind(provider).in(SINGLETON);
+    bind(String.class).annotatedWith(CanonicalWebUrl.class)
+        .toProvider(provider);
+  }
+
+  protected abstract Class<? extends Provider<String>> provider();
+}

src/main/java/com/google/gerrit/server/config/CanonicalWebUrlProvider.java

@@ -15,21 +15,16 @@
 package com.google.gerrit.server.config;
 
 import com.google.inject.Inject;
-import com.google.inject.OutOfScopeException;
 import com.google.inject.Provider;
-import com.google.inject.ProvisionException;
 
 import org.spearce.jgit.lib.Config;
 
-import javax.servlet.http.HttpServletRequest;
-
-/** Provides {@link String} annotated with {@link CanonicalWebUrl}. */
+/** Provides {@link CanonicalWebUrl} from {@code gerrit.canonicalWebUrl}. */
 public class CanonicalWebUrlProvider implements Provider<String> {
   private final String canonicalUrl;
-  private Provider<HttpServletRequest> requestProvider;
 
   @Inject
-  CanonicalWebUrlProvider(@GerritServerConfig final Config config) {
+  public CanonicalWebUrlProvider(@GerritServerConfig final Config config) {
     String u = config.getString("gerrit", null, "canonicalweburl");
     if (u != null && !u.endsWith("/")) {
       u += "/";
@@ -37,46 +32,8 @@ public class CanonicalWebUrlProvider implements Provider<String> {
     canonicalUrl = u;
   }
 
-  @Inject(optional = true)
-  public void setHttpServletRequest(final Provider<HttpServletRequest> hsr) {
-    requestProvider = hsr;
-  }
-
   @Override
   public String get() {
-    if (canonicalUrl != null) {
-      return canonicalUrl;
-    }
-
-    if (requestProvider != null) {
-      // No canonical URL configured? Maybe we can get a reasonable
-      // guess from the incoming HTTP request, if we are currently
-      // inside of an HTTP request scope.
-      //
-      final HttpServletRequest req;
-      try {
-        req = requestProvider.get();
-      } catch (ProvisionException noWeb) {
-        if (noWeb.getCause() instanceof OutOfScopeException) {
-          // We can't obtain the request as we are not inside of
-          // an HTTP request scope. Callers must handle null.
-          //
-          return null;
-        } else {
-          throw noWeb;
-        }
-      }
-
-      final StringBuffer url = req.getRequestURL();
-      url.setLength(url.length() - req.getServletPath().length());
-      if (url.charAt(url.length() - 1) != '/') {
-        url.append('/');
-      }
-      return url.toString();
-    }
-
-    // We have no way of guessing our HTTP url.
-    //
-    return null;
+    return canonicalUrl;
   }
 }

src/main/java/com/google/gerrit/server/config/GerritGlobalModule.java

@@ -66,6 +66,7 @@ import com.google.inject.Guice;
 import com.google.inject.Inject;
 import com.google.inject.Injector;
 import com.google.inject.Module;
+import com.google.inject.Provider;
 
 import org.spearce.jgit.lib.Config;
 import org.spearce.jgit.lib.PersonIdent;
@@ -76,14 +77,22 @@ import java.util.List;
 /** Starts global state with standard dependencies. */
 public class GerritGlobalModule extends FactoryModule {
   public static Injector createInjector() {
-    return createInjector(Guice
-        .createInjector(PRODUCTION, new DatabaseModule()));
+    final Injector db = Guice.createInjector(PRODUCTION, new DatabaseModule());
+    final CanonicalWebUrlModule canonicalWebUrl = new CanonicalWebUrlModule() {
+      @Override
+      protected Class<? extends Provider<String>> provider() {
+        return CanonicalWebUrlProvider.class;
+      }
+    };
+    return createInjector(db, canonicalWebUrl);
   }
 
-  public static Injector createInjector(final Injector db) {
+  public static Injector createInjector(final Injector db,
+      final CanonicalWebUrlModule canonicalWebUrl) {
     final Injector cfg = db.createChildInjector(new GerritConfigModule());
     final List<Module> modules = new ArrayList<Module>();
     modules.add(cfg.getInstance(GerritGlobalModule.class));
+    modules.add(canonicalWebUrl);
     return cfg.createChildInjector(modules);
   }
 
@@ -113,17 +122,6 @@ public class GerritGlobalModule extends FactoryModule {
         SINGLETON);
     bind(AnonymousUser.class);
 
-    // Note that the CanonicalWebUrl itself must not be a singleton, but its
-    // provider must be.
-    //
-    // If the value was not configured in the system configuration data the
-    // provider may try to guess it from the current HTTP request, if we are
-    // running in an HTTP environment.
-    //
-    bind(CanonicalWebUrlProvider.class).in(SINGLETON);
-    bind(String.class).annotatedWith(CanonicalWebUrl.class).toProvider(
-        CanonicalWebUrlProvider.class);
-
     bind(PersonIdent.class).annotatedWith(GerritPersonIdent.class).toProvider(
         GerritPersonIdentProvider.class);
 

src/main/java/com/google/gerrit/server/http/GerritServletConfig.java

@@ -20,7 +20,7 @@ import com.google.gerrit.git.PushAllProjectsOp;
 import com.google.gerrit.git.ReloadSubmitQueueOp;
 import com.google.gerrit.git.WorkQueue;
 import com.google.gerrit.server.cache.CachePool;
-import com.google.gerrit.server.config.CanonicalWebUrlProvider;
+import com.google.gerrit.server.config.CanonicalWebUrlModule;
 import com.google.gerrit.server.config.DatabaseModule;
 import com.google.gerrit.server.config.GerritGlobalModule;
 import com.google.gerrit.server.ssh.SshDaemon;
@@ -30,6 +30,7 @@ import com.google.inject.CreationException;
 import com.google.inject.Guice;
 import com.google.inject.Injector;
 import com.google.inject.Module;
+import com.google.inject.Provider;
 import com.google.inject.ProvisionException;
 import com.google.inject.servlet.GuiceServletContextListener;
 import com.google.inject.spi.Message;
@@ -80,7 +81,14 @@ public class GerritServletConfig extends GuiceServletContextListener {
         throw new CreationException(Collections.singleton(first));
       }
 
-      sysInjector = GerritGlobalModule.createInjector(dbInjector);
+      sysInjector =
+          GerritGlobalModule.createInjector(dbInjector,
+              new CanonicalWebUrlModule() {
+                @Override
+                protected Class<? extends Provider<String>> provider() {
+                  return HttpCanonicalWebUrlProvider.class;
+                }
+              });
       sshInjector = createSshInjector();
       webInjector = createWebInjector();
 
@@ -93,7 +101,7 @@ public class GerritServletConfig extends GuiceServletContextListener {
       // injection here because the HTTP environment is not visible
       // to the core server modules.
       //
-      sysInjector.getInstance(CanonicalWebUrlProvider.class)
+      sysInjector.getInstance(HttpCanonicalWebUrlProvider.class)
           .setHttpServletRequest(
               webInjector.getProvider(HttpServletRequest.class));
     }

src/main/java/com/google/gerrit/server/http/HttpCanonicalWebUrlProvider.java

@@ -0,0 +1,81 @@
+// Copyright (C) 2009 The Android Open Source Project
+//
+// Licensed 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
+//
+// http://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.google.gerrit.server.http;
+
+import com.google.gerrit.server.config.CanonicalWebUrl;
+import com.google.gerrit.server.config.CanonicalWebUrlProvider;
+import com.google.gerrit.server.config.GerritServerConfig;
+import com.google.inject.Inject;
+import com.google.inject.OutOfScopeException;
+import com.google.inject.Provider;
+import com.google.inject.ProvisionException;
+
+import org.spearce.jgit.lib.Config;
+
+import javax.servlet.http.HttpServletRequest;
+
+/** Sets {@link CanonicalWebUrl} to current HTTP request if not configured. */
+class HttpCanonicalWebUrlProvider extends CanonicalWebUrlProvider {
+  private Provider<HttpServletRequest> requestProvider;
+
+  @Inject
+  HttpCanonicalWebUrlProvider(@GerritServerConfig final Config config) {
+    super(config);
+  }
+
+  @Inject(optional = true)
+  void setHttpServletRequest(final Provider<HttpServletRequest> hsr) {
+    requestProvider = hsr;
+  }
+
+  @Override
+  public String get() {
+    String canonicalUrl = super.get();
+    if (canonicalUrl != null) {
+      return canonicalUrl;
+    }
+
+    if (requestProvider != null) {
+      // No canonical URL configured? Maybe we can get a reasonable
+      // guess from the incoming HTTP request, if we are currently
+      // inside of an HTTP request scope.
+      //
+      final HttpServletRequest req;
+      try {
+        req = requestProvider.get();
+      } catch (ProvisionException noWeb) {
+        if (noWeb.getCause() instanceof OutOfScopeException) {
+          // We can't obtain the request as we are not inside of
+          // an HTTP request scope. Callers must handle null.
+          //
+          return null;
+        } else {
+          throw noWeb;
+        }
+      }
+
+      final StringBuffer url = req.getRequestURL();
+      url.setLength(url.length() - req.getServletPath().length());
+      if (url.charAt(url.length() - 1) != '/') {
+        url.append('/');
+      }
+      return url.toString();
+    }
+
+    // We have no way of guessing our HTTP url.
+    //
+    return null;
+  }
+}