Support NULLS FIRST AND LAST order

Reinforce connection specs

lib/gitlab/pagination/keyset/simple_order_builder.rb

@@ -11,6 +11,8 @@ module Gitlab
       # [transformed_scope, true] # true indicates that the new scope was successfully built
       # [orginal_scope, false] # false indicates that the order values are not supported in this class
       class SimpleOrderBuilder
+        NULLS_ORDER_REGEX = /(?<column_name>.*) (?<direction>\bASC\b|\bDESC\b) (?<nullable>\bNULLS LAST\b|\bNULLS FIRST\b)/.freeze
+
         def self.build(scope)
           new(scope: scope).build
         end
@@ -65,7 +67,20 @@ module Gitlab
           attribute.is_a?(Arel::Nodes::NamedFunction) && attribute.name&.downcase == 'lower'
         end
 
-        def supported_column?(attribute)
+        def arel_nulls?(order_value)
+          return unless order_value.is_a?(Arel::Nodes::NullsLast) || order_value.is_a?(Arel::Nodes::NullsFirst)
+
+          column_name = order_value.try(:expr).try(:expr).try(:name)
+
+          table_column?(column_name)
+        end
+
+        def supported_column?(order_value)
+          return true if arel_nulls?(order_value)
+
+          attribute = order_value.try(:expr)
+          return unless attribute
+
           if lower_named_function?(attribute)
             attribute.expressions.one? && attribute.expressions.first.respond_to?(:name) && table_column?(attribute.expressions.first.name)
           else
@@ -73,16 +88,34 @@ module Gitlab
           end
         end
 
-        def column_name(order_value)
-          if lower_named_function?(order_value.expr)
-            order_value.expr.expressions.first.name
-          else
-            order_value.expr.name
+        # This method converts the first order value to a corresponding arel expression
+        # if the order value uses either NULLS LAST or NULLS FIRST ordering in raw SQL.
+        #
+        # TODO: https://gitlab.com/gitlab-org/gitlab/-/issues/356644
+        # We should stop matching raw literals once we switch to using the Arel methods.
+        def convert_raw_nulls_order!
+          order_value = order_values.first
+
+          return unless order_value.is_a?(Arel::Nodes::SqlLiteral)
+
+          # Detect NULLS LAST or NULLS FIRST ordering by looking at the raw SQL string.
+          if matches = order_value.match(NULLS_ORDER_REGEX)
+            return unless table_column?(matches[:column_name])
+
+            column_attribute = arel_table[matches[:column_name]]
+            direction = matches[:direction].downcase.to_sym
+            nullable = matches[:nullable].downcase.parameterize(separator: '_').to_sym
+
+            # Build an arel order expression for NULLS ordering.
+            order = direction == :desc ? column_attribute.desc : column_attribute.asc
+            arel_order_expression = nullable == :nulls_first ? order.nulls_first : order.nulls_last
+
+            order_values[0] = arel_order_expression
           end
         end
 
-        def nullability(order_value)
-          nullable = model_class.columns.find { |column| column.name == column_name(order_value) }.null
+        def nullability(order_value, attribute_name)
+          nullable = model_class.columns.find { |column| column.name == attribute_name }.null
 
           if nullable && order_value.is_a?(Arel::Nodes::Ascending)
             :nulls_last
@@ -119,22 +152,41 @@ module Gitlab
         end
 
         def column(order_value)
+          return nulls_order_column(order_value) if arel_nulls?(order_value)
           return lower_named_function_column(order_value) if lower_named_function?(order_value.expr)
 
+          attribute_name = order_value.expr.name
+
           Gitlab::Pagination::Keyset::ColumnOrderDefinition.new(
-            attribute_name: order_value.expr.name,
+            attribute_name: attribute_name,
             order_expression: order_value,
-            nullable: nullability(order_value),
+            nullable: nullability(order_value, attribute_name),
+            distinct: false
+          )
+        end
+
+        def nulls_order_column(order_value)
+          attribute = order_value.expr.expr
+
+          Gitlab::Pagination::Keyset::ColumnOrderDefinition.new(
+            attribute_name: attribute.name,
+            column_expression: attribute,
+            order_expression: order_value,
+            reversed_order_expression: order_value.reverse,
+            order_direction: order_value.expr.direction,
+            nullable: order_value.is_a?(Arel::Nodes::NullsLast) ? :nulls_last : :nulls_first,
             distinct: false
           )
         end
 
         def lower_named_function_column(order_value)
+          attribute_name = order_value.expr.expressions.first.name
+
           Gitlab::Pagination::Keyset::ColumnOrderDefinition.new(
-            attribute_name: column_name(order_value),
+            attribute_name: attribute_name,
+            column_expression: Arel::Nodes::NamedFunction.new("LOWER", [model_class.arel_table[attribute_name]]),
             order_expression: order_value,
-            column_expression: Arel::Nodes::NamedFunction.new("LOWER", [model_class.arel_table[column_name(order_value)]]),
-            nullable: nullability(order_value),
+            nullable: nullability(order_value, attribute_name),
             distinct: false
           )
         end
