aaberg / sql2o

sql2o is a small library, which makes it easy to convert the result of your sql-statements into objects. No resultset hacking required. Kind of like an orm, but without the sql-generation capabilities. Supports named parameters.
http://sql2o.org
MIT License
1.15k stars 229 forks source link

Should JDBC Connection's getAutoCommit() be checked before attempting to rollback after an exception? #248

Open DanStout opened 8 years ago

DanStout commented 8 years ago

Currently in Connection.java in the Connection rollback(boolean closeConnection) method, rollback() is called on the underlying JDBC connection without first checking whether autocommit is enabled.

This means an exception like: org.postgresql.util.PSQLException: Cannot rollback when autoCommit is enabled. will be thrown if an exception occurs.

One way to implement this check would maybe be to change the constructor like so:

Connection(Sql2o sql2o, boolean autoClose) {
    this.autoClose = autoClose;
    this.sql2o = sql2o;
    createConnection();
    try {
        this.rollbackOnException = this.rollbackOnClose = !jdbcConnection.getAutoCommit();
    }
    catch(SQLException e) {
        logger.warn("Unable to determine autocommit state", e);
    }
}

A similar check is already performed in the close() method, but I think it might make sense to do it once and set both rollbackOnException and rollbackOnClose at the same time. Alternatively the check could just be performed inside onException() similarly to how the check is performed inside close().

Also, the logging lines should probably be changed from: logger.warn("Could not roll back transaction. message: {}", e); to: logger.warn("Could not roll back transaction", e); Since currently the exception is printing the curly braces themselves. (At least SLF4J/Log4j)

tstrohmeier commented 4 years ago

I think this still exists. Currently I use the following workaround:

try (Connection con = dql2o.open()) {
            con.getJdbcConnection().setAutoCommit(false);
            con.createQuery(sql)
                    .addParameter("name", object.getName())
                    .executeUpdate();
            con.commit();
            ...
}

The problem ist if I don't set con.getJdbcConnection().setAutoCommit(false); then the commit fails cause autoCommit already triggered a commit. I also get an Exception when the connection is closed and rolled back, since a rollback is not possible with autoCommit.