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

Introduces attemptSomeSql into ApplicativeErrorOps #2085

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

Conversation

prascuna
Copy link

Introduces an attemptSomeSql catcher to partial match on a specific SQLException.
The use case for it is to match both the stage, but also log the exception itself:

        .attemptSomeSql {
          case e: PSQLException if SqlState(e.getSQLState) == sqlstate.class23.UNIQUE_VIOLATION =>
            logger.error("Database Unique Violation", e)
            DbCreateError.UniqueViolation
          case e: PSQLException if SqlState(e.getSQLState) == sqlstate.class23.CHECK_VIOLATION  =>
            logger.error("Database Check Violation", e)
            DbCreateError.CheckViolationOnCreate
        }

For some reason attemptSomeSql catcher is mentioned in the docs but not implemented.

Also, I've changed the docker-compose mysql image to not rely on the debian version, as that's not available for arm64 devs

@mergify mergify bot added the core label Aug 27, 2024
@prascuna prascuna changed the title Introduces attemptSomeSql into ApplicativeErrorOp Introduces attemptSomeSql into ApplicativeErrorOps Aug 27, 2024
@satorg
Copy link
Contributor

satorg commented Aug 27, 2024

Thank you for the contribution!

Just a note though: since SQLException is a Throwable, it does not make a lot of sense to restrict PartialFunction to that type only. For example, a combinator with the following signature

def attemptYOUNAMEIT[M[_], A, B](ma: M[A])(f: PartialFunction[Throwable, B]): M[Either[B, A]] = ???

would do here just as well:

.attemptYOUNAMEIT {
  case e: PSQLException if SqlState(e.getSQLState) == sqlstate.class23.UNIQUE_VIOLATION =>
    logger.error("Database Unique Violation", e)
    DbCreateError.UniqueViolation
  case e: PSQLException if SqlState(e.getSQLState) == sqlstate.class23.CHECK_VIOLATION  =>
    logger.error("Database Check Violation", e)
    DbCreateError.CheckViolationOnCreate
}

Apparently, the attemptYOUNAMEIT combinator is not restricted to the Doobie/JDBC domain only – it can work with any other exception type.

Therefore, I would suggest to propose that kind of combinator straight to the Cats library so that everyone besides Doobie could benefit from that. Then, Doobie users can get it automatically from Cats.

@jatcwang
Copy link
Collaborator

Thanks for the contribution @prascuna! I believe cat's attemptNarrow does exactly this so probably don't need to be added to Doobie?

Will take the docker related changes though 😄

@prascuna
Copy link
Author

prascuna commented Sep 3, 2024

Hey folks, thank you for your feedback!

@satorg you're right, there's no need to restrict it to just SQLException, I could give a go raising a PR into Cats

I believe cat's attemptNarrow does exactly this so probably don't need to be added to Doobie?

@jatcwang it's not quite the same, as I'm not just interested in narrowing the type, but also the value (namely, the sql state).

If I were to implement it with attemptNarrow without using EitherT would look pretty ugly:

        .attemptNarrow[PSQLException]
        .flatMap[Either[DbCreateError, MyType]] {
          case Left(e)      =>
            e match {
              case e: PSQLException if SqlState(e.getSQLState) == sqlstate.class23.UNIQUE_VIOLATION =>
                logger.error("Database Unique Violation", e)
                (DbCreateError.UniqueViolation: DbCreateError).asLeft[MyType].pure[ConnectionIO]
              case e: PSQLException if SqlState(e.getSQLState) == sqlstate.class23.CHECK_VIOLATION  =>
                logger.error("Database Check Violation", e)
                (DbCreateError.CheckViolationOnCreate: DbCreateError).asLeft[MyType].pure[ConnectionIO]
              case e                                                                                => 
                ApplicativeError[ConnectionIO, Throwable].raiseError(e)
            }
          case Right(value) => value.asRight[DbCreateError].pure[ConnectionIO]
        }

Shall we close this PR then? (and I can raise another one for Docker?)

@jatcwang
Copy link
Collaborator

jatcwang commented Sep 3, 2024

@prascuna I see what you mean. The best I can come up with is

            .attemptT
            .leftSemiflatMap {
              case e: SQLException if ...=> e.pure
              case e => e.raiseError
            }
            .value

which does use EitherT and it isn't obvious what leftSemiflatMap does at first glance (Seems like type inference works well here though).

Do agree with @satorg that this seems useful enough to try to put into cats. Let's close and reopen if cats maintainers doesn't want to add it :)

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

Successfully merging this pull request may close these issues.

4 participants