[Relay] Pagination bug about previous/next

[Xuhao]

According to this reply, there is a bug in Relay Cursor Connections Specification

hasPreviousPage will be false if the client is not paginating with last
hasNextPage will be false if the client is not paginating with first

And the implementation here:

graphql-ruby/lib/graphql/relay/relation_connection.rb

Lines 19 to 25 in 756373a

	def has_next_page
	!!(first && paged_nodes && @has_next_page)
	end
	def has_previous_page
	!!(last && starting_offset > 0)
	end

So:
pageInfo.hasPreviousPage is always false when paging forwards, pageInfo.hasNextPage is always false when paging backwards

---

My env:
server side: graphql (1.5.5)
client side: react-relay@^0.10.0

My query to support pagination:

articles(first: $first, after: $afterCursor, last: $last, before: $beforeCursor)

when clicking next:

  goToNextPage = () => {
    this.props.relay.setVariables({
      first: pageSize,
      afterCursor: this.props.viewer.articles.pageInfo.endCursor,
      last: null,
      beforeCursor: null
    });
  };

when clicking previous:

  goToPrevPage = () => {
    this.props.relay.setVariables({
      last: pageSize,
      beforeCursor: this.props.viewer.articles.pageInfo.startCursor,
      first: null,
      afterCursor: null
    });
  };

[rmosolgo]

Can you help me understand the problem? It looks like some people requested a change to the Relay spec, but nobody has changed the spec yet. Is that right?

[Xuhao]

@rmosolgo This makes me strange, the spec still incorrect after more than a year the bug found. Although there is a issue #103 about this change request, and there should be a PR for that according:

If the ideas expressed here are of interest, I'd be honored to submit a PR.

But I can‘t found the PR yet.

[blevine]

It looks like the PR for the bug in the spec was never opened. It'd be nice of graphql-ruby could fix this in anticipation of the spec being corrected.

[blevine]

Note that I quickly worked around this by monkey-patching GraphQL::Relay::BaseConnection, ArrayConnection, RelationConnection so that the methods has_next_page and has_previous page do not check the first and last arguments respectively. e.g., in BaseConnection:

      def has_next_page
        sliced_nodes.count > first
      end

      def has_previous_page
        sliced_nodes.count > last
      end

[Xuhao]

@blevine I tried this patching, than server side works good.
But I use relay as graphql client, looks relay wrap server response with wrong connection spec and expose wrong result 😆

[blevine]

That's unfortunate. I hadn't tried this with an actual Relay client yet. What request were you making and what was the specific error you received?

[Xuhao]

@blevine
Go to 2rd page:

articles(first: 10, after: $afterCursor, last: null, before: null)

Graphql Server respond correct:

{
  data {
     viewer {
       articles {
          edges {
             ...
          }
          pageInfo {
            hasNextPage: true
            hasPreviousPage: true
          }
       }
     }
   }
}

But I debug in react component to found this.props.viewer.artices.pageInfo.hasPreviousPage is false, and that's wrong.

[Xuhao]

Also here is my flawed solution for react-relay:

render() {
   const { edges, pageInfo } = this.props.viewer.articles;
   const hasPreviousPage = pageInfo.startCursor !== 'MQ==';
   const hasNextPage = pageInfo.startCursor === 'MQ==' || edges.length === pageSize;
   return (
     <Pagination
       goToNextPage={this.goToNextPage}
       goToPrevPage={this.goToPrevPage}
       hasNextPage={hasNextPage}
       hasPreviousPage={hasPreviousPage}
     />
   );
}

[rmosolgo]

It sounds like this is spec-compliant, right? I'd be happy to support behavior this behavior but only if:

• Someone else implements it, and it's not the default (that is, default behavior is spec-compliant)
• This behavior becomes part of the Relay spec

Feel free to reopen this when one of those happens!

[mbajur]

@blevine (or anyone) - could you share the exact monkey-patched code you used to make it work? I'm not good at monkey patching and all my attempts are failing so obviously i'm doing something wrong, it would be really handy to see a full code sample here :)

---

Edit: Ok, i assume it's not a proper solution as, in my case, hasPrevPage is always true now. So that's a no-go.

[Xuhao]

@mbajur Here is simple patch if you use rails:

Create config/initializers/graphql.rb and contain:

require 'graphql'

module GraphQL
  module Relay
    class BaseConnection
      def has_next_page
        sliced_nodes.count > first
      end

      def has_previous_page
        sliced_nodes.count > last
      end
    end

    class RelationConnection
      def has_next_page
        sliced_nodes_count > first
      end

      def has_previous_page
        sliced_nodes_count > last
      end
    end
  end
end

[mbajur]

Unfortunatelly @Xuhao - your code makes my API forces me to use both first and last arguments and makes pageInfo always returns true for both hasNextPage and hasPreviousPage. When pagination reaches last page - both of them starts to return false. So it's still unusable as a pagination solution :(

[Xuhao]

@mbajur So you just test your API directly(via curl or graphiQL) ,but without any graphql client(like relay) right?

[mbajur]

Yeah, exactly, i'm using graphiql for it

[Xuhao]

@mbajur strange, it tried this patch before and was worked. Maybe new version made some changes on it.

[mbajur]

Out of curiosity - are you using both first and last arguments in parallel with thish patch?

[Dmitrii1994]

In my case, last is nil. Strange