HHH-18830 fix extraneous updates for unowned List with @OrderColumn

Signed-off-by: Gavin King <gavin@hibernate.org>

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

@@ -22,8 +22,6 @@
 import org.hibernate.engine.jdbc.mutation.spi.MutationExecutorService;
 import org.hibernate.engine.spi.SharedSessionContractImplementor;
 import org.hibernate.internal.FilterAliasGenerator;
-import org.hibernate.internal.util.NullnessHelper;
-import org.hibernate.internal.util.collections.ArrayHelper;
 import org.hibernate.jdbc.Expectations;
 import org.hibernate.mapping.Collection;
 import org.hibernate.metamodel.mapping.CollectionPart;
@@ -74,6 +72,8 @@
 import org.hibernate.sql.model.internal.TableUpdateStandard;
 import org.hibernate.sql.model.jdbc.JdbcMutationOperation;
 
+import static org.hibernate.internal.util.NullnessHelper.areAllNonNull;
+import static org.hibernate.internal.util.collections.ArrayHelper.isAnyTrue;
 import static org.hibernate.internal.util.collections.CollectionHelper.arrayList;
 import static org.hibernate.sql.model.ModelMutationLogging.MODEL_MUTATION_LOGGER;
 import static org.hibernate.sql.model.ast.builder.TableUpdateBuilder.NULL;
