From 3cfd85d8e24808c7819886b2df977493efc6d05a Mon Sep 17 00:00:00 2001 From: Marco Belladelli Date: Mon, 6 Nov 2023 17:47:40 +0100 Subject: [PATCH] HHH-17384 Fix `@NotFound` to-one association nullness handling --- .../dialect/DerbyLegacySqlAstTranslator.java | 5 + .../dialect/H2LegacySqlAstTranslator.java | 5 + .../dialect/DerbySqlAstTranslator.java | 5 + .../hibernate/dialect/H2SqlAstTranslator.java | 5 + .../internal/ToOneAttributeMapping.java | 4 +- .../sqm/sql/BaseSqmToSqlAstConverter.java | 104 ++++-- .../EntityValuedPathInterpretation.java | 6 +- .../sql/ast/spi/AbstractSqlAstTranslator.java | 141 +++++++- .../sql/ast/tree/from/TableGroup.java | 11 +- .../mappedBy/IsNullAndMappedByTest.java | 330 ++++++++++++++++++ .../test/notfound/IsNullAndNotFoundTest.java | 58 ++- 11 files changed, 618 insertions(+), 56 deletions(-) create mode 100644 hibernate-core/src/test/java/org/hibernate/orm/test/mapping/mappedBy/IsNullAndMappedByTest.java diff --git a/hibernate-community-dialects/src/main/java/org/hibernate/community/dialect/DerbyLegacySqlAstTranslator.java b/hibernate-community-dialects/src/main/java/org/hibernate/community/dialect/DerbyLegacySqlAstTranslator.java index a404f0915a..deee312b08 100644 --- a/hibernate-community-dialects/src/main/java/org/hibernate/community/dialect/DerbyLegacySqlAstTranslator.java +++ b/hibernate-community-dialects/src/main/java/org/hibernate/community/dialect/DerbyLegacySqlAstTranslator.java @@ -263,6 +263,11 @@ public class DerbyLegacySqlAstTranslator extends Abstra return getDialect().getVersion().isSameOrAfter( 10, 5 ); } + @Override + protected boolean supportsJoinInMutationStatementSubquery() { + return false; + } + @Override public void visitBinaryArithmeticExpression(BinaryArithmeticExpression arithmeticExpression) { final BinaryArithmeticOperator operator = arithmeticExpression.getOperator(); diff --git a/hibernate-community-dialects/src/main/java/org/hibernate/community/dialect/H2LegacySqlAstTranslator.java b/hibernate-community-dialects/src/main/java/org/hibernate/community/dialect/H2LegacySqlAstTranslator.java index c13ae75cc5..9e27640531 100644 --- a/hibernate-community-dialects/src/main/java/org/hibernate/community/dialect/H2LegacySqlAstTranslator.java +++ b/hibernate-community-dialects/src/main/java/org/hibernate/community/dialect/H2LegacySqlAstTranslator.java @@ -302,4 +302,9 @@ public class H2LegacySqlAstTranslator extends AbstractS // Introduction of PERCENT support https://github.com/h2database/h2database/commit/f45913302e5f6ad149155a73763c0c59d8205849 return getDialect().getVersion().isSameOrAfter( 1, 4, 198 ); } + + @Override + protected boolean supportsJoinInMutationStatementSubquery() { + return false; + } } diff --git a/hibernate-core/src/main/java/org/hibernate/dialect/DerbySqlAstTranslator.java b/hibernate-core/src/main/java/org/hibernate/dialect/DerbySqlAstTranslator.java index 566906879c..be9f8577bb 100644 --- a/hibernate-core/src/main/java/org/hibernate/dialect/DerbySqlAstTranslator.java +++ b/hibernate-core/src/main/java/org/hibernate/dialect/DerbySqlAstTranslator.java @@ -261,6 +261,11 @@ public class DerbySqlAstTranslator extends AbstractSqlA return true; } + @Override + protected boolean supportsJoinInMutationStatementSubquery() { + return false; + } + @Override public void visitBinaryArithmeticExpression(BinaryArithmeticExpression arithmeticExpression) { final BinaryArithmeticOperator operator = arithmeticExpression.getOperator(); diff --git a/hibernate-core/src/main/java/org/hibernate/dialect/H2SqlAstTranslator.java b/hibernate-core/src/main/java/org/hibernate/dialect/H2SqlAstTranslator.java index 20e14e3b6b..258d5cab97 100644 --- a/hibernate-core/src/main/java/org/hibernate/dialect/H2SqlAstTranslator.java +++ b/hibernate-core/src/main/java/org/hibernate/dialect/H2SqlAstTranslator.java @@ -281,4 +281,9 @@ public class H2SqlAstTranslator extends SqlAstTranslato // Introduction of PERCENT support https://github.com/h2database/h2database/commit/f45913302e5f6ad149155a73763c0c59d8205849 return true; } + + @Override + protected boolean supportsJoinInMutationStatementSubquery() { + return false; + } } diff --git a/hibernate-core/src/main/java/org/hibernate/metamodel/mapping/internal/ToOneAttributeMapping.java b/hibernate-core/src/main/java/org/hibernate/metamodel/mapping/internal/ToOneAttributeMapping.java index 35c866e409..972f9a6cad 100644 --- a/hibernate-core/src/main/java/org/hibernate/metamodel/mapping/internal/ToOneAttributeMapping.java +++ b/hibernate-core/src/main/java/org/hibernate/metamodel/mapping/internal/ToOneAttributeMapping.java @@ -1534,7 +1534,7 @@ public class ToOneAttributeMapping if ( sideNature == ForeignKeyDescriptor.Nature.KEY ) { // If the key side is non-nullable we also need to add the keyResult // to be able to manually check invalid foreign key references - if ( notFoundAction != null || !isInternalLoadNullable ) { + if ( hasNotFoundAction() || !isInternalLoadNullable ) { keyResult = foreignKeyDescriptor.createKeyDomainResult( fetchablePath, tableGroup, @@ -1543,7 +1543,7 @@ public class ToOneAttributeMapping ); } } - else if ( notFoundAction != null + else if ( hasNotFoundAction() || getAssociatedEntityMappingType().getSoftDeleteMapping() != null ) { // For the target side only add keyResult when a not-found action is present keyResult = foreignKeyDescriptor.createTargetDomainResult( 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 index 0e977d0bc9..607b3ed4f0 100644 --- 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 @@ -3575,10 +3575,14 @@ public abstract class BaseSqmToSqlAstConverter extends Base } private X prepareReusablePath(SqmPath sqmPath, Supplier supplier) { - return prepareReusablePath( sqmPath, fromClauseIndexStack.getCurrent(), supplier ); + return prepareReusablePath( sqmPath, fromClauseIndexStack.getCurrent(), supplier, false ); } - private X prepareReusablePath(SqmPath sqmPath, FromClauseIndex fromClauseIndex, Supplier supplier) { + private X prepareReusablePath( + SqmPath sqmPath, + FromClauseIndex fromClauseIndex, + Supplier supplier, + boolean allowLeftJoins) { final Consumer implicitJoinChecker; if ( getCurrentClauseStack().getCurrent() != Clause.SET_EXPRESSION ) { implicitJoinChecker = tg -> {}; @@ -3601,7 +3605,8 @@ public abstract class BaseSqmToSqlAstConverter extends Base fromClauseIndex.getTableGroup( sqmPath.getLhs().getNavigablePath() ), sqmPath ), - sqmPath + sqmPath, + allowLeftJoins ); if ( createdTableGroup != null ) { if ( sqmPath instanceof SqmTreatedPath ) { @@ -3655,7 +3660,7 @@ public abstract class BaseSqmToSqlAstConverter extends Base } else { newTableGroup = getActualTableGroup( - createTableGroup( createdParentTableGroup, parentPath ), + createTableGroup( createdParentTableGroup, parentPath, false ), sqmPath ); } @@ -3669,9 +3674,21 @@ public abstract class BaseSqmToSqlAstConverter extends Base fromClauseIndex.register( sqmPath, parentTableGroup ); } - if ( parentPath instanceof SqmSimplePath + upgradeToInnerJoinIfNeeded( parentTableGroup, sqmPath, parentPath, fromClauseIndex ); + + registerPathAttributeEntityNameUsage( sqmPath, parentTableGroup ); + + return parentTableGroup; + } + + private void upgradeToInnerJoinIfNeeded( + TableGroup parentTableGroup, + SqmPath sqmPath, + SqmPath parentPath, + FromClauseIndex fromClauseIndex) { + if ( getCurrentClauseStack().getCurrent() != Clause.SELECT + && parentPath instanceof SqmSimplePath && CollectionPart.Nature.fromName( parentPath.getNavigablePath().getLocalName() ) == null - && getCurrentClauseStack().getCurrent() != Clause.SELECT && parentPath.getParentPath() != null && parentTableGroup.getModelPart() instanceof ToOneAttributeMapping ) { // we need to handle the case of an implicit path involving a to-one @@ -3695,9 +3712,6 @@ public abstract class BaseSqmToSqlAstConverter extends Base } } } - registerPathAttributeEntityNameUsage( sqmPath, parentTableGroup ); - - return parentTableGroup; } private void prepareForSelection(SqmPath selectionPath) { @@ -3723,7 +3737,8 @@ public abstract class BaseSqmToSqlAstConverter extends Base // But only create it for paths that are not handled by #prepareReusablePath anyway final TableGroup createdTableGroup = createTableGroup( getActualTableGroup( fromClauseIndex.getTableGroup( path.getLhs().getNavigablePath() ), path ), - path + path, + false ); if ( createdTableGroup != null ) { registerEntityNameProjectionUsage( path, createdTableGroup ); @@ -3744,7 +3759,7 @@ public abstract class BaseSqmToSqlAstConverter extends Base } } - private TableGroup createTableGroup(TableGroup parentTableGroup, SqmPath joinedPath) { + private TableGroup createTableGroup(TableGroup parentTableGroup, SqmPath joinedPath, boolean allowLeftJoins) { final SqmPath lhsPath = joinedPath.getLhs(); final FromClauseIndex fromClauseIndex = getFromClauseIndex(); final ModelPart subPart = parentTableGroup.getModelPart().findSubPart( @@ -3776,18 +3791,30 @@ public abstract class BaseSqmToSqlAstConverter extends Base querySpec.getFromClause().addRoot( tableGroup ); } else { - // Check if we can reuse a table group join of the parent - final TableGroup compatibleTableGroup = parentTableGroup.findCompatibleJoinedGroup( - joinProducer, - SqlAstJoinType.INNER - ); + final TableGroupJoin compatibleLeftJoin; + final SqlAstJoinType sqlAstJoinType; + if ( isMappedByOrNotFoundToOne( joinProducer ) ) { + compatibleLeftJoin = parentTableGroup.findCompatibleJoin( + joinProducer, + SqlAstJoinType.LEFT + ); + sqlAstJoinType = SqlAstJoinType.LEFT; + } + else { + compatibleLeftJoin = null; + sqlAstJoinType = null; + } + + final TableGroup compatibleTableGroup = compatibleLeftJoin != null ? + compatibleLeftJoin.getJoinedGroup() : + parentTableGroup.findCompatibleJoinedGroup( joinProducer, SqlAstJoinType.INNER ); if ( compatibleTableGroup == null ) { final TableGroupJoin tableGroupJoin = joinProducer.createTableGroupJoin( joinedPath.getNavigablePath(), parentTableGroup, null, null, - null, + allowLeftJoins ? sqlAstJoinType : null, false, false, this @@ -3807,6 +3834,10 @@ public abstract class BaseSqmToSqlAstConverter extends Base // Also register the table group under its original navigable path, which possibly contains an alias // This is important, as otherwise we might create new joins in subqueries which are unnecessary fromClauseIndex.registerTableGroup( tableGroup.getNavigablePath(), tableGroup ); + // Upgrade the join type to inner if the context doesn't allow left joins + if ( compatibleLeftJoin != null && !allowLeftJoins ) { + compatibleLeftJoin.setJoinType( SqlAstJoinType.INNER ); + } } } @@ -3819,6 +3850,16 @@ public abstract class BaseSqmToSqlAstConverter extends Base return tableGroup; } + private boolean isMappedByOrNotFoundToOne(TableGroupJoinProducer joinProducer) { + if ( joinProducer instanceof ToOneAttributeMapping ) { + final ToOneAttributeMapping toOne = (ToOneAttributeMapping) joinProducer; + if ( toOne.hasNotFoundAction() || toOne.getReferencedPropertyName() != null ) { + return true; + } + } + return false; + } + private boolean isRecursiveCte(TableGroup tableGroup) { if ( tableGroup instanceof CteTableGroup ) { final CteTableGroup cteTableGroup = (CteTableGroup) tableGroup; @@ -7674,11 +7715,30 @@ public abstract class BaseSqmToSqlAstConverter extends Base @Override public NullnessPredicate visitIsNullPredicate(SqmNullnessPredicate predicate) { - return new NullnessPredicate( - (Expression) visitWithInferredType( predicate.getExpression(), () -> basicType( Object.class )), - predicate.isNegated(), - getBooleanType() - ); + final SqmExpression sqmExpression = predicate.getExpression(); + final Expression expression; + if ( sqmExpression instanceof SqmEntityValuedSimplePath ) { + final SqmEntityValuedSimplePath entityValuedPath = (SqmEntityValuedSimplePath) sqmExpression; + inferrableTypeAccessStack.push( () -> basicType( Object.class ) ); + expression = withTreatRestriction( prepareReusablePath( + entityValuedPath, + fromClauseIndexStack.getCurrent(), + () -> EntityValuedPathInterpretation.from( + entityValuedPath, + getInferredValueMapping(), + this + ), + true + ), entityValuedPath ); + inferrableTypeAccessStack.pop(); + } + else { + expression = (Expression) visitWithInferredType( + predicate.getExpression(), + () -> basicType( Object.class ) + ); + } + return new NullnessPredicate( expression, predicate.isNegated(), getBooleanType() ); } @Override diff --git a/hibernate-core/src/main/java/org/hibernate/query/sqm/sql/internal/EntityValuedPathInterpretation.java b/hibernate-core/src/main/java/org/hibernate/query/sqm/sql/internal/EntityValuedPathInterpretation.java index 56721ae79f..a5ea0b7db8 100644 --- a/hibernate-core/src/main/java/org/hibernate/query/sqm/sql/internal/EntityValuedPathInterpretation.java +++ b/hibernate-core/src/main/java/org/hibernate/query/sqm/sql/internal/EntityValuedPathInterpretation.java @@ -221,12 +221,8 @@ public class EntityValuedPathInterpretation extends AbstractSqmPathInterpreta } else { // If the mapping is an inverse association, use the PK and disallow FK optimizations - resultModelPart = ( (EntityAssociationMapping) mapping ).getAssociatedEntityMappingType().getIdentifierMapping(); + resultModelPart = associationMapping.getAssociatedEntityMappingType().getIdentifierMapping(); resultTableGroup = tableGroup; - - // todo (not-found) : in the case of not-found=ignore, we want to do the join, however - - // * use a left join when the association is the path terminus (`root.association`) - // * use an inner join when it is further de-referenced (`root.association.stuff`) } } else if ( mapping instanceof AnonymousTupleEntityValuedModelPart ) { 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 index 01de7cacbb..c723ea7f7f 100644 --- 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 @@ -79,6 +79,7 @@ import org.hibernate.query.sqm.function.MultipatternSqmFunctionDescriptor; import org.hibernate.query.sqm.function.SelfRenderingAggregateFunctionSqlAstExpression; import org.hibernate.query.sqm.function.SelfRenderingFunctionSqlAstExpression; import org.hibernate.query.sqm.function.SqmFunctionDescriptor; +import org.hibernate.query.sqm.sql.internal.EntityValuedPathInterpretation; import org.hibernate.query.sqm.sql.internal.SqmParameterInterpretation; import org.hibernate.query.sqm.sql.internal.SqmPathInterpretation; import org.hibernate.query.sqm.tree.expression.Conversion; @@ -143,6 +144,8 @@ import org.hibernate.sql.ast.tree.from.LazyTableGroup; import org.hibernate.sql.ast.tree.from.NamedTableReference; import org.hibernate.sql.ast.tree.from.QueryPartTableGroup; import org.hibernate.sql.ast.tree.from.QueryPartTableReference; +import org.hibernate.sql.ast.tree.from.StandardTableGroup; +import org.hibernate.sql.ast.tree.from.StandardVirtualTableGroup; import org.hibernate.sql.ast.tree.from.TableGroup; import org.hibernate.sql.ast.tree.from.TableGroupJoin; import org.hibernate.sql.ast.tree.from.TableGroupProducer; @@ -1092,6 +1095,61 @@ public abstract class AbstractSqlAstTranslator implemen querySpec.getSelectClause().addSqlSelection( new SqlSelectionImpl( new QueryLiteral<>( 1, getIntegerType() ) ) ); + + final List collectedNonInnerJoins; + if ( supportsJoinInMutationStatementSubquery() ) { + collectedNonInnerJoins = new ArrayList<>(); + emulateWhereClauseRestrictionJoins( statement, querySpec, tableGroupJoin -> { + if ( tableGroupJoin.getJoinType() == SqlAstJoinType.INNER ) { + final TableGroup joinedGroup = tableGroupJoin.getJoinedGroup(); + final FromClause fromClause = querySpec.getFromClause(); + if ( fromClause.getRoots().isEmpty() ) { + final TableGroup copy = new StandardTableGroup( + joinedGroup.canUseInnerJoins(), + joinedGroup.getNavigablePath(), + (TableGroupProducer) joinedGroup.getModelPart(), + joinedGroup.getSourceAlias(), + joinedGroup.getPrimaryTableReference(), + null, + null + ); + fromClause.addRoot( copy ); + } + else { + fromClause.addRoot( joinedGroup ); + } + querySpec.applyPredicate( tableGroupJoin.getPredicate() ); + } + else { + collectedNonInnerJoins.add( tableGroupJoin ); + } + } ); + } + else { + collectedNonInnerJoins = null; + emulateWhereClauseRestrictionJoins( statement, querySpec, tableGroupJoin -> { + if ( tableGroupJoin.getJoinType() == SqlAstJoinType.INNER ) { + querySpec.getFromClause().addRoot( tableGroupJoin.getJoinedGroup() ); + querySpec.applyPredicate( tableGroupJoin.getPredicate() ); + } + } ); + } + + if ( querySpec.getFromClause().getRoots().isEmpty() ) { + return statement.getRestriction(); + } + else if ( collectedNonInnerJoins != null ) { + collectedNonInnerJoins.forEach( querySpec.getFromClause().getRoots().get( 0 )::addTableGroupJoin ); + } + + querySpec.applyPredicate( statement.getRestriction() ); + return new ExistsPredicate( querySpec, false, getBooleanType() ); + } + + private void emulateWhereClauseRestrictionJoins( + AbstractUpdateOrDeleteStatement statement, + QuerySpec querySpec, + Consumer joinConsumer) { for ( TableGroup root : statement.getFromClause().getRoots() ) { if ( root.getPrimaryTableReference() == statement.getTargetTable() ) { for ( TableReferenceJoin tableReferenceJoin : root.getTableReferenceJoins() ) { @@ -1106,23 +1164,13 @@ public abstract class AbstractSqlAstTranslator implemen ); querySpec.applyPredicate( tableReferenceJoin.getPredicate() ); } - for ( TableGroupJoin tableGroupJoin : root.getTableGroupJoins() ) { - assert tableGroupJoin.getJoinType() == SqlAstJoinType.INNER; - querySpec.getFromClause().addRoot( tableGroupJoin.getJoinedGroup() ); - querySpec.applyPredicate( tableGroupJoin.getPredicate() ); - } - for ( TableGroupJoin tableGroupJoin : root.getNestedTableGroupJoins() ) { - assert tableGroupJoin.getJoinType() == SqlAstJoinType.INNER; - querySpec.getFromClause().addRoot( tableGroupJoin.getJoinedGroup() ); - querySpec.applyPredicate( tableGroupJoin.getPredicate() ); - } + root.getTableGroupJoins().forEach( joinConsumer ); + root.getNestedTableGroupJoins().forEach( joinConsumer ); } else { querySpec.getFromClause().addRoot( root ); } } - querySpec.applyPredicate( statement.getRestriction() ); - return new ExistsPredicate( querySpec, false, getBooleanType() ); } protected void renderSetClause(UpdateStatement statement, Stack clauseStack) { @@ -7460,11 +7508,66 @@ public abstract class AbstractSqlAstTranslator implemen separator = " and "; } } + return; } - else { - expression.accept( this ); - appendSql( predicateValue ); + else if ( expression instanceof EntityValuedPathInterpretation ) { + final AbstractUpdateOrDeleteStatement statement = getCurrentOrParentUpdateOrDeleteStatement( !supportsJoinInMutationStatementSubquery() ); + if ( statement != null ) { + final TableGroup tableGroup = ( (EntityValuedPathInterpretation) expression ).getTableGroup(); + final TableGroupJoin tableGroupJoin = findTableGroupJoin( + tableGroup, + statement.getFromClause().getRoots() + ); + if ( tableGroupJoin != null && tableGroupJoin.getJoinType() != SqlAstJoinType.INNER ) { + emulateNullnessPredicateWithExistsSubquery( nullnessPredicate, tableGroup, tableGroupJoin ); + return; + } + } } + expression.accept( this ); + appendSql( predicateValue ); + } + + private void emulateNullnessPredicateWithExistsSubquery( + NullnessPredicate nullnessPredicate, + TableGroup tableGroup, + TableGroupJoin tableGroupJoin) { + final QuerySpec querySpec = new QuerySpec( false ); + querySpec.getSelectClause().addSqlSelection( + new SqlSelectionImpl( new QueryLiteral<>( 1, getIntegerType() ) ) + ); + querySpec.getFromClause().getRoots().add( tableGroup ); + querySpec.applyPredicate( tableGroupJoin.getPredicate() ); + + if ( !nullnessPredicate.isNegated() ) { + appendSql( "not " ); + } + appendSql( "exists(" ); + statementStack.push( new SelectStatement( querySpec ) ); + visitQuerySpec( querySpec ); + statementStack.pop(); + appendSql( ")" ); + } + + private AbstractUpdateOrDeleteStatement getCurrentOrParentUpdateOrDeleteStatement(boolean checkParent) { + if ( statementStack.getCurrent() instanceof AbstractUpdateOrDeleteStatement ) { + return (AbstractUpdateOrDeleteStatement) statementStack.getCurrent(); + } + else if ( checkParent && statementStack.depth() > 1 + && statementStack.peek( 1 ) instanceof AbstractUpdateOrDeleteStatement ) { + return (AbstractUpdateOrDeleteStatement) statementStack.peek( 1 ); + } + return null; + } + + private TableGroupJoin findTableGroupJoin(TableGroup tableGroup, List roots) { + for ( TableGroup root : roots ) { + final TableGroupJoin tableGroupJoin = root.findTableGroupJoin( tableGroup ); + if ( tableGroupJoin != null ) { + return tableGroupJoin; + } + } + return null; } @Override @@ -7795,6 +7898,14 @@ public abstract class AbstractSqlAstTranslator implemen return supportsRowValueConstructorSyntaxInInList(); } + /** + * If the dialect supports using joins in mutation statement subquery + * that could also use columns from the mutation target table + */ + protected boolean supportsJoinInMutationStatementSubquery() { + return true; + } + /** * Some databases require a bit of syntactic noise when * there are no tables in the from clause. diff --git a/hibernate-core/src/main/java/org/hibernate/sql/ast/tree/from/TableGroup.java b/hibernate-core/src/main/java/org/hibernate/sql/ast/tree/from/TableGroup.java index 8fef7b273f..ef75a5df17 100644 --- a/hibernate-core/src/main/java/org/hibernate/sql/ast/tree/from/TableGroup.java +++ b/hibernate-core/src/main/java/org/hibernate/sql/ast/tree/from/TableGroup.java @@ -161,7 +161,7 @@ public interface TableGroup extends SqlAstNode, ColumnReferenceQualifier, SqmPat return true; } - default TableGroup findCompatibleJoinedGroup( + default TableGroupJoin findCompatibleJoin( TableGroupJoinProducer joinProducer, SqlAstJoinType requestedJoinType) { // We don't look into nested table group joins as that wouldn't be "compatible" @@ -178,13 +178,20 @@ public interface TableGroup extends SqlAstNode, ColumnReferenceQualifier, SqmPat // regardless of the join type or predicate since the LHS is the same table group // If this is a left join though, we have to check if the predicate is simply the association predicate if ( joinType == SqlAstJoinType.INNER || joinProducer.isSimpleJoinPredicate( join.getPredicate() ) ) { - return join.getJoinedGroup(); + return join; } } } return null; } + default TableGroup findCompatibleJoinedGroup( + TableGroupJoinProducer joinProducer, + SqlAstJoinType requestedJoinType) { + final TableGroupJoin compatibleJoin = findCompatibleJoin( joinProducer, requestedJoinType ); + return compatibleJoin != null ? compatibleJoin.getJoinedGroup() : null; + } + default TableGroupJoin findTableGroupJoin(TableGroup tableGroup) { for ( TableGroupJoin join : getTableGroupJoins() ) { if ( join.getJoinedGroup() == tableGroup ) { diff --git a/hibernate-core/src/test/java/org/hibernate/orm/test/mapping/mappedBy/IsNullAndMappedByTest.java b/hibernate-core/src/test/java/org/hibernate/orm/test/mapping/mappedBy/IsNullAndMappedByTest.java new file mode 100644 index 0000000000..fa1a8fa54b --- /dev/null +++ b/hibernate-core/src/test/java/org/hibernate/orm/test/mapping/mappedBy/IsNullAndMappedByTest.java @@ -0,0 +1,330 @@ +/* + * Hibernate, Relational Persistence for Idiomatic Java + * + * License: GNU Lesser General Public License (LGPL), version 2.1 or later + * See the lgpl.txt file in the root directory or http://www.gnu.org/licenses/lgpl-2.1.html + */ +package org.hibernate.orm.test.mapping.mappedBy; + +import java.util.List; + +import org.hibernate.testing.jdbc.SQLStatementInspector; +import org.hibernate.testing.orm.junit.DomainModel; +import org.hibernate.testing.orm.junit.Jira; +import org.hibernate.testing.orm.junit.SessionFactory; +import org.hibernate.testing.orm.junit.SessionFactoryScope; +import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Test; + +import jakarta.persistence.Entity; +import jakarta.persistence.Id; +import jakarta.persistence.OneToOne; +import jakarta.persistence.Table; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.jupiter.api.Assertions.assertEquals; + +/** + * @author Marco Belladelli + * + * @see org.hibernate.orm.test.notfound.IsNullAndNotFoundTest + */ +@DomainModel( annotatedClasses = { + IsNullAndMappedByTest.Person.class, + IsNullAndMappedByTest.Account.class, +} ) +@SessionFactory( useCollectingStatementInspector = true ) +@Jira( "https://hibernate.atlassian.net/browse/HHH-17384" ) +public class IsNullAndMappedByTest { + @BeforeAll + public void setUp(SessionFactoryScope scope) { + scope.inTransaction( session -> { + final Person person1 = new Person( 1, "Luigi" ); + final Person person2 = new Person( 2, "Andrea" ); + final Person person3 = new Person( 3, "Max" ); + + final Account account1 = new Account( 1, null, null, person1 ); + final Account account2 = new Account( 2, "Fab", null, person2 ); + final Account account3 = new Account( 3, "And", null, null ); + + session.persist( person1 ); + session.persist( person2 ); + session.persist( person3 ); + session.persist( account1 ); + session.persist( account2 ); + session.persist( account3 ); + } ); + } + + @AfterAll + public void tearDown(SessionFactoryScope scope) { + scope.inTransaction( session -> { + session.createMutationQuery( "delete from Account" ).executeUpdate(); + session.createMutationQuery( "delete from Person" ).executeUpdate(); + } ); + } + + @Test + public void testAssociationDereferenceIsNullInWhereClause(SessionFactoryScope scope) { + final SQLStatementInspector inspector = scope.getCollectingStatementInspector(); + scope.inTransaction( session -> { + inspector.clear(); + + // should produce an inner join to ACCOUNT_TABLE + + final List ids = session.createQuery( + "select p.id from Person p where p.account.code is null", + Integer.class + ).getResultList(); + + assertEquals( 1, ids.size() ); + assertEquals( 1, (int) ids.get( 0 ) ); + + assertThat( inspector.getSqlQueries() ).hasSize( 1 ); + assertThat( inspector.getSqlQueries().get( 0 ) ).containsIgnoringCase( " join " ); + assertThat( inspector.getSqlQueries().get( 0 ) ).doesNotContainIgnoringCase( " left " ); + assertThat( inspector.getSqlQueries().get( 0 ) ).containsIgnoringCase( " ACCOUNT_TABLE " ); + } ); + } + + @Test + public void testAssociationIsNullInWhereClause(SessionFactoryScope scope) { + final SQLStatementInspector inspector = scope.getCollectingStatementInspector(); + scope.inTransaction( session -> { + inspector.clear(); + + // should produce a left join to ACCOUNT_TABLE and restrict based on the Account's id - + // + // ... + // from PERSON p + // left join ACCOUNT_TABLE a + // on p.account_id = a.id + // where a.id is null + + final List ids = session.createQuery( + "select distinct p.id from Person p where p.account is null", + Integer.class + ).getResultList(); + + assertEquals( 1, ids.size() ); + assertEquals( 3, (int) ids.get( 0 ) ); + + assertThat( inspector.getSqlQueries() ).hasSize( 1 ); + assertThat( inspector.getSqlQueries().get( 0 ) ).containsIgnoringCase( " left join " ); + assertThat( inspector.getSqlQueries().get( 0 ) ).containsIgnoringCase( ".id is null" ); + } ); + } + + @Test + public void testFetchedAssociationIsNullInWhereClause(SessionFactoryScope scope) { + final SQLStatementInspector inspector = scope.getCollectingStatementInspector(); + scope.inTransaction( session -> { + inspector.clear(); + + // should produce an inner join to ACCOUNT_TABLE since it's explicitly selected + // + // ... + // from PERSON p + // join ACCOUNT_TABLE a + // on p.account_id = a.id + // where a.id is null + + final List results = session.createQuery( + "select p.account from Person p where p.account is null", + Account.class + ).getResultList(); + + assertThat( results ).isEmpty(); + + assertThat( inspector.getSqlQueries() ).hasSize( 1 ); + assertThat( inspector.getSqlQueries().get( 0 ) ).containsIgnoringCase( "join" ); + assertThat( inspector.getSqlQueries().get( 0 ) ).doesNotContainIgnoringCase( " left join " ); + } ); + } + + @Test + public void testIsNullInWhereClause3(SessionFactoryScope scope) { + final SQLStatementInspector inspector = scope.getCollectingStatementInspector(); + scope.inTransaction( session -> { + inspector.clear(); + + final List ids = session.createQuery( + "select distinct a.id from Account a where fk(a.person) is null", + Integer.class + ).getResultList(); + + assertEquals( 1, ids.size() ); + assertEquals( 3, (int) ids.get( 0 ) ); + + assertThat( inspector.getSqlQueries() ).hasSize( 1 ); + assertThat( inspector.getSqlQueries().get( 0 ) ).doesNotContainIgnoringCase( " join " ); + assertThat( inspector.getSqlQueries().get( 0 ) ).containsIgnoringCase( ".person_id is null" ); + } ); + } + + @Test + public void testAssociationEqualsInWhereClause(SessionFactoryScope scope) { + final SQLStatementInspector inspector = scope.getCollectingStatementInspector(); + scope.inTransaction( session -> { + inspector.clear(); + + // at the moment - + // + // select + // distinct p1_0.id + // from + // Person p1_0 + // join + // ACCOUNT_TABLE a1_0 + // on a1_0.id=p1_0.account_id + // where + // a1_0.id=? + + final List ids = session.createQuery( + "select distinct p.id from Person p where p.account = :acct", + Integer.class + ).setParameter( "acct", new Account( 1, null, null, null ) ).getResultList(); + + assertThat( ids ).hasSize( 1 ); + assertThat( ids.get( 0 ) ).isEqualTo( 1 ); + + assertThat( inspector.getSqlQueries() ).hasSize( 1 ); + assertThat( inspector.getSqlQueries().get( 0 ) ).containsIgnoringCase( " join " ); + assertThat( inspector.getSqlQueries().get( 0 ) ).containsIgnoringCase( ".id=?" ); + } ); + } + + @Test + public void testIsNullInWhereClause5(SessionFactoryScope scope) { + scope.inTransaction( session -> { + final List ids = session.createQuery( + "select p.id from Person p where p.account.code is null or p.account.id is null", + Integer.class + ) + .getResultList(); + + assertEquals( 1, ids.size() ); + assertEquals( 1, (int) ids.get( 0 ) ); + } ); + } + + @Test + public void testWhereClause(SessionFactoryScope scope) { + scope.inTransaction( session -> { + final List ids = session.createQuery( + "select p.id from Person p where p.account.code = :code and p.account.id = :id", + Integer.class + ) + .setParameter( "code", "Fab" ) + .setParameter( "id", 2 ) + .getResultList(); + + assertEquals( 1, ids.size() ); + assertEquals( 2, (int) ids.get( 0 ) ); + } ); + } + + @Test + public void testDelete(SessionFactoryScope scope) { + final SQLStatementInspector inspector = scope.getCollectingStatementInspector(); + inspector.clear(); + + scope.inTransaction( (entityManager) -> { + entityManager.createMutationQuery( "delete from Person p where p.account is null" ).executeUpdate(); + + assertThat( inspector.getSqlQueries() ).hasSize( 1 ); + // could physically be a join or exists sub-query + assertThat( inspector.getSqlQueries() + .get( 0 ) ).matches( (sql) -> sql.contains( "left join" ) || sql.contains( "not exists" ) ); + } ); + } + + @Test + public void testHqlUpdate(SessionFactoryScope scope) { + final SQLStatementInspector inspector = scope.getCollectingStatementInspector(); + inspector.clear(); + + scope.inTransaction( (entityManager) -> { + entityManager.createMutationQuery( "update Person p set p.name = 'abc' where p.account is null" ).executeUpdate(); + + assertThat( inspector.getSqlQueries() ).hasSize( 1 ); + // could physically be a join or exists sub-query + assertThat( inspector.getSqlQueries() + .get( 0 ) ).matches( (sql) -> sql.contains( "left join" ) || sql.contains( "not exists" ) ); + } ); + } + + @Test + public void testHqlUpdateSet(SessionFactoryScope scope) { + final SQLStatementInspector inspector = scope.getCollectingStatementInspector(); + inspector.clear(); + + scope.inTransaction( (entityManager) -> { + entityManager.createMutationQuery( "update Account a set a.person = null where id = 99" ).executeUpdate(); + + assertThat( inspector.getSqlQueries() ).hasSize( 1 ); + assertThat( inspector.getSqlQueries().get( 0 ) ).doesNotContainIgnoringCase( " join " ); + assertThat( inspector.getSqlQueries().get( 0 ) ).containsIgnoringCase( "person_id=null" ); + } ); + } + + + @Entity( name = "Person" ) + public static class Person { + + @Id + private Integer id; + + private String name; + + @OneToOne( mappedBy = "person" ) + private Account account; + + public Person() { + } + + public Person(Integer id, String name) { + this.id = id; + this.name = name; + } + + public Integer getId() { + return id; + } + + public String getName() { + return name; + } + + public Account getAccount() { + return account; + } + } + + @SuppressWarnings( { "FieldCanBeLocal", "unused" } ) + @Entity( name = "Account" ) + @Table( name = "ACCOUNT_TABLE" ) + public static class Account { + @Id + private Integer id; + + private String code; + + private Double amount; + + @OneToOne + private Person person; + + public Account() { + } + + public Account(Integer id, String code, Double amount, Person person) { + this.id = id; + this.code = code; + this.amount = amount; + this.person = person; + } + + } +} diff --git a/hibernate-core/src/test/java/org/hibernate/orm/test/notfound/IsNullAndNotFoundTest.java b/hibernate-core/src/test/java/org/hibernate/orm/test/notfound/IsNullAndNotFoundTest.java index 2af11248e3..340216c556 100644 --- a/hibernate-core/src/test/java/org/hibernate/orm/test/notfound/IsNullAndNotFoundTest.java +++ b/hibernate-core/src/test/java/org/hibernate/orm/test/notfound/IsNullAndNotFoundTest.java @@ -13,16 +13,15 @@ import org.hibernate.annotations.NotFoundAction; import org.hibernate.boot.registry.StandardServiceRegistryBuilder; import org.hibernate.cfg.AvailableSettings; -import org.hibernate.testing.FailureExpected; import org.hibernate.testing.jdbc.SQLStatementInspector; import org.hibernate.testing.junit4.BaseNonConfigCoreFunctionalTestCase; -import org.hibernate.testing.orm.junit.EntityManagerFactoryScope; import org.hibernate.testing.orm.junit.Jira; import org.junit.After; import org.junit.Before; import org.junit.Test; import jakarta.persistence.Entity; +import jakarta.persistence.FetchType; import jakarta.persistence.Id; import jakarta.persistence.OneToOne; import jakarta.persistence.Table; @@ -99,10 +98,6 @@ public class IsNullAndNotFoundTest extends BaseNonConfigCoreFunctionalTestCase { } @Test - @FailureExpected( - jiraKey = "HHH-17143", - message = "Conceptually this should render as a left join because of the path terminal; currently uses inner join" - ) public void testAssociationIsNullInWhereClause() { inTransaction( session -> { @@ -130,6 +125,33 @@ public class IsNullAndNotFoundTest extends BaseNonConfigCoreFunctionalTestCase { ); } + @Test + public void testFetchedAssociationIsNullInWhereClause() { + inTransaction( + session -> { + inspector.clear(); + + // should produce an inner join to ACCOUNT_TABLE since it's explicitly selected + // + // ... + // from PERSON p + // join ACCOUNT_TABLE a + // on p.account_id = a.id + // where a.id is null + + final List results = session.createQuery( + "select p.account from Person p where p.account is null", Account.class ) + .getResultList(); + + assertThat( results ).isEmpty(); + + assertThat( inspector.getSqlQueries() ).hasSize( 1 ); + assertThat( inspector.getSqlQueries().get( 0 ) ).containsIgnoringCase( "join" ); + assertThat( inspector.getSqlQueries().get( 0 ) ).doesNotContainIgnoringCase( " left join " ); + } + ); + } + @Test public void testIsNullInWhereClause3() { inTransaction( @@ -225,8 +247,22 @@ public class IsNullAndNotFoundTest extends BaseNonConfigCoreFunctionalTestCase { assertThat( inspector.getSqlQueries() ).hasSize( 1 ); // could physically be a join or exists sub-query assertThat( inspector.getSqlQueries().get( 0 ) ) - .matches( (sql) -> sql.contains( "left join" ) || sql.contains( "where exists" ) ); - assertThat( inspector.getSqlQueries().get( 0 ) ).containsIgnoringCase( ".id is null" ); + .matches( (sql) -> sql.contains( "left join" ) || sql.contains( "not exists" ) ); + } ); + } + + @Test + @Jira( "https://hibernate.atlassian.net/browse/HHH-17384" ) + public void testDeleteAdditionalPredicate() { + inspector.clear(); + + inTransaction( (entityManager) -> { + entityManager.createQuery( "delete from Person p where p.account is null and p.lazyAccount.code <>'aaa'" ).executeUpdate(); + + assertThat( inspector.getSqlQueries() ).hasSize( 1 ); + // could physically be a join or exists sub-query + assertThat( inspector.getSqlQueries().get( 0 ) ) + .matches( (sql) -> sql.contains( "left join" ) || sql.contains( "not exists" ) ); } ); } @@ -240,8 +276,7 @@ public class IsNullAndNotFoundTest extends BaseNonConfigCoreFunctionalTestCase { assertThat( inspector.getSqlQueries() ).hasSize( 1 ); // could physically be a join or exists sub-query assertThat( inspector.getSqlQueries().get( 0 ) ) - .matches( (sql) -> sql.contains( "left join" ) || sql.contains( "where exists" ) ); - assertThat( inspector.getSqlQueries().get( 0 ) ).containsIgnoringCase( ".id is null" ); + .matches( (sql) -> sql.contains( "left join" ) || sql.contains( "not exists" ) ); } ); } @@ -271,6 +306,9 @@ public class IsNullAndNotFoundTest extends BaseNonConfigCoreFunctionalTestCase { @NotFound(action = NotFoundAction.IGNORE) private Account account; + @OneToOne(fetch = FetchType.LAZY) + private Account lazyAccount; + Person() { }