Teach keyset_connection multiple attribute order

by changing the cursor into json and
handling a NULL condition correctly.

ee/app/models/design_management/design.rb

@@ -46,7 +46,7 @@ module DesignManagement
       join = designs.join(actions)
         .on(actions[:design_id].eq(designs[:id]))
 
-      joins(join.join_sources).where(actions[:event].not_eq(deletion))
+      joins(join.join_sources).where(actions[:event].not_eq(deletion)).order(:id)
     end
 
     # Scope called by our REST API to avoid N+1 problems

lib/gitlab/graphql/connections.rb

@@ -6,7 +6,7 @@ module Gitlab
       def self.use(_schema)
         GraphQL::Relay::BaseConnection.register_connection_implementation(
           ActiveRecord::Relation,
-          Gitlab::Graphql::Connections::KeysetConnection
+          Gitlab::Graphql::Connections::Keyset::Connection
         )
       end
     end

lib/gitlab/graphql/connections/keyset/conditions/base_condition.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Graphql
+    module Connections
+      module Keyset
+        module Conditions
+          class BaseCondition
+            def initialize(arel_table, names, values, operator, before_or_after)
+              @arel_table, @names, @values, @operator, @before_or_after = arel_table, names, values, operator, before_or_after
+            end
+
+            def build
+              raise NotImplementedError
+            end
+
+            private
+
+            attr_reader :arel_table, :names, :values, :operator, :before_or_after
+
+            def assemble_conditions(conditions)
+              conditions.join
+            end
+
+            def table_condition(attribute, value, operator)
+              case operator
+              when '>'
+                arel_table[attribute].gt(value)
+              when '<'
+                arel_table[attribute].lt(value)
+              when '='
+                arel_table[attribute].eq(value)
+              when 'is_null'
+                arel_table[attribute].eq(nil)
+              when 'is_not_null'
+                arel_table[attribute].not_eq(nil)
+              end
+            end
+          end
+        end
+      end
+    end
+  end
+end

