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.

Sign up for GitHub

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-656] Assert on self-service error code details in ErrorFactoriesSpec #11289

Merged
merged 2 commits into from
Oct 20, 2021
Merged

[DPP-656] Assert on self-service error code details in ErrorFactoriesSpec #11289

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b6f0af2
[DPP-656] Assert on self-service error code details in ErrorFactories…
pbatko-da Oct 19, 2021
9ba71cc
TV review
pbatko-da Oct 19, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
116 changes: 108 additions & 8 deletions ...test/suite/scala/com/digitalasset/platform/server/api/validation/ErrorFactoriesSpec.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,21 +4,22 @@
package com.daml

import com.daml.error.{
DamlContextualizedErrorLogger,
ContextualizedErrorLogger,
DamlContextualizedErrorLogger,
ErrorCodesVersionSwitcher,
}
import com.daml.ledger.api.domain.LedgerId
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.rpc.Status
import com.google.rpc.{ErrorInfo, RequestInfo, ResourceInfo, RetryInfo, Status}
import io.grpc.Status.Code
import io.grpc.StatusRuntimeException
import io.grpc.protobuf.StatusProto
import org.scalatest.matchers.should.Matchers
import org.scalatest.prop.TableDrivenPropertyChecks
import org.scalatest.wordspec.AnyWordSpec
import com.google.protobuf

import scala.jdk.CollectionConverters._

Expand All @@ -30,6 +31,9 @@ class ErrorFactoriesSpec extends AnyWordSpec with Matchers with TableDrivenPrope
private implicit val contextualizedErrorLogger: ContextualizedErrorLogger =
new DamlContextualizedErrorLogger(logger, loggingContext, Some(correlationId))

private val DefaultTraceIdRequestInfo: ErrorDetails.RequestInfoDetail =
ErrorDetails.RequestInfoDetail("trace-id")

"ErrorFactories" should {
"return the DuplicateCommandException" in {
assertVersionedError(_.duplicateCommandException)(
Expand All @@ -39,6 +43,10 @@ class ErrorFactoriesSpec extends AnyWordSpec with Matchers with TableDrivenPrope
v2_code = Code.ALREADY_EXISTS,
v2_message =
s"DUPLICATE_COMMAND(10,$correlationId): A command with the given command id has already been successfully processed",
v2_details = Seq[ErrorDetails.ErrorDetail](
ErrorDetails.ErrorInfoDetail("DUPLICATE_COMMAND"),
DefaultTraceIdRequestInfo,
),
)
}

Expand All @@ -50,6 +58,10 @@ class ErrorFactoriesSpec extends AnyWordSpec with Matchers with TableDrivenPrope
v2_code = Code.PERMISSION_DENIED,
v2_message =
s"An error occurred. Please contact the operator and inquire about the request $correlationId",
v2_details = Seq[ErrorDetails.ErrorDetail](
ErrorDetails.ErrorInfoDetail("PERMISSION_DENIED"),
DefaultTraceIdRequestInfo,
),
)
}

Expand All @@ -68,6 +80,10 @@ class ErrorFactoriesSpec extends AnyWordSpec with Matchers with TableDrivenPrope
v2_code = Code.NOT_FOUND,
v2_message =
s"LEDGER_CONFIGURATION_NOT_FOUND(11,$correlationId): The ledger configuration is not available.",
v2_details = Seq[ErrorDetails.ErrorDetail](
ErrorDetails.ErrorInfoDetail("LEDGER_CONFIGURATION_NOT_FOUND"),
DefaultTraceIdRequestInfo,
),
)
}
}
Expand Down Expand Up @@ -105,6 +121,10 @@ class ErrorFactoriesSpec extends AnyWordSpec with Matchers with TableDrivenPrope
v2_code = Code.INVALID_ARGUMENT,
v2_message =
s"INVALID_FIELD(8,$correlationId): The submitted command has a field with invalid value: Invalid field my field: my message",
v2_details = Seq[ErrorDetails.ErrorDetail](
ErrorDetails.ErrorInfoDetail("INVALID_FIELD"),
DefaultTraceIdRequestInfo,
),
)
}
}
Expand All @@ -117,6 +137,10 @@ class ErrorFactoriesSpec extends AnyWordSpec with Matchers with TableDrivenPrope
v2_code = Code.UNAUTHENTICATED,
v2_message =
s"An error occurred. Please contact the operator and inquire about the request $correlationId",
v2_details = Seq[ErrorDetails.ErrorDetail](
ErrorDetails.ErrorInfoDetail("UNAUTHENTICATED"),
DefaultTraceIdRequestInfo,
),
)
}

