From 2a99c1ffa0251b76c95f242a181acb638be102fb Mon Sep 17 00:00:00 2001 From: rstoyanchev Date: Thu, 19 Oct 2023 12:17:05 +0100 Subject: [PATCH] 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 --- .../ScrollSubrangeMethodArgumentResolver.java | 5 +- .../SubrangeMethodArgumentResolver.java | 4 +- .../graphql/data/query/RepositoryUtils.java | 4 +- .../graphql/data/query/ScrollSubrange.java | 63 +++++++++++++++++++ .../data/query/ScrollSubrangeTests.java | 36 ++++++++--- .../QueryByExampleDataFetcherJpaTests.java | 2 +- ...QueryByExampleDataFetcherMongoDbTests.java | 2 +- ...xampleDataFetcherReactiveMongoDbTests.java | 2 +- .../QueryByExampleDataFetcherNeo4jTests.java | 2 +- ...xampleDataFetcherReactiveNeo4jDbTests.java | 2 +- 10 files changed, 103 insertions(+), 19 deletions(-) diff --git a/spring-graphql/src/main/java/org/springframework/graphql/data/method/annotation/support/ScrollSubrangeMethodArgumentResolver.java b/spring-graphql/src/main/java/org/springframework/graphql/data/method/annotation/support/ScrollSubrangeMethodArgumentResolver.java index 2dd72a5c..fbc259b7 100644 --- a/spring-graphql/src/main/java/org/springframework/graphql/data/method/annotation/support/ScrollSubrangeMethodArgumentResolver.java +++ b/spring-graphql/src/main/java/org/springframework/graphql/data/method/annotation/support/ScrollSubrangeMethodArgumentResolver.java @@ -44,8 +44,9 @@ public class ScrollSubrangeMethodArgumentResolver extends SubrangeMethodArgument 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/spring-graphql/src/main/java/org/springframework/graphql/data/method/annotation/support/SubrangeMethodArgumentResolver.java b/spring-graphql/src/main/java/org/springframework/graphql/data/method/annotation/support/SubrangeMethodArgumentResolver.java index 20234063..2bb37b93 100644 --- a/spring-graphql/src/main/java/org/springframework/graphql/data/method/annotation/support/SubrangeMethodArgumentResolver.java +++ b/spring-graphql/src/main/java/org/springframework/graphql/data/method/annotation/support/SubrangeMethodArgumentResolver.java @@ -62,8 +62,8 @@ public class SubrangeMethodArgumentResolver

implements HandlerMethodArgumentR /** * Allows subclasses to create an extension of {@link Subrange}. */ - protected Subrange

createSubrange(@Nullable P pos, @Nullable Integer size, boolean forward) { - return new Subrange<>(pos, size, forward); + protected Subrange

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 index 06046811..612b7109 100644 --- 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 @@ class RepositoryUtils { } 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 @@ class RepositoryUtils { 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 index 7d89d4ee..79c89b2e 100644 --- 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 @@ import org.springframework.lang.Nullable; public final class ScrollSubrange extends Subrange { + @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 @@ public final class ScrollSubrange extends Subrange { 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 index 81aa01fe..a829751d 100644 --- 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 @@ public class ScrollSubrangeTests { 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 @@ public class ScrollSubrangeTests { 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,7 +79,7 @@ public class ScrollSubrangeTests { @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(); @@ -87,9 +87,29 @@ public class ScrollSubrangeTests { } @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/spring-graphql/src/test/java/org/springframework/graphql/data/query/jpa/QueryByExampleDataFetcherJpaTests.java b/spring-graphql/src/test/java/org/springframework/graphql/data/query/jpa/QueryByExampleDataFetcherJpaTests.java index 64353dd0..1820978a 100644 --- a/spring-graphql/src/test/java/org/springframework/graphql/data/query/jpa/QueryByExampleDataFetcherJpaTests.java +++ b/spring-graphql/src/test/java/org/springframework/graphql/data/query/jpa/QueryByExampleDataFetcherJpaTests.java @@ -256,7 +256,7 @@ class QueryByExampleDataFetcherJpaTests { 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/spring-graphql/src/test/java/org/springframework/graphql/data/query/mongo/QueryByExampleDataFetcherMongoDbTests.java b/spring-graphql/src/test/java/org/springframework/graphql/data/query/mongo/QueryByExampleDataFetcherMongoDbTests.java index 1829aede..8fcb2242 100644 --- a/spring-graphql/src/test/java/org/springframework/graphql/data/query/mongo/QueryByExampleDataFetcherMongoDbTests.java +++ b/spring-graphql/src/test/java/org/springframework/graphql/data/query/mongo/QueryByExampleDataFetcherMongoDbTests.java @@ -236,7 +236,7 @@ class QueryByExampleDataFetcherMongoDbTests { (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/spring-graphql/src/test/java/org/springframework/graphql/data/query/mongo/QueryByExampleDataFetcherReactiveMongoDbTests.java b/spring-graphql/src/test/java/org/springframework/graphql/data/query/mongo/QueryByExampleDataFetcherReactiveMongoDbTests.java index 2dc8c65c..d5858345 100644 --- a/spring-graphql/src/test/java/org/springframework/graphql/data/query/mongo/QueryByExampleDataFetcherReactiveMongoDbTests.java +++ b/spring-graphql/src/test/java/org/springframework/graphql/data/query/mongo/QueryByExampleDataFetcherReactiveMongoDbTests.java @@ -207,7 +207,7 @@ class QueryByExampleDataFetcherReactiveMongoDbTests { 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/spring-graphql/src/test/java/org/springframework/graphql/data/query/neo4j/QueryByExampleDataFetcherNeo4jTests.java b/spring-graphql/src/test/java/org/springframework/graphql/data/query/neo4j/QueryByExampleDataFetcherNeo4jTests.java index 10eefad3..ade94445 100644 --- a/spring-graphql/src/test/java/org/springframework/graphql/data/query/neo4j/QueryByExampleDataFetcherNeo4jTests.java +++ b/spring-graphql/src/test/java/org/springframework/graphql/data/query/neo4j/QueryByExampleDataFetcherNeo4jTests.java @@ -235,7 +235,7 @@ class QueryByExampleDataFetcherNeo4jTests { (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/spring-graphql/src/test/java/org/springframework/graphql/data/query/neo4j/QueryByExampleDataFetcherReactiveNeo4jDbTests.java b/spring-graphql/src/test/java/org/springframework/graphql/data/query/neo4j/QueryByExampleDataFetcherReactiveNeo4jDbTests.java index 0de50aa1..5a9795c3 100644 --- a/spring-graphql/src/test/java/org/springframework/graphql/data/query/neo4j/QueryByExampleDataFetcherReactiveNeo4jDbTests.java +++ b/spring-graphql/src/test/java/org/springframework/graphql/data/query/neo4j/QueryByExampleDataFetcherReactiveNeo4jDbTests.java @@ -219,7 +219,7 @@ class QueryByExampleDataFetcherReactiveNeo4jDbTests { 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) {