Use deriveFormat for ErrorDetailsFormat instead of hand written JsonFormat (#13770)

* Use deriveFormat for ErrorDetailsFormat instead of hand written JsonFormat changelog_begin - [HTTP-JSON] The field "@type" was renamed to "type" for encoding the ErrorDetails case changelog_end * Use moar types! :) * Use proper tags, not aliases
2024-09-20 01:07:18 +03:00 · 2022-05-03 15:28:15 +02:00 · 2022-05-03 15:28:15 +02:00 · 3decea2a95
commit 3decea2a95
parent 6bb59e6a0f
7 changed files with 72 additions and 89 deletions
--- a/ledger-service/http-json/BUILD.bazel
+++ b/ledger-service/http-json/BUILD.bazel
@ -254,7 +254,6 @@ daml_compile(
            "//ledger-service/jwt",
            "//ledger-service/lf-value-json",
            "//ledger-service/utils",
-            "//ledger/error",
            "//ledger/ledger-api-common",
            "//libs-scala/contextualized-logging",
            "//libs-scala/db-utils",
@ -314,7 +313,6 @@ alias(
            "//ledger-api/rs-grpc-bridge",
            "//ledger-api/testing-utils",
            "//ledger-service/fetch-contracts",
-            "//ledger/error",
            "//ledger/ledger-resources",
            "//ledger/metrics",
            "//ledger/sandbox-common",
--- a/ledger-service/http-json/src/itlib/scala/http/AbstractHttpServiceIntegrationTest.scala
+++ b/ledger-service/http-json/src/itlib/scala/http/AbstractHttpServiceIntegrationTest.scala
@ -7,7 +7,6 @@ import java.time.{Instant, LocalDate}
 import akka.http.scaladsl.Http
 import akka.http.scaladsl.model._
 import com.daml.api.util.TimestampConversion
-import com.daml.error.utils.ErrorDetails.{ErrorInfoDetail, RequestInfoDetail, ResourceInfoDetail}
 import com.daml.lf.data.Ref
 import com.daml.http.domain.ContractId
 import com.daml.http.domain.TemplateId.OptionalPkg
@ -715,16 +714,16 @@ abstract class AbstractHttpServiceIntegrationTestTokenIndependent
          )
          import org.scalatest.Inspectors._
          forExactly(1, ledgerApiError.details) {
-            case ErrorInfoDetail(errorCodeId, _) =>
+            case domain.ErrorInfoDetail(errorCodeId, _) =>
              errorCodeId shouldBe "CONTRACT_NOT_FOUND"
            case _ => fail()
          }
          forExactly(1, ledgerApiError.details) {
-            case RequestInfoDetail(_) => succeed
+            case domain.RequestInfoDetail(_) => succeed
            case _ => fail()
          }
          forExactly(1, ledgerApiError.details) {
-            case ResourceInfoDetail(name, typ) =>
+            case domain.ResourceInfoDetail(name, typ) =>
              name shouldBe "000000000000000000000000000000000000000000000000000000000000000000"
              typ shouldBe "CONTRACT_ID"
            case _ => fail()
--- a/ledger-service/http-json/src/main/scala/com/digitalasset/http/EndpointsCompanion.scala
+++ b/ledger-service/http-json/src/main/scala/com/digitalasset/http/EndpointsCompanion.scala
@ -277,7 +277,7 @@ object EndpointsCompanion {
          domain.LedgerApiError(
            code = grpcStatus.getNumber,
            message = description,
-            details = details,
+            details = details.map(domain.ErrorDetail.fromErrorUtils),
          )
        mkErrorResponse(
          grpcStatus.asAkkaHttpForJsonApi,
--- a/ledger-service/http-json/src/main/scala/com/digitalasset/http/domain.scala
+++ b/ledger-service/http-json/src/main/scala/com/digitalasset/http/domain.scala
@ -23,6 +23,8 @@ import spray.json.JsValue
 import scala.annotation.tailrec

 package object domain extends com.daml.fetchcontracts.domain.Aliases {
+  import scalaz.{@@, Tag}
+
  type InputContractRef[LfV] =
    (TemplateId.OptionalPkg, LfV) \/ (Option[TemplateId.OptionalPkg], ContractId)

@ -45,11 +47,13 @@ package object domain extends com.daml.fetchcontracts.domain.Aliases {
  val CommandId = lar.CommandId

  type LfType = iface.Type
+
+  type RetryInfoDetailDuration = scala.concurrent.duration.Duration @@ RetryInfoDetailDurationTag
+  val RetryInfoDetailDuration = Tag.of[RetryInfoDetailDurationTag]
 }

 package domain {

-  import com.daml.error.utils.ErrorDetails.ErrorDetail
  import com.daml.fetchcontracts.domain.`fc domain ErrorOps`

  trait JwtPayloadTag
@ -563,6 +567,29 @@ package domain {
      status: StatusCode = StatusCodes.OK,
  ) extends SyncResponse[R]

+  sealed trait RetryInfoDetailDurationTag
+
+  sealed trait ErrorDetail extends Product with Serializable
+  final case class ResourceInfoDetail(name: String, typ: String) extends ErrorDetail
+  final case class ErrorInfoDetail(errorCodeId: String, metadata: Map[String, String])
+      extends ErrorDetail
+  final case class RetryInfoDetail(duration: RetryInfoDetailDuration) extends ErrorDetail
+  final case class RequestInfoDetail(correlationId: String) extends ErrorDetail
+
+  object ErrorDetail {
+    import com.daml.error.utils.ErrorDetails
+    def fromErrorUtils(errorDetail: ErrorDetails.ErrorDetail): domain.ErrorDetail =
+      errorDetail match {
+        case ErrorDetails.ResourceInfoDetail(name, typ) => domain.ResourceInfoDetail(name, typ)
+        case ErrorDetails.ErrorInfoDetail(errorCodeId, metadata) =>
+          domain.ErrorInfoDetail(errorCodeId, metadata)
+        case ErrorDetails.RetryInfoDetail(duration) =>
+          domain.RetryInfoDetail(domain.RetryInfoDetailDuration(duration))
+        case ErrorDetails.RequestInfoDetail(correlationId) =>
+          domain.RequestInfoDetail(correlationId)
+      }
+  }
+
  final case class LedgerApiError(
      code: Int,
      message: String,
--- a/ledger-service/http-json/src/main/scala/com/digitalasset/http/json/JsonProtocol.scala
+++ b/ledger-service/http-json/src/main/scala/com/digitalasset/http/json/JsonProtocol.scala
@ -4,13 +4,6 @@
 package com.daml.http.json

 import akka.http.scaladsl.model.StatusCode
-import com.daml.error.utils.ErrorDetails.{
-  ErrorDetail,
-  ErrorInfoDetail,
-  RequestInfoDetail,
-  ResourceInfoDetail,
-  RetryInfoDetail,
-}
 import com.daml.http.domain
 import com.daml.http.domain.TemplateId
 import com.daml.ledger.api.refinements.{ApiTypes => lar}
@ -20,6 +13,7 @@ import scalaz.syntax.std.option._
 import scalaz.{-\/, NonEmptyList, OneAnd, \/-}
 import spray.json._
 import spray.json.derived.Discriminator
+import scalaz.syntax.tag._

 object JsonProtocol extends JsonProtocolLow {

@ -458,66 +452,33 @@ object JsonProtocol extends JsonProtocolLow {
  implicit def OkResponseFormat[R: JsonFormat]: RootJsonFormat[domain.OkResponse[R]] =
    jsonFormat3(domain.OkResponse[R])

-  implicit val ResourceInfoDetailFormat: RootJsonFormat[ResourceInfoDetail] = jsonFormat2(
-    ResourceInfoDetail
+  implicit val ResourceInfoDetailFormat: RootJsonFormat[domain.ResourceInfoDetail] = jsonFormat2(
+    domain.ResourceInfoDetail
  )
-  implicit val ErrorInfoDetailFormat: RootJsonFormat[ErrorInfoDetail] = jsonFormat2(
-    ErrorInfoDetail
+  implicit val ErrorInfoDetailFormat: RootJsonFormat[domain.ErrorInfoDetail] = jsonFormat2(
+    domain.ErrorInfoDetail
  )

-  implicit val RetryInfoDetailFormat: RootJsonFormat[RetryInfoDetail] =
-    new RootJsonFormat[RetryInfoDetail] {
-      override def write(obj: RetryInfoDetail): JsValue = JsObject(
-        "duration" -> JsNumber(obj.duration.toNanos)
-      )
-      override def read(json: JsValue): RetryInfoDetail = json match {
-        case JsObject(fields) =>
-          fields
-            .get("duration")
-            .collect { case JsNumber(nanos) =>
-              val duration = scala.concurrent.duration.Duration.fromNanos(nanos.toLongExact)
-              RetryInfoDetail(duration)
-            }
-            .getOrElse(deserializationError("Expected field duration of type number"))
-        case _ => deserializationError("Expected an object with field duration of type number")
-      }
-    }
+  implicit val durationFormat: JsonFormat[domain.RetryInfoDetailDuration] =
+    jsonFormat[domain.RetryInfoDetailDuration](
+      JsonReader.func2Reader(
+        (LongJsonFormat.read _)
+          .andThen(scala.concurrent.duration.Duration.fromNanos)
+          .andThen(it => domain.RetryInfoDetailDuration(it: scala.concurrent.duration.Duration))
+      ),
+      JsonWriter.func2Writer[domain.RetryInfoDetailDuration](duration =>
+        LongJsonFormat.write(duration.unwrap.toNanos)
+      ),
+    )

-  implicit val RequestInfoDetailFormat: RootJsonFormat[RequestInfoDetail] = jsonFormat1(
-    RequestInfoDetail
+  implicit val RetryInfoDetailFormat: RootJsonFormat[domain.RetryInfoDetail] =
+    jsonFormat1(domain.RetryInfoDetail)
+
+  implicit val RequestInfoDetailFormat: RootJsonFormat[domain.RequestInfoDetail] = jsonFormat1(
+    domain.RequestInfoDetail
  )

-  implicit val ErrorDetailsFormat: RootJsonFormat[ErrorDetail] =
-    new RootJsonFormat[ErrorDetail] {
-      override def write(obj: ErrorDetail): JsValue = {
-        val (jsValue, name) = obj match {
-          case a: ResourceInfoDetail => ResourceInfoDetailFormat.write(a) -> "ResourceInfoDetail"
-          case a: ErrorInfoDetail => ErrorInfoDetailFormat.write(a) -> "ErrorInfoDetail"
-          case a: RetryInfoDetail => RetryInfoDetailFormat.write(a) -> "RetryInfoDetail"
-          case a: RequestInfoDetail => RequestInfoDetailFormat.write(a) -> "RequestInfoDetail"
-        }
-        val fields = jsValue.asJsObject.fields ++ Map("@type" -> JsString(name))
-        JsObject(fields.toList: _*)
-      }
-      override def read(json: JsValue): ErrorDetail = {
-        val obj = json.asJsObject
-        obj.fields
-          .get("@type")
-          .map {
-            case JsString(value) => value
-            case value => deserializationError(s"Expected string but got $value")
-          }
-          .map {
-            case "ResourceInfoDetail" => ResourceInfoDetailFormat.read(obj)
-            case "ErrorInfoDetail" => ErrorInfoDetailFormat.read(obj)
-            case "RetryInfoDetail" => RetryInfoDetailFormat.read(obj)
-            case "RequestInfoDetail" =>
-              RequestInfoDetailFormat.read(obj)
-            case name =>
-              deserializationError(s"Unknown value for @type field: $name")
-          }
-      }.getOrElse(deserializationError("Field @type is required for decoding ErrorDetail"))
-    }
+  implicit val ErrorDetailsFormat: JsonFormat[domain.ErrorDetail] = deriveFormat[domain.ErrorDetail]

  implicit val LedgerApiErrorFormat: RootJsonFormat[domain.LedgerApiError] =
    jsonFormat3(domain.LedgerApiError)
--- a/ledger-service/http-json/src/test/scala/com/digitalasset/http/json/JsonProtocolTest.scala
+++ b/ledger-service/http-json/src/test/scala/com/digitalasset/http/json/JsonProtocolTest.scala
@ -4,13 +4,6 @@
 package com.daml.http.json

 import akka.http.scaladsl.model.StatusCodes
-import com.daml.error.utils.ErrorDetails.{
-  ErrorDetail,
-  ErrorInfoDetail,
-  RequestInfoDetail,
-  ResourceInfoDetail,
-  RetryInfoDetail,
-}
 import com.daml.http.Generators.{
  OptionalPackageIdGen,
  contractGen,
@ -186,24 +179,29 @@ class JsonProtocolTest

  "ErrorDetail" - {
    "Encoding and decoding ResourceInfoDetail should result in the same object" in {
-      val resourceInfoDetail: ErrorDetail = ResourceInfoDetail("test", "test")
-      resourceInfoDetail shouldBe resourceInfoDetail.toJson.convertTo[ErrorDetail]
+      val resourceInfoDetail: domain.ErrorDetail = domain.ResourceInfoDetail("test", "test")
+      resourceInfoDetail shouldBe resourceInfoDetail.toJson.convertTo[domain.ErrorDetail]
    }

    "Encoding and decoding RetryInfoDetail should result in the same object" in {
-      val retryInfoDetail: ErrorDetail = RetryInfoDetail(scala.concurrent.duration.Duration.Zero)
-      retryInfoDetail shouldBe retryInfoDetail.toJson.convertTo[ErrorDetail]
+      val retryInfoDetail: domain.ErrorDetail =
+        domain.RetryInfoDetail(
+          domain.RetryInfoDetailDuration(
+            scala.concurrent.duration.Duration.Zero: scala.concurrent.duration.Duration
+          )
+        )
+      retryInfoDetail shouldBe retryInfoDetail.toJson.convertTo[domain.ErrorDetail]
    }

    "Encoding and decoding RequestInfoDetail should result in the same object" in {
-      val requestInfoDetail: ErrorDetail = RequestInfoDetail("test")
-      requestInfoDetail shouldBe requestInfoDetail.toJson.convertTo[ErrorDetail]
+      val requestInfoDetail: domain.ErrorDetail = domain.RequestInfoDetail("test")
+      requestInfoDetail shouldBe requestInfoDetail.toJson.convertTo[domain.ErrorDetail]
    }

    "Encoding and decoding ErrorInfoDetail should result in the same object" in {
-      val errorInfoDetail: ErrorDetail =
-        ErrorInfoDetail("test", Map("test" -> "test1", "test2" -> "test3"))
-      errorInfoDetail shouldBe errorInfoDetail.toJson.convertTo[ErrorDetail]
+      val errorInfoDetail: domain.ErrorDetail =
+        domain.ErrorInfoDetail("test", Map("test" -> "test1", "test2" -> "test3"))
+      errorInfoDetail shouldBe errorInfoDetail.toJson.convertTo[domain.ErrorDetail]
    }
  }

--- a/security-evidence.md
+++ b/security-evidence.md
@ -5,7 +5,7 @@
 - Updating the package service succeeds with sufficient authorization: [AuthorizationTest.scala](ledger-service/http-json/src/it/scala/http/AuthorizationTest.scala#L89)
 - accept user tokens: [TestMiddleware.scala](triggers/service/auth/src/test/scala/com/daml/auth/middleware/oauth2/TestMiddleware.scala#L152)
 - badly-authorized create is rejected: [AuthorizationSpec.scala](daml-lf/engine/src/test/scala/com/digitalasset/daml/lf/engine/AuthorizationSpec.scala#L60)
- badly-authorized create is rejected: [AbstractHttpServiceIntegrationTest.scala](ledger-service/http-json/src/itlib/scala/http/AbstractHttpServiceIntegrationTest.scala#L1069)
+- badly-authorized create is rejected: [AbstractHttpServiceIntegrationTest.scala](ledger-service/http-json/src/itlib/scala/http/AbstractHttpServiceIntegrationTest.scala#L1068)
 - badly-authorized exercise is rejected: [AuthorizationSpec.scala](daml-lf/engine/src/test/scala/com/digitalasset/daml/lf/engine/AuthorizationSpec.scala#L158)
 - badly-authorized exercise/create (create is unauthorized) is rejected: [AuthPropagationSpec.scala](daml-lf/engine/src/test/scala/com/digitalasset/daml/lf/engine/AuthPropagationSpec.scala#L274)
 - badly-authorized exercise/create (exercise is unauthorized) is rejected: [AuthPropagationSpec.scala](daml-lf/engine/src/test/scala/com/digitalasset/daml/lf/engine/AuthPropagationSpec.scala#L242)
@ -18,7 +18,7 @@
 - create with no signatories is rejected: [AuthorizationSpec.scala](daml-lf/engine/src/test/scala/com/digitalasset/daml/lf/engine/AuthorizationSpec.scala#L50)
 - create with non-signatory maintainers is rejected: [AuthorizationSpec.scala](daml-lf/engine/src/test/scala/com/digitalasset/daml/lf/engine/AuthorizationSpec.scala#L72)
 - exercise with no controllers is rejected: [AuthorizationSpec.scala](daml-lf/engine/src/test/scala/com/digitalasset/daml/lf/engine/AuthorizationSpec.scala#L148)
- fetch fails when readAs not authed, even if prior fetch succeeded: [AbstractHttpServiceIntegrationTest.scala](ledger-service/http-json/src/itlib/scala/http/AbstractHttpServiceIntegrationTest.scala#L1127)
+- fetch fails when readAs not authed, even if prior fetch succeeded: [AbstractHttpServiceIntegrationTest.scala](ledger-service/http-json/src/itlib/scala/http/AbstractHttpServiceIntegrationTest.scala#L1126)
 - forbid a non-authorized party to check the status of a trigger: [TriggerServiceTest.scala](triggers/service/src/test/scala/com/digitalasset/daml/lf/engine/trigger/TriggerServiceTest.scala#L643)
 - forbid a non-authorized party to list triggers: [TriggerServiceTest.scala](triggers/service/src/test/scala/com/digitalasset/daml/lf/engine/trigger/TriggerServiceTest.scala#L633)
 - forbid a non-authorized party to start a trigger: [TriggerServiceTest.scala](triggers/service/src/test/scala/com/digitalasset/daml/lf/engine/trigger/TriggerServiceTest.scala#L622)
@ -26,7 +26,7 @@
 - forbid a non-authorized user to upload a DAR: [TriggerServiceTest.scala](triggers/service/src/test/scala/com/digitalasset/daml/lf/engine/trigger/TriggerServiceTest.scala#L675)
 - multiple websocket requests over the same WebSocket connection are NOT allowed: [AbstractWebsocketServiceIntegrationTest.scala](ledger-service/http-json/src/itlib/scala/http/AbstractWebsocketServiceIntegrationTest.scala#L118)
 - refresh a token after expiry on the server side: [TriggerServiceTest.scala](triggers/service/src/test/scala/com/digitalasset/daml/lf/engine/trigger/TriggerServiceTest.scala#L700)
- reject requests with missing auth header: [AbstractHttpServiceIntegrationTest.scala](ledger-service/http-json/src/itlib/scala/http/AbstractHttpServiceIntegrationTest.scala#L541)
+- reject requests with missing auth header: [AbstractHttpServiceIntegrationTest.scala](ledger-service/http-json/src/itlib/scala/http/AbstractHttpServiceIntegrationTest.scala#L540)
 - request a fresh token after expiry on user request: [TriggerServiceTest.scala](triggers/service/src/test/scala/com/digitalasset/daml/lf/engine/trigger/TriggerServiceTest.scala#L685)
 - return the token from a cookie: [TestMiddleware.scala](triggers/service/auth/src/test/scala/com/daml/auth/middleware/oauth2/TestMiddleware.scala#L96)
 - return unauthorized on an expired token: [TestMiddleware.scala](triggers/service/auth/src/test/scala/com/daml/auth/middleware/oauth2/TestMiddleware.scala#L139)
@ -202,7 +202,7 @@

 ## Performance:
 - Tail call optimization: Tail recursion does not blow the scala JVM stack.: [TailCallTest.scala](daml-lf/interpreter/src/test/scala/com/digitalasset/daml/lf/speedy/TailCallTest.scala#L16)
- archiving a large number of contracts should succeed: [AbstractHttpServiceIntegrationTest.scala](ledger-service/http-json/src/itlib/scala/http/AbstractHttpServiceIntegrationTest.scala#L1419)
+- archiving a large number of contracts should succeed: [AbstractHttpServiceIntegrationTest.scala](ledger-service/http-json/src/itlib/scala/http/AbstractHttpServiceIntegrationTest.scala#L1418)
 - creating and listing 20K users should be possible: [HttpServiceIntegrationTestUserManagement.scala](ledger-service/http-json/src/it/scala/http/HttpServiceIntegrationTestUserManagement.scala#L562)

 ## Input Validation: