From 0b172da49a27e0974beaee61aa88fb09994a9536 Mon Sep 17 00:00:00 2001 From: Leonid Shlyapnikov Date: Wed, 11 Dec 2019 13:38:44 -0500 Subject: [PATCH] Add ExceptionOps to extract exception details safely (#3822) * Add ExceptionOps to extract exception details safely `Throwable.getMessage` can return `null` which caused unexpected NPE in JSON API error handling * Fix formatting * Remove show instance * Addressing code review comments, thanks Stefano! --- .../digitalasset/daml/lf/data/MapKOps.scala | 3 ++- .../ledger/client/binding/ValueSpec.scala | 3 ++- .../scala/com/digitalasset/http/Config.scala | 7 ++++--- .../digitalasset/http/ContractsFetch.scala | 5 +++-- .../digitalasset/http/ContractsService.scala | 3 ++- .../com/digitalasset/http/Endpoints.scala | 13 +++++++------ .../com/digitalasset/http/HttpService.scala | 5 +++-- .../digitalasset/http/json/HttpCodec.scala | 7 ++++--- .../digitalasset/http/json/JsonError.scala | 3 ++- .../digitalasset/http/json/SprayJson.scala | 7 ++++--- .../util/ApiValueToLfValueConverter.scala | 3 ++- .../digitalasset/http/util/ExceptionOps.scala | 19 +++++++++++++++++++ 12 files changed, 54 insertions(+), 24 deletions(-) create mode 100644 ledger-service/http-json/src/main/scala/com/digitalasset/http/util/ExceptionOps.scala diff --git a/daml-lf/data/src/main/scala/com/digitalasset/daml/lf/data/MapKOps.scala b/daml-lf/data/src/main/scala/com/digitalasset/daml/lf/data/MapKOps.scala index 42db8c8fb0..c545f0de66 100644 --- a/daml-lf/data/src/main/scala/com/digitalasset/daml/lf/data/MapKOps.scala +++ b/daml-lf/data/src/main/scala/com/digitalasset/daml/lf/data/MapKOps.scala @@ -8,7 +8,8 @@ import scala.collection.GenTraversableOnce import scala.collection.immutable.{Map, MapLike} /** Halfway between the *-kinded MapLike and *->*->*-kinded MapOps. */ -trait MapKOps[K, +V, +This[+TV] <: Map[K, TV] with MapKOps[K, TV, This]] extends MapLike[K, V, This[V]] { this: This[V] => +trait MapKOps[K, +V, +This[+TV] <: Map[K, TV] with MapKOps[K, TV, This]] + extends MapLike[K, V, This[V]] { this: This[V] => override def updated[V1 >: V](key: K, value: V1): This[V1] = this + ((key, value)) override def +[V1 >: V](kv: (K, V1)): This[V1] override def +[V1 >: V](elem1: (K, V1), elem2: (K, V1), elems: (K, V1)*): This[V1] = diff --git a/language-support/scala/codegen-testing/src/test/scala/com/digitalasset/ledger/client/binding/ValueSpec.scala b/language-support/scala/codegen-testing/src/test/scala/com/digitalasset/ledger/client/binding/ValueSpec.scala index a1bac0bc00..d4a5d45109 100644 --- a/language-support/scala/codegen-testing/src/test/scala/com/digitalasset/ledger/client/binding/ValueSpec.scala +++ b/language-support/scala/codegen-testing/src/test/scala/com/digitalasset/ledger/client/binding/ValueSpec.scala @@ -90,7 +90,8 @@ object ValueSpec { override def valueTextMap[A](implicit vc: ValueCheck[A]) = { import vc._ - implicit val arbTM: Arbitrary[P.TextMap[A]] = Arbitrary(GenEncoding.primitive.valueTextMap(TA.arbitrary)) + implicit val arbTM: Arbitrary[P.TextMap[A]] = Arbitrary( + GenEncoding.primitive.valueTextMap(TA.arbitrary)) ValueCheck[P.TextMap[A]](s"Map[$tName]") } diff --git a/ledger-service/http-json/src/main/scala/com/digitalasset/http/Config.scala b/ledger-service/http-json/src/main/scala/com/digitalasset/http/Config.scala index 3727228309..e28dfac4b7 100644 --- a/ledger-service/http-json/src/main/scala/com/digitalasset/http/Config.scala +++ b/ledger-service/http-json/src/main/scala/com/digitalasset/http/Config.scala @@ -6,10 +6,11 @@ package com.digitalasset.http import java.io.File import java.nio.file.Path +import com.digitalasset.http.util.ExceptionOps._ import com.digitalasset.ledger.api.refinements.ApiTypes.ApplicationId -import scalaz.{Show, \/} import scalaz.std.option._ import scalaz.syntax.traverse._ +import scalaz.{Show, \/} import scala.concurrent.duration.FiniteDuration import scala.util.Try @@ -53,14 +54,14 @@ private[http] abstract class ConfigCompanion[A](name: String) { protected def parseBoolean(k: String)(v: String): String \/ Boolean = \/.fromTryCatchNonFatal(v.toBoolean).leftMap(e => - s"$k=$v must be a boolean value: ${e.getMessage}") + s"$k=$v must be a boolean value: ${e.description}") protected def requiredDirectoryField(m: Map[String, String])(k: String): Either[String, File] = requiredField(m)(k).flatMap(directory) protected def directory(s: String): Either[String, File] = Try(new File(s).getAbsoluteFile).toEither.left - .map(e => e.getMessage) + .map(e => e.description) .flatMap { d => if (d.isDirectory) Right(d) else Left(s"Directory does not exist: ${d.getAbsolutePath}") diff --git a/ledger-service/http-json/src/main/scala/com/digitalasset/http/ContractsFetch.scala b/ledger-service/http-json/src/main/scala/com/digitalasset/http/ContractsFetch.scala index daf935eba8..93840b7fa3 100644 --- a/ledger-service/http-json/src/main/scala/com/digitalasset/http/ContractsFetch.scala +++ b/ledger-service/http-json/src/main/scala/com/digitalasset/http/ContractsFetch.scala @@ -26,6 +26,7 @@ import com.digitalasset.http.json.JsonProtocol.LfValueDatabaseCodec.{ apiValueToJsValue => lfValueToDbJsValue } import com.digitalasset.http.util.IdentifierConverters.apiIdentifier +import com.digitalasset.http.util.ExceptionOps._ import com.digitalasset.jwt.domain.Jwt import com.digitalasset.ledger.api.v1.transaction.Transaction import com.digitalasset.ledger.api.{v1 => lav1} @@ -81,10 +82,10 @@ private class ContractsFetch( logger.debug(s"contractsIo, maxAttempts: $maxAttempts") contractsIo_(jwt, party, templateId).exceptSql { case e if maxAttempts > 0 && retrySqlStates(e.getSQLState) => - logger.debug(s"contractsIo, exception: ${e.getMessage}, state: ${e.getSQLState}") + logger.debug(s"contractsIo, exception: ${e.description}, state: ${e.getSQLState}") connection.rollback flatMap (_ => loop(maxAttempts - 1)) case e @ _ => - logger.error(s"contractsIo3 exception: ${e.getMessage}, state: ${e.getSQLState}") + logger.error(s"contractsIo3 exception: ${e.description}, state: ${e.getSQLState}") connection.raiseError(e) } } diff --git a/ledger-service/http-json/src/main/scala/com/digitalasset/http/ContractsService.scala b/ledger-service/http-json/src/main/scala/com/digitalasset/http/ContractsService.scala index 7a2cb41bc7..6bc61ccfc9 100644 --- a/ledger-service/http-json/src/main/scala/com/digitalasset/http/ContractsService.scala +++ b/ledger-service/http-json/src/main/scala/com/digitalasset/http/ContractsService.scala @@ -14,6 +14,7 @@ import com.digitalasset.http.json.JsonProtocol.LfValueCodec import com.digitalasset.http.query.ValuePredicate import com.digitalasset.http.query.ValuePredicate.LfV import com.digitalasset.http.util.ApiValueToLfValueConverter +import com.digitalasset.http.util.ExceptionOps._ import com.digitalasset.http.util.FutureUtil.toFuture import com.digitalasset.http.util.IdentifierConverters.apiIdentifier import com.digitalasset.jwt.domain.Jwt @@ -300,7 +301,7 @@ class ContractsService( private def lfValueToJsValue(a: LfValue): Error \/ JsValue = \/.fromTryCatchNonFatal(LfValueCodec.apiValueToJsValue(a)).leftMap(e => - Error('lfValueToJsValue, e.getMessage)) + Error('lfValueToJsValue, e.description)) } object ContractsService { diff --git a/ledger-service/http-json/src/main/scala/com/digitalasset/http/Endpoints.scala b/ledger-service/http-json/src/main/scala/com/digitalasset/http/Endpoints.scala index 8b7ca33f08..67bb87515a 100644 --- a/ledger-service/http-json/src/main/scala/com/digitalasset/http/Endpoints.scala +++ b/ledger-service/http-json/src/main/scala/com/digitalasset/http/Endpoints.scala @@ -14,6 +14,7 @@ import com.digitalasset.daml.lf import com.digitalasset.http.Statement.discard import com.digitalasset.http.domain.JwtPayload import com.digitalasset.http.json.{DomainJsonDecoder, DomainJsonEncoder, ResponseFormats, SprayJson} +import com.digitalasset.http.util.ExceptionOps._ import com.digitalasset.http.util.FutureUtil.{either, eitherT} import com.digitalasset.http.util.{ApiValueToLfValueConverter, FutureUtil} import com.digitalasset.jwt.domain.{DecodedJwt, Jwt} @@ -105,14 +106,14 @@ class Endpoints( fa.map(a => a.leftMap(e => ServerError(e.shows))).recover { case NonFatal(e) => logger.error("Future failed", e) - -\/(ServerError(e.getMessage)) + -\/(ServerError(e.description)) } private def handleFutureFailure[A](fa: Future[A]): Future[ServerError \/ A] = fa.map(a => \/-(a)).recover { case NonFatal(e) => logger.error("Future failed", e) - -\/(ServerError(e.getMessage)) + -\/(ServerError(e.description)) } private def handleSourceFailure[E: Show, A]: Flow[E \/ A, ServerError \/ A, NotUsed] = @@ -121,7 +122,7 @@ class Endpoints( .recover { case NonFatal(e) => logger.error("Source failed", e) - -\/(ServerError(e.getMessage)) + -\/(ServerError(e.description)) } private def encodeList(as: Seq[JsValue]): ServerError \/ JsValue = @@ -200,7 +201,7 @@ class Endpoints( private def lfValueToJsValue(a: LfValue): Error \/ JsValue = \/.fromTryCatchNonFatal(LfValueCodec.apiValueToJsValue(a)).leftMap(e => - ServerError(e.getMessage)) + ServerError(e.description)) private def collectActiveContracts( predicates: Map[domain.TemplateId.RequiredPkg, LfValue => Boolean]): PartialFunction[ @@ -230,7 +231,7 @@ class Endpoints( case -\/(e) => httpResponseError(e) } .recover { - case NonFatal(e) => httpResponseError(ServerError(e.getMessage)) + case NonFatal(e) => httpResponseError(ServerError(e.description)) } } @@ -242,7 +243,7 @@ class Endpoints( case \/-(source) => httpResponseFromSource(source) } .recover { - case NonFatal(e) => httpResponseError(ServerError(e.getMessage)) + case NonFatal(e) => httpResponseError(ServerError(e.description)) } private def httpResponseOk(data: JsValue): HttpResponse = diff --git a/ledger-service/http-json/src/main/scala/com/digitalasset/http/HttpService.scala b/ledger-service/http-json/src/main/scala/com/digitalasset/http/HttpService.scala index 61f85498ae..d77198dd56 100644 --- a/ledger-service/http-json/src/main/scala/com/digitalasset/http/HttpService.scala +++ b/ledger-service/http-json/src/main/scala/com/digitalasset/http/HttpService.scala @@ -21,6 +21,7 @@ import com.digitalasset.http.json.{ JsValueToApiValueConverter } import com.digitalasset.http.util.ApiValueToLfValueConverter +import com.digitalasset.http.util.ExceptionOps._ import com.digitalasset.http.util.FutureUtil._ import com.digitalasset.http.util.IdentifierConverters.apiLedgerId import com.digitalasset.jwt.JwtDecoder @@ -33,8 +34,8 @@ import com.digitalasset.ledger.client.configuration.{ LedgerIdRequirement } import com.digitalasset.ledger.client.services.pkg.PackageClient -import com.digitalasset.ledger.service.LedgerReader.PackageStore import com.digitalasset.ledger.service.LedgerReader +import com.digitalasset.ledger.service.LedgerReader.PackageStore import com.typesafe.scalalogging.StrictLogging import io.grpc.netty.NettyChannelBuilder import scalaz.Scalaz._ @@ -231,6 +232,6 @@ object HttpService extends StrictLogging { .map(_.right) .recover { case NonFatal(e) => - \/.left(Error(s"Cannot connect to the ledger server, error: ${e.getMessage}")) + \/.left(Error(s"Cannot connect to the ledger server, error: ${e.description}")) } } diff --git a/ledger-service/http-json/src/main/scala/com/digitalasset/http/json/HttpCodec.scala b/ledger-service/http-json/src/main/scala/com/digitalasset/http/json/HttpCodec.scala index dbf2cc0040..9dc65d28ea 100644 --- a/ledger-service/http-json/src/main/scala/com/digitalasset/http/json/HttpCodec.scala +++ b/ledger-service/http-json/src/main/scala/com/digitalasset/http/json/HttpCodec.scala @@ -6,11 +6,12 @@ package com.digitalasset.http.json import akka.http.scaladsl.marshalling.{Marshaller, ToEntityMarshaller} import akka.http.scaladsl.model.MediaTypes.`application/json` import akka.http.scaladsl.model.StatusCodes -import akka.http.scaladsl.server.ExceptionHandler import akka.http.scaladsl.server.Directives._ +import akka.http.scaladsl.server.ExceptionHandler import akka.http.scaladsl.unmarshalling.{FromEntityUnmarshaller, Unmarshaller} -import scalaz.{@@, Tag} +import com.digitalasset.http.util.ExceptionOps._ import scalaz.std.stream.unfold +import scalaz.{@@, Tag} import spray.json._ import scala.concurrent.{ExecutionContext, Future} @@ -27,7 +28,7 @@ object HttpCodec { StatusCodes.BadRequest, ResponseFormats.errorsJsObject( StatusCodes.BadRequest, - s"JSON parser error: ${e.msg}" +: unfoldCauses(e.cause).map(_.getMessage): _*))) + s"JSON parser error: ${e.msg}" +: unfoldCauses(e.cause).map(_.description): _*))) } private[this] def unfoldCauses(t: Throwable): Seq[Throwable] = diff --git a/ledger-service/http-json/src/main/scala/com/digitalasset/http/json/JsonError.scala b/ledger-service/http-json/src/main/scala/com/digitalasset/http/json/JsonError.scala index 928c3e026d..d08022e058 100644 --- a/ledger-service/http-json/src/main/scala/com/digitalasset/http/json/JsonError.scala +++ b/ledger-service/http-json/src/main/scala/com/digitalasset/http/json/JsonError.scala @@ -3,6 +3,7 @@ package com.digitalasset.http.json +import com.digitalasset.http.util.ExceptionOps._ import scalaz.Show import scalaz.syntax.show._ @@ -13,7 +14,7 @@ object JsonError { def toJsonError[E: Show](e: E) = JsonError(e.shows) - def toJsonError(e: Throwable) = JsonError(e.getMessage) + def toJsonError(e: Throwable) = JsonError(e.description) implicit val ShowInstance: Show[JsonError] = Show shows { f => s"JsonError: ${f.message}" diff --git a/ledger-service/http-json/src/main/scala/com/digitalasset/http/json/SprayJson.scala b/ledger-service/http-json/src/main/scala/com/digitalasset/http/json/SprayJson.scala index 893767bfda..0a829112b4 100644 --- a/ledger-service/http-json/src/main/scala/com/digitalasset/http/json/SprayJson.scala +++ b/ledger-service/http-json/src/main/scala/com/digitalasset/http/json/SprayJson.scala @@ -3,6 +3,7 @@ package com.digitalasset.http.json +import com.digitalasset.http.util.ExceptionOps._ import scalaz.{-\/, Show, \/, \/-} import spray.json.{JsValue, JsonReader, _} @@ -32,7 +33,7 @@ object SprayJson { } def parse(str: String): JsonReaderError \/ JsValue = - \/.fromTryCatchNonFatal(JsonParser(str)).leftMap(e => JsonReaderError(str, e.getMessage)) + \/.fromTryCatchNonFatal(JsonParser(str)).leftMap(e => JsonReaderError(str, e.description)) def decode[A: JsonReader](str: String): JsonReaderError \/ A = for { @@ -41,11 +42,11 @@ object SprayJson { } yield a def decode[A: JsonReader](a: JsValue): JsonReaderError \/ A = - \/.fromTryCatchNonFatal(a.convertTo[A]).leftMap(e => JsonReaderError(a.toString, e.getMessage)) + \/.fromTryCatchNonFatal(a.convertTo[A]).leftMap(e => JsonReaderError(a.toString, e.description)) def encode[A: JsonWriter](a: A): JsonWriterError \/ JsValue = { import spray.json._ - \/.fromTryCatchNonFatal(a.toJson).leftMap(e => JsonWriterError(a, e.getMessage)) + \/.fromTryCatchNonFatal(a.toJson).leftMap(e => JsonWriterError(a, e.description)) } def mustBeJsObject(a: JsValue): JsonError \/ JsObject = a match { diff --git a/ledger-service/http-json/src/main/scala/com/digitalasset/http/util/ApiValueToLfValueConverter.scala b/ledger-service/http-json/src/main/scala/com/digitalasset/http/util/ApiValueToLfValueConverter.scala index d4a539fa42..fb8198feda 100644 --- a/ledger-service/http-json/src/main/scala/com/digitalasset/http/util/ApiValueToLfValueConverter.scala +++ b/ledger-service/http-json/src/main/scala/com/digitalasset/http/util/ApiValueToLfValueConverter.scala @@ -14,7 +14,8 @@ object ApiValueToLfValueConverter { object Error { implicit val ErrorShow: Show[Error] = Show shows { e => - s"ApiValueToLfValueConverter.Error: ${e.cause.getMessage}" + import ExceptionOps._ + s"ApiValueToLfValueConverter.Error: ${e.cause.description}" } } diff --git a/ledger-service/http-json/src/main/scala/com/digitalasset/http/util/ExceptionOps.scala b/ledger-service/http-json/src/main/scala/com/digitalasset/http/util/ExceptionOps.scala new file mode 100644 index 0000000000..ea8fb79b7e --- /dev/null +++ b/ledger-service/http-json/src/main/scala/com/digitalasset/http/util/ExceptionOps.scala @@ -0,0 +1,19 @@ +// Copyright (c) 2019 The DAML Authors. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package com.digitalasset.http.util + +object ExceptionOps { + + implicit class ExceptionOps(val underlying: Throwable) extends AnyVal { + def description: String = getDescription(underlying) + } + + def getDescription(e: Throwable): String = { + val name: String = e.getClass.getName + Option(e.getMessage).filter(_.nonEmpty) match { + case Some(m) => s"$name: $m" + case None => name + } + } +}