From a9cfe73511342c7f7d95a591bbf5ca1e7a8c1e86 Mon Sep 17 00:00:00 2001 From: Oliver Gierke Date: Wed, 29 Aug 2012 17:16:33 +0200 Subject: [PATCH] DATAJPA-252 - Applying sort to manually defined queries considers joins. If a manually defined query uses outer joins, prefixing the sort criteria handed to the execution of the method causes an additional join being added as the JPA considers path expressions to be inner joins always (see Sect. 4.4.4, JPA 2.0 spec). We now parse the outer join aliases potentially used in a manually defined JPQL query and do not prefix the query with the root entity's alias. --- .../data/jpa/repository/query/QueryUtils.java | 72 +++++++++++++++++-- .../jpa/repository/UserRepositoryTests.java | 13 ++++ .../repository/query/QueryUtilsUnitTests.java | 53 +++++++++++++- .../jpa/repository/sample/UserRepository.java | 4 +- 4 files changed, 130 insertions(+), 12 deletions(-) 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 b176250de..4405e024d 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 @@ -1,5 +1,5 @@ /* - * Copyright 2008-2011 the original author or authors. + * Copyright 2008-2012 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. @@ -18,9 +18,11 @@ package org.springframework.data.jpa.repository.query; import static java.util.regex.Pattern.*; import java.util.ArrayList; +import java.util.HashSet; import java.util.Iterator; import java.util.List; import java.util.Locale; +import java.util.Set; import java.util.regex.Matcher; import java.util.regex.Pattern; @@ -38,6 +40,7 @@ import org.springframework.data.domain.Sort; import org.springframework.data.domain.Sort.Order; import org.springframework.data.mapping.PropertyPath; import org.springframework.util.Assert; +import org.springframework.util.StringUtils; /** * Simple utility class to create JPA queries. @@ -60,6 +63,9 @@ public abstract class QueryUtils { private static final String IDENTIFIER = "[\\p{Alnum}._$]+"; private static final String IDENTIFIER_GROUP = String.format("(%s)", IDENTIFIER); + private static final String LEFT_JOIN = "left (outer )?join " + IDENTIFIER + " (as )?" + IDENTIFIER_GROUP; + private static final Pattern LEFT_JOIN_PATTERN = Pattern.compile(LEFT_JOIN, Pattern.CASE_INSENSITIVE); + static { StringBuilder builder = new StringBuilder(); @@ -128,24 +134,76 @@ public abstract class QueryUtils { Assert.hasText(query); - if (null == sort) { + if (null == sort || !sort.iterator().hasNext()) { return query; } StringBuilder builder = new StringBuilder(query); - builder.append(" order by"); - for (Order order : sort) { - builder.append(String.format(" %s.%s %s,", alias, order.getProperty(), toJpaDirection(order))); + if (!query.contains("order by")) { + builder.append(" order by "); + } else { + builder.append(", "); } - builder.deleteCharAt(builder.length() - 1); + Set aliases = getOuterJoinAliases(query); + + for (Order order : sort) { + builder.append(getOrderClause(aliases, alias, order)); + } + + builder.delete(builder.length() - 2, builder.length()); return builder.toString(); } - public static String toJpaDirection(Order order) { + /** + * Returns the order clause for the given {@link Order}. Will prefix the clause with the given alias if the referenced + * property refers to a join alias. + * + * @param joinAliases the join aliases of the original query. + * @param alias the alias for the root entity. + * @param order the order object to build the clause for. + * @return + */ + private static String getOrderClause(Set joinAliases, String alias, Order order) { + String property = order.getProperty(); + boolean qualifyReference = true; + + for (String joinAlias : joinAliases) { + if (property.startsWith(joinAlias)) { + qualifyReference = false; + break; + } + } + + return String.format("%s%s %s, ", qualifyReference ? alias + "." : "", property, toJpaDirection(order)); + } + + /** + * Returns the aliases used for {@code left (outer) join}s. + * + * @param query + * @return + */ + static Set getOuterJoinAliases(String query) { + + Set result = new HashSet(); + Matcher matcher = LEFT_JOIN_PATTERN.matcher(query); + + while (matcher.find()) { + + String alias = matcher.group(3); + if (StringUtils.hasText(alias)) { + result.add(alias); + } + } + + return result; + } + + private static String toJpaDirection(Order order) { return order.getDirection().name().toLowerCase(Locale.US); } diff --git a/src/test/java/org/springframework/data/jpa/repository/UserRepositoryTests.java b/src/test/java/org/springframework/data/jpa/repository/UserRepositoryTests.java index d8c7c8078..90571a87e 100644 --- a/src/test/java/org/springframework/data/jpa/repository/UserRepositoryTests.java +++ b/src/test/java/org/springframework/data/jpa/repository/UserRepositoryTests.java @@ -921,6 +921,19 @@ public class UserRepositoryTests { assertThat(all.getContent().isEmpty(), is(false)); } + /** + * @see DATAJPA-252 + */ + @Test + public void bindsSortingToOuterJoinCorrectly() { + + flushTestUsers(); + + // Managers not set, make sure adding the sort does not rule out those Users + Page result = repository.findAllPaged(new PageRequest(0, 10, new Sort("manager.lastname"))); + assertThat(result.getContent(), hasSize((int) repository.count())); + } + private Page executeSpecWithSort(Sort sort) { flushTestUsers(); diff --git a/src/test/java/org/springframework/data/jpa/repository/query/QueryUtilsUnitTests.java b/src/test/java/org/springframework/data/jpa/repository/query/QueryUtilsUnitTests.java index 8832da60b..2c144ca09 100644 --- a/src/test/java/org/springframework/data/jpa/repository/query/QueryUtilsUnitTests.java +++ b/src/test/java/org/springframework/data/jpa/repository/query/QueryUtilsUnitTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2008-2011 the original author or authors. + * Copyright 2008-2012 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. @@ -15,12 +15,15 @@ */ package org.springframework.data.jpa.repository.query; -import static org.hamcrest.CoreMatchers.*; +import static org.hamcrest.Matchers.*; import static org.junit.Assert.*; import static org.springframework.data.jpa.repository.query.QueryUtils.*; +import java.util.Set; + import org.hamcrest.Matcher; import org.junit.Test; +import org.springframework.data.domain.Sort; /** * Unit test for {@link QueryUtils}. @@ -131,8 +134,52 @@ public class QueryUtilsUnitTests { assertCountQuery(FQ_QUERY, "select count(u) from org.acme.domain.User$Foo_Bar u"); } - private void assertCountQuery(String originalQuery, String countQuery) { + /** + * @see DATAJPA-252 + */ + @Test + public void detectsJoinAliasesCorrectly() { + Set aliases = getOuterJoinAliases("select p from Person p left outer join x.foo b2_$ar where …"); + assertThat(aliases, hasSize(1)); + assertThat(aliases, hasItems("b2_$ar")); + + aliases = getOuterJoinAliases("select p from Person p left join x.foo b2_$ar where …"); + assertThat(aliases, hasSize(1)); + assertThat(aliases, hasItems("b2_$ar")); + + aliases = getOuterJoinAliases("select p from Person p left outer join x.foo as b2_$ar, left join x.bar as foo where …"); + assertThat(aliases, hasSize(2)); + assertThat(aliases, hasItems("b2_$ar", "foo")); + + aliases = getOuterJoinAliases("select p from Person p left join x.foo as b2_$ar, left outer join x.bar foo where …"); + assertThat(aliases, hasSize(2)); + assertThat(aliases, hasItems("b2_$ar", "foo")); + } + + /** + * @see DATAJPA-252 + */ + @Test + public void doesNotPrefixOrderReferenceIfOuterJoinAliasDetected() { + + String query = "select p from Person p left join p.address address"; + assertThat(applySorting(query, new Sort("address.city")), endsWith("order by address.city asc")); + assertThat(applySorting(query, new Sort("address.city", "lastname"), "p"), + endsWith("order by address.city asc, p.lastname asc")); + } + + /** + * @see DATAJPA-252 + */ + @Test + public void extendsExistingOrderByClausesCorrectly() { + + String query = "select p from Person p order by p.lastname asc"; + assertThat(applySorting(query, new Sort("firstname"), "p"), endsWith("order by p.lastname asc, p.firstname asc")); + } + + private void assertCountQuery(String originalQuery, String countQuery) { assertThat(createCountQueryFor(originalQuery), is(countQuery)); } } diff --git a/src/test/java/org/springframework/data/jpa/repository/sample/UserRepository.java b/src/test/java/org/springframework/data/jpa/repository/sample/UserRepository.java index 6bef5f0be..af36e4650 100644 --- a/src/test/java/org/springframework/data/jpa/repository/sample/UserRepository.java +++ b/src/test/java/org/springframework/data/jpa/repository/sample/UserRepository.java @@ -1,5 +1,5 @@ /* - * Copyright 2008-2011 the original author or authors. + * Copyright 2008-2012 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. @@ -68,7 +68,7 @@ public interface UserRepository extends JpaRepository, JpaSpecifi */ User findByEmailAddress(String emailAddress); - @Query("select u from User u ") + @Query("select u from User u left outer join u.manager as manager") Page findAllPaged(Pageable pageable); /**