HHH-7689 - addToBatch in BatchingBatch blows out of error handling and doesn't abort the batch

hibernate-core/src/main/java/org/hibernate/engine/jdbc/batch/internal/AbstractBatchImpl.java

@@ -101,6 +101,10 @@ public abstract class AbstractBatchImpl implements Batch {
 				.getSqlStatementLogger();
 	}
 
+	protected void abortBatch() {
+		jdbcCoordinator.abortBatch();
+	}
+
 	/**
 	 * Access to the batch's map of statements (keyed by SQL statement string).
 	 *

hibernate-core/src/main/java/org/hibernate/engine/jdbc/batch/internal/BatchingBatch.java

@@ -119,8 +119,8 @@ public class BatchingBatch extends AbstractBatchImpl {
 					checkRowCounts( statement.executeBatch(), statement );
 				}
 				catch ( SQLException e ) {
-					LOG.debug( "SQLException escaped proxy", e );
+					abortBatch();
-					throw sqlExceptionHelper().convert( e, "could not perform addBatch", entry.getKey() );
+					throw sqlExceptionHelper().convert( e, "could not execute batch", entry.getKey() );
 				}
 			}
 		}

hibernate-core/src/main/java/org/hibernate/engine/jdbc/batch/internal/NonBatchingBatch.java

@@ -27,6 +27,7 @@ import java.sql.PreparedStatement;
 import java.sql.SQLException;
 import java.util.Map;
 
+import org.hibernate.JDBCException;
 import org.hibernate.engine.jdbc.batch.spi.BatchKey;
 import org.hibernate.engine.jdbc.spi.JdbcCoordinator;
 import org.hibernate.internal.CoreMessageLogger;
@@ -62,8 +63,12 @@ public class NonBatchingBatch extends AbstractBatchImpl {
 				jdbcCoordinator.release( statement );
 			}
 			catch ( SQLException e ) {
-				LOG.debug( "SQLException escaped proxy", e );
+				abortBatch();
-				throw sqlExceptionHelper().convert( e, "could not execute batch statement", entry.getKey() );
+				throw sqlExceptionHelper().convert( e, "could not execute non-batched batch statement", entry.getKey() );
+			}
+			catch (JDBCException e) {
+				abortBatch();
+				throw e;
 			}
 		}
 		getStatements().clear();

hibernate-core/src/test/java/org/hibernate/test/batch/BatchingBatchFailureTest.java

@@ -31,7 +31,6 @@ import javax.persistence.Table;
 import java.lang.reflect.Field;
 import java.util.Map;
 
-import org.hibernate.HibernateException;
 import org.hibernate.Session;
 import org.hibernate.cfg.AvailableSettings;
 import org.hibernate.cfg.Configuration;
@@ -73,22 +72,20 @@ public class BatchingBatchFailureTest extends BaseCoreFunctionalTestCase {
 		Session session = openSession();
 		session.getTransaction().begin();
 
-		//open secondary session to insert a valid entity and an invalid entity
-		Session secondSession = session.sessionWithOptions()
-				.connection()
-				.flushBeforeCompletion( true )
-				.autoClose( true )
-				.openSession();
-
 		try {
-			secondSession.persist( new User( 1, "ok" ) );
+			session.persist( new User( 1, "ok" ) );
-			secondSession.persist( new User( 2, null ) );
+			session.persist( new User( 2, null ) );
+			session.persist( new User( 3, "ok" ) );
+			session.persist( new User( 4, "ok" ) );
+			session.persist( new User( 5, "ok" ) );
+			session.persist( new User( 6, "ok" ) );
 			// the flush should fail
-			secondSession.flush();
+			session.flush();
-			fail( "Expecting SQLException" );
+			fail( "Expecting failed flush" );
 		}
-		catch (HibernateException expected) {
+		catch (Exception expected) {
-			secondSession.close();
+			System.out.println( "Caught expected exception : " + expected );
+			expected.printStackTrace( System.out );
 
 			try {
 				//at this point the transaction is still active but the batch should have been aborted (have to use reflection to get at the field)
@@ -121,9 +118,7 @@ public class BatchingBatchFailureTest extends BaseCoreFunctionalTestCase {
 	@Entity( name = "User" )
 	@Table( name = "`USER`" )
 	public static class User {
-		@Id
 		private Integer id;
-		@Column( nullable = false )
 		private String name;
 
 		public User() {
@@ -133,5 +128,23 @@ public class BatchingBatchFailureTest extends BaseCoreFunctionalTestCase {
 			this.id = id;
 			this.name = name;
 		}
+
+		@Id
+		public Integer getId() {
+			return id;
+		}
+
+		public void setId(Integer id) {
+			this.id = id;
+		}
+
+		@Column( nullable = false )
+		public String getName() {
+			return name;
+		}
+
+		public void setName(String name) {
+			this.name = name;
+		}
 	}
 }

hibernate-core/src/test/java/org/hibernate/test/batch/NonBatchingBatchFailureTest.java

@@ -0,0 +1,146 @@
+/*
+ * Hibernate, Relational Persistence for Idiomatic Java
+ *
+ * Copyright (c) 2013, Red Hat Inc. or third-party contributors as
+ * indicated by the @author tags or express copyright attribution
+ * statements applied by the authors.  All third-party contributions are
+ * distributed under license by Red Hat Inc.
+ *
+ * This copyrighted material is made available to anyone wishing to use, modify,
+ * copy, or redistribute it subject to the terms and conditions of the GNU
+ * Lesser General Public License, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY
+ * or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU Lesser General Public License
+ * for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this distribution; if not, write to:
+ * Free Software Foundation, Inc.
+ * 51 Franklin Street, Fifth Floor
+ * Boston, MA  02110-1301  USA
+ */
+package org.hibernate.test.batch;
+
+import javax.persistence.Column;
+import javax.persistence.Entity;
+import javax.persistence.Id;
+import javax.persistence.Table;
+import java.lang.reflect.Field;
+import java.util.Map;
+
+import org.hibernate.Session;
+import org.hibernate.cfg.AvailableSettings;
+import org.hibernate.cfg.Configuration;
+import org.hibernate.engine.jdbc.batch.internal.AbstractBatchImpl;
+import org.hibernate.engine.jdbc.batch.internal.NonBatchingBatch;
+import org.hibernate.engine.jdbc.batch.spi.Batch;
+import org.hibernate.engine.spi.SessionImplementor;
+
+import org.junit.Test;
+
+import org.hibernate.testing.TestForIssue;
+import org.hibernate.testing.junit4.BaseCoreFunctionalTestCase;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.fail;
+
+/**
+ * @author Shawn Clowater
+ * @author Steve Ebersole
+ */
+@TestForIssue( jiraKey = "HHH-7689" )
+public class NonBatchingBatchFailureTest extends BaseCoreFunctionalTestCase {
+	@Override
+	protected Class<?>[] getAnnotatedClasses() {
+		return new Class[] { User.class };
+	}
+
+	@Override
+	protected void configure(Configuration configuration) {
+		super.configure( configuration );
+		// explicitly disable batching
+		configuration.setProperty( AvailableSettings.STATEMENT_BATCH_SIZE, "-1" );
+		// and disable in-vm nullability checking (so we can force in-db not-null constraint violations)
+		configuration.setProperty( AvailableSettings.CHECK_NULLABILITY, "false" );
+	}
+
+	@Test
+	public void testBasicInsertion() {
+		Session session = openSession();
+		session.getTransaction().begin();
+
+		try {
+			session.persist( new User( 1, "ok" ) );
+			session.persist( new User( 2, null ) );
+			session.persist( new User( 3, "ok" ) );
+			// the flush should fail
+			session.flush();
+			fail( "Expecting failed flush" );
+		}
+		catch (Exception expected) {
+			System.out.println( "Caught expected exception : " + expected );
+			expected.printStackTrace( System.out );
+
+			try {
+				//at this point the transaction is still active but the batch should have been aborted (have to use reflection to get at the field)
+				SessionImplementor sessionImplementor = (SessionImplementor) session;
+				Field field = sessionImplementor.getTransactionCoordinator().getJdbcCoordinator().getClass().getDeclaredField( "currentBatch" );
+				field.setAccessible( true );
+				Batch batch = (Batch) field.get( sessionImplementor.getTransactionCoordinator().getJdbcCoordinator() );
+				if ( batch == null ) {
+					throw new Exception( "Current batch was null" );
+				}
+				else {
+					//make sure it's actually a batching impl
+					assertEquals( NonBatchingBatch.class, batch.getClass() );
+					field = AbstractBatchImpl.class.getDeclaredField( "statements" );
+					field.setAccessible( true );
+					//check to see that there aren't any statements queued up (this can be an issue if using SavePoints)
+					assertEquals( 0, ((Map) field.get( batch )).size() );
+				}
+			}
+			catch (Exception fieldException) {
+				fail( "Couldn't inspect field " + fieldException.getMessage() );
+			}
+		}
+		finally {
+			session.getTransaction().rollback();
+			session.close();
+		}
+	}
+
+	@Entity( name = "User" )
+	@Table( name = "`USER`" )
+	public static class User {
+		private Integer id;
+		private String name;
+
+		public User() {
+		}
+
+		public User(Integer id, String name) {
+			this.id = id;
+			this.name = name;
+		}
+
+		@Id
+		public Integer getId() {
+			return id;
+		}
+
+		public void setId(Integer id) {
+			this.id = id;
+		}
+
+		@Column( nullable = false )
+		public String getName() {
+			return name;
+		}
+
+		public void setName(String name) {
+			this.name = name;
+		}
+	}
+}