finatra/http-server: Better HttpRouter Java support for adding gene…

…rically typed Filters

Problem

We do not properly document how Java users can apply generically typed
Filters to the `HttpRouter`. Which right now requires Java users that want
the injector to do the heavy lifting of Filter construction to manually obtain
an instance of the Filter from the object graph to apply it to the `HttpRouter`
(or play games with Scala type erasure from Java to create Manifests to call
the `HttpRouter#filter[T: Manifest]` function with enough type information).

The Guice recommended way to bind parameterized types is via usage
of a `TypeLiteral`.

Solution

Add versions of the `HttpRouter#filter` method which takes in a Guice
TypeLiteral which can help Java users properly use generically typed
Filters with the `HttpRouter`.

JIRA Issues: CSL-11419

Differential Revision: https://phabricator.twitter.biz/D768777

CHANGELOG.rst

@@ -7,6 +7,10 @@ Note that ``RB_ID=#`` and ``PHAB_ID=#`` correspond to associated message in comm
 Unreleased
 ----------
 
+* http-server: Add versions of `HttpRouter#filter` which accept a Guice `TypeLiteral` to
+  aid Java users in being able to apply generically typed Filters obtained from the object graph.
+  ``PHAB_ID=D768777``
+
 21.9.0
 ------
 

doc/src/sphinx/user-guide/http/server.rst

@@ -65,7 +65,7 @@ A more complete example includes adding Modules, a Controller, and Filters. In S
 
     import com.google.inject.Module
     import com.twitter.finatra.http.HttpServer
-    import com.twitter.finatra.http.filters.{CommonFilters, LoggingMDCFilter, TraceIdMDCFilter}
+    import com.twitter.finatra.http.filters.{LoggingMDCFilter, TraceIdMDCFilter}
     import com.twitter.finatra.http.routing.HttpRouter
 
     object ExampleServerMain extends ExampleServer
@@ -79,7 +79,7 @@ A more complete example includes adding Modules, a Controller, and Filters. In S
         router
           .filter[LoggingMDCFilter[Request, Response]]
           .filter[TraceIdMDCFilter[Request, Response]]
-          .filter[CommonFilters]
+          .filter[ExampleFilter]
           .add[ExampleController]
       }
     }
@@ -89,15 +89,16 @@ in Java:
 .. code:: java
 
     import com.google.inject.Module;
+    import com.google.inject.TypeLiteral;
     import com.twitter.app.Flaggable;
     import com.twitter.finatra.http.AbstractHttpServer;
-    import com.twitter.finatra.http.filters.CommonFilters;
     import com.twitter.finatra.http.routing.HttpRouter;
-    import com.twitter.finatra.http.filters.CommonFilters; 
-    import com.twitter.finatra.http.filters.LoggingMDCFilter; 
+    import com.twitter.finatra.http.filters.LoggingMDCFilter;
+    import com.twitter.finatra.http.filters.StatsFilter;
     import com.twitter.finatra.http.filters.TraceIdMDCFilter;
     import java.util.Collection;
     import java.util.Collections;
+    import scala.reflect.ManifestFactory;
 
     public class ExampleServer extends AbstractHttpServer {
 
@@ -118,13 +119,35 @@ in Java:
       @Override
       public void configureHttp(HttpRouter httpRouter) {
           httpRouter
-              .filter(LoggingMDCFilter<Request, Response>.class)
-              .filter(TraceIdMDCFilter<Request, Response>.class)
-              .filter(CommonFilters.class)
+              .filter(new TypeLiteral<LoggingMDCFilter<Request, Response>>(){})
+              .filter(ManifestFactory.classType(TraceIdMDCFilter.class))
+              .filter(new TypeLiteral<StatsFilter<Request>>() {})
+              .filter(ExampleFilter.class)
               .add(ExampleController.class);
       }
     }
 
