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

[Fix] Query with(pageable) and with(Pageable.unpaged) #4007

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Jatish-Khanna
Copy link

  • You have read the Spring Data contribution guidelines.
  • You use the code formatters provided here and have them applied to your changes. Don’t submit any formatting related changes.
  • You submit test cases (unit or integration tests) that back your changes.
  • You added yourself as author in the headers of the classes you touched. Amend the date range in the Apache license header if needed. For new types, add the license header (copy from another file and set the current year only).

There's a fix proposed when we want to use the same query for the pageable and unpaged instance. A typical case would be 

Get all the Object using the mongoTemplate from the Repository
      mongoTemplate.find(filterQuery.with(pageable), CustomBean.class)

Get the count of all the instance but unpaged
     mongoTemplate.count(filterQuery.with(Pageable.unpaged()), CustomBean.class);

And implementing the page instance
   new PageImpl<>(mongoDocumentsPaged, pageable, count);

In the order of invoking 
  with(pageable) comes first will set the parameters - "skip, limit, sort" and then invoking 
  with(Pageable.unpaged()) won't reset them to "0, 0, Sort.unsorted()" 

The fix will reset 

1) in the with(Pageable pageable)
  
			this.limit = 0; // default value
			this.skip = 0; // default value

2) in the with(Sort sort)
   this.sort = sort; // which will be Sort.unsorted()
[Fix] Query with(pageable) and with(Pageable.unpaged)
@pivotal-cla
Copy link

@Jatish-Khanna Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Mar 25, 2022
@pivotal-cla
Copy link

@Jatish-Khanna Thank you for signing the Contributor License Agreement!

@christophstrobl
Copy link
Member

christophstrobl commented Mar 25, 2022

Thanks for the PR! Please spend some time describing the problem fixed by the change. This helps not only us but also others who want to understand the reason behind it.

@christophstrobl christophstrobl added the status: waiting-for-feedback We need additional information before we can continue label Mar 25, 2022
@Jatish-Khanna
Copy link
Author

Jatish-Khanna commented Mar 25, 2022

There's a fix proposed when we want to use the same query for the pageable and unpaged instance. A typical case would be

Get all the Object using the mongoTemplate from the Repository
mongoTemplate.find(filterQuery.with(pageable), CustomBean.class)

Get the count of all the instance but unpaged
mongoTemplate.count(filterQuery.with(Pageable.unpaged()), CustomBean.class);

And implementing the page instance
new PageImpl<>(mongoDocumentsPaged, pageable, count);

In the order of invoking
with(pageable) comes first will set the parameters - "skip, limit, sort" and then invoking
with(Pageable.unpaged()) won't reset them to "0, 0, Sort.unsorted()"

The fix will reset

  1. in the with(Pageable pageable)
			this.limit = 0; // default value
			this.skip = 0; // default value
  1. in the with(Sort sort)
                       this.sort = sort; // which will be Sort.unsorted()

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Mar 25, 2022
@Jatish-Khanna
Copy link
Author

@christophstrobl please let me know if details to be added

@christophstrobl
Copy link
Member

thanks for adding the comment - please always make sure to run all tests, because this as change breaks existing behavior.
Query has a static of method that allows create a modifiable copy which could be used for running the count operation.

Query filter = ... // just the criteria no paging / sorting

template.find(Query.of(filter).with(pageable), CustomBean.class)
template.count(Query.of(filter).skip(-1).limit(-1), CustomBean.class);

Nevertheless, I'll take this to the team for further discussion.

@christophstrobl christophstrobl added status: pending-design-work Needs design work before any code can be developed for: team-attention An issue we need to discuss as a team to make progress and removed status: waiting-for-triage An issue we've not yet triaged status: feedback-provided Feedback has been provided labels May 10, 2022
@Jatish-Khanna
Copy link
Author

Jatish-Khanna commented May 24, 2022

I can understand that we can modify the Query with the static functions, although the feature has been provided to accept Pageable.unpaged() ->

Must auto-reset the properties "skip and limit" when an unpaged instance has been used as an argument.

In the case mentioned, you must also reset the sort property of pagination via the static method which will be done automatically once an unpaged instance has been used as an argument.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
for: team-attention An issue we need to discuss as a team to make progress status: pending-design-work Needs design work before any code can be developed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants