HHH-5732 @OrderColumn not updated if @OneToMany has mappedby defined

hibernate-core/src/main/java/org/hibernate/persister/collection/AbstractCollectionPersister.java

@@ -32,8 +32,6 @@ import java.util.HashMap;
 import java.util.Iterator;
 import java.util.Map;
 
-import org.jboss.logging.Logger;
-
 import org.hibernate.AssertionFailure;
 import org.hibernate.FetchMode;
 import org.hibernate.HibernateException;
@@ -47,7 +45,6 @@ import org.hibernate.cache.spi.entry.StructuredCollectionCacheEntry;
 import org.hibernate.cache.spi.entry.StructuredMapCacheEntry;
 import org.hibernate.cache.spi.entry.UnstructuredCacheEntry;
 import org.hibernate.cfg.Configuration;
-import org.hibernate.cfg.NotYetImplementedException;
 import org.hibernate.collection.spi.PersistentCollection;
 import org.hibernate.dialect.Dialect;
 import org.hibernate.engine.jdbc.batch.internal.BasicBatchKey;
@@ -64,7 +61,6 @@ import org.hibernate.id.IdentifierGenerator;
 import org.hibernate.internal.CoreMessageLogger;
 import org.hibernate.internal.FilterAliasGenerator;
 import org.hibernate.internal.FilterHelper;
-import org.hibernate.internal.StaticFilterAliasGenerator;
 import org.hibernate.internal.util.StringHelper;
 import org.hibernate.internal.util.collections.ArrayHelper;
 import org.hibernate.jdbc.Expectation;
@@ -92,13 +88,13 @@ import org.hibernate.sql.ordering.antlr.ColumnMapper;
 import org.hibernate.sql.ordering.antlr.ColumnReference;
 import org.hibernate.sql.ordering.antlr.FormulaReference;
 import org.hibernate.sql.ordering.antlr.OrderByAliasResolver;
-import org.hibernate.sql.ordering.antlr.OrderByFragmentParser;
 import org.hibernate.sql.ordering.antlr.OrderByTranslation;
 import org.hibernate.sql.ordering.antlr.SqlValueReference;
 import org.hibernate.type.CollectionType;
 import org.hibernate.type.CompositeType;
 import org.hibernate.type.EntityType;
 import org.hibernate.type.Type;
