#83
  • Created on
    2012-09-07 18:24
  • Reported by
    kevinoid
  • Status
    New
  • Priority
    Low (4)
  • Assigned to
    -
  • Milestone
    -
  • Plan Level
    -
  • Due Date
    -

Attachments

Related Tickets

Add new subtask or bug

Make it easier for users to distinguish Squeryl exceptions

I realize this is a bit subjective, so I won't be offended if you decide to close the issue, but I would really appreciate it if Squeryl threw a subclass of RuntimeException rather than "raw" RuntimeExceptions for SQL errors. In order to distinguish runtime database errors (such as uniqueness violations) I find myself writing a lot of excessive code like the following:

try {
    // Make a database request and possibly other stuff
} catch {
    case e: RuntimeException =>
        e.getCause match {
            case c: SQLException =>
                // Actually a SQL exception from Squeryl
            case _ =>
                // RuntimeException either not from Squeryl or that I don't know how to deal with
                throw e
        }
}


Whereas if Squeryl threw either SQLException or a new subclass of RuntimeException, say SquerylSQLException it could be handled much more simply as:

try {
    // Make a database request and possibly other stuff
} catch {
    case e: SquerylSQLException =>
    // SQL exception from Squeryl, use e.getCause (hopefully with covariant return type SQLException)
}


Of course, it's also possible that I'm missing some other easy way to recognize SQLExceptions from Squeryl... If so, don't hesitate to point that out.

Thanks for your consideration!
User picture
  on 2012-09-15 01:29 *
By maxime.levesque
I'm not against the idea, but I suspect that if you need this, you probably have code that tries to recover
from some cases SQLExceptions, is that right ?

You can write yourself a function like this :

def squerylBlock[A,B](a: =>A) (f: SQLException=>B) = {
  try {
    a
  } catch {
    case e: RuntimeException =>
        e.getCause match {
            case c: SQLException => f(c)
            case _ =>
                // RuntimeException either not from Squeryl or that I don't know how to deal with
                throw e
        }
  }
}

 squerylBlock {
  .... your squeryl code ...
 } ( sqlException =>
      ... do something with sqlException
 )


That will diminish the boilerplate, probably more so than introducing new exception types
User picture
  on 2012-09-15 03:28 *
By kevinoid
Sure, that would work, as would a custom partial function in combination with scala.util.control.Exception or Scala 2.9 generalized catch blocks. They all work. But, of course, they all add conceptual complexity to the codebase as well. It's not a lot, but it's easily avoided (at least it appears that way to me - is there any reason not to throw a custom exception type?).
User picture
  on 2012-09-16 00:09 *
By maxime.levesque
I would see nothing wrong in throwing custom exceptions,
it's just that I have never had the need for it, or heard someone express it.

In the most common use cases exceptions are errors that cannot be recovered,
the only thing that is done with them is to produce a meaningful log message.

You probably have a use case where altering the flow based on the type of exception
makes sense.

If someone does the change I would accept the patch, it can't do any harm.
User picture
  on 2012-09-25 23:13 *
By kevinoid
Great. I played around with the code a bit and worked up some potential changes. The branch is at https://github.com/kevinoid/Squeryl/commits/typed-exceptions . Note that I only really care about the first commit of those changes, but thought I'd post the others in case you were interested.

Before sending a pull request, I wanted to get your opinion on what to do about SQLExceptions which are not currently wrapped into RuntimeExceptions by Squeryl. For example, `Table._batchedUpdateOrInsert` may throw an SQLException from executeBatch. Should client code check for both SQLException and the wrapped type (which I called SquerylSQLException) or should SQLExceptions always be wrapped into SquerylSQLExceptions (which will break compatibility if any apps expect SQLExceptions from those methods)?
User picture
  on 2012-09-26 14:58 *
By maxime.levesque
It all looks good, though I'd have a preferece for not having a null in SquerylSQLException (see my comment)
User picture
  on 2012-10-03 16:19 *
By kevinoid
Just checking if you've had a chance to look at the updated pull request and whether you had given any thought to which exception type should be thrown from Table._batchedUpdateOrInsert.
User picture
  on 2012-10-03 17:34 *
By maxime.levesque
Sorry, I'm not seeing the pull request in Github, not sure why, can you make another one ?
User picture
  on 2012-10-03 18:58 *
By kevinoid
Sure thing. Pull request incoming.