@@ -149,15 +201,17 @@ module Gitlab
         def ordered_by_other_column?
           return unless order_values.one?
 
-          attribute = order_values.first.try(:expr)
-          attribute && supported_column?(attribute)
+          convert_raw_nulls_order!
+
+          supported_column?(order_values.first)
         end
 
         def ordered_by_other_column_with_tie_breaker?
           return unless order_values.size == 2
 
-          attribute = order_values.first.try(:expr)
-          return unless attribute && supported_column?(attribute)
+          convert_raw_nulls_order!
+
+          return unless supported_column?(order_values.first)
 
           tie_breaker_attribute = order_values.second.try(:expr)
           tie_breaker_attribute && primary_key?(tie_breaker_attribute)

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

@@ -256,11 +256,6 @@ RSpec.describe Gitlab::Graphql::Pagination::Keyset::Connection do
         end
       end
 
-      # rubocop: disable RSpec/EmptyExampleGroup
-      context 'when ordering uses LOWER' do
-      end
-      # rubocop: enable RSpec/EmptyExampleGroup
-
       context 'when ordering by similarity' do
         let_it_be(:project1) { create(:project, name: 'test') }
         let_it_be(:project2) { create(:project, name: 'testing') }

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

@@ -310,6 +310,76 @@ RSpec.describe Gitlab::Graphql::Pagination::Keyset::Connection do
       end
     end
 
+    context 'NULLS order' do
+      using RSpec::Parameterized::TableSyntax
+
+      let_it_be(:issue1) { create(:issue, relative_position: nil) }
+      let_it_be(:issue2) { create(:issue, relative_position: 100) }
+      let_it_be(:issue3) { create(:issue, relative_position: 200) }
+      let_it_be(:issue4) { create(:issue, relative_position: nil) }
+      let_it_be(:issue5) { create(:issue, relative_position: 300) }
+
+      context 'when ascending NULLS LAST (ties broken by id DESC implicitly)' do
+        let(:ascending_nodes) { [issue2, issue3, issue5, issue4, issue1] }
+
+        where(:nodes) do
+          [
+            lazy { Issue.order(::Gitlab::Database.nulls_last_order('relative_position', 'ASC')) },
+            lazy { Issue.order(Issue.arel_table[:relative_position].asc.nulls_last) }
+          ]
+        end
+
+        with_them do
+          it_behaves_like 'nodes are in ascending order'
+        end
+      end
+
+      context 'when descending NULLS LAST (ties broken by id DESC implicitly)' do
+        let(:descending_nodes) { [issue5, issue3, issue2, issue4, issue1] }
+
+        where(:nodes) do
+          [
+            lazy { Issue.order(::Gitlab::Database.nulls_last_order('relative_position', 'DESC')) },
+            lazy { Issue.order(Issue.arel_table[:relative_position].desc.nulls_last) }
+]
+        end
+
+        with_them do
+          it_behaves_like 'nodes are in descending order'
+        end
+      end
+
+      context 'when ascending NULLS FIRST with a tie breaker' do
+        let(:ascending_nodes) { [issue1, issue4, issue2, issue3, issue5] }
+
+        where(:nodes) do
+          [
+            lazy { Issue.order(::Gitlab::Database.nulls_first_order('relative_position', 'ASC')).order(id: :asc) },
+            lazy { Issue.order(Issue.arel_table[:relative_position].asc.nulls_first).order(id: :asc) }
+]
+        end
+
+        with_them do
+          it_behaves_like 'nodes are in ascending order'
+        end
+      end
+
+      context 'when descending NULLS FIRST with a tie breaker' do
+        let(:descending_nodes) { [issue1, issue4, issue5, issue3, issue2] }
+
+        where(:nodes) do
+          [
+            lazy { Issue.order(::Gitlab::Database.nulls_first_order('relative_position', 'DESC')).order(id: :asc) },
+            lazy { Issue.order(Issue.arel_table[:relative_position].desc.nulls_first).order(id: :asc) }
+]
+        end
+
+        with_them do
+          it_behaves_like 'nodes are in descending order'
+        end
+      end
+    end
+
     context 'when ordering by similarity' do
       let!(:project1) { create(:project, name: 'test') }
       let!(:project2) { create(:project, name: 'testing') }

spec/lib/gitlab/pagination/keyset/simple_order_builder_spec.rb

@@ -5,6 +5,7 @@ require 'spec_helper'
 RSpec.describe Gitlab::Pagination::Keyset::SimpleOrderBuilder do
   let(:ordered_scope) { described_class.build(scope).first }
   let(:order_object) { Gitlab::Pagination::Keyset::Order.extract_keyset_order_object(ordered_scope) }
+  let(:column_definition) { order_object.column_definitions.first }
 
   subject(:sql_with_order) { ordered_scope.to_sql }
 
@@ -16,8 +17,6 @@ RSpec.describe Gitlab::Pagination::Keyset::SimpleOrderBuilder do
     end
 
     it 'sets the column definition distinct and not nullable' do
-      column_definition = order_object.column_definitions.first
-
       expect(column_definition).to be_not_nullable
       expect(column_definition).to be_distinct
     end