@@ -99,6 +99,8 @@ public class OneToManyPersister extends AbstractCollectionPersister {
 	private final boolean keyIsNullable;
 	private final MutationExecutorService mutationExecutorService;
 
+	final boolean doWriteEvenWhenInverse; // contrary to intent of JPA
+
 	public OneToManyPersister(
 			Collection collectionBinding,
 			CollectionDataAccess cacheAccessStrategy,
@@ -106,13 +108,19 @@ public OneToManyPersister(
 		super( collectionBinding, cacheAccessStrategy, creationContext );
 		keyIsNullable = collectionBinding.getKey().isNullable();
 
-		this.rowMutationOperations = buildRowMutationOperations();
+		doWriteEvenWhenInverse = isInverse
+				&& hasIndex()
+				&& !indexContainsFormula
+				&& isAnyTrue( indexColumnIsSettable )
+				&& !getElementPersisterInternal().managesColumns( indexColumnNames );
 
-		this.insertRowsCoordinator = buildInsertCoordinator();
+		rowMutationOperations = buildRowMutationOperations();
-		this.updateRowsCoordinator = buildUpdateCoordinator();
+
-		this.deleteRowsCoordinator = buildDeleteCoordinator();
+		insertRowsCoordinator = buildInsertCoordinator();
-		this.removeCoordinator = buildDeleteAllCoordinator();
+		updateRowsCoordinator = buildUpdateCoordinator();
-		this.mutationExecutorService = creationContext.getServiceRegistry().getService(	MutationExecutorService.class );
+		deleteRowsCoordinator = buildDeleteCoordinator();
+		removeCoordinator = buildDeleteAllCoordinator();
+		mutationExecutorService = creationContext.getServiceRegistry().getService(	MutationExecutorService.class );
 	}
 
 	@Override
@@ -179,66 +187,59 @@ private void writeIndex(
 			Object key,
 			boolean resetIndex,
 			SharedSessionContractImplementor session) {
-		if ( !entries.hasNext() ) {
+		// See HHH-5732 and HHH-18830.
-			// no entries to update
+		// Claim: "If one-to-many and inverse, still need to create the index."
-			return;
+		// In fact this is wrong: JPA is very clear that bidirectional associations
-		}
+		// are persisted from the owning side. However, since this is a very ancient
+		// mistake, I have fixed it in a backward-compatible way, by writing to the
+		// order column if there is no mapping at all for it on the other side.
+		// But if the owning entity does have a mapping for the order column, don't
+		// do superfluous SQL UPDATEs here from the unowned side, no matter how many
+		// users complain.
+		if ( doWriteEvenWhenInverse && entries.hasNext() ) {
 
-		// If one-to-many and inverse, still need to create the index.  See HHH-5732.
+			final JdbcMutationOperation updateRowOperation = rowMutationOperations.getUpdateRowOperation();
-		final boolean doWrite = isInverse
+			final RowMutationOperations.Values updateRowValues = rowMutationOperations.getUpdateRowValues();
-				&& hasIndex()
+			final RowMutationOperations.Restrictions updateRowRestrictions = rowMutationOperations.getUpdateRowRestrictions();
-				&& !indexContainsFormula
+			assert areAllNonNull( updateRowOperation, updateRowValues, updateRowRestrictions );
-				&& ArrayHelper.countTrue( indexColumnIsSettable ) > 0;
-		if ( !doWrite ) {
-			return;
-		}
 
-		final JdbcMutationOperation updateRowOperation = rowMutationOperations.getUpdateRowOperation();
+			final MutationExecutor mutationExecutor = mutationExecutorService.createExecutor(
-		final RowMutationOperations.Values updateRowValues = rowMutationOperations.getUpdateRowValues();
+					() -> new BasicBatchKey( getNavigableRole() + "#INDEX" ),
-		final RowMutationOperations.Restrictions updateRowRestrictions = rowMutationOperations.getUpdateRowRestrictions();
+					MutationOperationGroupFactory.singleOperation( MutationType.UPDATE, this, updateRowOperation ),
-		assert NullnessHelper.areAllNonNull( updateRowOperation, updateRowValues, updateRowRestrictions );
+					session
+			);
 
-		final MutationExecutor mutationExecutor = mutationExecutorService.createExecutor(
+			final JdbcValueBindings jdbcValueBindings = mutationExecutor.getJdbcValueBindings();
-				() -> new BasicBatchKey( getNavigableRole() + "#INDEX" ),
+			try {
-				MutationOperationGroupFactory.singleOperation( MutationType.UPDATE, this, updateRowOperation ),
+				int nextIndex = ( resetIndex ? 0 : getSize( key, session ) ) +
-				session
+						Math.max( getAttributeMapping().getIndexMetadata().getListIndexBase(), 0 );
-		);
+				while ( entries.hasNext() ) {
-
+					final Object entry = entries.next();
-		final JdbcValueBindings jdbcValueBindings = mutationExecutor.getJdbcValueBindings();
+					if ( entry != null && collection.entryExists( entry, nextIndex ) ) {
-
+						updateRowValues.applyValues(
-		try {
+								collection,
-			int nextIndex = ( resetIndex ? 0 : getSize( key, session ) ) +
+								key,
-					Math.max( getAttributeMapping().getIndexMetadata().getListIndexBase(), 0 );
+								entry,
-
+								nextIndex,
-			while ( entries.hasNext() ) {
+								session,
-				final Object entry = entries.next();
+								jdbcValueBindings
-
+						);
-				if ( entry != null && collection.entryExists( entry, nextIndex ) ) {
+						updateRowRestrictions.applyRestrictions(
-					updateRowValues.applyValues(
+								collection,
-							collection,
+								key,
-							key,
+								entry,
-							entry,
+								nextIndex,
-							nextIndex,
+								session,
-							session,
+								jdbcValueBindings
-							jdbcValueBindings
+						);
-					);
+						mutationExecutor.execute( collection, null, null, null, session );
-
+						nextIndex++;
-					updateRowRestrictions.applyRestrictions(
+					}
-							collection,
-							key,
-							entry,
-							nextIndex,
-							session,
-							jdbcValueBindings
-					);
-
-					mutationExecutor.execute( collection, null, null, null, session );
-					nextIndex++;
 				}
 			}
-		}
+			finally {
-		finally {
+				mutationExecutor.release();
-			mutationExecutor.release();
+			}
 		}
 
 	}
@@ -368,11 +369,7 @@ private RowMutationOperations buildRowMutationOperations() {
 		final OperationProducer writeIndexOperationProducer;
 		final RowMutationOperations.Values writeIndexValues;
 		final RowMutationOperations.Restrictions writeIndexRestrictions;
-		final boolean needsWriteIndex = isInverse
+		if ( doWriteEvenWhenInverse ) {
-				&& hasIndex()
-				&& !indexContainsFormula
-				&& !ArrayHelper.isAllFalse( indexColumnIsSettable );
-		if ( needsWriteIndex ) {
 			writeIndexOperationProducer = this::generateWriteIndexOperation;
 			writeIndexValues = this::applyWriteIndexValues;
 			writeIndexRestrictions = this::applyWriteIndexRestrictions;

hibernate-core/src/main/java/org/hibernate/persister/entity/AbstractEntityPersister.java

@@ -299,6 +299,7 @@
 import static org.hibernate.internal.util.StringHelper.isEmpty;
 import static org.hibernate.internal.util.StringHelper.qualifyConditionally;
 import static org.hibernate.internal.util.collections.ArrayHelper.contains;
+import static org.hibernate.internal.util.collections.ArrayHelper.isAllTrue;
 import static org.hibernate.internal.util.collections.ArrayHelper.to2DStringArray;
 import static org.hibernate.internal.util.collections.ArrayHelper.toIntArray;
 import static org.hibernate.internal.util.collections.ArrayHelper.toObjectArray;
@@ -6193,4 +6194,28 @@ public String getDiscriminatorAlias() {
 	protected String getSqlWhereStringTableExpression(){
 		return sqlWhereStringTableExpression;
 	}
+
+	@Override
+	public boolean managesColumns(String[] columnNames) {
+		for (String columnName : columnNames) {
+			if ( !writesToColumn( columnName ) ) {
+				return false;
+			}
+		}
+		return true;
+	}
+
+	private boolean writesToColumn(String columnName) {
+		if ( contains( rootTableKeyColumnNames, columnName ) ) {
+			return true;
+		}
+		for ( int i = 0; i < propertyColumnNames.length; i++ ) {
+			if ( contains( propertyColumnNames[i], columnName )
+					&& isAllTrue( propertyColumnInsertable[i] )
+					&& isAllTrue( propertyColumnUpdateable[i] ) ) {
+				return true;
+			}
+		}
+		return false;
+	}
 }

hibernate-core/src/main/java/org/hibernate/persister/entity/EntityPersister.java

@@ -14,6 +14,7 @@
 import org.checkerframework.checker.nullness.qual.Nullable;
 import org.hibernate.HibernateException;
 import org.hibernate.Incubating;
+import org.hibernate.Internal;
 import org.hibernate.LockMode;
 import org.hibernate.LockOptions;
 import org.hibernate.MappingException;
@@ -1485,4 +1486,7 @@ default String getSelectByUniqueKeyString(String[] propertyNames) {
 	boolean isSharedColumn(String columnExpression);
 
 	String[][] getConstraintOrderedTableKeyColumnClosure();
+
+	@Internal
+	boolean managesColumns(String[] columnNames);
 }