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

fix: Schema Error Handling #1138

Merged
merged 2 commits into from
Jun 5, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -61,21 +61,15 @@ class SchemaRegistryServerEndpoints(
lookupSchemasByQueryEndpoint
.zServerSecurityLogic(SecurityLogic.authorizeWalletAccessWith(_)(authenticator, authorizer))
.serverLogic { wac =>
{
case (
ctx: RequestContext,
filter: FilterInput,
paginationInput: PaginationInput,
order: Option[Order]
) =>
credentialSchemaController
.lookupSchemas(
filter,
paginationInput.toPagination,
order
)(ctx)
.provideSomeLayer(ZLayer.succeed(wac))
.logTrace(ctx)
{ case (ctx: RequestContext, filter: FilterInput, paginationInput: PaginationInput, order: Option[Order]) =>
credentialSchemaController
.lookupSchemas(
filter,
paginationInput.toPagination,
order
)(ctx)
.provideSomeLayer(ZLayer.succeed(wac))
.logTrace(ctx)
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ package org.hyperledger.identus.pollux.credentialschema.controller
import org.hyperledger.identus.api.http.*
import org.hyperledger.identus.api.http.model.{Order, Pagination}
import org.hyperledger.identus.pollux.core.service.CredentialSchemaService
import org.hyperledger.identus.pollux.core.service.CredentialSchemaService.Error.*
import org.hyperledger.identus.pollux.core.service.CredentialSchemaService.*
import org.hyperledger.identus.pollux.credentialschema.http.{
CredentialSchemaInput,
CredentialSchemaResponse,
Expand Down Expand Up @@ -50,23 +50,7 @@ trait CredentialSchemaController {
object CredentialSchemaController {
def domainToHttpError(
error: CredentialSchemaService.Error
): ErrorResponse = {
error match {
case RepositoryError(cause: Throwable) =>
ErrorResponse.internalServerError("RepositoryError", detail = Option(cause.toString))
case NotFoundError(_, _, message) =>
ErrorResponse.notFound(detail = Option(message))
case UpdateError(id, version, author, message) =>
ErrorResponse.badRequest(
title = "CredentialSchemaUpdateError",
detail = Option(s"Credential schema update error: id=$id, version=$version, author=$author, msg=$message")
)
case UnexpectedError(msg: String) =>
ErrorResponse.internalServerError(detail = Option(msg))
case CredentialSchemaValidationError(cause) =>
ErrorResponse.badRequest(detail = Some(cause.message))
}
}
): ErrorResponse = error

implicit def domainToHttpErrorIO[R, T](
domainIO: ZIO[R, CredentialSchemaService.Error, T]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import org.hyperledger.identus.api.http.model.{CollectionStats, Order, Paginatio
import org.hyperledger.identus.castor.core.model.did.{LongFormPrismDID, PrismDID}
import org.hyperledger.identus.pollux.core.model.schema.CredentialSchema.FilteredEntries
import org.hyperledger.identus.pollux.core.service.CredentialSchemaService
import org.hyperledger.identus.pollux.core.service.CredentialSchemaService.Error.*
import org.hyperledger.identus.pollux.core.service.CredentialSchemaService.*
import org.hyperledger.identus.pollux.credentialschema.controller.CredentialSchemaController.domainToHttpErrorIO
import org.hyperledger.identus.pollux.credentialschema.http.{
CredentialSchemaInput,
Expand All @@ -30,29 +30,23 @@ class CredentialSchemaControllerImpl(service: CredentialSchemaService, managedDI
)(implicit
rc: RequestContext
): ZIO[WalletAccessContext, ErrorResponse, CredentialSchemaResponse] = {
(for {
for {
validated <- validatePrismDID(in.author)
result <- service
.create(toDomain(in))
.map(cs => fromDomain(cs).withBaseUri(rc.request.uri))
} yield result).mapError {
case e: ErrorResponse => e
case e: CredentialSchemaService.Error => CredentialSchemaController.domainToHttpError(e)
}
} yield result
}

override def updateSchema(author: String, id: UUID, in: CredentialSchemaInput)(implicit
rc: RequestContext
): ZIO[WalletAccessContext, ErrorResponse, CredentialSchemaResponse] = {
(for {
for {
_ <- validatePrismDID(in.author)
result <- service
.update(id, toDomain(in).copy(author = author))
.map(cs => fromDomain(cs).withBaseUri(rc.request.uri))
} yield result).mapError {
case e: ErrorResponse => e
case e: CredentialSchemaService.Error => CredentialSchemaController.domainToHttpError(e)
}
} yield result
}

override def getSchemaByGuid(guid: UUID)(implicit
Expand Down Expand Up @@ -112,7 +106,7 @@ class CredentialSchemaControllerImpl(service: CredentialSchemaService, managedDI
for {
authorDID <- ZIO
.fromEither(PrismDID.fromString(author))
.mapError(_ => ErrorResponse.badRequest(detail = Some(s"Unable to parse as a Prism DID: ${author}")))
.mapError(ex => ErrorResponse.badRequest(detail = Some(s"Unable to parse Prism DID from '${author}' due: $ex")))
longFormPrismDID <- getLongForm(authorDID, true)
} yield longFormPrismDID

Expand All @@ -125,7 +119,7 @@ class CredentialSchemaControllerImpl(service: CredentialSchemaService, managedDI
.getManagedDIDState(did.asCanonical)
.mapError(e =>
ErrorResponse.internalServerError(detail =
Some(s"Error occurred while getting did from wallet: ${e.toString}")
Some(s"Error occurred while getting DID from wallet: ${e.toString}")
)
)
.someOrFail(ErrorResponse.notFound(detail = Some(s"Issuer DID does not exist in the wallet: $did")))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,10 @@ case class CredentialSchemaControllerLogic(
stats: CollectionStats
) {

def composeNextUri(uri: Uri): Option[Uri] =
private def composeNextUri(uri: Uri): Option[Uri] =
PaginationUtils.composeNextUri(uri, page.contents, pagination, stats)

def composePreviousUri(uri: Uri): Option[Uri] =
private def composePreviousUri(uri: Uri): Option[Uri] =
PaginationUtils.composePreviousUri(uri, page.contents, pagination, stats)

def result: CredentialSchemaResponsePage = {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,17 @@ object CredentialSchemaAnoncredSpec extends ZIOSpecDefault with CredentialSchema
for {
response <- createResponse[ErrorResponse]("WrongSchema")
} yield assert(response.body)(
isRight(hasField("detail", _.detail, isSome(equalTo("Unsupported VC Schema type WrongSchema"))))
isRight(
hasField(
"detail",
_.detail,
isSome(
equalTo(
"Credential Schema Validation Error=UnsupportedCredentialSchemaType(Unsupported VC Schema type WrongSchema)"
)
)
)
)
)
}
)
Expand All @@ -121,7 +131,7 @@ object CredentialSchemaAnoncredSpec extends ZIOSpecDefault with CredentialSchema
response <- createResponse[ErrorResponse](CredentialJsonSchemaType.`type`)
} yield assert(response.body)(
isRight(
hasField("detail", _.detail, isSome(containsString("required property '$schema' not found;")))
hasField("detail", _.detail, isSome(containsString("required property '$schema' not found")))
)
)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -105,10 +105,10 @@ object CredentialSchemaMultiTenancySpec extends ZIOSpecDefault with CredentialSc
.exit

aliceCannotUpdateBobsVCSchema = assert(notFoundSchemaAError)(
fails(isSubtype[CredentialSchemaService.Error.NotFoundError](anything))
fails(isSubtype[CredentialSchemaService.GuidNotFoundError](anything))
)
bobCannotUpdateAlicesVCSchema = assert(notFoundSchemaBError)(
fails(isSubtype[CredentialSchemaService.Error.NotFoundError](anything))
fails(isSubtype[CredentialSchemaService.GuidNotFoundError](anything))
)

fetchedSchemaAbyB <- service.getByGUID(updatedSchemaA.guid).provideLayer(Bob.wacLayer)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -113,11 +113,11 @@ trait CredentialSchemaTestTools extends PostgresTestContainerSupport {
backend
}

def deleteAllCredentialSchemas: RIO[CredentialSchemaRepository & WalletAccessContext, Long] = {
def deleteAllCredentialSchemas: RIO[CredentialSchemaRepository & WalletAccessContext, Unit] = {
for {
repository <- ZIO.service[CredentialSchemaRepository]
count <- repository.deleteAll()
} yield count
result <- repository.deleteAll()
} yield result
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ object CredentialDefinition {
definition: Definition,
proofSchemaId: String,
proof: CorrectnessProof
): ZIO[Any, Nothing, CredentialDefinition] = {
): UIO[CredentialDefinition] = {
for {
id <- zio.Random.nextUUID
cs <- make(id, in, definitionSchemaId, definition, proofSchemaId, proof)
Expand All @@ -100,7 +100,7 @@ object CredentialDefinition {
definition: Definition,
keyCorrectnessProofSchemaId: String,
keyCorrectnessProof: CorrectnessProof
): ZIO[Any, Nothing, CredentialDefinition] = {
): UIO[CredentialDefinition] = {
for {
ts <- zio.Clock.currentDateTime.map(
_.atZoneSameInstant(ZoneOffset.UTC).toOffsetDateTime
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,13 +65,13 @@ object CredentialSchema {
def makeGUID(author: String, id: UUID, version: String) =
UUID.nameUUIDFromBytes(makeLongId(author, id, version).getBytes)

def make(in: Input): ZIO[Any, Nothing, CredentialSchema] = {
def make(in: Input): UIO[CredentialSchema] = {
for {
id <- zio.Random.nextUUID
cs <- make(id, in)
} yield cs
}
def make(id: UUID, in: Input): ZIO[Any, Nothing, CredentialSchema] = {
def make(id: UUID, in: Input): UIO[CredentialSchema] = {
for {
ts <- zio.Clock.currentDateTime.map(
_.atZoneSameInstant(ZoneOffset.UTC).toOffsetDateTime
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ class CredentialDefinitionRepositoryInMemory(

override def search(
query: Repository.SearchQuery[CredentialDefinition.Filter]
): RIO[WalletAccessContext, Repository.SearchResult[CredentialDefinition]] = {
): URIO[WalletAccessContext, Repository.SearchResult[CredentialDefinition]] = {
walletStoreRef.flatMap { storeRef =>
storeRef.get.map { store =>
val filtered = store.values.filter { credDef =>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,22 +4,22 @@ import org.hyperledger.identus.pollux.core.model.schema.CredentialSchema
import org.hyperledger.identus.pollux.core.model.schema.CredentialSchema.*
import org.hyperledger.identus.pollux.core.repository.Repository.SearchCapability
import org.hyperledger.identus.shared.models.WalletAccessContext
import zio.{RIO, Task}
import zio.{UIO, URIO}

import java.util.UUID

trait CredentialSchemaRepository
extends Repository[WalletTask, CredentialSchema]
with SearchCapability[WalletTask, CredentialSchema.Filter, CredentialSchema] {
def create(cs: CredentialSchema): RIO[WalletAccessContext, CredentialSchema]
def create(cs: CredentialSchema): URIO[WalletAccessContext, CredentialSchema]

def getByGuid(guid: UUID): Task[Option[CredentialSchema]]
def findByGuid(guid: UUID): UIO[Option[CredentialSchema]]

def update(cs: CredentialSchema): RIO[WalletAccessContext, Option[CredentialSchema]]
def update(cs: CredentialSchema): URIO[WalletAccessContext, CredentialSchema]

def getAllVersions(id: UUID, author: String): RIO[WalletAccessContext, Seq[String]]
def getAllVersions(id: UUID, author: String): URIO[WalletAccessContext, List[String]]

def delete(guid: UUID): RIO[WalletAccessContext, Option[CredentialSchema]]
def delete(guid: UUID): URIO[WalletAccessContext, CredentialSchema]

def deleteAll(): RIO[WalletAccessContext, Long]
def deleteAll(): URIO[WalletAccessContext, Unit]
}
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
package org.hyperledger.identus.pollux.core.repository

import org.hyperledger.identus.shared.models.WalletAccessContext
import zio.RIO
import zio.URIO

trait Repository[F[_], T]

type WalletTask[T] = RIO[WalletAccessContext, T]
type WalletTask[T] = URIO[WalletAccessContext, T]

object Repository {
case class SearchQuery[Filter](filter: Filter, skip: Int, limit: Int)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,6 @@ class CredentialDefinitionServiceImpl(
override def lookup(filter: CredentialDefinition.Filter, skip: Int, limit: Int): Result[FilteredEntries] = {
credentialDefinitionRepository
.search(SearchQuery(filter, skip, limit))
.mapError(t => RepositoryError(t))
.map(sr => FilteredEntries(sr.entries, sr.count.toInt, sr.totalCount.toInt))
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,12 @@ package org.hyperledger.identus.pollux.core.service
import org.hyperledger.identus.pollux.core.model.error.CredentialSchemaError
import org.hyperledger.identus.pollux.core.model.schema.CredentialSchema
import org.hyperledger.identus.pollux.core.model.schema.CredentialSchema.*
import org.hyperledger.identus.shared.models.WalletAccessContext
import org.hyperledger.identus.shared.models.{Failure, StatusCode, WalletAccessContext}
import zio.{IO, ZIO}

import java.util.UUID
trait CredentialSchemaService {
type Result[T] = ZIO[WalletAccessContext, CredentialSchemaService.Error, T]
private[service] type Result[T] = ZIO[WalletAccessContext, CredentialSchemaService.Error, T]

/** @param in
* CredentialSchema form for creating the instance
Expand All @@ -32,26 +32,28 @@ trait CredentialSchemaService {
}

object CredentialSchemaService {
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for being picky about this, but for consistency with other services, could you introduce a CredentialSchemaServiceError trait located in its own file, with concrete case classes inside the companion object? Thanks 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

sealed trait Error

object Error {
def apply(throwable: Throwable): Error = RepositoryError(throwable)

final case class RepositoryError(cause: Throwable) extends Error

final case class NotFoundError(guid: Option[UUID] = None, id: Option[UUID] = None, message: String) extends Error

object NotFoundError {
def byGuid(guid: UUID): NotFoundError =
NotFoundError(guid = Option(guid), message = s"Credential schema record cannot be found by `guid`=$guid")
def byId(id: UUID): NotFoundError =
NotFoundError(id = Option(id), message = s"Credential schema record cannot be found by `id`=$id")
}

final case class UpdateError(id: UUID, version: String, author: String, message: String) extends Error

final case class UnexpectedError(msg: String) extends Error

final case class CredentialSchemaValidationError(cause: CredentialSchemaError) extends Error
sealed trait Error(
val statusCode: StatusCode,
val userFacingMessage: String
) extends Failure {
override val namespace = "CredentialSchema"
}

final case class GuidNotFoundError(guid: UUID)
extends Error(
StatusCode.NotFound,
s"Credential Schema record cannot be found by `guid`=$guid"
)

final case class UpdateError(id: UUID, version: String, author: String, message: String)
extends Error(
StatusCode.BadRequest,
s"Credential schema update error: id=$id, version=$version, author=$author, msg=$message"
)

final case class CredentialSchemaValidationError(cause: CredentialSchemaError)
extends Error(
StatusCode.BadRequest,
s"Credential Schema Validation Error=$cause"
Copy link
Contributor

Choose a reason for hiding this comment

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

I would explicitly use $cause.message here. Relying on the default toString is unpredictable on what exactly will be returned to the user.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

)
}
Loading
Loading