inject-core: Allow TwitterModule.install to be used for non-TwitterMo…

…dules Problem TwitterModule.install is currently not supported. This prevents module composition with non-Twitter modules whose construction requires the results of parsed flags (e.g. install(TypesafeConfigModule.fromConfig(createConfigFromFlagValue()))) Solution Allow TwitterModule.install to be used for non-TwitterModules. RB_ID=739582
twitter · Sep 11, 2015 · c14ff12 · c14ff12
1 parent 7e7e797
commit c14ff12
Show file tree

Hide file tree

Showing 3 changed files with 28 additions and 8 deletions.
diff --git a/inject/inject-core/src/main/scala/com/twitter/inject/TwitterBaseModule.scala b/inject/inject-core/src/main/scala/com/twitter/inject/TwitterBaseModule.scala
@@ -11,7 +11,11 @@ trait TwitterBaseModule
   with TwitterModuleLifecycle {
 
   /** Additional modules to be composed into this module
-    * NOTE: This Seq of modules is used instead of the standard Guice 'install' method */
+    *
+    * NOTE: This Seq of modules is generally used instead of the standard Guice 'install' method so that
+    * TwitterModules with flag definitions can be supported.
+    * However, AbstractModule.install can still be used for non-TwitterModules, and is sometimes preferred
+    * due to install being deferred until after flag parsing occurs. */
   protected[inject] def modules: Seq[Module] = Seq()
 
   protected def createKey[T: Manifest] = {

diff --git a/inject/inject-core/src/main/scala/com/twitter/inject/TwitterModule.scala b/inject/inject-core/src/main/scala/com/twitter/inject/TwitterModule.scala
@@ -22,7 +22,12 @@ abstract class TwitterModule
   override protected def configure() {}
 
   override protected def install(module: Module) {
-    throw new Exception("Install not supported. Please use 'override val modules = Seq(module1, module2, ...)'")
+    module match {
+      case twitterModule: TwitterModule =>
+        throw new Exception("Install not supported for TwitterModules. Please use 'override val modules = Seq(module1, module2, ...)'")
+      case _ =>
+        super.install(module)
+    }
   }
 
   /* Protected */

diff --git a/...inject-server/src/test/scala/com/twitter/inject/server/tests/StartupIntegrationTest.scala b/...inject-server/src/test/scala/com/twitter/inject/server/tests/StartupIntegrationTest.scala
@@ -97,10 +97,15 @@ class StartupIntegrationTest extends Test {
       server.close()
     }
 
-    "calling GuiceModule.install throws exception" in {
+    "calling GuiceModule.install without a TwitterModule works" in {
+      new EmbeddedTwitterServer(
+        twitterServer = new ServerWithGuiceModuleInstall).start()
+    }
+
+    "calling GuiceModule.install with a TwitterModule throws exception" in {
       intercept[Exception] {
         new EmbeddedTwitterServer(
-          twitterServer = new ServerWithGuiceModuleInstall).start()
+          twitterServer = new ServerWithTwitterModuleInstall).start()
       }
     }
 
@@ -141,16 +146,22 @@ class SimpleGuiceTwitterServer extends TwitterServer {
 class SimpleGuiceHttpTwitterServer extends TwitterServer {
 }
 
-class ServerWithGuiceModuleInstall extends TwitterServer {
+class ServerWithTwitterModuleInstall extends TwitterServer {
   override val modules = Seq(new TwitterModule {
     override def configure() {
-      install(new FooModule)
+      install(new TwitterModule {})
     }
   })
 }
 
-class FooModule extends AbstractModule {
-  override def configure() {}
+class ServerWithGuiceModuleInstall extends TwitterServer {
+  override val modules = Seq(new TwitterModule {
+    override def configure() {
+      install(new AbstractModule {
+        override def configure(): Unit = {}
+      })
+    }
+  })
 }
 
 class PremainErrorBaseTwitterServer extends BaseTwitterServer with Ports with Warmup {