Merge branch '323730-support-lower-named-function' into 'master'

Update SimpleBuildOrder to support LOWER and NULLS order

See merge request gitlab-org/gitlab!82289

lib/gitlab/pagination/keyset/order.rb

@@ -99,6 +99,8 @@ module Gitlab
                                                        field_value.strftime('%Y-%m-%d %H:%M:%S.%N %Z')
                                                      elsif field_value.nil?
                                                        nil
+                                                     elsif lower_named_function?(column_definition)
+                                                       field_value.downcase
                                                      else
                                                        field_value.to_s
                                                      end
@@ -184,6 +186,10 @@ module Gitlab
 
         private
 
+        def lower_named_function?(column_definition)
+          column_definition.column_expression.is_a?(Arel::Nodes::NamedFunction) && column_definition.column_expression.name&.downcase == 'lower'
+        end
+
         def composite_row_comparison_possible?
           !column_definitions.one? &&
             column_definitions.all?(&:not_nullable?) &&

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
@@ -28,10 +30,13 @@ module Gitlab
                     primary_key_descending_order
                   elsif Gitlab::Pagination::Keyset::Order.keyset_aware?(scope)
                     Gitlab::Pagination::Keyset::Order.extract_keyset_order_object(scope)
+                  # Ordered by a primary key. Ex. 'ORDER BY id'.
                   elsif ordered_by_primary_key?
                     primary_key_order
+                  # Ordered by one non-primary table column. Ex. 'ORDER BY created_at'.
                   elsif ordered_by_other_column?
                     column_with_tie_breaker_order
+                  # Ordered by two table columns with the last column as a tie breaker. Ex. 'ORDER BY created, id ASC'.
                   elsif ordered_by_other_column_with_tie_breaker?
                     tie_breaker_attribute = order_values.second
 
@@ -50,6 +55,77 @@ module Gitlab
 
         attr_reader :scope, :order_values, :model_class, :arel_table, :primary_key
 
