Tie breaker in Deployment Finder should respect the sort order

This commit fixes the performance issue that the expected
database index is not used because the sort order is different
between updated_at and id.

app/finders/deployments_finder.rb

@@ -131,20 +131,22 @@ class DeploymentsFinder
   end
 
   def optimize_sort_params!(sort_params)
+    sort_direction = sort_params.each_value.first
+
     # Implicitly enforce the ordering when filtered by `updated_at` column for performance optimization.
     # See https://gitlab.com/gitlab-org/gitlab/-/issues/325627#note_552417509.
     # We remove this in https://gitlab.com/gitlab-org/gitlab/-/issues/328500.
     if filter_by_updated_at? && implicitly_enforce_ordering_for_updated_at_filter?
-      sort_params.replace('updated_at' => sort_params.each_value.first)
+      sort_params.replace('updated_at' => sort_direction)
     end
 
     if sort_params['created_at'] || sort_params['iid']
       # Sorting by `id` produces the same result as sorting by `created_at` or `iid`
-      sort_params.replace(id: sort_params.each_value.first)
+      sort_params.replace(id: sort_direction)
     elsif sort_params['updated_at']
       # This adds the order as a tie-breaker when multiple rows have the same updated_at value.
       # See https://gitlab.com/gitlab-org/gitlab/-/merge_requests/20848.
-      sort_params.merge!(id: :desc)
+      sort_params.merge!(id: sort_direction)
     end
   end
 

changelogs/unreleased/tie-breaker-should-respect-the-original-sort.yml

+---
+title: Tie-breaker in Deployment Finder should respect the original sort direction
+merge_request: 61444
+author:
+type: performance

spec/finders/deployments_finder_spec.rb

@@ -201,17 +201,31 @@ RSpec.describe DeploymentsFinder do
         it 'adds `id` sorting as the second order column' do
           order_value = subject.order_values[1]
 
-          expect(order_value.to_sql).to eq(Deployment.arel_table[:id].desc.to_sql)
+          expect(order_value.to_sql).to eq(Deployment.arel_table[:id].asc.to_sql)
         end
 
-        it 'uses the `id DESC` as tie-breaker when ordering' do
+        it 'uses the `id ASC` as tie-breaker when ordering' do
           updated_at = Time.now
 
           deployment_1 = create(:deployment, :success, project: project, updated_at: updated_at)
           deployment_2 = create(:deployment, :success, project: project, updated_at: updated_at)
           deployment_3 = create(:deployment, :success, project: project, updated_at: updated_at)
 
-          expect(subject).to eq([deployment_3, deployment_2, deployment_1])
+          expect(subject).to eq([deployment_1, deployment_2, deployment_3])
+        end
+
+        context 'when sort direction is desc' do
+          let(:params) { { **base_params, updated_after: 1.day.ago, order_by: 'updated_at', sort: 'desc' } }
+
+          it 'uses the `id DESC` as tie-breaker when ordering' do
+            updated_at = Time.now
+
+            deployment_1 = create(:deployment, :success, project: project, updated_at: updated_at)
+            deployment_2 = create(:deployment, :success, project: project, updated_at: updated_at)
+            deployment_3 = create(:deployment, :success, project: project, updated_at: updated_at)
+
+            expect(subject).to eq([deployment_3, deployment_2, deployment_1])
+          end
         end
       end
 
@@ -228,7 +242,7 @@ RSpec.describe DeploymentsFinder do
 
         it 'sorts by `updated_at`' do
           expect(subject.order_values.first.to_sql).to eq(Deployment.arel_table[:updated_at].asc.to_sql)
-          expect(subject.order_values.second.to_sql).to eq(Deployment.arel_table[:id].desc.to_sql)
+          expect(subject.order_values.second.to_sql).to eq(Deployment.arel_table[:id].asc.to_sql)
         end
 
         context 'when deployments_finder_implicitly_enforce_ordering_for_updated_at_filter feature flag is disabled' do