Defined sealed hierarchy of exceptions

[pablf]

Defined hierarchy of exceptions and changed Throwable to corresponding exception. If new features are added to ZIO JDBC, it is possible that some new Exceptions should be defined, but currently I don't see any other type of exception that could be needed.
/claim #74

[domartynov]

Change: I believe connection timeouts are not reported as SQLException, and I think reporting them via die is a regression in the API as this type of error is of a special interest for consumers as it's a retryable error. Given it's probably driver specific, we need to catch Throwable and use another case class to report it (i.e. we probably cannot use ZSQLException in the return type). Although ideally, i'd like to have the error reported via an explicit error case class.

[pablf]

If I am not mistaken executeLargeUpdate() can throw a SQLTimeoutException, but this should be caught as SQLTimeoutException that is a subclass of SQLException. https://docs.oracle.com/javase/8/docs/api/java/sql/PreparedStatement.html#executeLargeUpdate--

Or do you mean that the driver would report the SQLTimeoutException using a different class?

[domartynov]

yeah, you're right, my bad (i've misinterpreted a few issues reported in our Sentry) 🤦

[domartynov]

Question: what's the objective for the hierarchy of errors?

Given it's meant to be used for ZIO error channel in the library API, i think it should be tailored to assist with the error handing logic of a consumer of the API.

In this context I wonder:

• Do we want to handle DriverNotFound explicitly, should we report it via ZIO.die instead? What about CodecExceptions? Even if at the library API level we'd like to expose it via the error channel, should we allow an easy way to turn all such errors to ZIO.die in application level code?
• Do we expect the error handing at the application level where either connection or query errors are possible? With the new API, do I need to pattern match over both FailedToConnect and ZSQLException in order to handle connection timeout?
• In order to guide consumer should we prefer nested sealed error cases starting at JdbcException and do we want to keep "Exception" in the name (given the previous question)?
• Should we help to distinguish retryable errors, e.g. timeout vs decoding.
• What about common SQL errors, or it's out of the scope of the PR?

I just afraid that in the current form, it makes the API more complicated, while does not improve the consumer experience where it's mostly to have some pattern matching over Throwable to implement additional retry logic, and ZIO.die for other explicitly not handled cases. Even decoding errors seem handled well via ZIO.die as ZIO provides stack traces 🤔

What do you think? @pablf @jdegoes

Update: Now I think the issue is mainly with splitting error cases into ConnectionException and QueryException. Can we just use a flat hierarchy instead, ie:

sealed trait JdbcException extends Exception
final case class ZSQLException(cause: SQLException) extends Exception(cause) with FailedQuery
final case class DecodeException(cause: Throwable) extends Exception(cause) with CodecException
final case class JdbcDecoderError(...) extends IOException(message, cause)   with CodecException
final case class JdbcEncoderError(message: String, cause: Throwable)    extends IOException(message, cause)  with CodecException
final case class DriverNotFound(cause: Throwable, driver: String)  extends Exception(s"Could not found driver: $driver", cause) with JdbcException

[pablf]

I understand that it should clarify to the user what possible errors he must take into account and make easier

• I think that exposing more of the API gives more control to the user to decide how to handle the errors and this makes ZIO JDBC more powerful. But I think that making it easier to ZIO.die everything could be helpful at the application level.
• I would say so because it would mean less interference between JDBC and the user.
• I think that if all errors are direct cases of JdbcException we might lose the advantage of precisely categorizing errors. ConnectionException only has one sublevel. We could do the same with QueryException`. Again, we would be mixing errors from very different causes. What do you think?
• This could be very interesting if we were to provide the user with an easy way of specifying how he wants to treat retriable errors. For this, we might have to define a wrapper of SQLTimeoutException. But if this must be done by the user, in the end, he will have to decide on the retry policies, so maybe this should be in the documentation.
• All JdbcExceptions should wrap a Throwable, so the user could access the more specific SQL error. Currently, I don't think having more specific SQL errors is useful internally, but maybe in the future is helpful.

I agree that the API is more complicated. Maybe the solution is to have less categorization (e.g. using only one level of subclasses for QueryException). What do you think? Yet, you should be able now to do pattern matching over the corresponding error. I am sure @jdegoes will be able to give a more definitive opinion.

[pablf]

With the new API, do I need to pattern match over both FailedToConnect and ZSQLException in order to handle connection timeout?

Yes, it might be better to create a TimeoutException to deal with these cases. Although an user might want to treat differently the timeout if the cause was the failure to make a connection or the at the query level. Maybe it's better to have this information in the TimeoutException.

[jdegoes]

It could be useful to use sealed trait to our advantage. In addition to defining the precise taxonomy of errors, which is categorized according to the structure of the ADT, we could define at least two other sealed trait markers, which exist only to group errors: one for the grouping of fatal errors, one for the grouping of potentially recoverable errors. This way, a user can refineOrDie away the errors they don't want.

[domartynov]

Question: Do we expect new Restorable(...) to fail (it seems like a wrapper)?

[pablf]

I think so, because when making a new instance of Restorable some methods that might throw SQLException of the underlying connection are called.

[domartynov]

Question: What the idea behind wrapping with FailedAccess? As a user the way pool works is an implementation detail, do I want to see an underlying SQL error directly? Is my assumption correct, that the pool is expected not to fail due to its own error, only in result of some underlying error? No pool access is involved here, right?

[pablf]

I think that access and accessZIO are designed to be used also by the user, so a FailedAccess would return the error and the function that the user has employed. But when used internally it might be better to handle the error before getting to the user.

[domartynov]

I see you point of accessZIO being public API. Still not sure what's the benefit of wrapping in a separate case class FailedAccess. Assuming error handling logic is implemented up in the stack, along with FailedToConnection, now the user have to pattern match over multiple case classes in order to extract error details: FailedAccess, FailedToConnect, and ZSQLException. Plus, the only place accessZIO is used so far is executeSqlWith which extracts SQLException and "dies" on other failures.

I think ZSQLException makes more sense, but in this case we need to figure out how to handle user code errors in f. Can we limit its error channel to SQLException or ZSQLException (by requiring to refineOrDie in f). My problem with this approach is that if we consider accessZIO as publish API (i.e. f is use code), we should allow to use type error. No sure exactly why, but for some reason I still like Throwable at this level (maybe because the wrapper make the code in executeSqlWith more complicated, and still not sure about errors ZIO.attempt turned to "die"). 🤔

[pablf]

I also think that ideally accessZIO should return only ZSQLException. At this point, FailedAccess exists merely because accessZIO is public and the user might want to use it in other ways.

In reality, using accessZIO in unconventional ways that do not return only SQLException should not be standard. We could refine accessZIO and create an unsafeAccessZIO that returns Throwable. And this way we can avoid FailedAccess. What do you think?

[domartynov]

I also think that ideally accessZIO should return only ZSQLException. At this point, FailedAccess exists merely because accessZIO is public and the user might want to use it in other ways.

I think accessZIO, is a low level API to access Connection, so Throwable is ok, and access cannot even express user specific errors as it can only use ZIO.attempt. I think we can consider anything up to ZResultSet as semi-private API of the lib. 🤔

[jdegoes]

I agree with @domartynov

[jdegoes]

@pablf This is very close to merging! Just a few tweaks and then fixing the build (which appears broken).

Can you please re-open this pull request when you have a moment to wrap it up? Thank you! 🙏