add explicit join (#1364)

Co-authored-by: Chandrasekar Rajasekar <[email protected]>
yahoo · Jul 9, 2020 · 40e5bad · 40e5bad
1 parent 3188eb7
commit 40e5bad
Show file tree

Hide file tree

Showing 7 changed files with 201 additions and 21 deletions.
diff --git a/...e-hibernate/src/main/java/com/yahoo/elide/core/hibernate/hql/AbstractHQLQueryBuilder.java b/...e-hibernate/src/main/java/com/yahoo/elide/core/hibernate/hql/AbstractHQLQueryBuilder.java
@@ -136,10 +136,37 @@ protected String getJoinClauseFromFilters(FilterExpression filterExpression, boo
         Collection<FilterPredicate> predicates = filterExpression.accept(visitor);
 
         return predicates.stream()
-                .map(predicate -> extractJoinClause(predicate, skipFetches))
+                .map(predicate -> extractJoinClause(predicate.getPath(), skipFetches))
                 .collect(Collectors.joining(SPACE));
     }
 
+
+    /**
+     * Extracts all the HQL JOIN clauses from given filter expression.
+     * @param sorting the sort expression to extract a join clause from
+     * @return an HQL join clause
+     */
+    protected String getJoinClauseFromSort(Optional<Sorting> sorting) {
+        return getJoinClauseFromSort(sorting, false);
+    }
+
+    /**
+     * Extracts all the HQL JOIN clauses from given filter expression.
+     * @param sorting the sort expression to extract a join clause from
+     * @param skipFetches JOIN but don't FETCH JOIN a relationship.
+     * @return an HQL join clause
+     */
+    protected String getJoinClauseFromSort(Optional<Sorting> sorting, boolean skipFetches) {
+        if (sorting.isPresent() && !sorting.get().isDefaultInstance()) {
+            Map<Path, Sorting.SortOrder> validSortingRules = sorting.get().getSortingPaths();
+            return validSortingRules.keySet().stream()
+                    .map(path -> extractJoinClause(path, skipFetches))
+                    .collect(Collectors.joining(SPACE));
+        }
+        return "";
+    }
+
+
     /**
      * Modifies the HQL query to add OFFSET and LIMIT.
      * @param query The HQL query object
@@ -153,17 +180,17 @@ protected void addPaginationToQuery(Query query) {
     }
 
     /**
-     * Extracts a join clause from a filter predicate (if it exists).
-     * @param predicate The predicate to examine
+     * Extracts a join clause from a path (if it exists).
+     * @param path The path to examine
      * @param skipFetches Don't fetch join
      * @return A HQL string representing the join
      */
-    private String extractJoinClause(FilterPredicate predicate, boolean skipFetches) {
+    private String extractJoinClause(Path path, boolean skipFetches) {
         StringBuilder joinClause = new StringBuilder();
 
         String previousAlias = null;
 
-        for (Path.PathElement pathElement : predicate.getPath().getPathElements()) {
+        for (Path.PathElement pathElement : path.getPathElements()) {
             String fieldName = pathElement.getFieldName();
             Class<?> typeClass = dictionary.lookupEntityClass(pathElement.getType());
             String typeAlias = getTypeAlias(typeClass);
@@ -236,6 +263,7 @@ protected String extractToOneMergeJoins(Class<?> entityClass, String alias,
                 joinString.append(" LEFT JOIN FETCH ");
                 joinString.append(joinKey);
                 joinString.append(SPACE);
+                alreadyJoined.add(joinKey);
             }
         }
         return joinString.toString();
@@ -244,21 +272,34 @@ protected String extractToOneMergeJoins(Class<?> entityClass, String alias,
     /**
      * Returns a sorting object into a HQL ORDER BY string.
      * @param sorting The sorting object passed from the client
-     * @param sortClass The class to sort.
-     * @param prefixWithAlias Whether the sorting fields should be prefixed by an alias.
      * @return The sorting clause
      */
-    protected String getSortClause(final Optional<Sorting> sorting, Class<?> sortClass, boolean prefixWithAlias) {
+    protected String getSortClause(final Optional<Sorting> sorting) {
         String sortingRules = "";
         if (sorting.isPresent() && !sorting.get().isDefaultInstance()) {
             final Map<Path, Sorting.SortOrder> validSortingRules = sorting.get().getSortingPaths();
             if (!validSortingRules.isEmpty()) {
                 final List<String> ordering = new ArrayList<>();
                 // pass over the sorting rules
                 validSortingRules.forEach((path, order) -> {
-                    String prefix = (prefixWithAlias) ? getTypeAlias(sortClass) + PERIOD : "";
-
-                    ordering.add(prefix + path.getFieldPath() + SPACE
+                    String previousAlias = null;
+                    String aliasedFieldName = null;
+
+                    for (Path.PathElement pathElement : path.getPathElements()) {
+                        String fieldName = pathElement.getFieldName();
+                        Class<?> typeClass = dictionary.lookupEntityClass(pathElement.getType());
+                        previousAlias = previousAlias == null
+                                ? getTypeAlias(typeClass)
+                                : previousAlias;
+                        aliasedFieldName = previousAlias + PERIOD + fieldName;
+
+                        if (!dictionary.isRelation(pathElement.getType(), fieldName)) {
+                            break;
+                        }
+
+                        previousAlias = appendAlias(previousAlias, fieldName);
+                    }
+                    ordering.add(aliasedFieldName + SPACE
                             + (order.equals(Sorting.SortOrder.desc) ? "desc" : "asc"));
                 });
                 sortingRules = " order by " + StringUtils.join(ordering, COMMA);

diff --git a/...ate/src/main/java/com/yahoo/elide/core/hibernate/hql/RootCollectionFetchQueryBuilder.java b/...ate/src/main/java/com/yahoo/elide/core/hibernate/hql/RootCollectionFetchQueryBuilder.java
@@ -51,6 +51,7 @@ public Query build() {
 
             //Build the JOIN clause
             String joinClause =  getJoinClauseFromFilters(filterExpression.get())
+                    + getJoinClauseFromSort(sorting)
                     + extractToOneMergeJoins(entityClass, entityAlias);
 
             boolean requiresDistinct = pagination.isPresent() && containsOneToMany(filterExpression.get());
@@ -75,7 +76,7 @@ public Query build() {
                         + SPACE
                         + filterClause
                         + SPACE
-                        + getSortClause(sorting, entityClass, USE_ALIAS)
+                        + getSortClause(sorting)
             );
 
             //Fill in the query parameters
@@ -88,9 +89,10 @@ public Query build() {
                     + AS
                     + entityAlias
                     + SPACE
+                    + getJoinClauseFromSort(sorting)
                     + extractToOneMergeJoins(entityClass, entityAlias)
                     + SPACE
-                    + getSortClause(sorting, entityClass, USE_ALIAS));
+                    + getSortClause(sorting));
         }
 
         addPaginationToQuery(query);

diff --git a/...nate/src/main/java/com/yahoo/elide/core/hibernate/hql/SubCollectionFetchQueryBuilder.java b/...nate/src/main/java/com/yahoo/elide/core/hibernate/hql/SubCollectionFetchQueryBuilder.java
@@ -74,6 +74,7 @@ public Query build() {
             String filterClause = new FilterTranslator().apply(fe, USE_ALIAS);
 
             String joinClause =  getJoinClauseFromFilters(filterExpression.get())
+                    + getJoinClauseFromSort(sorting)
                     + extractToOneMergeJoins(relationship.getChildType(), childAlias);
 
             //SELECT parent_children from Parent parent JOIN parent.children parent_children
@@ -88,7 +89,7 @@ public Query build() {
                             + filterClause
                             + " AND " + parentAlias + "=:" + parentAlias
                             + SPACE
-                            + getSortClause(sorting, relationship.getChildType(), USE_ALIAS)
+                            + getSortClause(sorting)
             );
 
             supplyFilterQueryParameters(q, predicates);
@@ -99,9 +100,10 @@ public Query build() {
                             + parentName + SPACE + parentAlias
                             + JOIN
                             + parentAlias + PERIOD + relationshipName + SPACE + childAlias
+                            + getJoinClauseFromSort(sorting)
                             + extractToOneMergeJoins(relationship.getChildType(), childAlias)
                             + " WHERE " + parentAlias + "=:" + parentAlias
-                            + getSortClause(sorting, relationship.getChildType(), USE_ALIAS)
+                            + getSortClause(sorting)
         ));
 
         query.setParameter(parentAlias, relationship.getParent());

diff --git a/...e/src/test/java/com/yahoo/elide/datastores/hibernate/hql/AbstractHQLQueryBuilderTest.java b/...e/src/test/java/com/yahoo/elide/datastores/hibernate/hql/AbstractHQLQueryBuilderTest.java
@@ -121,9 +121,9 @@ public void testSortClauseWithoutPrefix() {
         sorting.put(TITLE, Sorting.SortOrder.asc);
         sorting.put(GENRE, Sorting.SortOrder.desc);
 
-        String actual = getSortClause(Optional.of(new SortingImpl(sorting, Book.class, dictionary)), Book.class, NO_ALIAS);
+        String actual = getSortClause(Optional.of(new SortingImpl(sorting, Book.class, dictionary)));
 
-        String expected = " order by title asc,genre desc";
+        String expected = " order by example_Book.title asc,example_Book.genre desc";
         assertEquals(expected, actual);
     }
 
@@ -133,7 +133,7 @@ public void testSortClauseWithPrefix() {
         sorting.put(TITLE, Sorting.SortOrder.asc);
         sorting.put(GENRE, Sorting.SortOrder.desc);
 
-        String actual = getSortClause(Optional.of(new SortingImpl(sorting, Book.class, dictionary)), Book.class, USE_ALIAS);
+        String actual = getSortClause(Optional.of(new SortingImpl(sorting, Book.class, dictionary)));
 
         String expected = " order by example_Book.title asc,example_Book.genre desc";
         assertEquals(expected, actual);
@@ -144,9 +144,9 @@ public void testSortClauseWithJoin() {
         Map<String, Sorting.SortOrder> sorting = new LinkedHashMap<>();
         sorting.put(PUBLISHER + PERIOD + NAME, Sorting.SortOrder.asc);
 
-        String actual = getSortClause(Optional.of(new SortingImpl(sorting, Book.class, dictionary)), Book.class, NO_ALIAS);
+        String actual = getSortClause(Optional.of(new SortingImpl(sorting, Book.class, dictionary)));
 
-        String expected = " order by publisher.name asc";
+        String expected = " order by example_Book_publisher.name asc";
         assertEquals(expected, actual);
     }
 
@@ -155,7 +155,7 @@ public void testSortClauseWithInvalidJoin() {
         Map<String, Sorting.SortOrder> sorting = new LinkedHashMap<>();
         sorting.put(AUTHORS + PERIOD + NAME, Sorting.SortOrder.asc);
 
-        assertThrows(InvalidValueException.class, () -> getSortClause(Optional.of(new SortingImpl(sorting, Book.class, dictionary)), Book.class, NO_ALIAS));
+        assertThrows(InvalidValueException.class, () -> getSortClause(Optional.of(new SortingImpl(sorting, Book.class, dictionary))));
     }
 
     @Test

diff --git a/...st/java/com/yahoo/elide/datastores/hibernate/hql/RootCollectionFetchQueryBuilderTest.java b/...st/java/com/yahoo/elide/datastores/hibernate/hql/RootCollectionFetchQueryBuilderTest.java
@@ -25,6 +25,7 @@
 import example.Author;
 import example.Book;
 import example.Chapter;
+import example.Editor;
 import example.Publisher;
 
 import org.junit.jupiter.api.BeforeAll;
@@ -40,6 +41,11 @@ public class RootCollectionFetchQueryBuilderTest {
     private EntityDictionary dictionary;
 
     private static final String TITLE = "title";
+    private static final String PUBLISHER = "publisher";
+    private static final String EDITOR = "editor";
+    private static final String PERIOD = ".";
+    private static final String NAME = "name";
+    private static final String FIRSTNAME = "firstName";
     private RSQLFilterDialect filterParser;
 
     @BeforeAll
@@ -49,6 +55,7 @@ public void initialize() {
         dictionary.bindEntity(Author.class);
         dictionary.bindEntity(Publisher.class);
         dictionary.bindEntity(Chapter.class);
+        dictionary.bindEntity(Editor.class);
         filterParser = new RSQLFilterDialect(dictionary, new CaseSensitivityStrategy.UseColumnCollation());
     }
 
@@ -183,4 +190,61 @@ public void testRootFetchWithSortingAndFilters() {
 
         assertEquals(expected, actual);
     }
+
+    @Test
+    public void testSortingWithJoin() {
+        RootCollectionFetchQueryBuilder builder = new RootCollectionFetchQueryBuilder(
+                Book.class, dictionary, new TestSessionWrapper());
+
+        Map<String, Sorting.SortOrder> sorting = new HashMap<>();
+        sorting.put(PUBLISHER + PERIOD + NAME, Sorting.SortOrder.asc);
+
+
+        TestQueryWrapper query = (TestQueryWrapper) builder
+                .withPossibleSorting(Optional.of(new SortingImpl(sorting, Book.class, dictionary)))
+                .build();
+
+        String expected =
+                "SELECT example_Book FROM example.Book AS example_Book "
+                        + "LEFT JOIN FETCH example_Book.publisher example_Book_publisher "
+                        + "order by example_Book_publisher.name asc";
+
+        String actual = query.getQueryText();
+        actual = actual.trim().replaceAll(" +", " ");
+        actual = actual.replaceFirst(":id_\\w+", ":id_XXX");
+
+        assertEquals(expected, actual);
+    }
+
+    @Test
+    public void testRootFetchWithRelationshipSortingAndFilters() {
+        RootCollectionFetchQueryBuilder builder = new RootCollectionFetchQueryBuilder(
+                Book.class, dictionary, new TestSessionWrapper());
+
+        Map<String, Sorting.SortOrder> sorting = new HashMap<>();
+        sorting.put(PUBLISHER + PERIOD + EDITOR + PERIOD + FIRSTNAME, Sorting.SortOrder.desc);
+
+        Path.PathElement idPath = new Path.PathElement(Book.class, Chapter.class, "id");
+
+        FilterPredicate idPredicate = new InPredicate(idPath, 1);
+
+
+        TestQueryWrapper query = (TestQueryWrapper) builder
+                .withPossibleSorting(Optional.of(new SortingImpl(sorting, Book.class, dictionary)))
+                .withPossibleFilterExpression(Optional.of(idPredicate))
+                .build();
+
+        String expected =
+                "SELECT example_Book FROM example.Book AS example_Book"
+                        + " LEFT JOIN FETCH example_Book.publisher example_Book_publisher"
+                        + " LEFT JOIN example_Book_publisher.editor example_Book_publisher_editor"
+                        + " WHERE example_Book.id IN (:id_XXX)"
+                        + " order by example_Book_publisher_editor.firstName desc";
+
+        String actual = query.getQueryText();
+        actual = actual.trim().replaceAll(" +", " ");
+        actual = actual.replaceFirst(":id_\\w+", ":id_XXX");
+
+        assertEquals(expected, actual);
+    }
 }
diff --git a/...est/java/com/yahoo/elide/datastores/hibernate/hql/SubCollectionFetchQueryBuilderTest.java b/...est/java/com/yahoo/elide/datastores/hibernate/hql/SubCollectionFetchQueryBuilderTest.java
@@ -43,6 +43,7 @@ public class SubCollectionFetchQueryBuilderTest {
     private static final String BOOKS = "books";
     private static final String NAME = "name";
     private static final String PUBLISHER = "publisher";
+    private static final String PERIOD = ".";
     private static final String PUB1 = "Pub1";
 
     @BeforeAll
@@ -236,4 +237,39 @@ public void testFetchJoinExcludesParent() {
 
         assertEquals(expected, actual);
     }
+
+    @Test
+    public void testSubCollectionFetchWithRelationshipSorting() {
+        Author author = new Author();
+        author.setId(1L);
+
+        Book book = new Book();
+        book.setId(2);
+
+        RelationshipImpl relationship = new RelationshipImpl(
+                Author.class,
+                Book.class,
+                BOOKS,
+                author,
+                Arrays.asList(book));
+
+        SubCollectionFetchQueryBuilder builder = new SubCollectionFetchQueryBuilder(relationship,
+                dictionary, new TestSessionWrapper());
+
+        Map<String, Sorting.SortOrder> sorting = new HashMap<>();
+        sorting.put(PUBLISHER + PERIOD + NAME, Sorting.SortOrder.asc);
+
+        TestQueryWrapper query = (TestQueryWrapper) builder
+                .withPossibleSorting(Optional.of(new SortingImpl(sorting, Book.class, dictionary)))
+                .build();
+
+        String expected = "SELECT example_Book FROM example.Author example_Author__fetch "
+                + "JOIN example_Author__fetch.books example_Book "
+                + "LEFT JOIN FETCH example_Book.publisher example_Book_publisher "
+                + "WHERE example_Author__fetch=:example_Author__fetch order by example_Book_publisher.name asc";
+        String actual = query.getQueryText();
+        actual = actual.trim().replaceAll(" +", " ");
+
+        assertEquals(expected, actual);
+    }
 }