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

Add overload to ReactiveSortingRepository accepting a Limit parameter. #2959

Closed
wants to merge 4 commits into from

Conversation

hwan33
Copy link

@hwan33 hwan33 commented Oct 18, 2023

Related issue: #2923

Motivation

Flux<T> findAll(Sort sort, Limit limit);

  • It's good to add findAll(sort, limit) on ReactiveSortingRepository

Modification

  • Add findAll(sort, limit) on ReactiveSortingRepository

Result

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Oct 18, 2023
replace `Limit.unlimited()` with `Limit.of(Integer.MAX_VALUE)`

Related spring-projects#2923
@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 Oct 23, 2023
@mp911de
Copy link
Member

mp911de commented Oct 25, 2023

Thanks for proposing a pull request. Adding Limit doesn't align well with the intent of this interface definition as ReactiveSortingRepository expresses sort capabilities. Also, implementing a default method using take creates a risk of fully loading all data. This will come as a surprise to users and can easily generate incidents. Therefore, we do not plan to merge this pull request and recommend using query derivation using Limit for the time being.

@mp911de mp911de closed this Oct 25, 2023
@hwan33
Copy link
Author

hwan33 commented Nov 3, 2023

@mp911de
Thank you for reviewing my pull request and for the feedback provided. I understand the concerns about aligning with the interface's intent and the potential risks associated with the default implementation using take that could lead to loading all data into memory unexpectedly.

Before I move on from this, I would like to clarify a few points to ensure that I fully understand the project's direction and to contribute effectively in the future:

  1. Is the main concern that introducing a limit parameter does not fit well with the ReactiveSortingRepository interface's intended purpose, which is to focus solely on sorting capabilities?

  2. If the addition of a limit parameter itself is not the issue, is the concern primarily with the default method's implementation that uses take? In that case, would an alternative implementation like the following be more aligned with the project's standards?

return findAll(sort)
    .handle((t, sink) -> {
        if (sink.requestedFromDownstream() > 0) {
            sink.next(t);
        }
    })
    .limitRequest(maxLimit);
  1. If neither the above concern addresses the core issue, could you please provide some guidance or a sample of the preferred implementation approach? It would be greatly beneficial to understand the direction in which we wish to take the repository capabilities, especially concerning limiting the result set.

Your insights will be invaluable for guiding future contributions and ensuring they are in line with the project's goals.

Thank you for your time and consideration.

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 this pull request may close these issues.

Add overload to ReactiveSortingRepository accepting a Limit parameter.
4 participants