From 45bdaecd56fefa17fa61932ceab1dad81bb2d98c Mon Sep 17 00:00:00 2001 From: Chandrasekar Rajasekar Date: Tue, 2 Jun 2020 20:20:58 -0500 Subject: [PATCH] add explicit join --- .../hql/AbstractHQLQueryBuilder.java | 63 ++++++++++++++---- .../hql/RootCollectionFetchQueryBuilder.java | 6 +- .../hql/SubCollectionFetchQueryBuilder.java | 6 +- .../hql/AbstractHQLQueryBuilderTest.java | 12 ++-- .../RootCollectionFetchQueryBuilderTest.java | 64 +++++++++++++++++++ .../SubCollectionFetchQueryBuilderTest.java | 36 +++++++++++ .../java/com/yahoo/elide/tests/SortingIT.java | 35 ++++++++++ 7 files changed, 201 insertions(+), 21 deletions(-) diff --git a/elide-datastore/elide-datastore-hibernate/src/main/java/com/yahoo/elide/core/hibernate/hql/AbstractHQLQueryBuilder.java b/elide-datastore/elide-datastore-hibernate/src/main/java/com/yahoo/elide/core/hibernate/hql/AbstractHQLQueryBuilder.java index f1face07b0..c2fcc05a30 100644 --- a/elide-datastore/elide-datastore-hibernate/src/main/java/com/yahoo/elide/core/hibernate/hql/AbstractHQLQueryBuilder.java +++ b/elide-datastore/elide-datastore-hibernate/src/main/java/com/yahoo/elide/core/hibernate/hql/AbstractHQLQueryBuilder.java @@ -136,10 +136,37 @@ protected String getJoinClauseFromFilters(FilterExpression filterExpression, boo Collection 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) { + 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, boolean skipFetches) { + if (sorting.isPresent() && !sorting.get().isDefaultInstance()) { + Map 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,11 +272,9 @@ 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, Class sortClass, boolean prefixWithAlias) { + protected String getSortClause(final Optional sorting) { String sortingRules = ""; if (sorting.isPresent() && !sorting.get().isDefaultInstance()) { final Map validSortingRules = sorting.get().getSortingPaths(); @@ -256,9 +282,24 @@ protected String getSortClause(final Optional sorting, Class sortCla final List 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/elide-datastore/elide-datastore-hibernate/src/main/java/com/yahoo/elide/core/hibernate/hql/RootCollectionFetchQueryBuilder.java b/elide-datastore/elide-datastore-hibernate/src/main/java/com/yahoo/elide/core/hibernate/hql/RootCollectionFetchQueryBuilder.java index d8b2808c9a..e553925223 100644 --- a/elide-datastore/elide-datastore-hibernate/src/main/java/com/yahoo/elide/core/hibernate/hql/RootCollectionFetchQueryBuilder.java +++ b/elide-datastore/elide-datastore-hibernate/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/elide-datastore/elide-datastore-hibernate/src/main/java/com/yahoo/elide/core/hibernate/hql/SubCollectionFetchQueryBuilder.java b/elide-datastore/elide-datastore-hibernate/src/main/java/com/yahoo/elide/core/hibernate/hql/SubCollectionFetchQueryBuilder.java index 3c94060126..8e3faab468 100644 --- a/elide-datastore/elide-datastore-hibernate/src/main/java/com/yahoo/elide/core/hibernate/hql/SubCollectionFetchQueryBuilder.java +++ b/elide-datastore/elide-datastore-hibernate/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/elide-datastore/elide-datastore-hibernate/src/test/java/com/yahoo/elide/datastores/hibernate/hql/AbstractHQLQueryBuilderTest.java b/elide-datastore/elide-datastore-hibernate/src/test/java/com/yahoo/elide/datastores/hibernate/hql/AbstractHQLQueryBuilderTest.java index 877e5ef414..2296d81afb 100644 --- a/elide-datastore/elide-datastore-hibernate/src/test/java/com/yahoo/elide/datastores/hibernate/hql/AbstractHQLQueryBuilderTest.java +++ b/elide-datastore/elide-datastore-hibernate/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 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 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/elide-datastore/elide-datastore-hibernate/src/test/java/com/yahoo/elide/datastores/hibernate/hql/RootCollectionFetchQueryBuilderTest.java b/elide-datastore/elide-datastore-hibernate/src/test/java/com/yahoo/elide/datastores/hibernate/hql/RootCollectionFetchQueryBuilderTest.java index e82a6de10a..7f5e4ebb8c 100644 --- a/elide-datastore/elide-datastore-hibernate/src/test/java/com/yahoo/elide/datastores/hibernate/hql/RootCollectionFetchQueryBuilderTest.java +++ b/elide-datastore/elide-datastore-hibernate/src/test/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 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 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/elide-datastore/elide-datastore-hibernate/src/test/java/com/yahoo/elide/datastores/hibernate/hql/SubCollectionFetchQueryBuilderTest.java b/elide-datastore/elide-datastore-hibernate/src/test/java/com/yahoo/elide/datastores/hibernate/hql/SubCollectionFetchQueryBuilderTest.java index 54c46b1eab..70cd44aadb 100644 --- a/elide-datastore/elide-datastore-hibernate/src/test/java/com/yahoo/elide/datastores/hibernate/hql/SubCollectionFetchQueryBuilderTest.java +++ b/elide-datastore/elide-datastore-hibernate/src/test/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 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); + } } diff --git a/elide-integration-tests/src/test/java/com/yahoo/elide/tests/SortingIT.java b/elide-integration-tests/src/test/java/com/yahoo/elide/tests/SortingIT.java index 45810f7193..0d27bd622d 100644 --- a/elide-integration-tests/src/test/java/com/yahoo/elide/tests/SortingIT.java +++ b/elide-integration-tests/src/test/java/com/yahoo/elide/tests/SortingIT.java @@ -7,6 +7,8 @@ import static com.yahoo.elide.Elide.JSONAPI_CONTENT_TYPE_WITH_JSON_PATCH_EXTENSION; import static io.restassured.RestAssured.given; +import static io.restassured.RestAssured.when; +import static org.hamcrest.Matchers.hasSize; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; @@ -62,6 +64,7 @@ public void setup() { public void testSortingRootCollectionByRelationshipProperty() throws IOException { JsonNode result = getAsNode("/book?sort=-publisher.name"); int size = result.get("data").size(); + assertEquals(8, size); JsonNode books = result.get("data"); String firstBookName = books.get(0).get("attributes").get("title").asText(); @@ -71,6 +74,8 @@ public void testSortingRootCollectionByRelationshipProperty() throws IOException assertEquals("The Old Man and the Sea", secondBookName); result = getAsNode("/book?sort=publisher.name"); + size = result.get("data").size(); + assertEquals(8, size); books = result.get("data"); firstBookName = books.get(size - 2).get("attributes").get("title").asText(); @@ -160,4 +165,34 @@ public void testSortingById() throws IOException { assertEquals(expectedTitle, actualTitle); } } + + @Test + public void testRootCollectionByNullRelationshipProperty() throws IOException { + // Test whether all book records are received + when() + .get("/book?sort=publisher.editor.lastName") + .then() + .body("data", hasSize(8)) + ; + when() + .get("/book?sort=-publisher.editor.lastName") + .then() + .body("data", hasSize(8)) + ; + + } + + @Test + public void testSubcollectionByNullRelationshipProperty() throws IOException { + when() + .get("/author/1/books?sort=publisher.editor.lastName") + .then() + .body("data", hasSize(2)) + ; + when() + .get("/author/1/books?sort=-publisher.editor.lastName") + .then() + .body("data", hasSize(2)) + ; + } }