From 7dc898ee1dd634003cc45b22c9711113777e4b09 Mon Sep 17 00:00:00 2001 From: Christoph Strobl Date: Thu, 21 Mar 2024 10:51:37 +0100 Subject: [PATCH 1/2] Prepare issue branch. --- pom.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pom.xml b/pom.xml index 8576363d34..362d494894 100644 --- a/pom.xml +++ b/pom.xml @@ -5,7 +5,7 @@ org.springframework.data spring-data-commons - 3.3.0-SNAPSHOT + 3.3.x-3070-SNAPSHOT Spring Data Core Core Spring concepts underpinning every Spring Data module. From b7ada7f14769c36f01b20f26d65fc5305c4380c8 Mon Sep 17 00:00:00 2001 From: Christoph Strobl Date: Fri, 22 Mar 2024 13:14:00 +0100 Subject: [PATCH 2/2] Align Offset Scrolling Position. --- .../ROOT/pages/repositories/scrolling.adoc | 20 +++++++-- .../data/domain/OffsetScrollPosition.java | 26 ++++++++---- .../springframework/data/domain/Pageable.java | 9 +++- .../domain/AbstractPageRequestUnitTests.java | 7 ++++ .../data/domain/PageRequestUnitTests.java | 8 ---- .../data/domain/ScrollPositionUnitTests.java | 41 +++++++++++++++---- .../data/domain/WindowUnitTests.java | 10 ++--- 7 files changed, 87 insertions(+), 34 deletions(-) diff --git a/src/main/antora/modules/ROOT/pages/repositories/scrolling.adoc b/src/main/antora/modules/ROOT/pages/repositories/scrolling.adoc index ac555cfcaa..8071dc91e8 100644 --- a/src/main/antora/modules/ROOT/pages/repositories/scrolling.adoc +++ b/src/main/antora/modules/ROOT/pages/repositories/scrolling.adoc @@ -6,7 +6,7 @@ Scrolling consists of a stable sort, a scroll type (Offset- or Keyset-based scro You can define simple sorting expressions by using property names and define static result limiting using the xref:repositories/query-methods-details.adoc#repositories.limit-query-result[`Top` or `First` keyword] through query derivation. You can concatenate expressions to collect multiple criteria into one expression. -Scroll queries return a `Window` that allows obtaining the scroll position to resume to obtain the next `Window` until your application has consumed the entire query result. +Scroll queries return a `Window` that allows obtaining the elements scroll position which can be used to fetch the next `Window` until your application has consumed the entire query result. Similar to consuming a Java `Iterator>` by obtaining the next batch of results, query result scrolling lets you access the a `ScrollPosition` through `Window.positionAt(...)`. [source,java] @@ -23,12 +23,19 @@ do { } while (!users.isEmpty() && users.hasNext()); ---- +[NOTE] +==== +The `ScrollPosition` identifies the exact position of an element with the entire query result. +Query execution treats the position parameter as _exclusive_, which means results will start _after_ the given position. +`ScrollPosition#offset` and `ScrollPosition#keyset()` as special incarnations of a `ScrollPosition` indicating the start of a scroll operation. +==== + `WindowIterator` provides a utility to simplify scrolling across ``Window``s by removing the need to check for the presence of a next `Window` and applying the `ScrollPosition`. [source,java] ---- WindowIterator users = WindowIterator.of(position -> repository.findFirst10ByLastnameOrderByFirstname("Doe", position)) - .startingAt(OffsetScrollPosition.initial()); + .startingAt(ScrollPosition.offset()); while (users.hasNext()) { User u = users.next(); @@ -56,7 +63,14 @@ WindowIterator users = WindowIterator.of(position -> repository.findFirst1 .startingAt(OffsetScrollPosition.initial()); <1> ---- -<1> Start from the initial offset at position `0`. +<1> Start with no offset to include the element at position `0`. +==== + +[CAUTION] +==== +There is a difference between `ScollPosition.offset()` and `ScollPosition.offset(0L)`. +The former indicates the start of scroll operation, pointing to no specific offset where as the latter identifies the first element (at position `0`) of the result. +Given the _exclusive_ nature of scrolling using `ScollPosition.offset(0)` will skip the first element and translate to an offset of 1. ==== [[repositories.scrolling.keyset]] diff --git a/src/main/java/org/springframework/data/domain/OffsetScrollPosition.java b/src/main/java/org/springframework/data/domain/OffsetScrollPosition.java index 3876c92bff..926afc7d39 100644 --- a/src/main/java/org/springframework/data/domain/OffsetScrollPosition.java +++ b/src/main/java/org/springframework/data/domain/OffsetScrollPosition.java @@ -23,25 +23,28 @@ /** * A {@link ScrollPosition} based on the offsets within query results. + *

+ * An initial {@link OffsetScrollPosition} does not point to a specific element and is different to a the Po * * @author Mark Paluch * @author Oliver Drotbohm + * @author Christoph Strobl * @since 3.1 */ public final class OffsetScrollPosition implements ScrollPosition { - private static final OffsetScrollPosition INITIAL = new OffsetScrollPosition(0); + private static final OffsetScrollPosition INITIAL = new OffsetScrollPosition(null); - private final long offset; + @Nullable private final Long offset; /** * Creates a new {@link OffsetScrollPosition} for the given non-negative offset. * * @param offset must be greater or equal to zero. */ - private OffsetScrollPosition(long offset) { + private OffsetScrollPosition(@Nullable Long offset) { - Assert.isTrue(offset >= 0, "Offset must not be negative"); + Assert.isTrue(offset == null || offset >= 0, "Offset must not be negative"); this.offset = offset; } @@ -62,7 +65,7 @@ static OffsetScrollPosition initial() { * @return will never be {@literal null}. */ static OffsetScrollPosition of(long offset) { - return offset == 0 ? initial() : new OffsetScrollPosition(offset); + return new OffsetScrollPosition(offset); } /** @@ -80,10 +83,15 @@ public static IntFunction positionFunction(long startOffse /** * The zero or positive offset. + *

+ * An {@link #isInitial() initial} position does not define an offset and will raise an error. * * @return the offset. + * @throws IllegalStateException if {@link #isInitial()}. */ public long getOffset() { + + Assert.state(offset != null, "Initial state does not have an offset. Make sure to check #isInitial()"); return offset; } @@ -96,14 +104,14 @@ public long getOffset() { */ public OffsetScrollPosition advanceBy(long delta) { - var value = offset + delta; + var value = isInitial() ? delta : offset + delta; return new OffsetScrollPosition(value < 0 ? 0 : value); } @Override public boolean isInitial() { - return offset == 0; + return offset == null; } @Override @@ -117,7 +125,7 @@ public boolean equals(@Nullable Object o) { return false; } - return offset == that.offset; + return Objects.equals(offset, that.offset); } @Override @@ -141,7 +149,7 @@ public OffsetScrollPosition apply(int offset) { throw new IndexOutOfBoundsException(offset); } - return of(startOffset + offset + 1); + return of(startOffset + offset); } } } diff --git a/src/main/java/org/springframework/data/domain/Pageable.java b/src/main/java/org/springframework/data/domain/Pageable.java index 36916c161c..f84b7822aa 100644 --- a/src/main/java/org/springframework/data/domain/Pageable.java +++ b/src/main/java/org/springframework/data/domain/Pageable.java @@ -191,8 +191,12 @@ default Limit toLimit() { /** * Returns an {@link OffsetScrollPosition} from this pageable if the page request {@link #isPaged() is paged}. + *

+ * Given the exclusive nature of scrolling the {@link ScrollPosition} for {@code Page(0, 10)} translates an + * {@link ScrollPosition#isInitial() initial} position, where as {@code Page(1, 10)} will point to the last element of + * {@code Page(0,10)} resulting in {@link ScrollPosition#offset(long) ScrollPosition(9)}. * - * @return + * @return new instance of {@link OffsetScrollPosition}. * @throws IllegalStateException if the request is {@link #isUnpaged()} * @since 3.1 */ @@ -202,6 +206,7 @@ default OffsetScrollPosition toScrollPosition() { throw new IllegalStateException("Cannot create OffsetScrollPosition from an unpaged instance"); } - return ScrollPosition.offset(getOffset()); + return getOffset() > 0 ? ScrollPosition.offset(getOffset() - 1 /* scrolling is exclusive */) + : ScrollPosition.offset(); } } diff --git a/src/test/java/org/springframework/data/domain/AbstractPageRequestUnitTests.java b/src/test/java/org/springframework/data/domain/AbstractPageRequestUnitTests.java index bf745673aa..6b61561362 100755 --- a/src/test/java/org/springframework/data/domain/AbstractPageRequestUnitTests.java +++ b/src/test/java/org/springframework/data/domain/AbstractPageRequestUnitTests.java @@ -84,4 +84,11 @@ void getOffsetShouldNotCauseOverflow() { assertThat(request.getOffset()).isGreaterThan(Integer.MAX_VALUE); } + + @Test // GH-2151, GH-3070 + void createsOffsetScrollPosition() { + + assertThat(newPageRequest(0, 10).toScrollPosition()).returns(true, ScrollPosition::isInitial); + assertThat(newPageRequest(1, 10).toScrollPosition()).returns(9L, OffsetScrollPosition::getOffset); + } } diff --git a/src/test/java/org/springframework/data/domain/PageRequestUnitTests.java b/src/test/java/org/springframework/data/domain/PageRequestUnitTests.java index bf7101214c..181d51f37e 100755 --- a/src/test/java/org/springframework/data/domain/PageRequestUnitTests.java +++ b/src/test/java/org/springframework/data/domain/PageRequestUnitTests.java @@ -71,12 +71,4 @@ void rejectsNullSort() { assertThatIllegalArgumentException() // .isThrownBy(() -> PageRequest.of(0, 10, null)); } - - @Test // GH-2151 - void createsOffsetScrollPosition() { - - PageRequest request = PageRequest.of(1, 10); - - assertThat(request.toScrollPosition()).isEqualTo(ScrollPosition.offset(10)); - } } diff --git a/src/test/java/org/springframework/data/domain/ScrollPositionUnitTests.java b/src/test/java/org/springframework/data/domain/ScrollPositionUnitTests.java index f3137abae7..baf81f9b21 100644 --- a/src/test/java/org/springframework/data/domain/ScrollPositionUnitTests.java +++ b/src/test/java/org/springframework/data/domain/ScrollPositionUnitTests.java @@ -29,6 +29,7 @@ * * @author Mark Paluch * @author Oliver Drotbohm + * @author Christoph Strobl */ class ScrollPositionUnitTests { @@ -56,14 +57,14 @@ void equalsAndHashCodeForOffsets() { assertThat(foo1).isNotEqualTo(bar).doesNotHaveSameHashCodeAs(bar); } - @Test // GH-2151 + @Test // GH-2151, GH-3070 void shouldCreateCorrectIndexPosition() { - assertThat(positionFunction(0).apply(0)).isEqualTo(ScrollPosition.offset(1)); - assertThat(positionFunction(0).apply(1)).isEqualTo(ScrollPosition.offset(2)); + assertThat(positionFunction(0).apply(0)).isEqualTo(ScrollPosition.offset(0)); + assertThat(positionFunction(0).apply(1)).isEqualTo(ScrollPosition.offset(1)); - assertThat(positionFunction(100).apply(0)).isEqualTo(ScrollPosition.offset(101)); - assertThat(positionFunction(100).apply(1)).isEqualTo(ScrollPosition.offset(102)); + assertThat(positionFunction(100).apply(0)).isEqualTo(ScrollPosition.offset(100)); + assertThat(positionFunction(100).apply(1)).isEqualTo(ScrollPosition.offset(101)); } @Test // GH-2151 @@ -80,6 +81,23 @@ void advanceOffsetBelowZeroCapsAtZero() { assertThat(offset.advanceBy(-10)).isEqualTo(ScrollPosition.offset(0)); } + @Test // GH-3070 + void advanceOffsetForward() { + + OffsetScrollPosition offset = ScrollPosition.offset(5); + + assertThat(offset.getOffset()).isEqualTo(5); + assertThat(offset.advanceBy(5)).isEqualTo(ScrollPosition.offset(10)); + } + + @Test // GH-3070 + void advanceInitialOffsetForward() { + + OffsetScrollPosition offset = ScrollPosition.offset(); + + assertThat(offset.advanceBy(5)).isEqualTo(ScrollPosition.offset(5)); + } + @Test // GH-2824 void setsUpForwardScrolling() { @@ -120,13 +138,22 @@ void setsUpBackwardScrolling() { assertThat(position.reverse()).isEqualTo(forward); } - @Test // GH-2824 + @Test // GH-2824, GH-3070 void initialOffsetPosition() { OffsetScrollPosition position = ScrollPosition.offset(); assertThat(position.isInitial()).isTrue(); - assertThat(position.getOffset()).isEqualTo(0); + assertThatExceptionOfType(IllegalStateException.class).isThrownBy(position::getOffset); + } + + @Test // GH-3070 + void initialOffsetPositionIsNotEqualToPositionOfFirstElement() { + + OffsetScrollPosition first = ScrollPosition.offset(0); + + assertThat(first.isInitial()).isFalse(); + assertThat(first).isNotEqualTo(ScrollPosition.offset()); } @Test // GH-2824, GH-2840 diff --git a/src/test/java/org/springframework/data/domain/WindowUnitTests.java b/src/test/java/org/springframework/data/domain/WindowUnitTests.java index c5bfbdd438..bda23c3a3c 100644 --- a/src/test/java/org/springframework/data/domain/WindowUnitTests.java +++ b/src/test/java/org/springframework/data/domain/WindowUnitTests.java @@ -53,18 +53,18 @@ void allowsIteration() { } } - @Test // GH-2151 + @Test // GH-2151, GH-3070 void shouldCreateCorrectPositions() { Window window = Window.from(List.of(1, 2, 3), OffsetScrollPosition.positionFunction(0)); - assertThat(window.positionAt(0)).isEqualTo(ScrollPosition.offset(1)); - assertThat(window.positionAt(window.size() - 1)).isEqualTo(ScrollPosition.offset(3)); + assertThat(window.positionAt(0)).isEqualTo(ScrollPosition.offset(0)); + assertThat(window.positionAt(window.size() - 1)).isEqualTo(ScrollPosition.offset(2)); // by index - assertThat(window.positionAt(1)).isEqualTo(ScrollPosition.offset(2)); + assertThat(window.positionAt(1)).isEqualTo(ScrollPosition.offset(1)); // by object - assertThat(window.positionAt(Integer.valueOf(1))).isEqualTo(ScrollPosition.offset(1)); + assertThat(window.positionAt(Integer.valueOf(1))).isEqualTo(ScrollPosition.offset(0)); } }