Provide access to all counters in case of BatchUpdateException with multiple batches

[asollberger]

Affects: 3.0.0.M1 to 5.2.0.RELEASE

Goal

I'm trying to use the jdbcTemplate.batchUpdate and would like to know the exact failed statement.

Issues

1. Trying to catch the BatchUpdateException which contains the counters does not work because the BatchUpdateException gets removed from the exception stack.
2. Even when the BatchUpdateException gets returned it only contains the counters of the current batch. The successful batch counts need to be communicated back to the caller as well.

Code to reproduce the problem

I use this table to quickly get a unique constraint exception

create table batch_test (
    id number primary key
);

Configuration

Spring: 5.1.3.RELEASE
Database: Oracle 18c

Code trying to catch the BatchUpdateException

final List<Integer> primaryKeys = Arrays.asList(1, 2, 3, 4, 5, 6, 2, 7, 8, 9);
try {
    return jdbcTemplate.batchUpdate(
            "insert into batch_test values (?)",
            primaryKeys,
            4,
            (ps, primaryKey) -> {
                ps.setInt(1, primaryKey);
            }
    );
} catch (final DataAccessException e) {
    final Throwable cause = e.getCause();
    if (cause instanceof BatchUpdateException) {
        final BatchUpdateException batchUpdateException = (BatchUpdateException) cause;
        final long[] updateCounts = batchUpdateException.getLargeUpdateCounts();
        // log the exact one that failed
        for (int index = 0; index < updateCounts.length; index++) {
            if (updateCounts[index] == Statement.EXECUTE_FAILED) {
                logger.debug("The insert of element " + index + " of the array failed");
            }
        }
        if (updateCounts.length < primaryKeys.size()) {
            logger.debug("The insert of elements " + (updateCounts.length - 1)
                    + " to " + (primaryKeys.size() - 1) + " of the array failed");
        }
    }
    throw e;
}

Possible solutions

The problem I'm having happens here:
https://github.com/spring-projects/spring-framework/blame/master/spring-jdbc/src/main/java/org/springframework/jdbc/support/SQLErrorCodeSQLExceptionTranslator.java#L179

The original stack sqlEx gets overridden with the next exception and thus the information of the BatchUpdateException gets lost.

I understand the intent to translate the cause into an exception that is a little more meaningful, but IMHO the original BatchUpdateException should not be trashed.

Wherever a new exception is created the original exception should be used for the stack. So instead of:

DataAccessException dae = customTranslate(task, sql, sqlEx);
DataAccessException customDex = customTranslator.translate(task, sql, sqlEx);
DataAccessException customException = createCustomException(task, sql, sqlEx, customTranslation.getExceptionClass());
return new BadSqlGrammarException(task, (sql != null ? sql : ""), sqlEx);

the original exception should be used:

DataAccessException dae = customTranslate(task, sql, ex);
DataAccessException customDex = customTranslator.translate(task, sql, ex);
DataAccessException customException = createCustomException(task, sql, ex, customTranslation.getExceptionClass());
return new BadSqlGrammarException(task, (sql != null ? sql : ""), ex);

The second issue is probably here:
https://github.com/spring-projects/spring-framework/blob/master/spring-jdbc/src/main/java/org/springframework/jdbc/core/JdbcTemplate.java#L1059

rowsAffected needs to be communicated back to the caller to pinpoint the problematic update

Workaround

My current solution consist of writing my own Translator, overriding the doTranslate method and setting the translator when I create the jdbcTemplate Bean

public class BatchUpdateExceptionTranslator extends SQLErrorCodeSQLExceptionTranslator {
    @Override
    @Nullable
    protected DataAccessException doTranslate(String task, @Nullable String sql, SQLException ex) {
        // exchanging sqlEx with ex whenever creating a new Exception
    }
}

@Configuration
public class MyConfiguration {
    @Bean
    public JdbcTemplate jdbcTemplate(
            final DataSource dataSource
    ) {
        final JdbcTemplate jdbcTemplate = new JdbcTemplate(dataSource);
        jdbcTemplate.setExceptionTranslator(new BatchUpdateExceptionTranslator());
        return jdbcTemplate;
    }
}

The second issue cannot be fixed as easily. Either avoid the use of the batched update or set the batch size very high to not run into that problem

[lafual]

If anyone is coming here for the work around (I am using spring-jdbc-5.3.29.jar, oracle8-19.3.0.0.jar, Oracle 19, JDK 8) and has the "second issue" (batch size < collection size), then the above sample needs to be adapted.

I am trying to insert ~40,000 records and Oracle is throwing ORA-00001: unique constraint (..) violated. In order to get the failed row I need to set "batch size = collection.size". The return results array only contains successes, so the bad row (I assume) is the new row, which means that collection.get(update.length) should be reported.

[marschall]

I don't know if everything in this issue is still current.

Trying to catch the BatchUpdateException which contains the counters does not work because the BatchUpdateException gets removed from the exception stack

