Fix value of request size sent to Elasticsearch when no Pageable is defined in a query

[sothawo]

When implementing the fix for #3089 in #3106 we introduced a regression. By adding the logic that the Pageable in a query defines the number of documents to retrieve from Elasticsearch in one call and the maxResults is used for the total number of documents fetched in a searchForStream called.

As a query that has no Pageable set has a default one with a page size of 10, this now leads to only 10 documents being returned on searches that do not use searchForStream. Users now need to explicitly need to set a Pageable.

One solution would be to not set a default Pageable in a Query, instead of the current default with size 10. Users that want to page in steps of ten anyway need to define a pageable in their query and adjust it for subsequent calls (setting the offset). Users not setting a pageable then would get the min(INDEX_MAX_RESULT_WINDOW, maxResults) number of documents. INDEX_MAX_RESULT_WINDOW has the value of 10000 and is the maximum that Elasticsearch normally might return. This should restore the previous behaviour for users not setting a Pageable but a maxResults value.

[pnowak85]

hi @sothawo

thanks for creating the issue - i like the proposed solution, but it might be breaking for others if the rely on that default Pageable with size 10 while not specifying Pageable nor maxResults on the query. Because it just returned you 10 elements until now - maybe for a quick suggestion query someone would skip the Pageable because the default is enough for their use case 🤔

[sothawo]

Thanks for the comment; valid point I was thinking about as well.

One other solution would be to have some "global" state somewhere where the user can set the default Pageable to use, and the default default-Pageable would be the 10. You could then set this to the unpaged instance for your case.

Though having some global state somewhere is something I'd like to avoid. In Spring Dat5a MongoDB there is a global object providing version info and such, but nothing the user can modify. But probably I'll add something like that.

[bugada]

This change broke all our services relying on withMaxResults and not on Pageable (we don't need pagination).

A simple query like this is

NativeQuery.builder().withQuery(q -> q.matchAll(ma -> ma)).withMaxResults(10000).build();

is returning only 10 records instead of 10000 max.
I think this is counter-intuitive.

Now it's a pain to fix all the queries in hundreds of projects.

Probably defaulting the maxResults to 10 and let pageable to be null would not create breaking change with previous verisons.

Hope you can find a solution
Thanx Andrea

[sothawo]

Thanks for the comment Andrea, indeed seems the best way to go with.

[sothawo]

I changed the calculation, the logic is described in the pull request linked to this issue.

Next release day is next friday, I'll keep this PR open until Tuesday, in case someone finds an error in this logic, otherwise I'll merge then.

[bugada]

The logic seems fine to me, hoping that INDEX_MAX_RESULT_WINDOW is not a constant but the value retrieved from index.max_result_window set in the index.

[sothawo]

At the moment this is (and has been before) the hard coded default value from Elasticsearch.

To get that value from the index the query is built for would mean that Spring Data Elasticsearch needs to request this value with an additional call. These values would need to be cached somehow, how long until a refresh would be necessary?

Not sure if we should implement something like this, Elasticsearch docs advice against changing:

See Scroll or Search After for a more efficient alternative to raising this.

[bugada]

I agree with you that is not advisable to change maxResultWindow, we always use scroll and searchAfter.
The only case I had was a pagination of about 12.000 elements back and forward: that is not achievable using Scroll o SearchAfter.

I would not change INDEX_MAX_RESULT_WINDOW for backward compatibility with Pageable.unpaged().

With this patch we can go over using that query

NativeQuery.builder().withQuery(q -> q.matchAll(ma -> ma)).withMaxResults(12000).build();

or the INDEX_MAX_RESULT_WINDOW is used also for limiting maxResults?

[sothawo]

If you do not set a Pageable (either with a size or unpaged) on the query, but only the maxResults you'll get 12000 requested from Elasticsearch.

[bugada]

Thanks, so it looks good to me.