From 88c607b781f2d11ecac2b1ab04398a3bd61cb5b5 Mon Sep 17 00:00:00 2001 From: pbatko-da Date: Thu, 21 Oct 2021 00:18:26 +0200 Subject: [PATCH] [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 --- .../error/ErrorCodesVersionSwitcher.scala | 4 - .../scala/com/daml/error/ErrorResource.scala | 3 + .../error/definitions/LedgerApiErrors.scala | 21 ++++- .../api/validation/ErrorFactories.scala | 38 ++++++++ .../api/validation/ErrorFactoriesSpec.scala | 56 ++++++++++-- .../transaction/ApiTransactionService.scala | 86 +++++++++---------- 6 files changed, 153 insertions(+), 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 index f1b9e5e61ef4..0172b67e9be1 100644 --- 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 index aa4dcc83444b..7953e482e639 100644 --- 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 index 2169cfb0c468..524fa6e54fa0 100644 --- 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/ledger/ledger-api-common/src/main/scala/com/digitalasset/platform/server/api/validation/ErrorFactories.scala b/ledger/ledger-api-common/src/main/scala/com/digitalasset/platform/server/api/validation/ErrorFactories.scala index 77df349d61f8..260783e2801e 100644 --- a/ledger/ledger-api-common/src/main/scala/com/digitalasset/platform/server/api/validation/ErrorFactories.scala +++ b/ledger/ledger-api-common/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/ledger/ledger-api-common/src/test/suite/scala/com/digitalasset/platform/server/api/validation/ErrorFactoriesSpec.scala b/ledger/ledger-api-common/src/test/suite/scala/com/digitalasset/platform/server/api/validation/ErrorFactoriesSpec.scala index 752f83684f38..5efce9320171 100644 --- a/ledger/ledger-api-common/src/test/suite/scala/com/digitalasset/platform/server/api/validation/ErrorFactoriesSpec.scala +++ b/ledger/ledger-api-common/src/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/ledger/participant-integration-api/src/main/scala/platform/apiserver/services/transaction/ApiTransactionService.scala b/ledger/participant-integration-api/src/main/scala/platform/apiserver/services/transaction/ApiTransactionService.scala index a7ee8cd9577c..8807eca27a55 100644 --- a/ledger/participant-integration-api/src/main/scala/platform/apiserver/services/transaction/ApiTransactionService.scala +++ b/ledger/participant-integration-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,31 +123,27 @@ 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]) } @@ -159,75 +151,83 @@ private[apiserver] final class ApiTransactionService private ( 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