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 PageCollectors into Util #2490

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

Conversation

zorglube
Copy link

@zorglube zorglube commented Oct 25, 2021

  • 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).

@pivotal-cla
Copy link

@zorglube Please sign the Contributor License Agreement!

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

See the FAQ for frequently asked questions.

@zorglube zorglube marked this pull request as draft October 25, 2021 16:38
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Oct 25, 2021
@zorglube zorglube marked this pull request as ready for review October 26, 2021 09:38
@pivotal-cla
Copy link

@zorglube Thank you for signing the Contributor License Agreement!

@zorglube zorglube marked this pull request as draft October 26, 2021 09:38
Signed-off-by: Zorglube <[email protected]>
Signed-off-by: Zorglube <[email protected]>
Signed-off-by: Zorglube <[email protected]>
Signed-off-by: Zorglube <[email protected]>
Signed-off-by: Zorglube <[email protected]>
@zorglube zorglube marked this pull request as ready for review October 28, 2021 17:39
@odrotbohm
Copy link
Member

Is there any existing ticket that describes, what issue this proposal is trying to solve? If not, would you mind to elaborate?

@odrotbohm odrotbohm self-assigned this Feb 16, 2022
@odrotbohm odrotbohm added status: waiting-for-feedback We need additional information before we can continue and removed status: waiting-for-triage An issue we've not yet triaged labels Feb 16, 2022
@zorglube
Copy link
Author

zorglube commented Feb 17, 2022

Is there any existing ticket that describes, what issue this proposal is trying to solve? If not, would you mind to elaborate?

As fa as I know their is no ticket related to this PR.

The problem I tried to handle is just a small writing 'issue'.
I you like to use Spring-Data to get a Page<T> of something an then appli any process to the Page before returning the processed Page you'll have to write something like :

import static java.utils.Objects.toList;

class Something {
  
  private final Predicate<T> filter;
  private final Comparator<T> order;
  
  public Page getProcessedPage(Pageable pageable){
    List<T> items = repo.getPage(pageable).stream().map(process).filter(filter).sort(order).collect(toList());
    Page<T> page = new PageImpl(items, pageable, items.size());
    return page; 
  }

}

My purpose is to make this possible with this writing :

import static org.springframework.data.util.PageCollectors.toFilteredSortedPage;

class Something {
  
  private final Predicate<T> filter;
  private final Comparator<T> order;

  public Page getProcessedPage(Pageable pageable){
    return repo.getPage(pageable).stream().collect(toFilteredSortedPage(pageable, filter, order));
  }

}

@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 Feb 17, 2022
@odrotbohm
Copy link
Member

You can apply processing to elements of aPage via its ….map(…) method. We do not offer any methods to filter or reorder the elements, as that would by definition result in invalid Pages regarding their metadata and position within the overall set of elements they provide a view into.

The code you show effective produces broken Page instances as the ordering potentially does not match the Sort provided in the Pageable, it will be considered the last page if the filtering of the elements drops elements etc. This is not idiomatic usage of a Page instance. I'd love to learn what the actual use case for the post-processing and retaining the original metadata is, but as it stands now, I'm rather inclined to reject this PR.

@zorglube
Copy link
Author

I forgotted another use cas I had :

import static org.springframework.data.util.PageCollectors.toFilteredSortedPage;

class Something {
  
  private final Predicate<T> filter;
  private final Comparator<T> order;

  public Page getProcessedPage(Pageable pageable) {
    List<T> list_1 = repo.getSomeData(); 
    List<T> list_2 = repo/service/watever.getSomeData(); 
    return list_1.stream().map/filter/sort( ... `imply list_2` ... ).collect(toFilteredSortedPage(pageable, filter, order));
  }

}

@zorglube
Copy link
Author

You can apply processing to elements of aPage via its ….map(…) method. We do not offer any methods to filter or reorder the elements, as that would by definition result in invalid Pages regarding their metadata and position within the overall set of elements they provide a view into.

Yes I'm aware of the Page<T>.map(...).

The code you show effective produces broken Page instances as the ordering potentially does not match the Sort provided in the Pageable, it will be considered the last page if the filtering of the elements drops elements etc. This is not idiomatic usage of a Page instance. I'd love to learn what the actual use case for the post-processing and retaining the original metadata is, but as it stands now, I'm rather inclined to reject this PR.

Can you wait a few, I'll dig to find my old use case, as you ca see it was four months ago.
Beside that, four months ago, I was already aware of the Page<T>.map(...) and I didn't found a solution, that's why I wrote those Collectors.

@odrotbohm
Copy link
Member

Sure thing. Be advised thought that we consider everything util internal utilities. I.e. unless we actually need code in there ourselves, we're not going to add code there, especially if it's only convenience, as it imposes maintenance cost for no real benefit to the team.

@zorglube
Copy link
Author

zorglube commented Feb 18, 2022

Sure thing. Be advised thought that we consider everything util internal utilities. I.e. unless we actually need code in there ourselves, we're not going to add code there, especially if it's only convenience, as it imposes maintenance cost for no real benefit to the team.

Yes I understand ;-)
I purposed it because I "needed" it, and I thought someone else could need it AND I though it might be good to provide it through Spring. Anyhow it's already integrated in my project.

@zorglube
Copy link
Author

Hello @odrotbohm ,

You were right, the use case that made me write this code, was the result of a bad design choices. As I said it was something like :

import static org.springframework.data.util.PageCollectors.toFilteredSortedPage;

class Something {
  
  private final Predicate<T> filter;
  private final Comparator<T> order;

  public Page<T> getProcessedPage(Pageable pageable) {
    List<T> list_1 = repo.getSomeData(); 
    ...
    List<T> list_2 = repo/service/watever.getSomeData(); 
    ...
    return list_1.stream().map/filter/sort( ... `imply list_2` ... ).collect(toFilteredSortedPage(pageable, filter, order));
  }

}

Thing that could have been written like :

class Something {
  
  private final Predicate<T> filter;
  private final Comparator<T> order;

  public Page<T> getProcessedPage(Pageable pageable) {
    ...
    return repo.getSomeDataPage(pageable);
  }

}

However, I still think there's a use case for this kind of code. You tell me.

BR,
@zorglube

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: feedback-provided Feedback has been provided
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants