Merge branch '239116-add-merge-request-sort-options-to-graphql' into 'master'

Add MergeRequest sort options to GraphQL API

See merge request gitlab-org/gitlab!40138

app/finders/concerns/merged_at_filter.rb

@@ -3,7 +3,6 @@
 module MergedAtFilter
   private
 
-  # rubocop: disable CodeReuse/ActiveRecord
   def by_merged_at(items)
     return items unless merged_after || merged_before
 
@@ -11,11 +10,8 @@ module MergedAtFilter
     mr_metrics_scope = mr_metrics_scope.merged_after(merged_after) if merged_after.present?
     mr_metrics_scope = mr_metrics_scope.merged_before(merged_before) if merged_before.present?
 
-    scope = items.joins(:metrics).merge(mr_metrics_scope)
-    scope = target_project_id_filter_on_metrics(scope) if Feature.enabled?(:improved_mr_merged_at_queries, default_enabled: true)
-    scope
+    items.join_metrics.merge(mr_metrics_scope)
   end
-  # rubocop: enable CodeReuse/ActiveRecord
 
   def merged_after
     params[:merged_after]
@@ -24,10 +20,4 @@ module MergedAtFilter
   def merged_before
     params[:merged_before]
   end
-
-  # rubocop: disable CodeReuse/ActiveRecord
-  def target_project_id_filter_on_metrics(scope)
-    scope.where(MergeRequest.arel_table[:target_project_id].eq(MergeRequest::Metrics.arel_table[:target_project_id]))
-  end
-  # rubocop: enable CodeReuse/ActiveRecord
 end

app/graphql/resolvers/merge_requests_resolver.rb

@@ -37,6 +37,10 @@ module Resolvers
     argument :milestone_title, GraphQL::STRING_TYPE,
              required: false,
              description: 'Title of the milestone'
+    argument :sort, Types::MergeRequestSortEnum,
+             description: 'Sort merge requests by this criteria',
+             required: false,
+             default_value: 'created_desc'
 
     def self.single
       ::Resolvers::MergeRequestResolver

app/graphql/types/merge_request_sort_enum.rb

+# frozen_string_literal: true
+
+module Types
+  class MergeRequestSortEnum < IssuableSortEnum
+    graphql_name 'MergeRequestSort'
+    description 'Values for sorting merge requests'
+
+    value 'MERGED_AT_ASC', 'Merge time by ascending order', value: :merged_at_asc
+    value 'MERGED_AT_DESC', 'Merge time by descending order', value: :merged_at_desc
+  end
+end

app/models/merge_request.rb

@@ -251,6 +251,15 @@ class MergeRequest < ApplicationRecord
     joins(:notes).where(notes: { commit_id: sha })
   end
   scope :join_project, -> { joins(:target_project) }
