[Self-service error codes] Adapt ApiTransactionService [DPP-613] (#11094

) * [DPP-417][DPP-613] Adopt self-service error codes ApiTransactionService CHANGELOG_BEGIN CHANGELOG_END * TV review * TV review * Uses ErrorFactories * Error loggers use enriched logging contexts * Deleted ApiTransactionServiceSpec * Add TODO wrt to CommandValidation.invalidArgument * Adapt transactionNotFound error assertions in conformance tests * ErrorFactories.invalidArgumentWasNotFound for handling invalid event ids * Revert improvement to TransactionNotFound cause message since Canton still outputs the old message and assertions fail. Co-authored-by: Tudor Voicu <[email protected]>
digital-asset · Oct 20, 2021 · 88c607b · 88c607b
1 parent 07ad3e0
commit 88c607b
Show file tree

Hide file tree

Showing 6 changed files with 153 additions and 55 deletions.
diff --git a/ledger/error/src/main/scala/com/daml/error/ErrorCodesVersionSwitcher.scala b/ledger/error/src/main/scala/com/daml/error/ErrorCodesVersionSwitcher.scala
@@ -6,10 +6,6 @@ import io.grpc.StatusRuntimeException
 
 import scala.concurrent.Future
 
-/** A mechanism to switch between the legacy error codes (v1) and the new self-service error codes (v2).
-  * This class is intended to facilitate transition to self-service error codes.
-  * Once the previous error codes are removed, this class should be dropped as well.
-  */
 final class ErrorCodesVersionSwitcher(enableSelfServiceErrorCodes: Boolean)
     extends ValueSwitch[StatusRuntimeException](enableSelfServiceErrorCodes) {
 

diff --git a/ledger/error/src/main/scala/com/daml/error/ErrorResource.scala b/ledger/error/src/main/scala/com/daml/error/ErrorResource.scala
@@ -26,6 +26,9 @@ object ErrorResource {
   object ContractKey extends ErrorResource {
     def asString: String = "CONTRACT_KEY"
   }
+  object TransactionId extends ErrorResource {
+    def asString: String = "TRANSACTION_ID"
+  }
   object DalfPackage extends ErrorResource {
     def asString: String = "PACKAGE"
   }

diff --git a/ledger/error/src/main/scala/com/daml/error/definitions/LedgerApiErrors.scala b/ledger/error/src/main/scala/com/daml/error/definitions/LedgerApiErrors.scala
@@ -102,6 +102,26 @@ object LedgerApiErrors extends LedgerApiErrorGroup {
             cause = message
           )
     }
+
+    @Explanation(
+      "The transaction does not exist or the requesting set of parties are not authorized to fetch it."
+    )
+    @Resolution(
+      "Check the transaction id and verify that the requested transaction is visible to the requesting parties."
+    )
+    object TransactionNotFound
+        extends ErrorCode(
+          id = "TRANSACTION_NOT_FOUND",
+          ErrorCategory.InvalidGivenCurrentSystemStateResourceMissing,
+        ) {
+
+      case class Reject(transactionId: String)(implicit loggingContext: ContextualizedErrorLogger)
+          extends LoggingTransactionErrorImpl(cause = "Transaction not found, or not visible.") {
+        override def resources: Seq[(ErrorResource, String)] = Seq(
+          (ErrorResource.TransactionId, transactionId)
+        )
+      }
+    }
   }
 
   // the authorization checks are here only for documentation purpose.
@@ -501,7 +521,6 @@ object LedgerApiErrors extends LedgerApiErrorGroup {
               cause = "The ledger configuration is not available."
             )
       }