This no longer seems to be the case. BatchUpdateException gets passed as a cause.

Even when the BatchUpdateException gets returned it only contains the counters of the current batch. The successful batch counts need to be communicated back to the caller as well.

I have three proposals how this could be solved:

1. Add an additional exception (eg. BatchUpdateDetailsException) in the stack that wraps BatchUpdateException
2. Add a suppressed exception to BatchUpdateException, eg. BatchUpdateDetailsException
3. Add a next exception to BatchUpdateException, eg. BatchUpdateDetailsException

[snicoll]

Playing a bit with the example above, we throw a DuplicateKeyException which sounds like the right call, having the BatchUpdateException as the nested cause. The problem is this method that lets your run several batches won't convey where it is indeed.

I think a try/catch here could be useful:

spring-framework/spring-jdbc/src/main/java/org/springframework/jdbc/core/JdbcTemplate.java

Line 1119 in 94e2bef

rowsAffected.add(ps.executeBatch());

We could then throw an exception that contains something built with rowsAffected. To limit disturbance, extending from BatchUpdateException is an idea but I am not aware of framework extending an SQL exception. Wrapping it does not look great as it would break backward compatiblity.

I've moved the sample above to a test case, see snicoll@b90368b

WDYT @jhoeller?

[jhoeller]

Maybe we could replace the original BatchUpdateException with an instance of our own, constructing it with aggregated update counts? Would not even have to be a custom subclass of BatchUpdateException if it does not need to expose additional state.

[snicoll]

@asollberger, @marschall, and anyone else interested, I've pushed a proposal for this see snicoll@bda23b4. Although I am not a huge fan of extending BatchSqlUpdateException that's what we should be doing to keep backward compatibility.

We'd like some feedback before moving on.

[marschall]

@snicoll I had a short look at it. I had encountered some problems when reviewing the code

• I'm not used to AssertJ and struggle with the readability of the test.
• I currently can't build Spring locally

Could not resolve all artifacts for configuration ':classpath'.
Could not resolve org.ysb33r.gradle:grolifant:0.14.
Required by:
project : > org.asciidoctor.jvm.convert:org.asciidoctor.jvm.convert.gradle.plugin:3.1.0 > org.asciidoctor:asciidoctor-gradle-jvm:3.1.0
project : > org.asciidoctor.jvm.pdf:org.asciidoctor.jvm.pdf.gradle.plugin:3.1.0 > org.asciidoctor:asciidoctor-gradle-jvm-pdf:3.1.0
project : > org.asciidoctor.jvm.convert:org.asciidoctor.jvm.convert.gradle.plugin:3.1.0 > org.asciidoctor:asciidoctor-gradle-jvm:3.1.0 > org.asciidoctor:asciidoctor-gradle-base:3.1.0
> Could not resolve org.ysb33r.gradle:grolifant:0.14.
> Could not get resource 'https://repo.spring.io/plugins-release/org/ysb33r/gradle/grolifant/0.14/grolifant-0.14.pom'.
> Could not GET 'https://repo.spring.io/plugins-release/org/ysb33r/gradle/grolifant/0.14/grolifant-0.14.pom'. Received status code 401 from server:

Regarding the code itself:

• Do I understand correctly the DuplicateKeyException comes from the SQLExceptionTranslator, so depending on the error code it could be something completely different?
• Are there any plans to support large updates in the future? If so we may think about how we may want to integrate this. Maybe a second *LargeUpdate*Exception.
• I don't think AggregatedBatchUpdateException is a good name. What does it aggregate? I would prefer something like BatchUpdateDetailsException.
• The cause of AggregatedBatchUpdateException will be cause.getCause(). Don't we lose the original BatchUpdateException from the JDBC driver this way? Is this intentional?
• I don't like the API. I don't like the asymmetry between #getUpdateCounts() having a scope of only the current batch and #getSuccessfulUpdateCounts() having a scope of all batches until the exception. What about instead having a #getCurrentBatchIndex() method (btd whether this is 0-based like Java or 1-based like JDBC)? This would keep the symetry. Altough I admit I may be less convenient to the user and the successful update count may be lost when a statement does not update a known number of rows (eg. INSERT). The other option would be to make AggregatedBatchUpdateException not a BatchUpdateException.

[snicoll]

Thanks for the feedback but let's please focus on the task at hand, I don't know why you can't build framework at the moment, I am not aware we rely on plugins-release so it might be something you've added on your end?

I don't think AggregatedBatchUpdateException is a good name. What does it aggregate? I would prefer something like BatchUpdateDetailsException.

It's not a details of the BatchUpdateDetailsException so we can't really name that. The intent of the exception is to aggregate information about the batches that worked with that one that failed.

The cause of AggregatedBatchUpdateException will be cause.getCause(). Don't we lose the original BatchUpdateException from the JDBC driver this way? Is this intentional?

