From 39ee00012a6e657d4147e2aa849350204b1625d0 Mon Sep 17 00:00:00 2001 From: Benjamin Voiturier Date: Thu, 28 Mar 2024 14:09:40 +0100 Subject: [PATCH 1/8] chore(connect): minor code cleanup Signed-off-by: Benjamin Voiturier --- .../connect/core/model/error/ConnectionRepositoryError.scala | 5 ----- .../core/repository/ConnectionRepositoryInMemory.scala | 1 - .../atala/connect/core/service/ConnectionServiceImpl.scala | 2 +- .../core/repository/ConnectionRepositorySpecSuite.scala | 1 - 4 files changed, 1 insertion(+), 8 deletions(-) delete mode 100644 connect/lib/core/src/main/scala/io/iohk/atala/connect/core/model/error/ConnectionRepositoryError.scala diff --git a/connect/lib/core/src/main/scala/io/iohk/atala/connect/core/model/error/ConnectionRepositoryError.scala b/connect/lib/core/src/main/scala/io/iohk/atala/connect/core/model/error/ConnectionRepositoryError.scala deleted file mode 100644 index c8277ae76a..0000000000 --- a/connect/lib/core/src/main/scala/io/iohk/atala/connect/core/model/error/ConnectionRepositoryError.scala +++ /dev/null @@ -1,5 +0,0 @@ -package io.iohk.atala.connect.core.model.error - -sealed trait ConnectionRepositoryError extends Throwable - -object ConnectionRepositoryError {} diff --git a/connect/lib/core/src/main/scala/io/iohk/atala/connect/core/repository/ConnectionRepositoryInMemory.scala b/connect/lib/core/src/main/scala/io/iohk/atala/connect/core/repository/ConnectionRepositoryInMemory.scala index 16fbd28598..f4f2f57142 100644 --- a/connect/lib/core/src/main/scala/io/iohk/atala/connect/core/repository/ConnectionRepositoryInMemory.scala +++ b/connect/lib/core/src/main/scala/io/iohk/atala/connect/core/repository/ConnectionRepositoryInMemory.scala @@ -2,7 +2,6 @@ package io.iohk.atala.connect.core.repository import io.iohk.atala.connect.core.model.ConnectionRecord import io.iohk.atala.connect.core.model.ConnectionRecord.ProtocolState -import io.iohk.atala.connect.core.model.error.ConnectionRepositoryError.* import io.iohk.atala.mercury.protocol.connection.ConnectionRequest import io.iohk.atala.mercury.protocol.connection.ConnectionResponse import io.iohk.atala.shared.models.WalletAccessContext diff --git a/connect/lib/core/src/main/scala/io/iohk/atala/connect/core/service/ConnectionServiceImpl.scala b/connect/lib/core/src/main/scala/io/iohk/atala/connect/core/service/ConnectionServiceImpl.scala index 856fb27dc8..d4132d7d65 100644 --- a/connect/lib/core/src/main/scala/io/iohk/atala/connect/core/service/ConnectionServiceImpl.scala +++ b/connect/lib/core/src/main/scala/io/iohk/atala/connect/core/service/ConnectionServiceImpl.scala @@ -146,7 +146,7 @@ private class ConnectionServiceImpl( ) maybeRecord <- connectionRepository .findById(record.id) - record <- ZIO.getOrFailWith(new RecordIdNotFound(recordId))(maybeRecord) + record <- ZIO.getOrFailWith(RecordIdNotFound(recordId))(maybeRecord) } yield record override def markConnectionRequestSent( diff --git a/connect/lib/core/src/test/scala/io/iohk/atala/connect/core/repository/ConnectionRepositorySpecSuite.scala b/connect/lib/core/src/test/scala/io/iohk/atala/connect/core/repository/ConnectionRepositorySpecSuite.scala index ff1fa60dd7..b7ecd2834f 100644 --- a/connect/lib/core/src/test/scala/io/iohk/atala/connect/core/repository/ConnectionRepositorySpecSuite.scala +++ b/connect/lib/core/src/test/scala/io/iohk/atala/connect/core/repository/ConnectionRepositorySpecSuite.scala @@ -2,7 +2,6 @@ package io.iohk.atala.connect.core.repository import io.iohk.atala.connect.core.model.ConnectionRecord import io.iohk.atala.connect.core.model.ConnectionRecord.* -import io.iohk.atala.connect.core.model.error.ConnectionRepositoryError.* import io.iohk.atala.mercury.model.DidId import io.iohk.atala.mercury.protocol.connection.{ConnectionRequest, ConnectionResponse} import io.iohk.atala.mercury.protocol.invitation.v2.Invitation From a3b7e7193b8ef02df80983b81220b373056016ea Mon Sep 17 00:00:00 2001 From: Benjamin Voiturier Date: Thu, 28 Mar 2024 14:20:53 +0100 Subject: [PATCH 2/8] docs(adr): upgrade ADR based on implementation experience Signed-off-by: Benjamin Voiturier --- ...se-zio-failures-and-defects-effectively.md | 324 ++++++++++++++++-- 1 file changed, 302 insertions(+), 22 deletions(-) diff --git a/docs/decisions/20240116-use-zio-failures-and-defects-effectively.md b/docs/decisions/20240116-use-zio-failures-and-defects-effectively.md index be56ad32d8..632622b1d1 100644 --- a/docs/decisions/20240116-use-zio-failures-and-defects-effectively.md +++ b/docs/decisions/20240116-use-zio-failures-and-defects-effectively.md @@ -108,7 +108,7 @@ commitment to enhancing the reliability, scalability, and maintainability of our While we acknowledge this option requires more refactoring, its long-term benefits in terms of code quality, developer efficiency, and robust error management outweigh the associated refactoring efforts. -## Option 2 - Implementation Rules and Principles +## Option 2 - General Implementation Rules and Principles ### Case 1: When designing a component or service @@ -122,12 +122,12 @@ This segregation should be done according to the principles outlined in the [ZIO Types of Errors](https://zio.dev/reference/error-management/types) documentation section. That is, carefully distinguishing between: -- **ZIO Failures** are: +- **ZIO Failures** - The expected/recoverable errors (i.e. domain-specific errors). - Declared in the Error channel of the effect => ZIO[R, E, A]. - Supposed to be handled by the caller to prevent call stack propagation. -- **ZIO Defects** are: +- **ZIO Defects** - The unexpected/unrecoverable errors. - Not represented in the ZIO effect. - We do NOT expect the caller to handle them. @@ -144,8 +144,6 @@ _Implementation tips:_ - **Use a sealed trait or abstract class** to represent the hierarchy of ZIO Failures, allowing for a well-defined set of error possibilities. -- **Evaluate the solution during the implementation phase to make the sealed trait extends Throwable** to enable usage of the **ZIO#orDie** and **ZIO#refineOrDie** methods, - allowing the caller to convert ZIO failures to defects conveniently. - **Define specific error cases within the companion object** of the sealed trait. This practice prevents potential conflicts when importing errors with common names (e.g. RecordNotFoundError), allowing users to prefix them with the name of the parent sealed trait for better code clarity. @@ -153,7 +151,7 @@ _Implementation tips:_ _Example:_ ```scala -sealed trait DomainError extends Throwable +sealed trait DomainError object DomainError { final case class BusinessLogicError(message: String) extends DomainError @@ -199,24 +197,16 @@ As a user of an existing component or service, you should exclusively catch fail effectively handle. Any unhandled failures should be transformed into defects and propagated through the call stack. You should not expect callers of your component to handle lower-level failures that you do not handle. -The conversion of a ZIO failure into a ZIO defect can be achieved using **ZIO#orDie** and **ZIO#refineOrDie**. - #### Use Failure Wrappers To Prevent Failures Leakage From Lower Layers -When invoking lower-level components, refrain from directly exposing their failure types in your component or service -interface. - -Using failures wrappers serves to encapsulate and abstract failure types originating from lower-level components or -services. It prevents the direct exposure of these lower-level failure types in the interface of higher-level components -or services, thus enhancing loose coupling and safeguarding against leakage of underlying implementation details to the -caller. +When invoking lower-level components, do not directly expose their failure types in your component interface. Use +wrappers to encapsulate and abstract failure types originating from lower-level components, thus enhancing +loose coupling and safeguarding against leakage of underlying implementation details to the caller. -While using failure wrappers to shield callers from lower-level failures is beneficial for abstraction and loose -coupling, **it should not be the default failure-handling strategy**. Lower-level failures should primarily be managed -at the component implementation level ensuring that the component handles and appropriately resolves and recovers them. +Using failure wrappers and propagate them **should not be the default strategy**. Lower-level failures should primarily +be managed at your component implementation level ensuring that it handles and appropriately recovers them. -Unhandled failures within the component's boundaries should preferably be transformed into defects rather than -propagated upward. +Unhandled failures within the component's boundaries should preferably be transformed into defects. #### Do not reflexively log errors @@ -231,6 +221,296 @@ Management Best Practices documentation. #### Adopt The “Let it Crash” Principle For ZIO Defects -Adopt the “Let it Crash” principle for ZIO defects. Let them bubble up the call stack and crash the current ZIO fibre. +Adopt the “Let it Crash” principle for ZIO defects. Let them bubble up the call stack and crash the current ZIO fiber. They will be handled/recovered at a higher level or logged appropriately “at the end of the world” by the uppermost -layer. \ No newline at end of file +layer. + +## Option 2 - Practical Implementation + +### Repository Layer + +#### Try using defects only (`UIO` or `URIO`) + +The repository layer leverages Doobie, +which [natively relies on unchecked exceptions](https://tpolecat.github.io/doobie/docs/09-Error-Handling.html#about-exceptions). +Doobie will report any database error as a subclass of `Throwable`, and its specific type will be directly +linked to the underlying database implementation (i.e. PostgreSQL). This means there is no deterministic way to recover +from an SQL execution error in a database agnostic way. + +A good approach is to use ZIO Defects to report repository errors, declaring all repository methods as `URIO` +or `UIO`([example](https://github.com/hyperledger-labs/open-enterprise-agent/blob/main/connect/lib/core/src/main/scala/io/iohk/atala/connect/core/repository/ConnectionRepository.scala)). +Conversely, declaring them as `Task` assumes that the caller (i.e. service) is able to properly handle and +recover from the low-level and database specific exceptions exposed in the error channel, which is a fallacy. + +```scala +trait ConnectionRepository { + def findAll: URIO[WalletAccessContext, Seq[ConnectionRecord]] +} +``` + +Converting a ZIO `Task` to ZIO `UIO` can easily be done +using `ZIO#orDie`([example](https://github.com/hyperledger-labs/open-enterprise-agent/blob/eb898e068f768507d6979a5d9bab35ef7ad4a045/connect/lib/sql-doobie/src/main/scala/io/iohk/atala/connect/sql/repository/JdbcConnectionRepository.scala#L114)). + +```scala +class JdbcConnectionRepository(xa: Transactor[ContextAwareTask], xb: Transactor[Task]) extends ConnectionRepository { + override def findAll: URIO[WalletAccessContext, Seq[ConnectionRecord]] = { + val cxnIO = + sql""" + | SELECT + | id, + | created_at, + | ... + | FROM public.connection_records + | ORDER BY created_at + """.stripMargin + .query[ConnectionRecord] + .to[Seq] + + cxnIO + .transactWallet(xa) + .orDie + } +} +``` + +For those cases where one has to generate a defect, a common way to do this is by using the following ZIO +construct ([example](https://github.com/hyperledger-labs/open-enterprise-agent/blob/eb898e068f768507d6979a5d9bab35ef7ad4a045/connect/lib/sql-doobie/src/main/scala/io/iohk/atala/connect/sql/repository/JdbcConnectionRepository.scala#L212)): + +```scala +class JdbcConnectionRepository(xa: Transactor[ContextAwareTask], xb: Transactor[Task]) extends ConnectionRepository { + override def getById(recordId: UUID): URIO[WalletAccessContext, ConnectionRecord] = + for { + maybeRecord <- findById(recordId) + record <- ZIO.fromOption(maybeRecord).orDieWith(_ => RuntimeException(s"Record not found: $recordId")) + } yield record +} +``` + +#### Apply the `get` vs `find` pattern + +Follow the `get` and `find` best practice in the repository interface for read operations: + +- `getXxx()` returns the requested record or throws an unexpected exception/defect when not + found ([example](https://github.com/hyperledger-labs/open-enterprise-agent/blob/eb898e068f768507d6979a5d9bab35ef7ad4a045/connect/lib/core/src/main/scala/io/iohk/atala/connect/core/repository/ConnectionRepository.scala#L36)). +- `findXxx()` returns an `Option` with or without the request record. This allows the caller service to handle + the `found` + and `not-found` cases and report appropriately to the end + user ([example](https://github.com/hyperledger-labs/open-enterprise-agent/blob/eb898e068f768507d6979a5d9bab35ef7ad4a045/connect/lib/core/src/main/scala/io/iohk/atala/connect/core/repository/ConnectionRepository.scala#L32)). + +```scala +trait ConnectionRepository { + def findById(recordId: UUID): URIO[WalletAccessContext, Option[ConnectionRecord]] + + def getById(recordId: UUID): URIO[WalletAccessContext, ConnectionRecord] +} +``` + +#### Do not return the affected row count + +The `create`, `update` or `delete` repository methods should not return an `Int` indicating the number of rows affected +by the operation but either return `Unit` when successful or throw an exception/defect when the row count is not what is +expected, like e.g. an update operation resulting in a `0` affected row +count ([example](https://github.com/hyperledger-labs/open-enterprise-agent/blob/eb898e068f768507d6979a5d9bab35ef7ad4a045/connect/lib/sql-doobie/src/main/scala/io/iohk/atala/connect/sql/repository/JdbcConnectionRepository.scala#L85)). + +```scala +class JdbcConnectionRepository(xa: Transactor[ContextAwareTask], xb: Transactor[Task]) extends ConnectionRepository { + override def create(record: ConnectionRecord): URIO[WalletAccessContext, Unit] = { + val cxnIO = + sql""" + | INSERT INTO public.connection_records( + | id, + | created_at, + | ... + | ) values ( + | ${record.id}, + | ${record.createdAt}, + | ... + | ) + """.stripMargin.update + + cxnIO.run + .transactWallet(xa) + .ensureOneAffectedRowOrDie + } +} + +extension[Int](ma: RIO[WalletAccessContext, Int]) { + def ensureOneAffectedRowOrDie: URIO[WalletAccessContext, Unit] = ma.flatMap { + case 1 => ZIO.unit + case count => ZIO.fail(RuntimeException(s"Unexpected affected row count: $count")) + }.orDie +} +``` + +#### Do not reflexively log errors + +The upper layer will automatically do so in an appropriate and consistent way using Tapir interceptor customisation. + +### Service Layer + +#### Reporting `404 Not Found` to user + +How can a service appropriately report a `404 Not Found` to a user that e.g. tries to update a record +that does not exist in the database? Following the above rules, the `update` method will throw a defect that will be +caught at the upper level and return a generic `500 Internal Server Error` to the user. + +For those cases where a specific error like `404` should be returned, it is up to the service to first call `find()` +before `update()` and construct a `NotFound` failure, propagated through error channel, if it gives +a `None` ([example](https://github.com/hyperledger-labs/open-enterprise-agent/blob/eb898e068f768507d6979a5d9bab35ef7ad4a045/connect/lib/core/src/main/scala/io/iohk/atala/connect/core/service/ConnectionServiceImpl.scala#L149)). + +Relying on the service layer to implement it will guarantee consistent behaviour regardless of the underlying database +type (could be different RDMS flavour, No-SQL, etc.). + +```scala +class ConnectionServiceImpl() extends ConnectionService { + override def markConnectionRequestSent(recordId: UUID): + ZIO[WalletAccessContext, RecordIdNotFound | InvalidStateForOperation, ConnectionRecord] = + for { + maybeRecord <- connectionRepository.findById(recordId) + record <- ZIO.fromOption(maybeRecord).mapError(_ => RecordIdNotFound(recordId)) + updatedRecord <- updateConnectionProtocolState( + recordId, + ProtocolState.ConnectionRequestPending, + ProtocolState.ConnectionRequestSent + ) + } yield updatedRecord +} +``` + +#### Do not type unexpected errors + +Defects from lower layers (typically repository) should not be wrapped in a failure, and error case class declarations +like [this](https://github.com/hyperledger-labs/open-enterprise-agent/blob/b579fd86ab96db711425f511154e74be75583896/connect/lib/core/src/main/scala/io/iohk/atala/connect/core/model/error/ConnectionServiceError.scala#L8) +should be prohibited. + +Also, considering that failures are expected errors from which user can potentially recover, error case class +like `UnexpectedError` should be +prohibited ([example](https://github.com/hyperledger-labs/open-enterprise-agent/blob/b579fd86ab96db711425f511154e74be75583896/connect/lib/core/src/main/scala/io/iohk/atala/connect/core/model/error/ConnectionServiceError.scala#L12)). + +#### Extend the common `Failure` trait + +Make sure all service errors extend the shared +trait [`io.iohk.atala.shared.models.Failure`](https://github.com/hyperledger-labs/open-enterprise-agent/blob/main/shared/src/main/scala/io/iohk/atala/shared/models/Failure.scala). +This allows handling "at the end of the world“ to be done in a consistent and in generic way. + +Create an exhaustive and meaningful list of service errors and make sure the value of the `userFacingMessage` attribute +is chosen wisely! It will be presented "as is" to the user and should not contain any sensitive +data ([example](https://github.com/hyperledger-labs/open-enterprise-agent/blob/eb898e068f768507d6979a5d9bab35ef7ad4a045/connect/lib/core/src/main/scala/io/iohk/atala/connect/core/model/error/ConnectionServiceError.scala#L14)). + +```scala +trait Failure { + val statusCode: StatusCode + val userFacingMessage: String +} + +sealed trait ConnectionServiceError( + val statusCode: StatusCode, + val userFacingMessage: String + ) extends Failure + +object ConnectionServiceError { + final case class InvitationAlreadyReceived(invitationId: String) + extends ConnectionServiceError( + StatusCode.BadRequest, + s"The provided invitation has already been used: invitationId=$invitationId" + ) +} +``` + +#### User Input Validation + +User input validation (business invariants) should primarily be enforced at the service layer and implemented using the +[ZIO Prelude framework](https://zio.dev/zio-prelude/functional-data-types/validation). + +While it may also be partially implemented at the REST entry point level via OpenAPI specification, it is crucial to +enforce validation at the service level as well. This ensures consistency and reliability across all interfaces that may +call our services, recognising that REST may not be the sole interface interacting with our services. + +```scala +class ConnectionServiceImpl() extends ConnectionService { + def validateInputs( + label: Option[String], + goalCode: Option[String], + goal: Option[String] + ): IO[UserInputValidationError, Unit] = { + val validation = Validation + .validate( + ValidationUtils.validateLengthOptional("label", label, 0, 255), + ValidationUtils.validateLengthOptional("goalCode", goalCode, 0, 255), + ValidationUtils.validateLengthOptional("goal", goal, 0, 255) + ) + .unit + ZIO.fromEither(validation.toEither).mapError(UserInputValidationError.apply) + } +} +``` + +Validation errors should be modelled using a dedicated error case class and, when possible, provide validation failure +details. One could use a construct like the following: + +```scala +sealed trait ConnectionServiceError( + val statusCode: StatusCode, + val userFacingMessage: String + ) extends Failure + +object ConnectionServiceError { + final case class UserInputValidationError(errors: NonEmptyChunk[String]) + extends ConnectionServiceError( + StatusCode.BadRequest, + s"The provided input failed validation: errors=${errors.mkString("[", "], [", "]")}" + ) +} +``` + +#### Use Scala 3 Union Types + +Use Scala 3 union-types declaration in the effect’s error channel to clearly notify the caller of potential +failures ([example](https://github.com/hyperledger-labs/open-enterprise-agent/blob/eb898e068f768507d6979a5d9bab35ef7ad4a045/connect/lib/core/src/main/scala/io/iohk/atala/connect/core/service/ConnectionServiceImpl.scala#L178)) + +````scala +class ConnectionServiceImpl() extends ConnectionService { + + override def receiveConnectionRequest(request: ConnectionRequest, expirationTime: Option[Duration] = None): + ZIO[WalletAccessContext, ThreadIdNotFound | InvalidStateForOperation | InvitationExpired, ConnectionRecord] = ??? + +} +```` + +#### Do not reflexively log errors + +The upper layer will automatically do so in an appropriate and consistent way using Tapir interceptor customisation. + +### Controller Layer + +#### Reporting RFC-9457 Error Response + +All declared Tapir endpoints must +use [`io.iohk.atala.api.http.ErrorResponse`](https://github.com/hyperledger-labs/open-enterprise-agent/blob/main/prism-agent/service/server/src/main/scala/io/iohk/atala/api/http/ErrorResponse.scala) +as their output error type ([example](https://github.com/hyperledger-labs/open-enterprise-agent/blob/eb898e068f768507d6979a5d9bab35ef7ad4a045/prism-agent/service/server/src/main/scala/io/iohk/atala/connect/controller/ConnectionEndpoints.scala#L45)) +This type ensures that the response returned to the user complies with +the [RFC-9457 Problem Details for HTTP APIs](https://www.rfc-editor.org/rfc/rfc9457.html). + +```scala +object ConnectionEndpoints { + + val createConnection: Endpoint[ + (ApiKeyCredentials, JwtCredentials), + (RequestContext, CreateConnectionRequest), + ErrorResponse, + Connection, + Any + ] = ??? + +} +``` + +#### Use "Failure => ErrorResponse" Implicit Conversion + +If all the underlying services used by a controller comply with the above rules, then the only error type that could +propagate through the effect’s error channel is the parent [`io.iohk.atala.shared.models.Failure`](https://github.com/hyperledger-labs/open-enterprise-agent/blob/main/shared/src/main/scala/io/iohk/atala/shared/models/Failure.scala) type and its conversion +to the ErrorResponse type is done automatically via [Scala implicit conversion](https://github.com/hyperledger-labs/open-enterprise-agent/blob/eb898e068f768507d6979a5d9bab35ef7ad4a045/prism-agent/service/server/src/main/scala/io/iohk/atala/api/http/ErrorResponse.scala#L44). + +#### Do not reflexively log errors + +The upper layer will automatically do so in an appropriate and consistent way using Tapir interceptor customisation. \ No newline at end of file From cc5105515e2098de6e446071afde157f5dcd39a6 Mon Sep 17 00:00:00 2001 From: bvoiturier Date: Fri, 29 Mar 2024 09:56:56 +0100 Subject: [PATCH 3/8] Update docs/decisions/20240116-use-zio-failures-and-defects-effectively.md Co-authored-by: Pete Vielhaber Signed-off-by: bvoiturier --- .../20240116-use-zio-failures-and-defects-effectively.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/decisions/20240116-use-zio-failures-and-defects-effectively.md b/docs/decisions/20240116-use-zio-failures-and-defects-effectively.md index 632622b1d1..d01462b234 100644 --- a/docs/decisions/20240116-use-zio-failures-and-defects-effectively.md +++ b/docs/decisions/20240116-use-zio-failures-and-defects-effectively.md @@ -204,7 +204,7 @@ wrappers to encapsulate and abstract failure types originating from lower-level loose coupling and safeguarding against leakage of underlying implementation details to the caller. Using failure wrappers and propagate them **should not be the default strategy**. Lower-level failures should primarily -be managed at your component implementation level ensuring that it handles and appropriately recovers them. +be managed at your component implementation level, ensuring that it appropriately handles and recovers them. Unhandled failures within the component's boundaries should preferably be transformed into defects. From 236c5fbebc8c074cd4c32cf0990e8c901544b09e Mon Sep 17 00:00:00 2001 From: bvoiturier Date: Fri, 29 Mar 2024 09:58:26 +0100 Subject: [PATCH 4/8] Update docs/decisions/20240116-use-zio-failures-and-defects-effectively.md Co-authored-by: Pete Vielhaber Signed-off-by: bvoiturier --- .../20240116-use-zio-failures-and-defects-effectively.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/decisions/20240116-use-zio-failures-and-defects-effectively.md b/docs/decisions/20240116-use-zio-failures-and-defects-effectively.md index d01462b234..34277f06ab 100644 --- a/docs/decisions/20240116-use-zio-failures-and-defects-effectively.md +++ b/docs/decisions/20240116-use-zio-failures-and-defects-effectively.md @@ -203,7 +203,7 @@ When invoking lower-level components, do not directly expose their failure types wrappers to encapsulate and abstract failure types originating from lower-level components, thus enhancing loose coupling and safeguarding against leakage of underlying implementation details to the caller. -Using failure wrappers and propagate them **should not be the default strategy**. Lower-level failures should primarily +Using failure wrappers and propagating them **should not be the default strategy**. Lower-level failures should primarily be managed at your component implementation level, ensuring that it appropriately handles and recovers them. Unhandled failures within the component's boundaries should preferably be transformed into defects. From 17fa167165e46186e0dc39c437539231d0477008 Mon Sep 17 00:00:00 2001 From: bvoiturier Date: Fri, 29 Mar 2024 09:58:50 +0100 Subject: [PATCH 5/8] Update docs/decisions/20240116-use-zio-failures-and-defects-effectively.md Co-authored-by: Pete Vielhaber Signed-off-by: bvoiturier --- .../20240116-use-zio-failures-and-defects-effectively.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/decisions/20240116-use-zio-failures-and-defects-effectively.md b/docs/decisions/20240116-use-zio-failures-and-defects-effectively.md index 34277f06ab..32273a5751 100644 --- a/docs/decisions/20240116-use-zio-failures-and-defects-effectively.md +++ b/docs/decisions/20240116-use-zio-failures-and-defects-effectively.md @@ -239,7 +239,7 @@ from an SQL execution error in a database agnostic way. A good approach is to use ZIO Defects to report repository errors, declaring all repository methods as `URIO` or `UIO`([example](https://github.com/hyperledger-labs/open-enterprise-agent/blob/main/connect/lib/core/src/main/scala/io/iohk/atala/connect/core/repository/ConnectionRepository.scala)). -Conversely, declaring them as `Task` assumes that the caller (i.e. service) is able to properly handle and +Conversely, declaring them as `Task` assumes that the caller (i.e. service) can properly handle and recover from the low-level and database specific exceptions exposed in the error channel, which is a fallacy. ```scala From 10cd7a2b1f61f58ad933c7826e36f77874325931 Mon Sep 17 00:00:00 2001 From: bvoiturier Date: Fri, 29 Mar 2024 11:24:23 +0100 Subject: [PATCH 6/8] Apply grammatical improvements suggested in code review Co-authored-by: Pete Vielhaber Signed-off-by: bvoiturier --- ...se-zio-failures-and-defects-effectively.md | 44 +++++++++---------- 1 file changed, 22 insertions(+), 22 deletions(-) diff --git a/docs/decisions/20240116-use-zio-failures-and-defects-effectively.md b/docs/decisions/20240116-use-zio-failures-and-defects-effectively.md index 32273a5751..1d4d080667 100644 --- a/docs/decisions/20240116-use-zio-failures-and-defects-effectively.md +++ b/docs/decisions/20240116-use-zio-failures-and-defects-effectively.md @@ -199,7 +199,7 @@ should not expect callers of your component to handle lower-level failures that #### Use Failure Wrappers To Prevent Failures Leakage From Lower Layers -When invoking lower-level components, do not directly expose their failure types in your component interface. Use +Do not directly expose their failure types in your component interface when invoking lower-level components. Use wrappers to encapsulate and abstract failure types originating from lower-level components, thus enhancing loose coupling and safeguarding against leakage of underlying implementation details to the caller. @@ -234,13 +234,13 @@ layer. The repository layer leverages Doobie, which [natively relies on unchecked exceptions](https://tpolecat.github.io/doobie/docs/09-Error-Handling.html#about-exceptions). Doobie will report any database error as a subclass of `Throwable`, and its specific type will be directly -linked to the underlying database implementation (i.e. PostgreSQL). This means there is no deterministic way to recover -from an SQL execution error in a database agnostic way. +linked to the underlying database implementation (i.e. PostgreSQL). Handling it this way means there is no deterministic way to recover +from an SQL execution error in a database-agnostic way. A good approach is to use ZIO Defects to report repository errors, declaring all repository methods as `URIO` or `UIO`([example](https://github.com/hyperledger-labs/open-enterprise-agent/blob/main/connect/lib/core/src/main/scala/io/iohk/atala/connect/core/repository/ConnectionRepository.scala)). Conversely, declaring them as `Task` assumes that the caller (i.e. service) can properly handle and -recover from the low-level and database specific exceptions exposed in the error channel, which is a fallacy. +recover from the low-level and database-specific exceptions exposed in the error channel, which is a fallacy. ```scala trait ConnectionRepository { @@ -288,11 +288,11 @@ class JdbcConnectionRepository(xa: Transactor[ContextAwareTask], xb: Transactor[ #### Apply the `get` vs `find` pattern -Follow the `get` and `find` best practice in the repository interface for read operations: +Follow the `get` and `find` best practices in the repository interface for read operations: - `getXxx()` returns the requested record or throws an unexpected exception/defect when not found ([example](https://github.com/hyperledger-labs/open-enterprise-agent/blob/eb898e068f768507d6979a5d9bab35ef7ad4a045/connect/lib/core/src/main/scala/io/iohk/atala/connect/core/repository/ConnectionRepository.scala#L36)). -- `findXxx()` returns an `Option` with or without the request record. This allows the caller service to handle +- `findXxx()` returns an `Option` with or without the request record, which allows the caller service to handle the `found` and `not-found` cases and report appropriately to the end user ([example](https://github.com/hyperledger-labs/open-enterprise-agent/blob/eb898e068f768507d6979a5d9bab35ef7ad4a045/connect/lib/core/src/main/scala/io/iohk/atala/connect/core/repository/ConnectionRepository.scala#L32)). @@ -309,7 +309,7 @@ trait ConnectionRepository { The `create`, `update` or `delete` repository methods should not return an `Int` indicating the number of rows affected by the operation but either return `Unit` when successful or throw an exception/defect when the row count is not what is -expected, like e.g. an update operation resulting in a `0` affected row +expected, like i.e. an update operation resulting in a `0` affected row count ([example](https://github.com/hyperledger-labs/open-enterprise-agent/blob/eb898e068f768507d6979a5d9bab35ef7ad4a045/connect/lib/sql-doobie/src/main/scala/io/iohk/atala/connect/sql/repository/JdbcConnectionRepository.scala#L85)). ```scala @@ -344,22 +344,22 @@ extension[Int](ma: RIO[WalletAccessContext, Int]) { #### Do not reflexively log errors -The upper layer will automatically do so in an appropriate and consistent way using Tapir interceptor customisation. +The upper layer will automatically do so appropriately and consistently using Tapir interceptor customization. ### Service Layer #### Reporting `404 Not Found` to user -How can a service appropriately report a `404 Not Found` to a user that e.g. tries to update a record +How can a service appropriately report a `404 Not Found` to a user that i.e. tries to update a record that does not exist in the database? Following the above rules, the `update` method will throw a defect that will be -caught at the upper level and return a generic `500 Internal Server Error` to the user. +caught at the upper level and returns a generic `500 Internal Server Error` to the user. For those cases where a specific error like `404` should be returned, it is up to the service to first call `find()` -before `update()` and construct a `NotFound` failure, propagated through error channel, if it gives +before `update()` and construct a `NotFound` failure, propagated through the error channel, if it gives a `None` ([example](https://github.com/hyperledger-labs/open-enterprise-agent/blob/eb898e068f768507d6979a5d9bab35ef7ad4a045/connect/lib/core/src/main/scala/io/iohk/atala/connect/core/service/ConnectionServiceImpl.scala#L149)). -Relying on the service layer to implement it will guarantee consistent behaviour regardless of the underlying database -type (could be different RDMS flavour, No-SQL, etc.). +Relying on the service layer to implement it will guarantee consistent behavior regardless of the underlying database +type (could be different RDMS flavor, No-SQL, etc.). ```scala class ConnectionServiceImpl() extends ConnectionService { @@ -379,7 +379,7 @@ class ConnectionServiceImpl() extends ConnectionService { #### Do not type unexpected errors -Defects from lower layers (typically repository) should not be wrapped in a failure, and error case class declarations +Do not wrap defects from lower layers (typically repository) in a failure and error case class declarations like [this](https://github.com/hyperledger-labs/open-enterprise-agent/blob/b579fd86ab96db711425f511154e74be75583896/connect/lib/core/src/main/scala/io/iohk/atala/connect/core/model/error/ConnectionServiceError.scala#L8) should be prohibited. @@ -394,7 +394,7 @@ trait [`io.iohk.atala.shared.models.Failure`](https://github.com/hyperledger-lab This allows handling "at the end of the world“ to be done in a consistent and in generic way. Create an exhaustive and meaningful list of service errors and make sure the value of the `userFacingMessage` attribute -is chosen wisely! It will be presented "as is" to the user and should not contain any sensitive +is chosen wisely! It will present "as is" to the user and should not contain any sensitive data ([example](https://github.com/hyperledger-labs/open-enterprise-agent/blob/eb898e068f768507d6979a5d9bab35ef7ad4a045/connect/lib/core/src/main/scala/io/iohk/atala/connect/core/model/error/ConnectionServiceError.scala#L14)). ```scala @@ -419,12 +419,12 @@ object ConnectionServiceError { #### User Input Validation -User input validation (business invariants) should primarily be enforced at the service layer and implemented using the +Enforcing user input validation (business invariants) should primarily sit at the service layer and be implemented using the [ZIO Prelude framework](https://zio.dev/zio-prelude/functional-data-types/validation). While it may also be partially implemented at the REST entry point level via OpenAPI specification, it is crucial to -enforce validation at the service level as well. This ensures consistency and reliability across all interfaces that may -call our services, recognising that REST may not be the sole interface interacting with our services. +enforce validation at the service level as well. This implementation ensures consistency and reliability across all interfaces that may +call our services, recognizing that REST may not be the sole interface interacting with our services. ```scala class ConnectionServiceImpl() extends ConnectionService { @@ -445,7 +445,7 @@ class ConnectionServiceImpl() extends ConnectionService { } ``` -Validation errors should be modelled using a dedicated error case class and, when possible, provide validation failure +Modeling validation errors should use a dedicated error case class and, when possible, provide validation failure details. One could use a construct like the following: ```scala @@ -465,7 +465,7 @@ object ConnectionServiceError { #### Use Scala 3 Union Types -Use Scala 3 union-types declaration in the effect’s error channel to clearly notify the caller of potential +Use Scala 3 union-types declaration in the effect’s error channel to notify the caller of potential failures ([example](https://github.com/hyperledger-labs/open-enterprise-agent/blob/eb898e068f768507d6979a5d9bab35ef7ad4a045/connect/lib/core/src/main/scala/io/iohk/atala/connect/core/service/ConnectionServiceImpl.scala#L178)) ````scala @@ -479,7 +479,7 @@ class ConnectionServiceImpl() extends ConnectionService { #### Do not reflexively log errors -The upper layer will automatically do so in an appropriate and consistent way using Tapir interceptor customisation. +The upper layer will automatically do so appropriately and consistently using Tapir interceptor customization. ### Controller Layer @@ -513,4 +513,4 @@ to the ErrorResponse type is done automatically via [Scala implicit conversion]( #### Do not reflexively log errors -The upper layer will automatically do so in an appropriate and consistent way using Tapir interceptor customisation. \ No newline at end of file +The upper layer will automatically do so appropriately and consistently using Tapir interceptor customization. \ No newline at end of file From 7698b507c13eb9ab27ba72a7105aa2786727ff2a Mon Sep 17 00:00:00 2001 From: Benjamin Voiturier Date: Fri, 29 Mar 2024 13:48:30 +0100 Subject: [PATCH 7/8] docs(adr): update ADR status and date Signed-off-by: Benjamin Voiturier --- .../20240116-use-zio-failures-and-defects-effectively.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/decisions/20240116-use-zio-failures-and-defects-effectively.md b/docs/decisions/20240116-use-zio-failures-and-defects-effectively.md index 1d4d080667..33eca44be3 100644 --- a/docs/decisions/20240116-use-zio-failures-and-defects-effectively.md +++ b/docs/decisions/20240116-use-zio-failures-and-defects-effectively.md @@ -1,8 +1,8 @@ # Use ZIO Failures and Defects Effectively -- Status: draft +- Status: accepted - Deciders: Fabio Pinheiro, Shailesh Patil, Pat Losoponkul, Yurii Shynbuiev, David Poltorak, Benjamin Voiturier -- Date: 2024-01-16 +- Date: 2024-03-29 - Tags: error-handling, zio ## Context and Problem Statement From c787284b4b5f43c58bc08afdf06103aceb6eea03 Mon Sep 17 00:00:00 2001 From: Benjamin Voiturier Date: Fri, 29 Mar 2024 18:07:28 +0100 Subject: [PATCH 8/8] docs(adr): grammar fixes Signed-off-by: Benjamin Voiturier --- ...se-zio-failures-and-defects-effectively.md | 34 ++++++++++++------- 1 file changed, 21 insertions(+), 13 deletions(-) diff --git a/docs/decisions/20240116-use-zio-failures-and-defects-effectively.md b/docs/decisions/20240116-use-zio-failures-and-defects-effectively.md index 33eca44be3..36fdf0cc0e 100644 --- a/docs/decisions/20240116-use-zio-failures-and-defects-effectively.md +++ b/docs/decisions/20240116-use-zio-failures-and-defects-effectively.md @@ -203,10 +203,11 @@ Do not directly expose their failure types in your component interface when invo wrappers to encapsulate and abstract failure types originating from lower-level components, thus enhancing loose coupling and safeguarding against leakage of underlying implementation details to the caller. -Using failure wrappers and propagating them **should not be the default strategy**. Lower-level failures should primarily +Using failure wrappers and propagating them **should not be the default strategy**. Lower-level failures should +primarily be managed at your component implementation level, ensuring that it appropriately handles and recovers them. -Unhandled failures within the component's boundaries should preferably be transformed into defects. +Failures not handled within the component's boundaries should preferably be transformed into defects whenever possible. #### Do not reflexively log errors @@ -234,7 +235,8 @@ layer. The repository layer leverages Doobie, which [natively relies on unchecked exceptions](https://tpolecat.github.io/doobie/docs/09-Error-Handling.html#about-exceptions). Doobie will report any database error as a subclass of `Throwable`, and its specific type will be directly -linked to the underlying database implementation (i.e. PostgreSQL). Handling it this way means there is no deterministic way to recover +linked to the underlying database implementation (i.e. PostgreSQL). Handling it this way means there is no deterministic +way to recover from an SQL execution error in a database-agnostic way. A good approach is to use ZIO Defects to report repository errors, declaring all repository methods as `URIO` @@ -383,7 +385,7 @@ Do not wrap defects from lower layers (typically repository) in a failure and er like [this](https://github.com/hyperledger-labs/open-enterprise-agent/blob/b579fd86ab96db711425f511154e74be75583896/connect/lib/core/src/main/scala/io/iohk/atala/connect/core/model/error/ConnectionServiceError.scala#L8) should be prohibited. -Also, considering that failures are expected errors from which user can potentially recover, error case class +Considering that failures are viewed as **expected errors** from which users can potentially recover, error case classes like `UnexpectedError` should be prohibited ([example](https://github.com/hyperledger-labs/open-enterprise-agent/blob/b579fd86ab96db711425f511154e74be75583896/connect/lib/core/src/main/scala/io/iohk/atala/connect/core/model/error/ConnectionServiceError.scala#L12)). @@ -419,12 +421,14 @@ object ConnectionServiceError { #### User Input Validation -Enforcing user input validation (business invariants) should primarily sit at the service layer and be implemented using the +Enforcing user input validation (business invariants) should primarily sit at the service layer and be implemented using +the [ZIO Prelude framework](https://zio.dev/zio-prelude/functional-data-types/validation). -While it may also be partially implemented at the REST entry point level via OpenAPI specification, it is crucial to -enforce validation at the service level as well. This implementation ensures consistency and reliability across all interfaces that may -call our services, recognizing that REST may not be the sole interface interacting with our services. +While partially implementing user input validation at the REST entry point level via OpenAPI specification, it is +crucial to enforce validation at the service level as well. This implementation ensures consistency and reliability +across all interfaces that may call our services, recognizing that REST may not be the sole interface interacting with +our services. ```scala class ConnectionServiceImpl() extends ConnectionService { @@ -487,13 +491,14 @@ The upper layer will automatically do so appropriately and consistently using Ta All declared Tapir endpoints must use [`io.iohk.atala.api.http.ErrorResponse`](https://github.com/hyperledger-labs/open-enterprise-agent/blob/main/prism-agent/service/server/src/main/scala/io/iohk/atala/api/http/ErrorResponse.scala) -as their output error type ([example](https://github.com/hyperledger-labs/open-enterprise-agent/blob/eb898e068f768507d6979a5d9bab35ef7ad4a045/prism-agent/service/server/src/main/scala/io/iohk/atala/connect/controller/ConnectionEndpoints.scala#L45)) +as their output error +type ([example](https://github.com/hyperledger-labs/open-enterprise-agent/blob/eb898e068f768507d6979a5d9bab35ef7ad4a045/prism-agent/service/server/src/main/scala/io/iohk/atala/connect/controller/ConnectionEndpoints.scala#L45)) This type ensures that the response returned to the user complies with the [RFC-9457 Problem Details for HTTP APIs](https://www.rfc-editor.org/rfc/rfc9457.html). ```scala object ConnectionEndpoints { - + val createConnection: Endpoint[ (ApiKeyCredentials, JwtCredentials), (RequestContext, CreateConnectionRequest), @@ -501,15 +506,18 @@ object ConnectionEndpoints { Connection, Any ] = ??? - + } ``` #### Use "Failure => ErrorResponse" Implicit Conversion If all the underlying services used by a controller comply with the above rules, then the only error type that could -propagate through the effect’s error channel is the parent [`io.iohk.atala.shared.models.Failure`](https://github.com/hyperledger-labs/open-enterprise-agent/blob/main/shared/src/main/scala/io/iohk/atala/shared/models/Failure.scala) type and its conversion -to the ErrorResponse type is done automatically via [Scala implicit conversion](https://github.com/hyperledger-labs/open-enterprise-agent/blob/eb898e068f768507d6979a5d9bab35ef7ad4a045/prism-agent/service/server/src/main/scala/io/iohk/atala/api/http/ErrorResponse.scala#L44). +propagate through the effect’s error channel is the +parent [`io.iohk.atala.shared.models.Failure`](https://github.com/hyperledger-labs/open-enterprise-agent/blob/main/shared/src/main/scala/io/iohk/atala/shared/models/Failure.scala) +type and its conversion +to the ErrorResponse type is done automatically +via [Scala implicit conversion](https://github.com/hyperledger-labs/open-enterprise-agent/blob/eb898e068f768507d6979a5d9bab35ef7ad4a045/prism-agent/service/server/src/main/scala/io/iohk/atala/api/http/ErrorResponse.scala#L44). #### Do not reflexively log errors