+        def table_column?(name)
+          model_class.column_names.include?(name.to_s)
+        end
+
+        def primary_key?(attribute)
+          arel_table[primary_key].to_s == attribute.to_s
+        end
+
+        def lower_named_function?(attribute)
+          attribute.is_a?(Arel::Nodes::NamedFunction) && attribute.name&.downcase == 'lower'
+        end
+
+        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
+            attribute.respond_to?(:name) && table_column?(attribute.name)
+          end
+        end
+
+        # 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, attribute_name)
+          nullable = model_class.columns.find { |column| column.name == attribute_name }.null
+
+          if nullable && order_value.is_a?(Arel::Nodes::Ascending)
+            :nulls_last
+          elsif nullable && order_value.is_a?(Arel::Nodes::Descending)
+            :nulls_first
+          else
+            :not_nullable
+          end
+        end
+
         def primary_key_descending_order
           Gitlab::Pagination::Keyset::Order.build([
             Gitlab::Pagination::Keyset::ColumnOrderDefinition.new(
@@ -69,63 +145,76 @@ module Gitlab
         end
 
         def column_with_tie_breaker_order(tie_breaker_column_order = default_tie_breaker_column_order)
-          order_expression = order_values.first
-          attribute_name = order_expression.expr.name
-
-          column_nullable = model_class.columns.find { |column| column.name == attribute_name }.null
-
-          nullable = if column_nullable && order_expression.is_a?(Arel::Nodes::Ascending)
-                       :nulls_last
-                     elsif column_nullable && order_expression.is_a?(Arel::Nodes::Descending)
-                       :nulls_first
-                     else
-                       :not_nullable
-                     end
-
           Gitlab::Pagination::Keyset::Order.build([
-            Gitlab::Pagination::Keyset::ColumnOrderDefinition.new(
-              attribute_name: attribute_name,
-              order_expression: order_expression,
-              nullable: nullable,
-              distinct: false
-            ),
+            column(order_values.first),
             tie_breaker_column_order
           ])
         end
 
-        def ordered_by_primary_key?
-          return unless order_values.one?
+        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 = order_values.first.try(:expr)
+          attribute_name = order_value.expr.name
 
-          return unless attribute
+          Gitlab::Pagination::Keyset::ColumnOrderDefinition.new(
+            attribute_name: attribute_name,
+            order_expression: order_value,
+            nullable: nullability(order_value, attribute_name),
+            distinct: false
+          )
+        end
 
-          arel_table[primary_key].to_s == attribute.to_s
+        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 ordered_by_other_column?
+        def lower_named_function_column(order_value)
+          attribute_name = order_value.expr.expressions.first.name
+
+          Gitlab::Pagination::Keyset::ColumnOrderDefinition.new(
+            attribute_name: attribute_name,
+            column_expression: Arel::Nodes::NamedFunction.new("LOWER", [model_class.arel_table[attribute_name]]),
+            order_expression: order_value,
+            nullable: nullability(order_value, attribute_name),
+            distinct: false
+          )
+        end
+
+        def ordered_by_primary_key?
           return unless order_values.one?
 
           attribute = order_values.first.try(:expr)
+          attribute && primary_key?(attribute)
+        end
 
-          return unless attribute
-          return unless attribute.try(:name)
+        def ordered_by_other_column?
+          return unless order_values.one?
 
-          model_class.column_names.include?(attribute.name.to_s)
+          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)
-          tie_breaker_attribute = order_values.second.try(:expr)
+          convert_raw_nulls_order!
 
-          return unless attribute
-          return unless tie_breaker_attribute
-          return unless attribute.respond_to?(:name)
+          return unless supported_column?(order_values.first)
 
-          model_class.column_names.include?(attribute.name.to_s) &&
-            arel_table[primary_key].to_s == tie_breaker_attribute.to_s
+          tie_breaker_attribute = order_values.second.try(:expr)
+          tie_breaker_attribute && primary_key?(tie_breaker_attribute)
         end
 
         def default_tie_breaker_column_order

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/in_operator_optimization/query_builder_spec.rb

@@ -239,7 +239,7 @@ RSpec.describe Gitlab::Pagination::Keyset::InOperatorOptimization::QueryBuilder
   end
 
   it 'raises error when unsupported scope is passed' do
-    scope = Issue.order(Issue.arel_table[:id].lower.desc)
+    scope = Issue.order(Arel::Nodes::NamedFunction.new('UPPER', [Issue.arel_table[:id]]))
 
     options = {
       scope: scope,

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

@@ -441,6 +441,47 @@ RSpec.describe Gitlab::Pagination::Keyset::Order do
       end
     end
 
+    context 'when ordering by the named function LOWER' do
+      let(:order) do
+        Gitlab::Pagination::Keyset::Order.build([
+          Gitlab::Pagination::Keyset::ColumnOrderDefinition.new(
+            attribute_name: 'title',
+            column_expression: Arel::Nodes::NamedFunction.new("LOWER", [table['title'].desc]),
+            order_expression: table['title'].lower.desc,
+            nullable: :not_nullable,
+            distinct: false
+          ),
+          Gitlab::Pagination::Keyset::ColumnOrderDefinition.new(
+            attribute_name: 'id',
+            column_expression: table['id'],
+            order_expression: table['id'].desc,
+            nullable: :not_nullable,
+            distinct: true
+          )
+        ])
+      end
+
+      let(:table_data) do
+        <<-SQL
+      VALUES (1,  'A')
+        SQL
+      end
+
+      let(:query) do
+        <<-SQL
+          SELECT id, title
+          FROM (#{table_data}) my_table (id, title)
+          ORDER BY #{order};
+        SQL
+      end
+
+      subject { run_query(query) }
+
+      it "uses downcased value for encoding and decoding a cursor" do
+        expect(order.cursor_attributes_for_node(subject.first)['title']).to eq("a")
+      end
+    end
+
     context 'when the passed cursor values do not match with the order definition' do
       let(:order) do
         Gitlab::Pagination::Keyset::Order.build([

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,14 +56,78 @@ 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
     end
   end
 
+  context 'when ordering by a column with the lower named function' do
+    let(:scope) { Project.where(id: [1, 2, 3]).order(Project.arel_table[:name].lower.desc) }
+
+    it 'sets the column definition for name' do
+      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')
+    end
+
+    it 'adds extra primary key order as tie-breaker' do
+      expect(sql_with_order).to end_with('ORDER BY LOWER("projects"."name") DESC, "projects"."id" DESC')
+    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 }
 
@@ -76,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