We don't since the one we create has all the information of the original, plus the information about the batches that were invoked prior to the exception. Yes, this is intentional.

I don't like the asymmetry between #getUpdateCounts() having a scope of only the current batch and #getSuccessfulUpdateCounts() having a scope of all batches until the exception.

OK. We have to keep getUpdateCounts for the simple reason that it's part of the API of the exception we inherit from. I don't have a strong opinion, except that can't go away.

The other option would be to make AggregatedBatchUpdateException not a BatchUpdateException.

It has to be for exception translation to occur as usual. Not being one would break backward compatiblity (see the original description if you need an example).

[marschall]

Thanks for the feedback but let's please focus on the task at hand, I don't know why you can't build framework at the moment, I am not aware we rely on plugins-release so it might be something you've added on your end?

I don't think AggregatedBatchUpdateException is a good name. What does it aggregate? I would prefer something like BatchUpdateDetailsException.

It's not a details of the BatchUpdateDetailsException so we can't really name that. The intent of the exception is to aggregate information about the batches that worked with that one that failed.

Alright.

The cause of AggregatedBatchUpdateException will be cause.getCause(). Don't we lose the original BatchUpdateException from the JDBC driver this way? Is this intentional?

We don't since the one we create has all the information of the original, plus the information about the batches that were invoked prior to the exception. Yes, this is intentional.

It doesn't have the same information. The next exception (java.sql.SQLException#getNextException()) and the suppressed exceptions are missing. That could be fixed. What can not be fixed is that some drivers throw subclasses of BatchUpdateException. Alone on GitHub we find H2 and Spanner. In the case of Spanner we lose the gRPC error code. We find various other drivers including the JDBC-ODBC bridge.

https://github.com/alibaba/wasp/blob/master/src/main/java/com/alibaba/wasp/jdbc/JdbcBatchUpdateException.java
https://github.com/freecores/myforthprocessor/blob/master/opencores/client/foundation%20classes/j2sdk-1_4_2-src-scsl/j2se/src/share/classes/sun/jdbc/odbc/JdbcOdbcBatchUpdateException.java
https://github.com/ciusji/guinsoo/blob/master/src/main/org/guinsoo/jdbc/JdbcBatchUpdateException.java
https://github.com/wplatform/jdbc-shards/blob/master/src/main/java/com/wplatform/ddal/jdbc/JdbcBatchUpdateException.java

Oracle seems to store their extensions in the cause which should keep working.

I don't like the asymmetry between #getUpdateCounts() having a scope of only the current batch and #getSuccessfulUpdateCounts() having a scope of all batches until the exception.

OK. We have to keep getUpdateCounts for the simple reason that it's part of the API of the exception we inherit from. I don't have a strong opinion, except that can't go away.

I see your point. Modifying it to give it different semantics may also not be a good idea.

The other option would be to make AggregatedBatchUpdateException not a BatchUpdateException.

It has to be for exception translation to occur as usual. Not being one would break backward compatiblity (see the original description if you need an example).

Do you mean the check in SQLErrorCodeSQLExceptionTranslator to find the actual underlying error code? We could also hard code our own SQLException subclass there but I do agree that is not a nice solution as well.

[snicoll]

Thanks very much for the feedback @marschall, that's very useful. Here is another commit that should address the issues you've raised:

• The original exception is made accessible. If you need a subclass, you can access it although I suspect this is going to be quite minor (but still a breaking change for those who were relying on that).
• Next SQL exceptions and suppressed exceptions are available on the aggregate exception

Do you mean the check in SQLErrorCodeSQLExceptionTranslator to find the actual underlying error code?

No I didn't mean that. Although that as well even if we can change our own translator to work with the exception. If you work with batches, it's very likely that the code is expecting a BatchUpdateException as the cause. As I've stated previously, look at the example in the first comment of this issue if you need an example of that.

I believe we're good to go but I am giving it a few more days for others to chime in if they have concerns. The next milestone is next week.

[asollberger]

Hi Stephane
I did not think this would ever be resolved. Thanks for your work. I'll try to test it this week.
Alain

[snicoll]

@asollberger to make it easier for you to test, I've merged this so that it's available in Spring Framework 6.2.0-SNAPSHOT. You'll need to add repo.spring.io/snapshot to download the bits. Let me know what you think and we still have plenty of time to iterate on the feature before GA in November.

[marschall]

I think the implementation is a fair compromise. If I could add one thing then I think this if should be changed to a while to keep the original behavior.

spring-framework/spring-jdbc/src/main/java/org/springframework/jdbc/support/SQLErrorCodeSQLExceptionTranslator.java

Line 187 in bf5e218

if (sqlEx instanceof BatchUpdateException && sqlEx.getNextException() != null) {

[asollberger]

Alright, finally got my own test case together and saw the returned successfulUpdateCount and the originalExpection.
Looks good to me and I love the addition of the original exception.

Thanks a lot