Skip to content

Commit

Permalink
Update MissingPackage error to allow reporting by package name (#19972
Browse files Browse the repository at this point in the history
)

In the participant, the handling of a command which contains a template id whose package id is not known should be the same, irrespective of whether the template id specified the package with a package id or a package name. From that point of view, it makes sense to handle both cases with the same error.

If the `MissingPackage` type contained a `PackageRef` rather than a `PackageId` it could make this more uniform, and cases which did need to distinguish could still pattern match on the subtype of `PackageRef`, as needed.
  • Loading branch information
raphael-speyer-da authored Sep 27, 2024
1 parent a2c7133 commit d5dda36
Show file tree
Hide file tree
Showing 9 changed files with 37 additions and 29 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,9 @@ object RejectionGenerators {
case Package.Validation(validationError) =>
CommandExecutionErrors.Package.PackageValidationFailed
.Reject(validationError.pretty)
case Package.MissingPackage(packageId, context) =>
case Package.MissingPackage(packageRef, context) =>
RequestValidationErrors.NotFound.Package
.InterpretationReject(Ref.PackageRef.Id(packageId), context)
.InterpretationReject(packageRef, context)
case Package.AllowedLanguageVersion(packageId, languageVersion, allowedLanguageVersions) =>
CommandExecutionErrors.Package.AllowedLanguageVersions.Error(
packageId,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ object PackageServiceErrors extends PackageServiceErrorGroup {
"detailMsg" -> detailMsg,
),
)
final case class Error(missing: Set[PackageId])(implicit
final case class Error(missing: Set[Ref.PackageRef])(implicit
val loggingContext: ContextualizedErrorLogger
) extends DamlError(
cause = "Failed to resolve package ids locally.",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import com.digitalasset.canton.ledger.error.LedgerApiErrors.{
}
import com.digitalasset.canton.ledger.error.ParticipantErrorGroup.LedgerApiErrorGroup.RequestValidationErrorGroup
import com.digitalasset.daml.lf.data.Ref
import com.digitalasset.daml.lf.language.Reference
import com.digitalasset.daml.lf.language.{LookupError, Reference}

import java.time.Duration

Expand Down Expand Up @@ -49,14 +49,12 @@ object RequestValidationErrors extends RequestValidationErrorGroup {
}

final case class InterpretationReject(
pkgRef: Ref.PackageRef,
packageRef: Ref.PackageRef,
reference: Reference,
)(implicit
loggingContext: ContextualizedErrorLogger
) extends DamlErrorWithDefiniteAnswer(
// TODO(i21337) Use LookupError.MissingPackage.pretty(packageRef, reference)
// once it can accept a package reference
cause = s"Couldn't find package $pkgRef while looking for " + reference.pretty
cause = LookupError.MissingPackage.pretty(packageRef, reference)
)
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import com.digitalasset.canton.platform.store.packagemeta.PackageMetadata.Implic
import com.digitalasset.canton.time.Clock
import com.digitalasset.canton.tracing.TraceContext
import com.digitalasset.daml.lf.archive.{DamlLf, Decode}
import com.digitalasset.daml.lf.data.Ref.PackageRef
import org.apache.pekko.actor.ActorSystem
import org.apache.pekko.stream.scaladsl.Source

Expand Down Expand Up @@ -129,7 +130,7 @@ class MutablePackageMetadataViewImpl(
.flatMap {
case Some(pkg) => Future.successful(pkg)
case None =>
Future.failed(PackageServiceErrors.InternalError.Error(Set(packageId)).asGrpcError)
Future.failed(PackageServiceErrors.InternalError.Error(Set(PackageRef.Id(packageId))).asGrpcError)
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,14 +37,19 @@ object Error {
def message: String = validationError.pretty
}

final case class MissingPackage(packageId: Ref.PackageId, context: language.Reference)
final case class MissingPackage(packageRef: Ref.PackageRef, context: language.Reference)
extends Error {
override def message: String = LookupError.MissingPackage.pretty(packageId, context)
override def message: String = LookupError.MissingPackage.pretty(packageRef, context)
}

object MissingPackage {
def apply(packageId: Ref.PackageId): MissingPackage =
MissingPackage(packageId, language.Reference.Package(packageId))
def apply(packageRef: Ref.PackageRef): MissingPackage =
MissingPackage(packageRef, language.Reference.Package(packageRef))

def apply(packageId: Ref.PackageId): MissingPackage = apply(Ref.PackageRef.Id(packageId))

def apply(packageId: Ref.PackageId, context: language.Reference): MissingPackage =
apply(Ref.PackageRef.Id(packageId), context)
}

final case class AllowedLanguageVersion(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
package com.digitalasset.daml.lf
package engine

import com.digitalasset.daml.lf.data.Ref.{Identifier, Name, PackageId}
import com.digitalasset.daml.lf.data.Ref.{Identifier, Name, PackageId, PackageRef}
import com.digitalasset.daml.lf.language.{Ast, LookupError}
import com.digitalasset.daml.lf.transaction.{
GlobalKey,
Expand Down Expand Up @@ -86,7 +86,7 @@ final class ValueEnricher(

private[this] def handleLookup[X](lookup: => Either[LookupError, X]) = lookup match {
case Right(value) => ResultDone(value)
case Left(LookupError.MissingPackage(pkgId, context)) =>
case Left(LookupError.MissingPackage(PackageRef.Id(pkgId), context)) =>
loadPackage(pkgId, context)
.flatMap(_ =>
lookup match {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,19 +24,23 @@ object LookupError {
}

object MissingPackage {
def unapply(err: NotFound): Option[(PackageId, Reference)] =
def unapply(err: NotFound): Option[(PackageRef, Reference)] =
err.notFound match {
case Reference.Package(packageId) => Some(packageId -> err.context)
case Reference.Package(packageRef) => Some(packageRef -> err.context)
case _ => None
}

def apply(pkgId: PackageId): NotFound = {
val ref = Reference.Package(pkgId)
def apply(pkgRef: PackageRef): NotFound = {
val ref = Reference.Package(pkgRef)
LookupError.NotFound(ref, ref)
}

def pretty(pkgId: PackageId, context: Reference): String =
s"Couldn't find package $pkgId" + contextDetails(context)
def apply(pkgId: PackageId): NotFound = apply(PackageRef.Id(pkgId))

def pretty(pkgRef: PackageRef, context: Reference): String =
s"Couldn't find package $pkgRef" + contextDetails(context)

def pretty(pkgId: PackageId, context: Reference): String = pretty(PackageRef.Id(pkgId), context)
}

}
Expand All @@ -47,13 +51,10 @@ sealed abstract class Reference extends Product with Serializable {

object Reference {

final case class PackageWithName(packageName: PackageName) extends Reference {
override def pretty: String = s"package $packageName"
}

final case class Package(packageId: PackageId) extends Reference {
override def pretty: String = s"package $packageId"
final case class Package(packageRef: PackageRef) extends Reference {
override def pretty: String = s"package $packageRef"
}
object Package { def apply(packageId: PackageId): Package = apply(PackageRef.Id(packageId)) }

final case class Module(packageId: PackageId, moduleName: ModuleName) extends Reference {
override def pretty: String = s"module $packageId:$moduleName"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,9 @@ object TransactionBuilder {
implicit def toPackageId(s: String): Ref.PackageId =
Ref.PackageId.assertFromString(s)

implicit def toPackageRef(s: String): Ref.PackageRef =
Ref.PackageRef.assertFromString(s)

implicit def toDottedName(s: String): Ref.DottedName =
Ref.DottedName.assertFromString(s)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -430,9 +430,9 @@ class IdeLedgerClient(
ref match {
// TODO: https://github.com/digital-asset/daml/issues/17995
// add support for package name
case Reference.PackageWithName(_) =>
case Reference.Package(PackageRef.Name(_)) =>
throw new IllegalArgumentException("package name not support")
case Reference.Package(packageId) => packageId
case Reference.Package(PackageRef.Id(packageId)) => packageId
case Reference.Module(packageId, _) => packageId
case Reference.Definition(name) => name.packageId
case Reference.TypeSyn(name) => name.packageId
Expand Down

0 comments on commit d5dda36

Please sign in to comment.