Revise OffsetScrollPosition to use zero-based indexes

pom.xml

@@ -5,7 +5,7 @@
 
 	<groupId>org.springframework.data</groupId>
 	<artifactId>spring-data-commons</artifactId>
-	<version>3.3.0-SNAPSHOT</version>
+	<version>3.3.x-3070-SNAPSHOT</version>
 
 	<name>Spring Data Core</name>
 	<description>Core Spring concepts underpinning every Spring Data module.</description>

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<T>` that allows obtaining the scroll position to resume to obtain the next `Window<T>` until your application has consumed the entire query result.
+Scroll queries return a `Window<T>` that allows obtaining the elements scroll position which can be used to fetch the next `Window<T>` until your application has consumed the entire query result.
 Similar to consuming a Java `Iterator<List<…>>` 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<User> 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<User> 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]]

src/main/java/org/springframework/data/domain/OffsetScrollPosition.java

@@ -23,25 +23,28 @@
 
 /**
  * A {@link ScrollPosition} based on the offsets within query results.
+ * <p>
+ * 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<OffsetScrollPosition> positionFunction(long startOffse
 
 	/**
 	 * The zero or positive offset.
+	 * <p>
+	 * 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);
 		}
 	}
 }

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}.
+	 * <p>
+	 * 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();
 	}
 }

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

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

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

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