finatra-http: Simplify ExceptionMapper configuration and usage

Problem

`ExceptionMapper` configuration and usage is more cumbersome than it should
be. We created a specialized `ExceptionMapper` in the form of the
`DefaultExceptionMapper` which was meant to be invoked as a backstop for
HTTP exception handling but how you replace this default and/or other
default mappers like the JsonParseExceptionMapper or CaseClassExceptionMapper
is not ideal as overriding the default (which is internal to the framework)
or the other mappers (which we also want to be internal to the framework)
is currently tied into the binding of the `DefaultExceptionMapper` to the
`FinatraDefaultExceptionMapper` in the `ExceptionMapperModule`.

If users want to just swap out the configuration of these two additional
`ExceptionMapper`s it is difficult as they have to swap the entire
`ExceptionMapperModule` by overriding the `exceptionMapperModule` in the
`HttpServer`. Further complicating the issue is that the narrow visbility of
the `FinatraDefaultExceptionMapper` means that users cannot easily re-use
it as the default when replacing the entire `ExceptionMapperModule`, so you
either play package visibility games or duplicate a version. Neither are
ideal usage patterns.

Solution

We are dropping the need for a specialized `DefaultExceptionMapper` (which
was simply an `ExceptionMapper[Throwable]`). Instead we now allow the
configuration of mappers in the `ExceptionManager` to be much more flexible.
Previously, the framework tried to prevent a user from registering a mapper
over a given exception type multiple times and specialized a "default"
`ExceptionMapper` to invoke on an exception type of `Throwable`. The
`ExceptionManager` will now accept any mapper. If a mapper is added over a
type already added, the previous mapper will be overwritten.

The last registered mapper for an exception type wins.

The framework adds three mappers to the manager by default. If a user wants
to swap out any of these defaults they simply need add their own mapper to
the manager for the exception type to map. E.g., by default the framework
will add:
  `Throwable` ->
    com.twitter.finatra.http.internal.exceptions.ThrowableExceptionMapper
  `JsonParseException` ->
    com.twitter.finatra.http.internal.exceptions.json.JsonParseExceptionMapper
  `CaseClassMappingException` ->
    com.twitter.finatra.http.internal.exceptions.json.CaseClassExceptionMapper

The manager walks the exception type hierarchy starting at the given
exceptiontype and moving up the inheritence chain until it finds mapper
configured for the type. In this manner an `ExceptionMapper[Throwable]` will
be the last mapper invoked and performs as the "default".

Thus, to change the "default" mapper, simply adding a new mapper over the
`Throwable` type will suffice, i.e., `ExceptionMapper[Throwable]` to the
`ExceptionManager`. There  are multiple ways to add a mapper. Either through
the HttpRouter:

```
  override def configureHttp(router: HttpRouter): Unit = {
    router
      .exceptionMapper[MyDefaultExceptionMapper]
      ...
  }
```

Or in a module which is then added to the Server, e.g.,

```
  object MyExceptionMapperModule extends TwitterModule {
    override def singletonStartup(injector: Injector): Unit = {
      val manager = injector.instance[ExceptionManager]
      manager.add[MyDefaultExceptionMapper]
      manager.add[OtherExceptionMapper]
    }
  }
```
```
  override val modules = Seq(
    MyExceptionMapperModule,
    ...)
```

This also means we can simplify the `HttpServer` as we no longer need to expose
any "framework" module for overridding the default `ExceptionMappers`. So the
"def exceptionMapperModule" has also been removed.

Result

Users will no longer need provide an entire `ExceptionMapperModule` just
to configure a custom JsonParseExceptionMapper or CaseClassExceptionMapper.
Instead users can just add their own mapper for the exception type directly
to the `ExceptionManager` and any other configure mapper will be replaced
(including the framework defaults).

RB_ID=868614
TBR=true

CHANGELOG.md

