From e5911d0f0fa382f37a22978a5bae1d6c4b47963f Mon Sep 17 00:00:00 2001 From: Zach Smith Date: Tue, 17 Oct 2017 14:57:31 -0700 Subject: Rename APIError -> ServiceException and subclasses --- .../driver/core/rest/errors/serviceException.scala | 41 ++++++++++++++++++++++ 1 file changed, 41 insertions(+) create mode 100644 src/main/scala/xyz/driver/core/rest/errors/serviceException.scala (limited to 'src/main/scala/xyz/driver/core/rest/errors/serviceException.scala') diff --git a/src/main/scala/xyz/driver/core/rest/errors/serviceException.scala b/src/main/scala/xyz/driver/core/rest/errors/serviceException.scala new file mode 100644 index 0000000..94f9734 --- /dev/null +++ b/src/main/scala/xyz/driver/core/rest/errors/serviceException.scala @@ -0,0 +1,41 @@ +package xyz.driver.core.rest.errors + +import akka.http.scaladsl.model.{StatusCode, StatusCodes} + +abstract class ServiceException extends Exception { + def isPatientSensitive: Boolean = false + + def statusCode: StatusCode + def message: String +} + +final case class InvalidInputException(override val message: String = "Invalid input", + override val isPatientSensitive: Boolean = false) + extends ServiceException { + override def statusCode: StatusCode = StatusCodes.BadRequest +} + +final case class InvalidActionException(override val message: String = "This action is not allowed", + override val isPatientSensitive: Boolean = false) + extends ServiceException { + override def statusCode: StatusCode = StatusCodes.Forbidden +} + +final case class ResourceNotFoundException(override val message: String = "Resource not found", + override val isPatientSensitive: Boolean = false) + extends ServiceException { + override def statusCode: StatusCode = StatusCodes.NotFound +} + +final case class ExternalServiceTimeoutException(override val message: String = + "Another service took too long to respond", + override val isPatientSensitive: Boolean = false) + extends ServiceException { + override def statusCode: StatusCode = StatusCodes.GatewayTimeout +} + +final case class DatabaseException(override val message: String = "Database access error", + override val isPatientSensitive: Boolean = false) + extends ServiceException { + override def statusCode: StatusCode = StatusCodes.InternalServerError +} -- cgit v1.2.3 From e7d849b55f975f1c119cebb22f95d9de903ae3c3 Mon Sep 17 00:00:00 2001 From: Zach Smith Date: Thu, 19 Oct 2017 13:49:22 -0700 Subject: Stop catching throwable, remove PHI filtering, move status code logic to exception handler --- .../scala/xyz/driver/core/rest/DriverRoute.scala | 54 +++++++++++++++------- .../driver/core/rest/errors/serviceException.scala | 40 ++++------------ 2 files changed, 46 insertions(+), 48 deletions(-) (limited to 'src/main/scala/xyz/driver/core/rest/errors/serviceException.scala') diff --git a/src/main/scala/xyz/driver/core/rest/DriverRoute.scala b/src/main/scala/xyz/driver/core/rest/DriverRoute.scala index f3260d0..be9c783 100644 --- a/src/main/scala/xyz/driver/core/rest/DriverRoute.scala +++ b/src/main/scala/xyz/driver/core/rest/DriverRoute.scala @@ -3,14 +3,14 @@ package xyz.driver.core.rest import java.sql.SQLException import akka.http.scaladsl.model._ -import akka.http.scaladsl.model.StatusCodes.{BadRequest, Conflict, InternalServerError} +import akka.http.scaladsl.model.StatusCodes import akka.http.scaladsl.model.headers._ import akka.http.scaladsl.server.Directives._ import akka.http.scaladsl.server.{ExceptionHandler, RequestContext, Route} import com.typesafe.scalalogging.Logger import org.slf4j.MDC import xyz.driver.core.rest -import xyz.driver.core.rest.errors.ServiceException +import xyz.driver.core.rest.errors._ import scala.compat.Platform.ConcurrentModificationException @@ -29,38 +29,58 @@ trait DriverRoute { * @return Exception handling route for exception type */ protected def exceptionHandler: PartialFunction[Throwable, Route] = { - case api: ServiceException if api.isPatientSensitive => - ctx => - log.info("PHI Sensitive error") - errorResponse(ctx, InternalServerError, "Server error", api)(ctx) - - case api: ServiceException => - ctx => - log.info("API Error") - errorResponse(ctx, api.statusCode, api.message, api)(ctx) + case serviceException: ServiceException => + serviceExceptionHandler(serviceException) case is: IllegalStateException => ctx => log.warn(s"Request is not allowed to ${ctx.request.method} ${ctx.request.uri}", is) - errorResponse(ctx, BadRequest, message = is.getMessage, is)(ctx) + errorResponse(ctx, StatusCodes.BadRequest, message = is.getMessage, is)(ctx) case cm: ConcurrentModificationException => ctx => log.warn(s"Concurrent modification of the resource ${ctx.request.method} ${ctx.request.uri}", cm) - errorResponse(ctx, Conflict, "Resource was changed concurrently, try requesting a newer version", cm)(ctx) + errorResponse(ctx, + StatusCodes.Conflict, + "Resource was changed concurrently, try requesting a newer version", + cm)(ctx) case se: SQLException => ctx => log.warn(s"Database exception for the resource ${ctx.request.method} ${ctx.request.uri}", se) - errorResponse(ctx, InternalServerError, "Data access error", se)(ctx) + errorResponse(ctx, StatusCodes.InternalServerError, "Data access error", se)(ctx) - case t: Throwable => + case t: Exception => ctx => log.warn(s"Request to ${ctx.request.method} ${ctx.request.uri} could not be handled normally", t) - errorResponse(ctx, InternalServerError, t.getMessage, t)(ctx) + errorResponse(ctx, StatusCodes.InternalServerError, t.getMessage, t)(ctx) + } + + protected def serviceExceptionHandler(serviceException: ServiceException): Route = { + val statusCode = serviceException match { + case e: InvalidInputException => + log.info("Invalid client input error", e) + StatusCodes.BadRequest + case e: InvalidActionException => + log.info("Invalid client action error", e) + StatusCodes.Forbidden + case e: ResourceNotFoundException => + log.info("Resource not found error", e) + StatusCodes.NotFound + case e: ExternalServiceTimeoutException => + log.error("Service timeout error", e) + StatusCodes.GatewayTimeout + case e: DatabaseException => + log.error("Database error", e) + StatusCodes.InternalServerError + } + + { (ctx: RequestContext) => + errorResponse(ctx, statusCode, serviceException.message, serviceException)(ctx) + } } - protected def errorResponse[T <: Throwable](ctx: RequestContext, + protected def errorResponse[T <: Exception](ctx: RequestContext, statusCode: StatusCode, message: String, exception: T): Route = { diff --git a/src/main/scala/xyz/driver/core/rest/errors/serviceException.scala b/src/main/scala/xyz/driver/core/rest/errors/serviceException.scala index 94f9734..7aa70bf 100644 --- a/src/main/scala/xyz/driver/core/rest/errors/serviceException.scala +++ b/src/main/scala/xyz/driver/core/rest/errors/serviceException.scala @@ -1,41 +1,19 @@ package xyz.driver.core.rest.errors -import akka.http.scaladsl.model.{StatusCode, StatusCodes} - abstract class ServiceException extends Exception { - def isPatientSensitive: Boolean = false - - def statusCode: StatusCode def message: String } -final case class InvalidInputException(override val message: String = "Invalid input", - override val isPatientSensitive: Boolean = false) - extends ServiceException { - override def statusCode: StatusCode = StatusCodes.BadRequest -} +final case class InvalidInputException(override val message: String = "Invalid input") extends ServiceException -final case class InvalidActionException(override val message: String = "This action is not allowed", - override val isPatientSensitive: Boolean = false) - extends ServiceException { - override def statusCode: StatusCode = StatusCodes.Forbidden -} +final case class InvalidActionException(override val message: String = "This action is not allowed") + extends ServiceException -final case class ResourceNotFoundException(override val message: String = "Resource not found", - override val isPatientSensitive: Boolean = false) - extends ServiceException { - override def statusCode: StatusCode = StatusCodes.NotFound -} +final case class ResourceNotFoundException(override val message: String = "Resource not found") + extends ServiceException -final case class ExternalServiceTimeoutException(override val message: String = - "Another service took too long to respond", - override val isPatientSensitive: Boolean = false) - extends ServiceException { - override def statusCode: StatusCode = StatusCodes.GatewayTimeout -} +final case class ExternalServiceTimeoutException( + override val message: String = "Another service took too long to respond") + extends ServiceException -final case class DatabaseException(override val message: String = "Database access error", - override val isPatientSensitive: Boolean = false) - extends ServiceException { - override def statusCode: StatusCode = StatusCodes.InternalServerError -} +final case class DatabaseException(override val message: String = "Database access error") extends ServiceException -- cgit v1.2.3 From 5f3330ffd4df8d87b97d88789aced1b1b8f7410d Mon Sep 17 00:00:00 2001 From: Zach Smith Date: Mon, 30 Oct 2017 20:27:22 -0700 Subject: Add ExternalServiceException and use in ServiceTransport --- .../core/rest/HttpRestServiceTransport.scala | 24 ++++++++++++++++------ .../driver/core/rest/errors/serviceException.scala | 4 ++++ src/main/scala/xyz/driver/core/rest/package.scala | 4 +++- 3 files changed, 25 insertions(+), 7 deletions(-) (limited to 'src/main/scala/xyz/driver/core/rest/errors/serviceException.scala') diff --git a/src/main/scala/xyz/driver/core/rest/HttpRestServiceTransport.scala b/src/main/scala/xyz/driver/core/rest/HttpRestServiceTransport.scala index 1e95811..2bec4c3 100644 --- a/src/main/scala/xyz/driver/core/rest/HttpRestServiceTransport.scala +++ b/src/main/scala/xyz/driver/core/rest/HttpRestServiceTransport.scala @@ -4,9 +4,12 @@ import akka.actor.ActorSystem import akka.http.scaladsl.model._ import akka.http.scaladsl.model.headers.RawHeader import akka.http.scaladsl.unmarshalling.Unmarshal +import akka.stream.Materializer +import akka.stream.scaladsl.TcpIdleTimeoutException import com.typesafe.scalalogging.Logger import org.slf4j.MDC import xyz.driver.core.Name +import xyz.driver.core.rest.errors.{ExternalServiceException, ExternalServiceTimeoutException} import xyz.driver.core.time.provider.TimeProvider import scala.concurrent.{ExecutionContext, Future} @@ -55,18 +58,27 @@ class HttpRestServiceTransport(applicationName: Name[App], log.warn(s"Failed to receive response from ${request.method} ${request.uri} in $responseLatency ms", t) }(executionContext) - response + response.transformWith { + case Success(r) => Future.successful(r) + case Failure(_: TcpIdleTimeoutException) => + Future.failed(ExternalServiceTimeoutException()) + case Failure(t: Throwable) => Future.failed(t) + } } - def sendRequest(context: ServiceRequestContext)(requestStub: HttpRequest): Future[Unmarshal[ResponseEntity]] = { + def sendRequest(context: ServiceRequestContext)(requestStub: HttpRequest)( + implicit mat: Materializer): Future[Unmarshal[ResponseEntity]] = { - sendRequestGetResponse(context)(requestStub) map { response => + sendRequestGetResponse(context)(requestStub) flatMap { response => if (response.status == StatusCodes.NotFound) { - Unmarshal(HttpEntity.Empty: ResponseEntity) + Future.successful(Unmarshal(HttpEntity.Empty: ResponseEntity)) } else if (response.status.isFailure()) { - throw new Exception(s"Http status is failure ${response.status} for ${requestStub.method} ${requestStub.uri}") + val serviceCalled = s"${requestStub.method} ${requestStub.uri}" + Unmarshal(response.entity).to[String] flatMap { error => + Future.failed(ExternalServiceException(serviceCalled, error)) + } } else { - Unmarshal(response.entity) + Future.successful(Unmarshal(response.entity)) } } } diff --git a/src/main/scala/xyz/driver/core/rest/errors/serviceException.scala b/src/main/scala/xyz/driver/core/rest/errors/serviceException.scala index 7aa70bf..82a1838 100644 --- a/src/main/scala/xyz/driver/core/rest/errors/serviceException.scala +++ b/src/main/scala/xyz/driver/core/rest/errors/serviceException.scala @@ -12,6 +12,10 @@ final case class InvalidActionException(override val message: String = "This act final case class ResourceNotFoundException(override val message: String = "Resource not found") extends ServiceException +final case class ExternalServiceException(serviceName: String, serviceMessage: String) extends ServiceException { + override def message = s"Error while calling another service: $serviceMessage" +} + final case class ExternalServiceTimeoutException( override val message: String = "Another service took too long to respond") extends ServiceException diff --git a/src/main/scala/xyz/driver/core/rest/package.scala b/src/main/scala/xyz/driver/core/rest/package.scala index 942ca3a..531cd8a 100644 --- a/src/main/scala/xyz/driver/core/rest/package.scala +++ b/src/main/scala/xyz/driver/core/rest/package.scala @@ -6,6 +6,7 @@ import akka.http.scaladsl.model.{HttpRequest, HttpResponse, ResponseEntity, Stat import akka.http.scaladsl.server.Directives._ import akka.http.scaladsl.server._ import akka.http.scaladsl.unmarshalling.Unmarshal +import akka.stream.Materializer import akka.stream.scaladsl.Flow import akka.util.ByteString import xyz.driver.tracing.TracingDirectives @@ -25,7 +26,8 @@ trait ServiceTransport { def sendRequestGetResponse(context: ServiceRequestContext)(requestStub: HttpRequest): Future[HttpResponse] - def sendRequest(context: ServiceRequestContext)(requestStub: HttpRequest): Future[Unmarshal[ResponseEntity]] + def sendRequest(context: ServiceRequestContext)(requestStub: HttpRequest)( + implicit mat: Materializer): Future[Unmarshal[ResponseEntity]] } final case class Pagination(pageSize: Int, pageNumber: Int) -- cgit v1.2.3 From c4fc9d9496ba382eb371a65860caf2cd49261a06 Mon Sep 17 00:00:00 2001 From: Zach Smith Date: Tue, 31 Oct 2017 09:09:13 -0700 Subject: Change transformWith to recoverWith --- .../scala/xyz/driver/core/rest/HttpRestServiceTransport.scala | 10 +++++----- .../scala/xyz/driver/core/rest/errors/serviceException.scala | 6 +++--- 2 files changed, 8 insertions(+), 8 deletions(-) (limited to 'src/main/scala/xyz/driver/core/rest/errors/serviceException.scala') diff --git a/src/main/scala/xyz/driver/core/rest/HttpRestServiceTransport.scala b/src/main/scala/xyz/driver/core/rest/HttpRestServiceTransport.scala index 2bec4c3..376b154 100644 --- a/src/main/scala/xyz/driver/core/rest/HttpRestServiceTransport.scala +++ b/src/main/scala/xyz/driver/core/rest/HttpRestServiceTransport.scala @@ -58,11 +58,11 @@ class HttpRestServiceTransport(applicationName: Name[App], log.warn(s"Failed to receive response from ${request.method} ${request.uri} in $responseLatency ms", t) }(executionContext) - response.transformWith { - case Success(r) => Future.successful(r) - case Failure(_: TcpIdleTimeoutException) => - Future.failed(ExternalServiceTimeoutException()) - case Failure(t: Throwable) => Future.failed(t) + response.recoverWith { + case _: TcpIdleTimeoutException => + val serviceCalled = s"${requestStub.method} ${requestStub.uri}" + Future.failed(ExternalServiceTimeoutException(serviceCalled)) + case t: Throwable => Future.failed(t) } } diff --git a/src/main/scala/xyz/driver/core/rest/errors/serviceException.scala b/src/main/scala/xyz/driver/core/rest/errors/serviceException.scala index 82a1838..ca1f759 100644 --- a/src/main/scala/xyz/driver/core/rest/errors/serviceException.scala +++ b/src/main/scala/xyz/driver/core/rest/errors/serviceException.scala @@ -16,8 +16,8 @@ final case class ExternalServiceException(serviceName: String, serviceMessage: S override def message = s"Error while calling another service: $serviceMessage" } -final case class ExternalServiceTimeoutException( - override val message: String = "Another service took too long to respond") - extends ServiceException +final case class ExternalServiceTimeoutException(serviceName: String) extends ServiceException { + override def message = s"$serviceName took too long to respond" +} final case class DatabaseException(override val message: String = "Database access error") extends ServiceException -- cgit v1.2.3 From 595d199f5e41c8e48131cec98b23452bc7ed6ef1 Mon Sep 17 00:00:00 2001 From: Zach Smith Date: Wed, 1 Nov 2017 09:24:16 -0700 Subject: Add DriverRouteTest --- .../scala/xyz/driver/core/rest/DriverRoute.scala | 3 + .../driver/core/rest/errors/serviceException.scala | 4 +- src/test/scala/xyz/driver/core/RestTest.scala | 16 ---- .../xyz/driver/core/rest/DriverRouteTest.scala | 89 ++++++++++++++++++++++ src/test/scala/xyz/driver/core/rest/RestTest.scala | 15 ++++ 5 files changed, 109 insertions(+), 18 deletions(-) delete mode 100644 src/test/scala/xyz/driver/core/RestTest.scala create mode 100644 src/test/scala/xyz/driver/core/rest/DriverRouteTest.scala create mode 100644 src/test/scala/xyz/driver/core/rest/RestTest.scala (limited to 'src/main/scala/xyz/driver/core/rest/errors/serviceException.scala') diff --git a/src/main/scala/xyz/driver/core/rest/DriverRoute.scala b/src/main/scala/xyz/driver/core/rest/DriverRoute.scala index 9af6657..eb9a31a 100644 --- a/src/main/scala/xyz/driver/core/rest/DriverRoute.scala +++ b/src/main/scala/xyz/driver/core/rest/DriverRoute.scala @@ -82,6 +82,9 @@ trait DriverRoute { case e: ResourceNotFoundException => log.info("Resource not found error", e) StatusCodes.NotFound + case e: ExternalServiceException => + log.error("Error while calling another service", e) + StatusCodes.InternalServerError case e: ExternalServiceTimeoutException => log.error("Service timeout error", e) StatusCodes.GatewayTimeout diff --git a/src/main/scala/xyz/driver/core/rest/errors/serviceException.scala b/src/main/scala/xyz/driver/core/rest/errors/serviceException.scala index ca1f759..e91a3c2 100644 --- a/src/main/scala/xyz/driver/core/rest/errors/serviceException.scala +++ b/src/main/scala/xyz/driver/core/rest/errors/serviceException.scala @@ -1,6 +1,6 @@ package xyz.driver.core.rest.errors -abstract class ServiceException extends Exception { +sealed abstract class ServiceException extends Exception { def message: String } @@ -13,7 +13,7 @@ final case class ResourceNotFoundException(override val message: String = "Resou extends ServiceException final case class ExternalServiceException(serviceName: String, serviceMessage: String) extends ServiceException { - override def message = s"Error while calling another service: $serviceMessage" + override def message = s"Error while calling '$serviceName': $serviceMessage" } final case class ExternalServiceTimeoutException(serviceName: String) extends ServiceException { diff --git a/src/test/scala/xyz/driver/core/RestTest.scala b/src/test/scala/xyz/driver/core/RestTest.scala deleted file mode 100644 index efb9d07..0000000 --- a/src/test/scala/xyz/driver/core/RestTest.scala +++ /dev/null @@ -1,16 +0,0 @@ -package xyz.driver.core.rest - -import org.scalatest.{FlatSpec, Matchers} - -import akka.util.ByteString - -class RestTest extends FlatSpec with Matchers { - "`escapeScriptTags` function" should "escap script tags properly" in { - val dirtyString = " akkaComplete} +import akka.http.scaladsl.server.Route +import akka.http.scaladsl.testkit.ScalatestRouteTest +import com.typesafe.scalalogging.Logger +import org.scalatest.{AsyncFlatSpec, Matchers} +import xyz.driver.core.logging.NoLogger +import xyz.driver.core.rest.errors._ + +import scala.concurrent.Future + +class DriverRouteTest extends AsyncFlatSpec with ScalatestRouteTest with Matchers { + class TestRoute(override val route: Route) extends DriverRoute { + override def log: Logger = NoLogger + } + + "DriverRoute" should "respond with 200 OK for a basic route" in { + val route = new TestRoute(akkaComplete(StatusCodes.OK)) + + Get("/api/v1/foo/bar") ~> route.routeWithDefaults ~> check { + handled shouldBe true + status shouldBe StatusCodes.OK + } + } + + it should "respond with a 400 for an InvalidInputException" in { + val route = new TestRoute(akkaComplete(Future.failed[String](InvalidInputException()))) + + Post("/api/v1/foo/bar") ~> route.routeWithDefaults ~> check { + handled shouldBe true + status shouldBe StatusCodes.BadRequest + responseAs[String] shouldBe "Invalid input" + } + } + + it should "respond with a 400 for InvalidActionException" in { + val route = new TestRoute(akkaComplete(Future.failed[String](InvalidActionException()))) + + Post("/api/v1/foo/bar") ~> route.routeWithDefaults ~> check { + handled shouldBe true + status shouldBe StatusCodes.Forbidden + responseAs[String] shouldBe "This action is not allowed" + } + } + + it should "respond with a 404 for ResourceNotFoundException" in { + val route = new TestRoute(akkaComplete(Future.failed[String](ResourceNotFoundException()))) + + Post("/api/v1/foo/bar") ~> route.routeWithDefaults ~> check { + handled shouldBe true + status shouldBe StatusCodes.NotFound + responseAs[String] shouldBe "Resource not found" + } + } + + it should "respond with a 500 for ExternalServiceException" in { + val error = ExternalServiceException("GET /api/v1/users/", "Permission denied") + val route = new TestRoute(akkaComplete(Future.failed[String](error))) + + Post("/api/v1/foo/bar") ~> route.routeWithDefaults ~> check { + handled shouldBe true + status shouldBe StatusCodes.InternalServerError + responseAs[String] shouldBe "Error while calling 'GET /api/v1/users/': Permission denied" + } + } + + it should "respond with a 503 for ExternalServiceTimeoutException" in { + val error = ExternalServiceTimeoutException("GET /api/v1/users/") + val route = new TestRoute(akkaComplete(Future.failed[String](error))) + + Post("/api/v1/foo/bar") ~> route.routeWithDefaults ~> check { + handled shouldBe true + status shouldBe StatusCodes.GatewayTimeout + responseAs[String] shouldBe "GET /api/v1/users/ took too long to respond" + } + } + + it should "respond with a 500 for DatabaseException" in { + val route = new TestRoute(akkaComplete(Future.failed[String](DatabaseException()))) + + Post("/api/v1/foo/bar") ~> route.routeWithDefaults ~> check { + handled shouldBe true + status shouldBe StatusCodes.InternalServerError + responseAs[String] shouldBe "Database access error" + } + } +} diff --git a/src/test/scala/xyz/driver/core/rest/RestTest.scala b/src/test/scala/xyz/driver/core/rest/RestTest.scala new file mode 100644 index 0000000..2c3fb7f --- /dev/null +++ b/src/test/scala/xyz/driver/core/rest/RestTest.scala @@ -0,0 +1,15 @@ +package xyz.driver.core.rest + +import akka.util.ByteString +import org.scalatest.{FlatSpec, Matchers} + +class RestTest extends FlatSpec with Matchers { + "`escapeScriptTags` function" should "escap script tags properly" in { + val dirtyString = "