HHH-19925 - Locking root(s) should be based on select-clause, not from-clause

sebersole · sebersole · commit d3b777fae6c8 · 2025-11-15T08:04:40.000-07:00
diff --git a/hibernate-community-dialects/src/main/java/org/hibernate/community/dialect/DerbyDialect.java b/hibernate-community-dialects/src/main/java/org/hibernate/community/dialect/DerbyDialect.java
@@ -59,6 +59,7 @@
 import org.hibernate.query.sqm.mutation.spi.SqmMultiTableInsertStrategy;
 import org.hibernate.query.sqm.mutation.spi.SqmMultiTableMutationStrategy;
 import org.hibernate.service.ServiceRegistry;
+import org.hibernate.spi.NavigablePath;
 import org.hibernate.sql.ast.SqlAstNodeRenderingMode;
 import org.hibernate.sql.ast.SqlAstTranslator;
 import org.hibernate.sql.ast.SqlAstTranslatorFactory;
@@ -85,6 +86,7 @@
 import java.sql.SQLException;
 import java.sql.Types;
 import java.util.Locale;
+import java.util.Set;
 
 import static org.hibernate.type.SqlTypes.BINARY;
 import static org.hibernate.type.SqlTypes.BLOB;
@@ -564,8 +566,9 @@ public LockingSupport getLockingSupport() {
 	protected LockingClauseStrategy buildLockingClauseStrategy(
 			PessimisticLockKind lockKind,
 			RowLockStrategy rowLockStrategy,
-			LockOptions lockOptions) {
-		return new DerbyLockingClauseStrategy( this, lockKind, rowLockStrategy, lockOptions );
+			LockOptions lockOptions,
+			Set<NavigablePath> rootPathsForLocking) {
+		return new DerbyLockingClauseStrategy( this, lockKind, rowLockStrategy, lockOptions, rootPathsForLocking );
 	}
 
 	@Override
diff --git a/hibernate-community-dialects/src/main/java/org/hibernate/community/dialect/DerbyLegacyDialect.java b/hibernate-community-dialects/src/main/java/org/hibernate/community/dialect/DerbyLegacyDialect.java
@@ -58,6 +58,7 @@
 import org.hibernate.query.sqm.mutation.spi.SqmMultiTableInsertStrategy;
 import org.hibernate.query.sqm.mutation.spi.SqmMultiTableMutationStrategy;
 import org.hibernate.service.ServiceRegistry;
+import org.hibernate.spi.NavigablePath;
 import org.hibernate.sql.ast.SqlAstNodeRenderingMode;
 import org.hibernate.sql.ast.SqlAstTranslator;
 import org.hibernate.sql.ast.SqlAstTranslatorFactory;
@@ -85,6 +86,7 @@
 import java.sql.DatabaseMetaData;
 import java.sql.SQLException;
 import java.sql.Types;
+import java.util.Set;
 
 import static org.hibernate.type.SqlTypes.BINARY;
 import static org.hibernate.type.SqlTypes.BLOB;
@@ -551,8 +553,9 @@ public boolean supportsCommentOn() {
 	protected LockingClauseStrategy buildLockingClauseStrategy(
 			PessimisticLockKind lockKind,
 			RowLockStrategy rowLockStrategy,
-			LockOptions lockOptions) {
-		return new DerbyLockingClauseStrategy( this, lockKind, rowLockStrategy, lockOptions );
+			LockOptions lockOptions,
+			Set<NavigablePath> rootPathsForLocking) {
+		return new DerbyLockingClauseStrategy( this, lockKind, rowLockStrategy, lockOptions, rootPathsForLocking );
 	}
 
 	@Override
diff --git a/hibernate-community-dialects/src/main/java/org/hibernate/community/dialect/DerbyLockingClauseStrategy.java b/hibernate-community-dialects/src/main/java/org/hibernate/community/dialect/DerbyLockingClauseStrategy.java
@@ -7,10 +7,13 @@
 import org.hibernate.LockOptions;
 import org.hibernate.dialect.Dialect;
 import org.hibernate.dialect.RowLockStrategy;
+import org.hibernate.spi.NavigablePath;
 import org.hibernate.sql.ast.internal.PessimisticLockKind;
 import org.hibernate.sql.ast.internal.StandardLockingClauseStrategy;
 import org.hibernate.sql.ast.spi.SqlAppender;
 
+import java.util.Set;
+
 /**
  * StandardLockingClauseStrategy subclass, specific for Derby.
  *
@@ -21,8 +24,9 @@ public DerbyLockingClauseStrategy(
 			Dialect dialect,
 			PessimisticLockKind lockKind,
 			RowLockStrategy rowLockStrategy,
-			LockOptions lockOptions) {
-		super( dialect, lockKind, rowLockStrategy, lockOptions );
+			LockOptions lockOptions,
+			Set<NavigablePath> rootPathsForLocking) {
+		super( dialect, lockKind, rowLockStrategy, lockOptions, rootPathsForLocking );
 	}
 
 	@Override
diff --git a/hibernate-community-dialects/src/main/java/org/hibernate/community/dialect/TeradataDialect.java b/hibernate-community-dialects/src/main/java/org/hibernate/community/dialect/TeradataDialect.java
@@ -40,6 +40,7 @@
 import org.hibernate.query.sqm.mutation.internal.temptable.GlobalTemporaryTableMutationStrategy;
 import org.hibernate.query.sqm.mutation.spi.SqmMultiTableInsertStrategy;
 import org.hibernate.query.sqm.mutation.spi.SqmMultiTableMutationStrategy;
+import org.hibernate.spi.NavigablePath;
 import org.hibernate.sql.ForUpdateFragment;
 import org.hibernate.sql.ast.SqlAstTranslator;
 import org.hibernate.sql.ast.SqlAstTranslatorFactory;
@@ -65,6 +66,7 @@
 import java.sql.SQLException;
 import java.sql.Types;
 import java.util.Map;
+import java.util.Set;
 
 import static org.hibernate.exception.spi.TemplatedViolatedConstraintNameExtractor.extractUsingTemplate;
 import static org.hibernate.type.SqlTypes.BIGINT;
@@ -529,13 +531,14 @@ public LockingSupport getLockingSupport() {
 	protected LockingClauseStrategy buildLockingClauseStrategy(
 			PessimisticLockKind lockKind,
 			RowLockStrategy rowLockStrategy,
-			LockOptions lockOptions) {
+			LockOptions lockOptions,
+			Set<NavigablePath> rootPathsForLocking) {
 		if ( getVersion().isBefore( 14 ) ) {
 			return NonLockingClauseStrategy.NON_CLAUSE_STRATEGY;
 		}
 		// we'll reuse the StandardLockingClauseStrategy for the collecting
 		// aspect and just handle the special rendering in the SQL AST translator
-		return super.buildLockingClauseStrategy( lockKind, rowLockStrategy, lockOptions );
+		return super.buildLockingClauseStrategy( lockKind, rowLockStrategy, lockOptions, rootPathsForLocking );
 	}
 
 	@Override
diff --git a/hibernate-core/src/main/java/org/hibernate/dialect/Dialect.java b/hibernate-core/src/main/java/org/hibernate/dialect/Dialect.java
@@ -114,6 +114,7 @@
 import org.hibernate.query.sqm.sql.SqmTranslatorFactory;
 import org.hibernate.service.ServiceRegistry;
 import org.hibernate.service.spi.ServiceRegistryImplementor;
+import org.hibernate.spi.NavigablePath;
 import org.hibernate.sql.ForUpdateFragment;
 import org.hibernate.sql.ast.SqlAstNodeRenderingMode;
 import org.hibernate.sql.ast.SqlAstTranslatorFactory;
@@ -2369,14 +2370,15 @@ public LockingClauseStrategy getLockingClauseStrategy(QuerySpec querySpec, LockO
 			default -> throw new IllegalStateException( "Should never happen due to checks above" );
 		}
 
-		return buildLockingClauseStrategy( lockKind, rowLockStrategy, lockOptions );
+		return buildLockingClauseStrategy( lockKind, rowLockStrategy, lockOptions, querySpec.getRootPathsForLocking() );
 	}
 
 	protected LockingClauseStrategy buildLockingClauseStrategy(
 			PessimisticLockKind lockKind,
 			RowLockStrategy rowLockStrategy,
-			LockOptions lockOptions) {
-		return new StandardLockingClauseStrategy( this, lockKind, rowLockStrategy, lockOptions );
+			LockOptions lockOptions,
+			Set<NavigablePath> rootPathsForLocking) {
+		return new StandardLockingClauseStrategy( this, lockKind, rowLockStrategy, lockOptions, rootPathsForLocking );
 	}
 
 	/**
diff --git a/hibernate-core/src/main/java/org/hibernate/dialect/lock/internal/SqlAstBasedLockingStrategy.java b/hibernate-core/src/main/java/org/hibernate/dialect/lock/internal/SqlAstBasedLockingStrategy.java
@@ -73,8 +73,11 @@ public void lock(
 		lockOptions.setScope( lockScope );
 		lockOptions.setTimeOut( timeout );
 
-		final var rootQuerySpec = new QuerySpec( true );
 		final var entityPath = new NavigablePath( entityToLock.getRootPathName() );
+
+		final var rootQuerySpec = new QuerySpec( true );
+		rootQuerySpec.applyRootPathForLocking( entityPath );
+
 		final var idMapping = entityToLock.getIdentifierMapping();
 
 		// NOTE: there are 2 possible ways to handle the select list for the query...
@@ -154,7 +157,7 @@ public void lock(
 			);
 		}
 
-		final var selectStatement = new SelectStatement( rootQuerySpec, List.of( idResult ), List.of( entityPath ) );
+		final var selectStatement = new SelectStatement( rootQuerySpec, List.of( idResult ) );
 		final JdbcSelect selectOperation =
 				session.getDialect().getSqlAstTranslatorFactory()
 						.buildSelectTranslator( factory, selectStatement )
diff --git a/hibernate-core/src/main/java/org/hibernate/loader/ast/internal/AbstractNaturalIdLoader.java b/hibernate-core/src/main/java/org/hibernate/loader/ast/internal/AbstractNaturalIdLoader.java
@@ -215,7 +215,7 @@ public Object resolveNaturalIdToId(Object naturalIdValue, SharedSessionContractI
 		return executeNaturalIdQuery(
 				naturalIdValue,
 				LockOptions.NONE,
-				new SelectStatement( rootQuerySpec, singletonList( domainResult ), List.of() ),
+				new SelectStatement( rootQuerySpec, singletonList( domainResult ) ),
 				rootTableGroup,
 				rootQuerySpec::applyPredicate,
 				sqlAstCreationState,
diff --git a/hibernate-core/src/main/java/org/hibernate/loader/ast/internal/DatabaseSnapshotExecutor.java b/hibernate-core/src/main/java/org/hibernate/loader/ast/internal/DatabaseSnapshotExecutor.java
@@ -128,7 +128,7 @@ class DatabaseSnapshotExecutor {
 				}
 		);
 
-		final var selectStatement = new SelectStatement( rootQuerySpec, domainResults, List.of() );
+		final var selectStatement = new SelectStatement( rootQuerySpec, domainResults );
 		jdbcSelect =
 				sessionFactory.getJdbcServices().getJdbcEnvironment().getSqlAstTranslatorFactory()
 						.buildSelectTranslator( sessionFactory, selectStatement )
diff --git a/hibernate-core/src/main/java/org/hibernate/loader/ast/internal/LoaderSelectBuilder.java b/hibernate-core/src/main/java/org/hibernate/loader/ast/internal/LoaderSelectBuilder.java
@@ -148,6 +148,8 @@ public static SelectStatement createSelectBySingleArrayParameter(
 		final var sqlAstCreationState = builder.createSqlAstCreationState( rootQuerySpec );
 
 		final var rootNavigablePath = new NavigablePath( loadable.getRootPathName() );
+		rootQuerySpec.applyRootPathForLocking( rootNavigablePath );
+
 		final var rootTableGroup =
 				builder.buildRootTableGroup( rootNavigablePath, rootQuerySpec, sqlAstCreationState );
 
@@ -177,7 +179,7 @@ public static SelectStatement createSelectBySingleArrayParameter(
 			builder.applyFiltering( rootQuerySpec, rootTableGroup, (Restrictable) loadable, sqlAstCreationState );
 		}
 
-		return new SelectStatement( rootQuerySpec, domainResults, List.of( rootNavigablePath ) );
+		return new SelectStatement( rootQuerySpec, domainResults );
 	}
 
 	private static void applyArrayParamRestriction(
@@ -464,6 +466,8 @@ private SelectStatement generateSelect() {
 		final var rootNavigablePath = new NavigablePath( loadable.getRootPathName() );
 
 		final var rootQuerySpec = new QuerySpec( true );
+		rootQuerySpec.applyRootPathForLocking( rootNavigablePath );
+
 		final var sqlAstCreationState = createSqlAstCreationState( rootQuerySpec );
 
 		final var rootTableGroup = buildRootTableGroup( rootNavigablePath, rootQuerySpec, sqlAstCreationState );
@@ -505,7 +509,7 @@ else if ( cachedDomainResult != null ) {
 			applyFiltering( rootQuerySpec, rootTableGroup, (Restrictable) loadable, sqlAstCreationState );
 		}
 
-		return new SelectStatement( rootQuerySpec, domainResults, List.of( rootNavigablePath ) );
+		return new SelectStatement( rootQuerySpec, domainResults );
 	}
 
 	private List<DomainResult<?>> buildRequestedDomainResults(
@@ -980,12 +984,11 @@ private SelectStatement generateSelect(SubselectFetch subselect) {
 		//		- so `loadable` is the owner entity-descriptor and the `partsToSelect` is the collection
 
 		assert loadable instanceof PluralAttributeMapping;
-
 		final var attributeMapping = (PluralAttributeMapping) loadable;
 
-		final var rootQuerySpec = new QuerySpec( true );
-
 		final var rootNavigablePath = new NavigablePath( loadable.getRootPathName() );
+		final var rootQuerySpec = new QuerySpec( true );
+		rootQuerySpec.applyRootPathForLocking( rootNavigablePath );
 
 		// We need to initialize the acronymMap based on subselect.getLoadingSqlAst() to avoid alias collisions
 		final var tableReferences = AliasCollector.getTableReferences( subselect.getLoadingSqlAst() );
@@ -1024,8 +1027,7 @@ private SelectStatement generateSelect(SubselectFetch subselect) {
 								rootTableGroup,
 								sqlAstCreationState
 						)
-				),
-				singletonList( rootNavigablePath )
+				)
 		);
 	}
 
diff --git a/hibernate-core/src/main/java/org/hibernate/query/sqm/mutation/internal/MatchingIdSelectionHelper.java b/hibernate-core/src/main/java/org/hibernate/query/sqm/mutation/internal/MatchingIdSelectionHelper.java
@@ -124,7 +124,7 @@ public static SelectStatement generateMatchingIdSelectStatement(
 				sqmConverter
 		);
 
-		return new SelectStatement( idSelectionQuery, domainResults, List.of() );
+		return new SelectStatement( idSelectionQuery, domainResults );
 	}
 
 	/// Generates a query-spec for selecting all ids matching the restriction defined as part
diff --git a/hibernate-core/src/main/java/org/hibernate/query/sqm/mutation/internal/cte/AbstractCteMutationHandler.java b/hibernate-core/src/main/java/org/hibernate/query/sqm/mutation/internal/cte/AbstractCteMutationHandler.java
@@ -138,7 +138,7 @@ public AbstractCteMutationHandler(
 		// Create the main query spec that will return the count of
 		final QuerySpec querySpec = new QuerySpec( true, 1 );
 		final List<DomainResult<?>> domainResults = new ArrayList<>( 1 );
-		final SelectStatement statement = new SelectStatement( querySpec, domainResults, List.of() );
+		final SelectStatement statement = new SelectStatement( querySpec, domainResults );
 		final JdbcServices jdbcServices = factory.getJdbcServices();
 		final SqlAstTranslator<JdbcOperationQuerySelect> translator = jdbcServices.getJdbcEnvironment()
 				.getSqlAstTranslatorFactory()
diff --git a/hibernate-core/src/main/java/org/hibernate/query/sqm/mutation/internal/cte/CteInsertHandler.java b/hibernate-core/src/main/java/org/hibernate/query/sqm/mutation/internal/cte/CteInsertHandler.java
@@ -291,7 +291,7 @@ public CteInsertHandler(
 		// Create the main query spec that will return the count of rows
 		final QuerySpec querySpec = new QuerySpec( true, 1 );
 		final List<DomainResult<?>> domainResults = new ArrayList<>( 1 );
-		final SelectStatement statement = new SelectStatement( querySpec, domainResults, List.of() );
+		final SelectStatement statement = new SelectStatement( querySpec, domainResults );
 
 		final CteStatement entityCte;
 		if ( additionalInsertValues.requiresRowNumberIntermediate() ) {
diff --git a/hibernate-core/src/main/java/org/hibernate/query/sqm/mutation/internal/temptable/TableBasedInsertHandler.java b/hibernate-core/src/main/java/org/hibernate/query/sqm/mutation/internal/temptable/TableBasedInsertHandler.java
@@ -593,8 +593,7 @@ private RootTableInserter createRootTableInserter(
 									null,
 									false
 							)
-					),
-					List.of()
+					)
 			);
 			temporaryTableIdentitySelect = jdbcServices.getJdbcEnvironment()
 					.getSqlAstTranslatorFactory()
diff --git a/hibernate-core/src/main/java/org/hibernate/query/sqm/sql/BaseSqmToSqlAstConverter.java b/hibernate-core/src/main/java/org/hibernate/query/sqm/sql/BaseSqmToSqlAstConverter.java
@@ -1640,12 +1640,12 @@ public SelectStatement visitSelectStatement(SqmSelectStatement<?> statement) {
 		final QueryPart queryPart = visitQueryPart( statement.getQueryPart() );
 		final List<DomainResult<?>> domainResults = queryPart.isRoot() ? this.domainResults : emptyList();
 		try {
-			return new SelectStatement( cteContainer, queryPart, domainResults, rootPathsForLocking );
+			return new SelectStatement( cteContainer, queryPart, domainResults );
 		}
 		finally {
 			this.currentSqmStatement = oldSqmStatement;
 			this.cteContainer = oldCteContainer;
-			rootPathsForLocking = null;
+			rootPathsForLockingCollector = null;
 		}
 	}
 
@@ -1754,7 +1754,7 @@ public CteStatement visitCteStatement(SqmCteStatement<?> sqmCteStatement) {
 
 								final CteStatement cteStatement = new CteStatement(
 										cteTable,
-										new SelectStatement( subCteContainer, group, emptyList(), emptyList() ),
+										new SelectStatement( subCteContainer, group, emptyList() ),
 										sqmCteStatement.getMaterialization(),
 										sqmCteStatement.getSearchClauseKind(),
 										visitSearchBySpecifications( cteTable, sqmCteStatement.getSearchBySpecifications() ),
@@ -2206,21 +2206,18 @@ private TableGroup findTableGroupByPath(NavigablePath navigablePath) {
 		return getFromClauseAccess().getTableGroup( navigablePath );
 	}
 
-	private List<NavigablePath> rootPathsForLocking;
 	private Consumer<NavigablePath> rootPathsForLockingCollector;
 
 	@Override
 	public SelectClause visitSelectClause(SqmSelectClause selectClause) {
 		currentClauseStack.push( Clause.SELECT );
 		try {
 			final SelectClause sqlSelectClause = currentQuerySpec().getSelectClause();
-			if ( sqmQueryPartStack.depth() == 1 ) {
-				rootPathsForLockingCollector = (navigablePath) -> {
-					if ( rootPathsForLocking == null ) {
-						rootPathsForLocking = new ArrayList<>();
-					}
-					rootPathsForLocking.add(  navigablePath );
-				};
+			if ( sqmQueryPartStack.depth() == 1 && currentClauseStack.depth() == 1 ) {
+				// these 2 conditions combined *should* indicate we have the
+				// root query-spec of a top-level select statement
+				rootPathsForLockingCollector = (path) ->
+						currentQuerySpec().applyRootPathForLocking( path );
 			}
 			if ( selectClause == null ) {
 				final SqmFrom<?, ?> implicitSelection = determineImplicitSelection( (SqmQuerySpec<?>) getCurrentSqmQueryPart() );
@@ -7190,7 +7187,7 @@ public SelectStatement visitSubQueryExpression(SqmSubQuery<?> sqmSubQuery) {
 		final QueryPart queryPart = visitQueryPart( sqmSubQuery.getQueryPart() );
 		currentlyProcessingJoin = oldJoin;
 		this.cteContainer = oldCteContainer;
-		return new SelectStatement( cteContainer, queryPart, emptyList(), emptyList() );
+		return new SelectStatement( cteContainer, queryPart, emptyList() );
 	}
 
 	@Override
diff --git a/hibernate-core/src/main/java/org/hibernate/sql/ast/internal/StandardLockingClauseStrategy.java b/hibernate-core/src/main/java/org/hibernate/sql/ast/internal/StandardLockingClauseStrategy.java
diff --git a/hibernate-core/src/main/java/org/hibernate/sql/ast/spi/AbstractSqlAstTranslator.java b/hibernate-core/src/main/java/org/hibernate/sql/ast/spi/AbstractSqlAstTranslator.java
diff --git a/hibernate-core/src/main/java/org/hibernate/sql/ast/tree/select/QuerySpec.java b/hibernate-core/src/main/java/org/hibernate/sql/ast/tree/select/QuerySpec.java
diff --git a/hibernate-core/src/main/java/org/hibernate/sql/ast/tree/select/SelectStatement.java b/hibernate-core/src/main/java/org/hibernate/sql/ast/tree/select/SelectStatement.java
diff --git a/hibernate-core/src/main/java/org/hibernate/sql/exec/internal/lock/TableLock.java b/hibernate-core/src/main/java/org/hibernate/sql/exec/internal/lock/TableLock.java
diff --git a/hibernate-core/src/test/java/org/hibernate/orm/test/locking/LockingBasedOnSelectClauseTests.java b/hibernate-core/src/test/java/org/hibernate/orm/test/locking/LockingBasedOnSelectClauseTests.java

Original file line number	Diff line number	Diff line change
`@@ -128,7 +128,7 @@ class DatabaseSnapshotExecutor {`
`128`	`128`	`}`
`129`	`129`	`);`
`130`	`130`
`131`		`- final var selectStatement = new SelectStatement( rootQuerySpec, domainResults, List.of() );`
	`131`	`+ final var selectStatement = new SelectStatement( rootQuerySpec, domainResults );`
`132`	`132`	`jdbcSelect =`
`133`	`133`	`sessionFactory.getJdbcServices().getJdbcEnvironment().getSqlAstTranslatorFactory()`
`134`	`134`	`.buildSelectTranslator( sessionFactory, selectStatement )`
Original file line number	Diff line number	Diff line change
`@@ -124,7 +124,7 @@ public static SelectStatement generateMatchingIdSelectStatement(`
`124`	`124`	`sqmConverter`
`125`	`125`	`);`
`126`	`126`
`127`		`- return new SelectStatement( idSelectionQuery, domainResults, List.of() );`
	`127`	`+ return new SelectStatement( idSelectionQuery, domainResults );`
`128`	`128`	`}`
`129`	`129`
`130`	`130`	`/// Generates a query-spec for selecting all ids matching the restriction defined as part`