Offset pagination with "last N before X" should not include the item at offset X

[nilshartmann]

When executing a query with a ScrollSubrange that contains both last and before the node with the before cursor is returned in the result.

query {
  posts(last:2, before:"T18z") {
    edges {
      cursor  # contains T18z in the result, and not the 2 nodes BEFORE T18z
    }
  }
}

  @QueryMapping
  public Window<Post> posts(ScrollSubrange subrange) {
      ScrollPosition position = subrange.position().orElse(ScrollPosition.offset());
      int count = subrange.count().orElse(5);

      var result = postRepository.findAllBy(position, Limit.of(count), Sort.by("id"));

      return result;
  }

I think the before node should not be included in the result (similar to first X after where after is also not included in the result). The graphql.relay.SimpleListConnection class also do not return the before node.

I tested with Spring Boot 3.2.0-M3 and used the out-of-the-box configuration (no changes to wiring etc.)

[rstoyanchev]

I presume this is with an OffsetScrollPosition which only advances forward and we use advanceBy(-count). That should advance back by one more in order to return items up to but not including the items at the offset.

KeysetScrollPostition supports backward pagination so there it's all Spring Data.

[Leigh-at-Atlassian]

I've just updated to 1.2.4 to make use of this fix, but it appears that some of the logic still does not match what is expected from the Relay last and before arguments.

Rather than immediately raise a new issue, I'll post the question here for clarification.

Consider last: 2, before: <cursor for index 3> (ie. ScrollSubrange.create(ScrollPosition.offset(3), 2, false)).
This produces a ScrollSubrange with offset 0 and count 2 (as expected).

However, last: 3, before: <cursor for index 3> (ie. ScrollSubrange.create(ScrollPosition.offset(3), 3, false)) produces the exactly the same ScrollSubrange (not expected). I would perhaps expect something like a ScrollSubrange with null position and count of 3.

This behaviour also occurs for page sizes beyond the start of the list: last: 5, before: <cursor for index 3>.

This test below (Kotlin) shows a comparison with the Relay SimpleListConnection:

    @Test
    fun `test backward pagination to start of list`() {
        // last: 3, before: index[3]
        val env = Mockito.mock(DataFetchingEnvironment::class.java)
        whenever(env.getArgument<Int>("last")).thenReturn(3)
        whenever(env.getArgument<String>("before")).thenReturn(createCursor(3))
        val connection = SimpleListConnection((0..10).toList()).get(env)
        Assertions.assertThat(connection.edges).hasSize(3)
        Assertions.assertThat(connection.edges[0].node).isEqualTo(0)
        Assertions.assertThat(connection.edges[1].node).isEqualTo(1)
        Assertions.assertThat(connection.edges[2].node).isEqualTo(2)

        // last: 2, before: index[3]
        val okRange = ScrollSubrange.create(ScrollPosition.offset(3), 2, false)
        Assertions.assertThat(okRange.count()).isEqualTo(OptionalInt.of(2))
        Assertions.assertThat(okRange.position()).isEqualTo(Optional.of(ScrollPosition.offset()))

        // last: 3, before: index[3]
        val brokenRange = ScrollSubrange.create(ScrollPosition.offset(3), 3, false)
        Assertions.assertThat(brokenRange.count()).isEqualTo(OptionalInt.of(3)) // FAIL
        Assertions.assertThat(brokenRange.position()).isEqualTo(Optional.empty<ScrollPosition>()) // FAIL

        // last: 5, before: index[3]
        val brokenRange2 = ScrollSubrange.create(ScrollPosition.offset(3), 5, false)
        Assertions.assertThat(brokenRange2.count()).isEqualTo(OptionalInt.of(3)) // FAIL
        Assertions.assertThat(brokenRange2.position()).isEqualTo(Optional.empty<ScrollPosition>()) // FAIL
    }

    // From SimpleListConnection.java
    private fun createCursor(offset: Int): String {
        val bytes: ByteArray = ("simple-cursor$offset").toByteArray(StandardCharsets.UTF_8)
        return Base64.getEncoder().encodeToString(bytes)
    }

[rstoyanchev]

I've created #873. Thanks for pointing this out @Leigh-at-Atlassian.

[Leigh-at-Atlassian]

I've created #873. Thanks for pointing this out @Leigh-at-Atlassian.

Thank you @rstoyanchev 👍

[rstoyanchev]

@nilshartmann after some follow-up issues, I've realized that offset based scrolling is inclusive of the index while the Relay spec defines it to be exclusive. So I've had to make changes under #916 to adapt offsets accordingly.

That means I no longer understand the cause for the issue you described here, and I will need your help with more details, if you still observe the problem after the latest changes for which a snapshot is now available.