+Adding filters in Java can happen in multiple ways which is somewhat dependent on your tolerance to
+type erasure if you need to apply Filters that have `parameterized types <https://github.com/google/guice/wiki/FrequentlyAskedQuestions#how-to-inject-class-with-generic-type>`__.
+
+Above, we show how you can choose to use the `.filter(typeLiteral: TypeLiteral[T])` method from the
+`HttpRouter` which will obtain the Filter instance described by the `TypeLiteral[T] <https://github.com/google/guice/wiki/BuiltInBindings#typeliterals>`__
+and append it to the filter chain. There is a version which also accepts a `binding annotation <../getting-started/binding_annotations.html>`__
+which is useful if you need to construct a Filter that is not only parameterized but also discriminated
+by a `binding annotation <../getting-started/binding_annotations.html>`__. **This is the recommended
+way to configure generically typed Filters.**
+
+Or you can choose to use the `scala.reflect.ManifestFactory <https://www.scala-lang.org/api/2.12.4/scala/reflect/ManifestFactory$.html>`__
+to pass the Manifest type for a Filter class. Note in the usage above, the `TraceIdMDCFilter <https://twitter.github.io/finatra/scaladocs/com/twitter/finatra/http/filters/TraceIdMDCFilter.html>`__
+has two type params, `[Req,Rep]`. They are not specified when building the Manifest which amounts
+to the types being erased. In this case, this is OK since we are a.) appending the instance
+to a chain which will adapt the types appropriately and b.) we don't expect to obtain the Filter
+instance from the injector anywhere else in the code (which would need to be asked for as just
+`TraceIdMDCFilter` and not `TraceIdMDCFilter[Request, Response]`).
+
+Finally, you can always pass the class of the Filter to append. The instance will be obtained from
+the injector and appended to the filter chain.
+
 .. tip::
 
     Note: to add `Modules <../getting-started/modules.html>`__ to your Java server override the

examples/http-server/java/src/main/java/com/twitter/finatra/example/HelloWorldServer.java

@@ -6,14 +6,23 @@
 import scala.reflect.ManifestFactory;
 
 import com.google.inject.Module;
+import com.google.inject.TypeLiteral;
 
 import com.twitter.app.Flaggable;
+import com.twitter.finagle.http.Request;
+import com.twitter.finagle.http.Response;
 import com.twitter.finatra.example.controllers.HelloWorldController;
 import com.twitter.finatra.example.exceptions.HelloWorldExceptionMapper;
 import com.twitter.finatra.example.filters.AppendToHeaderFilter;
 import com.twitter.finatra.example.modules.MagicNumberModule;
 import com.twitter.finatra.http.AbstractHttpServer;
