Spec violation in connectionFromArraySlice

[nemanja-stanarevic]

Hi,

It looks like there is a spec violation in connectionFromArraySlice implementation. Specifically, spec of the Relay pagination algorithm (4.3 / ApplyCursorsToEdges / Step 2.2) indicates that elements should be removed from the slice only if afterEdge exists (if afterEdge does not exist, allEdges are returned) [1]. Here is a counter example:

connectionFromArraySlice(
  [1, 2, 3, 4, 5, 6],
  { first: 6, after: 'YXJyYXljb25uZWN0aW9uOjEw' },
  { sliceStart: 0, arrayLength: 6 } )

This returns empty edges array. Argument after in this case is base64 encoded "arrayconnection:10" string. Clearly, edge corresponding to that cursor doesn't exist, so connectionFromArraySlice should return all six nodes in the edges array as per the spec.

There is no symmetrical issue with last / before combination, because endOffset is set to min of beforeOffset and array length.

Just wanted to confirm that my read of the spec makes sense to you all before sending a pull request with the fix.

Thanks,
Nemanja

[1] Spec: https://facebook.github.io/relay/graphql/connections.htm#ApplyCursorsToEdges()

[dschafer]

Yeah, we should probably treat that cursor the same way we would a malformed one. Good catch.

There is no symmetrical issue with last / before combination, because endOffset is set to min of beforeOffset and array length.

Hm, I'd expect that

connectionFromArraySlice(
[1, 2, 3, 4, 5, 6],
{ last: 6, before: base64('arrayconnection:-5') },
{ sliceStart: 0, arrayLength: 6 } )

would have similar issues? It would see -5 as the offset and return nothing?

[nemanja-stanarevic]

yup, spot on, negative offsets bug out as well. I was thinking of cursor > length case in which case last / before is okay. I'll pull together a patch for both along with a couple of tests and send them in. Thanks

[idibidiart]

Any progress on this?