Fix backward scroll calculations for offset positions

1. Offset to be advanced back by 1 more than the count in order to exclude the item at the specified offset. 2. Count to be adjusted down if offset is too low. 3. Count to be set to 0 if offset is 0. See gh-840
spring-projects · Oct 19, 2023 · 2a99c1f · 2a99c1f
1 parent 7ddb1a9
commit 2a99c1f
Show file tree

Hide file tree

Showing 10 changed files with 103 additions and 19 deletions.
diff --git a/...ramework/graphql/data/method/annotation/support/ScrollSubrangeMethodArgumentResolver.java b/...ramework/graphql/data/method/annotation/support/ScrollSubrangeMethodArgumentResolver.java
@@ -44,8 +44,9 @@ public boolean supportsParameter(MethodParameter parameter) {
 		return parameter.getParameterType().equals(ScrollSubrange.class);
 	}
 
-	protected ScrollSubrange createSubrange(@Nullable ScrollPosition pos, @Nullable Integer size, boolean forward) {
-		return new ScrollSubrange(pos, size, forward);
+	@Override
+	protected ScrollSubrange createSubrange(@Nullable ScrollPosition pos, @Nullable Integer count, boolean forward) {
+		return ScrollSubrange.create(pos, count, forward);
 	}
 
 }
diff --git a/...pringframework/graphql/data/method/annotation/support/SubrangeMethodArgumentResolver.java b/...pringframework/graphql/data/method/annotation/support/SubrangeMethodArgumentResolver.java
@@ -62,8 +62,8 @@ public Object resolveArgument(MethodParameter parameter, DataFetchingEnvironment
 	/**
 	 * Allows subclasses to create an extension of {@link Subrange}.
 	 */
-	protected Subrange<P> createSubrange(@Nullable P pos, @Nullable Integer size, boolean forward) {
-		return new Subrange<>(pos, size, forward);
+	protected Subrange<P> createSubrange(@Nullable P pos, @Nullable Integer count, boolean forward) {
+		return new Subrange<>(pos, count, forward);
 	}
 
 }
diff --git a/spring-graphql/src/main/java/org/springframework/graphql/data/query/RepositoryUtils.java b/spring-graphql/src/main/java/org/springframework/graphql/data/query/RepositoryUtils.java
@@ -85,7 +85,7 @@ public static CursorStrategy<ScrollPosition> defaultCursorStrategy() {
 	}
 
 	public static ScrollSubrange defaultScrollSubrange() {
-		return new ScrollSubrange(ScrollPosition.offset(), 20, true);
+		return ScrollSubrange.create(ScrollPosition.offset(), 20, true);
 	}
 
 	public static ScrollSubrange buildScrollSubrange(
@@ -96,7 +96,7 @@ public static ScrollSubrange buildScrollSubrange(
 		Integer count = environment.getArgument(forward ? "first" : "last");
 		String cursor = environment.getArgument(forward ? "after" : "before");
 		ScrollPosition position = (cursor != null ? cursorStrategy.fromCursor(cursor) : null);
-		return new ScrollSubrange(position, count, forward);
+		return ScrollSubrange.create(position, count, forward);
 	}
 
 }
diff --git a/spring-graphql/src/main/java/org/springframework/graphql/data/query/ScrollSubrange.java b/spring-graphql/src/main/java/org/springframework/graphql/data/query/ScrollSubrange.java
@@ -39,6 +39,19 @@
 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 */) {
+
+		super(pos, count, forward);
+	}
+
+	/**
+	 * Public constructor.
+	 * @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));
 	}
@@ -56,4 +69,54 @@ else if (pos instanceof KeysetScrollPosition keysetPosition) {
 		return pos;
 	}
 
+
+	/**
+	 * Create a {@link ScrollSubrange} instance.
+	 * @param position the position relative to which to scroll, or {@code null}
+	 * for scrolling from the beginning
+	 * @param count the number of elements requested
+	 * @param forward whether to return elements after (true) or before (false)
+	 * the element at the given position
+	 * @return the created subrange
+	 * @since 1.2.4
+	 */
+	public static ScrollSubrange create(@Nullable ScrollPosition position, @Nullable Integer count, boolean forward) {
+		if (position instanceof OffsetScrollPosition offsetScrollPosition) {
+			return initFromOffsetPosition(offsetScrollPosition, count, forward);
+		}
+		else if (position instanceof KeysetScrollPosition keysetScrollPosition) {
+			return initFromKeysetPosition(keysetScrollPosition, count, forward);
+		}
+		else {
+			return new ScrollSubrange(position, count, forward, null);
+		}
+	}
+
+	private static ScrollSubrange initFromOffsetPosition(
+			OffsetScrollPosition position, @Nullable Integer count, boolean forward) {
+
+		if (!forward) {
+			if (count != null) {
+				if (position.getOffset() == 0) {
+					count = 0;
+				}
+				else if (count >= position.getOffset()) {
+					count = (int) (position.getOffset() - 1);
+				}
+				position = position.advanceBy(-count - 1);
+			}
+			forward = true;
+		}
+		return new ScrollSubrange(position, count, forward, null);
+	}
+
+	private static ScrollSubrange initFromKeysetPosition(
+			KeysetScrollPosition position, @Nullable Integer count, boolean forward) {
+
+		if (!forward) {
+			position = position.backward();
+		}
+		return new ScrollSubrange(position, count, forward, null);
+	}
+
 }
