Skip to content

Check for invalid SimpleListConnection list indices on client-provided cursor#112

Merged
andimarek merged 2 commits into
graphql-java:masterfrom
dalenavi:master
May 20, 2017
Merged

Check for invalid SimpleListConnection list indices on client-provided cursor#112
andimarek merged 2 commits into
graphql-java:masterfrom
dalenavi:master

Conversation

@dalenavi

@dalenavi dalenavi commented Mar 21, 2016

Copy link
Copy Markdown
Contributor

Scenario to reproduce:
Client requests first 3 edges of relay list of list total length 4 at time t.

At time t+1,
underlying list is mutated at data source, list becomes 2 items shorter.
Length of edge list is now 2, but GraphQL client thinks list is length 4.

At time t+2,
Client requests some more edges specifying parameter after as 3

java.lang.IllegalArgumentException: fromIndex(3) > toIndex(2)
        at java.util.ArrayList.subListRangeCheck(ArrayList.java:1006)
        at java.util.ArrayList.subList(ArrayList.java:996)
        at graphql.relay.SimpleListConnection.get(SimpleListConnection.java:42)
        at GraphQL.DataFetchers.lambda$getPlaylistGroupVideosDataFetcher$20(DataFetchers.java:365)
        at graphql.execution.ExecutionStrategy.resolveField(ExecutionStrategy.java:46)
        at graphql.execution.SimpleExecutionStrategy.execute(SimpleExecutionStrategy.java:18)
        at graphql.execution.ExecutionStrategy.completeValue(ExecutionStrategy.java:92)
        at graphql.execution.ExecutionStrategy.resolveField(ExecutionStrategy.java:52)
        at graphql.execution.SimpleExecutionStrategy.execute(SimpleExecutionStrategy.java:18)
        at graphql.execution.Execution.executeOperation(Execution.java:69)
        at graphql.execution.Execution.execute(Execution.java:42)
        at graphql.GraphQL.execute(GraphQL.java:78)
        at graphql.GraphQL.execute(GraphQL.java:55)

@dalenavi dalenavi changed the title Check for invalid list indices on client-provided cursor Check for invalid SimpleListConnection list indices on client-provided cursor Mar 21, 2016
@andimarek

andimarek commented Apr 4, 2016

Copy link
Copy Markdown
Member

Thanks for this PR!
Is this case specified somewhere (by Relay) how this should be handled? I mean is this even "legal" that the number of edges is changed?

@dalenavi

dalenavi commented Apr 7, 2016

Copy link
Copy Markdown
Contributor Author

By the spec, 'before' or 'after' should be a cursor type, an opaque node ID, a string. Not an array index.

To enable forward pagination, two arguments are required.
 * first takes an integer.
 * after takes the cursor type as described in the cursor field section.

Forward pagination arguments

An “Edge Type” must contain a field called cursor. This field must return a type that serializes as a String
...
Whatever type this field returns will be referred to as the cursor type in the rest of this spec.

Cursor type

Let afterEdge be the edge in edges whose cursor is equal to the after argument.
  If afterEdge exists:
    Remove all elements of edges before and including afterEdge.

Pagination algorithm

@dalenavi

dalenavi commented Apr 8, 2016

Copy link
Copy Markdown
Contributor Author

I think I'm off track with the above references...

I had the impression the spec suggested using a node id as the cursor.
Maybe using the array offset as cursor is also a valid cursor type.
I see now the array index cursor is still encoded as an opaque node id...

Doesn't really change the question though.

If a client is requesting some List resource using Relay pagination,
and the underlying List changes between calls,
then this situation could occur.

What possible way to prevent it? Can't cache the list-as-it-was-when-first-requested indefinitely.

@exbe exbe self-requested a review January 27, 2017 05:18
@andimarek andimarek merged commit b638274 into graphql-java:master May 20, 2017
andimarek added a commit that referenced this pull request May 20, 2017
@andimarek
merge PR #112
b5bb623
@andimarek

andimarek commented May 20, 2017

Copy link
Copy Markdown
Member

@dalenavi It took some time to merge it, but is is finally it. :-) Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@exbe exbe Awaiting requested review from exbe

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@dalenavi @andimarek