Merge branch '251211-add-sorting-to-search-api' into 'master'

Add sorting to search APIs

See merge request gitlab-org/gitlab!46646

app/services/search/global_service.rb

@@ -16,6 +16,7 @@ module Search
       Gitlab::SearchResults.new(current_user,
                                 params[:search],
                                 projects,
+                                order_by: params[:order_by],
                                 sort: params[:sort],
                                 filters: { state: params[:state], confidential: params[:confidential] })
     end

app/services/search/group_service.rb

@@ -16,6 +16,7 @@ module Search
         params[:search],
         projects,
         group: group,
+        order_by: params[:order_by],
         sort: params[:sort],
         filters: { state: params[:state], confidential: params[:confidential] }
       )

app/services/search/project_service.rb

@@ -17,6 +17,7 @@ module Search
                                        params[:search],
                                        project: project,
                                        repository_ref: params[:repository_ref],
+                                       order_by: params[:order_by],
                                        sort: params[:sort],
                                        filters: { confidential: params[:confidential], state: params[:state] }
                                       )

changelogs/unreleased/251211-add-sorting-to-search-api.yml

+---
+title: Add ability to sort to search API
+merge_request: 46646
+author:
+type: added

doc/api/search.md

@@ -26,6 +26,8 @@ GET /search
 | `search`      | string   | yes        | The search query  |
 | `state`       | string   | no        | Filter by state. Issues and merge requests are supported; it is ignored for other scopes. |
 | `confidential` | boolean   | no         | Filter by confidentiality. Issues scope is supported; it is ignored for other scopes. |
+| `order_by`    | string   | no         | Allowed values are `created_at` only. If this is not set, the results will either be sorted by `created_at` in descending order for basic search, or by the most relevant documents when using advanced search.|
+| `sort`    | string   | no         | Allowed values are `asc` or `desc` only. If this is not set, the results will either be sorted by `created_at` in descending order for basic search, or by the most relevant documents when using advanced search.|
 
 Search the expression within the specified scope. Currently these scopes are supported: projects, issues, merge_requests, milestones, snippet_titles, users.
 
@@ -436,6 +438,8 @@ GET /groups/:id/search
 | `search`      | string   | yes        | The search query  |
 | `state`       | string   | no        | Filter by state. Issues and merge requests are supported; it is ignored for other scopes. |
 | `confidential` | boolean   | no         | Filter by confidentiality. Issues scope is supported; it is ignored for other scopes. |
+| `order_by`    | string   | no         | Allowed values are `created_at` only. If this is not set, the results will either be sorted by `created_at` in descending order for basic search, or by the most relevant documents when using advanced search.|
+| `sort`    | string   | no         | Allowed values are `asc` or `desc` only. If this is not set, the results will either be sorted by `created_at` in descending order for basic search, or by the most relevant documents when using advanced search.|
 
 Search the expression within the specified scope. Currently these scopes are supported: projects, issues, merge_requests, milestones, users.
 
@@ -816,6 +820,8 @@ GET /projects/:id/search
 | `ref`         | string   | no         | The name of a repository branch or tag to search on. The project's default branch is used by default. This is only applicable for scopes: commits, blobs, and wiki_blobs.  |
 | `state`       | string   | no        | Filter by state. Issues and merge requests are supported; it is ignored for other scopes. |
 | `confidential` | boolean   | no         | Filter by confidentiality. Issues scope is supported; it is ignored for other scopes. |
+| `order_by`    | string   | no         | Allowed values are `created_at` only. If this is not set, the results will either be sorted by `created_at` in descending order for basic search, or by the most relevant documents when using advanced search.|
+| `sort`    | string   | no         | Allowed values are `asc` or `desc` only. If this is not set, the results will either be sorted by `created_at` in descending order for basic search, or by the most relevant documents when using advanced search.|
 
 Search the expression within the specified scope. Currently these scopes are supported: issues, merge_requests, milestones, notes, wiki_blobs, commits, blobs, users.
 

ee/app/services/ee/search/global_service.rb

@@ -16,6 +16,7 @@ module EE
           params[:search],
           elastic_projects,
           public_and_internal_projects: elastic_global,
