From 60dccc96bd7803900c4d961639733676bb202dfc Mon Sep 17 00:00:00 2001 From: Eugene Ma Date: Tue, 28 Jul 2015 00:04:35 +0000 Subject: [PATCH] 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 --- .../CancelledRequestExceptionMapper.scala | 16 ------- .../exceptions/ExceptionManager.scala | 42 +++++++------------ .../FinatraDefaultExceptionMapper.scala | 8 +++- .../exceptions/HttpExceptionMapper.scala | 15 ------- .../HttpResponseExceptionMapper.scala | 12 ------ .../http/modules/ExceptionMapperModule.scala | 11 ++--- .../exceptions/ExceptionManagerTest.scala | 27 ------------ 7 files changed, 26 insertions(+), 105 deletions(-) delete mode 100644 http/src/main/scala/com/twitter/finatra/http/internal/exceptions/CancelledRequestExceptionMapper.scala delete mode 100644 http/src/main/scala/com/twitter/finatra/http/internal/exceptions/HttpExceptionMapper.scala delete mode 100644 http/src/main/scala/com/twitter/finatra/http/internal/exceptions/HttpResponseExceptionMapper.scala diff --git a/http/src/main/scala/com/twitter/finatra/http/internal/exceptions/CancelledRequestExceptionMapper.scala b/http/src/main/scala/com/twitter/finatra/http/internal/exceptions/CancelledRequestExceptionMapper.scala deleted file mode 100644 index be1047d889..0000000000 --- a/http/src/main/scala/com/twitter/finatra/http/internal/exceptions/CancelledRequestExceptionMapper.scala +++ /dev/null @@ -1,16 +0,0 @@ -package com.twitter.finatra.http.internal.exceptions - -import com.twitter.finagle.CancelledRequestException -import com.twitter.finagle.http.{Request, Response} -import com.twitter.finatra.http.exceptions._ -import com.twitter.finatra.http.response.ResponseBuilder -import javax.inject.{Inject, Singleton} - -@Singleton -class CancelledRequestExceptionMapper @Inject()( - response: ResponseBuilder) - extends ExceptionMapper[CancelledRequestException] { - - override def toResponse(request: Request, throwable: CancelledRequestException): Response = - response.clientClosed -} diff --git a/http/src/main/scala/com/twitter/finatra/http/internal/exceptions/ExceptionManager.scala b/http/src/main/scala/com/twitter/finatra/http/internal/exceptions/ExceptionManager.scala index 9d8e820545..97fb7f6239 100644 --- a/http/src/main/scala/com/twitter/finatra/http/internal/exceptions/ExceptionManager.scala +++ b/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) + } } } } diff --git a/http/src/main/scala/com/twitter/finatra/http/internal/exceptions/FinatraDefaultExceptionMapper.scala b/http/src/main/scala/com/twitter/finatra/http/internal/exceptions/FinatraDefaultExceptionMapper.scala index 63c88804aa..b0a04e7007 100644 --- a/http/src/main/scala/com/twitter/finatra/http/internal/exceptions/FinatraDefaultExceptionMapper.scala +++ b/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( diff --git a/http/src/main/scala/com/twitter/finatra/http/internal/exceptions/HttpExceptionMapper.scala b/http/src/main/scala/com/twitter/finatra/http/internal/exceptions/HttpExceptionMapper.scala deleted file mode 100644 index afcafe8652..0000000000 --- a/http/src/main/scala/com/twitter/finatra/http/internal/exceptions/HttpExceptionMapper.scala +++ /dev/null @@ -1,15 +0,0 @@ -package com.twitter.finatra.http.internal.exceptions - -import com.twitter.finagle.http.{Request, Response} -import com.twitter.finatra.http.exceptions._ -import com.twitter.finatra.http.response.ResponseBuilder -import javax.inject.{Inject, Singleton} - -@Singleton -class HttpExceptionMapper @Inject()( - response: ResponseBuilder) - extends ExceptionMapper[HttpException] { - - override def toResponse(request: Request, throwable: HttpException): Response = - throwable.createResponse(response) -} diff --git a/http/src/main/scala/com/twitter/finatra/http/internal/exceptions/HttpResponseExceptionMapper.scala b/http/src/main/scala/com/twitter/finatra/http/internal/exceptions/HttpResponseExceptionMapper.scala deleted file mode 100644 index 955f66b082..0000000000 --- a/http/src/main/scala/com/twitter/finatra/http/internal/exceptions/HttpResponseExceptionMapper.scala +++ /dev/null @@ -1,12 +0,0 @@ -package com.twitter.finatra.http.internal.exceptions - -import com.twitter.finagle.http.{Request, Response} -import com.twitter.finatra.http.exceptions._ -import javax.inject.Singleton - -@Singleton -class HttpResponseExceptionMapper extends ExceptionMapper[HttpResponseException] { - - override def toResponse(request: Request, throwable: HttpResponseException): Response = - throwable.response -} diff --git a/http/src/main/scala/com/twitter/finatra/http/modules/ExceptionMapperModule.scala b/http/src/main/scala/com/twitter/finatra/http/modules/ExceptionMapperModule.scala index 4293c6194c..3052140073 100644 --- a/http/src/main/scala/com/twitter/finatra/http/modules/ExceptionMapperModule.scala +++ b/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] } } diff --git a/http/src/test/scala/com/twitter/finatra/http/internal/exceptions/ExceptionManagerTest.scala b/http/src/test/scala/com/twitter/finatra/http/internal/exceptions/ExceptionManagerTest.scala index 63ac43494f..51d4b3a95d 100644 --- a/http/src/test/scala/com/twitter/finatra/http/internal/exceptions/ExceptionManagerTest.scala +++ b/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) -}