lib/gitlab/graphql/connections/keyset/conditions/not_null_condition.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Graphql
+    module Connections
+      module Keyset
+        module Conditions
+          class NotNullCondition < BaseCondition
+            def build
+              conditions = []
+              conditions << first_attribute_condition
+
+              # If there is only one order field, we can assume it
+              # does not contain NULLs, and don't need additional
+              # conditions
+              unless names.count == 1
+                conditions << second_attribute_condition
+                conditions << final_condition
+              end
+
+              assemble_conditions(conditions)
+            end
+
+            private
+
+            # ex: "(relative_position > 23)"
+            def first_attribute_condition
+              <<~SQL
+                (#{table_condition(names.first, values.first, operator.first).to_sql})
+              SQL
+            end
+
+            # ex: " OR (relative_position = 23 AND id > 500)"
+            def second_attribute_condition
+              condition = <<~SQL
+                OR (
+                  #{table_condition(names.first, values.first, '=').to_sql}
+                  AND
+                  #{table_condition(names[1], values[1], operator[1]).to_sql}
+                )
+              SQL
+
+              condition
+            end
+
+            # ex: " OR (relative_position IS NULL)"
+            def final_condition
+              if before_or_after == :after
+                <<~SQL
+                  OR (#{table_condition(names.first, nil, 'is_null').to_sql})
+                SQL
+              end
+            end
+          end
+        end
+      end
+    end
+  end
+end

lib/gitlab/graphql/connections/keyset/conditions/null_condition.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Graphql
+    module Connections
+      module Keyset
+        module Conditions
+          class NullCondition < BaseCondition
+            def build
+              conditions = []
+              conditions << first_attribute_condition
+              conditions << final_condition
+
+              assemble_conditions(conditions)
+            end
+
+            private
+
+            # ex: "(relative_position IS NULL AND id > 500)"
+            def first_attribute_condition
+              condition = <<~SQL
+                (
+                  #{table_condition(names.first, nil, 'is_null').to_sql}
+                  AND
+                  #{table_condition(names[1], values[1], operator[1]).to_sql}
+                )
+              SQL
+
+              condition
+            end
+
+            # ex: " OR (relative_position IS NOT NULL)"
+            def final_condition
+              if before_or_after == :before
+                <<~SQL
+                  OR (#{table_condition(names.first, nil, 'is_not_null').to_sql})
+                SQL
+              end
+            end
+          end
+        end
+      end
+    end
+  end
+end

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

+# frozen_string_literal: true
+
+# Keyset::Connection provides cursor based pagination, to avoid using OFFSET.
+# It basically sorts / filters using WHERE sorting_value > cursor.
+# We do this for performance reasons (https://gitlab.com/gitlab-org/gitlab-ce/issues/45756),
+# as well as for having stable pagination
+# https://graphql-ruby.org/pro/cursors.html#whats-the-difference
+# https://coderwall.com/p/lkcaag/pagination-you-re-probably-doing-it-wrong
+#
+# It currently supports sorting on two columns, but the last column must
+# be the primary key. For example
+#
+#   Issue.order(created_at: :asc).order(:id)
+#   Issue.order(due_date: :asc).order(:id)
+#
+# It will tolerate non-attribute ordering, but only attributes determine the cursor.
+# For example, this is legitimate:
+#
+#   Issue.order('issues.due_date IS NULL').order(due_date: :asc).order(:id)
+#
+# but anything more complex has a chance of not working.
+#
+module Gitlab
+  module Graphql
+    module Connections
+      module Keyset
+        class Connection < GraphQL::Relay::BaseConnection
+          include Gitlab::Utils::StrongMemoize
+
+          def cursor_from_node(node)
+            encoded_json_from_ordering(node)
+          end
+
+          def sliced_nodes
+            @sliced_nodes ||=
+              begin
+                OrderInfo.validate_ordering(ordered_nodes, order_list)
+
+                sliced = ordered_nodes
+                sliced = slice_nodes(sliced, before, :before) if before.present?
+                sliced = slice_nodes(sliced, after, :after) if after.present?
+
+                sliced
+              end
+          end
+
+          def paged_nodes
+            # These are the nodes that will be loaded into memory for rendering
+            # 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.
+            @paged_nodes ||= load_paged_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
+
+            if last
+              sliced_nodes.last(limit_value)
+            else
+              sliced_nodes.limit(limit_value) # rubocop: disable CodeReuse/ActiveRecord
+            end
+          end
+
+          # rubocop: disable CodeReuse/ActiveRecord
+          def slice_nodes(sliced, encoded_cursor, before_or_after)
+            decoded_cursor = ordering_from_encoded_json(encoded_cursor)
+            builder = QueryBuilder.new(arel_table, order_list, decoded_cursor, before_or_after)
+            ordering = builder.conditions
+
+            sliced.where(*ordering).where.not(id: decoded_cursor['id'])
+          end
+          # rubocop: enable CodeReuse/ActiveRecord
+
+          def limit_value
+            @limit_value ||= [first, last, max_page_size].compact.min
+          end
+
+          def ordered_nodes
+            strong_memoize(:order_nodes) do
+              list = OrderInfo.build_order_list(nodes)
+
+              # ensure there is a primary key ordering
+              if list&.last&.attribute_name != nodes.primary_key
+                nodes.order(arel_table[nodes.primary_key].desc) # rubocop: disable CodeReuse/ActiveRecord
+              else
+                nodes
+              end
+            end
+          end
+
+          def order_list
+            strong_memoize(:order_list) do
+              OrderInfo.build_order_list(ordered_nodes)
+            end
+          end
+
+          def arel_table
+            nodes.arel_table
+          end
+
+          # Storing the current order values in the cursor allows us to
+          # make an intelligent decision on handling NULL values.
+          # Otherwise we would either need to fetch the record first,
+          # or fetch it in the SQL, significantly complicating it.
+          def encoded_json_from_ordering(node)
+            ordering = { 'id' => node[:id].to_s }
+
+            order_list.each do |field|
+              field_name = field.attribute_name
+              ordering[field_name] = node[field_name].to_s
+            end
+
+            encode(ordering.to_json)
+          end
+
+          def ordering_from_encoded_json(cursor)
+            JSON.parse(decode(cursor))
+          rescue JSON::ParserError
+            # for the transition period where a client might request using an
+            # old style cursor.  Once removed, make it an error:
+            #   raise Gitlab::Graphql::Errors::ArgumentError, "Please provide a valid cursor"
+            # TODO can be removed in next release
+            # https://gitlab.com/gitlab-org/gitlab/issues/32933
+            field_name = order_list.first.attribute_name
+
+            { field_name => decode(cursor) }
+          end
+        end
+      end
+    end
+  end
+end

lib/gitlab/graphql/connections/keyset/order_info.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Graphql
+    module Connections
+      module Keyset
+        class OrderInfo
+          def initialize(order_value)
+            @order_value = order_value
+          end
+
+          def attribute_name
+            order_value.expr.name
+          end
+
+          def operator_for(before_or_after)
+            case before_or_after
+            when :before
+              sort_direction == :asc ? '<' : '>'
+            when :after
+              sort_direction == :asc ? '>' : '<'
+            end
+          end
+
+          # Only allow specific node types.  For example ignore String nodes
+          def self.build_order_list(relation)
+            order_list = relation.order_values.select do |value|
+              value.is_a?(Arel::Nodes::Ascending) || value.is_a?(Arel::Nodes::Descending)
+            end
+
+            order_list.map { |info| OrderInfo.new(info) }
+          end
+
+          def self.validate_ordering(relation, order_list)
+            if order_list.empty?
+              raise ::ArgumentError.new('A minimum of 1 ordering field is required')
+            end
+
+            if order_list.count > 2
+              raise Gitlab::Graphql::Errors::ArgumentError.new('A maximum of 2 ordering fields are allowed')
+            end
+
+            # make sure the last ordering field is non-nullable
+            attribute_name = order_list.last&.attribute_name
+
+            if relation.columns_hash[attribute_name].null
+              raise Gitlab::Graphql::Errors::ArgumentError.new("Column `#{attribute_name}` must not allow NULL")
+            end
+
+            if order_list.last.attribute_name != relation.primary_key
+              raise Gitlab::Graphql::Errors::ArgumentError.new("Last ordering field must be the primary key, `#{relation.primary_key}`")
+            end
+          end
+
+          private
+
+          attr_reader :order_value
+
+          def sort_direction
+            order_value.direction
+          end
+        end
+      end
+    end
+  end
+end

lib/gitlab/graphql/connections/keyset/query_builder.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Graphql
+    module Connections
+      module Keyset
+        class QueryBuilder
+          def initialize(arel_table, order_list, decoded_cursor, before_or_after)
+            @arel_table, @order_list, @decoded_cursor, @before_or_after = arel_table, order_list, decoded_cursor, before_or_after
+
+            if order_list.empty?
+              raise Gitlab::Graphql::Errors::ArgumentError.new('No ordering scopes have been supplied')
+            end
+          end
+
+          # Based on whether the main field we're ordering on is NULL in the
+          # cursor, we can more easily target our query condition.
+          # We assume that the last ordering field is unique, meaning
+          # it will not contain NULLs.
+          # We currently only support two ordering fields.
+          #
+          # Example of the conditions for
+          #   relation: Issue.order(relative_position: :asc).order(id: :asc)
+          #   after cursor: relative_position: 1500, id: 500
+          #
+          #   when cursor[relative_position] is not NULL
+          #
+          #       ("issues"."relative_position" > 1500)
+          #       OR (
+          #         "issues"."relative_position" = 1500
+          #         AND
+          #         "issues"."id" > 500
+          #       )
+          #       OR ("issues"."relative_position" IS NULL)
+          #
+          #   when cursor[relative_position] is NULL
+          #
+          #       "issues"."relative_position" IS NULL
+          #       AND
+          #       "issues"."id" > 500
+          #
+          def conditions
+            attr_names  = order_list.map { |field| field.attribute_name }
+            attr_values = attr_names.map { |name| decoded_cursor[name] }
+
+            if attr_names.count == 1 && attr_values.first.nil?
+              raise Gitlab::Graphql::Errors::ArgumentError.new('Only one sortable scope and nil was supplied')
+            end
+
+            operators = comparison_operators
+
+            if attr_names.count == 1 || attr_values.first.presence
+              Keyset::Conditions::NotNullCondition.new(arel_table, attr_names, attr_values, operators, before_or_after).build
+            else
+              Keyset::Conditions::NullCondition.new(arel_table, attr_names, attr_values, operators, before_or_after).build
+            end
+          end
+
+          private
+
+          attr_reader :arel_table, :order_list, :decoded_cursor, :before_or_after
+
+          def comparison_operators
+            order_list.map { |field| field.operator_for(before_or_after) }
+          end
+        end
+      end
+    end
+  end
+end

lib/gitlab/graphql/connections/keyset_connection.rb

-# frozen_string_literal: true
-
-module Gitlab
-  module Graphql
-    module Connections
-      class KeysetConnection < GraphQL::Relay::BaseConnection
-        def cursor_from_node(node)
-          encode(node[order_field].to_s)
-        end
-
-        # rubocop: disable CodeReuse/ActiveRecord
-        def sliced_nodes
-          @sliced_nodes ||=
-            begin
-              sliced = nodes
-
-              sliced = sliced.where(before_slice) if before.present?
-              sliced = sliced.where(after_slice) if after.present?
-
-              sliced
-            end
-        end
-        # rubocop: enable CodeReuse/ActiveRecord
-
-        def paged_nodes
-          # These are the nodes that will be loaded into memory for rendering
-          # 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.
-          @paged_nodes ||= load_paged_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
-
-          if last
-            sliced_nodes.last(limit_value)
-          else
-            sliced_nodes.limit(limit_value) # rubocop: disable CodeReuse/ActiveRecord
-          end
-        end
-
-        def before_slice
-          if sort_direction == :asc
-            table[order_field].lt(decode(before))
-          else
-            table[order_field].gt(decode(before))
-          end
-        end
-
-        def after_slice
-          if sort_direction == :asc
-            table[order_field].gt(decode(after))
-          else
-            table[order_field].lt(decode(after))
-          end
-        end
-
-        def limit_value
-          @limit_value ||= [first, last, max_page_size].compact.min
-        end
-
-        def table
-          nodes.arel_table
-        end
-
-        def order_info
-          @order_info ||= nodes.order_values.first
-        end
-
-        def order_field
-          @order_field ||= order_info&.expr&.name || nodes.primary_key
-        end
-
-        def sort_direction
-          @order_direction ||= order_info&.direction || :desc
-        end
-      end
-    end
-  end
-end

spec/graphql/features/authorization_spec.rb

@@ -259,7 +259,8 @@ describe 'Gitlab::Graphql::Authorization' do
     let(:project_type) do |type|
       type_factory do |type|
         type.graphql_name 'FakeProjectType'
-        type.field :test_issues, issue_type.connection_type, null: false, resolve: -> (_, _, _) { Issue.where(project: [visible_project, other_project]) }
+        type.field :test_issues, issue_type.connection_type, null: false,
+                   resolve: -> (_, _, _) { Issue.where(project: [visible_project, other_project]).order(id: :asc) }
       end
     end
     let(:query_type) do

spec/graphql/gitlab_schema_spec.rb

@@ -36,7 +36,7 @@ describe GitlabSchema do
   it 'paginates active record relations using `Gitlab::Graphql::Connections::KeysetConnection`' do
     connection = GraphQL::Relay::BaseConnection::CONNECTION_IMPLEMENTATIONS[ActiveRecord::Relation.name]
 
-    expect(connection).to eq(Gitlab::Graphql::Connections::KeysetConnection)
+    expect(connection).to eq(Gitlab::Graphql::Connections::Keyset::Connection)
   end
 
   describe '.execute' do

spec/lib/gitlab/graphql/connections/keyset/conditions/not_null_condition_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::Graphql::Connections::Keyset::Conditions::NotNullCondition do
+  describe '#build' do
+    let(:condition) { described_class.new(Issue.arel_table, %w(relative_position id), [1500, 500], ['>', '>'], before_or_after) }
+
+    context 'when there is only one ordering field' do
+      let(:condition) { described_class.new(Issue.arel_table, ['id'], [500], ['>'], :after) }
+
+      it 'generates a single condition sql' do
+        expected_sql = <<~SQL
+          ("issues"."id" > 500)
+        SQL
+
+        expect(condition.build).to eq expected_sql
+      end
+    end
+
+    context 'when :after' do
+      let(:before_or_after) { :after }
+
+      it 'generates :after sql' do
+        expected_sql = <<~SQL
+          ("issues"."relative_position" > 1500)
+          OR (
+            "issues"."relative_position" = 1500
+            AND
+            "issues"."id" > 500
+          )
+          OR ("issues"."relative_position" IS NULL)
+        SQL
+
+        expect(condition.build).to eq expected_sql
+      end
+    end
+
+    context 'when :before' do
+      let(:before_or_after) { :before }
+
+      it 'generates :before sql' do
+        expected_sql = <<~SQL
+          ("issues"."relative_position" > 1500)
+          OR (
+            "issues"."relative_position" = 1500
+            AND
+            "issues"."id" > 500
+          )
+        SQL
+
+        expect(condition.build).to eq expected_sql
+      end
+    end
+  end
+end

spec/lib/gitlab/graphql/connections/keyset/conditions/null_condition_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::Graphql::Connections::Keyset::Conditions::NullCondition do
+  describe '#build' do
+    let(:condition) { described_class.new(Issue.arel_table, %w(relative_position id), [nil, 500], [nil, '>'], before_or_after) }
+
+    context 'when :after' do
+      let(:before_or_after) { :after }
+
+      it 'generates sql' do
+        expected_sql = <<~SQL
+        (
+          "issues"."relative_position" IS NULL
+          AND
+          "issues"."id" > 500
+        )
+        SQL
+
+        expect(condition.build).to eq expected_sql
+      end
+    end
+
+    context 'when :before' do
+      let(:before_or_after) { :before }
+
+      it 'generates :before sql' do
+        expected_sql = <<~SQL
+          (
+            "issues"."relative_position" IS NULL
+            AND
+            "issues"."id" > 500
+          )
+          OR ("issues"."relative_position" IS NOT NULL)
+        SQL
+
+        expect(condition.build).to eq expected_sql
+      end
+    end
+  end
+end

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

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::Graphql::Connections::Keyset::Connection do
+  let(:nodes) { Project.all.order(id: :asc) }
+  let(:arguments) { {} }
+  subject(:connection) do
+    described_class.new(nodes, arguments, max_page_size: 3)
+  end
+
+  def encoded_cursor(node)
+    described_class.new(nodes, {}).cursor_from_node(node)
+  end
+
+  def decoded_cursor(cursor)
+    JSON.parse(Base64Bp.urlsafe_decode64(cursor))
+  end
+
+  describe '#cursor_from_nodes' do
+    let(:project) { create(:project) }
+    let(:cursor)  { connection.cursor_from_node(project) }
+
+    it 'returns an encoded ID' do
+      expect(decoded_cursor(cursor)).to eq({ 'id' => project.id.to_s })
+    end
+
+    context 'when an order is specified' do
+      let(:nodes) { Project.order(:updated_at) }
+
+      it 'returns the encoded value of the order' do
+        expect(decoded_cursor(cursor)).to include({ 'updated_at' => project.updated_at.to_s })
+      end
+
+      it 'includes the :id even when not specified in the order' do
+        expect(decoded_cursor(cursor)).to include({ 'id' => project.id.to_s })
+      end
+    end
+
+    context 'when multiple orders is specified' do
+      let(:nodes) { Project.order(:updated_at).order(:created_at) }
+
+      it 'returns the encoded value of the order' do
+        expect(decoded_cursor(cursor)).to include({ 'updated_at' => project.updated_at.to_s })
+      end
+    end
+
+    context 'when multiple orders with SQL is specified' do
+      let(:nodes) { Project.order(Arel.sql('projects.updated_at IS NULL')).order(:updated_at).order(:id) }
+
+      it 'returns the encoded value of the order' do
+        expect(decoded_cursor(cursor)).to include({ 'updated_at' => project.updated_at.to_s })
+      end
+    end
+  end
+
+  describe '#sliced_nodes' do
+    let(:projects) { create_list(:project, 4) }
+
+    context 'when before is passed' do
+      let(:arguments) { { before: encoded_cursor(projects[1]) } }
+
+      it 'only returns the project before the selected one' do
+        expect(subject.sliced_nodes).to contain_exactly(projects.first)
+      end
+
+      context 'when the sort order is descending' do
+        let(:nodes) { Project.all.order(id: :desc) }
+
+        it 'returns the correct nodes' do
+          expect(subject.sliced_nodes).to contain_exactly(*projects[2..-1])
+        end
+      end
+    end
+
+    context 'when after is passed' do
+      let(:arguments) { { after: encoded_cursor(projects[1]) } }
+
+      it 'only returns the project before the selected one' do
+        expect(subject.sliced_nodes).to contain_exactly(*projects[2..-1])
+      end
+
+      context 'when the sort order is descending' do
+        let(:nodes) { Project.all.order(id: :desc) }
+
+        it 'returns the correct nodes' do
+          expect(subject.sliced_nodes).to contain_exactly(projects.first)
+        end
+      end
+    end
+
+    context 'when both before and after are passed' do
+      let(:arguments) do
+        {
+          after: encoded_cursor(projects[1]),
+          before: encoded_cursor(projects[3])
+        }
+      end
+
+      it 'returns the expected set' do
+        expect(subject.sliced_nodes).to contain_exactly(projects[2])
+      end
+    end
+
+    context 'when multiple orders are defined' do
+      let!(:project1) { create(:project, last_repository_check_at: 10.days.ago) }    # Asc: project5  Desc: project3
+      let!(:project2) { create(:project, last_repository_check_at: nil) }            # Asc: project1  Desc: project1
+      let!(:project3) { create(:project, last_repository_check_at: 5.days.ago) }     # Asc: project3  Desc: project5
+      let!(:project4) { create(:project, last_repository_check_at: nil) }            # Asc: project2  Desc: project2
+      let!(:project5) { create(:project, last_repository_check_at: 20.days.ago) }    # Asc: project4  Desc: project4
+
+      context 'when ascending' do
+        let(:nodes) do
+          Project.order(Arel.sql('projects.last_repository_check_at IS NULL')).order(last_repository_check_at: :asc).order(id: :asc)
+        end
+
+        context 'when no cursor is passed' do
+          let(:arguments) { {} }
+
+          it 'returns projects in ascending order' do
+            expect(subject.sliced_nodes).to eq([project5, project1, project3, project2, project4])
+          end
+        end
+
+        context 'when before cursor value is NULL' do
+          let(:arguments) { { before: encoded_cursor(project4) } }
+
+          it 'returns all projects before the cursor' do
+            expect(subject.sliced_nodes).to eq([project5, project1, project3, project2])
+          end
+        end
+
+        context 'when before cursor value is not NULL' do
+          let(:arguments) { { before: encoded_cursor(project3) } }
+
+          it 'returns all projects before the cursor' do
+            expect(subject.sliced_nodes).to eq([project5, project1])
+          end
+        end
+
+        context 'when after cursor value is NULL' do
+          let(:arguments) { { after: encoded_cursor(project2) } }
+
+          it 'returns all projects after the cursor' do
+            expect(subject.sliced_nodes).to eq([project4])
+          end
+        end
+
+        context 'when after cursor value is not NULL' do
+          let(:arguments) { { after: encoded_cursor(project1) } }
+
+          it 'returns all projects after the cursor' do
+            expect(subject.sliced_nodes).to eq([project3, project2, project4])
+          end
+        end
+
+        context 'when before and after cursor' do
+          let(:arguments) { { before: encoded_cursor(project4), after: encoded_cursor(project5) } }
+
+          it 'returns all projects after the cursor' do
+            expect(subject.sliced_nodes).to eq([project1, project3, project2])
+          end
+        end
+      end
+
+      context 'when descending' do
+        let(:nodes) do
+          Project.order(Arel.sql('projects.last_repository_check_at IS NULL')).order(last_repository_check_at: :desc).order(id: :asc)
+        end
+
+        context 'when no cursor is passed' do
+          let(:arguments) { {} }
+
+          it 'only returns projects in descending order' do
+            expect(subject.sliced_nodes).to eq([project3, project1, project5, project2, project4])
+          end
+        end
+
+        context 'when before cursor value is NULL' do
+          let(:arguments) { { before: encoded_cursor(project4) } }
+
+          it 'returns all projects before the cursor' do
+            expect(subject.sliced_nodes).to eq([project3, project1, project5, project2])
+          end
+        end
+
+        context 'when before cursor value is not NULL' do
+          let(:arguments) { { before: encoded_cursor(project5) } }
+
+          it 'returns all projects before the cursor' do
+            expect(subject.sliced_nodes).to eq([project3, project1])
+          end
+        end
+
+        context 'when after cursor value is NULL' do
+          let(:arguments) { { after: encoded_cursor(project2) } }
+
+          it 'returns all projects after the cursor' do
+            expect(subject.sliced_nodes).to eq([project4])
+          end
+        end
+
+        context 'when after cursor value is not NULL' do
+          let(:arguments) { { after: encoded_cursor(project1) } }
+
+          it 'returns all projects after the cursor' do
+            expect(subject.sliced_nodes).to eq([project5, project2, project4])
+          end
+        end
+
+        context 'when before and after cursor' do
+          let(:arguments) { { before: encoded_cursor(project4), after: encoded_cursor(project3) } }
+
+          it 'returns all projects after the cursor' do
+            expect(subject.sliced_nodes).to eq([project1, project5, project2])
+          end
+        end
+      end
+    end
+
+    # TODO Enable this as part of below issue
+    # https://gitlab.com/gitlab-org/gitlab/issues/32933
+    # context 'when an invalid cursor is provided' do
+    #   let(:arguments) { { before: 'invalidcursor' } }
+    #
+    #   it 'raises an error' do
+    #     expect { expect(subject.sliced_nodes) }.to raise_error(Gitlab::Graphql::Errors::ArgumentError)
+    #   end
+    # end
+
+    # TODO Remove this as part of below issue
+    # https://gitlab.com/gitlab-org/gitlab/issues/32933
+    context 'when an old style cursor is provided' do
+      let(:arguments) { { before: Base64Bp.urlsafe_encode64(projects[1].id.to_s, padding: false) } }
+
+      it 'only returns the project before the selected one' do
+        expect(subject.sliced_nodes).to contain_exactly(projects.first)
+      end
+    end
+  end
+
+  describe '#paged_nodes' do
+    let!(:projects) { create_list(:project, 5) }
+
+    it 'returns the collection limited to max page size' do
+      expect(subject.paged_nodes.size).to eq(3)
+    end
+
+    it 'is a loaded memoized array' do
+      expect(subject.paged_nodes).to be_an(Array)
+      expect(subject.paged_nodes.object_id).to eq(subject.paged_nodes.object_id)
+    end
+
+    context 'when `first` is passed' do
+      let(:arguments) { { first: 2 } }
+
+      it 'returns only the first elements' do
+        expect(subject.paged_nodes).to contain_exactly(projects.first, projects.second)
+      end
+    end
+
+    context 'when `last` is passed' do
+      let(:arguments) { { last: 2 } }
+
+      it 'returns only the last elements' do
+        expect(subject.paged_nodes).to contain_exactly(projects[3], projects[4])
+      end
+    end
+
+    context 'when both are passed' do
+      let(:arguments) { { first: 2, last: 2 } }
+
+      it 'raises an error' do
+        expect { subject.paged_nodes }.to raise_error(Gitlab::Graphql::Errors::ArgumentError)
+      end
+    end
+
+    context 'when primary key is not in original order' do
+      let(:nodes) { Project.order(last_repository_check_at: :desc) }
+
+      it 'is added to end' do
+        sliced = subject.sliced_nodes
+        last_order_name = sliced.order_values.last.expr.name
+
+        expect(last_order_name).to eq sliced.primary_key
+      end
+    end
+  end
+end

spec/lib/gitlab/graphql/connections/keyset/order_info_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::Graphql::Connections::Keyset::OrderInfo do
+  describe '#build_order_list' do
+    let(:order_list) { described_class.build_order_list(relation) }
+
+    context 'when multiple orders with SQL is specified' do
+      let(:relation) { Project.order(Arel.sql('projects.updated_at IS NULL')).order(:updated_at).order(:id) }
+
+      it 'ignores the SQL order' do
+        expect(order_list.count).to eq 2
+        expect(order_list.first.attribute_name).to eq 'updated_at'
+        expect(order_list.first.operator_for(:after)).to eq '>'
+        expect(order_list.last.attribute_name).to eq 'id'
+        expect(order_list.last.operator_for(:after)).to eq '>'
+      end
+    end
+  end
+
+  describe '#validate_ordering' do
+    let(:order_list) { described_class.build_order_list(relation) }
+
+    context 'when number of ordering fields is 0' do
+      let(:relation) { Project.all }
+
+      it 'raises an error' do
+        expect { described_class.validate_ordering(relation, order_list) }
+          .to raise_error(::ArgumentError, 'A minimum of 1 ordering field is required')
+      end
+    end
+
+    context 'when number of ordering fields is over 2' do
+      let(:relation) { Project.order(last_repository_check_at: :desc).order(updated_at: :desc).order(:id) }
+
+      it 'raises an error' do
+        expect { described_class.validate_ordering(relation, order_list) }
+          .to raise_error(Gitlab::Graphql::Errors::ArgumentError, 'A maximum of 2 ordering fields are allowed')
+      end
+    end
+
+    context 'when the second (or first) column is nullable' do
+      let(:relation) { Project.order(last_repository_check_at: :desc).order(updated_at: :desc) }
+
+      it 'raises an error' do
+        expect { described_class.validate_ordering(relation, order_list) }
+          .to raise_error(Gitlab::Graphql::Errors::ArgumentError, "Column `updated_at` must not allow NULL")
+      end
+    end
+
+    context 'for last ordering field' do
+      let(:relation) { Project.order(namespace_id: :desc) }
+
+      it 'raises error if primary key is not last field' do
+        expect { described_class.validate_ordering(relation, order_list) }
+          .to raise_error(Gitlab::Graphql::Errors::ArgumentError, "Last ordering field must be the primary key, `#{relation.primary_key}`")
+      end
+    end
+  end
+end

spec/lib/gitlab/graphql/connections/keyset/query_builder_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::Graphql::Connections::Keyset::QueryBuilder do
+  context 'when number of ordering fields is 0' do
+    it 'raises an error' do
+      expect { described_class.new(Issue.arel_table, [], {}, :after) }
+        .to raise_error(Gitlab::Graphql::Errors::ArgumentError, 'No ordering scopes have been supplied')
+    end
+  end
+
+  describe '#conditions' do
+    let(:relation) { Issue.order(relative_position: :desc).order(:id) }
+    let(:order_list) { Gitlab::Graphql::Connections::Keyset::OrderInfo.build_order_list(relation) }
+    let(:builder) { described_class.new(arel_table, order_list, decoded_cursor, before_or_after) }
+    let(:before_or_after) { :after }
+
+    context 'when only a single ordering' do
+      let(:relation) { Issue.order(id: :desc) }
+
+      context 'when the value is nil' do
+        let(:decoded_cursor) { { 'id' => nil } }
+
+        it 'raises an error' do
+          expect { builder.conditions }
+            .to raise_error(Gitlab::Graphql::Errors::ArgumentError, 'Only one sortable scope and nil was supplied')
+        end
+      end
+
+      context 'when value is not nil' do
+        let(:decoded_cursor) { { 'id' => 100 } }
+        let(:conditions) { builder.conditions }
+
+        context 'when :after' do
+          it 'generates the correct condition' do
+            expect(conditions.strip).to eq '("issues"."id" < 100)'
+          end
+        end
+
+        context 'when :before' do
+          let(:before_or_after) { :before }
+
+          it 'generates the correct condition' do
+            expect(conditions.strip).to eq '("issues"."id" > 100)'
+          end
+        end
+      end
+    end
+
+    context 'when two orderings' do
+      let(:decoded_cursor) { { 'relative_position' => 1500, 'id' => 100 } }
+
+      context 'when no values are nil' do
+        context 'when :after' do
+          it 'generates the correct condition' do
+            conditions = builder.conditions
+
+            expect(conditions).to include '"issues"."relative_position" < 1500'
+            expect(conditions).to include '"issues"."id" > 100'
+            expect(conditions).to include 'OR ("issues"."relative_position" IS NULL)'
+          end
+        end
+
+        context 'when :before' do
+          let(:before_or_after) { :before }
+
+          it 'generates the correct condition' do
+            conditions = builder.conditions
+
+            expect(conditions).to include '("issues"."relative_position" > 1500)'
+            expect(conditions).to include '"issues"."id" < 100'
+            expect(conditions).to include '"issues"."relative_position" = 1500'
+          end
+        end
+      end
+
+      context 'when first value is nil' do
+        let(:decoded_cursor) { { 'relative_position' => nil, 'id' => 100 } }
+
+        context 'when :after' do
+          it 'generates the correct condition' do
+            conditions = builder.conditions
+
+            expect(conditions).to include '"issues"."relative_position" IS NULL'
+            expect(conditions).to include '"issues"."id" > 100'
+          end
+        end
+
+        context 'when :before' do
+          let(:before_or_after) { :before }
+
+          it 'generates the correct condition' do
+            conditions = builder.conditions
+
+            expect(conditions).to include '"issues"."relative_position" IS NULL'
+            expect(conditions).to include '"issues"."id" < 100'
+            expect(conditions).to include 'OR ("issues"."relative_position" IS NOT NULL)'
+          end
+        end
+      end
+    end
+  end
+
+  def arel_table
+    Issue.arel_table
+  end
+end

spec/lib/gitlab/graphql/connections/keyset_connection_spec.rb

-require 'spec_helper'
-
-describe Gitlab::Graphql::Connections::KeysetConnection do
-  let(:nodes) { Project.all.order(id: :asc) }
-  let(:arguments) { {} }
-  subject(:connection) do
-    described_class.new(nodes, arguments, max_page_size: 3)
-  end
-
-  def encoded_property(value)
-    Base64Bp.urlsafe_encode64(value.to_s, padding: false)
-  end
-
-  describe '#cursor_from_nodes' do
-    let(:project) { create(:project) }
-
-    it 'returns an encoded ID' do
-      expect(connection.cursor_from_node(project))
-        .to eq(encoded_property(project.id))
-    end
-
-    context 'when an order was specified' do
-      let(:nodes) { Project.order(:updated_at) }
-
-      it 'returns the encoded value of the order' do
-        expect(connection.cursor_from_node(project))
-          .to eq(encoded_property(project.updated_at))
-      end
-    end
-  end
-
-  describe '#sliced_nodes' do
-    let(:projects) { create_list(:project, 4) }
-
-    context 'when before is passed' do
-      let(:arguments) { { before: encoded_property(projects[1].id) } }
-
-      it 'only returns the project before the selected one' do
-        expect(subject.sliced_nodes).to contain_exactly(projects.first)
-      end
-
-      context 'when the sort order is descending' do
-        let(:nodes) { Project.all.order(id: :desc) }
-
-        it 'returns the correct nodes' do
-          expect(subject.sliced_nodes).to contain_exactly(*projects[2..-1])
-        end
-      end
-    end
-
-    context 'when after is passed' do
-      let(:arguments) { { after: encoded_property(projects[1].id) } }
-
-      it 'only returns the project before the selected one' do
-        expect(subject.sliced_nodes).to contain_exactly(*projects[2..-1])
-      end
-
-      context 'when the sort order is descending' do
-        let(:nodes) { Project.all.order(id: :desc) }
-
-        it 'returns the correct nodes' do
-          expect(subject.sliced_nodes).to contain_exactly(projects.first)
-        end
-      end
-    end
-
-    context 'when both before and after are passed' do
-      let(:arguments) do
-        {
-          after: encoded_property(projects[1].id),
-          before: encoded_property(projects[3].id)
-        }
-      end
-
-      it 'returns the expected set' do
-        expect(subject.sliced_nodes).to contain_exactly(projects[2])
-      end
-    end
-  end
-
-  describe '#paged_nodes' do
-    let!(:projects) { create_list(:project, 5) }
-
-    it 'returns the collection limited to max page size' do
-      expect(subject.paged_nodes.size).to eq(3)
-    end
-
-    it 'is a loaded memoized array' do
-      expect(subject.paged_nodes).to be_an(Array)
-      expect(subject.paged_nodes.object_id).to eq(subject.paged_nodes.object_id)
-    end
-
-    context 'when `first` is passed' do
-      let(:arguments) { { first: 2 } }
-
-      it 'returns only the first elements' do
-        expect(subject.paged_nodes).to contain_exactly(projects.first, projects.second)
-      end
-    end
-
-    context 'when `last` is passed' do
-      let(:arguments) { { last: 2 } }
-
-      it 'returns only the last elements' do
-        expect(subject.paged_nodes).to contain_exactly(projects[3], projects[4])
-      end
-    end
-
-    context 'when both are passed' do
-      let(:arguments) { { first: 2, last: 2 } }
-
-      it 'raises an error' do
-        expect { subject.paged_nodes }.to raise_error(Gitlab::Graphql::Errors::ArgumentError)
-      end
-    end
-  end
-end