Merge branch '199536-refactor-graphql-extend-keyset-pagination' into 'master'

Small refactor of keyset pagination

See merge request gitlab-org/gitlab!24106

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

@@ -6,8 +6,10 @@ module Gitlab
       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
+            def initialize(arel_table, order_list, values, operators, before_or_after)
+              @arel_table, @order_list, @values, @operators, @before_or_after = arel_table, order_list, values, operators, before_or_after
+
+              @before_or_after = :after unless [:after, :before].include?(@before_or_after)
             end
 
             def build
@@ -16,7 +18,7 @@ module Gitlab
 
             private
 
-            attr_reader :arel_table, :names, :values, :operator, :before_or_after
+            attr_reader :arel_table, :order_list, :values, :operators, :before_or_after
 
             def table_condition(attribute, value, operator)
               case operator

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

@@ -12,7 +12,7 @@ module Gitlab
               # 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
+              unless order_list.count == 1
                 conditions << [second_attribute_condition, final_condition]
               end
 
@@ -24,7 +24,7 @@ module Gitlab
             # ex: "(relative_position > 23)"
             def first_attribute_condition
               <<~SQL
-                (#{table_condition(names.first, values.first, operator.first).to_sql})
+                (#{table_condition(order_list.first, values.first, operators.first).to_sql})
               SQL
             end
 
@@ -32,9 +32,9 @@ module Gitlab
             def second_attribute_condition
               condition = <<~SQL
                 OR (
-                  #{table_condition(names.first, values.first, '=').to_sql}
+                  #{table_condition(order_list.first, values.first, '=').to_sql}
                   AND
-                  #{table_condition(names[1], values[1], operator[1]).to_sql}
+                  #{table_condition(order_list[1], values[1], operators[1]).to_sql}
                 )
               SQL
 
@@ -45,7 +45,7 @@ module Gitlab
             def final_condition
               if before_or_after == :after
                 <<~SQL
-                  OR (#{table_condition(names.first, nil, 'is_null').to_sql})
+                  OR (#{table_condition(order_list.first, nil, 'is_null').to_sql})
                 SQL
               end
             end

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

@@ -16,9 +16,9 @@ module Gitlab
             def first_attribute_condition
               condition = <<~SQL
                 (
-                  #{table_condition(names.first, nil, 'is_null').to_sql}
+                  #{table_condition(order_list.first, nil, 'is_null').to_sql}
                   AND
-                  #{table_condition(names[1], values[1], operator[1]).to_sql}
+                  #{table_condition(order_list[1], values[1], operators[1]).to_sql}
                 )
               SQL
 
@@ -29,7 +29,7 @@ module Gitlab
             def final_condition
               if before_or_after == :before
                 <<~SQL
-                  OR (#{table_condition(names.first, nil, 'is_not_null').to_sql})
+                  OR (#{table_condition(order_list.first, nil, 'is_not_null').to_sql})
                 SQL
               end
             end

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

@@ -8,12 +8,12 @@ module Gitlab
           attr_reader :attribute_name, :sort_direction
 
           def initialize(order_value)
-            if order_value.is_a?(String)
-              @attribute_name, @sort_direction = extract_nulls_last_order(order_value)
-            else
-              @attribute_name = order_value.expr.name
-              @sort_direction = order_value.direction
-            end
+            @attribute_name, @sort_direction =
+              if order_value.is_a?(String)
+                extract_nulls_last_order(order_value)
+              else
+                extract_attribute_values(order_value)
+              end
           end
 
           def operator_for(before_or_after)
@@ -71,6 +71,10 @@ module Gitlab
 
             [tokens.first, (tokens[1] == 'asc' ? :asc : :desc)]
           end
+
+          def extract_attribute_values(order_value)
+            [order_value.expr.name, order_value.direction]
+          end
         end
       end
     end

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

@@ -4,10 +4,15 @@ 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) }
+    let(:operators) { ['>', '>'] }
+    let(:before_or_after) { :after }
+    let(:condition) { described_class.new(arel_table, order_list, values, operators, before_or_after) }
 
     context 'when there is only one ordering field' do
-      let(:condition) { described_class.new(Issue.arel_table, ['id'], [500], ['>'], :after) }
+      let(:arel_table) { Issue.arel_table }
+      let(:order_list) { ['id'] }
+      let(:values)     { [500] }
+      let(:operators)  { ['>'] }
 
       it 'generates a single condition sql' do
         expected_sql = <<~SQL
@@ -18,38 +23,52 @@ describe Gitlab::Graphql::Connections::Keyset::Conditions::NotNullCondition do
       end
     end
 
-    context 'when :after' do
-      let(:before_or_after) { :after }
+    context 'when ordering by a column attribute' do
+      let(:arel_table) { Issue.arel_table }
+      let(:order_list) { %w(relative_position id) }
+      let(:values)     { [1500, 500] }
 
-      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
+      shared_examples ':after condition' do
+        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.squish).to eq expected_sql.squish
+          expect(condition.build.squish).to eq expected_sql.squish
+        end
       end
-    end
 
-    context 'when :before' do
-      let(:before_or_after) { :before }
+      context 'when :after' do
+        it_behaves_like ':after condition'
+      end
 
-      it 'generates :before sql' do
-        expected_sql = <<~SQL
-          ("issues"."relative_position" > 1500)
-          OR (
-            "issues"."relative_position" = 1500
-            AND
-            "issues"."id" > 500
-          )
-        SQL
+      context 'when :before' do
+        let(:before_or_after) { :before }
 
-        expect(condition.build.squish).to eq expected_sql.squish
+        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.squish).to eq expected_sql.squish
+        end
+      end
+
+      context 'when :foo' do
+        let(:before_or_after) { :foo }
+
+        it_behaves_like ':after condition'
       end
     end
   end

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

@@ -4,38 +4,54 @@ 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
+    let(:values) { [nil, 500] }
+    let(:operators) { [nil, '>'] }
+    let(:before_or_after) { :after }
+    let(:condition) { described_class.new(arel_table, order_list, values, operators, before_or_after) }
+
+    context 'when ordering by a column attribute' do
+      let(:arel_table) { Issue.arel_table }
+      let(:order_list) { %w(relative_position id) }
+
+      shared_examples ':after condition' do
+        it 'generates sql' do
+          expected_sql = <<~SQL
+            (
+              "issues"."relative_position" IS NULL
+              AND
+              "issues"."id" > 500
+            )
+          SQL
+
+          expect(condition.build.squish).to eq expected_sql.squish
+        end
+      end
 
-        expect(condition.build.squish).to eq expected_sql.squish
+      context 'when :after' do
+        it_behaves_like ':after condition'
       end
-    end
 
-    context 'when :before' do
-      let(:before_or_after) { :before }
+      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.squish).to eq expected_sql.squish
+        end
+      end
 
-      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
+      context 'when :foo' do
+        let(:before_or_after) { :foo }
 
-        expect(condition.build.squish).to eq expected_sql.squish
+        it_behaves_like ':after condition'
       end
     end
   end

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

@@ -13,6 +13,7 @@ describe Gitlab::Graphql::Connections::Keyset::QueryBuilder do
   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(:arel_table) { Issue.arel_table }
     let(:builder) { described_class.new(arel_table, order_list, decoded_cursor, before_or_after) }
     let(:before_or_after) { :after }
 
@@ -101,8 +102,4 @@ describe Gitlab::Graphql::Connections::Keyset::QueryBuilder do
       end
     end
   end
-
-  def arel_table
-    Issue.arel_table
-  end
 end