-
Notifications
You must be signed in to change notification settings - Fork 701
Description
Hi team,
I have the following use case, that I believe will be fairly common for rest endpoints.
Use Case
I have a use case for a controller that can support pagination and sorting, but both are optional.
class FooController {
@GetMapping
List<Foo> getFoos(Pageable pageable) {
if (pageable.isPaged()) {
return foos.fetchPage(pageable.getPageNumber(), pageable.getPageSize(), pageable.getSort());
} else {
return foos.fetchAll(pageable.getSort());
}
}
}/fooswill return all resources/foos?page=2&size=3will return page 2, size 3 of resources, without sorting./foos?sort=bar,descwill return all resources, sorted by property bar descending./foos?page=2&size=3&sort=bar,descwill return page 2, size 3 of the sorted resource by property bar, descending.
Issue
I have set PageableHandlerMethodArgumentResolverSupport.fallbackPageable to Pageable.unpaged().
This works for /foos, returning all items without sorting.
But there is an error when attempting /foos?sort=bar,desc due to
Lines 85 to 87 in 2f43bd5
| if (sort.isSorted()) { | |
| return PageRequest.of(pageable.getPageNumber(), pageable.getPageSize(), sort); | |
| } |
- Attempting to construct
PageRequestfails onpageable.getPageNumber()throwingUnsupportedOperationException
I would have expected the above code to have looked something like
if (sort.isSorted()) {
if (pageable.isPaged()) {
return PageRequest.of(pageable.getPageNumber(), pageable.getPageSize(), sort);
} else {
return Pageable.unpaged(sort);
}
}as this would allow returning all items in the resource with sorting.
This feels like a bug to me, the ability to sort a resource shouldn't require pagination.
Fallback to unpaged property
As an aside, it seems strange that the fallbackPageable is the only property not settable via configuration
A property like spring.data.web.pageable.default-unpaged=true that can set the fallbackPageable in SpringDataWebAutoConfiguration.pageableCustomizer() to Pageable.unpaged() could make sense to me.
Thanks,
Josh