diff --git a/spring-graphql/src/test/java/org/springframework/graphql/data/query/ScrollSubrangeTests.java b/spring-graphql/src/test/java/org/springframework/graphql/data/query/ScrollSubrangeTests.java
@@ -41,13 +41,13 @@ void offset() {
 		ScrollPosition position = ScrollPosition.offset(30);
 		int count = 10;
 
-		ScrollSubrange subrange = new ScrollSubrange(position, count, true);
+		ScrollSubrange subrange = ScrollSubrange.create(position, count, true);
 		assertThat(((OffsetScrollPosition) subrange.position().get())).isEqualTo(position);
 		assertThat(subrange.count().orElse(0)).isEqualTo(count);
 		assertThat(subrange.forward()).isTrue();
 
-		subrange = new ScrollSubrange(position, count, false);
-		assertThat(((OffsetScrollPosition) subrange.position().get()).getOffset()).isEqualTo(20);
+		subrange = ScrollSubrange.create(position, count, false);
+		assertThat(((OffsetScrollPosition) subrange.position().get()).getOffset()).isEqualTo(19);
 		assertThat(subrange.count().orElse(0)).isEqualTo(count);
 		assertThat(subrange.forward()).isTrue();
 	}
@@ -62,14 +62,14 @@ void keyset() {
 		ScrollPosition position = ScrollPosition.forward(keys);
 		int count = 10;
 
-		ScrollSubrange subrange = new ScrollSubrange(position, count, true);
+		ScrollSubrange subrange = ScrollSubrange.create(position, count, true);
 		KeysetScrollPosition actualPosition = (KeysetScrollPosition) subrange.position().get();
 		assertThat(actualPosition.getKeys()).isEqualTo(keys);
 		assertThat(actualPosition.getDirection()).isEqualTo(Direction.FORWARD);
 		assertThat(subrange.count().orElse(0)).isEqualTo(count);
 		assertThat(subrange.forward()).isTrue();
 
-		subrange = new ScrollSubrange(position, count, false);
+		subrange = ScrollSubrange.create(position, count, false);
 		actualPosition = (KeysetScrollPosition) subrange.position().get();
 		assertThat(actualPosition.getKeys()).isEqualTo(keys);
 		assertThat(actualPosition.getDirection()).isEqualTo(Direction.BACKWARD);
@@ -79,17 +79,37 @@ void keyset() {
 
 	@Test
 	void nullInput() {
-		ScrollSubrange subrange = new ScrollSubrange(null, null, true);
+		ScrollSubrange subrange = ScrollSubrange.create(null, null, true);
 
 		assertThat(subrange.position()).isNotPresent();
 		assertThat(subrange.count()).isNotPresent();
 		assertThat(subrange.forward()).isTrue();
 	}
 
 	@Test
-	void offsetBackwardPaginationNullSize() {
+	void offsetBackwardPaginationWithInsufficientCount() {
+		ScrollPosition position = ScrollPosition.offset(5);
+		ScrollSubrange subrange = ScrollSubrange.create(position, 10, false);
+
+		assertThat(((OffsetScrollPosition) subrange.position().get()).getOffset()).isEqualTo(0);
+		assertThat(subrange.count().getAsInt()).isEqualTo(4);
+		assertThat(subrange.forward()).isTrue();
+	}
+
+	@Test
+	void offsetBackwardPaginationWithOffsetZero() {
+		ScrollPosition position = ScrollPosition.offset(0);
+		ScrollSubrange subrange = ScrollSubrange.create(position, 10, false);
+
+		assertThat(((OffsetScrollPosition) subrange.position().get()).getOffset()).isEqualTo(0);
+		assertThat(subrange.count().getAsInt()).isEqualTo(0);
+		assertThat(subrange.forward()).isTrue();
+	}
+
+	@Test
+	void offsetBackwardPaginationWithNullCount() {
 		ScrollPosition position = ScrollPosition.offset(30);
-		ScrollSubrange subrange = new ScrollSubrange(position, null, false);
+		ScrollSubrange subrange = ScrollSubrange.create(position, null, false);
 
 		assertThat(((OffsetScrollPosition) subrange.position().get())).isEqualTo(position);
 		assertThat(subrange.count()).isNotPresent();

diff --git a/...st/java/org/springframework/graphql/data/query/jpa/QueryByExampleDataFetcherJpaTests.java b/...st/java/org/springframework/graphql/data/query/jpa/QueryByExampleDataFetcherJpaTests.java
@@ -256,7 +256,7 @@ private static RuntimeWiringConfigurer createRuntimeWiringConfigurer(QueryByExam
 				executor != null ? Collections.singletonList(executor) : Collections.emptyList(),
 				Collections.emptyList(),
 				new ScrollPositionCursorStrategy(),
-				new ScrollSubrange(ScrollPosition.offset(), 10, true));
+				ScrollSubrange.create(ScrollPosition.offset(), 10, true));
 	}
 
 	private WebGraphQlRequest request(String query) {

diff --git a/...a/org/springframework/graphql/data/query/mongo/QueryByExampleDataFetcherMongoDbTests.java b/...a/org/springframework/graphql/data/query/mongo/QueryByExampleDataFetcherMongoDbTests.java
@@ -236,7 +236,7 @@ private static RuntimeWiringConfigurer createRuntimeWiringConfigurer(QueryByExam
 				(executor != null ? Collections.singletonList(executor) : Collections.emptyList()),
 				Collections.emptyList(),
 				new ScrollPositionCursorStrategy(),
-				new ScrollSubrange(ScrollPosition.offset(), 10, true));
+				ScrollSubrange.create(ScrollPosition.offset(), 10, true));
 	}
 
 	private WebGraphQlRequest request(String query) {

diff --git a/...ringframework/graphql/data/query/mongo/QueryByExampleDataFetcherReactiveMongoDbTests.java b/...ringframework/graphql/data/query/mongo/QueryByExampleDataFetcherReactiveMongoDbTests.java
@@ -207,7 +207,7 @@ private static RuntimeWiringConfigurer createRuntimeWiringConfigurer(ReactiveQue
 				Collections.emptyList(),
 				(executor != null ? Collections.singletonList(executor) : Collections.emptyList()),
 				new ScrollPositionCursorStrategy(),
-				new ScrollSubrange(ScrollPosition.offset(), 10, true));
+				ScrollSubrange.create(ScrollPosition.offset(), 10, true));
 	}
 
 	private WebGraphQlRequest request(String query) {

diff --git a/...ava/org/springframework/graphql/data/query/neo4j/QueryByExampleDataFetcherNeo4jTests.java b/...ava/org/springframework/graphql/data/query/neo4j/QueryByExampleDataFetcherNeo4jTests.java
@@ -235,7 +235,7 @@ private static RuntimeWiringConfigurer createRuntimeWiringConfigurer(QueryByExam
 				(executor != null ? Collections.singletonList(executor) : Collections.emptyList()),
 				Collections.emptyList(),
 				new ScrollPositionCursorStrategy(),
-				new ScrollSubrange(ScrollPosition.offset(), 10, true));
+				ScrollSubrange.create(ScrollPosition.offset(), 10, true));
 	}
 
 	private WebGraphQlRequest request(String query) {

diff --git a/...ringframework/graphql/data/query/neo4j/QueryByExampleDataFetcherReactiveNeo4jDbTests.java b/...ringframework/graphql/data/query/neo4j/QueryByExampleDataFetcherReactiveNeo4jDbTests.java
@@ -219,7 +219,7 @@ private static RuntimeWiringConfigurer createRuntimeWiringConfigurer(ReactiveQue
 				Collections.emptyList(),
 				(executor != null ? Collections.singletonList(executor) : Collections.emptyList()),
 				new ScrollPositionCursorStrategy(),
-				new ScrollSubrange(ScrollPosition.offset(), 10, true));
+				ScrollSubrange.create(ScrollPosition.offset(), 10, true));
 	}
 
 	private WebGraphQlRequest request(String query) {