Merge branch 'tie-breaker-should-respect-the-original-sort' into 'master'

Tie-breaker in Deployment Finder should respect the original sort direction

See merge request gitlab-org/gitlab!61444

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,9 +201,22 @@ 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 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_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
 
@@ -214,6 +227,7 @@ RSpec.describe DeploymentsFinder do
             expect(subject).to eq([deployment_3, deployment_2, deployment_1])
           end
         end
+      end
 
       describe 'enforce sorting to `updated_at` sorting' do
         let(:params) { { **base_params, updated_before: 1.day.ago, order_by: 'id', sort: 'asc' } }
@@ -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