Skip to content
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

Ensure proper hint is applied for projections of paginated queries for Querydsl fluent queries #2827

Closed
wants to merge 2 commits into from
Closed

Ensure proper hint is applied for projections of paginated queries for Querydsl fluent queries #2827

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0c7639a
Create branch.
gregturn Feb 27, 2023
7d4f4ef
Apply hints for pagined projections on fetchable Querydsl queries.
gregturn Feb 27, 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-2820-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-2820-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-2820-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-2820-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-2820-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-2820-SNAPSHOT</version>
<relativePath>../pom.xml</relativePath>
</parent>

Expand Down
16 changes: 10 additions & 6 deletions ...java/org/springframework/data/jpa/repository/support/FetchableFluentQueryByPredicate.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@
*/
package org.springframework.data.jpa.repository.support;

import jakarta.persistence.EntityManager;

import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
Expand All @@ -23,8 +25,6 @@
import java.util.function.Function;
import java.util.stream.Stream;

import jakarta.persistence.EntityManager;

import org.springframework.dao.IncorrectResultSizeDataAccessException;
import org.springframework.data.domain.Page;
import org.springframework.data.domain.PageImpl;
Expand Down Expand Up @@ -69,8 +69,7 @@ public FetchableFluentQueryByPredicate(Predicate predicate, Class<S> entityType,
private FetchableFluentQueryByPredicate(Predicate predicate, Class<S> entityType, Class<R> resultType, Sort sort,
Collection<String> properties, Function<Sort, AbstractJPAQuery<?, ?>> finder,
BiFunction<Sort, Pageable, AbstractJPAQuery<?, ?>> pagedFinder, Function<Predicate, Long> countOperation,
Function<Predicate, Boolean> existsOperation,
EntityManager entityManager) {
Function<Predicate, Boolean> existsOperation, EntityManager entityManager) {

super(resultType, sort, properties, entityType);
this.predicate = predicate;
Expand Down Expand Up @@ -175,8 +174,13 @@ public boolean exists() {

private Page<R> readPage(Pageable pageable) {

AbstractJPAQuery<?, ?> pagedQuery = pagedFinder.apply(sort, pageable);
List<R> paginatedResults = convert(pagedQuery.fetch());
AbstractJPAQuery<?, ?> query = pagedFinder.apply(sort, pageable);

if (!properties.isEmpty()) {
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to do the same for Query by Example and Specifications?

Copy link
Contributor

Choose a reason for hiding this comment

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

See #2721

Copy link
Contributor Author

@gregturn gregturn Mar 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

To be more precise, FetchableFluentQueryByExample 's readPage method taps the createSortedAndProjectedQuery method, which applies the expected hint in all scenarios.

The same can be said for the Specification-based variant.

This one is the only one that doesn't do that, probably due to the nature of Querydsl's APIs.

query.setHint(EntityGraphFactory.HINT, EntityGraphFactory.create(entityManager, entityType, properties));
}

List<R> paginatedResults = convert(query.fetch());

return PageableExecutionUtils.getPage(paginatedResults, pageable, () -> countOperation.apply(predicate));
}
Expand Down
65 changes: 49 additions & 16 deletions ...g-data-jpa/src/test/java/org/springframework/data/jpa/repository/UserRepositoryTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,23 +15,14 @@
*/
package org.springframework.data.jpa.repository;

import static java.util.Arrays.asList;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
import static org.springframework.data.domain.Example.of;
import static org.springframework.data.domain.ExampleMatcher.GenericPropertyMatcher;
import static org.springframework.data.domain.ExampleMatcher.StringMatcher;
import static org.springframework.data.domain.ExampleMatcher.matching;
import static org.springframework.data.domain.Sort.Direction.ASC;
import static org.springframework.data.domain.Sort.Direction.DESC;
import static java.util.Arrays.*;
Copy link
Member

Choose a reason for hiding this comment

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

The import reformatting indicates a different threshold for * imports. Please revisit your formatter settings.

Copy link
Contributor Author

@gregturn gregturn Mar 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I believe our standard is 1 for static imports, and I didn't have this properly configured in the past.

Copy link
Member

Choose a reason for hiding this comment

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

Please ignore my comment. I missed the static.

import static org.assertj.core.api.Assertions.*;
import static org.springframework.data.domain.Example.*;
import static org.springframework.data.domain.ExampleMatcher.*;
import static org.springframework.data.domain.Sort.Direction.*;
import static org.springframework.data.jpa.domain.Specification.*;
import static org.springframework.data.jpa.domain.Specification.not;
import static org.springframework.data.jpa.domain.Specification.where;
import static org.springframework.data.jpa.domain.sample.UserSpecifications.userHasAgeLess;
import static org.springframework.data.jpa.domain.sample.UserSpecifications.userHasFirstname;
import static org.springframework.data.jpa.domain.sample.UserSpecifications.userHasFirstnameLike;
import static org.springframework.data.jpa.domain.sample.UserSpecifications.userHasLastname;
import static org.springframework.data.jpa.domain.sample.UserSpecifications.userHasLastnameLikeWithSort;
import static org.springframework.data.jpa.domain.sample.UserSpecifications.*;

import jakarta.persistence.EntityManager;
import jakarta.persistence.PersistenceContext;
Expand Down Expand Up @@ -75,6 +66,7 @@
import org.springframework.data.domain.Sort.Order;
import org.springframework.data.jpa.domain.Specification;
import org.springframework.data.jpa.domain.sample.Address;
import org.springframework.data.jpa.domain.sample.QUser;
import org.springframework.data.jpa.domain.sample.Role;
import org.springframework.data.jpa.domain.sample.SpecialUser;
import org.springframework.data.jpa.domain.sample.User;
Expand Down Expand Up @@ -2444,6 +2436,47 @@ void findByFluentSpecificationWithSimplePropertyPathsDoesntLoadUnrequestedPaths(
);
}

@Test // GH-2820
void findByFluentPredicateWithProjectionAndPageRequest() {

flushTestUsers();
em.clear();

Page<User> users = repository.findBy(QUser.user.firstname.contains("v"), q -> q //
.project("firstname") //
.page(PageRequest.of(0, 10)));

assertThat(users).extracting(User::getFirstname).containsExactlyInAnyOrder(firstUser.getFirstname(),
thirdUser.getFirstname(), fourthUser.getFirstname());
}

@Test // GH-2820
void findByFluentPredicateWithProjectionAndAll() {

flushTestUsers();
em.clear();

List<User> users = repository.findBy(QUser.user.firstname.contains("v"), q -> q //
.project("firstname") //
.all());

assertThat(users).extracting(User::getFirstname).containsExactlyInAnyOrder(firstUser.getFirstname(),
thirdUser.getFirstname(), fourthUser.getFirstname());
}

@Test // GH-2820
void findByFluentPredicateWithPageRequest() {

flushTestUsers();
em.clear();

Page<User> users = repository.findBy(QUser.user.firstname.contains("v"), q -> q //
.page(PageRequest.of(0, 10)));

assertThat(users).extracting(User::getFirstname).containsExactlyInAnyOrder(firstUser.getFirstname(),
thirdUser.getFirstname(), fourthUser.getFirstname());
}

@Test // GH-2274
void findByFluentSpecificationWithCollectionPropertyPathsDoesntLoadUnrequestedPaths() {

Expand Down
24 changes: 19 additions & 5 deletions ...data-jpa/src/test/java/org/springframework/data/jpa/repository/sample/UserRepository.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,15 +18,29 @@
import jakarta.persistence.EntityManager;
import jakarta.persistence.QueryHint;

import java.util.*;
import java.util.Collection;
import java.util.Date;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.Set;
import java.util.stream.Stream;

import org.springframework.data.domain.*;
Copy link
Member

Choose a reason for hiding this comment

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

These changes seem fine as we have less than 10 imports.

import org.springframework.data.domain.Page;
import org.springframework.data.domain.PageRequest;
import org.springframework.data.domain.Pageable;
import org.springframework.data.domain.Slice;
import org.springframework.data.domain.Sort;
import org.springframework.data.jpa.domain.sample.Role;
import org.springframework.data.jpa.domain.sample.SpecialUser;
import org.springframework.data.jpa.domain.sample.User;
import org.springframework.data.jpa.repository.*;
import org.springframework.data.jpa.repository.JpaRepository;
import org.springframework.data.jpa.repository.JpaSpecificationExecutor;
import org.springframework.data.jpa.repository.Modifying;
import org.springframework.data.jpa.repository.Query;
import org.springframework.data.jpa.repository.QueryHints;
import org.springframework.data.jpa.repository.query.Procedure;
import org.springframework.data.querydsl.ListQuerydslPredicateExecutor;
import org.springframework.data.repository.CrudRepository;
import org.springframework.data.repository.query.Param;
import org.springframework.transaction.annotation.Transactional;
Expand All @@ -45,8 +59,8 @@
* @author Diego Krupitza
* @author Geoffrey Deremetz
*/
public interface UserRepository
extends JpaRepository<User, Integer>, JpaSpecificationExecutor<User>, UserRepositoryCustom {
public interface UserRepository extends JpaRepository<User, Integer>, JpaSpecificationExecutor<User>,
UserRepositoryCustom, ListQuerydslPredicateExecutor<User> {

/**
* Retrieve users by their lastname. The finder {@literal User.findByLastname} is declared in
Expand Down
11 changes: 1 addition & 10 deletions ...ta/jpa/repository/support/JpaRepositoryFactoryBeanEntityPathResolverIntegrationTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,6 @@
import org.springframework.data.querydsl.SimpleEntityPathResolver;
import org.springframework.test.util.ReflectionTestUtils;

import com.querydsl.core.types.EntityPath;

/**
* Unit tests for {@link EntityPathResolver} related tests on {@link JpaRepositoryFactoryBean}.
*
Expand All @@ -47,14 +45,7 @@ class JpaRepositoryFactoryBeanEntityPathResolverIntegrationTests {
@EnableJpaRepositories(basePackageClasses = UserRepository.class, //
includeFilters = @Filter(type = FilterType.ASSIGNABLE_TYPE, classes = UserRepository.class))
static class BaseConfig {

static final EntityPathResolver RESOLVER = new EntityPathResolver() {

@Override
public <T> EntityPath<T> createPath(Class<T> domainClass) {
return null;
}
};
static final EntityPathResolver RESOLVER = SimpleEntityPathResolver.INSTANCE;
}

@Configuration
Expand Down