Return appropriate return code for wrong JSON request (#47)

avast · Sep 13, 2019 · 1c72320 · 1c72320
1 parent 58c5f2c
commit 1c72320
Show file tree

Hide file tree

Showing 9 changed files with 54 additions and 37 deletions.
diff --git a/akka-http/src/main/scala/com/avast/grpc/jsonbridge/akkahttp/AkkaHttp.scala b/akka-http/src/main/scala/com/avast/grpc/jsonbridge/akkahttp/AkkaHttp.scala
@@ -70,21 +70,21 @@ object AkkaHttp extends SprayJsonSupport with DefaultJsonProtocol with LazyLoggi
                             respondWithHeader(JsonContentType) {
                               complete(StatusCodes.NotFound, BridgeErrorResponse.fromMessage(message))
                             }
-                          case er: BridgeError.RequestJsonParseError =>
-                            val message = "Cannot parse JSON request body"
+                          case er: BridgeError.Json =>
+                            val message = "Wrong JSON"
                             logger.debug(message, er.t)
                             respondWithHeader(JsonContentType) {
                               complete(StatusCodes.BadRequest, BridgeErrorResponse.fromException(message, er.t))
                             }
-                          case er: BridgeError.RequestErrorGrpc =>
-                            val message = "gRPC request error" + Option(er.s.getDescription).map(": " + _).getOrElse("")
+                          case er: BridgeError.Grpc =>
+                            val message = "gRPC error" + Option(er.s.getDescription).map(": " + _).getOrElse("")
                             logger.trace(message, er.s.getCause)
                             val (s, body) = mapStatus(er.s)
                             respondWithHeader(JsonContentType) {
                               complete(s, body)
                             }
-                          case er: BridgeError.RequestError =>
-                            val message = "Unknown request error"
+                          case er: BridgeError.Unknown =>
+                            val message = "Unknown error"
                             logger.warn(message, er.t)
                             respondWithHeader(JsonContentType) {
                               complete(StatusCodes.InternalServerError, BridgeErrorResponse.fromException(message, er.t))

diff --git a/core-scalapb/src/main/scala/com/avast/grpc/jsonbridge/scalapb/ScalaPBServiceHandlers.scala b/core-scalapb/src/main/scala/com/avast/grpc/jsonbridge/scalapb/ScalaPBServiceHandlers.scala
@@ -5,20 +5,21 @@ import java.lang.reflect.{InvocationTargetException, Method}
 import cats.effect.Async
 import cats.implicits._
 import com.avast.grpc.jsonbridge.GrpcJsonBridge.GrpcMethodName
-import com.avast.grpc.jsonbridge.{BridgeError, JavaGenericHelper, ReflectionGrpcJsonBridge}
 import com.avast.grpc.jsonbridge.ReflectionGrpcJsonBridge.{HandlerFunc, ServiceHandlers}
-import com.fasterxml.jackson.core.JsonParseException
+import com.avast.grpc.jsonbridge.{BridgeError, JavaGenericHelper, ReflectionGrpcJsonBridge}
+import com.fasterxml.jackson.core.{JsonParseException, JsonProcessingException}
 import com.typesafe.scalalogging.StrictLogging
 import io.grpc._
 import io.grpc.protobuf.ProtoFileDescriptorSupplier
 import io.grpc.stub.AbstractStub
+import scalapb.json4s.JsonFormatException
 import scalapb.{GeneratedMessage, GeneratedMessageCompanion}
 
 import scala.collection.JavaConverters._
 import scala.concurrent.{ExecutionContext, Future}
 import scala.language.{existentials, higherKinds}
 import scala.util.control.NonFatal
-import scala.util.{Failure, Success, Try}
+import scala.util.{Failure, Success}
 
 private[jsonbridge] object ScalaPBServiceHandlers extends ServiceHandlers with StrictLogging {
   def createServiceHandlers[F[+ _]](ec: ExecutionContext)(inProcessChannel: ManagedChannel)(ssd: ServerServiceDefinition)(
@@ -42,10 +43,11 @@ private[jsonbridge] object ScalaPBServiceHandlers extends ServiceHandlers with S
     .find(_.getName == "fromJsonString")
     .getOrElse(sys.error(s"Method 'fromJsonString' not found on ${parser.getClass}"))
   private def parse(input: String, companion: GeneratedMessageCompanion[_]): Either[Throwable, GeneratedMessage] =
-    Try(parserMethod.invoke(parser, input, companion).asInstanceOf[GeneratedMessage]) match {
-      case Failure(ie: InvocationTargetException) => Left(ie.getCause)
-      case Failure(e) => Left(e)
-      case Success(m) => Right(m)
+    try {
+      Right(parserMethod.invoke(parser, input, companion).asInstanceOf[GeneratedMessage])
+    } catch {
+      case ie: InvocationTargetException => Left(ie.getCause)
+      case NonFatal(e) => Left(e)
     }
 
   private def createFutureStubCtor(sd: ServiceDescriptor, inProcessChannel: Channel): () => AbstractStub[_] = {
@@ -91,8 +93,9 @@ private[jsonbridge] object ScalaPBServiceHandlers extends ServiceHandlers with S
     val scalaMethod = futureStubCtor().getClass.getDeclaredMethod(getScalaMethodName(method), requestClass)
     val handler: HandlerFunc[F] = (input: String, headers: Map[String, String]) =>
       parse(input, requestCompanion) match {
-        case Left(e: JsonParseException) => F.pure(Left(BridgeError.RequestJsonParseError(e)))
-        case Left(e) => F.pure(Left(BridgeError.RequestError(e)))
+        case Left(e: JsonProcessingException) => F.pure(Left(BridgeError.Json(e)))
+        case Left(e: JsonFormatException) => F.pure(Left(BridgeError.Json(e)))
+        case Left(e) => F.pure(Left(BridgeError.Unknown(e)))
         case Right(request) =>
           fromScalaFuture(ec) {
             F.delay {
@@ -121,11 +124,11 @@ private[jsonbridge] object ScalaPBServiceHandlers extends ServiceHandlers with S
       .map(Right(_): Either[BridgeError.Narrow, String])
       .recover {
         case e: StatusException =>
-          Left(BridgeError.RequestErrorGrpc(e.getStatus))
+          Left(BridgeError.Grpc(e.getStatus))
         case e: StatusRuntimeException =>
-          Left(BridgeError.RequestErrorGrpc(e.getStatus))
+          Left(BridgeError.Grpc(e.getStatus))
         case NonFatal(ex) =>
-          Left(BridgeError.RequestError(ex))
+          Left(BridgeError.Unknown(ex))
       }
   }
 
@@ -145,7 +148,7 @@ private[jsonbridge] object ScalaPBServiceHandlers extends ServiceHandlers with S
     F.async { cb =>
       sf.onComplete {
         case Success(r) => cb(Right(r))
-        case Failure(e) => cb(Left(BridgeError.RequestError(e)))
+        case Failure(e) => cb(Left(BridgeError.Unknown(e)))
       }(ec)
     }
   }

diff --git a/...est/scala/com/avast/grpc/jsonbridge/scalapbtest/ScalaPBReflectionGrpcJsonBridgeTest.scala b/...est/scala/com/avast/grpc/jsonbridge/scalapbtest/ScalaPBReflectionGrpcJsonBridgeTest.scala
@@ -44,10 +44,17 @@ class ScalaPBReflectionGrpcJsonBridgeTest extends fixture.FlatSpec with Matchers
     status shouldBe BridgeError.GrpcMethodNotFound
   }
 
+  it must "return BridgeError.Json for wrongly named field" in { f =>
+    val Left(status) = f.bridge
+      .invoke(GrpcMethodName("com.avast.grpc.jsonbridge.scalapbtest.TestService/Add"), """ { "x": 1, "b": 2} """, Map.empty)
+      .unsafeRunSync()
+    status should matchPattern { case BridgeError.Json(_) => }
+  }
+
   it must "return expected status code for malformed JSON" in { f =>
     val Left(status) =
       f.bridge.invoke(GrpcMethodName("com.avast.grpc.jsonbridge.scalapbtest.TestService/Add"), "{ble}", Map.empty).unsafeRunSync()
-    status should matchPattern { case BridgeError.RequestJsonParseError(_) => }
+    status should matchPattern { case BridgeError.Json(_) => }
   }
 
 }
diff --git a/...st/scala/com/avast/grpc/jsonbridge/scalapbtest/ScalaPBReflectionGrpcJsonBridgeTest2.scala b/...st/scala/com/avast/grpc/jsonbridge/scalapbtest/ScalaPBReflectionGrpcJsonBridgeTest2.scala
@@ -7,7 +7,7 @@ import com.avast.grpc.jsonbridge.scalapbtest.TestServices2.TestService2Grpc
 import com.avast.grpc.jsonbridge.{BridgeError, GrpcJsonBridge}
 import io.grpc.inprocess.InProcessServerBuilder
 import io.grpc.protobuf.services.ProtoReflectionService
-import org.scalatest.{Matchers, Outcome, fixture}
+import org.scalatest.{fixture, Matchers, Outcome}
 
 import scala.concurrent.ExecutionContext.Implicits.global
 
@@ -47,7 +47,7 @@ class ScalaPBReflectionGrpcJsonBridgeTest2 extends fixture.FlatSpec with Matcher
   it must "return expected status code for malformed JSON" in { f =>
     val Left(status) =
       f.bridge.invoke(GrpcMethodName("com.avast.grpc.jsonbridge.scalapbtest.TestService2/Add2"), "{ble}", Map.empty).unsafeRunSync()
-    status should matchPattern { case BridgeError.RequestJsonParseError(_) => }
+    status should matchPattern { case BridgeError.Json(_) => }
   }
 
 }
diff --git a/core/src/main/scala/com/avast/grpc/jsonbridge/BridgeError.scala b/core/src/main/scala/com/avast/grpc/jsonbridge/BridgeError.scala
@@ -7,7 +7,7 @@ object BridgeError {
   final case object GrpcMethodNotFound extends BridgeError
 
   sealed trait Narrow extends BridgeError
-  final case class RequestJsonParseError(t: Throwable) extends Narrow
-  final case class RequestErrorGrpc(s: Status) extends Narrow
-  final case class RequestError(t: Throwable) extends Narrow
+  final case class Json(t: Throwable) extends Narrow
+  final case class Grpc(s: Status) extends Narrow
+  final case class Unknown(t: Throwable) extends Narrow
 }
diff --git a/core/src/main/scala/com/avast/grpc/jsonbridge/JavaServiceHandlers.scala b/core/src/main/scala/com/avast/grpc/jsonbridge/JavaServiceHandlers.scala
@@ -72,15 +72,15 @@ private[jsonbridge] object JavaServiceHandlers extends ServiceHandlers with Stri
             .map(Right(_): Either[BridgeError.Narrow, String])
             .recover {
               case e: StatusException =>
-                Left(BridgeError.RequestErrorGrpc(e.getStatus))
+                Left(BridgeError.Grpc(e.getStatus))
               case e: StatusRuntimeException =>
-                Left(BridgeError.RequestErrorGrpc(e.getStatus))
+                Left(BridgeError.Grpc(e.getStatus))
               case NonFatal(ex) =>
-                Left(BridgeError.RequestError(ex))
+                Left(BridgeError.Unknown(ex))
             }
         case Left(ex) =>
           F.pure {
-            Left(BridgeError.RequestJsonParseError(ex))
+            Left(BridgeError.Json(ex))
           }
       }
     }

diff --git a/core/src/test/scala/com/avast/grpc/jsonbridge/ReflectionGrpcJsonBridgeTest.scala b/core/src/test/scala/com/avast/grpc/jsonbridge/ReflectionGrpcJsonBridgeTest.scala
@@ -41,10 +41,17 @@ class ReflectionGrpcJsonBridgeTest extends fixture.FlatSpec with Matchers {
     status shouldBe BridgeError.GrpcMethodNotFound
   }
 
+  it must "return BridgeError.Json for wrongly named field" in { f =>
+    val Left(status) = f.bridge
+      .invoke(GrpcMethodName("com.avast.grpc.jsonbridge.test.TestService/Add"), """ { "x": 1, "b": 2} """, Map.empty)
+      .unsafeRunSync()
+    status should matchPattern { case BridgeError.Json(_) => }
+  }
+
   it must "return expected status code for malformed JSON" in { f =>
     val Left(status) =
       f.bridge.invoke(GrpcMethodName("com.avast.grpc.jsonbridge.test.TestService/Add"), "{ble}", Map.empty).unsafeRunSync()
-    status should matchPattern { case BridgeError.RequestJsonParseError(_) => }
+    status should matchPattern { case BridgeError.Json(_) => }
   }
 
 }
diff --git a/core/src/test/scala/com/avast/grpc/jsonbridge/ReflectionGrpcJsonBridgeTest2.scala b/core/src/test/scala/com/avast/grpc/jsonbridge/ReflectionGrpcJsonBridgeTest2.scala
@@ -44,7 +44,7 @@ class ReflectionGrpcJsonBridgeTest2 extends fixture.FlatSpec with Matchers {
   it must "return expected status code for malformed JSON" in { f =>
     val Left(status) =
       f.bridge.invoke(GrpcMethodName("com.avast.grpc.jsonbridge.test.TestService2/Add2"), "{ble}", Map.empty).unsafeRunSync()
-    status should matchPattern { case BridgeError.RequestJsonParseError(_) => }
+    status should matchPattern { case BridgeError.Json(_) => }
   }
 
 }
diff --git a/http4s/src/main/scala/com/avast/grpc/jsonbridge/http4s/Http4s.scala b/http4s/src/main/scala/com/avast/grpc/jsonbridge/http4s/Http4s.scala
@@ -66,16 +66,16 @@ object Http4s extends LazyLogging {
                             val message = s"Method '${methodNameString.fullName}' not found"
                             logger.debug(message)
                             NotFound(BridgeErrorResponse.fromMessage(message))
-                          case er: BridgeError.RequestJsonParseError =>
-                            val message = "Cannot parse JSON request body"
+                          case er: BridgeError.Json =>
+                            val message = "Wrong JSON"
                             logger.debug(message, er.t)
                             BadRequest(BridgeErrorResponse.fromException(message, er.t))
-                          case er: BridgeError.RequestErrorGrpc =>
-                            val message = "gRPC request error" + Option(er.s.getDescription).map(": " + _).getOrElse("")
+                          case er: BridgeError.Grpc =>
+                            val message = "gRPC error" + Option(er.s.getDescription).map(": " + _).getOrElse("")
                             logger.trace(message, er.s.getCause)
                             mapStatus(er.s, configuration)
-                          case er: BridgeError.RequestError =>
-                            val message = "Unknown request error"
+                          case er: BridgeError.Unknown =>
+                            val message = "Unknown error"
                             logger.warn(message, er.t)
                             InternalServerError(BridgeErrorResponse.fromException(message, er.t))
                         }