+          order_by: params[:order_by],
           sort: params[:sort],
           filters: { confidential: params[:confidential], state: params[:state] }
         )

ee/app/services/ee/search/group_service.rb

@@ -30,6 +30,7 @@ module EE
           elastic_projects,
           group: group,
           public_and_internal_projects: elastic_global,
+          order_by: params[:order_by],
           sort: params[:sort],
           filters: { confidential: params[:confidential], state: params[:state] }
         )

ee/app/services/ee/search/project_service.rb

@@ -15,6 +15,7 @@ module EE
           params[:search],
           project: project,
           repository_ref: repository_ref,
+          order_by: params[:order_by],
           sort: params[:sort],
           filters: { confidential: params[:confidential], state: params[:state] }
         )

ee/lib/elastic/latest/application_class_proxy.rb

@@ -137,14 +137,16 @@ module Elastic
       end
 
       def apply_sort(query_hash, options)
-        case options[:sort]
-        when 'created_asc'
+        # Due to different uses of sort param we prefer order_by when
+        # present
+        case ::Gitlab::Search::SortOptions.sort_and_direction(options[:order_by], options[:sort])
+        when :created_at_asc
           query_hash.merge(sort: {
             created_at: {
               order: 'asc'
             }
           })
-        when 'created_desc'
+        when :created_at_desc
           query_hash.merge(sort: {
             created_at: {
               order: 'desc'

ee/lib/gitlab/elastic/group_search_results.rb

@@ -8,13 +8,15 @@ module Gitlab
     class GroupSearchResults < Gitlab::Elastic::SearchResults
       attr_reader :group, :default_project_filter, :filters
 
-      def initialize(current_user, query, limit_project_ids = nil, group:, public_and_internal_projects: false, default_project_filter: false, sort: nil, filters: {})
+      # rubocop:disable Metrics/ParameterLists
+      def initialize(current_user, query, limit_project_ids = nil, group:, public_and_internal_projects: false, default_project_filter: false, order_by: nil, sort: nil, filters: {})
         @group = group
         @default_project_filter = default_project_filter
         @filters = filters
 
-        super(current_user, query, limit_project_ids, public_and_internal_projects: public_and_internal_projects, sort: sort, filters: filters)
+        super(current_user, query, limit_project_ids, public_and_internal_projects: public_and_internal_projects, order_by: order_by, sort: sort, filters: filters)
       end
+      # rubocop:enable Metrics/ParameterLists
     end
   end
 end

ee/lib/gitlab/elastic/project_search_results.rb

@@ -8,11 +8,11 @@ module Gitlab
     class ProjectSearchResults < Gitlab::Elastic::SearchResults
       attr_reader :project, :repository_ref, :filters
 
-      def initialize(current_user, query, project:, repository_ref: nil, sort: nil, filters: {})
+      def initialize(current_user, query, project:, repository_ref: nil, order_by: nil, sort: nil, filters: {})
         @project = project
         @repository_ref = repository_ref.presence || project.default_branch
 
-        super(current_user, query, [project.id], public_and_internal_projects: false, sort: sort, filters: filters)
+        super(current_user, query, [project.id], public_and_internal_projects: false, order_by: order_by, sort: sort, filters: filters)
       end
 
       private

ee/lib/gitlab/elastic/search_results.rb

@@ -7,17 +7,18 @@ module Gitlab
 
       DEFAULT_PER_PAGE = Gitlab::SearchResults::DEFAULT_PER_PAGE
 
-      attr_reader :current_user, :query, :public_and_internal_projects, :sort, :filters
+      attr_reader :current_user, :query, :public_and_internal_projects, :order_by, :sort, :filters
 
       # Limit search results by passed projects
       # It allows us to search only for projects user has access to
       attr_reader :limit_project_ids
 
-      def initialize(current_user, query, limit_project_ids = nil, public_and_internal_projects: true, sort: nil, filters: {})
+      def initialize(current_user, query, limit_project_ids = nil, public_and_internal_projects: true, order_by: nil, sort: nil, filters: {})
         @current_user = current_user
         @query = query
         @limit_project_ids = limit_project_ids
         @public_and_internal_projects = public_and_internal_projects
+        @order_by = order_by
         @sort = sort
         @filters = filters
       end
@@ -202,6 +203,7 @@ module Gitlab
           current_user: current_user,
           project_ids: limit_project_ids,
           public_and_internal_projects: public_and_internal_projects,
+          order_by: order_by,
           sort: sort
         }
       end
@@ -214,7 +216,7 @@ module Gitlab
 
       def issues
         strong_memoize(:issues) do
-          options = base_options.merge(filters.slice(:sort, :confidential, :state))
+          options = base_options.merge(filters.slice(:order_by, :sort, :confidential, :state))
 
           Issue.elastic_search(query, options: options)
         end
@@ -235,7 +237,7 @@ module Gitlab
 
       def merge_requests
         strong_memoize(:merge_requests) do
-          options = base_options.merge(filters.slice(:sort, :state))
+          options = base_options.merge(filters.slice(:order_by, :sort, :state))
 
           MergeRequest.elastic_search(query, options: options)
         end

ee/spec/requests/api/search_spec.rb

@@ -22,6 +22,7 @@ RSpec.describe API::Search, factory_default: :keep do
     it 'returns a different result for each page' do
       get api(endpoint, user), params: { scope: scope, search: search, page: 1, per_page: 1 }
 
+      expect(response).to have_gitlab_http_status(:success)
       expect(json_response.count).to eq(1)
 
       first = json_response.first
@@ -37,6 +38,30 @@ RSpec.describe API::Search, factory_default: :keep do
     end
   end
 
+  shared_examples 'orderable by created_at' do |scope:|
+    it 'allows ordering results by created_at asc' do
+      get api(endpoint, user), params: { scope: scope, search: '*', order_by: 'created_at', sort: 'asc' }
+
+      expect(response).to have_gitlab_http_status(:success)
+      expect(json_response.count).to be > 1
+
+      created_ats = json_response.map { |r| Time.parse(r['created_at']) }
+
+      expect(created_ats).to eq(created_ats.sort)
+    end
+
+    it 'allows ordering results by created_at desc' do
+      get api(endpoint, user), params: { scope: scope, search: '*', order_by: 'created_at', sort: 'desc' }
+
+      expect(response).to have_gitlab_http_status(:success)
+      expect(json_response.count).to be > 1
+
+      created_ats = json_response.map { |r| Time.parse(r['created_at']) }
+
+      expect(created_ats).to eq(created_ats.sort.reverse)
+    end
+  end
+
   shared_examples 'elasticsearch disabled' do
     it 'returns 400 error for wiki_blobs, blobs and commits scope' do
       get api(endpoint, user), params: { scope: 'wiki_blobs', search: 'awesome' }
@@ -61,6 +86,7 @@ RSpec.describe API::Search, factory_default: :keep do
       end
 
       it_behaves_like 'pagination', scope: 'merge_requests'
+      it_behaves_like 'orderable by created_at', scope: 'merge_requests'
 
       it 'avoids N+1 queries' do
         control = ActiveRecord::QueryRecorder.new { get api(endpoint, user), params: { scope: 'merge_requests', search: '*' } }
@@ -213,6 +239,7 @@ RSpec.describe API::Search, factory_default: :keep do
       end
 
       it_behaves_like 'pagination', scope: 'issues'
+      it_behaves_like 'orderable by created_at', scope: 'issues'
     end
 
     unless level == :project

lib/api/search.rb

@@ -39,7 +39,9 @@ module API
           snippets: snippets?,
           basic_search: params[:basic_search],
           page: params[:page],
-          per_page: params[:per_page]
+          per_page: params[:per_page],
+          order_by: params[:order_by],
+          sort: params[:sort]
         }.merge(additional_params)
 
         results = SearchService.new(current_user, search_params).search_objects(preload_method)

lib/gitlab/group_search_results.rb

@@ -4,10 +4,10 @@ module Gitlab
   class GroupSearchResults < SearchResults
     attr_reader :group
 
-    def initialize(current_user, query, limit_projects = nil, group:, default_project_filter: false, sort: nil, filters: {})
+    def initialize(current_user, query, limit_projects = nil, group:, default_project_filter: false, order_by: nil, sort: nil, filters: {})
       @group = group
 
-      super(current_user, query, limit_projects, default_project_filter: default_project_filter, sort: sort, filters: filters)
+      super(current_user, query, limit_projects, default_project_filter: default_project_filter, order_by: order_by, sort: sort, filters: filters)
     end
 
     # rubocop:disable CodeReuse/ActiveRecord

lib/gitlab/project_search_results.rb

@@ -4,11 +4,11 @@ module Gitlab
   class ProjectSearchResults < SearchResults
     attr_reader :project, :repository_ref
 
-    def initialize(current_user, query, project:, repository_ref: nil, sort: nil, filters: {})
+    def initialize(current_user, query, project:, repository_ref: nil, order_by: nil, sort: nil, filters: {})
       @project = project
       @repository_ref = repository_ref.presence
 
-      super(current_user, query, [project], sort: sort, filters: filters)
+      super(current_user, query, [project], order_by: order_by, sort: sort, filters: filters)
     end
 
     def objects(scope, page: nil, per_page: DEFAULT_PER_PAGE, preload_method: nil)

lib/gitlab/search/sort_options.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Search
+    module SortOptions
+      def sort_and_direction(order_by, sort)
+        # Due to different uses of sort param in web vs. API requests we prefer
+        # order_by when present
+        case [order_by, sort]
+        when %w[created_at asc], [nil, 'created_asc']
+          :created_at_asc
+        when %w[created_at desc], [nil, 'created_desc']
+          :created_at_desc
+        else
+          :unknown
+        end
+      end
+      module_function :sort_and_direction # rubocop: disable Style/AccessModifierDeclarations
+    end
+  end
+end

lib/gitlab/search_results.rb

@@ -7,7 +7,7 @@ module Gitlab
     DEFAULT_PAGE = 1
     DEFAULT_PER_PAGE = 20
 
-    attr_reader :current_user, :query, :sort, :filters
+    attr_reader :current_user, :query, :order_by, :sort, :filters
 
     # Limit search results by passed projects
     # It allows us to search only for projects user has access to
@@ -19,11 +19,12 @@ module Gitlab
     # query
     attr_reader :default_project_filter
 
-    def initialize(current_user, query, limit_projects = nil, sort: nil, default_project_filter: false, filters: {})
+    def initialize(current_user, query, limit_projects = nil, order_by: nil, sort: nil, default_project_filter: false, filters: {})
       @current_user = current_user
       @query = query
       @limit_projects = limit_projects || Project.all
       @default_project_filter = default_project_filter
+      @order_by = order_by
       @sort = sort
       @filters = filters
     end
@@ -128,10 +129,12 @@ module Gitlab
 
     # rubocop: disable CodeReuse/ActiveRecord
     def apply_sort(scope)
-      case sort
-      when 'created_asc'
+      # Due to different uses of sort param we prefer order_by when
+      # present
+      case ::Gitlab::Search::SortOptions.sort_and_direction(order_by, sort)
+      when :created_at_asc
         scope.reorder('created_at ASC')
-      when 'created_desc'
+      when :created_at_desc
         scope.reorder('created_at DESC')
       else
         scope.reorder('created_at DESC')

spec/lib/gitlab/search/sort_options_spec.rb

+# frozen_string_literal: true
+
+require 'fast_spec_helper'
+require 'gitlab/search/sort_options'
+
+RSpec.describe ::Gitlab::Search::SortOptions do
+  describe '.sort_and_direction' do
+    context 'using order_by and sort' do
+      it 'returns matched options' do
+        expect(described_class.sort_and_direction('created_at', 'asc')).to eq(:created_at_asc)
+        expect(described_class.sort_and_direction('created_at', 'desc')).to eq(:created_at_desc)
+      end
+    end
+
+    context 'using just sort' do
+      it 'returns matched options' do
+        expect(described_class.sort_and_direction(nil, 'created_asc')).to eq(:created_at_asc)
+        expect(described_class.sort_and_direction(nil, 'created_desc')).to eq(:created_at_desc)
+      end
+    end
+
+    context 'when unknown option' do
+      it 'returns unknown' do
+        expect(described_class.sort_and_direction(nil, 'foo_asc')).to eq(:unknown)
+        expect(described_class.sort_and_direction(nil, 'bar_desc')).to eq(:unknown)
+        expect(described_class.sort_and_direction(nil, 'created_bar')).to eq(:unknown)
+
+        expect(described_class.sort_and_direction('created_at', 'foo')).to eq(:unknown)
+        expect(described_class.sort_and_direction('foo', 'desc')).to eq(:unknown)
+        expect(described_class.sort_and_direction('created_at', nil)).to eq(:unknown)
+      end
+    end
+  end
+end

spec/requests/api/search_spec.rb

@@ -23,6 +23,48 @@ RSpec.describe API::Search do
     end
   end
 
+  shared_examples 'orderable by created_at' do |scope:|
+    it 'allows ordering results by created_at asc' do
+      get api(endpoint, user), params: { scope: scope, search: 'sortable', order_by: 'created_at', sort: 'asc' }
+
+      expect(response).to have_gitlab_http_status(:success)
+      expect(json_response.count).to be > 1
+
+      created_ats = json_response.map { |r| Time.parse(r['created_at']) }
+      expect(created_ats.uniq.count).to be > 1
+
+      expect(created_ats).to eq(created_ats.sort)
+    end
+
+    it 'allows ordering results by created_at desc' do
+      get api(endpoint, user), params: { scope: scope, search: 'sortable', order_by: 'created_at', sort: 'desc' }
+
+      expect(response).to have_gitlab_http_status(:success)
+      expect(json_response.count).to be > 1
+
+      created_ats = json_response.map { |r| Time.parse(r['created_at']) }
+      expect(created_ats.uniq.count).to be > 1
+
+      expect(created_ats).to eq(created_ats.sort.reverse)
+    end
+  end
+
+  shared_examples 'issues orderable by created_at' do
+    before do
+      create_list(:issue, 3, title: 'sortable item', project: project)
+    end
+
+    it_behaves_like 'orderable by created_at', scope: :issues
+  end
+
+  shared_examples 'merge_requests orderable by created_at' do
+    before do
+      create_list(:merge_request, 3, :unique_branches, title: 'sortable item', target_project: repo_project, source_project: repo_project)
+    end
+
+    it_behaves_like 'orderable by created_at', scope: :merge_requests
+  end
+
   shared_examples 'pagination' do |scope:, search: ''|
     it 'returns a different result for each page' do
       get api(endpoint, user), params: { scope: scope, search: search, page: 1, per_page: 1 }
@@ -121,6 +163,8 @@ RSpec.describe API::Search do
 
           it_behaves_like 'ping counters', scope: :issues
 
+          it_behaves_like 'issues orderable by created_at'
+
           describe 'pagination' do
             before do
               create(:issue, project: project, title: 'another issue')
@@ -181,6 +225,8 @@ RSpec.describe API::Search do
 
           it_behaves_like 'ping counters', scope: :merge_requests
 
+          it_behaves_like 'merge_requests orderable by created_at'
+
           describe 'pagination' do
             before do
               create(:merge_request, source_project: repo_project, title: 'another mr', target_branch: 'another_branch')
@@ -354,6 +400,8 @@ RSpec.describe API::Search do
 
         it_behaves_like 'ping counters', scope: :issues
 
+        it_behaves_like 'issues orderable by created_at'
+
         describe 'pagination' do
           before do
             create(:issue, project: project, title: 'another issue')
@@ -374,6 +422,8 @@ RSpec.describe API::Search do
 
         it_behaves_like 'ping counters', scope: :merge_requests
 
+        it_behaves_like 'merge_requests orderable by created_at'
+
         describe 'pagination' do
           before do
             create(:merge_request, source_project: repo_project, title: 'another mr', target_branch: 'another_branch')
@@ -506,6 +556,8 @@ RSpec.describe API::Search do
 
         it_behaves_like 'ping counters', scope: :issues
 
+        it_behaves_like 'issues orderable by created_at'
+
         describe 'pagination' do
           before do
             create(:issue, project: project, title: 'another issue')
@@ -536,6 +588,8 @@ RSpec.describe API::Search do
 
         it_behaves_like 'ping counters', scope: :merge_requests
 
+        it_behaves_like 'merge_requests orderable by created_at'
+
         describe 'pagination' do
           before do
             create(:merge_request, source_project: repo_project, title: 'another mr', target_branch: 'another_branch')