Skip to content

Differentiate between JPQL and native queries in count query derivation #2777

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed

Differentiate between JPQL and native queries in count query derivation #2777

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c0e9211
Prepare issue branch.
mp911de Jan 25, 2023
9732cf3
Differentiate between JPQL and native queries in count query derivation.
mp911de Jan 25, 2023
fff3ae8
Refactor QueryEnhancer Tests into TCK-style.
mp911de Jan 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

<groupId>org.springframework.data</groupId>
<artifactId>spring-data-jpa-parent</artifactId>
<version>3.1.0-SNAPSHOT</version>
<version>3.1.0-GH-2773-SNAPSHOT</version>
<packaging>pom</packaging>

<name>Spring Data JPA Parent</name>
Expand Down
4 changes: 2 additions & 2 deletions spring-data-envers/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,12 @@

<groupId>org.springframework.data</groupId>
<artifactId>spring-data-envers</artifactId>
<version>3.1.0-SNAPSHOT</version>
<version>3.1.0-GH-2773-SNAPSHOT</version>

<parent>
<groupId>org.springframework.data</groupId>
<artifactId>spring-data-jpa-parent</artifactId>
<version>3.1.0-SNAPSHOT</version>
<version>3.1.0-GH-2773-SNAPSHOT</version>
<relativePath>../pom.xml</relativePath>
</parent>

Expand Down
2 changes: 1 addition & 1 deletion spring-data-jpa-distribution/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
<parent>
<groupId>org.springframework.data</groupId>
<artifactId>spring-data-jpa-parent</artifactId>
<version>3.1.0-SNAPSHOT</version>
<version>3.1.0-GH-2773-SNAPSHOT</version>
<relativePath>../pom.xml</relativePath>
</parent>

Expand Down
4 changes: 2 additions & 2 deletions spring-data-jpa/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

<groupId>org.springframework.data</groupId>
<artifactId>spring-data-jpa</artifactId>
<version>3.1.0-SNAPSHOT</version>
<version>3.1.0-GH-2773-SNAPSHOT</version>

<name>Spring Data JPA</name>
<description>Spring Data module for JPA repositories.</description>
Expand All @@ -15,7 +15,7 @@
<parent>
<groupId>org.springframework.data</groupId>
<artifactId>spring-data-jpa-parent</artifactId>
<version>3.1.0-SNAPSHOT</version>
<version>3.1.0-GH-2773-SNAPSHOT</version>
<relativePath>../pom.xml</relativePath>
</parent>

Expand Down
2 changes: 1 addition & 1 deletion ...jpa/src/main/java/org/springframework/data/jpa/repository/query/DefaultQueryEnhancer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ public String detectAlias() {

@Override
public String createCountQueryFor(@Nullable String countProjection) {
return QueryUtils.createCountQueryFor(this.query.getQueryString(), countProjection);
return QueryUtils.createCountQueryFor(this.query.getQueryString(), countProjection, this.query.isNativeQuery());
}

@Override
Expand Down
23 changes: 17 additions & 6 deletions .../src/main/java/org/springframework/data/jpa/repository/query/JSqlParserQueryEnhancer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,8 @@
*/
package org.springframework.data.jpa.repository.query;

import static org.springframework.data.jpa.repository.query.JSqlParserUtils.getJSqlCount;
import static org.springframework.data.jpa.repository.query.JSqlParserUtils.getJSqlLower;
import static org.springframework.data.jpa.repository.query.QueryUtils.checkSortExpression;
import static org.springframework.data.jpa.repository.query.JSqlParserUtils.*;
import static org.springframework.data.jpa.repository.query.QueryUtils.*;

import net.sf.jsqlparser.JSQLParserException;
import net.sf.jsqlparser.expression.Alias;
Expand All @@ -29,11 +28,23 @@
import net.sf.jsqlparser.statement.delete.Delete;
import net.sf.jsqlparser.statement.insert.Insert;
import net.sf.jsqlparser.statement.merge.Merge;
import net.sf.jsqlparser.statement.select.*;
import net.sf.jsqlparser.statement.select.OrderByElement;
import net.sf.jsqlparser.statement.select.PlainSelect;
import net.sf.jsqlparser.statement.select.Select;
import net.sf.jsqlparser.statement.select.SelectBody;
import net.sf.jsqlparser.statement.select.SelectExpressionItem;
import net.sf.jsqlparser.statement.select.SelectItem;
import net.sf.jsqlparser.statement.select.SetOperationList;
import net.sf.jsqlparser.statement.select.WithItem;
import net.sf.jsqlparser.statement.update.Update;
import net.sf.jsqlparser.statement.values.ValuesStatement;

import java.util.*;
import java.util.ArrayList;
import java.util.Collections;
import java.util.HashSet;
import java.util.List;
import java.util.Objects;
import java.util.Set;
import java.util.stream.Collectors;

import org.springframework.data.domain.Sort;
Expand Down Expand Up @@ -400,7 +411,7 @@ public String createCountQueryFor(@Nullable String countProjection) {
return selectBody.toString();
}

String countProp = tableAlias == null ? "*" : tableAlias;
String countProp = query.isNativeQuery() ? (distinct ? "*" : "1") : tableAlias == null ? "*" : tableAlias;

Function jSqlCount = getJSqlCount(Collections.singletonList(countProp), distinct);
selectBody.setSelectItems(Collections.singletonList(new SelectExpressionItem(jSqlCount)));
Expand Down
43 changes: 37 additions & 6 deletions spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/QueryUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,23 @@
import static jakarta.persistence.metamodel.Attribute.PersistentAttributeType.*;
import static java.util.regex.Pattern.*;

import jakarta.persistence.*;
import jakarta.persistence.criteria.*;
import jakarta.persistence.metamodel.*;
import jakarta.persistence.EntityManager;
import jakarta.persistence.ManyToOne;
import jakarta.persistence.OneToOne;
import jakarta.persistence.Parameter;
import jakarta.persistence.Query;
import jakarta.persistence.criteria.CriteriaBuilder;
import jakarta.persistence.criteria.Expression;
import jakarta.persistence.criteria.Fetch;
import jakarta.persistence.criteria.From;
import jakarta.persistence.criteria.Join;
import jakarta.persistence.criteria.JoinType;
import jakarta.persistence.metamodel.Attribute;
import jakarta.persistence.metamodel.Attribute.PersistentAttributeType;
import jakarta.persistence.metamodel.Bindable;
import jakarta.persistence.metamodel.ManagedType;
import jakarta.persistence.metamodel.PluralAttribute;
import jakarta.persistence.metamodel.SingularAttribute;

import java.lang.annotation.Annotation;
import java.lang.reflect.AnnotatedElement;
Expand Down Expand Up @@ -570,6 +583,19 @@ public static String createCountQueryFor(String originalQuery) {
*/
@Deprecated
public static String createCountQueryFor(String originalQuery, @Nullable String countProjection) {
return createCountQueryFor(originalQuery, countProjection, false);
}

/**
* Creates a count projected query from the given original query.
*
* @param originalQuery must not be {@literal null}.
* @param countProjection may be {@literal null}.
* @param nativeQuery whether the underlying query is a native query.
* @return a query String to be used a count query for pagination. Guaranteed to be not {@literal null}.
* @since 2.7.8
*/
static String createCountQueryFor(String originalQuery, @Nullable String countProjection, boolean nativeQuery) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer two separate methods createCountQueryForNative and createCountQueryForJpql plus extracting the common parts into a new method.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My gut feeling is that we should rethink QueryUtils entirely and eventually eliminate the most methods (or create delegates). Therefore, I would want to refrain from introducing additional methods for the time being unless new methods heavily improve the code.


Assert.hasText(originalQuery, "OriginalQuery must not be null or empty");

Expand All @@ -591,9 +617,14 @@ public static String createCountQueryFor(String originalQuery, @Nullable String

String replacement = useVariable ? SIMPLE_COUNT_VALUE : complexCountValue;

String alias = QueryUtils.detectAlias(originalQuery);
if ("*".equals(variable) && alias != null) {
replacement = alias;
if (nativeQuery && (variable.contains(",") || "*".equals(variable))) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if it is a native query and the variable is null 😬

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you run into a bug or you can reproduce such a case, please file a bug report.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

already present #2812

replacement = "1";
} else {

String alias = QueryUtils.detectAlias(originalQuery);
if (("*".equals(variable) && alias != null)) {
replacement = alias;
}
}

countQuery = matcher.replaceFirst(String.format(COUNT_REPLACEMENT_TEMPLATE, replacement));
Expand Down
37 changes: 37 additions & 0 deletions ...est/java/org/springframework/data/jpa/repository/query/DefaultQueryEnhancerUnitTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
/*
* Copyright 2023 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.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package org.springframework.data.jpa.repository.query;

import org.junit.jupiter.api.Disabled;
import org.junit.jupiter.api.Test;

/**
* TCK Tests for {@link DefaultQueryEnhancer}.
*
* @author Mark Paluch
*/
public class DefaultQueryEnhancerUnitTests extends QueryEnhancerTckTests {

@Override
QueryEnhancer createQueryEnhancer(DeclaredQuery declaredQuery) {
return new DefaultQueryEnhancer(declaredQuery);
}

@Override
@Test // GH-2511, GH-2773
@Disabled("Not properly supported by QueryUtils")
void shouldDeriveNativeCountQueryWithVariable(String query, String expected) {}
}
Loading