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

Preserve existing errors when using transaction #192

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

endertunc
Copy link

@endertunc endertunc commented Sep 17, 2022

Fixes #issue_number

Problem

I would like to be able to run ZIO effects without losing existing information on error channel.

For example;

enum AppError {
  case UserNotFound(message: String) extends AppErrorScala3
  case InvalidCredentials(message: String) extends AppErrorScala3
}

val login: ZIO[Connection, SQLException | InvalidCredentials | UserNotFound, User] = for {
  maybeUser <- userRepository.findByEmail(email)
  user <- ZIO.getOrFailWith[UserNotFound, User](UserNotFound(s"User with given email [$email] is not found"))(
    maybeUser
  )
  result <- hasherService.check(plainTextPassword, user.hashedPassword)
  _      <- ZIO.fail[InvalidCredentials](InvalidCredentials("Invalid Credentials")).unless(result)
} yield user

// Given above code, if we were to run "login" effect in transaction we will lose existing errors.
val transactionResult: ZIO[Connection, Throwable, User] = ctx.underlying.transaction(login)

// I would like to be able to run "login" effect in transaction and still keep existing errors.
// This PR allows you to do the following
val transactionResult: ZIO[Connection, SQLException | InvalidCredentials | UserNotFound, User] = ctx.underlying.transaction(login)

// In case of missing SQLException in error channel, it should added to error channel along with the existing ones
val zioThatCanFailWithString = ZIO[Connection, String, User] = ???
val zioThatCanFailWithStringTransaction: ZIO[Connection, String | SQLException, User] = ctx.underlying.transaction(zioThatCanFailWithString)

Solution

I changed ZioJdbcUnderlyingContext.transaction

// from
def transaction[R <: Connection, A](f: ZIO[R, Throwable, A]): ZIO[R, Throwable, A]
// to
def transaction[R <: Connection, E, A](f: ZIO[R, E, A]): ZIO[R, E | SQLException, A]

Also updated QuillZioExtPlain.onDataSource and QuillZioExtPlain.implicitDS so that can be run on ZIO[Connection, E, T] rather than ZIO[Connection, Throwable, T]

Notes

I only updated ZioJdbcUnderlyingContext.transaction as part of the initial PoC/Draft. If we are happy with the approach, we need to apply the same approach to ZioJdbcContext.transaction as well.

Checklist

  • Unit test all changes - TODO
  • Update README.md if applicable TODO
  • Add [WIP] to the pull request title if it's work in progress
  • Squash commits that aren't meaningful changes

@getquill/maintainers

@deusaquilus
Copy link
Contributor

This looks good. Could you please change ZioJdbcContext.transaction as well?

@endertunc
Copy link
Author

endertunc commented Mar 29, 2023

Hi @deusaquilus thanks for taking a look. Yes, I will take a look at this again this weekend and implement the remaining pieces. I will ping you again once it's ready to review.

@endertunc endertunc force-pushed the transaction-keeps-existing-errors branch from 36e55d9 to f34ee1f Compare April 2, 2023 22:08
@endertunc endertunc marked this pull request as ready for review April 2, 2023 22:09
@endertunc endertunc changed the title [WIP] preserve existing errors when using transaction Preserve existing errors when using transaction Apr 2, 2023
@endertunc
Copy link
Author

Hi @deusaquilus I think this should be ready to review. Let me know if you have any feedback! :)

import java.sql.SQLException

val zioThatCanFail: ZIO[Connection, String, Nothing] = ZIO.service[Connection] *> ZIO.fail("Custom Error")
"val result: ZIO[DataSource, String | SQLException, List[Person]] = zioThatCanFail.onDataSource" should compile
Copy link
Author

Choose a reason for hiding this comment

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

I wrote the tests in this style (something must compile) because the only thing that changed is the types. Let me know if you have any suggestions!

@endertunc
Copy link
Author

Hi @deusaquilus, did you have chance to look at the changes? Let me know if there is anything I can improve/change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants