Skip to content

Commit

Permalink
HHH-17384 Fix @NotFound to-one association nullness handling
Browse files Browse the repository at this point in the history
  • Loading branch information
mbladel authored and beikov committed Dec 1, 2023
1 parent 3e96ddc commit e369015
Show file tree
Hide file tree
Showing 11 changed files with 747 additions and 57 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,11 @@ private boolean supportsOffsetFetchClause() {
return getDialect().getVersion().isSameOrAfter( 10, 5 );
}

@Override
protected boolean supportsJoinInMutationStatementSubquery() {
return false;
}

@Override
public void visitBinaryArithmeticExpression(BinaryArithmeticExpression arithmeticExpression) {
final BinaryArithmeticOperator operator = arithmeticExpression.getOperator();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -302,4 +302,9 @@ private boolean supportsOffsetFetchClausePercentWithTies() {
// Introduction of PERCENT support https://github.com/h2database/h2database/commit/f45913302e5f6ad149155a73763c0c59d8205849
return getDialect().getVersion().isSameOrAfter( 1, 4, 198 );
}

@Override
protected boolean supportsJoinInMutationStatementSubquery() {
return false;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -261,6 +261,11 @@ private boolean supportsOffsetFetchClause() {
return true;
}

@Override
protected boolean supportsJoinInMutationStatementSubquery() {
return false;
}

@Override
public void visitBinaryArithmeticExpression(BinaryArithmeticExpression arithmeticExpression) {
final BinaryArithmeticOperator operator = arithmeticExpression.getOperator();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -307,4 +307,9 @@ private boolean supportsOffsetFetchClausePercentWithTies() {
// Introduction of PERCENT support https://github.com/h2database/h2database/commit/f45913302e5f6ad149155a73763c0c59d8205849
return getDialect().getVersion().isSameOrAfter( 1, 4, 198 );
}

@Override
protected boolean supportsJoinInMutationStatementSubquery() {
return false;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -1521,7 +1521,7 @@ public static class EntityB {
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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3530,10 +3530,14 @@ private TableGroup getPluralPartTableGroup(PluralTableGroup pluralTableGroup, Sq
}

private <X> X prepareReusablePath(SqmPath<?> sqmPath, Supplier<X> supplier) {
return prepareReusablePath( sqmPath, fromClauseIndexStack.getCurrent(), supplier );
return prepareReusablePath( sqmPath, fromClauseIndexStack.getCurrent(), supplier, false );
}

private <X> X prepareReusablePath(SqmPath<?> sqmPath, FromClauseIndex fromClauseIndex, Supplier<X> supplier) {
private <X> X prepareReusablePath(
SqmPath<?> sqmPath,
FromClauseIndex fromClauseIndex,
Supplier<X> supplier,
boolean allowLeftJoins) {
final Consumer<TableGroup> implicitJoinChecker;
if ( getCurrentClauseStack().getCurrent() != Clause.SET_EXPRESSION ) {
implicitJoinChecker = tg -> {};
Expand All @@ -3556,7 +3560,8 @@ private <X> X prepareReusablePath(SqmPath<?> sqmPath, FromClauseIndex fromClause
fromClauseIndex.getTableGroup( sqmPath.getLhs().getNavigablePath() ),
sqmPath
),
sqmPath
sqmPath,
allowLeftJoins
);
if ( createdTableGroup != null ) {
if ( sqmPath instanceof SqmTreatedPath<?, ?> ) {
Expand Down Expand Up @@ -3610,7 +3615,7 @@ else if ( createdParentTableGroup instanceof PluralTableGroup ) {
}
else {
newTableGroup = getActualTableGroup(
createTableGroup( createdParentTableGroup, parentPath ),
createTableGroup( createdParentTableGroup, parentPath, false ),
sqmPath
);
}
Expand All @@ -3624,9 +3629,21 @@ else if ( sqmPath instanceof SqmTreatedPath<?, ?> ) {
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
Expand All @@ -3650,9 +3667,6 @@ && getCurrentClauseStack().getCurrent() != Clause.SELECT
}
}
}
registerPathAttributeEntityNameUsage( sqmPath, parentTableGroup );

return parentTableGroup;
}

private void prepareForSelection(SqmPath<?> selectionPath) {
Expand All @@ -3678,7 +3692,8 @@ private void prepareForSelection(SqmPath<?> selectionPath) {
// 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 );
Expand All @@ -3699,7 +3714,7 @@ private void prepareForSelection(SqmPath<?> selectionPath) {
}
}

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(
Expand Down Expand Up @@ -3731,18 +3746,30 @@ private TableGroup createTableGroup(TableGroup parentTableGroup, SqmPath<?> join
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
Expand All @@ -3762,6 +3789,10 @@ private TableGroup createTableGroup(TableGroup parentTableGroup, SqmPath<?> join
// 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 );
}
}
}

Expand All @@ -3774,6 +3805,16 @@ private TableGroup createTableGroup(TableGroup parentTableGroup, SqmPath<?> join
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;
Expand Down Expand Up @@ -7530,11 +7571,30 @@ public LikePredicate visitLikePredicate(SqmLikePredicate predicate) {

@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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,7 @@ && hasNotFound( mapping )
}
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 -
Expand Down

0 comments on commit e369015

Please sign in to comment.