Add ability to sort to search API

Sorting already exists for search results from the web. Most of this
code for searching is shared. Unfortunately in the web search results we
had to implement this by using a single parameter. In order for our API
to be in line with other APIs it seemed preferable to implement sorting
as 2 params (field, direction).

To make matters worse the web sorting does things like `created_asc` for
sorting by `created_at asc`. So it wasn't even perfectly constructed by
concatenating field name and direction. Unfortunately the simplest
option here seemed to be to push the understanding of how to interpret
these different params into the searching code so it needs to handle 2
different meanings for the `sort` param.

I considered changing the web code to do a translation of params before
passing down to the `SearchService` but this was tricky due `params`
being passed in and also `@sort` is used by the view. We may in future
be able to refactor this to use 2 params but it seemed tricky to
introduce now with a single form field.

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 [options[:order_by], options[:sort]]
+        when %w[created_at asc], [nil, 'created_asc']
           query_hash.merge(sort: {
             created_at: {
               order: 'asc'
             }
           })
-        when 'created_desc'
+        when %w[created_at desc], [nil, 'created_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_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 [order_by, sort]
+      when %w[created_at asc], [nil, 'created_asc']
         scope.reorder('created_at ASC')
-      when 'created_desc'
+      when %w[created_at desc], [nil, 'created_desc']
         scope.reorder('created_at DESC')
       else
         scope.reorder('created_at DESC')

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')