[fix] Add proper JOIN FETCH clause (darrachequesne#67)

There was a regression in 2422028, where a JOIN FETCH would be added for `@OneToMany` relationships, triggering the dreaded "firstResult/maxResults specified with collection fetch; applying in memory".

Author: darrachequesne

src/main/java/org/springframework/data/jpa/datatables/SpecificationBuilder.java

@@ -1,5 +1,6 @@
 package org.springframework.data.jpa.datatables;
 
+import org.hibernate.jpa.criteria.path.AbstractPathImpl;
 import org.springframework.data.jpa.datatables.mapping.DataTablesInput;
 import org.springframework.data.jpa.domain.Specification;
 
@@ -24,17 +25,12 @@ private class DataTablesSpecification<S> implements Specification<S> {
 
  @Override
  public Predicate toPredicate(Root<S> root, CriteriaQuery<?> query, CriteriaBuilder criteriaBuilder) {
- initPredicatesRecursively(tree, root, criteriaBuilder);
-
- boolean isCountQuery = query.getResultType() == Long.class;
- if (!isCountQuery) {
- addFetchRecursively(tree, root);
- }
+ initPredicatesRecursively(tree, root, root, criteriaBuilder);
 
  return createFinalPredicate(criteriaBuilder);
  }
 
- private void initPredicatesRecursively(Node<Filter> node, From<S, S> from, CriteriaBuilder criteriaBuilder) {
+ private void initPredicatesRecursively(Node<Filter> node, From<S, S> from, FetchParent<S, S> fetch, CriteriaBuilder criteriaBuilder) {
  if (node.isLeaf()) {
  boolean hasColumnFilter = node.getData() != null;
  if (hasColumnFilter) {
@@ -46,19 +42,20 @@ private void initPredicatesRecursively(Node<Filter> node, From<S, S> from, Crite
  }
  }
  for (Node<Filter> child : node.getChildren()) {
- From<S, S> childFrom = from;
- if (!child.isLeaf()) {
- childFrom = from.join(child.getName(), JoinType.LEFT);
+ Path<Object> path = from.get(child.getName());
+ if (path instanceof AbstractPathImpl) {
+ if (((AbstractPathImpl) path).getAttribute().isCollection()) {
+ // ignore OneToMany and ManyToMany relationships
+ continue;
+ }
+ }
+ if (child.isLeaf()) {
+ initPredicatesRecursively(child, from, fetch, criteriaBuilder);
+ } else {
+ Join<S, S> join = from.join(child.getName(), JoinType.LEFT);
+ Fetch<S, S> childFetch = fetch.fetch(child.getName(), JoinType.LEFT);
+ initPredicatesRecursively(child, join, childFetch, criteriaBuilder);
  }
- initPredicatesRecursively(child, childFrom, criteriaBuilder);
- }
- }
-
- private void addFetchRecursively(Node<?> node, FetchParent<S, S> fetch) {
- for (Node<?> child : node.getChildren()) {
- if (child.isLeaf()) continue;
- FetchParent<S, S> childFetch = fetch.fetch(child.getName(), JoinType.LEFT);
- addFetchRecursively(child, childFetch);
  }
  }
 

src/test/java/org/springframework/data/jpa/datatables/model/A.java

@@ -0,0 +1,70 @@
+package org.springframework.data.jpa.datatables.model;
+
+import lombok.*;
+import lombok.experimental.Tolerate;
+
+import javax.persistence.*;
+import java.util.Collections;
+import java.util.List;
+
+import static java.util.Arrays.asList;
+import static java.util.Collections.singletonList;
+
+@Data
+@AllArgsConstructor
+@NoArgsConstructor(access = AccessLevel.PRIVATE)
+@EqualsAndHashCode(exclude = "b")
+@ToString(exclude = "b")
+@Builder
+@Entity
+@Table(name = "a")
+public class A {
+
+ private @Id String name;
+
+ @OneToMany(cascade = CascadeType.ALL)
+ @JoinColumn(name="a_id")
+ private List<B> b;
+
+ @ManyToOne(cascade = CascadeType.ALL)
+ @JoinColumn(name = "c_id")
+ private C c;
+
+ @Embedded
+ private D d;
+
+ private static C C1 = new C("C1", "VAL1", new C("C3", "VAL3", null));
+ private static C C2 = new C("C2", "VAL2", null);
+
+ public static A A1 = A.builder()
+ .name("A1")
+ .b(asList(
+ new B("B1", "VAL1"),
+ new B("B2", "VAL2")
+ ))
+ .c(C1)
+ .d(new D("D1"))
+ .build();
+
+ public static A A2 = A.builder()
+ .name("A2")
+ .b(singletonList(
+ new B("B3", "VAL3")
+ ))
+ .c(C2)
+ .d(new D("D2"))
+ .build();
+
+ public static A A3 = A.builder()
+ .name("A3")
+ .b(Collections.<B>emptyList())
+ .c(C2)
+ .build();
+
+ public static List<A> ALL = asList(
+ A1,
+ A2,
+ A3
+ );
+
+}

src/test/java/org/springframework/data/jpa/datatables/model/B.java

@@ -0,0 +1,20 @@
+package org.springframework.data.jpa.datatables.model;
+
+import lombok.AccessLevel;
+import lombok.AllArgsConstructor;
+import lombok.Data;
+import lombok.NoArgsConstructor;
+
+import javax.persistence.Entity;
+import javax.persistence.Id;
+import javax.persistence.Table;
+
+@Data
+@AllArgsConstructor
+@NoArgsConstructor(access = AccessLevel.PRIVATE)
+@Entity
+@Table(name = "b")
+class B {
+ private @Id String name;
+ private String value;
+}

src/test/java/org/springframework/data/jpa/datatables/model/C.java

@@ -0,0 +1,26 @@
+package org.springframework.data.jpa.datatables.model;
+
+import lombok.AccessLevel;
+import lombok.AllArgsConstructor;
+import lombok.Data;
+import lombok.NoArgsConstructor;
+import lombok.experimental.Tolerate;
+
+import javax.persistence.*;
+
+@Data
+@AllArgsConstructor
+@NoArgsConstructor(access = AccessLevel.PRIVATE)
+@Entity
+@Table(name = "c")
+class C {
+
+ private @Id String name;
+
+ private String value;
+
+ @ManyToOne(cascade = CascadeType.ALL)
+ @JoinColumn(name = "parent_id")
+ private C parent;
+
+}

src/test/java/org/springframework/data/jpa/datatables/model/D.java

@@ -0,0 +1,17 @@
+package org.springframework.data.jpa.datatables.model;
+
+import lombok.AccessLevel;
+import lombok.AllArgsConstructor;
+import lombok.Data;
+import lombok.NoArgsConstructor;
+import lombok.experimental.Tolerate;
+
+import javax.persistence.Embeddable;
+
+@Data
+@AllArgsConstructor
+@NoArgsConstructor(access = AccessLevel.PRIVATE)
+@Embeddable
+class D {
+ private String value;
+}

src/test/java/org/springframework/data/jpa/datatables/qrepository/QRelationshipsRepository.java

@@ -0,0 +1,7 @@
+package org.springframework.data.jpa.datatables.qrepository;
+
+import org.springframework.data.jpa.datatables.model.A;
+
+public interface QRelationshipsRepository extends QDataTablesRepository<A, String> {
+
+}

src/test/java/org/springframework/data/jpa/datatables/qrepository/QRelationshipsRepositoryTest.java

@@ -0,0 +1,33 @@
+package org.springframework.data.jpa.datatables.qrepository;
+
+import org.junit.Ignore;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.data.jpa.datatables.Config;
+import org.springframework.data.jpa.datatables.QConfig;
+import org.springframework.data.jpa.datatables.mapping.DataTablesInput;
+import org.springframework.data.jpa.datatables.mapping.DataTablesOutput;
+import org.springframework.data.jpa.datatables.model.A;
+import org.springframework.data.jpa.datatables.repository.RelationshipsRepositoryTest;
+import org.springframework.test.context.ContextConfiguration;
+import org.springframework.test.context.junit4.SpringJUnit4ClassRunner;
+
+@RunWith(SpringJUnit4ClassRunner.class)
+@ContextConfiguration(classes = {Config.class, QConfig.class})
+public class QRelationshipsRepositoryTest extends RelationshipsRepositoryTest {
+ @Autowired
+ private QRelationshipsRepository repository;
+
+ @Override
+ protected DataTablesOutput<A> getOutput(DataTablesInput input) {
+ return repository.findAll(input);
+ }
+
+ @Test
+ @Ignore
+ @Override
+ public void checkFetchJoin() {
+ // see https://github.com/darrachequesne/spring-data-jpa-datatables/issues/7
+ }
+}

src/test/java/org/springframework/data/jpa/datatables/repository/RelationshipsRepository.java

@@ -0,0 +1,7 @@
+package org.springframework.data.jpa.datatables.repository;
+
+import org.springframework.data.jpa.datatables.model.A;
+
+interface RelationshipsRepository extends DataTablesRepository<A, String> {
+
+}

src/test/java/org/springframework/data/jpa/datatables/repository/RelationshipsRepositoryTest.java

@@ -0,0 +1,85 @@
+package org.springframework.data.jpa.datatables.repository;
+
+import org.hibernate.SessionFactory;
+import org.hibernate.stat.Statistics;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.data.jpa.datatables.Config;
+import org.springframework.data.jpa.datatables.mapping.DataTablesInput;
+import org.springframework.data.jpa.datatables.mapping.DataTablesOutput;
+import org.springframework.data.jpa.datatables.model.A;
+import org.springframework.test.context.ContextConfiguration;
+import org.springframework.test.context.junit4.SpringJUnit4ClassRunner;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+@RunWith(SpringJUnit4ClassRunner.class)
+@ContextConfiguration(classes = Config.class)
+public class RelationshipsRepositoryTest {
+ protected DataTablesInput input;
+
+ @Autowired
+ private RelationshipsRepository repository;
+
+ @Autowired
+ private SessionFactory sessionFactory;
+
+ protected DataTablesOutput<A> getOutput(DataTablesInput input) {
+ return repository.findAll(input);
+ }
+
+ @Before
+ public void init() {
+ repository.deleteAll();
+ repository.save(A.ALL);
+ input = getBasicInput();
+ }
+
+ @Test
+ public void manyToOne() {
+ input.getColumn("c.value").setSearchValue("VAL2");
+ DataTablesOutput<A> output = getOutput(input);
+ assertThat(output.getData()).containsOnly(A.A2, A.A3);
+ }
+
+ @Test
+ public void twoLevels() {
+ input.getColumn("c.parent.value").setSearchValue("VAL3");
+ DataTablesOutput<A> output = getOutput(input);
+ assertThat(output.getData()).containsOnly(A.A1);
+ }
+
+ @Test
+ public void embedded() {
+ input.getColumn("d.value").setSearchValue("D1");
+ DataTablesOutput<A> output = getOutput(input);
+ assertThat(output.getData()).containsOnly(A.A1);
+ }
+
+ @Test
+ public void checkFetchJoin() {
+ Statistics statistics = sessionFactory.getStatistics();
+ statistics.setStatisticsEnabled(true);
+
+ DataTablesOutput<A> output = getOutput(input);
+
+ assertThat(output.getRecordsFiltered()).isEqualTo(3);
+ assertThat(statistics.getPrepareStatementCount()).isEqualTo(2);
+ assertThat(statistics.getEntityLoadCount()).isEqualTo(3 /* A */ + 3 /* C */);
+ }
+
+ private static DataTablesInput getBasicInput() {
+ DataTablesInput input = new DataTablesInput();
+ input.addColumn("name", true, true, "");
+ input.addColumn("b.name", true, true, "");
+ input.addColumn("b.value", true, true, "");
+ input.addColumn("c.name", true, true, "");
+ input.addColumn("c.value", true, true, "");
+ input.addColumn("c.parent.name", true, true, "");
+ input.addColumn("c.parent.value", true, true, "");
+ input.addColumn("d.value", true, true, "");
+ return input;
+ }
+}