@@ -39,8 +38,6 @@ RSpec.describe Gitlab::Pagination::Keyset::SimpleOrderBuilder do
     end
 
     it 'sets the column definition for created_at non-distinct and nullable' do
-      column_definition = order_object.column_definitions.first
-
       expect(column_definition.attribute_name).to eq('created_at')
       expect(column_definition.nullable?).to eq(true) # be_nullable calls non_null? method for some reason
       expect(column_definition).not_to be_distinct
@@ -59,8 +56,6 @@ RSpec.describe Gitlab::Pagination::Keyset::SimpleOrderBuilder do
     let(:scope) { Project.where(id: [1, 2, 3]).order(namespace_id: :asc, id: :asc) }
 
     it 'sets the column definition for namespace_id non-distinct and non-nullable' do
-      column_definition = order_object.column_definitions.first
-
       expect(column_definition.attribute_name).to eq('namespace_id')
       expect(column_definition).to be_not_nullable
       expect(column_definition).not_to be_distinct
@@ -71,8 +66,6 @@ RSpec.describe Gitlab::Pagination::Keyset::SimpleOrderBuilder do
     let(:scope) { Project.where(id: [1, 2, 3]).order(Project.arel_table[:name].lower.desc) }
 
     it 'sets the column definition for name' do
-      column_definition = order_object.column_definitions.first
-
       expect(column_definition.attribute_name).to eq('name')
       expect(column_definition.column_expression.expressions.first.name).to eq('name')
       expect(column_definition.column_expression.name).to eq('LOWER')
@@ -83,6 +76,58 @@ RSpec.describe Gitlab::Pagination::Keyset::SimpleOrderBuilder do
     end
   end
 
+  context "NULLS order given as as an Arel literal" do
+    context 'when NULLS LAST order is given without a tie-breaker' do
+      let(:scope) { Project.order(::Gitlab::Database.nulls_last_order('created_at', 'ASC')) }
+
+      it 'sets the column definition for created_at appropriately' do
+        expect(column_definition.attribute_name).to eq('created_at')
+      end
+
+      it 'orders by primary key' do
+        expect(sql_with_order).to end_with('ORDER BY "projects".created_at ASC NULLS LAST, "projects"."id" DESC')
+      end
+    end
+
+    context 'when NULLS FIRST order is given with a tie-breaker' do
+      let(:scope) { Issue.order(::Gitlab::Database.nulls_first_order('relative_position', 'DESC')).order(id: :asc) }
+
+      it 'sets the column definition for created_at appropriately' do
+        expect(column_definition.attribute_name).to eq('relative_position')
+      end
+
+      it 'orders by the given primary key' do
+        expect(sql_with_order).to end_with('ORDER BY "issues".relative_position DESC NULLS FIRST, "issues"."id" ASC')
+      end
+    end
+  end
+
+  context "NULLS order given as as an Arel node" do
+    context 'when NULLS LAST order is given without a tie-breaker' do
+      let(:scope) { Project.order(Project.arel_table[:created_at].asc.nulls_last) }
+
+      it 'sets the column definition for created_at appropriately' do
+        expect(column_definition.attribute_name).to eq('created_at')
+      end
+
+      it 'orders by primary key' do
+        expect(sql_with_order).to end_with('ORDER BY "projects"."created_at" ASC NULLS LAST, "projects"."id" DESC')
+      end
+    end
+
+    context 'when NULLS FIRST order is given with a tie-breaker' do
+      let(:scope) { Issue.order(Issue.arel_table[:relative_position].desc.nulls_first).order(id: :asc) }
+
+      it 'sets the column definition for created_at appropriately' do
+        expect(column_definition.attribute_name).to eq('relative_position')
+      end
+
+      it 'orders by the given primary key' do
+        expect(sql_with_order).to end_with('ORDER BY "issues"."relative_position" DESC NULLS FIRST, "issues"."id" ASC')
+      end
+    end
+  end
+
   context 'return :unable_to_order symbol when order cannot be built' do
     subject(:success) { described_class.build(scope).last }
 
@@ -92,10 +137,20 @@ RSpec.describe Gitlab::Pagination::Keyset::SimpleOrderBuilder do
       it { is_expected.to eq(false) }
     end
 
-    context 'when NULLS LAST order is given' do
-      let(:scope) { Project.order(::Gitlab::Database.nulls_last_order('created_at', 'ASC')) }
+    context 'when an invalid NULLS order is given' do
+      using RSpec::Parameterized::TableSyntax
 
-      it { is_expected.to eq(false) }
+      where(:scope) do
+        [
+          lazy { Project.order(Arel.sql('projects.updated_at created_at Asc Nulls Last')) },
+          lazy { Project.order(::Gitlab::Database.nulls_first_order('created_at', 'ZZZ')) },
+          lazy { Project.order(::Gitlab::Database.nulls_last_order('relative_position', 'ASC')) }
+        ]
+      end
+
+      with_them do
+        it { is_expected.to eq(false) }
+      end
     end
 
     context 'when more than 2 columns are given for the order' do