Skip to content

Commit ce7f77c

Browse files
rstoyanchevrstoyanchev
committed
Improve logic to resolve pagination arguments
The presence of either "after" or "first" leads to forward pagination. Else if either "before" or "last" leads to backward pagination. Fall back on forward if none are provided at all. In addition, a small adjustment to backward pagination. If a count is not provided, use 1 rather than 0, to advance to previous item. Closes gh-929
1 parent 1a3ae73 commit ce7f77c

File tree

6 files changed

+154
-66
lines changed

6 files changed

+154
-66
lines changed

‎spring-graphql/src/main/java/org/springframework/graphql/data/method/annotation/support/SubrangeMethodArgumentResolver.java‎

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright 2020-2023 the original author or authors.
2+
* Copyright 2020-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.
@@ -52,11 +52,18 @@ public boolean supportsParameter(MethodParameter parameter) {
5252

5353
@Override
5454
public Object resolveArgument(MethodParameter parameter, DataFetchingEnvironment environment) throws Exception {
55-
boolean forward = !environment.getArguments().containsKey("last");
56-
Integer count = environment.getArgument(forward ? "first" : "last");
57-
String cursor = environment.getArgument(forward ? "after" : "before");
58-
P position = (cursor != null ? this.cursorStrategy.fromCursor(cursor) : null);
59-
return createSubrange(position, count, forward);
55+
boolean forward = true;
56+
String cursor = environment.getArgument("after");
57+
Integer count = environment.getArgument("first");
58+
if (cursor == null && count == null) {
59+
cursor = environment.getArgument("before");
60+
count = environment.getArgument("last");
61+
if (cursor != null || count != null) {
62+
forward = false;
63+
}
64+
}
65+
P pos = (cursor != null ? this.cursorStrategy.fromCursor(cursor) : null);
66+
return createSubrange(pos, count, forward);
6067
}
6168

6269
/**

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

Lines changed: 14 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -93,18 +93,21 @@ public static Function<Boolean, ScrollPosition> defaultScrollPosition() {
9393
return forward -> ScrollPosition.offset();
9494
}
9595

96-
public static ScrollSubrange defaultScrollSubrange() {
97-
return ScrollSubrange.create(ScrollPosition.offset(), 20, true);
98-
}
99-
10096
public static ScrollSubrange getScrollSubrange(
101-
DataFetchingEnvironment env, CursorStrategy<ScrollPosition> strategy) {
102-
103-
boolean forward = !env.getArguments().containsKey("last");
104-
Integer count = env.getArgument(forward ? "first" : "last");
105-
String cursor = env.getArgument(forward ? "after" : "before");
106-
ScrollPosition position = (cursor != null ? strategy.fromCursor(cursor) : null);
107-
return ScrollSubrange.create(position, count, forward);
97+
DataFetchingEnvironment env, CursorStrategy<ScrollPosition> cursorStrategy) {
98+
99+
boolean forward = true;
100+
String cursor = env.getArgument("after");
101+
Integer count = env.getArgument("first");
102+
if (cursor == null && count == null) {
103+
cursor = env.getArgument("before");
104+
count = env.getArgument("last");
105+
if (cursor != null || count != null) {
106+
forward = false;
107+
}
108+
}
109+
ScrollPosition pos = (cursor != null ? cursorStrategy.fromCursor(cursor) : null);
110+
return ScrollSubrange.create(pos, count, forward);
108111
}
109112

110113
}

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

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -110,9 +110,10 @@ private static ScrollSubrange initFromOffsetPosition(
110110
position = position.advanceBy(1);
111111
}
112112
else {
113-
int countOrZero = (count != null ? count : 0);
114-
if (position.getOffset() >= countOrZero) {
115-
position = position.advanceBy(-countOrZero);
113+
// Advance back by 1 at least to item before position
114+
int advanceCount = (count != null ? count : 1);
115+
if (position.getOffset() >= advanceCount) {
116+
position = position.advanceBy(-advanceCount);
116117
}
117118
else {
118119
count = (int) position.getOffset();

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

Lines changed: 68 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright 2020-2023 the original author or authors.
2+
* Copyright 2020-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.
@@ -16,10 +16,9 @@
1616

1717
package org.springframework.graphql.data.method.annotation.support;
1818

19+
import java.util.Collections;
1920
import java.util.Map;
2021

21-
import graphql.schema.DataFetchingEnvironment;
22-
import graphql.schema.DataFetchingEnvironmentImpl;
2322
import org.junit.jupiter.api.Test;
2423

2524
import org.springframework.core.MethodParameter;
@@ -28,6 +27,7 @@
2827
import org.springframework.graphql.data.method.annotation.QueryMapping;
2928
import org.springframework.graphql.data.pagination.CursorStrategy;
3029
import org.springframework.graphql.data.pagination.Subrange;
30+
import org.springframework.lang.Nullable;
3131
import org.springframework.stereotype.Controller;
3232

3333
import static org.assertj.core.api.Assertions.assertThat;
@@ -41,8 +41,7 @@ public class SubrangeMethodArgumentResolverTests extends ArgumentResolverTestSup
4141
private final SubrangeMethodArgumentResolver<MyPosition> resolver =
4242
new SubrangeMethodArgumentResolver<>(new MyPositionCursorStrategy());
4343

44-
private final MethodParameter param =
45-
methodParam(BookController.class, "getBooks", Subrange.class);
44+
private final MethodParameter param = methodParam(BookController.class, "getBooks", Subrange.class);
4645

4746

4847
@Test
@@ -54,30 +53,87 @@ void supports() {
5453
}
5554

5655
@Test
57-
void forwardPagination() throws Exception {
56+
void forward() throws Exception {
5857
int count = 10;
5958
int index = 25;
6059
Map<String, Object> arguments = Map.of("first", count, "after", String.valueOf(index));
6160
Object result = this.resolver.resolveArgument(this.param, environment(arguments));
6261

63-
testRequest(count, index, result, true);
62+
assertResult(true, count, index, result);
6463
}
6564

6665
@Test
67-
void backwardPagination() throws Exception {
66+
void forwardWithCountOnly() throws Exception {
67+
int count = 10;
68+
Map<String, Object> arguments = Map.of("first", count);
69+
Object result = this.resolver.resolveArgument(this.param, environment(arguments));
70+
71+
assertResult(true, count, null, result);
72+
}
73+
74+
@Test
75+
void forwardWithIndexOnly() throws Exception {
76+
int index = 25;
77+
Map<String, Object> arguments = Map.of("after", String.valueOf(index));
78+
Object result = this.resolver.resolveArgument(this.param, environment(arguments));
79+
80+
assertResult(true, null, index, result);
81+
}
82+
83+
@Test
84+
void backward() throws Exception {
6885
int count = 20;
6986
int index = 100;
7087
Map<String, Object> arguments = Map.of("last", count, "before", String.valueOf(index));
7188
Object result = this.resolver.resolveArgument(this.param, environment(arguments));
7289

73-
testRequest(count, index, result, false);
90+
assertResult(false, count, index, result);
91+
}
92+
93+
@Test
94+
void backwardWithCountOnly() throws Exception {
95+
int count = 10;
96+
Map<String, Object> arguments = Map.of("last", count);
97+
Object result = this.resolver.resolveArgument(this.param, environment(arguments));
98+
99+
assertResult(false, count, null, result);
74100
}
75101

76-
private static void testRequest(int count, int index, Object result, boolean forward) {
102+
@Test
103+
void backwardWithIndexOnly() throws Exception {
104+
int index = 25;
105+
Map<String, Object> arguments = Map.of("before", String.valueOf(index));
106+
Object result = this.resolver.resolveArgument(this.param, environment(arguments));
107+
108+
assertResult(false, null, index, result);
109+
}
110+
111+
@Test
112+
void noInput() throws Exception {
113+
Object result = this.resolver.resolveArgument(this.param, environment(Collections.emptyMap()));
114+
assertResult(true, null, null, result);
115+
}
116+
117+
private static void assertResult(
118+
boolean forward, @Nullable Integer count, @Nullable Integer index, @Nullable Object result) {
119+
120+
assertThat(result).isNotNull();
77121
Subrange<MyPosition> subrange = (Subrange<MyPosition>) result;
78-
assertThat(subrange.position().get().index()).isEqualTo(index);
79-
assertThat(subrange.count().orElse(0)).isEqualTo(count);
80122
assertThat(subrange.forward()).isEqualTo(forward);
123+
124+
if (count != null) {
125+
assertThat(subrange.count().orElse(0)).isEqualTo(count);
126+
}
127+
else {
128+
assertThat(subrange.count()).isNotPresent();
129+
}
130+
131+
if (index != null) {
132+
assertThat(subrange.position().get().index()).isEqualTo(index);
133+
}
134+
else {
135+
assertThat(subrange.position()).isNotPresent();
136+
}
81137
}
82138

83139

@@ -94,7 +150,6 @@ public Window<Book> getBooks(Subrange<MyPosition> subrange) {
94150
public Window<Book> getBooksWithUnknownPosition(Subrange<UnknownPosition> subrange) {
95151
return null;
96152
}
97-
98153
}
99154

100155

@@ -115,7 +170,6 @@ public String toCursor(MyPosition position) {
115170
public MyPosition fromCursor(String cursor) {
116171
return new MyPosition(Integer.parseInt(cursor));
117172
}
118-
119173
}
120174

121175

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

Lines changed: 54 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
import graphql.schema.DataFetchingEnvironment;
2323
import graphql.schema.DataFetchingEnvironmentImpl;
2424
import org.junit.jupiter.api.Test;
25+
import org.testcontainers.shaded.org.checkerframework.checker.nullness.qual.Nullable;
2526

2627
import org.springframework.data.domain.OffsetScrollPosition;
2728
import org.springframework.data.domain.ScrollPosition;
@@ -39,65 +40,67 @@ public class RepositoryUtilsTests {
3940

4041

4142
@Test
42-
void buildScrollSubrangeForward() {
43-
OffsetScrollPosition offset = ScrollPosition.offset(10);
43+
void forward() {
44+
OffsetScrollPosition pos = ScrollPosition.offset(10);
4445
int count = 5;
46+
DataFetchingEnvironment env = environment(Map.of("first", count, "after", cursorStrategy.toCursor(pos)));
47+
ScrollSubrange range = RepositoryUtils.getScrollSubrange(env, cursorStrategy);
4548

46-
DataFetchingEnvironment env = environment(
47-
Map.of("first", count, "after", cursorStrategy.toCursor(offset)));
49+
assertSubrange(true, count, pos.advanceBy(1), range);
50+
}
4851

52+
@Test
53+
void backward() {
54+
OffsetScrollPosition pos = ScrollPosition.offset(10);
55+
int count = 5;
56+
DataFetchingEnvironment env = environment(Map.of("last", count, "before", cursorStrategy.toCursor(pos)));
4957
ScrollSubrange range = RepositoryUtils.getScrollSubrange(env, cursorStrategy);
5058

51-
assertThat(range.position().get()).isEqualTo(ScrollPosition.offset(11));
52-
assertThat(range.count().getAsInt()).isEqualTo(count);
53-
assertThat(range.forward()).isTrue();
59+
assertSubrange(true, count, ScrollPosition.offset(5), range);
5460
}
5561

5662
@Test
57-
void buildScrollSubrangeBackward() {
58-
OffsetScrollPosition offset = ScrollPosition.offset(10);
63+
void forwardWithCountOnly() {
5964
int count = 5;
65+
DataFetchingEnvironment env = environment(Map.of("first", count));
66+
ScrollSubrange range = RepositoryUtils.getScrollSubrange(env, cursorStrategy);
6067

61-
DataFetchingEnvironment env = environment(
62-
Map.of("last", count, "before", cursorStrategy.toCursor(offset)));
68+
assertSubrange(true, count, null, range);
69+
}
6370

71+
@Test
72+
void forwardWithPositionOnly() {
73+
OffsetScrollPosition pos = ScrollPosition.offset(10);
74+
DataFetchingEnvironment env = environment(Map.of("after", cursorStrategy.toCursor(pos)));
6475
ScrollSubrange range = RepositoryUtils.getScrollSubrange(env, cursorStrategy);
6576

66-
assertThat(range.position().get()).isEqualTo(ScrollPosition.offset(5));
67-
assertThat(range.count().getAsInt()).isEqualTo(count);
68-
assertThat(range.forward()).isTrue();
77+
assertSubrange(true, null, pos.advanceBy(1), range);
6978
}
7079

7180
@Test
72-
void noInput() {
73-
DataFetchingEnvironment env = environment(Collections.emptyMap());
81+
void backwardWithCountOnly() {
82+
int count = 5;
83+
DataFetchingEnvironment env = environment(Map.of("last", count));
7484
ScrollSubrange range = RepositoryUtils.getScrollSubrange(env, cursorStrategy);
7585

76-
assertThat(range.position()).isNotPresent();
77-
assertThat(range.count()).isNotPresent();
78-
assertThat(range.forward()).isTrue();
86+
assertSubrange(false, count, null, range);
7987
}
8088

8189
@Test
82-
void buildScrollSubrangeForwardWithoutPosition() {
83-
int count = 5;
84-
DataFetchingEnvironment env = environment(Map.of("first", count));
90+
void backwardWithPositionOnly() {
91+
OffsetScrollPosition pos = ScrollPosition.offset(10);
92+
DataFetchingEnvironment env = environment(Map.of("before", cursorStrategy.toCursor(pos)));
8593
ScrollSubrange range = RepositoryUtils.getScrollSubrange(env, cursorStrategy);
8694

87-
assertThat(range.position()).isNotPresent();
88-
assertThat(range.count().getAsInt()).isEqualTo(count);
89-
assertThat(range.forward()).isTrue();
95+
assertSubrange(true, null, pos.advanceBy(-1), range);
9096
}
9197

9298
@Test
93-
void buildScrollSubrangeBackwardWithoutPosition() {
94-
int count = 5;
95-
DataFetchingEnvironment env = environment(Map.of("last", count));
99+
void noInput() {
100+
DataFetchingEnvironment env = environment(Collections.emptyMap());
96101
ScrollSubrange range = RepositoryUtils.getScrollSubrange(env, cursorStrategy);
97102

98-
assertThat(range.position()).isNotPresent();
99-
assertThat(range.count().getAsInt()).isEqualTo(count);
100-
assertThat(range.forward()).isFalse();
103+
assertSubrange(true, null, null, range);
101104
}
102105

103106
private static DataFetchingEnvironment environment(Map<String, Object> arguments) {
@@ -106,4 +109,24 @@ private static DataFetchingEnvironment environment(Map<String, Object> arguments
106109
.build();
107110
}
108111

112+
private static void assertSubrange(
113+
boolean forward, @Nullable Integer count, @Nullable ScrollPosition pos, ScrollSubrange subrange) {
114+
115+
assertThat(subrange.forward()).isEqualTo(forward);
116+
117+
if (count != null) {
118+
assertThat(subrange.count().orElse(0)).isEqualTo(count);
119+
}
120+
else {
121+
assertThat(subrange.count()).isNotPresent();
122+
}
123+
124+
if (pos != null) {
125+
assertThat(subrange.position().get()).isEqualTo(pos);
126+
}
127+
else {
128+
assertThat(subrange.position()).isNotPresent();
129+
}
130+
}
131+
109132
}

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -121,7 +121,7 @@ void offsetBackwardFromInitialOffset() {
121121
void offsetBackwardWithNullCount() {
122122
ScrollSubrange subrange = ScrollSubrange.create(ScrollPosition.offset(30), null, false);
123123

124-
assertThat(getOffset(subrange)).isEqualTo(30);
124+
assertThat(getOffset(subrange)).isEqualTo(29);
125125
assertThat(subrange.count()).isNotPresent();
126126
assertThat(subrange.forward()).isTrue();
127127
}

0 commit comments

Comments
 (0)