@@ -22,6 +22,67 @@ All notable changes to this project will be documented in this file. Note that `
 
 ### Changed
 
+* finatra-http: Simplify ExceptionMapper configuration and usage. 
+  We are dropping the need for a specialized DefaultExceptionMapper (which
+  was simply an ExceptionMapper[Throwable]). Instead we now allow the 
+  configuration of mappers in the ExceptionManager to be much more flexible. 
+  Previously, the framework tried to prevent a user from registering a mapper 
+  over a given exception type multiple times and specialized a "default" 
+  ExceptionMapper to invoke on an exception type of Throwable. The 
+  ExceptionManager will now accept any mapper. If a mapper is added over a 
+  type already added, the previous mapper will be overwritten. 
+
+  The last registered mapper for an exception type wins.
+
+  The framework adds three mappers to the manager by default. If a user wants 
+  to swap out any of these defaults they simply need add their own mapper to 
+  the manager for the exception type to map. E.g., by default the framework 
+  will add:
+  Throwable -> 
+      com.twitter.finatra.http.internal.exceptions.ThrowableExceptionMapper
+  JsonParseException -> 
+      com.twitter.finatra.http.internal.exceptions.json.JsonParseExceptionMapper
+  CaseClassMappingException -> 
+      com.twitter.finatra.http.internal.exceptions.json.CaseClassExceptionMapper
+
+  The manager walks the exception type hierarchy starting at the given 
+  exceptiontype and moving up the inheritence chain until it finds mapper 
+  configured for the type. In this manner an ExceptionMapper[Throwable] will 
+  be the last mapper invoked and performs as the "default".
+
+  Thus, to change the "default" mapper, simply adding a new mapper over the 
+  Throwable type will suffice, i.e., ExceptionMapper[Throwable] to the 
+  ExceptionManager. There  are multiple ways to add a mapper. Either through 
+  the HttpRouter: 
+
+
+    override def configureHttp(router: HttpRouter): Unit = {
+      router
+        .exceptionMapper[MyDefaultExceptionMapper]
+        ...
+    }
+
+
+  Or in a module which is then added to the Server, e.g.,
+
+
+    object MyExceptionMapperModule extends TwitterModule {
+      override def singletonStartup(injector: Injector): Unit = {
+        val manager = injector.instance[ExceptionManager]
+        manager.add[MyDefaultExceptionMapper]
+        manager.add[OtherExceptionMapper]
+      }
+    }
+
+
+    override val modules = Seq(
+      MyExceptionMapperModule,
+      ...)
+
+
+  This also means we can simplify the HttpServer as we no longer need to expose 
+  any "framework" module for overridding the default ExceptionMappers. So the 
+  "def exceptionMapperModule" has also been removed.``RB_ID=868614``
 * finatra-http: Specify HTTP Java API consistently. ``RB_ID=868264``
 * inject-core: Clean up inject.Logging trait. Remove dead code from Logging. ``RB_ID=868261``
 * finatra-http: Move integration tests to a package under `com.twitter.finatra.http`. ``RB_ID=866487``

benchmarks/src/test/scala/com/twitter/finatra/http/benchmarks/ControllerBenchmark.scala

@@ -20,7 +20,6 @@ class ControllerBenchmark {
       "http.response.charset.enabled" -> "false"),
     modules = Seq(
       ExceptionManagerModule,
-      ExceptionMapperModule,
       MessageBodyModule,
       FinatraJacksonModule,
       MustacheModule,

http/src/main/scala/com/twitter/finatra/http/HttpServer.scala

@@ -9,7 +9,6 @@ import com.twitter.finatra.http.modules._
 import com.twitter.finatra.http.routing.HttpRouter
 import com.twitter.finatra.json.modules.FinatraJacksonModule
 import com.twitter.finatra.logging.modules.Slf4jBridgeModule
-import com.twitter.inject.TwitterModule
 import com.twitter.inject.annotations.Lifecycle
 import com.twitter.server.AdminHttpServer
 
@@ -18,14 +17,14 @@ abstract class AbstractHttpServer extends HttpServer
 
 trait HttpServer extends BaseHttpServer {
 
+  /** Add Framework Modules */
   addFrameworkModules(
-    mustacheModule,
-    messageBodyModule,
-    exceptionMapperModule,
-    exceptionManagerModule,
-    jacksonModule,
-    DocRootModule,
     accessLogModule,
+    DocRootModule,
+    ExceptionManagerModule,
+    jacksonModule,
+    messageBodyModule,
+    mustacheModule,
     Slf4jBridgeModule)
 
   /* Abstract */
@@ -76,23 +75,6 @@ trait HttpServer extends BaseHttpServer {
    */
   protected def messageBodyModule: Module = MessageBodyModule
 
-  /**
-   * Default [[com.twitter.inject.TwitterModule]] for providing an implementation for a
-   * [[com.twitter.finatra.http.exceptions.DefaultExceptionMapper]].
-   *
-   * @return a [[com.twitter.inject.TwitterModule]] which provides a
-   *         [[com.twitter.finatra.http.exceptions.DefaultExceptionMapper]] implementation.
-   */
-  protected def exceptionMapperModule: Module = ExceptionMapperModule
-
-  /**
-   * Default [[com.twitter.inject.TwitterModule]] for providing a [[com.twitter.finatra.http.exceptions.ExceptionManager]].
-   *
-   * @return a [[com.twitter.inject.TwitterModule]] which provides a
-   *         [[com.twitter.finatra.http.exceptions.ExceptionManager]] implementation.
-   */
-  protected def exceptionManagerModule: TwitterModule = ExceptionManagerModule
-
   /**
    * Default [[com.twitter.inject.TwitterModule]] for providing a [[com.twitter.finatra.json.FinatraObjectMapper]].
    *
@@ -111,13 +93,15 @@ trait HttpServer extends BaseHttpServer {
       throw new Exception(errorMessage)
     }
 
-    // Constant routes that don't start with /admin/finatra can be added to the admin index,
-    // all other routes cannot. Only constant /GET routes will be eligible to be added to the admin UI.
-    // NOTE: beforeRouting = true filters will not be properly evaluated on adminIndexRoutes
-    // since the localMuxer in the AdminHttpServer does exact route matching before marshalling
-    // to the handler service (where the filter is composed). Thus if this filter defines a route
-    // it will not get routed to by the localMuxer. Any beforeRouting = true filters should act
-    // only on paths behind /admin/finatra.
+    /*
+     Constant routes which do not begin with /admin/finatra can be added to the admin index,
+     all other routes cannot. Only constant /GET routes will be eligible to be added to the admin UI.
+     NOTE: beforeRouting = true filters will not be properly evaluated on adminIndexRoutes
+     since the local Muxer in the AdminHttpServer does exact route matching before marshalling
+     to the handler service (where the filter is composed). Thus if this filter defines a route
+     it will not get routed to by the local Muxer. Any beforeRouting = true filters should act
+     only on paths behind /admin/finatra.
+    */
     val (adminIndexRoutes, adminRichHandlerRoutes) = router.routesByType.admin.partition { route =>
       // can't start with /admin/finatra/ and is a constant route
       !route.path.startsWith(HttpRouter.FinatraAdminPrefix) && route.constantRoute

http/src/main/scala/com/twitter/finatra/http/exceptions/DefaultExceptionMapper.scala

@@ -1,3 +1,4 @@
 package com.twitter.finatra.http.exceptions
 
-trait DefaultExceptionMapper extends ExceptionMapper[Throwable]
+@deprecated("Specially typing a default exception mapper is no longer necessary. Extend ExceptionMapper[Throwable] directly.", "2016-09-07")
+trait DefaultExceptionMapper extends ExceptionMapper[Throwable]

http/src/main/scala/com/twitter/finatra/http/exceptions/ExceptionManager.scala

@@ -15,17 +15,19 @@ import scala.annotation.tailrec
 import scala.collection.JavaConverters._
 
 /**
- * A class to register ExceptionMappers and handle exceptions.
+ * A class to register [[com.twitter.finatra.http.exceptions.ExceptionMapper]]s
+ * and handle exceptions.
  *
- * Given some exception, an ExceptionManager will find an ExceptionMapper
- * to handle that particular class of exceptions. If the mapper for that
- * exception isn't registered, ExceptionManager will try its parent
- * class, and so on, until it reaches the Throwable class. At that point
- * the DefaultExceptionMapper will run which should be defined over all
- * Throwables.
+ * Given an exception, the ExceptionManager will find an
+ * [[com.twitter.finatra.http.exceptions.ExceptionMapper]] to handle
+ * that particular class of exceptions. If the mapper for that exception
+ * isn't registered, the ExceptionManager will try the exception's parent class,
+ * and so on, until it reaches the Throwable class. The framework registers a "root"
+ * exception mapper over Throwable which will eventually be invoked. Users are
+ * free to register their own ExceptionMapper[Throwable] which overrides the
+ * "root" exception mapper.
  *
- * @throws java.lang.IllegalStateException when an exception type is
- * registered twice.
+ * @see [[com.twitter.finatra.http.internal.exceptions.ThrowableExceptionMapper]]
  *
  * Note: When searching for the parent exception mapper, it would be nice
  * to traverse the entire class linearization so it works for
@@ -37,27 +39,57 @@ import scala.collection.JavaConverters._
  * [2] http://docs.scala-lang.org/overviews/reflection/thread-safety.html
  */
 @Singleton
-class ExceptionManager(
+class ExceptionManager  (
   injector: Injector,
-  defaultExceptionMapper: DefaultExceptionMapper,
   statsReceiver: StatsReceiver) {
 
   private val mappers = new ConcurrentHashMap[Type, ExceptionMapper[_]]().asScala
 
   /* Public */
 
-  def add[T <: Throwable : Manifest](mapper: ExceptionMapper[T]) {
+  /**
+   * Add a [[com.twitter.finatra.http.exceptions.ExceptionMapper]] over type [[T]] to the manager.
+   * If a mapper has already been added for the given [[T]], it will be replaced.
+   *
+   * @param mapper - [[com.twitter.finatra.http.exceptions.ExceptionMapper]] to add
+   * @tparam T - exception class type which should subclass [[java.lang.Throwable]]
+   */
+  def add[T <: Throwable : Manifest](mapper: ExceptionMapper[T]): Unit = {
     add(manifest[T].runtimeClass, mapper)
   }
 
-  def add[T <: ExceptionMapper[_]: Manifest] {
+  /**
+   * Add a collection of [[com.twitter.finatra.http.exceptions.ExceptionMapper]] as defined by a
+   * [[com.twitter.finatra.http.exceptions.ExceptionMapperCollection]]. The collection is iterated
+   * over and each mapper contained therein is added. If a mapper has already been added for the given
+   * type T in ExceptionMapper[T], it will be replaced.
+   *
+   * @param collection - [[com.twitter.finatra.http.exceptions.ExceptionMapperCollection]] to add.
+   */
+  def add(collection: ExceptionMapperCollection): Unit = {
+    collection.foreach(add(_))
+  }
+
+  /**
+   * Add a [[com.twitter.finatra.http.exceptions.ExceptionMapper]] by type [[T]]
+   * @tparam T - ExceptionMapper type T which should subclass [[com.twitter.finatra.http.exceptions.ExceptionMapper]]
+   */
+  def add[T <: ExceptionMapper[_]: Manifest]: Unit = {
     val mapperType = typeLiteral[T].getSupertype(classOf[ExceptionMapper[_]]).getType
     val throwableType = singleTypeParam(mapperType)
     add(throwableType, injector.instance[T])
   }
 
+  /**
+   * Returns a [[com.twitter.finagle.http.Response]] as computed by the matching
+   * [[com.twitter.finatra.http.exceptions.ExceptionMapper]] to the given throwable.
+   *
+   * @param request - a [[com.twitter.finagle.http.Request]]
+   * @param throwable - [[java.lang.Throwable]] to match against registered ExceptionMappers.
+   * @return a [[com.twitter.finagle.http.Response]]
+   */
   def toResponse(request: Request, throwable: Throwable): Response = {
-    val mapper = cachedGetMapper(throwable.getClass)
+    val mapper = getMapper(throwable.getClass)
     val response = mapper.asInstanceOf[ExceptionMapper[Throwable]].toResponse(request, throwable)
     RouteInfo(request).foreach { info =>
       statException(info, request, throwable, response)
@@ -91,18 +123,9 @@ class ExceptionManager(
     }
   }
 
+  // Last entry by type overrides any previous entry.
   private def add(throwableType: Type, mapper: ExceptionMapper[_]) {
-    if (mappers.contains(throwableType)) {
-      throw new IllegalStateException(s"ExceptionMapper for $throwableType already registered")
-    } else {
-      mappers(throwableType) = mapper // mutation
-    }
-  }
-
-  // Assumes mappers are never explicitly registered after configuration
-  // phase, otherwise we'd need to invalidate the cache.
-  private def cachedGetMapper(cls: Class[_]): ExceptionMapper[_] = {
-    mappers.getOrElseUpdate(cls, getMapper(cls))
+    mappers.getOrElseUpdate(throwableType, mapper)
   }
 
   // Get mapper for this throwable class if it exists, otherwise
@@ -112,13 +135,9 @@ class ExceptionManager(
   // Note: we avoid getOrElse so we have tail recursion
   @tailrec
   private def getMapper(cls: Class[_]): ExceptionMapper[_] = {
-    if (cls == classOf[Throwable]) {
-      defaultExceptionMapper
-    } else {
-      mappers.get(cls) match {
-        case Some(mapper) => mapper
-        case None => getMapper(cls.getSuperclass)
-      }
+    mappers.get(cls) match {
+      case Some(mapper) => mapper
+      case None => getMapper(cls.getSuperclass)
     }
   }
 }

http/src/main/scala/com/twitter/finatra/http/exceptions/ExceptionMapperCollection.scala

@@ -0,0 +1,25 @@
+package com.twitter.finatra.http.exceptions
+
+import scala.collection.mutable.ArrayBuffer
+
+/**
+ * Represents a collection of [[com.twitter.finatra.http.exceptions.ExceptionMapper]]s
+ * which is a Traversable[Manifest[ExceptionMapper[_\]\]\].
+ */
+class ExceptionMapperCollection extends Traversable[Manifest[ExceptionMapper[_]]] {
+
+  private[this] val manifests = ArrayBuffer[Manifest[ExceptionMapper[_]]]()
+
+  /**
+   * Add a [[com.twitter.finatra.http.exceptions.ExceptionMapper]] by type [[T]]
+   * @tparam T - ExceptionMapper type T which should subclass [[com.twitter.finatra.http.exceptions.ExceptionMapper]]
+   */
+  def add[T <: ExceptionMapper[_]: Manifest]: Unit = {
+    val m: Manifest[T] = manifest[T]
+    manifests += m.asInstanceOf[Manifest[ExceptionMapper[_]]]
+  }
+
+  override def foreach[U](f: (Manifest[ExceptionMapper[_]]) => U): Unit = manifests.foreach(f)
+
+  override def seq: Traversable[Manifest[ExceptionMapper[_]]] = manifests.seq
+}

http/src/main/scala/com/twitter/finatra/http/filters/CommonFilters.scala

@@ -10,4 +10,4 @@ class CommonFilters @Inject()(
   b: AccessLoggingFilter[Request],
   c: HttpResponseFilter[Request],
   d: ExceptionMappingFilter[Request])
-  extends MergedFilter(a, b, c, d)
+  extends MergedFilter(a, b, c, d)

http/src/main/scala/com/twitter/finatra/http/internal/exceptions/FinatraDefaultExceptionMapper.scala → http/src/main/scala/com/twitter/finatra/http/internal/exceptions/ThrowableExceptionMapper.scala

@@ -1,18 +1,18 @@
 package com.twitter.finatra.http.internal.exceptions
 
 import com.google.common.net.MediaType
-import com.twitter.finagle.http.{Request, Response}
-import com.twitter.finagle.{CancelledRequestException, Failure}
-import com.twitter.finatra.http.exceptions.{DefaultExceptionMapper, HttpException, HttpResponseException}
-import com.twitter.finatra.http.internal.exceptions.FinatraDefaultExceptionMapper._
+import com.twitter.finagle.{Failure, CancelledRequestException}
+import com.twitter.finagle.http.{Response, Request}
+import com.twitter.finatra.http.exceptions.{ExceptionMapper, HttpResponseException, HttpException}
+import com.twitter.finatra.http.internal.exceptions.ThrowableExceptionMapper._
 import com.twitter.finatra.http.response.{ErrorsResponse, ResponseBuilder}
 import com.twitter.finatra.utils.DeadlineValues
 import com.twitter.inject.Logging
 import com.twitter.inject.utils.ExceptionUtils._
 import javax.inject.{Inject, Singleton}
 import org.apache.thrift.TException
 
-private[http] object FinatraDefaultExceptionMapper {
+private[http] object ThrowableExceptionMapper {
   private val MaxDepth = 5
   private val DefaultExceptionSource = "Internal"
 
@@ -29,9 +29,9 @@ private[http] object FinatraDefaultExceptionMapper {
 }
 
 @Singleton
-private[http] class FinatraDefaultExceptionMapper @Inject()(
+private[http] class ThrowableExceptionMapper @Inject()(
   response: ResponseBuilder)
-  extends DefaultExceptionMapper
+  extends ExceptionMapper[Throwable]
   with Logging {
 
   override def toResponse(request: Request, throwable: Throwable): Response = {
@@ -86,4 +86,4 @@ private[http] class FinatraDefaultExceptionMapper @Inject()(
           toExceptionDetails(e)))
       .jsonError
   }
-}
+}

http/src/main/scala/com/twitter/finatra/http/internal/exceptions/json/CaseClassExceptionMapper.scala

@@ -7,7 +7,7 @@ import com.twitter.finatra.json.internal.caseclass.exceptions.CaseClassMappingEx
 import javax.inject.{Inject, Singleton}
 
 @Singleton
-class CaseClassExceptionMapper @Inject()(
+private[http] class CaseClassExceptionMapper @Inject()(
   response: ResponseBuilder)
   extends ExceptionMapper[CaseClassMappingException] {
 

http/src/main/scala/com/twitter/finatra/http/internal/exceptions/json/JsonParseExceptionMapper.scala

@@ -9,7 +9,7 @@ import com.twitter.inject.Logging
 import javax.inject.{Inject, Singleton}
 
 @Singleton
-class JsonParseExceptionMapper @Inject()(
+private[http] class JsonParseExceptionMapper @Inject()(
   response: ResponseBuilder)
   extends ExceptionMapper[JsonParseException]
   with Logging {