Expand All @@ -137,6 +161,10 @@ class ErrorFactoriesSpec extends AnyWordSpec with Matchers with TableDrivenPrope
v2_code = Code.NOT_FOUND,
v2_message =
s"LEDGER_ID_MISMATCH(11,$correlationId): Ledger ID 'received' not found. Actual Ledger ID is 'expected'.",
v2_details = Seq[ErrorDetails.ErrorDetail](
ErrorDetails.ErrorInfoDetail("LEDGER_ID_MISMATCH"),
DefaultTraceIdRequestInfo,
),
)
}
}
Expand All @@ -156,6 +184,10 @@ class ErrorFactoriesSpec extends AnyWordSpec with Matchers with TableDrivenPrope
v1_details = Seq.empty,
v2_code = Code.OUT_OF_RANGE,
v2_message = s"PARTICIPANT_PRUNED_DATA_ACCESSED(12,$correlationId): my message",
v2_details = Seq[ErrorDetails.ErrorDetail](
ErrorDetails.ErrorInfoDetail("PARTICIPANT_PRUNED_DATA_ACCESSED"),
DefaultTraceIdRequestInfo,
),
)
}

Expand All @@ -166,6 +198,10 @@ class ErrorFactoriesSpec extends AnyWordSpec with Matchers with TableDrivenPrope
v1_details = Seq.empty,
v2_code = Code.OUT_OF_RANGE,
v2_message = s"REQUESTED_OFFSET_OUT_OF_RANGE(12,$correlationId): my message",
v2_details = Seq[ErrorDetails.ErrorDetail](
ErrorDetails.ErrorInfoDetail("REQUESTED_OFFSET_OUT_OF_RANGE"),
DefaultTraceIdRequestInfo,
),
)
}

Expand All @@ -183,6 +219,11 @@ class ErrorFactoriesSpec extends AnyWordSpec with Matchers with TableDrivenPrope
v1_details = expectedDetails,
v2_code = Code.UNAVAILABLE,
v2_message = s"SERVICE_NOT_RUNNING(1,$correlationId): Service has been shut down.",
v2_details = Seq[ErrorDetails.ErrorDetail](
ErrorDetails.ErrorInfoDetail("SERVICE_NOT_RUNNING"),
DefaultTraceIdRequestInfo,
ErrorDetails.RetryInfoDetail(1),
),
)
}
}
Expand All @@ -195,6 +236,10 @@ class ErrorFactoriesSpec extends AnyWordSpec with Matchers with TableDrivenPrope
v2_code = Code.NOT_FOUND,
v2_message =
s"LEDGER_CONFIGURATION_NOT_FOUND(11,$correlationId): The ledger configuration is not available.",
v2_details = Seq[ErrorDetails.ErrorDetail](
ErrorDetails.ErrorInfoDetail("LEDGER_CONFIGURATION_NOT_FOUND"),
DefaultTraceIdRequestInfo,
),
)
}

Expand All @@ -213,6 +258,10 @@ class ErrorFactoriesSpec extends AnyWordSpec with Matchers with TableDrivenPrope
v2_code = Code.INVALID_ARGUMENT,
v2_message =
s"MISSING_FIELD(8,$correlationId): The submitted command is missing a mandatory field: my field",
v2_details = Seq[ErrorDetails.ErrorDetail](
ErrorDetails.ErrorInfoDetail("MISSING_FIELD"),
DefaultTraceIdRequestInfo,
),
)
}
}
Expand All @@ -232,6 +281,10 @@ class ErrorFactoriesSpec extends AnyWordSpec with Matchers with TableDrivenPrope
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,
),
)
}
}
Expand All @@ -245,29 +298,76 @@ class ErrorFactoriesSpec extends AnyWordSpec with Matchers with TableDrivenPrope