+import org.jboss.logging.Logger;
 
 /**
  * Base implementation of the <tt>QueryableCollection</tt> interface.
@@ -184,7 +180,7 @@ public abstract class AbstractCollectionPersister
 	protected final boolean hasIdentifier;
 	private final boolean isLazy;
 	private final boolean isExtraLazy;
-	private final boolean isInverse;
+	protected final boolean isInverse;
 	private final boolean isMutable;
 	private final boolean isVersioned;
 	protected final int batchSize;
@@ -197,7 +193,7 @@ public abstract class AbstractCollectionPersister
 	private final String entityName;
 
 	private final Dialect dialect;
-	private final SqlExceptionHelper sqlExceptionHelper;
+	protected final SqlExceptionHelper sqlExceptionHelper;
 	private final SessionFactoryImplementor factory;
 	private final EntityPersister ownerPersister;
 	private final IdentifierGenerator identifierGenerator;
@@ -1194,7 +1190,7 @@ public abstract class AbstractCollectionPersister
 
 	}
 
-	private BasicBatchKey recreateBatchKey;
+	protected BasicBatchKey recreateBatchKey;
 
 	public void recreate(PersistentCollection collection, Serializable id, SessionImplementor session)
 			throws HibernateException {

hibernate-core/src/main/java/org/hibernate/persister/collection/OneToManyPersister.java

@@ -29,6 +29,7 @@ import java.sql.PreparedStatement;
 import java.sql.SQLException;
 import java.util.Iterator;
 
+import org.hibernate.HibernateException;
 import org.hibernate.MappingException;
 import org.hibernate.cache.CacheException;
 import org.hibernate.cache.spi.access.CollectionRegionAccessStrategy;
@@ -43,7 +44,6 @@ import org.hibernate.internal.FilterAliasGenerator;
 import org.hibernate.internal.util.collections.ArrayHelper;
 import org.hibernate.jdbc.Expectation;
 import org.hibernate.jdbc.Expectations;
-import org.hibernate.loader.collection.BatchingCollectionInitializer;
 import org.hibernate.loader.collection.BatchingCollectionInitializerBuilder;
 import org.hibernate.loader.collection.CollectionInitializer;
 import org.hibernate.loader.collection.SubselectOneToManyLoader;
@@ -58,6 +58,7 @@ import org.hibernate.sql.Update;
  * Collection persister for one-to-many associations.
  *
  * @author Gavin King
+ * @author Brett Meyer
  */
 public class OneToManyPersister extends AbstractCollectionPersister {
 
@@ -135,11 +136,20 @@ public class OneToManyPersister extends AbstractCollectionPersister {
 	}
 
 	/**
-	 * Not needed for one-to-many association
+	 * Generate the SQL UPDATE that inserts a collection index
 	 */
 	@Override
     protected String generateUpdateRowString() {
-		return null;
+		Update update = new Update( getDialect() ).setTableName( qualifiedTableName );
+		update.addPrimaryKeyColumns( elementColumnNames, elementColumnIsSettable, elementColumnWriters );
+		if ( hasIdentifier ) {
+			update.addPrimaryKeyColumns( new String[]{ identifierColumnName } );
+		}
+		if ( hasIndex && !indexContainsFormula ) {
+			update.addColumns( indexColumnNames );
+		}
+		
+		return update.toStatementString();
 	}
 
 	/**
@@ -166,6 +176,105 @@ public class OneToManyPersister extends AbstractCollectionPersister {
 		return update.addPrimaryKeyColumns( rowSelectColumnNames )
 				.toStatementString();
 	}
+	
+	@Override
+	public void recreate(PersistentCollection collection, Serializable id, SessionImplementor session)
+			throws HibernateException {
+		super.recreate( collection, id, session );
+		writeIndex( collection, id, session );
+	}
+	
+	@Override
+	public void insertRows(PersistentCollection collection, Serializable id, SessionImplementor session)
+			throws HibernateException {
+		super.insertRows( collection, id, session );
+		writeIndex( collection, id, session );
+	}
+	
+	private void writeIndex(PersistentCollection collection, Serializable id, SessionImplementor session) {
+		// If one-to-many and inverse, still need to create the index.  See HHH-5732.
+		if ( isInverse && hasIndex && !indexContainsFormula ) {
+			try {
+				Iterator entries = collection.entries( this );
+				if ( entries.hasNext() ) {
+					Expectation expectation = Expectations.appropriateExpectation( getUpdateCheckStyle() );
+					int i = 0;
+					int count = 0;
+					while ( entries.hasNext() ) {
+
+						final Object entry = entries.next();
+						if ( collection.entryExists( entry, i ) ) {
+							int offset = 1;
+							PreparedStatement st = null;
+							boolean callable = isUpdateCallable();
+							boolean useBatch = expectation.canBeBatched();
+							String sql = getSQLUpdateRowString();
+
+							if ( useBatch ) {
+								if ( recreateBatchKey == null ) {
+									recreateBatchKey = new BasicBatchKey(
+											getRole() + "#RECREATE",
+											expectation
+											);
+								}
+								st = session.getTransactionCoordinator()
+										.getJdbcCoordinator()
+										.getBatch( recreateBatchKey )
+										.getBatchStatement( sql, callable );
+							}
+							else {
+								st = session.getTransactionCoordinator()
+										.getJdbcCoordinator()
+										.getStatementPreparer()
+										.prepareStatement( sql, callable );
+							}
+
+							try {
+								offset += expectation.prepare( st );
+								if ( hasIdentifier ) {
+									offset = writeIdentifier( st, collection.getIdentifier( entry, i ), offset, session );
+								}
+								offset = writeIndex( st, collection.getIndex( entry, i, this ), offset, session );
+								offset = writeElement( st, collection.getElement( entry ), offset, session );
+
+								if ( useBatch ) {
+									session.getTransactionCoordinator()
+											.getJdbcCoordinator()
+											.getBatch( recreateBatchKey )
+											.addToBatch();
+								}
+								else {
+									expectation.verifyOutcome( session.getTransactionCoordinator().getJdbcCoordinator().getResultSetReturn().executeUpdate( st ), st, -1 );
+								}
+								count++;
+							}
+							catch ( SQLException sqle ) {
+								if ( useBatch ) {
+									session.getTransactionCoordinator().getJdbcCoordinator().abortBatch();
+								}
+								throw sqle;
+							}
+							finally {
+								if ( !useBatch ) {
+									session.getTransactionCoordinator().getJdbcCoordinator().release( st );
+								}
+							}
+
+						}
+						i++;
+					}
+				}
+			}
+			catch ( SQLException sqle ) {
+				throw sqlExceptionHelper.convert(
+						sqle,
+						"could not update collection: " +
+								MessageHelper.collectionInfoString( this, collection, id, session ),
+						getSQLUpdateRowString()
+						);
+			}
+		}
+	}
 
 	public boolean consumesEntityAlias() {
 		return true;

hibernate-core/src/test/java/org/hibernate/test/annotations/onetomany/BankAccount.java

@@ -0,0 +1,71 @@
+/* 
+ * Hibernate, Relational Persistence for Idiomatic Java
+ * 
+ * JBoss, Home of Professional Open Source
+ * Copyright 2013 Red Hat Inc. and/or its affiliates and other contributors
+ * as indicated by the @authors tag. All rights reserved.
+ * See the copyright.txt in the distribution for a
+ * full listing of individual contributors.
+ *
+ * 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, v. 2.1.
+ * This program is distributed in the hope that it will be useful, but WITHOUT A
+ * 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,
+ * v.2.1 along with this distribution; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston,
+ * MA  02110-1301, USA.
+ */
+package org.hibernate.test.annotations.onetomany;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import javax.persistence.CascadeType;
+import javax.persistence.Entity;
+import javax.persistence.GeneratedValue;
+import javax.persistence.Id;
+import javax.persistence.OneToMany;
+import javax.persistence.OrderColumn;
+
+/**
+ * @author Brett Meyer
+ */
+@Entity
+public class BankAccount {
+
+	@Id
+	@GeneratedValue
+	private long id;
+
+	@OneToMany(mappedBy = "account", cascade = { CascadeType.ALL })
+	@OrderColumn(name = "transactions_index")
+	private List<Transaction> transactions;
+
+	public long getId() {
+		return id;
+	}
+
+	public void setId(long id) {
+		this.id = id;
+	}
+
+	public List<Transaction> getTransactions() {
+		return transactions;
+	}
+
+	public void setTransactions(List<Transaction> transactions) {
+		this.transactions = transactions;
+	}
+
+	public void addTransaction(String code) {
+		if ( transactions == null ) {
+			transactions = new ArrayList<Transaction>();
+		}
+		Transaction transaction = new Transaction();
+		transaction.setCode( code );
+		transactions.add( transaction );
+	}
+}

hibernate-core/src/test/java/org/hibernate/test/annotations/onetomany/OrderByTest.java

@@ -23,12 +23,19 @@
  */
 package org.hibernate.test.annotations.onetomany;
 
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+
+import java.sql.PreparedStatement;
+import java.sql.ResultSet;
+import java.sql.SQLException;
 import java.util.Arrays;
+import java.util.HashMap;
 import java.util.Iterator;
 import java.util.List;
-
+import java.util.Map;
-import org.junit.Assert;
-import org.junit.Test;
 
 import org.hibernate.Criteria;
 import org.hibernate.NullPrecedence;
@@ -36,15 +43,20 @@ import org.hibernate.Session;
 import org.hibernate.dialect.H2Dialect;
 import org.hibernate.dialect.MySQLDialect;
 import org.hibernate.dialect.Oracle8iDialect;
+import org.hibernate.engine.spi.SessionImplementor;
+import org.hibernate.persister.collection.CollectionPersister;
+import org.hibernate.persister.collection.QueryableCollection;
+import org.hibernate.sql.SimpleSelect;
 import org.hibernate.testing.RequiresDialect;
 import org.hibernate.testing.TestForIssue;
 import org.hibernate.testing.junit4.BaseCoreFunctionalTestCase;
-
+import org.junit.Assert;
-import static org.junit.Assert.assertEquals;
+import org.junit.Test;
 
 /**
  * @author Emmanuel Bernard
  * @author Lukasz Antoniak (lukasz dot antoniak at gmail dot com)
+ * @author Brett Meyer
  */
 public class OrderByTest extends BaseCoreFunctionalTestCase {
 	@Test
@@ -305,11 +317,61 @@ public class OrderByTest extends BaseCoreFunctionalTestCase {
 
 		session.close();
 	}
+	
+	@Test
+	@TestForIssue(jiraKey = "HHH-5732")
+	public void testInverseIndex() {
+		final CollectionPersister transactionsPersister = sessionFactory().getCollectionPersister(
+				BankAccount.class.getName() + ".transactions" );
+		assertTrue( transactionsPersister.isInverse() );
+
+		Session s = openSession();
+		s.getTransaction().begin();
+
+		BankAccount account = new BankAccount();
+		account.addTransaction( "zzzzz" );
+		account.addTransaction( "aaaaa" );
+		account.addTransaction( "mmmmm" );
+		s.save( account );
+		s.getTransaction().commit();
+
+		s.close();
+
+		s = openSession();
+		s.getTransaction().begin();
+		
+		try {
+			final QueryableCollection queryableCollection = (QueryableCollection) transactionsPersister;
+			SimpleSelect select = new SimpleSelect( getDialect() )
+					.setTableName( queryableCollection.getTableName() )
+					.addColumn( "code" )
+					.addColumn( "transactions_index" );
+			PreparedStatement preparedStatement = ((SessionImplementor)s).getTransactionCoordinator().getJdbcCoordinator().getStatementPreparer().prepareStatement( select.toStatementString() );
+			ResultSet resultSet = ((SessionImplementor)s).getTransactionCoordinator().getJdbcCoordinator().getResultSetReturn().extract( preparedStatement );
+			Map<Integer, String> valueMap = new HashMap<Integer, String>();
+			while ( resultSet.next() ) {
+				final String code = resultSet.getString( 1 );
+				assertFalse( "code column was null", resultSet.wasNull() );
+				final int indx = resultSet.getInt( 2 );
+				assertFalse( "List index column was null", resultSet.wasNull() );
+				valueMap.put( indx, code );
+			}
+			assertEquals( 3, valueMap.size() );
+			assertEquals( "zzzzz", valueMap.get( 0 ) );
+			assertEquals( "aaaaa", valueMap.get( 1 ) );
+			assertEquals( "mmmmm", valueMap.get( 2 ) );
+		}
+		catch ( SQLException e ) {
+			fail(e.getMessage());
+		}
+	}
 
 	@Override
 	protected Class[] getAnnotatedClasses() {
 		return new Class[] {
-				Order.class, OrderItem.class, Zoo.class, Tiger.class, Monkey.class, Visitor.class, Box.class, Item.class
+				Order.class, OrderItem.class, Zoo.class, Tiger.class,
+				Monkey.class, Visitor.class, Box.class, Item.class,
+				BankAccount.class, Transaction.class
 		};
 	}
 }

hibernate-core/src/test/java/org/hibernate/test/annotations/onetomany/Transaction.java

@@ -0,0 +1,67 @@
+/* 
+ * Hibernate, Relational Persistence for Idiomatic Java
+ * 
+ * JBoss, Home of Professional Open Source
+ * Copyright 2013 Red Hat Inc. and/or its affiliates and other contributors
+ * as indicated by the @authors tag. All rights reserved.
+ * See the copyright.txt in the distribution for a
+ * full listing of individual contributors.
+ *
+ * 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, v. 2.1.
+ * This program is distributed in the hope that it will be useful, but WITHOUT A
+ * 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,
+ * v.2.1 along with this distribution; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston,
+ * MA  02110-1301, USA.
+ */
+package org.hibernate.test.annotations.onetomany;
+
+import javax.persistence.CascadeType;
+import javax.persistence.Entity;
+import javax.persistence.GeneratedValue;
+import javax.persistence.Id;
+import javax.persistence.ManyToOne;
+
+/**
+ * @author Brett Meyer
+ */
+@Entity
+public class Transaction {
+	
+	@Id
+	@GeneratedValue
+	private long id;
+	
+	private String code;
+	
+	@ManyToOne( cascade = { CascadeType.ALL } )
+	private BankAccount account;
+
+	public long getId() {
+		return id;
+	}
+
+	public void setId(long id) {
+		this.id = id;
+	}
+
+	public String getCode() {
+		return code;
+	}
+
+	public void setCode(String code) {
+		this.code = code;
+	}
+
+	public BankAccount getAccount() {
+		return account;
+	}
+
+	public void setAccount(BankAccount account) {
+		this.account = account;
+	}
+}

hibernate-core/src/test/java/org/hibernate/test/collection/list/PersistentListTest.java

@@ -43,7 +43,6 @@ import org.hibernate.jdbc.Work;
 import org.hibernate.persister.collection.CollectionPersister;
 import org.hibernate.persister.collection.QueryableCollection;
 import org.hibernate.sql.SimpleSelect;
-import org.hibernate.testing.FailureExpected;
 import org.hibernate.testing.TestForIssue;
 import org.hibernate.testing.junit4.BaseCoreFunctionalTestCase;
 import org.junit.Test;
@@ -54,6 +53,7 @@ import org.junit.Test;
  * @author Steve Ebersole
  */
 public class PersistentListTest extends BaseCoreFunctionalTestCase {
+	
 	@Override
 	public String[] getMappings() {
 		return new String[] { "collection/list/Mappings.hbm.xml" };
@@ -61,8 +61,7 @@ public class PersistentListTest extends BaseCoreFunctionalTestCase {
 
 	@Test
 	@TestForIssue( jiraKey = "HHH-5732"  )
-	@FailureExpected( jiraKey = "HHH-5732" )
+	public void testInverseListIndex() {
-	public void testInverseListIndexColumnWritten() {
 		// make sure no one changes the mapping
 		final CollectionPersister collectionPersister = sessionFactory().getCollectionPersister( ListOwner.class.getName() + ".children" );
 		assertTrue( collectionPersister.isInverse() );
@@ -123,8 +122,7 @@ public class PersistentListTest extends BaseCoreFunctionalTestCase {
 
 	@Test
 	@TestForIssue( jiraKey = "HHH-5732"  )
-	@FailureExpected( jiraKey = "HHH-5732" )
+	public void testInverseListIndex2() {
-	public void testInverseListIndexColumnWritten2() {
 		// make sure no one changes the mapping
 		final CollectionPersister collectionPersister = sessionFactory().getCollectionPersister( Order.class.getName() + ".lineItems" );
 		assertTrue( collectionPersister.isInverse() );