-
     }
 
     @Explanation("""This error occurs if the Daml transaction fails due to an authorization error.

diff --git a/...ommon/src/main/scala/com/digitalasset/platform/server/api/validation/ErrorFactories.scala b/...ommon/src/main/scala/com/digitalasset/platform/server/api/validation/ErrorFactories.scala
@@ -9,6 +9,7 @@ import com.daml.error.{ContextualizedErrorLogger, ErrorCodesVersionSwitcher}
 import com.daml.ledger.api.domain.LedgerId
 import com.daml.ledger.grpc.GrpcStatuses
 import com.daml.logging.{ContextualizedLogger, LoggingContext}
+import com.daml.lf.data.Ref.TransactionId
 import com.daml.platform.server.api.validation.ErrorFactories.{
   addDefiniteAnswerDetails,
   definiteAnswers,
@@ -22,6 +23,21 @@ import io.grpc.{Metadata, StatusRuntimeException}
 import scalaz.syntax.tag._
 
 class ErrorFactories private (errorCodesVersionSwitcher: ErrorCodesVersionSwitcher) {
+  def transactionNotFound(transactionId: TransactionId)(implicit
+      contextualizedErrorLogger: ContextualizedErrorLogger
+  ): StatusRuntimeException =
+    errorCodesVersionSwitcher.choose(
+      v1 = grpcError(
+        Status
+          .newBuilder()
+          .setCode(Code.NOT_FOUND.value())
+          .setMessage("Transaction not found, or not visible.")
+          .build()
+      ),
+      v2 = LedgerApiErrors.ReadErrors.TransactionNotFound
+        .Reject(transactionId)
+        .asGrpcError,
+    )
 
   def malformedPackageId[Request](request: Request, message: String)(implicit
       contextualizedErrorLogger: ContextualizedErrorLogger,
@@ -136,6 +152,28 @@ class ErrorFactories private (errorCodesVersionSwitcher: ErrorCodesVersionSwitch
         addDefiniteAnswerDetails(definiteAnswer, statusBuilder)
         grpcError(statusBuilder.build())
       },
+      // TODO error codes: This error group is confusing for this generic error as it can be dispatched
+      //                   from call-sites that do not involve command validation (e.g. ApiTransactionService).
+      v2 = LedgerApiErrors.CommandValidation.InvalidArgument
+        .Reject(message)
+        .asGrpcError,
+    )
+
+  // This error builder covers cases where existing logic handling invalid arguments returned NOT_FOUND.
+  def invalidArgumentWasNotFound(definiteAnswer: Option[Boolean])(message: String)(implicit
+      contextualizedErrorLogger: ContextualizedErrorLogger
+  ): StatusRuntimeException =
+    errorCodesVersionSwitcher.choose(
+      v1 = {
+        val statusBuilder = Status
+          .newBuilder()
+          .setCode(Code.NOT_FOUND.value())
+          .setMessage(message)
+        addDefiniteAnswerDetails(definiteAnswer, statusBuilder)
+        grpcError(statusBuilder.build())
+      },
+      // TODO error codes: This error group is confusing for this generic error as it can be dispatched
+      //                   from call-sites that do not involve command validation (e.g. ApiTransactionService).
       v2 = LedgerApiErrors.CommandValidation.InvalidArgument
         .Reject(message)
         .asGrpcError,

diff --git a/...test/suite/scala/com/digitalasset/platform/server/api/validation/ErrorFactoriesSpec.scala b/...test/suite/scala/com/digitalasset/platform/server/api/validation/ErrorFactoriesSpec.scala
@@ -3,12 +3,16 @@
 
 package com.daml
 
-import error.{ContextualizedErrorLogger, DamlContextualizedErrorLogger, ErrorCodesVersionSwitcher}
-import ledger.api.domain.LedgerId
-import logging.{ContextualizedLogger, LoggingContext}
-import platform.server.api.validation.ErrorFactories
-import platform.server.api.validation.ErrorFactories._
-
+import com.daml.error.{
+  ContextualizedErrorLogger,
+  DamlContextualizedErrorLogger,
+  ErrorCodesVersionSwitcher,
+}
+import com.daml.ledger.api.domain.LedgerId
+import com.daml.lf.data.Ref
+import com.daml.logging.{ContextualizedLogger, LoggingContext}
+import com.daml.platform.server.api.validation.ErrorFactories
+import com.daml.platform.server.api.validation.ErrorFactories._
 import com.google.protobuf
 import com.google.rpc._
 import io.grpc.Status.Code
@@ -32,7 +36,6 @@ class ErrorFactoriesSpec extends AnyWordSpec with Matchers with TableDrivenPrope
     ErrorDetails.RequestInfoDetail("trace-id")
 
   "ErrorFactories" should {
-
     "return malformedPackageId" in {
       assertVersionedError(
         _.malformedPackageId(request = "request123", message = "message123")(
@@ -68,6 +71,22 @@ class ErrorFactoriesSpec extends AnyWordSpec with Matchers with TableDrivenPrope
       )
     }
 
+    "return a transactionNotFound error" in {
+      assertVersionedError(_.transactionNotFound(Ref.TransactionId.assertFromString("tId")))(
+        v1_code = Code.NOT_FOUND,
+        v1_message = "Transaction not found, or not visible.",
+        v1_details = Seq.empty,
+        v2_code = Code.NOT_FOUND,
+        v2_message =
+          s"TRANSACTION_NOT_FOUND(11,$correlationId): Transaction not found, or not visible.",
+        v2_details = Seq[ErrorDetails.ErrorDetail](
+          ErrorDetails.ErrorInfoDetail("TRANSACTION_NOT_FOUND"),
+          DefaultTraceIdRequestInfo,
+          ErrorDetails.ResourceInfoDetail("TRANSACTION_ID", "tId"),
+        ),
+      )
+    }
+
     "return the DuplicateCommandException" in {
       assertVersionedError(_.duplicateCommandException)(
         v1_code = Code.ALREADY_EXISTS,
@@ -339,6 +358,29 @@ class ErrorFactoriesSpec extends AnyWordSpec with Matchers with TableDrivenPrope
       }
     }
 
+    "return an invalidArgument (with legacy error code as NOT_FOUND) error" in {
+      val testCases = Table(
+        ("definite answer", "expected details"),
+        (None, Seq.empty),
+        (Some(false), Seq(definiteAnswers(false))),
+      )
+
+      forEvery(testCases) { (definiteAnswer, expectedDetails) =>
+        assertVersionedError(_.invalidArgumentWasNotFound(definiteAnswer)("my message"))(
+          v1_code = Code.NOT_FOUND,
+          v1_message = "my message",
+          v1_details = expectedDetails,
+          v2_code = Code.INVALID_ARGUMENT,
+          v2_message =
+            s"INVALID_ARGUMENT(8,$correlationId): The submitted command has invalid arguments: my message",
+          v2_details = Seq[ErrorDetails.ErrorDetail](
+            ErrorDetails.ErrorInfoDetail("INVALID_ARGUMENT"),
+            DefaultTraceIdRequestInfo,
+          ),
+        )
+      }
+    }
+
     "should create an ApiException without the stack trace" in {
       val status = Status.newBuilder().setCode(Code.INTERNAL.value()).build()
       val exception = grpcError(status)

diff --git a/...on-api/src/main/scala/platform/apiserver/services/transaction/ApiTransactionService.scala b/...on-api/src/main/scala/platform/apiserver/services/transaction/ApiTransactionService.scala
@@ -6,8 +6,11 @@ package com.daml.platform.apiserver.services.transaction
 import akka.NotUsed
 import akka.stream.Materializer
 import akka.stream.scaladsl.Source
-import com.daml.error.{DamlContextualizedErrorLogger, ErrorCodesVersionSwitcher}
-import com.daml.error.definitions.LedgerApiErrors
+import com.daml.error.{
+  ContextualizedErrorLogger,
+  DamlContextualizedErrorLogger,
+  ErrorCodesVersionSwitcher,
+}
 import com.daml.grpc.adapter.ExecutionSequencerFactory
 import com.daml.ledger.api.domain.{
   Filters,
@@ -31,10 +34,10 @@ import com.daml.logging.LoggingContext.withEnrichedLoggingContext
 import com.daml.logging.entries.LoggingEntries
 import com.daml.logging.{ContextualizedLogger, LoggingContext}
 import com.daml.metrics.Metrics
-import com.daml.platform.apiserver.services.transaction.ApiTransactionService._
 import com.daml.platform.apiserver.services.{StreamMetrics, logging}
 import com.daml.platform.server.api.services.domain.TransactionService
 import com.daml.platform.server.api.services.grpc.GrpcTransactionService
+import com.daml.platform.server.api.validation.ErrorFactories
 import io.grpc._
 import scalaz.syntax.tag._
 
@@ -57,14 +60,6 @@ private[apiserver] object ApiTransactionService {
       ledgerId,
       PartyNameChecker.AllowAllParties,
     )
-
-  @throws[StatusRuntimeException]
-  private def getOrElseThrowNotFound[A](a: Option[A]): A =
-    a.getOrElse(
-      throw Status.NOT_FOUND
-        .withDescription("Transaction not found, or not visible.")
-        .asRuntimeException()
-    )
 }
 
 private[apiserver] final class ApiTransactionService private (
@@ -75,6 +70,7 @@ private[apiserver] final class ApiTransactionService private (
     extends TransactionService {
 
   private val logger: ContextualizedLogger = ContextualizedLogger.get(this.getClass)
+  private val errorFactories = ErrorFactories(errorCodesVersionSwitcher)
 
   override def getLedgerEnd(ledgerId: String): Future[LedgerOffset.Absolute] =
     transactionsService.currentLedgerEnd().andThen(logger.logErrorsOnCall[LedgerOffset.Absolute])
@@ -127,107 +123,111 @@ private[apiserver] final class ApiTransactionService private (
   override def getTransactionByEventId(
       request: GetTransactionByEventIdRequest
   ): Future[GetTransactionResponse] = {
-    withEnrichedLoggingContext(
+    // There is no problem in leaking the loggingContext in here, but the construction looks suspicious
+    // TODO error codes: Replace with non-closure-based enriched loggingContext builder here and in other constructions as well
+    implicit val errorLogger: ContextualizedErrorLogger = withEnrichedLoggingContext(
       logging.ledgerId(request.ledgerId),
       logging.eventId(request.eventId),
       logging.parties(request.requestingParties),
     ) { implicit loggingContext =>
       logger.info("Received request for transaction by event ID.")
+      new DamlContextualizedErrorLogger(logger, loggingContext, None)
     }
     logger.trace(s"Transaction by event ID request: $request")
+
     LfEventId
       .fromString(request.eventId.unwrap)
       .map { case LfEventId(transactionId, _) =>
         lookUpTreeByTransactionId(TransactionId(transactionId), request.requestingParties)
       }
       .getOrElse {
-        val msg = s"invalid eventId: ${request.eventId}"
-        Future.failed(
-          errorCodesVersionSwitcher.choose(
-            v1 = Status.NOT_FOUND
-              .withDescription(msg)
-              .asRuntimeException(),
-            v2 = LedgerApiErrors.CommandValidation.InvalidArgument
-              .Reject(msg)(new DamlContextualizedErrorLogger(logger, loggingContext, None))
-              .asGrpcError,
-          )
-        )
+        Future.failed {
+          errorFactories.invalidArgumentWasNotFound(None)(s"invalid eventId: ${request.eventId}")
+        }
       }
       .andThen(logger.logErrorsOnCall[GetTransactionResponse])
   }
 
   override def getTransactionById(
       request: GetTransactionByIdRequest
   ): Future[GetTransactionResponse] = {
-    withEnrichedLoggingContext(
+    val errorLogger: DamlContextualizedErrorLogger = withEnrichedLoggingContext(
       logging.ledgerId(request.ledgerId),
       logging.transactionId(request.transactionId),
       logging.parties(request.requestingParties),
     ) { implicit loggingContext =>
       logger.info("Received request for transaction by ID.")
+      new DamlContextualizedErrorLogger(logger, loggingContext, None)
     }
     logger.trace(s"Transaction by ID request: $request")
-    lookUpTreeByTransactionId(request.transactionId, request.requestingParties)
+
+    lookUpTreeByTransactionId(request.transactionId, request.requestingParties)(errorLogger)
       .andThen(logger.logErrorsOnCall[GetTransactionResponse])
   }
 
   override def getFlatTransactionByEventId(
       request: GetTransactionByEventIdRequest
   ): Future[GetFlatTransactionResponse] = {
-    withEnrichedLoggingContext(
+    implicit val errorLogger: DamlContextualizedErrorLogger = withEnrichedLoggingContext(
       logging.ledgerId(request.ledgerId),
       logging.eventId(request.eventId),
       logging.parties(request.requestingParties),
     ) { implicit loggingContext =>
       logger.info("Received request for flat transaction by event ID.")
+      new DamlContextualizedErrorLogger(logger, loggingContext, None)
     }
     logger.trace(s"Flat transaction by event ID request: $request")
+
     LfEventId
       .fromString(request.eventId.unwrap)
-      .fold(
-        err =>
-          Future.failed[GetFlatTransactionResponse](
-            Status.NOT_FOUND.withDescription(s"invalid eventId: $err").asRuntimeException()
-          ),
-        eventId =>
-          lookUpFlatByTransactionId(
-            TransactionId(eventId.transactionId),
-            request.requestingParties,
-          ),
-      )
+      .map { case LfEventId(transactionId, _) =>
+        lookUpFlatByTransactionId(TransactionId(transactionId), request.requestingParties)
+      }
+      .getOrElse {
+        val msg = s"eventId: ${request.eventId}"
+        Future.failed(errorFactories.invalidArgumentWasNotFound(None)(msg))
+      }
       .andThen(logger.logErrorsOnCall[GetFlatTransactionResponse])
   }
 
   override def getFlatTransactionById(
       request: GetTransactionByIdRequest
   ): Future[GetFlatTransactionResponse] = {
-    withEnrichedLoggingContext(
+    val errorLogger = withEnrichedLoggingContext(
       logging.ledgerId(request.ledgerId),
       logging.transactionId(request.transactionId),
       logging.parties(request.requestingParties),
     ) { implicit loggingContext =>
       logger.info("Received request for flat transaction by ID.")
+      new DamlContextualizedErrorLogger(logger, loggingContext, None)
     }
     logger.trace(s"Flat transaction by ID request: $request")
-    lookUpFlatByTransactionId(request.transactionId, request.requestingParties)
+
+    lookUpFlatByTransactionId(request.transactionId, request.requestingParties)(errorLogger)
       .andThen(logger.logErrorsOnCall[GetFlatTransactionResponse])
   }
 
   private def lookUpTreeByTransactionId(
       transactionId: TransactionId,
       requestingParties: Set[Party],
-  ): Future[GetTransactionResponse] =
+  )(implicit errorLogger: ContextualizedErrorLogger): Future[GetTransactionResponse] =
     transactionsService
       .getTransactionTreeById(transactionId, requestingParties)
-      .map(getOrElseThrowNotFound)
+      .flatMap {
+        case None => Future.failed(errorFactories.transactionNotFound(transactionId.unwrap))
+        case Some(transaction) => Future.successful(transaction)
+      }
 
   private def lookUpFlatByTransactionId(
       transactionId: TransactionId,
       requestingParties: Set[Party],
-  ): Future[GetFlatTransactionResponse] =
+  )(implicit errorLogger: ContextualizedErrorLogger): Future[GetFlatTransactionResponse] =
     transactionsService
       .getTransactionById(transactionId, requestingParties)
-      .map(getOrElseThrowNotFound)
+      .flatMap {
+        case None => Future.failed(errorFactories.transactionNotFound(transactionId.unwrap))
+        case Some(transaction) => Future.successful(transaction)
+      }
 
   private def transactionsLoggable(transactions: GetTransactionsResponse): String =
     s"Responding with transactions: ${transactions.transactions.toList