Repository: jakartaee/data
Pull request #521 (2024-03-05 17:21:11)

requestTotal is lost when subsequent methods are invoked
While I was bringing in the requestTotal updates, I spotted a bug where a value of `false` is lost when additional builder methods are invoked.  For example, if you write `PageRequest.ofSize(20).requestTotal(false).page(5)`, the `page` method reverts the requestTotal value back to its default of `true`.  This is happening because of a constructor in `Pagination` that defaults the value to `true` which all of these fluent builder-style methods are using rather than supplying the current value of requestTotal.  I added a unit test confirming this is broken, and fixed the issue by removing the extra constructor so that we force all methods to either explicitly supply or default the value as is done for all other record components.


Patch:
diff --git a/api/src/main/java/jakarta/data/page/PageRequest.java b/api/src/main/java/jakarta/data/page/PageRequest.java
index e61aa7a5..d6ec61c7 100644
--- a/api/src/main/java/jakarta/data/page/PageRequest.java
+++ b/api/src/main/java/jakarta/data/page/PageRequest.java
@@ -102,7 +102,7 @@ public interface PageRequest<T> {
      * @return a new instance of <code>PageRequest</code>. This method never returns <code>null</code>.
      */
     static <T> PageRequest<T> of(Class<T> entityClass) {
-        return new Pagination<T>(1, 10, Collections.emptyList(), Mode.OFFSET, null);
+        return new Pagination<T>(1, 10, Collections.emptyList(), Mode.OFFSET, null, true);
     }
 
     /**
@@ -114,7 +114,7 @@ public interface PageRequest<T> {
      * @throws IllegalArgumentException when the page number is negative or zero.
      */
     static <T> PageRequest<T> ofPage(long pageNumber) {
-        return new Pagination<T>(pageNumber, 10, Collections.emptyList(), Mode.OFFSET, null);
+        return new Pagination<T>(pageNumber, 10, Collections.emptyList(), Mode.OFFSET, null, true);
     }
 
     /**
@@ -127,7 +127,7 @@ public interface PageRequest<T> {
      * @throws IllegalArgumentException when maximum page size is negative or zero.
      */
     static <T> PageRequest<T> ofSize(int maxPageSize) {
-        return new Pagination<T>(1, maxPageSize, Collections.emptyList(), Mode.OFFSET, null);
+        return new Pagination<T>(1, maxPageSize, Collections.emptyList(), Mode.OFFSET, null, true);
     }
 
     /**
diff --git a/api/src/main/java/jakarta/data/page/Pagination.java b/api/src/main/java/jakarta/data/page/Pagination.java
index dac99ab8..ddad618a 100644
--- a/api/src/main/java/jakarta/data/page/Pagination.java
+++ b/api/src/main/java/jakarta/data/page/Pagination.java
@@ -31,10 +31,6 @@ import java.util.stream.StreamSupport;
  */
 record Pagination<T>(long page, int size, List<Sort<? super T>> sorts, Mode mode, Cursor type, boolean requestTotal) implements PageRequest<T> {
 
-    Pagination(long page, int size, List<Sort<? super T>> sorts, Mode mode, Cursor type) {
-        this(page, size, sorts, mode, type, true);
-    }
-
     Pagination {
         if (page < 1) {
             throw new IllegalArgumentException("pageNumber: " + page);
@@ -59,32 +55,32 @@ record Pagination<T>(long page, int size, List<Sort<? super T>> sorts, Mode mode
 
     @Override
     public PageRequest<T> afterKeyset(Object... keyset) {
-        return new Pagination<T>(page, size, sorts, Mode.CURSOR_NEXT, new PageRequestCursor(keyset));
+        return new Pagination<T>(page, size, sorts, Mode.CURSOR_NEXT, new PageRequestCursor(keyset), requestTotal);
     }
 
     @Override
     public PageRequest<T> beforeKeyset(Object... keyset) {
-        return new Pagination<T>(page, size, sorts, Mode.CURSOR_PREVIOUS, new PageRequestCursor(keyset));
+        return new Pagination<T>(page, size, sorts, Mode.CURSOR_PREVIOUS, new PageRequestCursor(keyset), requestTotal);
     }
 
     @Override
     public PageRequest<T> afterKeysetCursor(Cursor keysetCursor) {
-        return new Pagination<T>(page, size, sorts, Mode.CURSOR_NEXT, keysetCursor);
+        return new Pagination<T>(page, size, sorts, Mode.CURSOR_NEXT, keysetCursor, requestTotal);
     }
 
     @Override
     public PageRequest<T> beforeKeysetCursor(Cursor keysetCursor) {
-        return new Pagination<T>(page, size, sorts, Mode.CURSOR_PREVIOUS, keysetCursor);
+        return new Pagination<T>(page, size, sorts, Mode.CURSOR_PREVIOUS, keysetCursor, requestTotal);
     }
 
     @Override
     public PageRequest<T> asc(String property) {
-        return new Pagination<T>(page, size, combine(sorts, Sort.asc(property)), mode, type);
+        return new Pagination<T>(page, size, combine(sorts, Sort.asc(property)), mode, type, requestTotal);
     }
 
     @Override
     public PageRequest<T> ascIgnoreCase(String property) {
-        return new Pagination<T>(page, size, combine(sorts, Sort.ascIgnoreCase(property)), mode, type);
+        return new Pagination<T>(page, size, combine(sorts, Sort.ascIgnoreCase(property)), mode, type, requestTotal);
     }
 
     private static final <E> List<E> combine(List<E> list, E element) {
@@ -107,18 +103,18 @@ record Pagination<T>(long page, int size, List<Sort<? super T>> sorts, Mode mode
 
     @Override
     public PageRequest<T> desc(String property) {
-        return new Pagination<T>(page, size, combine(sorts, Sort.desc(property)), mode, type);
+        return new Pagination<T>(page, size, combine(sorts, Sort.desc(property)), mode, type, requestTotal);
     }
 
     @Override
     public PageRequest<T> descIgnoreCase(String property) {
-        return new Pagination<T>(page, size, combine(sorts, Sort.descIgnoreCase(property)), mode, type);
+        return new Pagination<T>(page, size, combine(sorts, Sort.descIgnoreCase(property)), mode, type, requestTotal);
     }
 
     @Override
     public PageRequest<T> next() {
         if (mode == Mode.OFFSET) {
-            return new Pagination<T>(page + 1, this.size, this.sorts, Mode.OFFSET, null);
+            return new Pagination<T>(page + 1, this.size, this.sorts, Mode.OFFSET, null, requestTotal);
         } else {
             throw new UnsupportedOperationException("Not supported for keyset pagination. Instead use afterKeyset or afterKeysetCursor " +
                     "to provide the next keyset values or obtain the nextPageRequest from a CursoredPage.");
@@ -128,7 +124,7 @@ record Pagination<T>(long page, int size, List<Sort<? super T>> sorts, Mode mode
     @Override
     public PageRequest<T> previous() {
         if (mode == Mode.OFFSET) {
-            return page()<=1 ? null : new Pagination<T>(page - 1, this.size, this.sorts, Mode.OFFSET, null);
+            return page()<=1 ? null : new Pagination<T>(page - 1, this.size, this.sorts, Mode.OFFSET, null, requestTotal);
         } else {
             throw new UnsupportedOperationException("Not supported for keyset pagination. Instead use beforeKeyset or beforeKeysetCursor " +
                     "to provide the previous keyset values or obtain the previousPageRequest from a CursoredPage.");
@@ -156,12 +152,12 @@ record Pagination<T>(long page, int size, List<Sort<? super T>> sorts, Mode mode
 
     @Override
     public PageRequest<T> page(long pageNumber) {
-        return new Pagination<T>(pageNumber, size, sorts, mode, type);
+        return new Pagination<T>(pageNumber, size, sorts, mode, type, requestTotal);
     }
 
     @Override
     public PageRequest<T> size(int maxPageSize) {
-        return new Pagination<T>(page, maxPageSize, sorts, mode, type);
+        return new Pagination<T>(page, maxPageSize, sorts, mode, type, requestTotal);
     }
 
     @Override
@@ -169,31 +165,31 @@ record Pagination<T>(long page, int size, List<Sort<? super T>> sorts, Mode mode
         List<Sort<? super T>> sortList = sorts instanceof List ? List.copyOf((List<Sort<? super T>>) sorts)
                 : sorts == null ? Collections.emptyList()
                 : StreamSupport.stream(sorts.spliterator(), false).collect(Collectors.toUnmodifiableList());
-        return new Pagination<T>(page, size, sortList, mode, type);
+        return new Pagination<T>(page, size, sortList, mode, type, requestTotal);
     }
 
     @Override
     public PageRequest<T> sortBy(Sort<? super T> sort) {
-        return new Pagination<T>(page, size, List.of(sort), mode, type);
+        return new Pagination<T>(page, size, List.of(sort), mode, type, requestTotal);
     }
 
     @Override
     public PageRequest<T> sortBy(Sort<? super T> sort1, Sort<? super T> sort2) {
-        return new Pagination<T>(page, size, List.of(sort1, sort2), mode, type);
+        return new Pagination<T>(page, size, List.of(sort1, sort2), mode, type, requestTotal);
     }
 
     @Override
     public PageRequest<T> sortBy(Sort<? super T> sort1, Sort<? super T> sort2, Sort<? super T> sort3) {
-        return new Pagination<T>(page, size, List.of(sort1, sort2, sort3), mode, type);
+        return new Pagination<T>(page, size, List.of(sort1, sort2, sort3), mode, type, requestTotal);
     }
 
     @Override
     public PageRequest<T> sortBy(Sort<? super T> sort1, Sort<? super T> sort2, Sort<? super T> sort3, Sort<? super T> sort4) {
-        return new Pagination<T>(page, size, List.of(sort1, sort2, sort3, sort4), mode, type);
+        return new Pagination<T>(page, size, List.of(sort1, sort2, sort3, sort4), mode, type, requestTotal);
     }
 
     @Override
     public PageRequest<T> sortBy(Sort<? super T> sort1, Sort<? super T> sort2, Sort<? super T> sort3, Sort<? super T> sort4, Sort<? super T> sort5) {
-        return new Pagination<T>(page, size, List.of(sort1, sort2, sort3, sort4, sort5), mode, type);
+        return new Pagination<T>(page, size, List.of(sort1, sort2, sort3, sort4, sort5), mode, type, requestTotal);
     }
 }
\ No newline at end of file


Test patch:
diff --git a/api/src/test/java/jakarta/data/page/PageRequestTest.java b/api/src/test/java/jakarta/data/page/PageRequestTest.java
index efdd18dd..942fa5eb 100644
--- a/api/src/test/java/jakarta/data/page/PageRequestTest.java
+++ b/api/src/test/java/jakarta/data/page/PageRequestTest.java
@@ -88,6 +88,26 @@ class PageRequestTest {
               .isEqualTo("PageRequest{page=1, size=80, yearBorn DESC, monthBorn ASC, id ASC}"));
     }
 
+    @Test
+    @DisplayName("The requestTotal configuration must be preserved when adding subsequent configuration.")
+    void shouldRequestTotalConfigBePreserved() {
+        PageRequest<?> pageRequest1 = PageRequest.ofPage(1).withTotal().size(70);
+
+        assertSoftly(softly -> {
+            softly.assertThat(pageRequest1.page()).isEqualTo(1L);
+            softly.assertThat(pageRequest1.size()).isEqualTo(70);
+            softly.assertThat(pageRequest1.requestTotal()).isEqualTo(true);
+        });
+
+        PageRequest<?> pageRequest2 = PageRequest.ofSize(80).withoutTotal().page(2);
+
+        assertSoftly(softly -> {
+            softly.assertThat(pageRequest2.page()).isEqualTo(2L);
+            softly.assertThat(pageRequest2.size()).isEqualTo(80);
+            softly.assertThat(pageRequest2.requestTotal()).isEqualTo(false);
+        });
+    }
+
     @Test
     @DisplayName("Should throw IllegalArgumentException when page is not present")
     void shouldReturnErrorWhenThereIsIllegalArgument() {
diff --git a/api/src/test/java/jakarta/data/page/PaginationTest.java b/api/src/test/java/jakarta/data/page/PaginationTest.java
index ffc4daf0..774769b0 100644
--- a/api/src/test/java/jakarta/data/page/PaginationTest.java
+++ b/api/src/test/java/jakarta/data/page/PaginationTest.java
@@ -32,14 +32,17 @@ class PaginationTest {
     void shouldThrowExceptionWhenNoKeysetValuesWereProvided() {
         assertThatIllegalArgumentException()
                 .as("Mode must be different from OFFSET when cursor is null")
-                .isThrownBy(() -> new Pagination<>(1, 10, Collections.emptyList(), PageRequest.Mode.CURSOR_NEXT, null));
+                .isThrownBy(() -> new Pagination<>(1, 10, Collections.emptyList(), PageRequest.Mode.CURSOR_NEXT, null, true));
     }
 
     @Test
     @DisplayName("Should throw UnsupportedOperationException when keyset is not supported")
     void shouldThrowExceptionWhenKeysetIsNotSupported() {
         assertThatThrownBy(() -> {
-            Pagination<?> pagination = new Pagination<>(1, 10, Collections.emptyList(), PageRequest.Mode.CURSOR_NEXT, new PageRequestCursor("me", 200));
+            Pagination<?> pagination = new Pagination<>(1, 10, Collections.emptyList(),
+                                                        PageRequest.Mode.CURSOR_NEXT,
+                                                        new PageRequestCursor("me", 200),
+                                                        true);
             pagination.next();
         }).isInstanceOf(UnsupportedOperationException.class)
                 .hasMessage("Not supported for keyset pagination. Instead use afterKeyset or afterKeysetCursor to provide the next keyset values or obtain the nextPageRequest from a CursoredPage.");