Commit e2ad2749 authored by Brett Walker's avatar Brett Walker Committed by charlie ablett

Fix has_next_page and has_previous_page

for stable/keyset cursor pagination
parent 4412bcc0
---
title: GraphQL hasNextPage and hasPreviousPage return correct values
merge_request: 32476
author:
type: fixed
......@@ -32,6 +32,49 @@ module Gitlab
class Connection < GraphQL::Pagination::ActiveRecordRelationConnection
include Gitlab::Utils::StrongMemoize
# rubocop: disable Naming/PredicateName
# https://relay.dev/graphql/connections.htm#sec-undefined.PageInfo.Fields
def has_previous_page
strong_memoize(:has_previous_page) do
if after
# If `after` is specified, that points to a specific record,
# even if it's the first one. Since we're asking for `after`,
# then the specific record we're pointing to is in the
# previous page
true
elsif last
limited_nodes
!!@has_previous_page
else
# Key thing to remember. When `before` is specified (and no `last`),
# the spec says return _all_ edges minus anything after the `before`.
# Which means the returned list starts at the very first record.
# Then the max_page kicks in, and returns the first max_page items.
# Because of this, `has_previous_page` will be false
false
end
end
end
def has_next_page
strong_memoize(:has_next_page) do
if before
# If `before` is specified, that points to a specific record,
# even if it's the last one. Since we're asking for `before`,
# then the specific record we're pointing to is in the
# next page
true
elsif first
# If we count the number of requested items plus one (`limit_value + 1`),
# then if we get `limit_value + 1` then we know there is a next page
relation_count(set_limit(sliced_nodes, limit_value + 1)) == limit_value + 1
else
false
end
end
end
# rubocop: enable Naming/PredicateName
def cursor_for(node)
encoded_json_from_ordering(node)
end
......@@ -54,20 +97,32 @@ module Gitlab
# So we're ok loading them into memory here as that's bound to happen
# anyway. Having them ready means we can modify the result while
# rendering the fields.
@nodes ||= load_paged_nodes.to_a
@nodes ||= limited_nodes.to_a
end
private
def load_paged_nodes
if first && last
raise Gitlab::Graphql::Errors::ArgumentError.new("Can only provide either `first` or `last`, not both")
end
# Apply `first` and `last` to `sliced_nodes`
def limited_nodes
strong_memoize(:limited_nodes) do
if first && last
raise Gitlab::Graphql::Errors::ArgumentError.new("Can only provide either `first` or `last`, not both")
end
if last
# grab one more than we need
paginated_nodes = sliced_nodes.last(limit_value + 1)
if paginated_nodes.count > limit_value
# there is an extra node, so there is a previous page
@has_previous_page = true
paginated_nodes = paginated_nodes.last(limit_value)
end
else
paginated_nodes = sliced_nodes.limit(limit_value) # rubocop: disable CodeReuse/ActiveRecord
end
if last
sliced_nodes.last(limit_value)
else
sliced_nodes.limit(limit_value) # rubocop: disable CodeReuse/ActiveRecord
paginated_nodes
end
end
......@@ -82,6 +137,7 @@ module Gitlab
# rubocop: enable CodeReuse/ActiveRecord
def limit_value
# note: only first _or_ last can be specified, not both
@limit_value ||= [first, last, max_page_size].compact.min
end
......
......@@ -311,4 +311,96 @@ describe Gitlab::Graphql::Pagination::Keyset::Connection do
end
end
end
describe '#has_previous_page and #has_next_page' do
# using a list of 5 items with a max_page of 3
let_it_be(:project_list) { create_list(:project, 5) }
let_it_be(:nodes) { Project.order(:id) }
context 'when default query' do
let(:arguments) { {} }
it 'has no previous, but a next' do
expect(subject.has_previous_page).to be_falsey
expect(subject.has_next_page).to be_truthy
end
end
context 'when before is first item' do
let(:arguments) { { before: encoded_cursor(project_list.first) } }
it 'has no previous, but a next' do
expect(subject.has_previous_page).to be_falsey
expect(subject.has_next_page).to be_truthy
end
end
describe 'using `before`' do
context 'when before is the last item' do
let(:arguments) { { before: encoded_cursor(project_list.last) } }
it 'has no previous, but a next' do
expect(subject.has_previous_page).to be_falsey
expect(subject.has_next_page).to be_truthy
end
end
context 'when before and last specified' do
let(:arguments) { { before: encoded_cursor(project_list.last), last: 2 } }
it 'has a previous and a next' do
expect(subject.has_previous_page).to be_truthy
expect(subject.has_next_page).to be_truthy
end
end
context 'when before and last does not request all remaining nodes' do
let(:arguments) { { before: encoded_cursor(project_list.last), last: 2 } }
it 'has a previous and a next' do
expect(subject.has_previous_page).to be_truthy
expect(subject.has_next_page).to be_truthy
end
end
context 'when before and last does request all remaining nodes' do
let(:arguments) { { before: encoded_cursor(project_list[1]), last: 3 } }
it 'has a previous and a next' do
expect(subject.has_previous_page).to be_falsey
expect(subject.has_next_page).to be_truthy
expect(subject.nodes).to eq [project_list[0]]
end
end
end
describe 'using `after`' do
context 'when after is the first item' do
let(:arguments) { { after: encoded_cursor(project_list.first) } }
it 'has a previous, and a next' do
expect(subject.has_previous_page).to be_truthy
expect(subject.has_next_page).to be_truthy
end
end
context 'when after and first specified' do
let(:arguments) { { after: encoded_cursor(project_list.first), first: 2 } }
it 'has a previous and a next' do
expect(subject.has_previous_page).to be_truthy
expect(subject.has_next_page).to be_truthy
end
end
context 'when before and last does request all remaining nodes' do
let(:arguments) { { after: encoded_cursor(project_list[2]), last: 3 } }
it 'has a previous but no next' do
expect(subject.has_previous_page).to be_truthy
expect(subject.has_next_page).to be_falsey
end
end
end
end
end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment