[fix] Add proper JOIN FETCH clause (#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".

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;
+    }
+}