Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DPP-645][Self-service error codes] Adapt ApiConfigManagementService #11312

Merged
merged 8 commits into from
Oct 22, 2021
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import io.grpc.StatusRuntimeException

import scala.concurrent.Future

final class ErrorCodesVersionSwitcher(enableSelfServiceErrorCodes: Boolean)
class ErrorCodesVersionSwitcher(enableSelfServiceErrorCodes: Boolean)
extends ValueSwitch[StatusRuntimeException](enableSelfServiceErrorCodes) {

def chooseAsFailedFuture[T](
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,22 @@ object LedgerApiErrors extends LedgerApiErrorGroup {
)
}

object WriteErrors extends ErrorGroup() {
@Explanation("This rejection is given when a configuration entry write was rejected.")
@Resolution("Fetch newest configuration and/or retry.")
object ConfigurationEntryRejected
extends ErrorCode(
id = "CONFIGURATION_ENTRY_REJECTED",
ErrorCategory.InvalidGivenCurrentSystemStateOther,
) {
case class Reject(message: String)(implicit
loggingContext: ContextualizedErrorLogger
) extends LoggingTransactionErrorImpl(
cause = message
)
}
}

object ReadErrors extends ErrorGroup() {

@Explanation("This rejection is given when a package id is malformed.")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,15 @@ class ErrorFactories private (errorCodesVersionSwitcher: ErrorCodesVersionSwitch
grpcError(statusBuilder.build())
}

def configurationEntryRejected(message: String, definiteAnswer: Option[Boolean])(implicit
contextualizedErrorLogger: ContextualizedErrorLogger
): StatusRuntimeException = {
errorCodesVersionSwitcher.choose(
v1 = aborted(message, definiteAnswer),
v2 = LedgerApiErrors.WriteErrors.ConfigurationEntryRejected.Reject(message).asGrpcError,
Copy link
Contributor Author

@pbatko-da pbatko-da Oct 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tudor-da For other management services I would have similar dedicated error codes, e.g.:

  • LedgerApiErrors.WriteErrors.PartyAllocationRejected for ApiPartyManagementService,
  • LedgerApiErrors.WriteErrors.DarUploadRejected for ApiPackageManagementService.
    Wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed offline and AFAIU the above approach is ok for the time being

)
}

// permission denied is intentionally without description to ensure we don't leak security relevant information by accident
def permissionDenied(cause: String)(implicit
contextualizedErrorLogger: ContextualizedErrorLogger
Expand Down Expand Up @@ -302,6 +311,8 @@ 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 Daml interpreter.
v2 = LedgerApiErrors.InterpreterErrors.LookupErrors.LedgerConfigurationNotFound
.Reject()
.asGrpcError,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,20 @@ class ErrorFactoriesSpec extends AnyWordSpec with Matchers with TableDrivenPrope
)
}

"return the configurationEntryRejected" in {
assertVersionedError(_.configurationEntryRejected("message123", None))(
v1_code = Code.ABORTED,
v1_message = "message123",
v1_details = Seq.empty,
v2_code = Code.FAILED_PRECONDITION,
v2_message = s"CONFIGURATION_ENTRY_REJECTED(9,$correlationId): message123",
v2_details = Seq[ErrorDetails.ErrorDetail](
ErrorDetails.ErrorInfoDetail("CONFIGURATION_ENTRY_REJECTED"),
DefaultTraceIdRequestInfo,
),
)
}

"return a transactionNotFound error" in {
assertVersionedError(_.transactionNotFound(Ref.TransactionId.assertFromString("tId")))(
v1_code = Code.NOT_FOUND,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -289,6 +289,7 @@ private[daml] object ApiServices {
configManagementService,
writeService,
timeProvider,
errorsVersionsSwitcher,
)

val apiParticipantPruningService =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,11 @@ import java.time.{Duration => JDuration}
import akka.stream.Materializer
import akka.stream.scaladsl.Source
import com.daml.api.util.{DurationConversion, TimeProvider, TimestampConversion}
import com.daml.error.{DamlContextualizedErrorLogger, ContextualizedErrorLogger}
import com.daml.error.{
ContextualizedErrorLogger,
DamlContextualizedErrorLogger,
ErrorCodesVersionSwitcher,
}
import com.daml.ledger.api.domain
import com.daml.ledger.api.domain.{ConfigurationEntry, LedgerOffset}
import com.daml.ledger.api.v1.admin.config_management_service.ConfigManagementServiceGrpc.ConfigManagementService
Expand Down Expand Up @@ -36,6 +40,7 @@ private[apiserver] final class ApiConfigManagementService private (
writeService: state.WriteConfigService,
timeProvider: TimeProvider,
submissionIdGenerator: String => Ref.SubmissionId,
errorCodesVersionSwitcher: ErrorCodesVersionSwitcher,
)(implicit
materializer: Materializer,
executionContext: ExecutionContext,
Expand All @@ -46,6 +51,9 @@ private[apiserver] final class ApiConfigManagementService private (
private implicit val contextualizedErrorLogger: ContextualizedErrorLogger =
new DamlContextualizedErrorLogger(logger, loggingContext, None)

private val errorFactories =
com.daml.platform.server.api.validation.ErrorFactories(errorCodesVersionSwitcher)

override def close(): Unit = ()

override def bindService(): ServerServiceDefinition =
Expand All @@ -60,7 +68,7 @@ private[apiserver] final class ApiConfigManagementService private (
Future.successful(configurationToResponse(configuration))
case None =>
// TODO error codes: Duplicate of missingLedgerConfig
Future.failed(ErrorFactories.missingLedgerConfigUponRequest)
Future.failed(errorFactories.missingLedgerConfigUponRequest)
}
.andThen(logger.logErrorsOnCall[GetTimeModelResponse])
}
Expand Down Expand Up @@ -106,7 +114,7 @@ private[apiserver] final class ApiConfigManagementService private (
logger.warn(
"Could not get the current time model. The index does not yet have any ledger configuration."
)
Future.failed(ErrorFactories.missingLedgerConfig(None))
Future.failed(errorFactories.missingLedgerConfig(None))
Copy link
Contributor

@tudor-da tudor-da Oct 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method uses LedgerApiErrors.InterpreterErrors.LookupErrors.LedgerConfigurationNotFound, which is again interpreter specific. Please add a TODO (e.g. see invalidArgument)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added

      // TODO error codes: This error group is confusing for this generic error as it can be dispatched
      //                   from call-sites that do not involve Daml interpreter.
      v2 = LedgerApiErrors.InterpreterErrors.LookupErrors.LedgerConfigurationNotFound
        .Reject()
        .asGrpcError,

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's wait to settle the discussion of generic vs non-generic errors.

}
(ledgerEndBeforeRequest, currentConfig) = configuration

Expand All @@ -117,7 +125,7 @@ private[apiserver] final class ApiConfigManagementService private (
Future.failed(
ValidationLogger.logFailureWithContext(
request,
ErrorFactories.invalidArgument(None)(
errorFactories.invalidArgument(None)(
s"Mismatching configuration generation, expected $expectedGeneration, received ${request.configurationGeneration}"
),
)
Expand All @@ -139,6 +147,7 @@ private[apiserver] final class ApiConfigManagementService private (
writeService,
index,
ledgerEndBeforeRequest,
errorFactories,
),
timeToLive = JDuration.ofMillis(params.timeToLive.toMillis),
)
Expand Down Expand Up @@ -176,7 +185,7 @@ private[apiserver] final class ApiConfigManagementService private (
minSkew = DurationConversion.fromProto(pMinSkew),
maxSkew = DurationConversion.fromProto(pMaxSkew),
) match {
case Failure(err) => Left(ErrorFactories.invalidArgument(None)(err.toString))
case Failure(err) => Left(errorFactories.invalidArgument(None)(err.toString))
case Success(ok) => Right(ok)
}
// TODO(JM): The maximum record time should be constrained, probably by the current active time model?
Expand All @@ -189,7 +198,7 @@ private[apiserver] final class ApiConfigManagementService private (
}
maximumRecordTime <- Time.Timestamp
.fromInstant(mrtInstant)
.fold(err => Left(ErrorFactories.invalidArgument(None)(err)), Right(_))
.fold(err => Left(errorFactories.invalidArgument(None)(err)), Right(_))
} yield SetTimeModelParameters(newTimeModel, maximumRecordTime, timeToLive)
}

Expand All @@ -201,6 +210,7 @@ private[apiserver] object ApiConfigManagementService {
readBackend: IndexConfigManagementService,
writeBackend: state.WriteConfigService,
timeProvider: TimeProvider,
errorCodesVersionSwitcher: ErrorCodesVersionSwitcher,
submissionIdGenerator: String => Ref.SubmissionId = augmentSubmissionId,
)(implicit
materializer: Materializer,
Expand All @@ -212,13 +222,15 @@ private[apiserver] object ApiConfigManagementService {
writeBackend,
timeProvider,
submissionIdGenerator,
errorCodesVersionSwitcher,
)

private final class SynchronousResponseStrategy(
writeConfigService: state.WriteConfigService,
configManagementService: IndexConfigManagementService,
ledgerEnd: LedgerOffset.Absolute,
)(implicit loggingContext: LoggingContext)
errorFactories: ErrorFactories,
)(implicit loggingContext: LoggingContext, contextualizedErrorLogger: ContextualizedErrorLogger)
extends SynchronousResponse.Strategy[
(Time.Timestamp, Configuration),
ConfigurationEntry,
Expand Down Expand Up @@ -252,7 +264,7 @@ private[apiserver] object ApiConfigManagementService {
submissionId: Ref.SubmissionId
): PartialFunction[ConfigurationEntry, StatusRuntimeException] = {
case domain.ConfigurationEntry.Rejected(`submissionId`, reason, _) =>
ErrorFactories.aborted(reason, None)
errorFactories.configurationEntryRejected(reason, None)
}
}

Expand Down
Loading