-import com.twitter.finatra.http.filters.CommonFilters;
+import com.twitter.finatra.http.filters.AccessLoggingFilter;
+import com.twitter.finatra.http.filters.ExceptionMappingFilter;
+import com.twitter.finatra.http.filters.HttpNackFilter;
+import com.twitter.finatra.http.filters.HttpResponseFilter;
+import com.twitter.finatra.http.filters.LoggingMDCFilter;
+import com.twitter.finatra.http.filters.StatsFilter;
+import com.twitter.finatra.http.filters.TraceIdMDCFilter;
 import com.twitter.finatra.http.routing.HttpRouter;
 
 public class HelloWorldServer extends AbstractHttpServer {
@@ -34,7 +43,16 @@ public Collection<Module> javaModules() {
   @Override
   public void configureHttp(HttpRouter router) {
     router
-        .filter(CommonFilters.class)
+        .filter(new TypeLiteral<LoggingMDCFilter<Request, Response>>(){})
+        // note this binds the TraceIdMDCFilter type without the parameterized types which in
+        // this case is ok since the filter is appended to a fully typed
+        // filter (the loggingMDCFilter).
+        .filter(ManifestFactory.classType(TraceIdMDCFilter.class))
+        .filter(new TypeLiteral<StatsFilter<Request>>() {})
+        .filter(new TypeLiteral<AccessLoggingFilter<Request>>() {})
+        .filter(new TypeLiteral<HttpResponseFilter<Request>>() {})
+        .filter(new TypeLiteral<ExceptionMappingFilter<Request>>() {})
+        .filter(new TypeLiteral<HttpNackFilter<Request>>() {})
         .filter(new AppendToHeaderFilter("foo", "1"))
         .add(HelloWorldController.class)
         .register(ManifestFactory.classType(CatMessageBodyReader.class))

examples/http-server/scala/src/main/scala/com/twitter/finatra/example/HelloWorldServer.scala

@@ -1,8 +1,15 @@
 package com.twitter.finatra.example
 
-import com.twitter.finagle.http.{Request, Response}
+import com.twitter.finagle.http.Request
+import com.twitter.finagle.http.Response
 import com.twitter.finatra.http.HttpServer
-import com.twitter.finatra.http.filters.{CommonFilters, LoggingMDCFilter, TraceIdMDCFilter}
+import com.twitter.finatra.http.filters.AccessLoggingFilter
+import com.twitter.finatra.http.filters.ExceptionMappingFilter
+import com.twitter.finatra.http.filters.HttpNackFilter
+import com.twitter.finatra.http.filters.HttpResponseFilter
+import com.twitter.finatra.http.filters.LoggingMDCFilter
+import com.twitter.finatra.http.filters.StatsFilter
+import com.twitter.finatra.http.filters.TraceIdMDCFilter
 import com.twitter.finatra.http.routing.HttpRouter
 
 object HelloWorldServerMain extends HelloWorldServer
@@ -13,7 +20,11 @@ class HelloWorldServer extends HttpServer {
     router
       .filter[LoggingMDCFilter[Request, Response]]
       .filter[TraceIdMDCFilter[Request, Response]]
-      .filter[CommonFilters]
+      .filter[StatsFilter[Request]]
+      .filter[AccessLoggingFilter[Request]]
+      .filter[HttpResponseFilter[Request]]
+      .filter[ExceptionMappingFilter[Request]]
+      .filter[HttpNackFilter[Request]]
       .add[HelloWorldController]
   }
 }

http-server/src/main/scala/com/twitter/finatra/http/routing/HttpRouter.scala

@@ -1,20 +1,27 @@
 package com.twitter.finatra.http.routing
 
+import com.google.inject.Key
+import com.google.inject.TypeLiteral
 import com.twitter.finagle.Filter
-import com.twitter.finatra.http.exceptions.{
-  AbstractExceptionMapper,
-  ExceptionManager,
-  ExceptionMapper,
-  ExceptionMapperCollection
-}
-import com.twitter.finatra.http.internal.routing.{CallbackConverterImpl, Route, RoutingService}
-import com.twitter.finatra.http.marshalling.{MessageBodyComponent, MessageBodyManager}
-import com.twitter.finatra.http.{AbstractController, Controller, HttpFilter}
+import com.twitter.finatra.http.exceptions.AbstractExceptionMapper
+import com.twitter.finatra.http.exceptions.ExceptionManager
+import com.twitter.finatra.http.exceptions.ExceptionMapper
+import com.twitter.finatra.http.exceptions.ExceptionMapperCollection
+import com.twitter.finatra.http.internal.routing.CallbackConverterImpl
+import com.twitter.finatra.http.internal.routing.Route
+import com.twitter.finatra.http.internal.routing.RoutingService
+import com.twitter.finatra.http.marshalling.MessageBodyComponent
+import com.twitter.finatra.http.marshalling.MessageBodyManager
+import com.twitter.finatra.http.AbstractController
+import com.twitter.finatra.http.Controller
+import com.twitter.finatra.http.HttpFilter
 import com.twitter.inject.TypeUtils
 import com.twitter.inject.internal.LibraryRegistry
-import com.twitter.inject.{Injector, Logging}
+import com.twitter.inject.Injector
+import com.twitter.inject.Logging
 import java.lang.annotation.{Annotation => JavaAnnotation}
-import javax.inject.{Inject, Singleton}
+import javax.inject.Inject
+import javax.inject.Singleton
 import scala.collection.mutable.ArrayBuffer
 
 object HttpRouter {
@@ -107,38 +114,44 @@ class HttpRouter @Inject() (
   }
 
   /** Add global filter used for all requests annotated with Annotation Type */
-  def filter[FilterType <: HttpFilter: Manifest, Ann <: JavaAnnotation: Manifest]: HttpRouter = {
+  def filter[FilterType <: HttpFilter: Manifest, Ann <: JavaAnnotation: Manifest]: HttpRouter =
     filter(injector.instance[FilterType, Ann])
-  }
 
   /** Add global filter used for all requests, by default applied AFTER route matching */
-  def filter(clazz: Class[_ <: HttpFilter]): HttpRouter = {
+  def filter(clazz: Class[_ <: HttpFilter]): HttpRouter =
     filter(injector.instance(clazz))
-  }
 
   /** Add global filter used for all requests, optionally BEFORE route matching */
-  def filter(clazz: Class[_ <: HttpFilter], beforeRouting: Boolean): HttpRouter = {
-    if (beforeRouting) {
-      filter(injector.instance(clazz), beforeRouting = true)
-    } else {
-      filter(injector.instance(clazz))
-    }
-  }
+  def filter(clazz: Class[_ <: HttpFilter], beforeRouting: Boolean): HttpRouter =
+    filter(injector.instance(clazz), beforeRouting)
+
+  /** Add global filter used for all requests, by default applied AFTER route matching */
+  def filter(typeLiteral: TypeLiteral[_ <: HttpFilter]): HttpRouter =
+    filter(injector.instance(Key.get(typeLiteral)))
+
+  /** Add global filter used for all requests, by default applied AFTER route matching */
+  def filter(typeLiteral: TypeLiteral[_ <: HttpFilter], annotation: JavaAnnotation): HttpRouter =
+    filter(injector.instance(Key.get(typeLiteral, annotation)))
+
+  /** Add global filter used for all requests, optionally BEFORE route matching */
+  def filter(typeLiteral: TypeLiteral[_ <: HttpFilter], beforeRouting: Boolean): HttpRouter =
+    filter(injector.instance(Key.get(typeLiteral)), beforeRouting)
+
+  /** Add global filter used for all requests, optionally BEFORE route matching */
+  def filter(
+    typeLiteral: TypeLiteral[_ <: HttpFilter],
+    annotation: JavaAnnotation,
+    beforeRouting: Boolean
+  ): HttpRouter =
+    filter(injector.instance(Key.get(typeLiteral, annotation)), beforeRouting)
 
   /** Add global filter used for all requests, by default applied AFTER route matching */
-  def filter[FilterType <: HttpFilter: Manifest]: HttpRouter = {
+  def filter[FilterType <: HttpFilter: Manifest]: HttpRouter =
     filter(injector.instance[FilterType])
-  }
 
   /** Add global filter used for all requests, optionally BEFORE route matching */
-  def filter[FilterType <: HttpFilter: Manifest](beforeRouting: Boolean): HttpRouter = {
-    if (beforeRouting) {
-      filter(injector.instance[FilterType], beforeRouting = true)
-      this
-    } else {
-      filter(injector.instance[FilterType])
-    }
-  }
+  def filter[FilterType <: HttpFilter: Manifest](beforeRouting: Boolean): HttpRouter =
+    filter(injector.instance[FilterType], beforeRouting)
 
   /** Add global filter used for all requests, by default applied AFTER route matching */
   def filter(filter: HttpFilter): HttpRouter = {

http-server/src/test/java/com/twitter/finatra/http/tests/integration/doeverything/main/DoEverythingJavaServer.java

@@ -3,10 +3,21 @@
 import java.util.Collection;
 import java.util.Collections;
 
+import scala.reflect.ManifestFactory;
+
 import com.google.inject.Module;
+import com.google.inject.TypeLiteral;
 
+import com.twitter.finagle.http.Request;
+import com.twitter.finagle.http.Response;
 import com.twitter.finatra.http.AbstractHttpServer;
-import com.twitter.finatra.http.filters.CommonFilters;
+import com.twitter.finatra.http.filters.AccessLoggingFilter;
+import com.twitter.finatra.http.filters.ExceptionMappingFilter;
+import com.twitter.finatra.http.filters.HttpNackFilter;
+import com.twitter.finatra.http.filters.HttpResponseFilter;
+import com.twitter.finatra.http.filters.LoggingMDCFilter;
+import com.twitter.finatra.http.filters.StatsFilter;
+import com.twitter.finatra.http.filters.TraceIdMDCFilter;
 import com.twitter.finatra.http.routing.HttpRouter;
 
 public class DoEverythingJavaServer extends AbstractHttpServer {
@@ -24,7 +35,13 @@ public String name() {
   @Override
   public void configureHttp(HttpRouter router) {
     router
-        .filter(CommonFilters.class)
+        .filter(new TypeLiteral<LoggingMDCFilter<Request, Response>>(){})
+        .filter(ManifestFactory.classType(TraceIdMDCFilter.class))
+        .filter(new TypeLiteral<StatsFilter<Request>>() {})
+        .filter(new TypeLiteral<AccessLoggingFilter<Request>>() {})
+        .filter(new TypeLiteral<HttpResponseFilter<Request>>() {})
+        .filter(new TypeLiteral<ExceptionMappingFilter<Request>>() {})
+        .filter(new TypeLiteral<HttpNackFilter<Request>>() {})
         .filter(new AppendToHeaderJavaFilter("test", "1"))
         .add(DoEverythingJavaController.class)
         .add(new DoEverythingJavaNonInjectedController())