finatra/http: remove ExceptionMapper.replace

Problem:

ExceptionManager.replace is too fragile because it relies on module
initialization order.

Solution:

Remove ExceptionManager.replace. Fold HttpException, HttpResponseException,
CancelledRequestException into FinatraDefaultExceptionMapper to
enforce built-in exception handling that shouldn't be configurable.

Result:

ExceptionManager.replace is gone. Users can write their own modules
if the default module isn't suitable.

RB_ID=720818

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

@@ -42,25 +42,16 @@ class ExceptionManager @Inject()(
   private val mappers = mapAsScalaConcurrentMap(
     new ConcurrentHashMap[Type, ExceptionMapper[_]]())
 
-  // run after constructing `mappers`
-  add[Throwable](defaultExceptionMapper)
-
   /* Public */
 
   def add[T <: Throwable : Manifest](mapper: ExceptionMapper[T]) {
-    set(manifest[T].runtimeClass, mapper, replace = false)
+    add(manifest[T].runtimeClass, mapper)
   }
 
   def add[T <: ExceptionMapper[_]: Manifest] {
-    set[T](replace = false)
-  }
-
-  def replace[T <: Throwable : Manifest](mapper: ExceptionMapper[T]) {
-    set(manifest[T].runtimeClass, mapper, replace = true)
-  }
-
-  def replace[T <: ExceptionMapper[_]: Manifest] {
-    set[T](replace = true)
+    val mapperType = typeLiteral[T].getSupertype(classOf[ExceptionMapper[_]]).getType
+    val throwableType = singleTypeParam(mapperType)
+    add(throwableType, injector.instance[T])
   }
 
   def toResponse(request: Request, throwable: Throwable): Response = {
@@ -70,14 +61,8 @@ class ExceptionManager @Inject()(
 
   /* Private */
 
-  private def set[T <: ExceptionMapper[_]: Manifest](replace: Boolean) {
-    val mapperType = typeLiteral[T].getSupertype(classOf[ExceptionMapper[_]]).getType
-    val throwableType = singleTypeParam(mapperType)
-    set(throwableType, injector.instance[T], replace)
-  }
-
-  private def set(throwableType: Type, mapper: ExceptionMapper[_], replace: Boolean) {
-    if (!replace && mappers.contains(throwableType)) {
+  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
@@ -91,16 +76,19 @@ class ExceptionManager @Inject()(
   }
 
   // Get mapper for this throwable class if it exists, otherwise
-  // search for parent throwable class. This will always terminate
-  // because we added an ExceptionMapper[Throwable] in our constructor
-  // body.
+  // search for parent throwable class. If we reach the Throwable
+  // class then return the default mapper.
   //
   // Note: we avoid getOrElse so we have tail recursion
   @tailrec
   private def getMapper(cls: Class[_]): ExceptionMapper[_] = {
-    mappers.get(cls) match {
-      case Some(mapper) => mapper
-      case None => getMapper(cls.getSuperclass)
+    if (cls == classOf[Throwable]) {
+      defaultExceptionMapper
+    } else {
+      mappers.get(cls) match {
+        case Some(mapper) => mapper
+        case None => getMapper(cls.getSuperclass)
+      }
     }
   }
 }

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

@@ -3,7 +3,7 @@ package com.twitter.finatra.http.internal.exceptions
 import com.twitter.finagle.CancelledRequestException
 import com.twitter.finagle.http.{Request, Response}
 import com.twitter.finatra.exceptions.ExternalServiceExceptionMatcher
-import com.twitter.finatra.http.exceptions.DefaultExceptionMapper
+import com.twitter.finatra.http.exceptions.{DefaultExceptionMapper, HttpException, HttpResponseException}
 import com.twitter.finatra.http.response.{ResponseBuilder, ErrorsResponse}
 import com.twitter.inject.Logging
 import javax.inject.{Inject, Singleton}
@@ -16,6 +16,12 @@ class FinatraDefaultExceptionMapper @Inject()(
 
   override def toResponse(request: Request, throwable: Throwable): Response = {
     throwable match {
+      case e: HttpException =>
+        e.createResponse(response)
+      case e: HttpResponseException =>
+        e.response
+      case e: CancelledRequestException =>
+        response.clientClosed
       case ExternalServiceExceptionMatcher(e) =>
         warn("service unavailable", e)
         response.serviceUnavailable.json(

http/src/main/scala/com/twitter/finatra/http/modules/ExceptionMapperModule.scala

@@ -1,7 +1,7 @@
 package com.twitter.finatra.http.modules
 
 import com.twitter.finatra.http.exceptions.DefaultExceptionMapper
-import com.twitter.finatra.http.internal.exceptions._
+import com.twitter.finatra.http.internal.exceptions.{ExceptionManager, FinatraDefaultExceptionMapper}
 import com.twitter.finatra.http.internal.exceptions.json.{JsonParseExceptionMapper, CaseClassExceptionMapper}
 import com.twitter.inject.{TwitterModule, Injector, InjectorModule}
 
@@ -13,11 +13,8 @@ object ExceptionMapperModule extends TwitterModule {
   }
 
   override def singletonStartup(injector: Injector) {
-      val manager = injector.instance[ExceptionManager]
-      manager.add[JsonParseExceptionMapper]
-      manager.add[CaseClassExceptionMapper]
-      manager.add[HttpExceptionMapper]
-      manager.add[HttpResponseExceptionMapper]
-      manager.add[CancelledRequestExceptionMapper]
+    val manager = injector.instance[ExceptionManager]
+    manager.add[JsonParseExceptionMapper]
+    manager.add[CaseClassExceptionMapper]
   }
 }

http/src/test/scala/com/twitter/finatra/http/internal/exceptions/ExceptionManagerTest.scala

@@ -52,23 +52,6 @@ class ExceptionManagerTest extends Test with Mockito {
       exceptionManager.add[ForbiddenExceptionMapper]
     }
   }
-
-  "replace an exception mapper" in {
-    val exceptionManager = newExceptionManager
-    exceptionManager.add[ForbiddenExceptionMapper]
-    exceptionManager.replace[ForbiddenIsOkExceptionMapper]
-    val request = mock[Request]
-    val response = exceptionManager.toResponse(request, new ForbiddenException)
-    response.status should equal(Status.Ok)
-  }
-
-  "replace the default exception mapper" in {
-    val exceptionManager = newExceptionManager
-    exceptionManager.replace[EverythingIsFineMapper]
-    val request = mock[Request]
-    val response = exceptionManager.toResponse(request, new Exception)
-    response.status should equal(Status.Ok)
-  }
 }
 
 class UnregisteredException extends Exception
@@ -89,11 +72,6 @@ class ForbiddenExceptionMapper extends ExceptionMapper[ForbiddenException] {
     new SimpleResponse(Status.Forbidden)
 }
 
-class ForbiddenIsOkExceptionMapper extends ExceptionMapper[ForbiddenException] {
-  def toResponse(request: Request, throwable: ForbiddenException): Response =
-    new SimpleResponse(Status.Ok)
-}
-
 class UnauthorizedExceptionMapper extends ExceptionMapper[UnauthorizedException] {
   def toResponse(request: Request, throwable: UnauthorizedException): Response =
     new SimpleResponse(Status.Unauthorized)
@@ -103,8 +81,3 @@ class UnauthorizedException1Mapper extends ExceptionMapper[UnauthorizedException
   def toResponse(request: Request, throwable: UnauthorizedException1): Response =
     new SimpleResponse(Status.NotFound)
 }
-
-class EverythingIsFineMapper extends ExceptionMapper[Throwable] {
-  def toResponse(request: Request, throwable: Throwable): Response =
-    new SimpleResponse(Status.Ok)
-}