Skip to content

Commit 9817230

Browse files
rstoyanchevrstoyanchev
committed
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
1 parent 53108d0 commit 9817230

File tree

10 files changed

+78
-52
lines changed

10 files changed

+78
-52
lines changed

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

Lines changed: 25 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -71,16 +71,20 @@ else if (pos instanceof KeysetScrollPosition keysetPosition) {
7171

7272

7373
/**
74-
* Create a {@link ScrollSubrange} instance.
75-
* @param position the position relative to which to scroll, or {@code null}
76-
* for scrolling from the beginning
77-
* @param count the number of elements requested
78-
* @param forward whether to return elements after (true) or before (false)
79-
* the element at the given position
80-
* @return the created subrange
74+
* Create a {@link ScrollSubrange} from the given inputs.
75+
* <p>Pagination with offset-based scrolling is always forward and inclusive
76+
* of the referenced item. Therefore, an {@link OffsetScrollPosition} is
77+
* adjusted as follows. For forward pagination, advanced by 1. For backward
78+
* pagination, advanced back by the count, and switched to forward.
79+
* @param position the reference position, or {@code null} if not specified
80+
* @param count how many to return, or {@code null} if not specified
81+
* @param forward whether scroll forward (true) or backward (false)
82+
* @return the created instance
8183
* @since 1.2.4
8284
*/
83-
public static ScrollSubrange create(@Nullable ScrollPosition position, @Nullable Integer count, boolean forward) {
85+
public static ScrollSubrange create(
86+
@Nullable ScrollPosition position, @Nullable Integer count, boolean forward) {
87+
8488
if (count != null && count < 0) {
8589
count = null;
8690
}
@@ -98,16 +102,24 @@ else if (position instanceof KeysetScrollPosition keysetScrollPosition) {
98102
private static ScrollSubrange initFromOffsetPosition(
99103
OffsetScrollPosition position, @Nullable Integer count, boolean forward) {
100104

101-
if (!forward) {
105+
// Offset is inclusive, adapt to exclusive:
106+
// - for forward, add 1 to return items after position
107+
// - for backward, subtract count to get items before position
108+
109+
if (forward) {
110+
position = position.advanceBy(1);
111+
}
112+
else {
102113
int countOrZero = (count != null ? count : 0);
103-
if (countOrZero < position.getOffset()) {
104-
position = position.advanceBy(-countOrZero-1);
114+
if (position.getOffset() >= countOrZero) {
115+
position = position.advanceBy(-countOrZero);
105116
}
106117
else {
107-
count = (position.getOffset() > 0 ? (int) (position.getOffset() - 1) : 0);
108-
position = null;
118+
count = (int) position.getOffset();
119+
position = ScrollPosition.offset();
109120
}
110121
}
122+
111123
return new ScrollSubrange(position, count, true, null);
112124
}
113125

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright 2002-2023 the original author or authors.
2+
* Copyright 2002-2024 the original author or authors.
33
*
44
* Licensed under the Apache License, Version 2.0 (the "License");
55
* you may not use this file except in compliance with the License.
@@ -49,7 +49,7 @@ public class SchemaMappingPaginationTests {
4949
@Test
5050
void forwardPagination() {
5151

52-
String document = BookSource.booksConnectionQuery("first:2, after:\"O_3\"");
52+
String document = BookSource.booksConnectionQuery("first:2, after:\"O_2\"");
5353
Mono<ExecutionGraphQlResponse> response = graphQlService().execute(document);
5454

5555
ResponseHelper.forResponse(response).assertData(

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

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -139,12 +139,12 @@ void shouldFetchWindow() {
139139
ResponseHelper.forResponse(response).assertData(
140140
"{\"books\":{" +
141141
"\"edges\":[" +
142-
"{\"cursor\":\"O_4\",\"node\":{\"id\":\"42\",\"name\":\"Hitchhiker's Guide to the Galaxy\"}}," +
143-
"{\"cursor\":\"O_5\",\"node\":{\"id\":\"53\",\"name\":\"Breaking Bad\"}}" +
142+
"{\"cursor\":\"O_0\",\"node\":{\"id\":\"42\",\"name\":\"Hitchhiker's Guide to the Galaxy\"}}," +
143+
"{\"cursor\":\"O_1\",\"node\":{\"id\":\"53\",\"name\":\"Breaking Bad\"}}" +
144144
"]," +
145145
"\"pageInfo\":{" +
146-
"\"startCursor\":\"O_4\"," +
147-
"\"endCursor\":\"O_5\"," +
146+
"\"startCursor\":\"O_0\"," +
147+
"\"endCursor\":\"O_1\"," +
148148
"\"hasPreviousPage\":true," +
149149
"\"hasNextPage\":false" +
150150
"}}}"

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

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ void buildScrollSubrangeForward() {
5050

5151
ScrollSubrange range = RepositoryUtils.getScrollSubrange(env, cursorStrategy, defaultSubrange);
5252

53-
assertThat(range.position().get()).isEqualTo(offset);
53+
assertThat(range.position().get()).isEqualTo(ScrollPosition.offset(11));
5454
assertThat(range.count().getAsInt()).isEqualTo(count);
5555
assertThat(range.forward()).isTrue();
5656
}
@@ -65,7 +65,7 @@ void buildScrollSubrangeBackward() {
6565

6666
ScrollSubrange range = RepositoryUtils.getScrollSubrange(env, cursorStrategy, defaultSubrange);
6767

68-
assertThat(range.position().get()).isEqualTo(offset.advanceBy(-count-1));
68+
assertThat(range.position().get()).isEqualTo(ScrollPosition.offset(5));
6969
assertThat(range.count().getAsInt()).isEqualTo(count);
7070
assertThat(range.forward()).isTrue();
7171
}
@@ -75,7 +75,7 @@ void buildScrollSubrangeForwardWithDefaultScrollSubrange() {
7575
DataFetchingEnvironment env = environment(Collections.emptyMap());
7676
ScrollSubrange range = RepositoryUtils.getScrollSubrange(env, cursorStrategy, defaultSubrange);
7777

78-
assertThat(range.position().get()).isEqualTo(getDefaultPosition());
78+
assertThat(range.position().get()).isEqualTo(getDefaultPosition().advanceBy(1));
7979
assertThat(range.count().getAsInt()).isEqualTo(this.defaultSubrange.count().getAsInt());
8080
assertThat(range.forward()).isTrue();
8181
}
@@ -86,7 +86,7 @@ void buildScrollSubrangeBackwardFromDefaultPosition() {
8686
DataFetchingEnvironment env = environment(Map.of("last", count));
8787
ScrollSubrange range = RepositoryUtils.getScrollSubrange(env, cursorStrategy, defaultSubrange);
8888

89-
assertThat(range.position().get()).isEqualTo(getDefaultPosition().advanceBy(-count-1));
89+
assertThat(range.position().get()).isEqualTo(getDefaultPosition().advanceBy(-count));
9090
assertThat(range.count().getAsInt()).isEqualTo(count);
9191
assertThat(range.forward()).isTrue();
9292
}

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

Lines changed: 38 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -37,40 +37,53 @@
3737
public class ScrollSubrangeTests {
3838

3939
@Test
40-
void offset() {
41-
ScrollPosition position = ScrollPosition.offset(30);
40+
void offsetForward() {
4241
int count = 10;
42+
ScrollSubrange subrange = ScrollSubrange.create(ScrollPosition.offset(30), count, true);
4343

44-
ScrollSubrange subrange = ScrollSubrange.create(position, count, true);
45-
assertThat(((OffsetScrollPosition) subrange.position().get())).isEqualTo(position);
44+
assertThat(getOffset(subrange)).isEqualTo(31);
4645
assertThat(subrange.count().orElse(0)).isEqualTo(count);
4746
assertThat(subrange.forward()).isTrue();
47+
}
4848

49-
subrange = ScrollSubrange.create(position, count, false);
50-
assertThat(((OffsetScrollPosition) subrange.position().get()).getOffset()).isEqualTo(19);
49+
@Test
50+
void offsetBackward() {
51+
int count = 10;
52+
ScrollSubrange subrange = ScrollSubrange.create(ScrollPosition.offset(30), count, false);
53+
54+
assertThat(getOffset(subrange)).isEqualTo(20);
5155
assertThat(subrange.count().orElse(0)).isEqualTo(count);
5256
assertThat(subrange.forward()).isTrue();
5357
}
5458

5559
@Test
56-
void keyset() {
60+
void keysetForward() {
5761
Map<String, Object> keys = new LinkedHashMap<>();
5862
keys.put("firstName", "Joseph");
5963
keys.put("lastName", "Heller");
6064
keys.put("id", 103);
6165

62-
ScrollPosition position = ScrollPosition.forward(keys);
6366
int count = 10;
67+
ScrollSubrange subrange = ScrollSubrange.create(ScrollPosition.forward(keys), count, true);
6468

65-
ScrollSubrange subrange = ScrollSubrange.create(position, count, true);
6669
KeysetScrollPosition actualPosition = (KeysetScrollPosition) subrange.position().get();
6770
assertThat(actualPosition.getKeys()).isEqualTo(keys);
6871
assertThat(actualPosition.getDirection()).isEqualTo(Direction.FORWARD);
6972
assertThat(subrange.count().orElse(0)).isEqualTo(count);
7073
assertThat(subrange.forward()).isTrue();
74+
}
75+
76+
@Test
77+
void keysetBackward() {
78+
Map<String, Object> keys = new LinkedHashMap<>();
79+
keys.put("firstName", "Joseph");
80+
keys.put("lastName", "Heller");
81+
keys.put("id", 103);
7182

72-
subrange = ScrollSubrange.create(position, count, false);
73-
actualPosition = (KeysetScrollPosition) subrange.position().get();
83+
int count = 10;
84+
ScrollSubrange subrange = ScrollSubrange.create(ScrollPosition.forward(keys), count, false);
85+
86+
KeysetScrollPosition actualPosition = (KeysetScrollPosition) subrange.position().get();
7487
assertThat(actualPosition.getKeys()).isEqualTo(keys);
7588
assertThat(actualPosition.getDirection()).isEqualTo(Direction.BACKWARD);
7689
assertThat(subrange.count().orElse(0)).isEqualTo(count);
@@ -87,33 +100,34 @@ void nullInput() {
87100
}
88101

89102
@Test
90-
void offsetBackwardPaginationWithInsufficientCount() {
91-
ScrollPosition position = ScrollPosition.offset(5);
92-
ScrollSubrange subrange = ScrollSubrange.create(position, 10, false);
103+
void offsetBackwardWithInsufficientCount() {
104+
ScrollSubrange subrange = ScrollSubrange.create(ScrollPosition.offset(5), 10, false);
93105

94-
assertThat(subrange.position()).isNotPresent();
95-
assertThat(subrange.count().getAsInt()).isEqualTo(4);
106+
assertThat(getOffset(subrange)).isEqualTo(0);
107+
assertThat(subrange.count().getAsInt()).isEqualTo(5);
96108
assertThat(subrange.forward()).isTrue();
97109
}
98110

99111
@Test
100-
void offsetBackwardPaginationWithOffsetZero() {
101-
ScrollPosition position = ScrollPosition.offset(0);
102-
ScrollSubrange subrange = ScrollSubrange.create(position, 10, false);
112+
void offsetBackwardFromInitialOffset() {
113+
ScrollSubrange subrange = ScrollSubrange.create(ScrollPosition.offset(0), 10, false);
103114

104-
assertThat(subrange.position()).isNotPresent();
115+
assertThat(getOffset(subrange)).isEqualTo(0);
105116
assertThat(subrange.count().getAsInt()).isEqualTo(0);
106117
assertThat(subrange.forward()).isTrue();
107118
}
108119

109120
@Test
110-
void offsetBackwardPaginationWithNullCount() {
111-
ScrollPosition position = ScrollPosition.offset(30);
112-
ScrollSubrange subrange = ScrollSubrange.create(position, null, false);
121+
void offsetBackwardWithNullCount() {
122+
ScrollSubrange subrange = ScrollSubrange.create(ScrollPosition.offset(30), null, false);
113123

114-
assertThat(subrange.position()).hasValue(ScrollPosition.offset(29));
124+
assertThat(getOffset(subrange)).isEqualTo(30);
115125
assertThat(subrange.count()).isNotPresent();
116126
assertThat(subrange.forward()).isTrue();
117127
}
118128

129+
private static long getOffset(ScrollSubrange subrange) {
130+
return ((OffsetScrollPosition) subrange.position().get()).getOffset();
131+
}
132+
119133
}

‎spring-graphql/src/test/java/org/springframework/graphql/data/query/jpa/QueryByExampleDataFetcherJpaTests.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -137,7 +137,7 @@ void shouldFetchWindow() {
137137

138138
Mono<WebGraphQlResponse> response = graphQlSetup
139139
.toWebGraphQlHandler()
140-
.handleRequest(request(BookSource.booksConnectionQuery("first:2, after:\"O_3\"")));
140+
.handleRequest(request(BookSource.booksConnectionQuery("first:2, after:\"O_2\"")));
141141

142142
List<Map<String, Object>> edges = ResponseHelper.forResponse(response).toEntity("books.edges", List.class);
143143
assertThat(edges.size()).isEqualTo(2);

‎spring-graphql/src/test/java/org/springframework/graphql/data/query/mongo/QueryByExampleDataFetcherMongoDbTests.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -134,7 +134,7 @@ void shouldFetchWindow() {
134134

135135
Mono<WebGraphQlResponse> response = graphQlSetup
136136
.toWebGraphQlHandler()
137-
.handleRequest(request(BookSource.booksConnectionQuery("first:2, after:\"O_3\"")));
137+
.handleRequest(request(BookSource.booksConnectionQuery("first:2, after:\"O_2\"")));
138138

139139
List<Map<String, Object>> edges = ResponseHelper.forResponse(response).toEntity("books.edges", List.class);
140140
assertThat(edges.size()).isEqualTo(2);

‎spring-graphql/src/test/java/org/springframework/graphql/data/query/mongo/QueryByExampleDataFetcherReactiveMongoDbTests.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -159,7 +159,7 @@ void shouldFetchWindow() {
159159

160160
Mono<WebGraphQlResponse> response = graphQlSetup
161161
.toWebGraphQlHandler()
162-
.handleRequest(request(BookSource.booksConnectionQuery("first:2, after:\"O_3\"")));
162+
.handleRequest(request(BookSource.booksConnectionQuery("first:2, after:\"O_2\"")));
163163

164164
List<Map<String, Object>> edges = ResponseHelper.forResponse(response).toEntity("books.edges", List.class);
165165
assertThat(edges.size()).isEqualTo(2);

‎spring-graphql/src/test/java/org/springframework/graphql/data/query/neo4j/QueryByExampleDataFetcherNeo4jTests.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -136,7 +136,7 @@ void shouldFetchWindow() {
136136

137137
Mono<WebGraphQlResponse> response = graphQlSetup
138138
.toWebGraphQlHandler()
139-
.handleRequest(request(BookSource.booksConnectionQuery("first:2, after:\"O_3\"")));
139+
.handleRequest(request(BookSource.booksConnectionQuery("first:2, after:\"O_2\"")));
140140

141141
List<Map<String, Object>> edges = ResponseHelper.forResponse(response).toEntity("books.edges", List.class);
142142
assertThat(edges.size()).isEqualTo(2);

‎spring-graphql/src/test/java/org/springframework/graphql/data/query/neo4j/QueryByExampleDataFetcherReactiveNeo4jDbTests.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -172,7 +172,7 @@ void shouldFetchWindow() {
172172

173173
Mono<WebGraphQlResponse> response = graphQlSetup
174174
.toWebGraphQlHandler()
175-
.handleRequest(request(BookSource.booksConnectionQuery("first:2, after:\"O_3\"")));
175+
.handleRequest(request(BookSource.booksConnectionQuery("first:2, after:\"O_2\"")));
176176

177177
List<Map<String, Object>> edges = ResponseHelper.forResponse(response).toEntity("books.edges", List.class);
178178
assertThat(edges.size()).isEqualTo(2);

0 commit comments

Comments
 (0)