private def assertVersionedError(
error: ErrorFactories => StatusRuntimeException
)(v1_code: Code, v1_message: String, v1_details: Seq[Any], v2_code: Code, v2_message: String) = {
)(
v1_code: Code,
v1_message: String,
v1_details: Seq[Any],
v2_code: Code,
v2_message: String,
v2_details: Seq[ErrorDetails.ErrorDetail],
Comment on lines +302 to +307
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be camel case, right?

Copy link
Contributor

@tudor-da tudor-da Oct 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I wrote them like this in the initial implementation invoking better readability. However, I don't have a strong opinion here and I dislike as well random snake-case in Scala files. Feel free to change if necessary or use a different naming scheme/signature for improving readability

): Unit = {
val errorFactoriesV1 = ErrorFactories(new ErrorCodesVersionSwitcher(false))
val errorFactoriesV2 = ErrorFactories(new ErrorCodesVersionSwitcher(true))
assertV1Error(error(errorFactoriesV1))(v1_code, v1_message, v1_details)
assertV2Error(error(errorFactoriesV2))(v2_code, v2_message)
assertV2Error(error(errorFactoriesV2))(v2_code, v2_message, v2_details)
}

private def assertV1Error(
statusRuntimeException: StatusRuntimeException
)(expectedCode: Code, expectedMessage: String, expectedDetails: Seq[Any]) = {
)(expectedCode: Code, expectedMessage: String, expectedDetails: Seq[Any]): Unit = {
val status = StatusProto.fromThrowable(statusRuntimeException)
status.getCode shouldBe expectedCode.value()
status.getMessage shouldBe expectedMessage
status.getDetailsList.asScala shouldBe expectedDetails
val _ = status.getDetailsList.asScala shouldBe expectedDetails
Comment on lines 318 to +321
Copy link
Contributor

Choose a reason for hiding this comment

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

You could use:

daml/libs-scala/scala-utils/src/main/scala/com/daml/scalautil/Statement.scala

Line 13 in 525e4ce

@specialized def discard[A](evaluateForSideEffectOnly: A): Unit = {

like:

    discard {
      val status = StatusProto.fromThrowable(statusRuntimeException)
      status.getCode shouldBe expectedCode.value()
      status.getMessage shouldBe expectedMessage
      status.getDetailsList.asScala shouldBe expectedDetails
    }

It's just a matter of preference though.

}

private def assertV2Error(
statusRuntimeException: StatusRuntimeException
)(expectedCode: Code, expectedMessage: String) = {
)(
expectedCode: Code,
expectedMessage: String,
expectedDetails: Seq[ErrorDetails.ErrorDetail],
): Unit = {
val status = StatusProto.fromThrowable(statusRuntimeException)
status.getCode shouldBe expectedCode.value()
status.getMessage shouldBe expectedMessage
// TODO error codes: Assert error details
Copy link
Contributor

Choose a reason for hiding this comment

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

🙌

val details = status.getDetailsList.asScala.toSeq
val _ = ErrorDetails.from(details) should contain theSameElementsAs (expectedDetails)
// TODO error codes: Assert logging
}
}

object ErrorDetails {

sealed trait ErrorDetail

final case class ResourceInfoDetail(name: String, typ: String) extends ErrorDetail

final case class ErrorInfoDetail(reason: String) extends ErrorDetail

final case class RetryInfoDetail(retryDelayInSeconds: Long) extends ErrorDetail

final case class RequestInfoDetail(requestId: String) extends ErrorDetail

def from(anys: Seq[protobuf.Any]): Seq[ErrorDetail] = {
anys.toList.map(from)
}

private def from(any: protobuf.Any): ErrorDetail = {
if (any.is(classOf[ResourceInfo])) {
val v = any.unpack(classOf[ResourceInfo])
ResourceInfoDetail(v.getResourceType, v.getResourceName)
} else if (any.is(classOf[ErrorInfo])) {
val v = any.unpack(classOf[ErrorInfo])
ErrorInfoDetail(v.getReason)
} else if (any.is(classOf[RetryInfo])) {
val v = any.unpack(classOf[RetryInfo])
RetryInfoDetail(v.getRetryDelay.getSeconds)
} else if (any.is(classOf[RequestInfo])) {
val v = any.unpack(classOf[RequestInfo])
RequestInfoDetail(v.getRequestId)
} else {
throw new IllegalStateException(s"Could not unpack value of: |$any|")
}
}
}