From 98172306387326881ed58bf410d8e076d76b3fec Mon Sep 17 00:00:00 2001 From: rstoyanchev Date: Tue, 20 Feb 2024 21:13:37 +0000 Subject: [PATCH] Adapt offset scroll from inclusive to exclusive Both forward and backward scrolling needed adjustment. For forward, we were not advancing by 1. For backward, we were advancing by the count and 1 more than necessary. Closes gh-916 --- .../graphql/data/query/ScrollSubrange.java | 38 +++++++---- .../support/SchemaMappingPaginationTests.java | 4 +- .../data/query/QuerydslDataFetcherTests.java | 8 +-- .../data/query/RepositoryUtilsTests.java | 8 +-- .../data/query/ScrollSubrangeTests.java | 68 +++++++++++-------- .../QueryByExampleDataFetcherJpaTests.java | 2 +- ...QueryByExampleDataFetcherMongoDbTests.java | 2 +- ...xampleDataFetcherReactiveMongoDbTests.java | 2 +- .../QueryByExampleDataFetcherNeo4jTests.java | 2 +- ...xampleDataFetcherReactiveNeo4jDbTests.java | 2 +- 10 files changed, 81 insertions(+), 55 deletions(-) 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 3cd347d3..4aefdcf6 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 @@ -71,16 +71,20 @@ public final class ScrollSubrange extends Subrange { /** - * 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 + * Create a {@link ScrollSubrange} from the given inputs. + *

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. + * @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) + * @return the created instance * @since 1.2.4 */ - public static ScrollSubrange create(@Nullable ScrollPosition position, @Nullable Integer count, boolean forward) { + public static ScrollSubrange create( + @Nullable ScrollPosition position, @Nullable Integer count, boolean forward) { + if (count != null && count < 0) { count = null; } @@ -98,16 +102,24 @@ public final class ScrollSubrange extends Subrange { private static ScrollSubrange initFromOffsetPosition( OffsetScrollPosition position, @Nullable Integer count, boolean forward) { - if (!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 + + if (forward) { + position = position.advanceBy(1); + } + else { int countOrZero = (count != null ? count : 0); - if (countOrZero < position.getOffset()) { - position = position.advanceBy(-countOrZero-1); + if (position.getOffset() >= countOrZero) { + position = position.advanceBy(-countOrZero); } else { - count = (position.getOffset() > 0 ? (int) (position.getOffset() - 1) : 0); - position = null; + count = (int) position.getOffset(); + position = ScrollPosition.offset(); } } + return new ScrollSubrange(position, count, true, null); } diff --git a/spring-graphql/src/test/java/org/springframework/graphql/data/method/annotation/support/SchemaMappingPaginationTests.java b/spring-graphql/src/test/java/org/springframework/graphql/data/method/annotation/support/SchemaMappingPaginationTests.java index 8008091a..04e2f097 100644 --- a/spring-graphql/src/test/java/org/springframework/graphql/data/method/annotation/support/SchemaMappingPaginationTests.java +++ b/spring-graphql/src/test/java/org/springframework/graphql/data/method/annotation/support/SchemaMappingPaginationTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2023 the original author or authors. + * Copyright 2002-2024 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -49,7 +49,7 @@ public class SchemaMappingPaginationTests { @Test void forwardPagination() { - String document = BookSource.booksConnectionQuery("first:2, after:\"O_3\""); + String document = BookSource.booksConnectionQuery("first:2, after:\"O_2\""); Mono response = graphQlService().execute(document); ResponseHelper.forResponse(response).assertData( diff --git a/spring-graphql/src/test/java/org/springframework/graphql/data/query/QuerydslDataFetcherTests.java b/spring-graphql/src/test/java/org/springframework/graphql/data/query/QuerydslDataFetcherTests.java index b8aa82af..0eb54c67 100644 --- a/spring-graphql/src/test/java/org/springframework/graphql/data/query/QuerydslDataFetcherTests.java +++ b/spring-graphql/src/test/java/org/springframework/graphql/data/query/QuerydslDataFetcherTests.java @@ -139,12 +139,12 @@ class QuerydslDataFetcherTests { ResponseHelper.forResponse(response).assertData( "{\"books\":{" + "\"edges\":[" + - "{\"cursor\":\"O_4\",\"node\":{\"id\":\"42\",\"name\":\"Hitchhiker's Guide to the Galaxy\"}}," + - "{\"cursor\":\"O_5\",\"node\":{\"id\":\"53\",\"name\":\"Breaking Bad\"}}" + + "{\"cursor\":\"O_0\",\"node\":{\"id\":\"42\",\"name\":\"Hitchhiker's Guide to the Galaxy\"}}," + + "{\"cursor\":\"O_1\",\"node\":{\"id\":\"53\",\"name\":\"Breaking Bad\"}}" + "]," + "\"pageInfo\":{" + - "\"startCursor\":\"O_4\"," + - "\"endCursor\":\"O_5\"," + + "\"startCursor\":\"O_0\"," + + "\"endCursor\":\"O_1\"," + "\"hasPreviousPage\":true," + "\"hasNextPage\":false" + "}}}" diff --git a/spring-graphql/src/test/java/org/springframework/graphql/data/query/RepositoryUtilsTests.java b/spring-graphql/src/test/java/org/springframework/graphql/data/query/RepositoryUtilsTests.java index 0a523e3b..11a8acc4 100644 --- a/spring-graphql/src/test/java/org/springframework/graphql/data/query/RepositoryUtilsTests.java +++ b/spring-graphql/src/test/java/org/springframework/graphql/data/query/RepositoryUtilsTests.java @@ -50,7 +50,7 @@ public class RepositoryUtilsTests { ScrollSubrange range = RepositoryUtils.getScrollSubrange(env, cursorStrategy, defaultSubrange); - assertThat(range.position().get()).isEqualTo(offset); + assertThat(range.position().get()).isEqualTo(ScrollPosition.offset(11)); assertThat(range.count().getAsInt()).isEqualTo(count); assertThat(range.forward()).isTrue(); } @@ -65,7 +65,7 @@ public class RepositoryUtilsTests { ScrollSubrange range = RepositoryUtils.getScrollSubrange(env, cursorStrategy, defaultSubrange); - assertThat(range.position().get()).isEqualTo(offset.advanceBy(-count-1)); + assertThat(range.position().get()).isEqualTo(ScrollPosition.offset(5)); assertThat(range.count().getAsInt()).isEqualTo(count); assertThat(range.forward()).isTrue(); } @@ -75,7 +75,7 @@ public class RepositoryUtilsTests { DataFetchingEnvironment env = environment(Collections.emptyMap()); ScrollSubrange range = RepositoryUtils.getScrollSubrange(env, cursorStrategy, defaultSubrange); - assertThat(range.position().get()).isEqualTo(getDefaultPosition()); + assertThat(range.position().get()).isEqualTo(getDefaultPosition().advanceBy(1)); assertThat(range.count().getAsInt()).isEqualTo(this.defaultSubrange.count().getAsInt()); assertThat(range.forward()).isTrue(); } @@ -86,7 +86,7 @@ public class RepositoryUtilsTests { DataFetchingEnvironment env = environment(Map.of("last", count)); ScrollSubrange range = RepositoryUtils.getScrollSubrange(env, cursorStrategy, defaultSubrange); - assertThat(range.position().get()).isEqualTo(getDefaultPosition().advanceBy(-count-1)); + assertThat(range.position().get()).isEqualTo(getDefaultPosition().advanceBy(-count)); assertThat(range.count().getAsInt()).isEqualTo(count); assertThat(range.forward()).isTrue(); } 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 0776e8ac..c8babeea 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 @@ -37,40 +37,53 @@ import static org.assertj.core.api.Assertions.assertThat; public class ScrollSubrangeTests { @Test - void offset() { - ScrollPosition position = ScrollPosition.offset(30); + void offsetForward() { int count = 10; + ScrollSubrange subrange = ScrollSubrange.create(ScrollPosition.offset(30), 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 = ScrollSubrange.create(position, count, false); - assertThat(((OffsetScrollPosition) subrange.position().get()).getOffset()).isEqualTo(19); + assertThat(getOffset(subrange)).isEqualTo(31); assertThat(subrange.count().orElse(0)).isEqualTo(count); assertThat(subrange.forward()).isTrue(); } @Test - void keyset() { + void offsetBackward() { + int count = 10; + ScrollSubrange subrange = ScrollSubrange.create(ScrollPosition.offset(30), count, false); + + assertThat(getOffset(subrange)).isEqualTo(20); + assertThat(subrange.count().orElse(0)).isEqualTo(count); + assertThat(subrange.forward()).isTrue(); + } + + @Test + void keysetForward() { Map keys = new LinkedHashMap<>(); keys.put("firstName", "Joseph"); keys.put("lastName", "Heller"); keys.put("id", 103); - ScrollPosition position = ScrollPosition.forward(keys); int count = 10; + ScrollSubrange subrange = ScrollSubrange.create(ScrollPosition.forward(keys), 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 = ScrollSubrange.create(position, count, false); - actualPosition = (KeysetScrollPosition) subrange.position().get(); + @Test + void keysetBackward() { + Map keys = new LinkedHashMap<>(); + keys.put("firstName", "Joseph"); + keys.put("lastName", "Heller"); + keys.put("id", 103); + + int count = 10; + ScrollSubrange subrange = ScrollSubrange.create(ScrollPosition.forward(keys), count, false); + + KeysetScrollPosition actualPosition = (KeysetScrollPosition) subrange.position().get(); assertThat(actualPosition.getKeys()).isEqualTo(keys); assertThat(actualPosition.getDirection()).isEqualTo(Direction.BACKWARD); assertThat(subrange.count().orElse(0)).isEqualTo(count); @@ -87,33 +100,34 @@ public class ScrollSubrangeTests { } @Test - void offsetBackwardPaginationWithInsufficientCount() { - ScrollPosition position = ScrollPosition.offset(5); - ScrollSubrange subrange = ScrollSubrange.create(position, 10, false); + void offsetBackwardWithInsufficientCount() { + ScrollSubrange subrange = ScrollSubrange.create(ScrollPosition.offset(5), 10, false); - assertThat(subrange.position()).isNotPresent(); - assertThat(subrange.count().getAsInt()).isEqualTo(4); + assertThat(getOffset(subrange)).isEqualTo(0); + assertThat(subrange.count().getAsInt()).isEqualTo(5); assertThat(subrange.forward()).isTrue(); } @Test - void offsetBackwardPaginationWithOffsetZero() { - ScrollPosition position = ScrollPosition.offset(0); - ScrollSubrange subrange = ScrollSubrange.create(position, 10, false); + void offsetBackwardFromInitialOffset() { + ScrollSubrange subrange = ScrollSubrange.create(ScrollPosition.offset(0), 10, false); - assertThat(subrange.position()).isNotPresent(); + assertThat(getOffset(subrange)).isEqualTo(0); assertThat(subrange.count().getAsInt()).isEqualTo(0); assertThat(subrange.forward()).isTrue(); } @Test - void offsetBackwardPaginationWithNullCount() { - ScrollPosition position = ScrollPosition.offset(30); - ScrollSubrange subrange = ScrollSubrange.create(position, null, false); + void offsetBackwardWithNullCount() { + ScrollSubrange subrange = ScrollSubrange.create(ScrollPosition.offset(30), null, false); - assertThat(subrange.position()).hasValue(ScrollPosition.offset(29)); + assertThat(getOffset(subrange)).isEqualTo(30); assertThat(subrange.count()).isNotPresent(); assertThat(subrange.forward()).isTrue(); } + private static long getOffset(ScrollSubrange subrange) { + return ((OffsetScrollPosition) subrange.position().get()).getOffset(); + } + } 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 0a41f632..18d30715 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 @@ -137,7 +137,7 @@ class QueryByExampleDataFetcherJpaTests { Mono response = graphQlSetup .toWebGraphQlHandler() - .handleRequest(request(BookSource.booksConnectionQuery("first:2, after:\"O_3\""))); + .handleRequest(request(BookSource.booksConnectionQuery("first:2, after:\"O_2\""))); List> edges = ResponseHelper.forResponse(response).toEntity("books.edges", List.class); assertThat(edges.size()).isEqualTo(2); 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 58dbf134..9cd3d855 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 @@ -134,7 +134,7 @@ class QueryByExampleDataFetcherMongoDbTests { Mono response = graphQlSetup .toWebGraphQlHandler() - .handleRequest(request(BookSource.booksConnectionQuery("first:2, after:\"O_3\""))); + .handleRequest(request(BookSource.booksConnectionQuery("first:2, after:\"O_2\""))); List> edges = ResponseHelper.forResponse(response).toEntity("books.edges", List.class); assertThat(edges.size()).isEqualTo(2); 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 3019acd6..a5e32c0b 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 @@ -159,7 +159,7 @@ class QueryByExampleDataFetcherReactiveMongoDbTests { Mono response = graphQlSetup .toWebGraphQlHandler() - .handleRequest(request(BookSource.booksConnectionQuery("first:2, after:\"O_3\""))); + .handleRequest(request(BookSource.booksConnectionQuery("first:2, after:\"O_2\""))); List> edges = ResponseHelper.forResponse(response).toEntity("books.edges", List.class); assertThat(edges.size()).isEqualTo(2); 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 9943dd6d..406f096a 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 @@ -136,7 +136,7 @@ class QueryByExampleDataFetcherNeo4jTests { Mono response = graphQlSetup .toWebGraphQlHandler() - .handleRequest(request(BookSource.booksConnectionQuery("first:2, after:\"O_3\""))); + .handleRequest(request(BookSource.booksConnectionQuery("first:2, after:\"O_2\""))); List> edges = ResponseHelper.forResponse(response).toEntity("books.edges", List.class); assertThat(edges.size()).isEqualTo(2); 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 994162d2..0361f5d1 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 @@ -172,7 +172,7 @@ class QueryByExampleDataFetcherReactiveNeo4jDbTests { Mono response = graphQlSetup .toWebGraphQlHandler() - .handleRequest(request(BookSource.booksConnectionQuery("first:2, after:\"O_3\""))); + .handleRequest(request(BookSource.booksConnectionQuery("first:2, after:\"O_2\""))); List> edges = ResponseHelper.forResponse(response).toEntity("books.edges", List.class); assertThat(edges.size()).isEqualTo(2);