From 858f5bb7de7e6b94fcb47248806b43aa34b5f1dd Mon Sep 17 00:00:00 2001 From: Oliver Gierke Date: Wed, 25 Sep 2013 12:11:50 +0200 Subject: [PATCH] DATAJPA-401 - Non-optional associations don't trigger join creation anymore. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When building Expression instances for attribute traversals we now inspect the mapping annotation to detect non-optional associations and prevent an extra left join from being created. This is because the join is only necessary to not drop null values from the result. Added integration tests for Hibernate, EclipseLink and OpenJpa. EclipseLink needs a bit of special treatment as it exposes the inner join being created for a plain ….get(…) attribute traversal. --- .../data/jpa/repository/query/QueryUtils.java | 64 ++++++++++++------- .../data/jpa/domain/sample/Customer.java | 28 ++++++++ .../data/jpa/domain/sample/Order.java | 32 ++++++++++ ...EclipseLinkQueryUtilsIntegrationTests.java | 49 ++++++++++++++ .../OpenJpaQueryUtilsIntegrationTests.java | 26 ++++++++ .../query/QueryUtilsIntegrationTests.java | 33 ++++++++++ src/test/resources/META-INF/persistence.xml | 2 + 7 files changed, 210 insertions(+), 24 deletions(-) create mode 100644 src/test/java/org/springframework/data/jpa/domain/sample/Customer.java create mode 100644 src/test/java/org/springframework/data/jpa/domain/sample/Order.java create mode 100644 src/test/java/org/springframework/data/jpa/repository/query/EclipseLinkQueryUtilsIntegrationTests.java create mode 100644 src/test/java/org/springframework/data/jpa/repository/query/OpenJpaQueryUtilsIntegrationTests.java diff --git a/src/main/java/org/springframework/data/jpa/repository/query/QueryUtils.java b/src/main/java/org/springframework/data/jpa/repository/query/QueryUtils.java index a6fbf2e7e..761164b3f 100644 --- a/src/main/java/org/springframework/data/jpa/repository/query/QueryUtils.java +++ b/src/main/java/org/springframework/data/jpa/repository/query/QueryUtils.java @@ -18,17 +18,24 @@ package org.springframework.data.jpa.repository.query; import static java.util.regex.Pattern.*; import static javax.persistence.metamodel.Attribute.PersistentAttributeType.*; +import java.lang.annotation.Annotation; +import java.lang.reflect.AnnotatedElement; +import java.lang.reflect.Member; import java.util.ArrayList; import java.util.Collections; +import java.util.HashMap; import java.util.HashSet; import java.util.Iterator; import java.util.List; import java.util.Locale; +import java.util.Map; import java.util.Set; import java.util.regex.Matcher; import java.util.regex.Pattern; import javax.persistence.EntityManager; +import javax.persistence.ManyToOne; +import javax.persistence.OneToOne; import javax.persistence.Parameter; import javax.persistence.Query; import javax.persistence.TypedQuery; @@ -42,9 +49,9 @@ import javax.persistence.criteria.Root; import javax.persistence.metamodel.Attribute; import javax.persistence.metamodel.Attribute.PersistentAttributeType; import javax.persistence.metamodel.Bindable; -import javax.persistence.metamodel.Bindable.BindableType; import javax.persistence.metamodel.ManagedType; +import org.springframework.core.annotation.AnnotationUtils; import org.springframework.data.domain.Sort; import org.springframework.data.domain.Sort.Order; import org.springframework.data.mapping.PropertyPath; @@ -81,7 +88,7 @@ public abstract class QueryUtils { private static final String EQUALS_CONDITION_STRING = "%s.%s = :%s"; private static final Pattern ORDER_BY = Pattern.compile(".*order\\s+by\\s+.*", CASE_INSENSITIVE); - private static final Set ASSOCIATION_TYPES; + private static final Map> ASSOCIATION_TYPES; static { @@ -104,13 +111,13 @@ public abstract class QueryUtils { COUNT_MATCH = compile(builder.toString(), CASE_INSENSITIVE); - Set persistentAttributeTypes = new HashSet(); - persistentAttributeTypes.add(ONE_TO_ONE); - persistentAttributeTypes.add(ONE_TO_MANY); - persistentAttributeTypes.add(MANY_TO_ONE); - persistentAttributeTypes.add(MANY_TO_MANY); + Map> persistentAttributeTypes = new HashMap>(); + persistentAttributeTypes.put(ONE_TO_ONE, OneToOne.class); + persistentAttributeTypes.put(ONE_TO_MANY, null); + persistentAttributeTypes.put(MANY_TO_ONE, ManyToOne.class); + persistentAttributeTypes.put(MANY_TO_MANY, null); - ASSOCIATION_TYPES = Collections.unmodifiableSet(persistentAttributeTypes); + ASSOCIATION_TYPES = Collections.unmodifiableMap(persistentAttributeTypes); } /** @@ -442,7 +449,7 @@ public abstract class QueryUtils { propertyPathModel = from.get(property.getSegment()).getModel(); } - if (property.isCollection() || isEntityPath(propertyPathModel)) { + if (property.isCollection() || requiresJoin(propertyPathModel)) { Join join = getOrCreateJoin(from, property.getSegment()); return (Expression) (property.hasNext() ? toExpressionRecursively(join, property.next()) : join); } else { @@ -452,29 +459,38 @@ public abstract class QueryUtils { } /** - * Returns whether the given {@code propertyPathModel} can be considered referring an entity. + * Returns whether the given {@code propertyPathModel} requires the creation of a join. This is the case if we find a + * non-optional association. * * @param propertyPathModel must not be {@literal null}. * @return */ - private static boolean isEntityPath(Bindable propertyPathModel) { + private static boolean requiresJoin(Bindable propertyPathModel) { - if (BindableType.ENTITY_TYPE.equals(propertyPathModel.getBindableType())) { + if (!(propertyPathModel instanceof Attribute)) { + return false; + } + + Attribute attribute = (Attribute) propertyPathModel; + + if (!ASSOCIATION_TYPES.containsKey(attribute.getPersistentAttributeType())) { + return false; + } + + Class associationAnnotation = ASSOCIATION_TYPES.get(attribute.getPersistentAttributeType()); + + if (associationAnnotation == null) { + return false; + } + + Member member = attribute.getJavaMember(); + + if (!(member instanceof AnnotatedElement)) { return true; } - if (propertyPathModel instanceof Attribute) { - - Attribute attribute = (Attribute) propertyPathModel; - - if (attribute.isAssociation()) { - return true; - } - - return ASSOCIATION_TYPES.contains(attribute.getPersistentAttributeType()); - } - - return false; + Annotation annotation = AnnotationUtils.getAnnotation((AnnotatedElement) member, associationAnnotation); + return annotation == null ? true : (Boolean) AnnotationUtils.getValue(annotation, "optional"); } static Expression toExpressionRecursively(Path path, PropertyPath property) { diff --git a/src/test/java/org/springframework/data/jpa/domain/sample/Customer.java b/src/test/java/org/springframework/data/jpa/domain/sample/Customer.java new file mode 100644 index 000000000..a6819e3af --- /dev/null +++ b/src/test/java/org/springframework/data/jpa/domain/sample/Customer.java @@ -0,0 +1,28 @@ +/* + * Copyright 2013 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 + * + * http://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 javax.persistence.Entity; +import javax.persistence.Id; + +/** + * @author Oliver Gierke + */ +@Entity +public class Customer { + + @Id Long id; +} diff --git a/src/test/java/org/springframework/data/jpa/domain/sample/Order.java b/src/test/java/org/springframework/data/jpa/domain/sample/Order.java new file mode 100644 index 000000000..3016651f7 --- /dev/null +++ b/src/test/java/org/springframework/data/jpa/domain/sample/Order.java @@ -0,0 +1,32 @@ +/* + * Copyright 2013 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 + * + * http://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 javax.persistence.Entity; +import javax.persistence.Id; +import javax.persistence.ManyToOne; +import javax.persistence.Table; + +/** + * @author Oliver Gierke + */ +@Entity +@Table(name = "ORDERS") +public class Order { + + @Id Long id; + @ManyToOne(optional = false) Customer customer; +} diff --git a/src/test/java/org/springframework/data/jpa/repository/query/EclipseLinkQueryUtilsIntegrationTests.java b/src/test/java/org/springframework/data/jpa/repository/query/EclipseLinkQueryUtilsIntegrationTests.java new file mode 100644 index 000000000..5c3127805 --- /dev/null +++ b/src/test/java/org/springframework/data/jpa/repository/query/EclipseLinkQueryUtilsIntegrationTests.java @@ -0,0 +1,49 @@ +/* + * Copyright 2013 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 + * + * http://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.repository.query; + +import static org.hamcrest.Matchers.*; +import static org.junit.Assert.*; + +import java.util.Set; + +import javax.persistence.criteria.Join; +import javax.persistence.criteria.JoinType; +import javax.persistence.criteria.Root; + +import org.springframework.data.jpa.domain.sample.Order; +import org.springframework.test.context.ContextConfiguration; + +/** + * @author Oliver Gierke + */ +@ContextConfiguration("classpath:eclipselink.xml") +public class EclipseLinkQueryUtilsIntegrationTests extends QueryUtilsIntegrationTests { + + /** + * Required as EclipseLink generates an inner join for plain association traversal. + */ + @Override + protected void assertNoJoinRequestedForOptionalAssociation(Root root) { + + Set> joins = root.getJoins(); + assertThat(joins, hasSize(1)); + + Join join = joins.iterator().next(); + assertThat(join.getAttribute().getName(), is("manager")); + assertThat(join.getJoinType(), is(JoinType.INNER)); + } +} diff --git a/src/test/java/org/springframework/data/jpa/repository/query/OpenJpaQueryUtilsIntegrationTests.java b/src/test/java/org/springframework/data/jpa/repository/query/OpenJpaQueryUtilsIntegrationTests.java new file mode 100644 index 000000000..aa89c66b1 --- /dev/null +++ b/src/test/java/org/springframework/data/jpa/repository/query/OpenJpaQueryUtilsIntegrationTests.java @@ -0,0 +1,26 @@ +/* + * Copyright 2013 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 + * + * http://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.repository.query; + +import org.springframework.test.context.ContextConfiguration; + +/** + * @author Oliver Gierke + */ +@ContextConfiguration("classpath:openjpa.xml") +public class OpenJpaQueryUtilsIntegrationTests extends QueryUtilsIntegrationTests { + +} diff --git a/src/test/java/org/springframework/data/jpa/repository/query/QueryUtilsIntegrationTests.java b/src/test/java/org/springframework/data/jpa/repository/query/QueryUtilsIntegrationTests.java index 070efe7ed..682b4c166 100644 --- a/src/test/java/org/springframework/data/jpa/repository/query/QueryUtilsIntegrationTests.java +++ b/src/test/java/org/springframework/data/jpa/repository/query/QueryUtilsIntegrationTests.java @@ -26,6 +26,7 @@ import javax.persistence.criteria.Root; import org.junit.Test; import org.junit.runner.RunWith; +import org.springframework.data.jpa.domain.sample.Order; import org.springframework.data.jpa.domain.sample.User; import org.springframework.data.mapping.PropertyPath; import org.springframework.test.context.ContextConfiguration; @@ -60,4 +61,36 @@ public class QueryUtilsIntegrationTests { assertThat(from.getJoins(), hasSize(1)); } + + /** + * @see DATAJPA-401 + */ + @Test + public void createsJoinForOptionalAssociation() { + + CriteriaBuilder builder = em.getCriteriaBuilder(); + CriteriaQuery query = builder.createQuery(User.class); + Root root = query.from(User.class); + + QueryUtils.toExpressionRecursively(root, PropertyPath.from("manager", User.class)); + + assertThat(root.getJoins(), hasSize(1)); + } + + /** + * @see DATAJPA-401 + */ + @Test + public void doesNotCreateAJoinForNonOptionalAssociation() { + + CriteriaBuilder builder = em.getCriteriaBuilder(); + CriteriaQuery query = builder.createQuery(Order.class); + Root root = query.from(Order.class); + + QueryUtils.toExpressionRecursively(root, PropertyPath.from("customer", Order.class)); + } + + protected void assertNoJoinRequestedForOptionalAssociation(Root root) { + assertThat(root.getJoins(), is(empty())); + } } diff --git a/src/test/resources/META-INF/persistence.xml b/src/test/resources/META-INF/persistence.xml index b91202a06..2c107f047 100644 --- a/src/test/resources/META-INF/persistence.xml +++ b/src/test/resources/META-INF/persistence.xml @@ -24,6 +24,8 @@ 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.Customer + org.springframework.data.jpa.domain.sample.Order true