Skip to content

Commit

Permalink
Merge pull request #27976 from njr-11/27748-insert-parenthesis-for-cu…
Browse files Browse the repository at this point in the history 
…rsor-pagination

insert parenthesis around WHERE clause for cursor pagination
  • Loading branch information
@njr-11
njr-11 authored Mar 22, 2024
2 parents 6ffa1cd + d688107 commit 2f0ab10
Show file tree
Hide file tree
Showing 4 changed files with 78 additions and 12 deletions.
50 changes: 39 additions & 11 deletions ...rty.data.internal.persistence/src/io/openliberty/data/internal/persistence/QueryInfo.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
import jakarta.data.Sort;
import jakarta.data.exceptions.DataException;
import jakarta.data.exceptions.MappingException;
import jakarta.data.page.CursoredPage;
import jakarta.data.page.PageRequest;
import jakarta.data.repository.Delete;
import jakarta.data.repository.Find;
Expand Down Expand Up @@ -275,13 +276,14 @@ void addSort(boolean ignoreCase, String attribute, boolean descending) {
* TODO remove this method once we have Jakarta Persistence 3.2.
*
* @oaram o_ entity identifier variable followed by the . character.
* @param ql Jakarta Data Query Language
* @param startAt position in query language to start at.
* @param q simulated JPQL to which to append.
* @param q simulated JPQL count query to which to append unless null. The ORDER BY clause is not appended.
* @param ql Jakarta Data Query Language
* @param startAt position in query language to start at.
* @param q simulated JPQL to which to append.
* @param c simulated JPQL count query to which to append unless null. The ORDER BY clause is not appended.
* @param isCursoredPage indicates if the return type is CursoredPage.
* @return simulated JPQL.
*/
private StringBuilder appendWithIdentifierName(String o_, String ql, int startAt, StringBuilder q, StringBuilder c) {
private StringBuilder appendWithIdentifierName(String o_, String ql, int startAt, StringBuilder q, StringBuilder c, boolean isCursoredPage) {
boolean appendToCountQuery = c != null;
boolean isLiteral = false;
boolean isNamedParamOrEmbedded = false;
Expand Down Expand Up @@ -332,6 +334,13 @@ private StringBuilder appendWithIdentifierName(String o_, String ql, int startAt
if ("ORDER".equalsIgnoreCase(str)
&& i + 3 < length
&& (by = indexOfAfterWhitespace("BY", ql, i + 1)) > 0) {
if (isCursoredPage)
throw new UnsupportedOperationException("The " + ql + " query that is supplied to the " + method.getName() +
" method of the " + method.getDeclaringClass().getName() +
" repository cannot include an ORDER BY clause because" +
" the method returns a " + "CursoredPage" + ". Remove the ORDER BY" +
" clause and instead use the " + "OrderBy" +
" annotation to specify static sort criteria."); // TODO NLS
for (; i < by + 2; i++)
s.append(ql.charAt(i));
str = s.toString();
Expand Down Expand Up @@ -682,7 +691,7 @@ private String assembleCountQuery(String countQL) {
.append("SELECT COUNT(o) FROM ").append(entityInfo.name).append(" o");
c.append(" WHERE");

return appendWithIdentifierName("o.", countQL, startAt + 5, c, null).toString();
return appendWithIdentifierName("o.", countQL, startAt + 5, c, null, false).toString();
}
break;
default:
Expand All @@ -701,8 +710,10 @@ private String assembleCountQuery(String countQL) {
* @param ql Query.value() might be JPQL or JDQL
* @param countQL Query.count() might be JPQL or JDQL or "" (unspecified)
* @param countPages whether or not to obtain a count of pages.
* @param multiType the type of data structure that returns multiple results for this query. Otherwise null.
*/
void initForQuery(String ql, String countQL, boolean countPages) {
void initForQuery(String ql, String countQL, boolean countPages, Class<?> multiType) {
boolean isCursoredPage = CursoredPage.class.equals(multiType);

StringBuilder q = null; // main query
StringBuilder c = null; // count query
Expand Down Expand Up @@ -743,7 +754,7 @@ void initForQuery(String ql, String countQL, boolean countPages) {
entityVar_ = "o.";
q = new StringBuilder(ql.length() * 3 / 2) //
.append("DELETE FROM ").append(entityName).append(" o WHERE");
jpql = appendWithIdentifierName(entityVar_, ql, startAt + 5, q, null).toString();
jpql = appendWithIdentifierName(entityVar_, ql, startAt + 5, q, null, false).toString();
}
}
}
Expand Down Expand Up @@ -773,7 +784,7 @@ void initForQuery(String ql, String countQL, boolean countPages) {
entityVar_ = "o.";
q = new StringBuilder(ql.length() * 3 / 2) //
.append("UPDATE ").append(entityName).append(" o SET");
jpql = appendWithIdentifierName(entityVar_, ql, startAt + 3, q, null).toString();
jpql = appendWithIdentifierName(entityVar_, ql, startAt + 3, q, null, false).toString();
}
}
}
Expand Down Expand Up @@ -826,6 +837,7 @@ void initForQuery(String ql, String countQL, boolean countPages) {
&& ql.regionMatches(true, startAt, "WHERE", 0, 5)
&& !Character.isLetterOrDigit(ql.charAt(startAt + 5))) {
hasWhere = true;
startAt += 5;

if (q == null) {
type = Type.FIND;
Expand All @@ -844,7 +856,17 @@ void initForQuery(String ql, String countQL, boolean countPages) {
c.append(" WHERE");
}

jpql = appendWithIdentifierName(entityVar_, ql, startAt + 5, q, c).toString();
// Cursor-based pagination queries must end with the WHERE clause, per the spec.
// Insert parenthesis to allow later appending conditions.
if (isCursoredPage)
q.append(" (");

appendWithIdentifierName(entityVar_, ql, startAt, q, c, isCursoredPage);

if (isCursoredPage)
q.append(')');

jpql = q.toString();

if (countPages)
if (c == null)
Expand Down Expand Up @@ -880,7 +902,7 @@ void initForQuery(String ql, String countQL, boolean countPages) {
c = new StringBuilder(q.length()).append("SELECT COUNT(").append(entityVar) //
.append(") FROM ").append(entityInfo.name).append(' ').append(entityVar);

jpql = appendWithIdentifierName(entityVar_, ql, startAt, q, null).toString();
jpql = appendWithIdentifierName(entityVar_, ql, startAt, q, null, false).toString();

if (countPages)
if (c == null)
Expand Down Expand Up @@ -973,6 +995,12 @@ void initForQuery(String ql, String countQL, boolean countPages) {
}
hasWhere = upperTrimmed.contains("WHERE");
}

if (isCursoredPage && !hasWhere)
throw new UnsupportedOperationException("The " + ql + " query that is supplied to the " + method.getName() +
" method of the " + method.getDeclaringClass().getName() +
" repository must end with a WHERE clause because the method returns a " +
"CursoredPage" + "."); // TODO NLS
}

/**
Expand Down
2 changes: 1 addition & 1 deletion ...ata.internal.persistence/src/io/openliberty/data/internal/persistence/RepositoryImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -301,7 +301,7 @@ private QueryInfo completeQueryInfo(EntityInfo entityInfo, QueryInfo queryInfo)
count, exists, select);

if (query != null) { // @Query annotation
queryInfo.initForQuery(query.value(), query.count(), countPages);
queryInfo.initForQuery(query.value(), query.count(), countPages, multiType);
} else if (save != null) { // @Save annotation
queryInfo.init(Save.class, QueryInfo.Type.SAVE);
} else if (insert != null) { // @Insert annotation
Expand Down
35 changes: 35 additions & 0 deletions ...pa/test-applications/DataJPATestApp/src/test/jakarta/data/jpa/web/DataJPATestServlet.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2526,6 +2526,41 @@ public void testOneToOne() {
drivers.deleteByFullNameEndsWith(" TestOneToOne");
}

/**
* Use a repository method with cursor-based pagination that does not specify parenthesis
* in the WHERE clause.
*/
@Test
public void testParenthesisInsertionForCursorPagination() {
PageRequest<?> page1Request = PageRequest.ofSize(3).asc("name").asc(ID);
CursoredPage<Business> page1 = mixed.withZipCodeIn(55901, 55904, page1Request);

assertEquals(List.of("Benike Construction", "Crenlo", "Home Federal Savings Bank"),
page1.stream()
.map(b -> b.name)
.collect(Collectors.toList()));

assertEquals(true, page1.hasNext());

CursoredPage<Business> page2 = mixed.withZipCodeIn(55901, 55904, page1.nextPageRequest());

assertEquals(List.of("IBM", "Metafile", "Olmsted Medical"),
page2.stream()
.map(b -> b.name)
.collect(Collectors.toList()));

assertEquals(true, page1.hasNext());

CursoredPage<Business> page3 = mixed.withZipCodeIn(55901, 55904, page2.nextPageRequest());

assertEquals(List.of("RAC", "Think Bank"),
page3.stream()
.map(b -> b.name)
.collect(Collectors.toList()));

assertEquals(false, page3.hasNext());
}

/**
* Tests lifecycle methods returning a single record.
*/
Expand Down
3 changes: 3 additions & 0 deletions ...t_jpa/test-applications/DataJPATestApp/src/test/jakarta/data/jpa/web/MixedRepository.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,4 +55,7 @@ public interface MixedRepository { // Do not inherit from a supertype

@Query("FROM Business WHERE location.address.city=:city AND location.address.state=:state")
CursoredPage<Business> locatedIn(String city, String state, PageRequest<Business> pageRequest);

@Query("FROM Business WHERE location.address.zip=?1 OR location.address.zip=?2")
CursoredPage<Business> withZipCodeIn(int zip1, int zip2, PageRequest<?> pageRequest);
}

0 comments on commit 2f0ab10

Please sign in to comment.