Merge branch '218340-graphql-haspreviouspage-and-hasnextpage' into 'master'

GraphQL: hasPreviousPage and hasNextPage not working consistently

Closes #218340

See merge request gitlab-org/gitlab!32476

changelogs/unreleased/218340-graphql-haspreviouspage-and-hasnextpage.yml

+---
+title: GraphQL hasNextPage and hasPreviousPage return correct values
+merge_request: 32476
+author:
+type: fixed

lib/gitlab/graphql/pagination/keyset/connection.rb

@@ -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
 

spec/lib/gitlab/graphql/pagination/keyset/connection_spec.rb

@@ -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