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

Consider adding a transactEither method #75

Open
AugustNagro opened this issue Dec 3, 2024 · 10 comments
Open

Consider adding a transactEither method #75

AugustNagro opened this issue Dec 3, 2024 · 10 comments

Comments

@AugustNagro
Copy link
Owner

As described in https://softwaremill.com/direct-style-bootzooka-2024-update/#database-access

@Nexus6
Copy link

Nexus6 commented Dec 3, 2024

Yes, this would be nice.
What would the method signature look like? And would I be assured of never seeing an unhandled exception from it? Unless I'm reading too much into this it sounds like I'd be able to get rid of a lot of exception handling code by using it, though obviously I'd still have to handle any Lefts returned.

@AugustNagro
Copy link
Owner Author

I haven't put too much thought into it yet, but something that rolls back the transaction on a Left() like

https://github.com/softwaremill/bootzooka/blob/master/backend/src/main/scala/com/softwaremill/bootzooka/infrastructure/DB.scala#L23

  /** Runs `f` in a transaction. The transaction is commited if the result is a [[Right]], and rolled back otherwise. */
  def transactEither[E, T](f: DbTx ?=> Either[E, T])(using IO): Either[E, T] =
    try com.augustnagro.magnum.transact(dataSource)(Right(f.fold(e => throw LeftException(e), identity)))
    catch case e: LeftException[E] => Left(e.left)

So it would still throw if for example a SqlException occurred.

Would love to have your feedback / thoughts.

@AugustNagro
Copy link
Owner Author

AugustNagro commented Dec 3, 2024

Personally, I like to embrace exceptions with my User-defined / recoverable error types.

My error hierarchy looks like

enum Errors(val msg: String) extends NoStackTrace:
  case UserPermission(allowed: List[String]) extends Errors(s"User does not have valid permission. Allowed permissions: $allowed")
  case InvalidJwt(msg: String) extends Errors(s"Invalid JWT: $msg")
  case Etc extends Errors("another error")

And I still compose / validate with Either (or a type similar to Cats Validated), but then throw if Left.

My webserver then has a root-level exception handler that catches the Errors subclasses, and produces the error message, returning 500 for all others.

@AugustNagro
Copy link
Owner Author

cc @adamw

@adamw
Copy link
Contributor

adamw commented Dec 3, 2024

Ah! Sorry should have looked in the open issues :)

So there's two aspects here:

  1. programatically deciding, if the transaction should commit or rollback. Currently this can be done with exceptions (code block completes == commit, throws == rollback), but it could also be done on a value level (code block returns Commit[T] == commit, Rollback[E] == rollback). You could use a custom Commit/Rollback ADT, or reuse Either. The first one is more readable, but isomorphic to an either, so not sure if it's worth the effort

The downside of transact(Either[E, T]): Either[E, T] is that the return type also specifies if the transaction was committed or not (left - rolled back, right - committed). You could also imagine a `transact(RollbackOrCommit[E, T]): RolledBackOrCommited[E, T], which gives you the full information in a very clear way, but that's inventing way too many new types ;).

  1. exception handling. If I understand @AugustNagro 's approach correctly, it's also a distinction that I like to make (following Rust's philosophy) of recoverable errors & panics. Anything recoverable becomes a value-level Either, anything not - an untyped exception throw. The fine print here is that what's a panic at one level might be recoverable at another, so an exception thrown in a thread processing a request becomes recoverable when it reaches the http server (which translates it into a 500). Though I'd think of this as a feature.

Now, you might imagine having a transactSafe method, which would guarantee that no exception is thrown; it would have to return sth like Error(Throwable) / Commited(T) / RolledBack(E), making it kind of a three-way either. But since it would be a simple try-catch wrapper on top of transact (sql drivers can always throw), probably no much sense in including this OOTB.

@adamw
Copy link
Contributor

adamw commented Dec 3, 2024

@lbialy you might also be interested :)

@lbialy
Copy link
Contributor

lbialy commented Dec 3, 2024

oh niiice

@AugustNagro
Copy link
Owner Author

Good argument Adam. I know my philosophy is hard-core direct style, so bear with me;

Anything recoverable becomes a value-level Either, anything not - an untyped exception throw

I find that business validations are not typically recoverable, except at (1) the immediate callsite, or (2) the root exception handler, where they are translated into, for example, HTTP error codes.

Threading Either or Try vertically through the application quickly becomes tiresome, with little benefit if we never recover in the intermediate layers. Either becomes viral like any other monad, which results in more work for service methods to call other service methods. This breaks a programmer's zen flow.

Exceptions let us bypass this issue entirely. While there's a stigma against Exceptions for control flow, extending ControlThrowable nearly eliminates the overhead (https://shipilev.net/blog/2014/exceptional-performance/). Note that I'm not arguing for removing Either entirely, but rather to localize its usage (ie, don't return Either from service methods).

Here's an example of how this would look in Bootzooka. First, Fail would become throwable:

abstract class Fail extends ControlThrowable

Then we remove Either from the signature of service methods like UserService.changePassword;

def changePassword(userId: Id[User], currentPassword: String, newPassword: String)(using DbTx): ApiKey =
  ???
  val user = either {
    validateUserPassword(userId, currentPassword).ok()
    validateNewPassword().ok()
  }.toTry.get
  updateUserPassword(user, newPassword)
  invalidateKeysAndCreateNew(user)

Finally in the controller

  private val changePasswordEndpoint = authedEndpoint.post
    .in(UserPath / "changepassword")
    .in(jsonBody[ChangePassword_IN])
    .out(jsonBody[ChangePassword_OUT])
    .handle { id => data =>
      try
        val apiKeyResult = db.transact(userService.changePassword(id, data.currentPassword, data.newPassword))
        Right(ChangePassword_OUT(apiKey.id.toString))
      catch case f: Fail => Left(f)
    }

However, I understand that this is very much a preference, and I'm not opposed to having a value-based transact method. But this is a fun debate to have.

@AugustNagro
Copy link
Owner Author

SqlExceptions may be transient (connection interruption, etc), and therefore recoverable. However, it greatly depends on the context so I wouldn't want to punish users by returning an Either or Try.

When consuming external REST APIs it can make a lot of sense to use Try + exponential retries, but your DB should have a reliable connection to your application; therefore I argue that it's better to fail fast on a connection issue.

The user can always retry again in the very rare cases of transiency, meanwhile retries may actually serve to mask an infrastructure issue. One exception is serialization anomalies, which might make sense to retry in the application. However that can be handled explicitly with a library like Ox.

@adamw
Copy link
Contributor

adamw commented Dec 17, 2024

Ha yes, error handling is an eternal and eternally unsolved problem ;). No approach is perfect: checked exceptions (for many reasons), unchecked exceptions (unreliable, "lying" method signatures), Eithers.

But, to provide some counter-points:

  • using Either in the result type is introducing yet another monad, however a monad which is easier to work with: you can pattern match on it (unlike IO), fold locally etc. In a way, it's "direct", vs. other asynchronous monads
  • boundary-break for eithers further helps when using direct-style, and unlike other "direct" macros for IO/ZIO, this works well with lambdas (with a slight footnote, that to be bullet-proof, you need capture checking)
  • invalid password / invalid new username etc. is in some ways a different error than an SQLException. First, it's a user error (4xx in HTTP terms), vs. an infrastructure/server error (5xx). Secondly, you can provide meaningful information back to the user, that is formulate a specific response, vs. just returning an "internal server error" and logging the stack trace. So I think it might make sense to treat them differently. But I agree that errors are rarely recoverable, at the level of an individual request.

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

No branches or pull requests

4 participants