+  scope :join_metrics, -> do
+    query = joins(:metrics)
+
+    if Feature.enabled?(:improved_mr_merged_at_queries, default_enabled: true)
+      query = query.where(MergeRequest.arel_table[:target_project_id].eq(MergeRequest::Metrics.arel_table[:target_project_id]))
+    end
+
+    query
+  end
   scope :references_project, -> { references(:target_project) }
   scope :with_api_entity_associations, -> {
     preload_routables
@@ -264,6 +273,14 @@ class MergeRequest < ApplicationRecord
     where("target_branch LIKE ?", ApplicationRecord.sanitize_sql_like(wildcard_branch_name).tr('*', '%'))
   end
   scope :by_target_branch, ->(branch_name) { where(target_branch: branch_name) }
+  scope :order_merged_at, ->(direction) do
+    query = join_metrics.order(Gitlab::Database.nulls_last_order('merge_request_metrics.merged_at', direction))
+
+    # Add `merge_request_metrics.merged_at` to the `SELECT` in order to make the keyset pagination work.
+    query.select(*query.arel.projections, MergeRequest::Metrics.arel_table[:merged_at].as('"merge_request_metrics.merged_at"'))
+  end
+  scope :order_merged_at_asc, -> { order_merged_at('ASC') }
+  scope :order_merged_at_desc, -> { order_merged_at('DESC') }
   scope :preload_source_project, -> { preload(:source_project) }
   scope :preload_target_project, -> { preload(:target_project) }
   scope :preload_routables, -> do
@@ -320,6 +337,15 @@ class MergeRequest < ApplicationRecord
       .pluck(:target_branch)
   end
 
+  def self.sort_by_attribute(method, excluded_labels: [])
+    case method.to_s
+    when 'merged_at', 'merged_at_asc' then order_merged_at_asc.with_order_id_desc
+    when 'merged_at_desc' then order_merged_at_desc.with_order_id_desc
+    else
+      super
+    end
+  end
+
   def rebase_in_progress?
     rebase_jid.present? && Gitlab::SidekiqStatus.running?(rebase_jid)
   end

changelogs/unreleased/239116-add-merge-request-sort-options-to-graphql.yml

+---
+title: Add MergeRequest sort options to GraphQL API
+merge_request: 40138
+author:
+type: added

doc/api/graphql/reference/gitlab_schema.graphql

@@ -9520,6 +9520,71 @@ type MergeRequestSetWipPayload {
   mergeRequest: MergeRequest
 }
 
+"""
+Values for sorting merge requests
+"""
+enum MergeRequestSort {
+  """
+  Label priority by ascending order
+  """
+  LABEL_PRIORITY_ASC
+
+  """
+  Label priority by descending order
+  """
+  LABEL_PRIORITY_DESC
+
+  """
+  Merge time by ascending order
+  """
+  MERGED_AT_ASC
+
+  """
+  Merge time by descending order
+  """
+  MERGED_AT_DESC
+
+  """
+  Milestone due date by ascending order
+  """
+  MILESTONE_DUE_ASC
+
+  """
+  Milestone due date by descending order
+  """
+  MILESTONE_DUE_DESC
+
+  """
+  Priority by ascending order
+  """
+  PRIORITY_ASC
+
+  """
+  Priority by descending order
+  """
+  PRIORITY_DESC
+
+  """
+  Created at ascending order
+  """
+  created_asc
+
+  """
+  Created at descending order
+  """
+  created_desc
+
+  """
+  Updated at ascending order
+  """
+  updated_asc
+
+  """
+  Updated at descending order
+  """
+  updated_desc
+}
+
 """
 State of a GitLab merge request
 """
@@ -11741,6 +11806,11 @@ type Project {
     """
     milestoneTitle: String
 
+    """
+    Sort merge requests by this criteria
+    """
+    sort: MergeRequestSort = created_desc
+
     """
     Array of source branch names. All resolved merge requests will have one of these branches as their source.
     """
@@ -16808,6 +16878,11 @@ type User {
     """
     projectPath: String
 
+    """
+    Sort merge requests by this criteria
+    """
+    sort: MergeRequestSort = created_desc
+
     """
     Array of source branch names. All resolved merge requests will have one of these branches as their source.
     """
@@ -16883,6 +16958,11 @@ type User {
     """
     projectPath: String
 
+    """
+    Sort merge requests by this criteria
+    """
+    sort: MergeRequestSort = created_desc
+
     """
     Array of source branch names. All resolved merge requests will have one of these branches as their source.
     """

doc/api/graphql/reference/gitlab_schema.json

@@ -26639,6 +26639,89 @@
           "enumValues": null,
           "possibleTypes": null
         },
+        {
+          "kind": "ENUM",
+          "name": "MergeRequestSort",
+          "description": "Values for sorting merge requests",
+          "fields": null,
+          "inputFields": null,
+          "interfaces": null,
+          "enumValues": [
+            {
+              "name": "updated_desc",
+              "description": "Updated at descending order",
+              "isDeprecated": false,
+              "deprecationReason": null
+            },
+            {
+              "name": "updated_asc",
+              "description": "Updated at ascending order",
+              "isDeprecated": false,
+              "deprecationReason": null
+            },
+            {
+              "name": "created_desc",
+              "description": "Created at descending order",
+              "isDeprecated": false,
+              "deprecationReason": null
+            },
+            {
+              "name": "created_asc",
+              "description": "Created at ascending order",
+              "isDeprecated": false,
+              "deprecationReason": null
+            },
+            {
+              "name": "PRIORITY_ASC",
+              "description": "Priority by ascending order",
+              "isDeprecated": false,
+              "deprecationReason": null
+            },
+            {
+              "name": "PRIORITY_DESC",
+              "description": "Priority by descending order",
+              "isDeprecated": false,
+              "deprecationReason": null
+            },
+            {
+              "name": "LABEL_PRIORITY_ASC",
+              "description": "Label priority by ascending order",
+              "isDeprecated": false,
+              "deprecationReason": null
+            },
+            {
+              "name": "LABEL_PRIORITY_DESC",
+              "description": "Label priority by descending order",
+              "isDeprecated": false,
+              "deprecationReason": null
+            },
+            {
+              "name": "MILESTONE_DUE_ASC",
+              "description": "Milestone due date by ascending order",
+              "isDeprecated": false,
+              "deprecationReason": null
+            },
+            {
+              "name": "MILESTONE_DUE_DESC",
+              "description": "Milestone due date by descending order",
+              "isDeprecated": false,
+              "deprecationReason": null
+            },
+            {
+              "name": "MERGED_AT_ASC",
+              "description": "Merge time by ascending order",
+              "isDeprecated": false,
+              "deprecationReason": null
+            },
+            {
+              "name": "MERGED_AT_DESC",
+              "description": "Merge time by descending order",
+              "isDeprecated": false,
+              "deprecationReason": null
+            }
+          ],
+          "possibleTypes": null
+        },
         {
           "kind": "ENUM",
           "name": "MergeRequestState",
@@ -34771,6 +34854,16 @@
                   },
                   "defaultValue": null
                 },
+                {
+                  "name": "sort",
+                  "description": "Sort merge requests by this criteria",
+                  "type": {
+                    "kind": "ENUM",
+                    "name": "MergeRequestSort",
+                    "ofType": null
+                  },
+                  "defaultValue": "created_desc"
+                },
                 {
                   "name": "assigneeUsername",
                   "description": "Username of the assignee",
@@ -49461,6 +49554,16 @@
                   },
                   "defaultValue": null
                 },
+                {
+                  "name": "sort",
+                  "description": "Sort merge requests by this criteria",
+                  "type": {
+                    "kind": "ENUM",
+                    "name": "MergeRequestSort",
+                    "ofType": null
+                  },
+                  "defaultValue": "created_desc"
+                },
                 {
                   "name": "projectPath",
                   "description": "The full-path of the project the authored merge requests should be in. Incompatible with projectId.",
@@ -49646,6 +49749,16 @@
                   },
                   "defaultValue": null
                 },
+                {
+                  "name": "sort",
+                  "description": "Sort merge requests by this criteria",
+                  "type": {
+                    "kind": "ENUM",
+                    "name": "MergeRequestSort",
+                    "ofType": null
+                  },
+                  "defaultValue": "created_desc"
+                },
                 {
                   "name": "projectPath",
                   "description": "The full-path of the project the authored merge requests should be in. Incompatible with projectId.",

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

@@ -29,7 +29,7 @@ module Gitlab
             def table_condition(order_info, value, operator)
               if order_info.named_function
                 target = order_info.named_function
-                value  = value&.downcase if target&.name&.downcase == 'lower'
+                value  = value&.downcase if target.respond_to?(:name) && target&.name&.downcase == 'lower'
               else
                 target = arel_table[order_info.attribute_name]
               end

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

@@ -71,7 +71,22 @@ module Gitlab
           def extract_nulls_last_order(order_value)
             tokens = order_value.downcase.split
 
-            [tokens.first, (tokens[1] == 'asc' ? :asc : :desc), nil]
+            column_reference = tokens.first
+            sort_direction = tokens[1] == 'asc' ? :asc : :desc
+
+            # Handles the case when the order value is coming from another table.
+            # Example: table_name.column_name
+            # Query the value using the fully qualified column name: pass table_name.column_name as the named_function
+            if fully_qualified_column_reference?(column_reference)
+              [column_reference, sort_direction, Arel.sql(column_reference)]
+            else
+              [column_reference, sort_direction, nil]
+            end
+          end
+
+          # Example: table_name.column_name
+          def fully_qualified_column_reference?(attribute)
+            attribute.to_s.count('.') == 1
           end
 
           def extract_attribute_values(order_value)

spec/graphql/resolvers/merge_requests_resolver_spec.rb

@@ -206,6 +206,33 @@ RSpec.describe Resolvers::MergeRequestsResolver do
         expect(result.compact).to contain_exactly(merge_request_4)
       end
     end
+
+    describe 'sorting' do
+      context 'when sorting by created' do
+        it 'sorts merge requests ascending' do
+          expect(resolve_mr(project, sort: 'created_asc')).to eq [merge_request_1, merge_request_2, merge_request_3, merge_request_4, merge_request_5, merge_request_6, merge_request_with_milestone]
+        end
+
+        it 'sorts merge requests descending' do
+          expect(resolve_mr(project, sort: 'created_desc')).to eq [merge_request_with_milestone, merge_request_6, merge_request_5, merge_request_4, merge_request_3, merge_request_2, merge_request_1]
+        end
+      end
+
+      context 'when sorting by merged at' do
+        before do
+          merge_request_1.metrics.update!(merged_at: 10.days.ago)
+          merge_request_3.metrics.update!(merged_at: 5.days.ago)
+        end
+
+        it 'sorts merge requests ascending' do
+          expect(resolve_mr(project, sort: :merged_at_asc)).to eq [merge_request_1, merge_request_3, merge_request_with_milestone, merge_request_6, merge_request_5, merge_request_4, merge_request_2]
+        end
+
+        it 'sorts merge requests descending' do
+          expect(resolve_mr(project, sort: :merged_at_desc)).to eq [merge_request_3, merge_request_1, merge_request_with_milestone, merge_request_6, merge_request_5, merge_request_4, merge_request_2]
+        end
+      end
+    end
   end
 
   def resolve_mr_single(project, iid)

spec/graphql/types/merge_request_sort_enum_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe GitlabSchema.types['MergeRequestSort'] do
+  specify { expect(described_class.graphql_name).to eq('MergeRequestSort') }
+
+  it_behaves_like 'common sort values'
+
+  it 'exposes all the existing issue sort values' do
+    expect(described_class.values.keys).to include(
+      *%w[MERGED_AT_ASC MERGED_AT_DESC]
+    )
+  end
+end

spec/graphql/types/project_type_spec.rb

@@ -75,7 +75,8 @@ RSpec.describe GitlabSchema.types['Project'] do
                                             :merged_before,
                                             :author_username,
                                             :assignee_username,
-                                            :milestone_title
+                                            :milestone_title,
+                                            :sort
                                            )
     end
   end

spec/models/merge_request_spec.rb

@@ -61,6 +61,24 @@ RSpec.describe MergeRequest, factory_default: :keep do
     end
   end
 
+  describe '.order_merged_at_asc' do
+    let_it_be(:older_mr) { create(:merge_request, :with_merged_metrics) }
+    let_it_be(:newer_mr) { create(:merge_request, :with_merged_metrics) }
+
+    it 'returns MRs ordered by merged_at ascending' do
+      expect(described_class.order_merged_at_asc).to eq([older_mr, newer_mr])
+    end
+  end
+
+  describe '.order_merged_at_desc' do
+    let_it_be(:older_mr) { create(:merge_request, :with_merged_metrics) }
+    let_it_be(:newer_mr) { create(:merge_request, :with_merged_metrics) }
+
+    it 'returns MRs ordered by merged_at descending' do
+      expect(described_class.order_merged_at_desc).to eq([newer_mr, older_mr])
+    end
+  end
+
   describe '#squash_in_progress?' do
     let(:repo_path) do
       Gitlab::GitalyClient::StorageSettings.allow_disk_access do
@@ -431,6 +449,23 @@ RSpec.describe MergeRequest, factory_default: :keep do
     end
   end
 
+  describe '.sort_by_attribute' do
+    context 'merged_at' do
+      let_it_be(:older_mr) { create(:merge_request, :with_merged_metrics) }
+      let_it_be(:newer_mr) { create(:merge_request, :with_merged_metrics) }
+
+      it 'sorts asc' do
+        merge_requests = described_class.sort_by_attribute(:merged_at_asc)
+        expect(merge_requests).to eq([older_mr, newer_mr])
+      end
+
+      it 'sorts desc' do
+        merge_requests = described_class.sort_by_attribute(:merged_at_desc)
+        expect(merge_requests).to eq([newer_mr, older_mr])
+      end
+    end
+  end
+
   describe '#target_branch_sha' do
     let(:project) { create(:project, :repository) }
 

spec/requests/api/graphql/project/merge_requests_spec.rb

@@ -210,4 +210,48 @@ RSpec.describe 'getting merge request listings nested in a project' do
       include_examples 'N+1 query check'
     end
   end
+  describe 'sorting and pagination' do
+    let(:data_path) { [:project, :mergeRequests] }
+
+    def pagination_query(params, page_info)
+      graphql_query_for(
+        :project,
+        { full_path: project.full_path },
+        <<~QUERY
+        mergeRequests(#{params}) {
+          #{page_info} edges {
+            node {
+              id
+            }
+          }
+        }
+        QUERY
+      )
+    end
+
+    def pagination_results_data(data)
+      data.map { |project| project.dig('node', 'id') }
+    end
+
+    context 'when sorting by merged_at DESC' do
+      it_behaves_like 'sorted paginated query' do
+        let(:sort_param) { 'MERGED_AT_DESC' }
+        let(:first_param) { 2 }
+
+        let(:expected_results) do
+          [
+            merge_request_b,
+            merge_request_c,
+            merge_request_d,
+            merge_request_a
+          ].map(&:to_gid).map(&:to_s)
+        end
+
+        before do
+          merge_request_c.metrics.update!(merged_at: 5.days.ago)
+          merge_request_b.metrics.update!(merged_at: 1.day.ago)
+        end
+      end
+    end
+  end
 end