Ensure endCursor is NULL when hasNextPage is FALSE in GraphQL response

[aaronburtle]

Why make this change?

Closes #2030

Closes #2130

Fixes a bug where we would have a non null endCursor in GraphQL responses, despite hasNextPage being false. In other words, providing a bookmark to the next page of results when no such page exists.

And also, when we do not include hasNextPage but we are still on the last page, endCursor should be null.

What is this change?

We now check if we have an extra element or not before we create the endCursor. We define having a next page of results as having an extra element in the result set that is returned from our query. This is because when we make a paginated query we add 1 to our limit, that way we can tell if there is another page of results based on if that extra element returns or not. We now include this logic if either hasNextPage, or endCursor is in the request so that we will correctly follow the logic for any pagination scenario. That means adding 1 to the limit and checking if there is another page of records beyond the number in the request.

How was this tested?

Updated the pagination tests to have null endCursor expected when hasNextPage is false.

Added cases to the RequestAfterTokenOnly test cases to include both being on the final page and not, this verifies we get a correctly built non null and correctly built null endCursor for those respective cases.

Sample Request(s)

{
	reviews(first: 2, after: "W3siRW50aXR5TmFtZSI6IlJldmlld3MiLCJGaWVsZE5hbWUiOiJib29rX2lkIiwiRmllbGRWYWx1ZSI6MSwiRGlyZWN0aW9uIjowfSx7IkVudGl0eU5hbWUiOiJSZXZpZXdzIiwiRmllbGROYW1lIjoiaWQiLCJGaWVsZFZhbHVlIjo1NjcsIkRpcmVjdGlvbiI6MH1d") {
		items {
			id
            content
        }
        hasNextPage
        endCursor
    }
}

Before the fix, this request would result in a response that would include an endCursor to a page that does not exist.

  "data": {
    "reviews": {
      "items": [
        {
          "id": 568,
          "content": "I loved it"
        },
        {
          "id": 569,
          "content": "best book I read in years"
        }
      ],
      "hasNextPage": false,
      "endCursor": "W3siRW50aXR5TmFtZSI6IlJldmlldyIsIkZpZWxkTmFtZSI6ImJvb2tfaWQiLCJGaWVsZFZhbHVlIjoxLCJEaXJlY3Rpb24iOjB9LHsiRW50aXR5TmFtZSI6IlJldmlldyIsIkZpZWxkTmFtZSI6ImlkIiwiRmllbGRWYWx1ZSI6NTY5LCJEaXJlY3Rpb24iOjB9XQ=="
    }
  }
}

W3siRW50aXR5TmFtZSI6IlJldmlldyIsIkZpZWxkTmFtZSI6ImJvb2tfaWQiLCJGaWVsZFZhbHVlIjoxLCJEaXJlY3Rpb24iOjB9LHsiRW50aXR5TmFtZSI6IlJldmlldyIsIkZpZWxkTmFtZSI6ImlkIiwiRmllbGRWYWx1ZSI6NTY5LCJEaXJlY3Rpb24iOjB9XQ== decodes to [{"EntityName":"Review","FieldName":"book_id","FieldValue":1,"Direction":0},{"EntityName":"Review","FieldName":"id","FieldValue":569,"Direction":0}] and if we look at the data in our table we can see that an endCursor that indicates the next page of data starts after id=569 will not be providing a link to a page at all as there are no more records after id=569

After adding this fix we can see that instead we will have a null endCursor, as expected

  "data": {
    "reviews": {
      "items": [
        {
          "id": 568,
          "content": "I loved it"
        },
        {
          "id": 569,
          "content": "best book I read in years"
        }
      ],
      "hasNextPage": false,
      "endCursor": null
    }
  }
}

Similar behavior can be seen for only request an endCursor but not including hasNextPage. If you simply remove hasNextPage from the above requests, you will see the same inconsistent behavior before the fix, and the same consistency now after the fix.

So looking at the request

{
	reviews(first: 2, after: "W3siRW50aXR5TmFtZSI6IlJldmlld3MiLCJGaWVsZE5hbWUiOiJib29rX2lkIiwiRmllbGRWYWx1ZSI6MSwiRGlyZWN0aW9uIjowfSx7IkVudGl0eU5hbWUiOiJSZXZpZXdzIiwiRmllbGROYW1lIjoiaWQiLCJGaWVsZFZhbHVlIjo1NjcsIkRpcmVjdGlvbiI6MH1d") {
		items {
			id
            content
        }
        endCursor
    }
}

Before the fix we would see

  "data": {
    "reviews": {
      "items": [
        {
          "id": 568,
          "content": "I loved it"
        },
        {
          "id": 569,
          "content": "best book I read in years"
        }
      ],
      "endCursor": "W3siRW50aXR5TmFtZSI6IlJldmlldyIsIkZpZWxkTmFtZSI6ImJvb2tfaWQiLCJGaWVsZFZhbHVlIjoxLCJEaXJlY3Rpb24iOjB9LHsiRW50aXR5TmFtZSI6IlJldmlldyIsIkZpZWxkTmFtZSI6ImlkIiwiRmllbGRWYWx1ZSI6NTY5LCJEaXJlY3Rpb24iOjB9XQ=="
    }
  }
}

But after the fix we have

  "data": {
    "reviews": {
      "items": [
        {
          "id": 568,
          "content": "I loved it"
        },
        {
          "id": 569,
          "content": "best book I read in years"
        }
      ],
      "endCursor": null
    }
  }
}

[seantleonard]

can you show example of contents of last page before and after your change?

[seantleonard]

Few questions. Can you also point out in PR description some test that exercise pagination to the last page? so we can look locally to see what the response contents as a result of this change?

[aaronburtle]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[seantleonard]

Need to prevent the regression introduced and add a test to protect against it.

[aaronburtle]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[seantleonard]

for your pr description:

We not include this logic if either hasNextPage, or endCursor is in the request so that we will correctly follow the logic for any pagination scenario.

do you mean that "this logic" for adding to the limit() value is excluded when both hasNextPage and endCursor are excluded from the GraphQL request?

Does this change look to affect REST API at all, with your latest commits?

Would be good to include a copy/paste of the example request/response for this:

Similar behavior can be seen for only request an endCursor but not including hasNextPage. If you simply remove hasNextPage from the above requests, you will see the same inconsistent behavior before the fix, and the same consistency now after the fix.

[aaronburtle]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[aaronburtle]

for your pr description:

We not include this logic if either hasNextPage, or endCursor is in the request so that we will correctly follow the logic for any pagination scenario.

do you mean that "this logic" for adding to the limit() value is excluded when both hasNextPage and endCursor are excluded from the GraphQL request?

Yes, I added that to the PR description now to be more clear.

Does this change look to affect REST API at all, with your latest commits?

No. Nothing on the REST side is changing.

Would be good to include a copy/paste of the example request/response for this:

Similar behavior can be seen for only request an endCursor but not including hasNextPage. If you simply remove hasNextPage from the above requests, you will see the same inconsistent behavior before the fix, and the same consistency now after the fix.

Included!

[aaronburtle]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[aaronburtle]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[seantleonard]

Thank you for making these changes, adding tests for coverage of both bugs identified and having pr description outlining all context and before/after

[aaronburtle]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[severussundar]

Thanks for making these changes. This will help bring consistency in the value of endCursor across Cosmos and SQL database types, GraphQL and REST.

[seantleonard]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[aaronburtle]

connection issue caused test to fail, re-running

[aaronburtle]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).