From c1740e9a77c4b84ca8bbe5d5474378e3ae21e4db Mon Sep 17 00:00:00 2001
From: Brett Walker <bwalker@gitlab.com>
Date: Thu, 26 Sep 2019 16:18:06 -0500
Subject: [PATCH] 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 | 2 +-
lib/gitlab/graphql/connections.rb | 2 +-
.../keyset/conditions/base_condition.rb | 44 +++
.../keyset/conditions/not_null_condition.rb | 59 ++++
.../keyset/conditions/null_condition.rb | 45 +++
.../graphql/connections/keyset/connection.rb | 137 +++++++++
.../graphql/connections/keyset/order_info.rb | 66 ++++
.../connections/keyset/query_builder.rb | 70 +++++
.../graphql/connections/keyset_connection.rb | 85 ------
spec/graphql/features/authorization_spec.rb | 3 +-
spec/graphql/gitlab_schema_spec.rb | 2 +-
.../conditions/not_null_condition_spec.rb | 56 ++++
.../keyset/conditions/null_condition_spec.rb | 42 +++
.../connections/keyset/connection_spec.rb | 289 ++++++++++++++++++
.../connections/keyset/order_info_spec.rb | 61 ++++
.../connections/keyset/query_builder_spec.rb | 108 +++++++
.../connections/keyset_connection_spec.rb | 117 -------
17 files changed, 982 insertions(+), 206 deletions(-)
create mode 100644 lib/gitlab/graphql/connections/keyset/conditions/base_condition.rb
create mode 100644 lib/gitlab/graphql/connections/keyset/conditions/not_null_condition.rb
create mode 100644 lib/gitlab/graphql/connections/keyset/conditions/null_condition.rb
create mode 100644 lib/gitlab/graphql/connections/keyset/connection.rb
create mode 100644 lib/gitlab/graphql/connections/keyset/order_info.rb
create mode 100644 lib/gitlab/graphql/connections/keyset/query_builder.rb
delete mode 100644 lib/gitlab/graphql/connections/keyset_connection.rb
create mode 100644 spec/lib/gitlab/graphql/connections/keyset/conditions/not_null_condition_spec.rb
create mode 100644 spec/lib/gitlab/graphql/connections/keyset/conditions/null_condition_spec.rb
create mode 100644 spec/lib/gitlab/graphql/connections/keyset/connection_spec.rb
create mode 100644 spec/lib/gitlab/graphql/connections/keyset/order_info_spec.rb
create mode 100644 spec/lib/gitlab/graphql/connections/keyset/query_builder_spec.rb
delete mode 100644 spec/lib/gitlab/graphql/connections/keyset_connection_spec.rb
diff --git a/ee/app/models/design_management/design.rb b/ee/app/models/design_management/design.rb
index 010c349aa2cf..d4f2fce9fd1c 100644
--- a/ee/app/models/design_management/design.rb
+++ b/ee/app/models/design_management/design.rb
@@ -46,7 +46,7 @@ class Design < ApplicationRecord
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
diff --git a/lib/gitlab/graphql/connections.rb b/lib/gitlab/graphql/connections.rb
index fbccdfa7b081..64f7a268b7e0 100644
--- a/lib/gitlab/graphql/connections.rb
+++ b/lib/gitlab/graphql/connections.rb
@@ -6,7 +6,7 @@ module Connections
def self.use(_schema)
GraphQL::Relay::BaseConnection.register_connection_implementation(
ActiveRecord::Relation,
- Gitlab::Graphql::Connections::KeysetConnection
+ Gitlab::Graphql::Connections::Keyset::Connection
)
end
end
diff --git a/lib/gitlab/graphql/connections/keyset/conditions/base_condition.rb b/lib/gitlab/graphql/connections/keyset/conditions/base_condition.rb
new file mode 100644
index 000000000000..c017ff06c6a2
--- /dev/null
+++ b/lib/gitlab/graphql/connections/keyset/conditions/base_condition.rb
@@ -0,0 +1,44 @@
+# 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
diff --git a/lib/gitlab/graphql/connections/keyset/conditions/not_null_condition.rb b/lib/gitlab/graphql/connections/keyset/conditions/not_null_condition.rb
new file mode 100644
index 000000000000..8e6b16cfa6e6
--- /dev/null
+++ b/lib/gitlab/graphql/connections/keyset/conditions/not_null_condition.rb
@@ -0,0 +1,59 @@
+# 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
diff --git a/lib/gitlab/graphql/connections/keyset/conditions/null_condition.rb b/lib/gitlab/graphql/connections/keyset/conditions/null_condition.rb
new file mode 100644
index 000000000000..1f3f684a5706
--- /dev/null
+++ b/lib/gitlab/graphql/connections/keyset/conditions/null_condition.rb
@@ -0,0 +1,45 @@
+# 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
diff --git a/lib/gitlab/graphql/connections/keyset/connection.rb b/lib/gitlab/graphql/connections/keyset/connection.rb
new file mode 100644
index 000000000000..79f5a9027dbf
--- /dev/null
+++ b/lib/gitlab/graphql/connections/keyset/connection.rb
@@ -0,0 +1,137 @@
+# 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
diff --git a/lib/gitlab/graphql/connections/keyset/order_info.rb b/lib/gitlab/graphql/connections/keyset/order_info.rb
new file mode 100644
index 000000000000..e7fb31847405
--- /dev/null
+++ b/lib/gitlab/graphql/connections/keyset/order_info.rb
@@ -0,0 +1,66 @@
+# 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
diff --git a/lib/gitlab/graphql/connections/keyset/query_builder.rb b/lib/gitlab/graphql/connections/keyset/query_builder.rb
new file mode 100644
index 000000000000..b0bfe3887127
--- /dev/null
+++ b/lib/gitlab/graphql/connections/keyset/query_builder.rb
@@ -0,0 +1,70 @@
+# 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
diff --git a/lib/gitlab/graphql/connections/keyset_connection.rb b/lib/gitlab/graphql/connections/keyset_connection.rb
deleted file mode 100644
index 715963a44c1d..000000000000
--- a/lib/gitlab/graphql/connections/keyset_connection.rb
+++ /dev/null
@@ -1,85 +0,0 @@
-# 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
diff --git a/spec/graphql/features/authorization_spec.rb b/spec/graphql/features/authorization_spec.rb
index 9a60ff3b78c3..7ad6a622b4b6 100644
--- a/spec/graphql/features/authorization_spec.rb
+++ b/spec/graphql/features/authorization_spec.rb
@@ -259,7 +259,8 @@
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
diff --git a/spec/graphql/gitlab_schema_spec.rb b/spec/graphql/gitlab_schema_spec.rb
index 0a27bbecfef3..dcf3c9890478 100644
--- a/spec/graphql/gitlab_schema_spec.rb
+++ b/spec/graphql/gitlab_schema_spec.rb
@@ -36,7 +36,7 @@
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
diff --git a/spec/lib/gitlab/graphql/connections/keyset/conditions/not_null_condition_spec.rb b/spec/lib/gitlab/graphql/connections/keyset/conditions/not_null_condition_spec.rb
new file mode 100644
index 000000000000..7fd21b6fd974
--- /dev/null
+++ b/spec/lib/gitlab/graphql/connections/keyset/conditions/not_null_condition_spec.rb
@@ -0,0 +1,56 @@
+# 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
diff --git a/spec/lib/gitlab/graphql/connections/keyset/conditions/null_condition_spec.rb b/spec/lib/gitlab/graphql/connections/keyset/conditions/null_condition_spec.rb
new file mode 100644
index 000000000000..f52e7a5c8df4
--- /dev/null
+++ b/spec/lib/gitlab/graphql/connections/keyset/conditions/null_condition_spec.rb
@@ -0,0 +1,42 @@
+# 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
diff --git a/spec/lib/gitlab/graphql/connections/keyset/connection_spec.rb b/spec/lib/gitlab/graphql/connections/keyset/connection_spec.rb
new file mode 100644
index 000000000000..fd3dd839d144
--- /dev/null
+++ b/spec/lib/gitlab/graphql/connections/keyset/connection_spec.rb
@@ -0,0 +1,289 @@
+# 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
diff --git a/spec/lib/gitlab/graphql/connections/keyset/order_info_spec.rb b/spec/lib/gitlab/graphql/connections/keyset/order_info_spec.rb
new file mode 100644
index 000000000000..256113e9ae88
--- /dev/null
+++ b/spec/lib/gitlab/graphql/connections/keyset/order_info_spec.rb
@@ -0,0 +1,61 @@
+# 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
diff --git a/spec/lib/gitlab/graphql/connections/keyset/query_builder_spec.rb b/spec/lib/gitlab/graphql/connections/keyset/query_builder_spec.rb
new file mode 100644
index 000000000000..e1dd74f931f6
--- /dev/null
+++ b/spec/lib/gitlab/graphql/connections/keyset/query_builder_spec.rb
@@ -0,0 +1,108 @@
+# 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
diff --git a/spec/lib/gitlab/graphql/connections/keyset_connection_spec.rb b/spec/lib/gitlab/graphql/connections/keyset_connection_spec.rb
deleted file mode 100644
index 4eb121794e1b..000000000000
--- a/spec/lib/gitlab/graphql/connections/keyset_connection_spec.rb
+++ /dev/null
@@ -1,117 +0,0 @@
-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
--
GitLab