search

VanRoy / spring-data-jest

Spring Data Implementation for Jest
Apache License 2.0
227 stars 78 forks source link

Use Pageable.getOffset instead of calculating the start record in JestElasticsearchTemplate #122

Closed jbogan closed 5 years ago

jbogan commented 5 years ago

Request

Could the following line of code in JestElasticsearchTemplate be changed to use Pageable.getOffset instead?

https://github.com/VanRoy/spring-data-jest/blob/97428c3bf7893a61b1b7b18f5ce2369505a474f4/spring-data-jest/src/main/java/com/github/vanroy/springdata/jest/JestElasticsearchTemplate.java#L1153

Details

In the referenced line of code, the start position is calculated using Pageable.getPageSize * Pageable.getPageNumber. The same result is also available through the Pageable.getOffset method.

I know the main difference is Pageable.getOffset returns a long and the startRecord assignment is an int so there will be some Integer overflow issues to worry about (see Potential Concerns).

Reasoning

The reasoning for asking is we are using a custom Pageable that works with limits and offsets (similar to https://stackoverflow.com/a/32763885). Unfortunately there does not seem to be an easy way to alter/override the referenced JestElasticsearchTemplate line without making our own version of the class.

Potential Concerns

My main concerns with the change are:

  1. It would impact current users of the library that use a custom Pageable that doesn't calculate the offset [correctly]. That means this change will have to be communicated clearly in the the release notes.
  2. Potential integer overflow issue during the downcast from long to int. Though, this would be an issue in the current code if pageSize * pageNumber was high enough. Plus that would be well into the Elasticsearch max_result_window territory.

Way Ahead

If you agree with this change, I'd be happy to submit a PR for approval.

VanRoy commented 5 years ago

Hi @jbogan , It's really seem's to be the right way to implement this. I will be very glad if we propose a PR.

Thanks a lot for your feedback. Julien.

jbogan commented 5 years ago

Hi @VanRoy, sounds good! I'll find some time to submit the PR within the next week.

VanRoy commented 5 years ago

Hi @jbogan , Thanks a lot for your help. You PR is merged and released in 3.2.5.RELEASE version.