From 1b144a46b2fb14a51189695b8060df7cd67328e0 Mon Sep 17 00:00:00 2001 From: Jakub Soltys Date: Wed, 18 Jun 2025 22:15:30 +0200 Subject: [PATCH] Remove unnecessary join when filtering on relationship id Closes #3349 Signed-off-by: Jakub Soltys --- .../data/jpa/repository/query/QueryUtils.java | 63 +++++++++++++------ .../ReferencingEmbeddedIdExampleEmployee.java | 44 +++++++++++++ .../ReferencingIdClassExampleEmployee.java | 44 +++++++++++++ ...EclipseLinkQueryUtilsIntegrationTests.java | 18 ++++++ .../query/QueryUtilsIntegrationTests.java | 43 ++++++++++++- .../test/resources/META-INF/persistence.xml | 2 + 6 files changed, 194 insertions(+), 20 deletions(-) create mode 100644 spring-data-jpa/src/test/java/org/springframework/data/jpa/domain/sample/ReferencingEmbeddedIdExampleEmployee.java create mode 100644 spring-data-jpa/src/test/java/org/springframework/data/jpa/domain/sample/ReferencingIdClassExampleEmployee.java diff --git a/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/QueryUtils.java b/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/QueryUtils.java index d250c89d7a..21bb552f12 100644 --- a/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/QueryUtils.java +++ b/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/QueryUtils.java @@ -42,6 +42,7 @@ import java.lang.reflect.AnnotatedElement; import java.lang.reflect.Member; import java.util.*; +import java.util.function.Supplier; import java.util.regex.Matcher; import java.util.regex.Pattern; import java.util.stream.Collectors; @@ -88,6 +89,7 @@ * @author Eduard Dudar * @author Yanming Zhou * @author Alim Naizabek + * @author Jakub Soltys */ public abstract class QueryUtils { @@ -773,11 +775,17 @@ static Expression toExpressionRecursively(From from, PropertyPath p boolean isLeafProperty = !property.hasNext(); - boolean requiresOuterJoin = requiresOuterJoin(from, property, isForSelection, hasRequiredOuterJoin); + boolean isRelationshipId = isRelationshipId(from, property); + boolean requiresOuterJoin = requiresOuterJoin(from, property, isForSelection, hasRequiredOuterJoin, isLeafProperty, isRelationshipId); - // if it does not require an outer join and is a leaf, simply get the segment - if (!requiresOuterJoin && isLeafProperty) { - return from.get(segment); + // if it does not require an outer join and is a leaf or relationship id, simply get rest of the segment path + if (!requiresOuterJoin && (isLeafProperty || isRelationshipId)) { + Path trailingPath = from.get(segment); + while (property.hasNext()) { + property = property.next(); + trailingPath = trailingPath.get(property.getSegment()); + } + return trailingPath; } // get or create the join @@ -806,10 +814,12 @@ static Expression toExpressionRecursively(From from, PropertyPath p * to generate an explicit outer join in order to prevent Hibernate to use an inner join instead. see * https://hibernate.atlassian.net/browse/HHH-12999 * @param hasRequiredOuterJoin has a parent already required an outer join? + * @param isLeafProperty is leaf property + * @param isRelationshipId whether property path refers to relationship id * @return whether an outer join is to be used for integrating this attribute in a query. */ static boolean requiresOuterJoin(From from, PropertyPath property, boolean isForSelection, - boolean hasRequiredOuterJoin) { + boolean hasRequiredOuterJoin, boolean isLeafProperty, boolean isRelationshipId) { // already inner joined so outer join is useless if (isAlreadyInnerJoined(from, property.getSegment())) { @@ -818,14 +828,7 @@ static boolean requiresOuterJoin(From from, PropertyPath property, boolean Bindable model = from.getModel(); ManagedType managedType = getManagedTypeForModel(model); - Bindable propertyPathModel = getModelForPath(property, managedType, from); - - // is the attribute of Collection type? - boolean isPluralAttribute = model instanceof PluralAttribute; - - if (propertyPathModel == null && isPluralAttribute) { - return true; - } + Bindable propertyPathModel = getModelForPath(property, managedType, () -> from); if (!(propertyPathModel instanceof Attribute attribute)) { return false; @@ -843,14 +846,36 @@ static boolean requiresOuterJoin(From from, PropertyPath property, boolean boolean isInverseOptionalOneToOne = ONE_TO_ONE == attribute.getPersistentAttributeType() && StringUtils.hasText(getAnnotationProperty(attribute, "mappedBy", "")); - boolean isLeafProperty = !property.hasNext(); - if (isLeafProperty && !isForSelection && !isCollection && !isInverseOptionalOneToOne && !hasRequiredOuterJoin) { + if ((isLeafProperty || isRelationshipId) && !isForSelection && !isCollection && !isInverseOptionalOneToOne && !hasRequiredOuterJoin) { return false; } return hasRequiredOuterJoin || getAnnotationProperty(attribute, "optional", true); } + /** + * Checks if this property path is referencing to relationship id. + * + * @param from the {@link From} to check for attribute model. + * @param property the property path + * @return whether in a query is relationship id. + */ + static boolean isRelationshipId(From from, PropertyPath property) { + if (!property.hasNext()) { + return false; + } + + Bindable model = from.getModel(); + ManagedType managedType = getManagedTypeForModel(model); + Bindable propertyPathModel = getModelForPath(property, managedType, () -> from); + ManagedType propertyPathManagedType = getManagedTypeForModel(propertyPathModel); + Bindable nextPropertyPathModel = getModelForPath(property.next(), propertyPathManagedType, () -> from.get(property.getSegment())); + if (nextPropertyPathModel instanceof SingularAttribute) { + return ((SingularAttribute) nextPropertyPathModel).isId(); + } + return false; + } + @SuppressWarnings("unchecked") static T getAnnotationProperty(Attribute attribute, String propertyName, T defaultValue) { @@ -954,14 +979,14 @@ static void checkSortExpression(Order order) { * @param path the current {@link PropertyPath} segment. * @param managedType primary source for the resulting {@link Bindable}. Can be {@literal null}. * @param fallback must not be {@literal null}. - * @return the corresponding {@link Bindable} of {@literal null}. + * @return the corresponding {@link Bindable}. * @see https://hibernate.atlassian.net/browse/HHH-16144 * @see https://github.com/jakartaee/persistence/issues/562 */ - private static @Nullable Bindable getModelForPath(PropertyPath path, @Nullable ManagedType managedType, - Path fallback) { + private static Bindable getModelForPath(PropertyPath path, @Nullable ManagedType managedType, + Supplier> fallback) { String segment = path.getSegment(); if (managedType != null) { @@ -972,7 +997,7 @@ static void checkSortExpression(Order order) { } } - return fallback.get(segment).getModel(); + return fallback.get().get(segment).getModel(); } /** diff --git a/spring-data-jpa/src/test/java/org/springframework/data/jpa/domain/sample/ReferencingEmbeddedIdExampleEmployee.java b/spring-data-jpa/src/test/java/org/springframework/data/jpa/domain/sample/ReferencingEmbeddedIdExampleEmployee.java new file mode 100644 index 0000000000..818032d7a0 --- /dev/null +++ b/spring-data-jpa/src/test/java/org/springframework/data/jpa/domain/sample/ReferencingEmbeddedIdExampleEmployee.java @@ -0,0 +1,44 @@ +/* + * Copyright 2013-2025 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.springframework.data.jpa.domain.sample; + + +import jakarta.persistence.Entity; +import jakarta.persistence.Id; +import jakarta.persistence.ManyToOne; + +@Entity +public class ReferencingEmbeddedIdExampleEmployee { + + @Id private long id; + @ManyToOne private EmbeddedIdExampleEmployee employee; + + public long getId() { + return id; + } + + public void setId(long id) { + this.id = id; + } + + public EmbeddedIdExampleEmployee getEmployee() { + return employee; + } + + public void setEmployee(EmbeddedIdExampleEmployee employee) { + this.employee = employee; + } +} diff --git a/spring-data-jpa/src/test/java/org/springframework/data/jpa/domain/sample/ReferencingIdClassExampleEmployee.java b/spring-data-jpa/src/test/java/org/springframework/data/jpa/domain/sample/ReferencingIdClassExampleEmployee.java new file mode 100644 index 0000000000..83544066e6 --- /dev/null +++ b/spring-data-jpa/src/test/java/org/springframework/data/jpa/domain/sample/ReferencingIdClassExampleEmployee.java @@ -0,0 +1,44 @@ +/* + * Copyright 2013-2025 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.springframework.data.jpa.domain.sample; + + +import jakarta.persistence.Entity; +import jakarta.persistence.Id; +import jakarta.persistence.ManyToOne; + +@Entity +public class ReferencingIdClassExampleEmployee { + + @Id private long id; + @ManyToOne private IdClassExampleEmployee employee; + + public long getId() { + return id; + } + + public void setId(long id) { + this.id = id; + } + + public IdClassExampleEmployee getEmployee() { + return employee; + } + + public void setEmployee(IdClassExampleEmployee employee) { + this.employee = employee; + } +} diff --git a/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/EclipseLinkQueryUtilsIntegrationTests.java b/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/EclipseLinkQueryUtilsIntegrationTests.java index ce1b95d90e..7c06e01efb 100644 --- a/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/EclipseLinkQueryUtilsIntegrationTests.java +++ b/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/EclipseLinkQueryUtilsIntegrationTests.java @@ -63,4 +63,22 @@ void prefersFetchOverJoin() { assertThat(from.getJoins()).hasSize(1); } + + @Test // GH-3349 + @Override + void doesNotCreateJoinForRelationshipSimpleId() { + //eclipse link produces join for path.get(relationship) + } + + @Test // GH-3349 + @Override + void doesNotCreateJoinForRelationshipEmbeddedId() { + //eclipse link produces join for path.get(relationship) + } + + @Test // GH-3349 + @Override + void doesNotCreateJoinForRelationshipIdClass() { + //eclipse link produces join for path.get(relationship) + } } diff --git a/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/QueryUtilsIntegrationTests.java b/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/QueryUtilsIntegrationTests.java index a7aecc36a7..702c65199b 100644 --- a/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/QueryUtilsIntegrationTests.java +++ b/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/QueryUtilsIntegrationTests.java @@ -34,7 +34,6 @@ import jakarta.persistence.criteria.JoinType; import jakarta.persistence.criteria.Nulls; import jakarta.persistence.criteria.Path; -import jakarta.persistence.criteria.Nulls; import jakarta.persistence.criteria.Root; import jakarta.persistence.spi.PersistenceProvider; import jakarta.persistence.spi.PersistenceProviderResolver; @@ -56,6 +55,8 @@ import org.springframework.data.jpa.domain.sample.Invoice; import org.springframework.data.jpa.domain.sample.InvoiceItem; import org.springframework.data.jpa.domain.sample.Order; +import org.springframework.data.jpa.domain.sample.ReferencingEmbeddedIdExampleEmployee; +import org.springframework.data.jpa.domain.sample.ReferencingIdClassExampleEmployee; import org.springframework.data.jpa.domain.sample.User; import org.springframework.data.jpa.infrastructure.HibernateTestUtils; import org.springframework.data.mapping.PropertyPath; @@ -71,6 +72,7 @@ * @author Patrice Blanchardie * @author Diego Krupitza * @author Krzysztof Krason + * @author Jakub Soltys */ @ExtendWith(SpringExtension.class) @ContextConfiguration("classpath:infrastructure.xml") @@ -387,6 +389,45 @@ void demonstrateDifferentBehaviorOfGetJoin() { assertThat(root.getJoins()).hasSize(getNumberOfJoinsAfterCreatingAPath()); } + @Test // GH-3349 + void doesNotCreateJoinForRelationshipSimpleId() { + + CriteriaBuilder builder = em.getCriteriaBuilder(); + CriteriaQuery query = builder.createQuery(User.class); + Root from = query.from(User.class); + + QueryUtils.toExpressionRecursively(from, PropertyPath.from("manager.id", User.class)); + + assertThat(from.getFetches()).hasSize(0); + assertThat(from.getJoins()).hasSize(0); + } + + @Test // GH-3349 + void doesNotCreateJoinForRelationshipEmbeddedId() { + + CriteriaBuilder builder = em.getCriteriaBuilder(); + CriteriaQuery query = builder.createQuery(ReferencingEmbeddedIdExampleEmployee.class); + Root from = query.from(ReferencingEmbeddedIdExampleEmployee.class); + + QueryUtils.toExpressionRecursively(from, PropertyPath.from("employee.employeePk.employeeId", ReferencingEmbeddedIdExampleEmployee.class)); + + assertThat(from.getFetches()).hasSize(0); + assertThat(from.getJoins()).hasSize(0); + } + + @Test // GH-3349 + void doesNotCreateJoinForRelationshipIdClass() { + + CriteriaBuilder builder = em.getCriteriaBuilder(); + CriteriaQuery query = builder.createQuery(ReferencingIdClassExampleEmployee.class); + Root from = query.from(ReferencingIdClassExampleEmployee.class); + + QueryUtils.toExpressionRecursively(from, PropertyPath.from("employee.empId", ReferencingIdClassExampleEmployee.class)); + + assertThat(from.getFetches()).hasSize(0); + assertThat(from.getJoins()).hasSize(0); + } + int getNumberOfJoinsAfterCreatingAPath() { return 0; } diff --git a/spring-data-jpa/src/test/resources/META-INF/persistence.xml b/spring-data-jpa/src/test/resources/META-INF/persistence.xml index a12c866d21..9f497edf95 100644 --- a/spring-data-jpa/src/test/resources/META-INF/persistence.xml +++ b/spring-data-jpa/src/test/resources/META-INF/persistence.xml @@ -27,9 +27,11 @@ org.springframework.data.jpa.domain.sample.EmbeddedIdExampleEmployeePK org.springframework.data.jpa.domain.sample.EmbeddedIdExampleEmployee org.springframework.data.jpa.domain.sample.EmbeddedIdExampleDepartment + org.springframework.data.jpa.domain.sample.ReferencingEmbeddedIdExampleEmployee org.springframework.data.jpa.domain.sample.EmployeeWithName org.springframework.data.jpa.domain.sample.IdClassExampleEmployee org.springframework.data.jpa.domain.sample.IdClassExampleDepartment + org.springframework.data.jpa.domain.sample.ReferencingIdClassExampleEmployee org.springframework.data.jpa.domain.sample.Invoice org.springframework.data.jpa.domain.sample.InvoiceItem org.springframework.data.jpa.domain.sample.Item