Permalink
Browse files

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.
  • Loading branch information...
1 parent 0e6c4fd commit a9cfe73511342c7f7d95a591bbf5ca1e7a8c1e86 @olivergierke olivergierke committed Aug 29, 2012
View
70 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 @@
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.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 @@
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 static String applySorting(String query, Sort sort, String alias) {
Assert.hasText(query);
- if (null == sort) {
+ if (null == sort || !sort.iterator().hasNext()) {
return query;
}
StringBuilder builder = new StringBuilder(query);
- builder.append(" order by");
+
+ if (!query.contains("order by")) {
+ builder.append(" order by ");
+ } else {
+ builder.append(", ");
+ }
+
+ Set<String> aliases = getOuterJoinAliases(query);
for (Order order : sort) {
- builder.append(String.format(" %s.%s %s,", alias, order.getProperty(), toJpaDirection(order)));
+ builder.append(getOrderClause(aliases, alias, order));
}
- builder.deleteCharAt(builder.length() - 1);
+ 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<String> 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<String> getOuterJoinAliases(String query) {
+
+ Set<String> result = new HashSet<String>();
+ 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);
}
View
13 src/test/java/org/springframework/data/jpa/repository/UserRepositoryTests.java
@@ -921,6 +921,19 @@ public void ordersByReferencedEntityCorrectly() {
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<User> result = repository.findAllPaged(new PageRequest(0, 10, new Sort("manager.lastname")));
+ assertThat(result.getContent(), hasSize((int) repository.count()));
+ }
+
private Page<User> executeSpecWithSort(Sort sort) {
flushTestUsers();
View
53 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 void allowsFullyQualifiedEntityNamesInQuery() {
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<String> 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));
}
}
View
4 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 @@
*/
User findByEmailAddress(String emailAddress);
- @Query("select u from User u ")
+ @Query("select u from User u left outer join u.manager as manager")
Page<User> findAllPaged(Pageable pageable);
/**

0 comments on commit a9cfe73

Please sign in to comment.