Unable to catch specific sql exception

[ajitj]

This change https://github.com/luminus-framework/conman/commit/632d579580b8d2b05a34434d3aff64a228d7444a prevents us from catching and handling more specific exceptions such as java.sql.SQLException and its subclasses such as java.sql.SQLIntegrityConstraintViolationException that we were relying on.

[yogthos]

Would you be up for doing a pr to preserve the exception type when rethrowing?

[yogthos]

I was just looking at this again. Since the thrown exception is initialized with the original exception, you should be able to just call getCause on it:

(try
  (try
    (throw (IllegalArgumentException. "foo"))
    (catch Exception e
      (throw (Exception. "Exception in foo" e))))
  (catch Exception e
    (type (.getCause e))))
=> java.lang.IllegalArgumentException

[not-raspberry]

I don't really understand - the point of exceptions is so I can catch exactly what I want without conditionals.

This actually broke my code in production so in order to fix it I was thinking of unwrapping it like that:

(defn db-q [q-fn & args]
  (try
    (apply q-fn args)
    (catch Exception e
      (throw
        (if-some [cause (.getCause e)]
          cause
          e)))))

But adding it to every DB function was too tedious so I reverted conman to the last known working version.

I'm not from the Java world (I mostly write Python) but I think the changeset in 632d579580b8d2b05a34434d3aff64a228d7444a is pretty much how-not-to raise exceptions.

Can we get it reverted? A nice message can go to log/error. Yeah, I know, the tracebacks are too long for anyone to see it but what else can we do?

[yogthos]

Is the problem here that you were relying on the original exception type? I think the change is useful because it makes it much easier to tell what generated function caused the exception. However, perhaps the logic can be improved, one option could be to append the original error message to the generated exception. Another option could be to use ex-info with more detailed information in it.

[not-raspberry]

I think an the exception type is a contract not to be broken.

Appending to the original exception won't always work:

user=> (import 'org.postgresql.util.PSQLException)
#<Class@79a1aa75 org.postgresql.util.PSQLException>
user=> (PSQLException. "a")

user=> java.lang.ClassCastException: java.lang.String cannot be cast to org.postgresql.util.ServerErrorMessage

Not all of them take strings.

Ex-info is more acceptable (at least I can match it with slingshot) but still a breaking change.

I would still revert the change if possible, maybe replacing raising a generic exception with logging.

Keep in mind I'm not an expert in Clojure.

[yogthos]

What I meant is that you could do something like

(Exception. (str (format "Exception in %s" id#) " cause: "  (.getMessage e)) e)

This way you'd see the reason the error happened without having to look at the cause.

I don't think logging is appropriate for a library since it makes too many assumptions about how it is used. Logging should be done by the application using the library.

Wrapping exceptions is considered idiomatic in Java/Clojure. Since the original exception is still preserved in the trace. The trade off here is that you might have to check the cause to see what the original exception was.

I'm leaning towards using the ex-info option since that is the conventional way to handle exceptions in Clojure, and it will provide the additional information with the name of the generated function that caused the exception.

Note that you can also write a macro to handle the exception unwrapping as follows:

(defmacro query [fn-name & args]
  `(try
     (~fn-name ~@args)
     (catch Exception e#
       (throw (or (.getCause e#) e#)))))

(query query-fn {:column-name "value"})

[not-raspberry]

The function works just as well as the macro.

I'll stay with the lower versions of conman for now because the change solves a non-problem for me (I know were my exceptions come from) and requires me to do work I have no budget for.

This may be me not understanding something but I just don't get Clojure's fondness of generic exceptions. Try-catch is painful enough, why would I want to have an additional cond inside?

[yogthos]

The generic exceptions are an artifact of how Java exceptions work unfortunately. Since you can't always instantiate an exception with a specific type.

Meanwhile, exception wrapping is the only way to provide additional information regarding what happened. In a lot of cases it is useful to know the name of the generated function that caused the error. Since the original error is still in the trace you're not losing any information in the process.

[ajitj]

Looking forward to this change https://github.com/layerware/hugsql/issues/77

[socksy]

This has bitten me a couple of times. If you wrap the exception needlessly (and I don't really see the value add here) you can't catch it, and you can't add an exception handler to the compojure API exception handlers for it, and it greatly complects the code base I'm working on. Maybe I'm not seeing some obvious benefit, but I would prefer that the original exception is what is thrown...

[gdrte]

I too feel the same.

[mhavrlent]

Hey guys. Is someone working on this? I'm using the latest version and I still see wrapped exceptions.

I'm trying to learn Clojure by reading book called "Web Development with Clojure - 2nd Edition" and because of this change, the instructions in chapter 8 are incorrect: (instance? java.sql.SQLException e) (-> e (.getNextException) (.getMessage) (.startsWith "ERROR: duplicate key value")))

Here's what works for me with wrapped exception: (-> e (.getCause) (.getNextException) (.getMessage) (.startsWith "ERROR: duplicate key value"))

[nackjicholson]

Like others here, I think the original errors should be thrown. It is a mistake to wrap exceptions of this library with ex-info in my opinion. I want to check error codes off of PSQLExceptions, but can't seem to.

There is a lot of great infomration accessible with .getErrorCode and .getSQLState that users of conman can't use in order ot form sensible user facing error messages. Am I missing something? How do other people use this library without either running extra queries before making updates or somehow trying to interpret what went wrong by parsing the error messages?

https://jdbc.postgresql.org/documentation/publicapi/index.html

https://www.postgresql.org/docs/current/errcodes-appendix.html

EDIT: I guess it's not too bad. After some digging I did figure out how to get at what I wanted to get at by going up the cause chain. Hope this will also help others confused by this.

(defn do-my-thing! [a b]
  (try
    (db/do-thing!* {:a a :b b})
    (catch clojure.lang.ExceptionInfo e
      (.getSQLState (.getCause e)))))