PageInfo.hasPreviousPage is false on second page

[daironmichel]

Hi guys,

I'm using graphene==1.1.3 with graphene-django==1.2.1. I'm following the Relay specification and I'm having an issue with pagination on connections.

When paginating forward using the first and after arguments getting the first page is working fine but from the second page forward hasPreviousPage comes back false. For example, on a list of 20 users the endCursor for the first page of 5 items is "abc5". If I ask for the second page of 5 items:

query { users(first: 5, after: "abc5"){ 
  pageInfo {hasPreviousPage hasNextPage startCursor endCursor} 
}}

I get back:

{ "data": { "users": {
    "pageInfo": {
      "hasPreviousPage": false,    <-- This should be true
      "hasNextPage": true,
      "startCursor": "abc6",
       "endCursor": "abc10"
    }
}}}

Same thing happens when paginating backwards but with hasNextPage. When I ask for the ...(last: 5, before: "abc16") then hasNextPage comes back as false.

I think it's a bug on the connection_from_list_slice method in this file. And here is a fragment of the code where I think the problem is:

def connection_from_list_slice(list_slice, args=None, connection_type=None,
                               edge_type=None, pageinfo_type=None,
                               slice_start=0, list_length=0, list_slice_length=None):
    # ...

    first_edge_cursor = edges[0].cursor if edges else None
    last_edge_cursor = edges[-1].cursor if edges else None
    lower_bound = after_offset + 1 if after else 0
    upper_bound = before_offset if before else list_length

    return connection_type(
        edges=edges,
        page_info=pageinfo_type(
            start_cursor=first_edge_cursor,
            end_cursor=last_edge_cursor,
            has_previous_page=isinstance(last, int) and start_offset > lower_bound,
            has_next_page=isinstance(first, int) and end_offset < upper_bound
        )
    )

    # ...

[LexFrench]

I'm seeing the same issue as well even when not using the graphene-django plugin.

[chrisbrantley]

I have this problem as well. According to the spec it seems like this is expected behavior but for the life of me I can't understand why.

[syrusakbary]

I'm closing the issue here, as is more related on the spec than this implementation.
Feel free to open an issue in the GraphQL relay spec: https://github.com/facebook/relay

[janosroden]

Hi @syrusakbary, coud you please reopen this issue because the spec changed on 12 Sep 2017?

Current spec

[jkimbo]

Hi @daironmichel . We're currently going through old issues that appear to have gone stale (ie. not updated in about the last 3 months) to try and clean up the issue tracker. If this is still important to you please comment and we'll re-open this.

Thanks!

[janosroden]

Hi, I'd like to keep this issue open if you don't mind.
Thanks!

[djsmedes]

Though I find this fairly annoying, I do understand why the Relay spec allows this - it can apparently reduce server load, and the client doesn't need the server to tell it the missing info. When you're paginating, the client knows there is a previous page if you just clicked "next"; you must have clicked "next" from somewhere. Implementation details:

• Maintain local state for hasNext and hasPrev with 3 possible values, true, false, and null in addition to the results of your GraphQL query
• Have some kind of function that first uses hasX local state if it's non-null, otherwise checking the result of the GraphQL query
• When you trigger your GraphQL query to get the next page, set local hasPrev to true (we know there's a previous, since you must have clicked "next" from somewhere) and hasNext to null (so that our function above uses the result of the query, which must contain hasNextPage according to the Relay spec). When you query the previous page, set hasPrev to null and hasNext to true.

Here's a concrete example, using Vue and Vue-Apollo:

export default {
  apollo: {
    myQuery: {
      query: gql`
        query myPaginatedQuery(
          $first: Int
          $last: Int
          $after: String
          $before: String
        ) {
          myQuery(
            first: $first
            after: $after
            last: $last
            before: $before
          ) {
            edges {
              node {
                uuid
                name
              }
            }
            pageInfo {
              hasNextPage
              hasPreviousPage
              startCursor
              endCursor
            }
          }
        }
      `,
      variables() {
        return {
          first: this.itemsPerPage,
          after: null,
        };
      },
    },
  },
  data() {
    return {
      itemsPerPage: 10,
      hasNextPage: null,
      hasPrevPage: null,
    };
  },
  computed: {
    // use these to determine whether there is actually a previous or next page
    canNextPage() {
      if (this.hasNextPage !== null) {
        return this.hasNextPage;
      } else if (this.myQuery) {
        return this.myQuery.pageInfo.hasNextPage;
      } else {
        return false;
      }
    },
    canPrevPage() {
      if (this.hasPrevPage !== null) {
        return this.hasPrevPage;
      } else if (this.myQuery) {
        return this.myQuery.pageInfo.hasPreviousPage;
      } else {
        return false;
      }
    },
  },
  methods: {
    async showMore(previous = false) {
      await this.$apollo.queries.myQuery.fetchMore({
        variables: previous
          ? {
              last: this.itemsPerPage,
              before: this.combatantSet.pageInfo.startCursor,
            }
          : {
              first: this.itemsPerPage,
              after: this.combatantSet.pageInfo.endCursor,
            },
        updateQuery: (priorResult, { fetchMoreResult }) => fetchMoreResult,
      });
      if (previous) {
        this.hasPrevPage = null;
        this.hasNextPage = true;
      } else {
        this.hasPrevPage = true;
        this.hasNextPage = null;
      }
    },
  },
};

[tony]

Related: graphql-python/graphql-relay-py#14, facebook/relay#2079

[tony]

@jkimbo Can you keep this open? I don't believe this is fixed.

[kkinder]

I'm also a little flummoxed. The spec reads:

HasPreviousPage(allEdges, before, after, first, last)

1. If last is set:
   a. Let edges be the result of calling ApplyCursorsToEdges(allEdges, before, after).
   b. If edges contains more than last elements return true, otherwise false.
2. If after is set:
   a. If the server can efficiently determine that elements exist prior to after, return true.
3. Return false.

In this instance, after is set and there's really no reason the server can't efficiently determine that a prior page exists. At the very least, it should be an option on one's Connection class to spend the extra few cycles to look backwards in the index.

I think for a lot of purposes, it would make sense for hasPreviousPage to report on whether the server has a previous page, and to my reading of the spec, while that's not required, it is allowed.