Default ScrollSubrange is not applied correctly for backwards scrolling in QueryDSL and QBE

[mspiess]

Example:
There is a Entity called Document.
Schema:

type Query {
  documents(first: Int, last: Int): DocumentConnection
}

A query for document returns the same data, regardless of whether first or last is provided.

The culprit:
QuerydslDataFetcher#ScrollableEntityFetcher.getResult uses the limit from the subrange that is build out of the arguments. If a cursor is provided using before or after it also uses the built subrange's position, which has the intended direction. However in the case where there is no cursor provided, it uses the default subrange's position, overriding the provided direction.

@SuppressWarnings("OptionalGetWithoutIsPresent")
@Override
protected Iterable<R> getResult(FetchableFluentQuery<R> queryToUse, DataFetchingEnvironment env) {
	ScrollSubrange subrange = buildScrollSubrange(env);
	int limit = subrange.count().orElse(this.defaultSubrange.count().getAsInt());
	ScrollPosition position = subrange.position().orElse(this.defaultSubrange.position().get());
	return queryToUse.limit(limit).scroll(position);
}

[rstoyanchev]

Thanks for the report. ScrollSubrange.create always uses forward pagination for offset scrolling, switching fro backward to forward if necessary. So we need to apply defaults earlier, before the call to create.

[rstoyanchev]

@mspiess thanks for the report again. There is now a fix available in 1.2.5-SNAPSHOT if you'd like to give that a try.

[mspiess]

Hi, @rstoyanchev,
I would like to re-open this issue. @jokuni tried the snapshot and found it didn't fix the issue.
This tracks with my confusion over

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

Line 91 in b727854

assertThat(range.forward()).isTrue();

Shouldn't that line assert forward to be false?

[jokuni]

For example, given following Entity:

Todo

id	content
1	Todo 1
2	Todo 2

Associated query:

query {
   todos(last: 1) {
      ...
   }
}

Actual response:

{
   "data": {
      "todos": {
         "edges": [
            {
               "node": {
                  "id": 1,
                  "content": "Todo 1"
               }
            }
         ]
      }
   }
}

Expected response:

{
   "data": {
      "todos": {
         "edges": [
            {
               "node": {
                  "id": 2,
                  "content": "Todo 2"
               }
            }
         ]
      }
   }
}

[rstoyanchev]

Offset Scrolling works by getting results beginning at an offset, and so there is no concept of going back. Instead we we advance the index back and turn backward to forward.

For example, given request for the last 5 relative to 50, we create OffsetScrollPosition with offset 44, count 5, and forward=true. That's the last 5 items before 50. This is why forward=true in all test cases.

On further thought, I don't see a straight forward way to support last N requests without a before cursor as offset scrolling can only return results forward from a specific offset. There was a real improvement made for this issue to apply correctly default ScrollSubrange but that has to have a position and by default it is 20, from the beginning.

Thinking about what could be done, perhaps we could reverse the Sort order if defined so that the first N from the beginning is effectively the last N. What do you think? /cc @mp911de

[mp911de]

Requesting the last n items without providing a cursor could work in the sense of returning the last n items from the end of the result. That would require us to fetch all query results and then return a sublist of result.sublist(result.size() - last, result.size()).

In other words, reversing the sort order (if a sort order is provided), fetching the first n and then reversing the result list would be an optimization.