Skip to content

Commit 25f610c

Browse files
mp911demp911de
committed
Fix keyset backwards scrolling.
We now correctly scroll backwards by reversing sort order to apply the correct limit and reverse the results again to restore the actual sort order. Closes #4332
1 parent d8c04f0 commit 25f610c

File tree

4 files changed

+155
-87
lines changed

4 files changed

+155
-87
lines changed

‎spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/MongoTemplate.java‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,7 @@
6666
import org.springframework.data.mongodb.core.QueryOperations.DistinctQueryContext;
6767
import org.springframework.data.mongodb.core.QueryOperations.QueryContext;
6868
import org.springframework.data.mongodb.core.QueryOperations.UpdateContext;
69-
import org.springframework.data.mongodb.core.ScrollUtils.KeySetScrollQuery;
69+
import org.springframework.data.mongodb.core.ScrollUtils.KeysetScrollQuery;
7070
import org.springframework.data.mongodb.core.aggregation.Aggregation;
7171
import org.springframework.data.mongodb.core.aggregation.AggregationOperationContext;
7272
import org.springframework.data.mongodb.core.aggregation.AggregationOptions;
@@ -876,14 +876,14 @@ <T> Window<T> doScroll(Query query, Class<?> sourceClass, Class<T> targetClass,
876876

877877
if (query.hasKeyset()) {
878878

879-
KeySetScrollQuery keysetPaginationQuery = ScrollUtils.createKeysetPaginationQuery(query,
879+
KeysetScrollQuery keysetPaginationQuery = ScrollUtils.createKeysetPaginationQuery(query,
880880
operations.getIdPropertyName(sourceClass));
881881

882882
List<T> result = doFind(collectionName, createDelegate(query), keysetPaginationQuery.query(),
883883
keysetPaginationQuery.fields(), sourceClass,
884884
new QueryCursorPreparer(query, keysetPaginationQuery.sort(), limit, 0, sourceClass), callback);
885885

886-
return ScrollUtils.createWindow(query.getSortObject(), query.getLimit(), result, sourceClass, operations);
886+
return ScrollUtils.createWindow(query, result, sourceClass, operations);
887887
}
888888

889889
List<T> result = doFind(collectionName, createDelegate(query), query.getQueryObject(), query.getFieldsObject(),

‎spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/ReactiveMongoTemplate.java‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,7 @@
8080
import org.springframework.data.mongodb.core.QueryOperations.DistinctQueryContext;
8181
import org.springframework.data.mongodb.core.QueryOperations.QueryContext;
8282
import org.springframework.data.mongodb.core.QueryOperations.UpdateContext;
83-
import org.springframework.data.mongodb.core.ScrollUtils.KeySetScrollQuery;
83+
import org.springframework.data.mongodb.core.ScrollUtils.KeysetScrollQuery;
8484
import org.springframework.data.mongodb.core.aggregation.Aggregation;
8585
import org.springframework.data.mongodb.core.aggregation.AggregationOperationContext;
8686
import org.springframework.data.mongodb.core.aggregation.AggregationOptions;
@@ -855,14 +855,14 @@ <T> Mono<Window<T>> doScroll(Query query, Class<?> sourceClass, Class<T> targetC
855855

856856
if (query.hasKeyset()) {
857857

858-
KeySetScrollQuery keysetPaginationQuery = ScrollUtils.createKeysetPaginationQuery(query,
858+
KeysetScrollQuery keysetPaginationQuery = ScrollUtils.createKeysetPaginationQuery(query,
859859
operations.getIdPropertyName(sourceClass));
860860

861861
Mono<List<T>> result = doFind(collectionName, ReactiveCollectionPreparerDelegate.of(query),
862862
keysetPaginationQuery.query(), keysetPaginationQuery.fields(), sourceClass,
863863
new QueryFindPublisherPreparer(query, keysetPaginationQuery.sort(), limit, 0, sourceClass), callback).collectList();
864864

865-
return result.map(it -> ScrollUtils.createWindow(query.getSortObject(), query.getLimit(), it, sourceClass, operations));
865+
return result.map(it -> ScrollUtils.createWindow(query, it, sourceClass, operations));
866866
}
867867

868868
Mono<List<T>> result = doFind(collectionName, ReactiveCollectionPreparerDelegate.of(query), query.getQueryObject(),

‎spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/ScrollUtils.java‎

Lines changed: 134 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
package org.springframework.data.mongodb.core;
1717

1818
import java.util.ArrayList;
19+
import java.util.Collections;
1920
import java.util.List;
2021
import java.util.Map;
2122
import java.util.function.IntFunction;
@@ -45,32 +46,110 @@ class ScrollUtils {
4546
* @param idPropertyName
4647
* @return
4748
*/
48-
static KeySetScrollQuery createKeysetPaginationQuery(Query query, String idPropertyName) {
49+
static KeysetScrollQuery createKeysetPaginationQuery(Query query, String idPropertyName) {
4950

5051
KeysetScrollPosition keyset = query.getKeyset();
51-
Map<String, Object> keysetValues = keyset.getKeys();
52-
Document queryObject = query.getQueryObject();
52+
KeysetScrollDirector director = KeysetScrollDirector.of(keyset.getDirection());
53+
Document sortObject = director.getSortObject(idPropertyName, query);
54+
Document fieldsObject = director.getFieldsObject(query.getFieldsObject(), sortObject);
55+
Document queryObject = director.createQuery(keyset, query.getQueryObject(), sortObject);
5356

54-
Document sortObject = query.isSorted() ? query.getSortObject() : new Document();
55-
sortObject.put(idPropertyName, 1);
57+
return new KeysetScrollQuery(queryObject, fieldsObject, sortObject);
58+
}
5659

57-
// make sure we can extract the keyset
58-
Document fieldsObject = query.getFieldsObject();
59-
if (!fieldsObject.isEmpty()) {
60-
for (String field : sortObject.keySet()) {
61-
fieldsObject.put(field, 1);
62-
}
60+
static <T> Window<T> createWindow(Query query, List<T> result, Class<?> sourceType, EntityOperations operations) {
61+
62+
Document sortObject = query.getSortObject();
63+
KeysetScrollPosition keyset = query.getKeyset();
64+
KeysetScrollDirector director = KeysetScrollDirector.of(keyset.getDirection());
65+
66+
director.postPostProcessResults(result);
67+
68+
IntFunction<KeysetScrollPosition> positionFunction = value -> {
69+
70+
T last = result.get(value);
71+
Entity<T> entity = operations.forEntity(last);
72+
73+
Map<String, Object> keys = entity.extractKeys(sortObject, sourceType);
74+
return KeysetScrollPosition.of(keys);
75+
};
76+
77+
return createWindow(result, query.getLimit(), positionFunction);
78+
}
79+
80+
static <T> Window<T> createWindow(List<T> result, int limit, IntFunction<? extends ScrollPosition> positionFunction) {
81+
return Window.from(getSubList(result, limit), positionFunction, hasMoreElements(result, limit));
82+
}
83+
84+
static boolean hasMoreElements(List<?> result, int limit) {
85+
return !result.isEmpty() && result.size() > limit;
86+
}
87+
88+
static <T> List<T> getSubList(List<T> result, int limit) {
89+
90+
if (limit > 0 && result.size() > limit) {
91+
return result.subList(0, limit);
6392
}
6493

65-
List<Document> or = (List<Document>) queryObject.getOrDefault("$or", new ArrayList<>());
66-
List<String> sortKeys = new ArrayList<>(sortObject.keySet());
94+
return result;
95+
}
96+
97+
record KeysetScrollQuery(Document query, Document fields, Document sort) {
6798

68-
if (!keysetValues.isEmpty() && !keysetValues.keySet().containsAll(sortKeys)) {
69-
throw new IllegalStateException("KeysetScrollPosition does not contain all keyset values");
99+
}
100+
101+
/**
102+
* Director for keyset scrolling.
103+
*/
104+
static class KeysetScrollDirector {
105+
106+
private static final KeysetScrollDirector forward = new KeysetScrollDirector();
107+
private static final KeysetScrollDirector reverse = new ReverseKeysetScrollDirector();
108+
109+
/**
110+
* Factory method to obtain the right {@link KeysetScrollDirector}.
111+
*
112+
* @param direction
113+
* @return
114+
*/
115+
public static KeysetScrollDirector of(KeysetScrollPosition.Direction direction) {
116+
return direction == Direction.Forward ? forward : reverse;
117+
}
118+
119+
public Document getSortObject(String idPropertyName, Query query) {
120+
121+
Document sortObject = query.isSorted() ? query.getSortObject() : new Document();
122+
sortObject.put(idPropertyName, 1);
123+
124+
return sortObject;
70125
}
71126

72-
// first query doesn't come with a keyset
73-
if (!keysetValues.isEmpty()) {
127+
public Document getFieldsObject(Document fieldsObject, Document sortObject) {
128+
129+
// make sure we can extract the keyset
130+
if (!fieldsObject.isEmpty()) {
131+
for (String field : sortObject.keySet()) {
132+
fieldsObject.put(field, 1);
133+
}
134+
}
135+
136+
return fieldsObject;
137+
}
138+
139+
public Document createQuery(KeysetScrollPosition keyset, Document queryObject, Document sortObject) {
140+
141+
Map<String, Object> keysetValues = keyset.getKeys();
142+
List<Document> or = (List<Document>) queryObject.getOrDefault("$or", new ArrayList<>());
143+
List<String> sortKeys = new ArrayList<>(sortObject.keySet());
144+
145+
// first query doesn't come with a keyset
146+
if (keysetValues.isEmpty()) {
147+
return queryObject;
148+
}
149+
150+
if (!keysetValues.keySet().containsAll(sortKeys)) {
151+
throw new IllegalStateException("KeysetScrollPosition does not contain all keyset values");
152+
}
74153

75154
// build matrix query for keyset paging that contains sort^2 queries
76155
// reflecting a query that follows sort order semantics starting from the last returned keyset
@@ -89,7 +168,7 @@ static KeySetScrollQuery createKeysetPaginationQuery(Query query, String idPrope
89168
throw new IllegalStateException(
90169
"Cannot resume from KeysetScrollPosition. Offending key: '%s' is 'null'".formatted(sortSegment));
91170
}
92-
sortConstraint.put(sortSegment, new Document(getComparator(sortOrder, keyset.getDirection()), o));
171+
sortConstraint.put(sortSegment, new Document(getComparator(sortOrder), o));
93172
break;
94173
}
95174

@@ -100,61 +179,60 @@ static KeySetScrollQuery createKeysetPaginationQuery(Query query, String idPrope
100179
or.add(sortConstraint);
101180
}
102181
}
103-
}
104182

105-
if (!or.isEmpty()) {
106-
queryObject.put("$or", or);
107-
}
183+
if (!or.isEmpty()) {
184+
queryObject.put("$or", or);
185+
}
108186

109-
return new KeySetScrollQuery(queryObject, fieldsObject, sortObject);
110-
}
187+
return queryObject;
188+
}
111189

112-
private static String getComparator(int sortOrder, Direction direction) {
190+
public <T> void postPostProcessResults(List<T> result) {
113191

114-
// use gte/lte to include the object at the cursor/keyset so that
115-
// we can include it in the result to check whether there is a next object.
116-
// It needs to be filtered out later on.
117-
if (direction == Direction.Backward) {
118-
return sortOrder == 0 ? "$gte" : "$lte";
119192
}
120193

121-
return sortOrder == 1 ? "$gt" : "$lt";
194+
protected String getComparator(int sortOrder) {
195+
return sortOrder == 1 ? "$gt" : "$lt";
196+
}
122197
}
123198

124-
static <T> Window<T> createWindow(Document sortObject, int limit, List<T> result, Class<?> sourceType,
125-
EntityOperations operations) {
126-
127-
IntFunction<KeysetScrollPosition> positionFunction = value -> {
128-
129-
T last = result.get(value);
130-
Entity<T> entity = operations.forEntity(last);
131-
132-
Map<String, Object> keys = entity.extractKeys(sortObject, sourceType);
133-
return KeysetScrollPosition.of(keys);
134-
};
199+
/**
200+
* Reverse scrolling director variant applying {@link KeysetScrollPosition.Direction#Backward}. In reverse scrolling,
201+
* we need to flip directions for the actual query so that we do not get everything from the top position and apply
202+
* the limit but rather flip the sort direction, apply the limit and then reverse the result to restore the actual
203+
* sort order.
204+
*/
205+
private static class ReverseKeysetScrollDirector extends KeysetScrollDirector {
135206

136-
return createWindow(result, limit, positionFunction);
137-
}
207+
@Override
208+
public Document getSortObject(String idPropertyName, Query query) {
138209

139-
static <T> Window<T> createWindow(List<T> result, int limit, IntFunction<? extends ScrollPosition> positionFunction) {
140-
return Window.from(getSubList(result, limit), positionFunction, hasMoreElements(result, limit));
141-
}
210+
Document sortObject = super.getSortObject(idPropertyName, query);
142211

143-
static boolean hasMoreElements(List<?> result, int limit) {
144-
return !result.isEmpty() && result.size() > limit;
145-
}
212+
// flip sort direction for backward scrolling
146213

147-
static <T> List<T> getSubList(List<T> result, int limit) {
214+
for (String field : sortObject.keySet()) {
215+
sortObject.put(field, sortObject.getInteger(field) == 1 ? -1 : 1);
216+
}
148217

149-
if (limit > 0 && result.size() > limit) {
150-
return result.subList(0, limit);
218+
return sortObject;
151219
}
152220

153-
return result;
154-
}
221+
@Override
222+
protected String getComparator(int sortOrder) {
155223

156-
record KeySetScrollQuery(Document query, Document fields, Document sort) {
224+
// use gte/lte to include the object at the cursor/keyset so that
225+
// we can include it in the result to check whether there is a next object.
226+
// It needs to be filtered out later on.
227+
return sortOrder == 1 ? "$gte" : "$lte";
228+
}
157229

230+
@Override
231+
public <T> void postPostProcessResults(List<T> result) {
232+
// flip direction of the result list as we need to accomodate for the flipped sort order for proper offset
233+
// querying.
234+
Collections.reverse(result);
235+
}
158236
}
159237

160238
}

‎spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/MongoTemplateScrollTests.java‎

Lines changed: 15 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -167,42 +167,32 @@ void shouldErrorOnNullValueForQuery() {
167167
@Test // GH-4308
168168
void shouldAllowReverseSort() {
169169

170-
WithNestedDocument john20 = new WithNestedDocument(null, "John", 120, new WithNestedDocument("John", 20),
171-
new Document("name", "bar"));
172-
WithNestedDocument john40 = new WithNestedDocument(null, "John", 140, new WithNestedDocument("John", 40),
173-
new Document("name", "baz"));
174-
WithNestedDocument john41 = new WithNestedDocument(null, "John", 141, new WithNestedDocument("John", 41),
175-
new Document("name", "foo"));
176-
177-
template.insertAll(Arrays.asList(john20, john40, john41));
178-
179-
Query q = new Query(where("name").regex("J.*")).with(Sort.by("nested.name", "nested.age", "document.name"))
180-
.limit(2);
181-
q.with(KeysetScrollPosition.initial());
182-
183-
Window<WithNestedDocument> window = template.scroll(q, WithNestedDocument.class);
170+
Person jane_20 = new Person("Jane", 20);
171+
Person jane_40 = new Person("Jane", 40);
172+
Person jane_42 = new Person("Jane", 42);
173+
Person john20 = new Person("John", 20);
174+
Person john40_1 = new Person("John", 40);
175+
Person john40_2 = new Person("John", 40);
184176

185-
assertThat(window.hasNext()).isTrue();
186-
assertThat(window.isLast()).isFalse();
187-
assertThat(window).hasSize(2);
188-
assertThat(window).containsOnly(john20, john40);
177+
template.insertAll(Arrays.asList(john20, john40_1, john40_2, jane_20, jane_40, jane_42));
178+
Query q = new Query(where("firstName").regex("J.*")).with(Sort.by("firstName", "age"));
179+
q.with(KeysetScrollPosition.initial()).limit(6);
189180

190-
window = template.scroll(q.with(window.positionAt(window.size() - 1)), WithNestedDocument.class);
181+
Window<Person> window = template.scroll(q, Person.class);
191182

192183
assertThat(window.hasNext()).isFalse();
193184
assertThat(window.isLast()).isTrue();
194-
assertThat(window).hasSize(1);
195-
assertThat(window).containsOnly(john41);
185+
assertThat(window).hasSize(6);
196186

197-
KeysetScrollPosition scrollPosition = (KeysetScrollPosition) window.positionAt(0);
187+
KeysetScrollPosition scrollPosition = (KeysetScrollPosition) window.positionAt(window.size() - 1);
198188
KeysetScrollPosition reversePosition = KeysetScrollPosition.of(scrollPosition.getKeys(), Direction.Backward);
199189

200-
window = template.scroll(q.with(reversePosition), WithNestedDocument.class);
190+
window = template.scroll(q.with(reversePosition).limit(2), Person.class);
201191

192+
assertThat(window).hasSize(2);
193+
assertThat(window).containsOnly(john20, john40_1);
202194
assertThat(window.hasNext()).isTrue();
203195
assertThat(window.isLast()).isFalse();
204-
assertThat(window).hasSize(2);
205-
assertThat(window).containsOnly(john20, john40);
206196
}
207197

208198
@ParameterizedTest // GH-4308

0 commit comments

Comments
 (0)