From ad6c4d06b0f60e68b218e8d83e8f28e02d68b8cd Mon Sep 17 00:00:00 2001 From: Brett Meyer Date: Wed, 6 Mar 2013 17:56:43 -0500 Subject: [PATCH] HHH-5732 @OrderColumn not updated if @OneToMany has mappedby defined --- .../AbstractCollectionPersister.java | 12 +- .../collection/OneToManyPersister.java | 115 +++++++++++++++++- .../annotations/onetomany/BankAccount.java | 71 +++++++++++ .../annotations/onetomany/OrderByTest.java | 74 ++++++++++- .../annotations/onetomany/Transaction.java | 67 ++++++++++ .../collection/list/PersistentListTest.java | 8 +- 6 files changed, 325 insertions(+), 22 deletions(-) create mode 100644 hibernate-core/src/test/java/org/hibernate/test/annotations/onetomany/BankAccount.java create mode 100644 hibernate-core/src/test/java/org/hibernate/test/annotations/onetomany/Transaction.java diff --git a/hibernate-core/src/main/java/org/hibernate/persister/collection/AbstractCollectionPersister.java b/hibernate-core/src/main/java/org/hibernate/persister/collection/AbstractCollectionPersister.java index 89716a6e21..9e6a054b67 100644 --- a/hibernate-core/src/main/java/org/hibernate/persister/collection/AbstractCollectionPersister.java +++ b/hibernate-core/src/main/java/org/hibernate/persister/collection/AbstractCollectionPersister.java @@ -32,8 +32,6 @@ 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.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.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.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 QueryableCollection 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 void remove(Serializable id, SessionImplementor session) throws Hibernate } - private BasicBatchKey recreateBatchKey; + protected BasicBatchKey recreateBatchKey; public void recreate(PersistentCollection collection, Serializable id, SessionImplementor session) throws HibernateException { diff --git a/hibernate-core/src/main/java/org/hibernate/persister/collection/OneToManyPersister.java b/hibernate-core/src/main/java/org/hibernate/persister/collection/OneToManyPersister.java index 93da4d857b..6cec2e9259 100644 --- a/hibernate-core/src/main/java/org/hibernate/persister/collection/OneToManyPersister.java +++ b/hibernate-core/src/main/java/org/hibernate/persister/collection/OneToManyPersister.java @@ -29,6 +29,7 @@ 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.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 @@ * Collection persister for one-to-many associations. * * @author Gavin King + * @author Brett Meyer */ public class OneToManyPersister extends AbstractCollectionPersister { @@ -135,11 +136,20 @@ protected String generateInsertRowString() { } /** - * 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 @@ protected String generateDeleteRowString() { 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; diff --git a/hibernate-core/src/test/java/org/hibernate/test/annotations/onetomany/BankAccount.java b/hibernate-core/src/test/java/org/hibernate/test/annotations/onetomany/BankAccount.java new file mode 100644 index 0000000000..f75cc75067 --- /dev/null +++ b/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 transactions; + + public long getId() { + return id; + } + + public void setId(long id) { + this.id = id; + } + + public List getTransactions() { + return transactions; + } + + public void setTransactions(List transactions) { + this.transactions = transactions; + } + + public void addTransaction(String code) { + if ( transactions == null ) { + transactions = new ArrayList(); + } + Transaction transaction = new Transaction(); + transaction.setCode( code ); + transactions.add( transaction ); + } +} diff --git a/hibernate-core/src/test/java/org/hibernate/test/annotations/onetomany/OrderByTest.java b/hibernate-core/src/test/java/org/hibernate/test/annotations/onetomany/OrderByTest.java index 3bc9fcd100..4f6366a307 100644 --- a/hibernate-core/src/test/java/org/hibernate/test/annotations/onetomany/OrderByTest.java +++ b/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 org.junit.Assert; -import org.junit.Test; +import java.util.Map; import org.hibernate.Criteria; import org.hibernate.NullPrecedence; @@ -36,15 +43,20 @@ 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 static org.junit.Assert.assertEquals; +import org.junit.Assert; +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 void testOrderByReferencingFormulaColumn() { 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 valueMap = new HashMap(); + 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 }; } } diff --git a/hibernate-core/src/test/java/org/hibernate/test/annotations/onetomany/Transaction.java b/hibernate-core/src/test/java/org/hibernate/test/annotations/onetomany/Transaction.java new file mode 100644 index 0000000000..788f7a9889 --- /dev/null +++ b/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; + } +} diff --git a/hibernate-core/src/test/java/org/hibernate/test/collection/list/PersistentListTest.java b/hibernate-core/src/test/java/org/hibernate/test/collection/list/PersistentListTest.java index c4bbb4257b..d811d44fcd 100644 --- a/hibernate-core/src/test/java/org/hibernate/test/collection/list/PersistentListTest.java +++ b/hibernate-core/src/test/java/org/hibernate/test/collection/list/PersistentListTest.java @@ -43,7 +43,6 @@ 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 @@ * @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 String[] getMappings() { @Test @TestForIssue( jiraKey = "HHH-5732" ) - @FailureExpected( jiraKey = "HHH-5732" ) - public void testInverseListIndexColumnWritten() { + public void testInverseListIndex() { // make sure no one changes the mapping final CollectionPersister collectionPersister = sessionFactory().getCollectionPersister( ListOwner.class.getName() + ".children" ); assertTrue( collectionPersister.isInverse() ); @@ -123,8 +122,7 @@ public void execute(Connection connection) throws SQLException { @Test @TestForIssue( jiraKey = "HHH-5732" ) - @FailureExpected( jiraKey = "HHH-5732" ) - public void testInverseListIndexColumnWritten2() { + public void testInverseListIndex2() { // make sure no one changes the mapping final CollectionPersister collectionPersister = sessionFactory().getCollectionPersister( Order.class.getName() + ".lineItems" ); assertTrue( collectionPersister.isInverse() );