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.

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

Allow sorting of unpaged results #2691

Closed
naihil opened this issue Nov 7, 2022 · 9 comments
Closed

Allow sorting of unpaged results #2691

naihil opened this issue Nov 7, 2022 · 9 comments
Assignees
Labels
status: declined A suggestion or change that we don't feel we should currently apply

Comments

@naihil
Copy link

naihil commented Nov 7, 2022

QuerydslPredicateExecutor provides a method Page<T> findAll(Predicate predicate, Pageable pageable);
You can pass Pageable.unpaged() to get all rows in single page, in this case result not sorted.
If you write custom implementation of unpaged() that has some sort and pass it to findAll, this doesnt work because of this code:
https://github.com/spring-projects/spring-data-jpa/blob/main/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/support/Querydsl.java#L117
`

	if (pageable.isUnpaged()) {
		return query;
	}

	query.offset(pageable.getOffset());
	query.limit(pageable.getPageSize());

	return applySorting(pageable.getSort(), query);

`
Sorting applied after isUnpaged() check.

We can change code to:
`

	if (pageable.isPaged()) {
		query.offset(pageable.getOffset());
		query.limit(pageable.getPageSize());
	}

	return applySorting(pageable.getSort(), query);

`
And this code allows sorting of unpaged queries.
Linked with #3761

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Nov 7, 2022
naihil added a commit to naihil/spring-data-jpa that referenced this issue Nov 10, 2022
@schauder schauder self-assigned this Nov 14, 2022
@schauder schauder added status: waiting-for-feedback We need additional information before we can continue and removed status: waiting-for-triage An issue we've not yet triaged labels Nov 15, 2022
@schauder
Copy link
Contributor

Could you please clarify why you would want to use sorting with pagination if you actually do not want to page the result?

Instead you could use one of the methods that allow direct supply of sorting.

@naihil
Copy link
Author

naihil commented Nov 15, 2022

Simple example: frontend with data table and rows-per-page combobox with option "All rows", from request you get pageable, pass it to repository without any additional logic and receive page that can be sorted/unsorted/one from any others or single page with all rows

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Nov 15, 2022
@schauder schauder added status: waiting-for-triage An issue we've not yet triaged and removed status: feedback-provided Feedback has been provided labels Nov 15, 2022
@mp911de
Copy link
Member

mp911de commented Dec 2, 2022

Pageable.unpaged() is a substitute for null. Previously, we accepted null as parameter for Pageable. Following that line of thought, there would not be a Sort associated with a null Pageable.

@mp911de mp911de closed this as not planned Won't fix, can't repro, duplicate, stale Dec 2, 2022
@mp911de mp911de added status: declined A suggestion or change that we don't feel we should currently apply and removed status: waiting-for-triage An issue we've not yet triaged labels Dec 2, 2022
@jiayp
Copy link

jiayp commented Dec 2, 2022

If we don't accept unpaged with sort Pageable,I think we should add a assert for that.

@jiayp
Copy link

jiayp commented Dec 2, 2022

My understanding is that no unpaged is a special kind of paging, so we should allow him to carry sorting properties.

@julianschelker
Copy link

I have a similar usecase: Query is quite complex and table has 17 million entries. I want to fetch the first 100 entries that match a criteria sorted by a field. This all works quite fast thanks to indices on field that needs to be sorted in the database. However the findAll with pageable in Spring also executes a query "Select count(...)" to return the total rows which takes subsequently forever.

Why can't I provide the findAll method with the information that I don't need to know the count of all rows, but still provide sort and limit parameters?

That corresponds to the request of providing a Unpaged object with sort and limit.

@mp911de
Copy link
Member

mp911de commented Jan 2, 2023

How about defining a query method returning Slice: Slice<MyObject> findAllByFoo(String foo, Pageable pageable)?

@trohr
Copy link

trohr commented May 4, 2023

What the hell?

I need to support paging in a spring rest controller. When paging attributes are specified, query should be paged. When paging is not specified, all results should be returned. This is intuitive behaviour one would expect from a general API method.

Sorting can be specified for either paged or unpaged version of the request.

Because of this issue (and the fact that I cannot have two controller methods for the same endpoint, but different args), we would have to be forced to either not use Pageable as automatically resolved parameter to the GET methods and create our own type and then decide in APPLICATION to convert it to Pageable / Sort and call different methods on a repository based on whether the request is sorted and unpaged / or paged... OR we could try to fix the problems and override a lot of spring code such as PageableHandlerMethodArgumentResolver, create own Unpaged instance, etc...

Both solutions are very poor and hard to maintain solutions.

This is poor design decision from you guys. I get that Unpaged was upgrade from null, but ... let's go the next step please.

Would you please revisit this problem with Unpaged Pageable with sorting?

@naihil
Copy link
Author

naihil commented May 4, 2023

@trohr I use this workaround:

  1. In controller get method i have 2 params: Pageable pageable and @RequestParam(value = "unpaged", required = false) Boolean unpaged;

  2. This params provided to service method: findService.find(Pageable pageable, Boolean unpaged)

  3. In service method i have such code:
    ` PageRequest pageRequest;
    if (Boolean.TRUE.equals(unpaged)) {
    long count = workObjectRepository.count(predicate);
    pageRequest = PageRequest.of(0, (int) count, pageable.getSort());
    }
    else {
    pageRequest = PageRequest.of(pageable.getPageNumber(), pageable.getPageSize(), pageable.getSort());
    }

     final Page<WorkType> workObjectPage = workObjectRepository.findAll(predicate, pageRequest);
    

`

There is +1 query to get rows count with needed predicate, but you dont need multiple methods for paged and unpaged requests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: declined A suggestion or change that we don't feel we should currently apply
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants