JdbcAggregateTemplate.findAll with Query and Pageable issue

[Alterant-zz]

The JdbcAggregateTemplate.findAll(Query query, Class domainType, Pageable pageable) accept Pageable, but Query can have it's own limit\offset\sort params set.
When this happens an incorrect SQL is generated.

I think it's worth to at least mention in the javadoc that Query should not have any limit\offset\sort params for this method.

Possible fixes:

• An exception in this case
• Pageable should fully override limit\offset\sort params of the Query

Example:

Criteria criteria = Criteria.where(ParentEntity.COLUMN_ACTIVE).is(true);
Query query = Query.query(criteria)
    .offset(1)
    .limit(3)
    .sort(Sort.by(Sort.Order.desc(ParentEntity.COLUMN_TITLE).nullsFirst()));

Page<ParentEntity> page = operations.findAll(
    query, ParentEntity.class,
    PageRequest.of(1, 2, Sort.Direction.ASC, ParentEntity.COLUMN_TITLE)
);

generates a query where limit and offset is from Pageable, but sorting includes both criteria

SELECT "parent_entity"."id" AS "id", "parent_entity"."title" AS "title", "parent_entity"."active" AS "active"
FROM "parent_entity"
WHERE "parent_entity"."active" = ?
ORDER BY
  "parent_entity"."title" DESC NULLS FIRST,
  "parent_entity"."title" ASC
LIMIT 2 OFFSET 2

and COUNT have all criteria from Query including limit and offset and does not work at least in Postgresql

SELECT COUNT(1)
FROM "parent_entity"
WHERE "parent_entity"."active" = ?
ORDER BY "parent_entity"."title" DESC NULLS FIRST
LIMIT 3 OFFSET 1

[schauder]

I tried, but failed, to reproduce the issue in https://github.com/spring-projects/spring-data-relational/tree/issue/2138-query-with-pageable

Please provide a full reproducer, preferably as a pull request to the branch mentioned above, but I'm also happy to take a GitHub repository or a zip-file.

[Alterant-zz]

Sorry @schauder I was not able to build your branch because of this:

org\springframework\data\jdbc\repository\config\JdbcRepositoryConfigExtension.java:158:18
java: cannot find symbol
  symbol: method contribute(org.springframework.data.repository.config.AotRepositoryContext,org.springframework.aot.generate.GenerationContext)

here

public class JdbcRepositoryConfigExtension extends RepositoryConfigurationExtensionSupport {
...
	public static class JdbcRepositoryRegistrationAotProcessor extends RepositoryRegistrationAotProcessor {

		private static final String MODULE_NAME = "jdbc";

		protected @Nullable JdbcRepositoryContributor contribute(AotRepositoryContext repositoryContext,
				GenerationContext generationContext) {

			// do some custom type registration here
			super.contribute(repositoryContext, generationContext);      <-- line with error

			repositoryContext.getResolvedTypes().stream().forEach(type -> {
				TypeContributor.contribute(type, it -> true, generationContext);
			});

I have found an issue in the tests, fixed it and was able to reproduce the issue on 3.5.x.
You can find your tests ported to 3.5.x here:
https://github.com/Alterant-zz/spring-data-relational/tree/35x-query-with-pageable

Also checked the code 3.5.x <-> 4.0.0-SNAPSHOT and think that issue should still be there. So I created a PR with a fix for the tests:
#2144

[Alterant-zz]

Things get a bit complicated because both Query and Pageable can be sorted/unsorted and with/without limits independently.

And I see no good solution for this. The initial idea was for Pageable to always take precedence, even when it is unsorted. But we can have a derived methods like findAllByOrderBy... with Pageable.
Also Pageable can be Unpaged. Should we use limits from the Query in this case ...

[Alterant-zz]

In my understanding, the problem is that the count query uses sorting and limitation from the Query:

public class JdbcAggregateTemplate implements JdbcAggregateOperations {
  ...
  @Override
  public <T> Page<T> findAll(Query query, Class<T> domainType, Pageable pageable) {

     Iterable<T> items = triggerAfterConvert(accessStrategy.findAll(query, domainType, pageable));
     List<T> content = StreamSupport.stream(items.spliterator(), false).collect(Collectors.toList());

                                                              //count is not aware of Pageable,
                                                              //but use everything from the query,
                                                              //including sorting and limits
     return PageableExecutionUtils.getPage(content, pageable, () -> accessStrategy.count(query, domainType));
  }
  ...
}

So the first step could be to remove limit and offset from the count query.
Also it might be worth considering removing sorting from the count query altogether.
I checked with Postresql - it does not take sorting into account in the query plan. But probably for some other DBMS it could have a performance penalty.

It should fix the issue.

The interaction of Pageable and Query can be left as it is:

• use sorting from both
• use limits from Query and override them from Pageable (if it isPaged).

Or changed somehow.
But this is a separate story.