Skip to content

Introduce Limit type #2836

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 5 commits into from
Closed

Introduce Limit type #2836

wants to merge 5 commits into from

Conversation

christophstrobl
Copy link
Member

Limit can be used as Parameter within repository query methods to define the maximum number of results to be returned.

List<Person> findByFirstnameLike(String firstname, Limit limit);

List<Person> findByFirstnameLike(String firstname, Sort orderBy, Limit limit);

Window<User> findByFirstnameLikeOrderByLastname(ScrollPosition pageable, Limit limit);

The default ParameterAccessor implementation will pick up Limit and Sort to assemble a Pageable out of the provided values if present to provide out of the box support for modules using ParameterAcessor#getPageable() to apply order and limit.

Limit#max is aligned to Page.size, both using int. We might want to consider long for Limit and error if the actual value exceeds the Page one when assembling the Pageable.

@christophstrobl christophstrobl linked an issue May 31, 2023 that may be closed by this pull request
Allow dynamic limiting of repository query results #2827
Closed
@christophstrobl christophstrobl mentioned this pull request May 31, 2023
Closed
odrotbohm
odrotbohm reviewed Jun 1, 2023
View reviewed changes
Copy link
Member

@odrotbohm odrotbohm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of minor findings.

src/main/asciidoc/repositories-paging-sorting.adoc

|===

The `Top` keyword used to limit results can be used to along with `Pageable` whereas `Top` defines the total maximum of results, whereas the Pageable parameter may reduce this this number.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Double "this this"?

src/main/java/org/springframework/data/repository/query/QueryMethod.java
@@ -290,6 +264,41 @@ public String toString() {
return method.toString();
}

public void validate() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs to be public?

odrotbohm
odrotbohm reviewed Jun 1, 2023
View reviewed changes
Copy link
Member

@odrotbohm odrotbohm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of minor findings.

mp911de
mp911de requested changes Jul 4, 2023
View reviewed changes
Copy link
Member

@mp911de mp911de left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some comments, seconding what Ollie said.

src/main/java/org/springframework/data/domain/Limit.java
/**
* @return {@literal true} if no limiting (maximum value) should be applied.
*/
default boolean isUnlimited() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also have a isLimited() method.

src/main/java/org/springframework/data/domain/Pageable.java
@@ -24,6 +24,7 @@
*
* @author Oliver Gierke
* @author Mark Paluch
* @author Christoph Strobl
*/
public interface Pageable {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add a toLimit() method as well, similar to toScrollPosition()?

@mp911de mp911de marked this pull request as ready for review July 5, 2023 07:32
mp911de pushed a commit that referenced this pull request Jul 5, 2023
@christophstrobl @mp911de
Introduce Limit type to limit repository query results.
a5408a4 
We now accept Limit as type to express dynamic repository query limits.

Closes #2827
Original pull request: #2836
mp911de added a commit that referenced this pull request Jul 5, 2023
@mp911de
Polishing.
15bb8aa 
Refine documentation. Introduce isLimited method to mirror isPresent/isSorted semantics. Introduce Pageable.toLimit() method to deduplicate code.

See #2827
Original pull request: #2836
@mp911de mp911de changed the title Introduce Limit. Introduce Limit type Jul 5, 2023
@mp911de
Copy link
Member

mp911de commented Jul 5, 2023

That's merged and polished now.

@mp911de mp911de closed this Jul 5, 2023
@mp911de mp911de deleted the issue/2827 branch July 5, 2023 09:54
@mp911de mp911de added this to the 3.2 M1 (2023.1.0) milestone Jul 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@odrotbohm odrotbohm odrotbohm left review comments

@mp911de mp911de mp911de requested changes

Assignees

@mp911de mp911de

Labels
None yet
Projects
None yet
Milestone
3.2 M1 (2023.1.0)
Development

Successfully merging this pull request may close these issues.

Allow dynamic limiting of repository query results
3 participants
@christophstrobl @mp911de @odrotbohm