Align offset scrolling with Spring Data 2024.0

Closes gh-946

Author: rstoyanchev

platform/build.gradle

@@ -12,7 +12,7 @@ dependencies {
 	api(platform("io.projectreactor:reactor-bom:2023.0.3"))
 	api(platform("io.micrometer:micrometer-bom:1.13.0-M1"))
 	api(platform("io.micrometer:micrometer-tracing-bom:1.3.0-M1"))
-	api(platform("org.springframework.data:spring-data-bom:2023.1.3"))
+	api(platform("org.springframework.data:spring-data-bom:2024.0.0-SNAPSHOT"))
 	api(platform("org.springframework.security:spring-security-bom:6.3.0-M2"))
 	api(platform("com.querydsl:querydsl-bom:5.0.0"))
 	api(platform("io.rsocket:rsocket-bom:1.1.4"))

spring-graphql/build.gradle

@@ -21,7 +21,7 @@ dependencies {
 	compileOnly 'org.springframework.security:spring-security-core'
 
 	compileOnly 'com.querydsl:querydsl-core'
-	compileOnly 'org.springframework.data:spring-data-commons:3.1.0-SNAPSHOT'
+	compileOnly 'org.springframework.data:spring-data-commons'
 
 	compileOnly 'io.rsocket:rsocket-core'
 	compileOnly 'io.rsocket:rsocket-transport-netty'
@@ -51,14 +51,14 @@ dependencies {
 	testImplementation 'org.springframework:spring-websocket'
 	testImplementation 'org.springframework.data:spring-data-commons'
 	testImplementation 'org.springframework.data:spring-data-keyvalue'
-	testImplementation 'org.springframework.data:spring-data-jpa:3.1.0-SNAPSHOT'
+	testImplementation 'org.springframework.data:spring-data-jpa'
 	testImplementation 'io.micrometer:micrometer-observation-test'
 	testImplementation 'io.micrometer:micrometer-tracing-test'
 	testImplementation 'com.h2database:h2'
 	testImplementation 'org.hibernate:hibernate-core'
 	testImplementation 'org.hibernate.validator:hibernate-validator'
 	testImplementation 'org.springframework.data:spring-data-mongodb'
-	testImplementation 'org.springframework.data:spring-data-neo4j:7.1.1'
+	testImplementation 'org.springframework.data:spring-data-neo4j'
 	testImplementation 'org.mongodb:mongodb-driver-sync'
 	testImplementation 'org.mongodb:mongodb-driver-reactivestreams'
 	testImplementation 'org.testcontainers:mongodb'

spring-graphql/src/main/java/org/springframework/graphql/data/query/ScrollSubrange.java

@@ -24,13 +24,7 @@
 import org.springframework.lang.Nullable;
 
 /**
- * Container for parameters that limit result elements to a subrange including a
- * relative {@link ScrollPosition}, number of elements, and direction.
- *
- * <p> For backward pagination, the offset of an {@link OffsetScrollPosition}
- * is adjusted to point to the first item in the range by subtracting the count
- * from it. Hence, for {@code OffsetScrollPosition} {@link #forward()} is
- * always {@code true}.
+ * {@link Subrange} implementation for a {@link ScrollPosition} cursor.
  *
  * @author Rossen Stoyanchev
  * @author Oliver Drotbohm
@@ -40,45 +34,16 @@ public final class ScrollSubrange extends Subrange<ScrollPosition> {
 
 
 	@SuppressWarnings("unused")
-	private ScrollSubrange(
-			@Nullable ScrollPosition pos, @Nullable Integer count, boolean forward,
-			@Nullable Object unused /* temporarily to differentiate from deprecated constructor */) {
-
+	private ScrollSubrange(@Nullable ScrollPosition pos, @Nullable Integer count, boolean forward) {
 		super(pos, count, forward);
 	}
 
-	/**
-	 * Public constructor.
-	 * @param pos the reference position, or {@code null} if not specified
-	 * @param count how many to return, or {@code null} if not specified
-	 * @param forward whether scroll forward (true) or backward (false)
-	 * @deprecated in favor of {@link #create}, to be removed in 1.3.
-	 */
-	@Deprecated(since = "1.2.4", forRemoval = true)
-	public ScrollSubrange(@Nullable ScrollPosition pos, @Nullable Integer count, boolean forward) {
-		super(initPosition(pos, count, forward), count, (pos instanceof OffsetScrollPosition || forward));
-	}
-
-	@Nullable
-	private static ScrollPosition initPosition(@Nullable ScrollPosition pos, @Nullable Integer count, boolean forward) {
-		if (!forward) {
-			if (pos instanceof OffsetScrollPosition offsetPosition && count != null) {
-				return offsetPosition.advanceBy(-count);
-			}
-			else if (pos instanceof KeysetScrollPosition keysetPosition) {
-				pos = keysetPosition.backward();
-			}
-		}
-		return pos;
-	}
-
 
 	/**
 	 * Create a {@link ScrollSubrange} from the given inputs.
-	 * <p>Pagination with offset-based scrolling is always forward and inclusive
-	 * of the referenced item. Therefore, an {@link OffsetScrollPosition} is
-	 * adjusted as follows. For forward pagination, advanced by 1. For backward
-	 * pagination, advanced back by the count, and switched to forward.
+	 * <p>Offset scrolling is always forward and exclusive of the referenced item.
+	 * Therefore, for backward pagination, the offset is advanced back by the
+	 * count + 1, and the direction is switched to forward.
 	 * @param position the reference position, or {@code null} if not specified
 	 * @param count how many to return, or {@code null} if not specified
 	 * @param forward whether scroll forward (true) or backward (false)
@@ -98,23 +63,24 @@ else if (position instanceof KeysetScrollPosition keysetScrollPosition) {
 			return initFromKeysetPosition(keysetScrollPosition, count, forward);
 		}
 		else {
-			return new ScrollSubrange(position, count, forward, null);
+			return new ScrollSubrange(position, count, forward);
 		}
 	}
 
 	private static ScrollSubrange initFromOffsetPosition(
 			OffsetScrollPosition position, @Nullable Integer count, boolean forward) {
 
-		// Offset is inclusive, adapt to exclusive:
-		//  - for forward, add 1 to return items after position
-		//  - for backward, subtract count to get items before position
+		// Offset is exclusive:
+		//  - for forward, nothing to do
+		//  - for backward, subtract (count + 1) to get items before position
 
-		if (forward) {
-			position = position.advanceBy(1);
-		}
-		else {
+		if (!forward) {
 			// Advance back by 1 at least to item before position
-			int advanceCount = (count != null) ? count : 1;
+			int advanceCount = ((count != null) ? count : 1);
+
+			// Add 1 more to exclude item at reference position
+			advanceCount++;
+
 			if (position.getOffset() >= advanceCount) {
 				position = position.advanceBy(-advanceCount);
 			}
@@ -124,7 +90,7 @@ private static ScrollSubrange initFromOffsetPosition(
 			}
 		}
 
-		return new ScrollSubrange(position, count, true, null);
+		return new ScrollSubrange(position, count, true);
 	}
 
 	private static ScrollSubrange initFromKeysetPosition(
@@ -133,7 +99,7 @@ private static ScrollSubrange initFromKeysetPosition(
 		if (!forward) {
 			position = position.backward();
 		}
-		return new ScrollSubrange(position, count, forward, null);
+		return new ScrollSubrange(position, count, forward);
 	}
 
 }

spring-graphql/src/main/java/org/springframework/graphql/data/query/WindowConnectionAdapter.java

@@ -35,9 +35,6 @@
 public final class WindowConnectionAdapter
 		extends ConnectionAdapterSupport<ScrollPosition> implements ConnectionAdapter {
 
-	private static final long ZERO_OFFSET_ADJUSTMENT =
-			OffsetScrollPosition.positionFunction(0).apply(0).getOffset();
-
 
 	public WindowConnectionAdapter(CursorStrategy<ScrollPosition> strategy) {
 		super(strategy);
@@ -59,10 +56,13 @@ public <T> Collection<T> getContent(Object container) {
 	public boolean hasPrevious(Object container) {
 		Window<?> window = window(container);
 		if (!window.isEmpty()) {
-			ScrollPosition position = positionAt(window, 0);
+			ScrollPosition position = window.positionAt(0);
 			if (position instanceof KeysetScrollPosition keysetPosition) {
 				return (keysetPosition.scrollsBackward() && window.hasNext());
 			}
+			else if (position instanceof OffsetScrollPosition offsetPosition) {
+				return (offsetPosition.getOffset() != 0);
+			}
 			else {
 				return !position.isInitial();
 			}
@@ -74,7 +74,7 @@ public boolean hasPrevious(Object container) {
 	public boolean hasNext(Object container) {
 		Window<?> window = window(container);
 		if (!window.isEmpty()) {
-			ScrollPosition pos = positionAt(window, 0);
+			ScrollPosition pos = window.positionAt(0);
 			if (pos instanceof KeysetScrollPosition keysetPos) {
 				return (keysetPos.scrollsForward() && window.hasNext());
 			}
@@ -87,22 +87,10 @@ public boolean hasNext(Object container) {
 
 	@Override
 	public String cursorAt(Object container, int index) {
-		ScrollPosition position = positionAt(window(container), index);
+		ScrollPosition position = window(container).positionAt(index);
 		return getCursorStrategy().toCursor(position);
 	}
 
-	private ScrollPosition positionAt(Window<?> window, int index) {
-		ScrollPosition position = window.positionAt(index);
-
-		// Workaround for OffsetScrollPosition#positionFunction adding 1 to the actual offset:
-		// See https://github.com/spring-projects/spring-data-commons/issues/3070
-		if (ZERO_OFFSET_ADJUSTMENT > 0 && position instanceof OffsetScrollPosition offsetPos) {
-			position = offsetPos.advanceBy(-ZERO_OFFSET_ADJUSTMENT);
-		}
-
-		return position;
-	}
-
 	@SuppressWarnings("unchecked")
 	private <T> Window<T> window(Object container) {
 		return (Window<T>) container;

spring-graphql/src/test/java/org/springframework/graphql/data/method/annotation/support/SchemaMappingPaginationTests.java

@@ -112,8 +112,8 @@ private static class BookController {
 		@QueryMapping
 		public Window<Book> books(ScrollSubrange subrange) {
 			int offset = (int) ((OffsetScrollPosition) subrange.position().orElse(ScrollPosition.offset())).getOffset();
-			int count = subrange.count().orElse(5);
-			List<Book> books = BookSource.books().subList(offset, offset + count);
+			offset++; // data stores treat offset as exclusive
+			List<Book> books = BookSource.books().subList(offset, offset + subrange.count().orElse(5));
 			return Window.from(books, OffsetScrollPosition.positionFunction(offset));
 		}
 

spring-graphql/src/test/java/org/springframework/graphql/data/query/RepositoryUtilsTests.java

@@ -22,11 +22,11 @@
 import graphql.schema.DataFetchingEnvironment;
 import graphql.schema.DataFetchingEnvironmentImpl;
 import org.junit.jupiter.api.Test;
-import org.testcontainers.shaded.org.checkerframework.checker.nullness.qual.Nullable;
 
 import org.springframework.data.domain.OffsetScrollPosition;
 import org.springframework.data.domain.ScrollPosition;
 import org.springframework.graphql.data.pagination.CursorStrategy;
+import org.springframework.lang.Nullable;
 
 import static org.assertj.core.api.Assertions.assertThat;
 
@@ -46,7 +46,7 @@ void forward() {
 		DataFetchingEnvironment env = environment(Map.of("first", count, "after", cursorStrategy.toCursor(pos)));
 		ScrollSubrange range = RepositoryUtils.getScrollSubrange(env, cursorStrategy);
 
-		assertSubrange(true, count, pos.advanceBy(1), range);
+		assertSubrange(true, count, pos, range);
 	}
 
 	@Test
@@ -56,7 +56,7 @@ void backward() {
 		DataFetchingEnvironment env = environment(Map.of("last", count, "before", cursorStrategy.toCursor(pos)));
 		ScrollSubrange range = RepositoryUtils.getScrollSubrange(env, cursorStrategy);
 
-		assertSubrange(true, count, ScrollPosition.offset(5), range);
+		assertSubrange(true, count, ScrollPosition.offset(4), range);
 	}
 
 	@Test
@@ -74,7 +74,7 @@ void forwardWithPositionOnly() {
 		DataFetchingEnvironment env = environment(Map.of("after", cursorStrategy.toCursor(pos)));
 		ScrollSubrange range = RepositoryUtils.getScrollSubrange(env, cursorStrategy);
 
-		assertSubrange(true, null, pos.advanceBy(1), range);
+		assertSubrange(true, null, pos, range);
 	}
 
 	@Test
@@ -92,7 +92,7 @@ void backwardWithPositionOnly() {
 		DataFetchingEnvironment env = environment(Map.of("before", cursorStrategy.toCursor(pos)));
 		ScrollSubrange range = RepositoryUtils.getScrollSubrange(env, cursorStrategy);
 
-		assertSubrange(true, null, pos.advanceBy(-1), range);
+		assertSubrange(true, null, pos.advanceBy(-2), range);
 	}
 
 	@Test

spring-graphql/src/test/java/org/springframework/graphql/data/query/ScrollSubrangeTests.java

@@ -41,7 +41,7 @@ void offsetForward() {
 		int count = 10;
 		ScrollSubrange subrange = ScrollSubrange.create(ScrollPosition.offset(30), count, true);
 
-		assertThat(getOffset(subrange)).isEqualTo(31);
+		assertThat(getOffset(subrange)).isEqualTo(30);
 		assertThat(subrange.count().orElse(0)).isEqualTo(count);
 		assertThat(subrange.forward()).isTrue();
 	}
@@ -51,7 +51,7 @@ void offsetBackward() {
 		int count = 10;
 		ScrollSubrange subrange = ScrollSubrange.create(ScrollPosition.offset(30), count, false);
 
-		assertThat(getOffset(subrange)).isEqualTo(20);
+		assertThat(getOffset(subrange)).isEqualTo(19);
 		assertThat(subrange.count().orElse(0)).isEqualTo(count);
 		assertThat(subrange.forward()).isTrue();
 	}
@@ -103,7 +103,7 @@ void nullInput() {
 	void offsetBackwardWithInsufficientCount() {
 		ScrollSubrange subrange = ScrollSubrange.create(ScrollPosition.offset(5), 10, false);
 
-		assertThat(getOffset(subrange)).isEqualTo(0);
+		assertThat(subrange.position().get().isInitial()).isTrue();
 		assertThat(subrange.count().getAsInt()).isEqualTo(5);
 		assertThat(subrange.forward()).isTrue();
 	}
@@ -112,7 +112,7 @@ void offsetBackwardWithInsufficientCount() {
 	void offsetBackwardFromInitialOffset() {
 		ScrollSubrange subrange = ScrollSubrange.create(ScrollPosition.offset(0), 10, false);
 
-		assertThat(getOffset(subrange)).isEqualTo(0);
+		assertThat(subrange.position().get().isInitial()).isTrue();
 		assertThat(subrange.count().getAsInt()).isEqualTo(0);
 		assertThat(subrange.forward()).isTrue();
 	}
@@ -121,7 +121,7 @@ void offsetBackwardFromInitialOffset() {
 	void offsetBackwardWithNullCount() {
 		ScrollSubrange subrange = ScrollSubrange.create(ScrollPosition.offset(30), null, false);
 
-		assertThat(getOffset(subrange)).isEqualTo(29);
+		assertThat(getOffset(subrange)).isEqualTo(28);
 		assertThat(subrange.count()).isNotPresent();
 		assertThat(subrange.forward()).isTrue();
 	}