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

Author: sebersole

hibernate-core/src/main/java/org/hibernate/sql/ast/internal/AbstractLockingClauseStrategy.java

@@ -15,9 +15,9 @@
 import java.util.HashSet;
 import java.util.Set;
 
-/**
- * @author Steve Ebersole
- */
+/// Base support for LockingClauseStrategy implementations
+///
+/// @author Steve Ebersole
 public abstract class AbstractLockingClauseStrategy implements LockingClauseStrategy {
 	private final Locking.Scope lockingScope;
 	private final Set<NavigablePath> rootsForLocking;
@@ -31,21 +31,12 @@ public AbstractLockingClauseStrategy(
 		this.rootsForLocking = rootsForLocking == null ? Set.of() : rootsForLocking;
 	}
 
-	protected Locking.Scope getLockingScope() {
-		return lockingScope;
-	}
-
-	protected Set<NavigablePath> getRootsForLocking() {
-		return rootsForLocking;
-	}
-
-	protected Set<NavigablePath> getPathsToLock() {
-		return pathsToLock;
-	}
-
 	@Override
 	public boolean shouldLockRoot(TableGroup root) {
-		return rootsForLocking.contains( root.getNavigablePath() );
+		// NOTE : the NavigablePath can be null in some cases.
+		// 		we don't care about these cases, so easier to just
+		// 		handle the nullness here
+		return root.getNavigablePath() != null && rootsForLocking.contains( root.getNavigablePath() );
 	}
 
 	@Override
@@ -104,9 +95,9 @@ protected void trackRoot(TableGroup root) {
 	}
 
 	@Override
-	public void registerJoin(TableGroupJoin join) {
-		if ( shouldLockJoin(  join.getJoinedGroup() ) ) {
-			trackJoin( join );
+	public void registerJoin(TableGroupJoin joinedGroup) {
+		if ( shouldLockJoin(  joinedGroup.getJoinedGroup() ) ) {
+			trackJoin( joinedGroup );
 		}
 	}
 

hibernate-core/src/main/java/org/hibernate/sql/ast/internal/NonLockingClauseStrategy.java

@@ -38,7 +38,7 @@ public void registerRoot(TableGroup root) {
 	}
 
 	@Override
-	public void registerJoin(TableGroupJoin join) {
+	public void registerJoin(TableGroupJoin joinedGroup) {
 		// nothing to do
 	}
 

hibernate-core/src/main/java/org/hibernate/sql/ast/internal/StandardLockingClauseStrategy.java

@@ -100,9 +100,9 @@ protected void trackRoot(TableGroup root) {
 	}
 
 	@Override
-	public void registerJoin(TableGroupJoin join) {
-		checkForOuterJoins( join );
-		super.registerJoin( join );
+	public void registerJoin(TableGroupJoin joinedGroup) {
+		checkForOuterJoins( joinedGroup );
+		super.registerJoin( joinedGroup );
 	}
 
 	@Override

hibernate-core/src/main/java/org/hibernate/sql/ast/spi/LockingClauseStrategy.java

@@ -4,48 +4,57 @@
  */
 package org.hibernate.sql.ast.spi;
 
+import org.hibernate.Incubating;
 import org.hibernate.sql.ast.tree.from.TableGroup;
 import org.hibernate.sql.ast.tree.from.TableGroupJoin;
 
 import java.util.Collection;
 
-/**
- * Strategy for dealing with locking via a SQL {@code FOR UPDATE (OF)}
- * clause.
- * <p/>
- * Some dialects do not use a {@code FOR UPDATE (OF)} to apply
- * locks - e.g., they apply locks in the {@code FROM} clause.  Such
- * dialects would return a no-op version of this contract.
- * <p/>
- * Some dialects support an additional {@code FOR SHARE (OF)} clause
- * as well to acquire non-exclusive locks.  That is also handled here,
- * varied by the requested {@linkplain org.hibernate.LockMode LockMode}.
- * <p/>
- * Operates in 2 "phases"-<ol>
- *     <li>
- *         collect tables which are to be locked (based on {@linkplain org.hibernate.Locking.Scope},
- *         and other things)
- *     </li>
- *     <li>
- *         render the appropriate locking fragment
- *     </li>
- * </ol>
- *
- * @see org.hibernate.dialect.Dialect#getLockingClauseStrategy
- *
- * @author Steve Ebersole
- */
+/// Strategy for dealing with locking via a SQL `FOR UPDATE (OF)`
+/// clause.
+///
+/// Some dialects do not use a `FOR UPDATE (OF)` to apply
+/// locks - e.g., they apply locks in the `FROM` clause.  Such
+/// dialects would return a no-op version of this contract.
+///
+/// Some dialects support an additional `FOR SHARE (OF)` clause
+/// as well to acquire non-exclusive locks.  That is also handled here,
+/// varied by the requested {@linkplain org.hibernate.LockMode LockMode}.
+///
+/// Operates in 2 "phases"-
+/// * collect tables which are to be locked (based on {@linkplain org.hibernate.Locking.Scope}, and other things)
+/// * render the appropriate locking fragment
+///
+/// @implSpec Note that this is also used to determine and track which
+/// tables to lock even for cases (T-SQL e.g.) where a "locking clause"
+/// per-se won't be used.  In such cases, only the first phase (along
+/// with [#shouldLockRoot] and [#shouldLockJoin]) have any impact.
+///
+/// @see org.hibernate.dialect.Dialect#getLockingClauseStrategy
+///
+/// @author Steve Ebersole
+@Incubating
 public interface LockingClauseStrategy {
+	/// Should the specified `root` be locked
 	boolean shouldLockRoot(TableGroup root);
+	/// Should the specified `joinedGroup` be locked
 	boolean shouldLockJoin(TableGroup joinedGroup);
 
+	/// Register the given `root`
 	void registerRoot(TableGroup root);
-	void registerJoin(TableGroupJoin join);
+	/// Register the given `joinedGroup`
+	void registerJoin(TableGroupJoin joinedGroup);
 
+	/// Are any outer joins encountered during registration
+	/// of [roots][#registerRoot] and [joins][#registerJoin]
 	boolean containsOuterJoins();
 
+	/// For cases where a locking clause is to be used,
+	/// render that locking clause.
 	void render(SqlAppender sqlAppender);
 
+	/// All roots to be locked.
 	Collection<TableGroup> getRootsToLock();
+	/// All joins to be locked.
 	Collection<TableGroupJoin> getJoinsToLock();
 }