spring-data-commons icon indicating copy to clipboard operation
spring-data-commons copied to clipboard

Add PageCollectors into Util

Open zorglube opened this issue 4 years ago • 10 comments

  • [x] You have read the Spring Data contribution guidelines.
  • [x] You use the code formatters provided here and have them applied to your changes. Don’t submit any formatting related changes.
  • [x] You submit test cases (unit or integration tests) that back your changes.
  • [x] 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).

zorglube avatar Oct 25 '21 16:10 zorglube

@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.

pivotal-cla avatar Oct 25 '21 16:10 pivotal-cla

@zorglube Thank you for signing the Contributor License Agreement!

pivotal-cla avatar Oct 26 '21 09:10 pivotal-cla

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

odrotbohm avatar Feb 16 '22 15:02 odrotbohm

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));
  }

}

zorglube avatar Feb 17 '22 20:02 zorglube

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.

odrotbohm avatar Feb 18 '22 07:02 odrotbohm

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 avatar Feb 18 '22 12:02 zorglube

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.

zorglube avatar Feb 18 '22 12:02 zorglube

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.

odrotbohm avatar Feb 18 '22 12:02 odrotbohm

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 avatar Feb 18 '22 12:02 zorglube

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

zorglube avatar Feb 23 '22 16:02 zorglube