Merge branch '36301-add-tie-breaker-id-sorting-to-deployments' into 'master'

More consistent deployment API ordering

See merge request gitlab-org/gitlab!20848

app/finders/deployments_finder.rb

@@ -22,9 +22,28 @@ class DeploymentsFinder
 
   private
 
+  # rubocop: disable CodeReuse/ActiveRecord
   def init_collection
-    project.deployments
+    project
+      .deployments
+      .includes(
+        :user,
+        environment: [],
+        deployable: {
+          job_artifacts: [],
+          pipeline: {
+            project: {
+              route: [],
+              namespace: :route
+            }
+          },
+          project: {
+            namespace: :route
+          }
+        }
+      )
   end
+  # rubocop: enable CodeReuse/ActiveRecord
 
   # rubocop: disable CodeReuse/ActiveRecord
   def sort(items)
@@ -43,6 +62,9 @@ class DeploymentsFinder
     order_by = ALLOWED_SORT_VALUES.include?(params[:order_by]) ? params[:order_by] : DEFAULT_SORT_VALUE
     order_direction = ALLOWED_SORT_DIRECTIONS.include?(params[:sort]) ? params[:sort] : DEFAULT_SORT_DIRECTION
 
-    { order_by => order_direction }
+    { order_by => order_direction }.tap do |sort_values|
+      sort_values['id'] = 'desc' if sort_values['updated_at']
+      sort_values['id'] = sort_values.delete('created_at') if sort_values['created_at'] # Sorting by `id` produces the same result as sorting by `created_at`
+    end
   end
 end

changelogs/unreleased/36301-add-tie-breaker-id-sorting-to-deployments.yml

+---
+title: Optimize Deployments endpoint by preloading associations and make record ordering more consistent
+merge_request: 20848
+author:
+type: changed

db/migrate/20191204070713_change_updated_at_index_and_add_index_to_id_on_deployments.rb

+# frozen_string_literal: true
+
+class ChangeUpdatedAtIndexAndAddIndexToIdOnDeployments < ActiveRecord::Migration[5.2]
+  include Gitlab::Database::MigrationHelpers
+
+  disable_ddl_transaction!
+
+  DOWNTIME = false
+
+  PROJECT_ID_INDEX_PARAMS = [[:project_id, :id], order: { id: :desc }]
+  OLD_UPDATED_AT_INDEX_PARAMS = [[:project_id, :updated_at]]
+  NEW_UPDATED_AT_INDEX_PARAMS = [[:project_id, :updated_at, :id], order: { updated_at: :desc, id: :desc }]
+
+  def up
+    add_concurrent_index :deployments, *NEW_UPDATED_AT_INDEX_PARAMS
+
+    remove_concurrent_index :deployments, *OLD_UPDATED_AT_INDEX_PARAMS
+
+    add_concurrent_index :deployments, *PROJECT_ID_INDEX_PARAMS
+  end
+
+  def down
+    add_concurrent_index :deployments, *OLD_UPDATED_AT_INDEX_PARAMS
+
+    remove_concurrent_index :deployments, *NEW_UPDATED_AT_INDEX_PARAMS
+
+    remove_concurrent_index :deployments, *PROJECT_ID_INDEX_PARAMS
+  end
+end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 2019_12_02_031812) do
+ActiveRecord::Schema.define(version: 2019_12_04_070713) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "pg_trgm"
@@ -1339,10 +1339,11 @@ ActiveRecord::Schema.define(version: 2019_12_02_031812) do
     t.index ["environment_id", "iid", "project_id"], name: "index_deployments_on_environment_id_and_iid_and_project_id"
     t.index ["environment_id", "status"], name: "index_deployments_on_environment_id_and_status"
     t.index ["id"], name: "partial_index_deployments_for_legacy_successful_deployments", where: "((finished_at IS NULL) AND (status = 2))"
+    t.index ["project_id", "id"], name: "index_deployments_on_project_id_and_id", order: { id: :desc }
     t.index ["project_id", "iid"], name: "index_deployments_on_project_id_and_iid", unique: true
     t.index ["project_id", "status", "created_at"], name: "index_deployments_on_project_id_and_status_and_created_at"
     t.index ["project_id", "status"], name: "index_deployments_on_project_id_and_status"
-    t.index ["project_id", "updated_at"], name: "index_deployments_on_project_id_and_updated_at"
+    t.index ["project_id", "updated_at", "id"], name: "index_deployments_on_project_id_and_updated_at_and_id", order: { updated_at: :desc, id: :desc }
   end
 
   create_table "description_versions", force: :cascade do |t|

spec/finders/deployments_finder_spec.rb

@@ -32,13 +32,13 @@ describe DeploymentsFinder do
 
       let(:params) { { order_by: order_by, sort: sort } }
 
-      let!(:deployment_1) { create(:deployment, :success, project: project, iid: 11, ref: 'master', created_at: Time.now, updated_at: Time.now) }
+      let!(:deployment_1) { create(:deployment, :success, project: project, iid: 11, ref: 'master', created_at: 2.days.ago, updated_at: Time.now) }
       let!(:deployment_2) { create(:deployment, :success, project: project, iid: 12, ref: 'feature', created_at: 1.day.ago, updated_at: 2.hours.ago) }
-      let!(:deployment_3) { create(:deployment, :success, project: project, iid: 8, ref: 'patch', created_at: 2.days.ago, updated_at: 1.hour.ago) }
+      let!(:deployment_3) { create(:deployment, :success, project: project, iid: 8, ref: 'patch', created_at: Time.now, updated_at: 1.hour.ago) }
 
       where(:order_by, :sort, :ordered_deployments) do
-        'created_at' | 'asc'  | [:deployment_3, :deployment_2, :deployment_1]
-        'created_at' | 'desc' | [:deployment_1, :deployment_2, :deployment_3]
+        'created_at' | 'asc'  | [:deployment_1, :deployment_2, :deployment_3]
+        'created_at' | 'desc' | [:deployment_3, :deployment_2, :deployment_1]
         'id'         | 'asc'  | [:deployment_1, :deployment_2, :deployment_3]
         'id'         | 'desc' | [:deployment_3, :deployment_2, :deployment_1]
         'iid'        | 'asc'  | [:deployment_3, :deployment_1, :deployment_2]
@@ -57,5 +57,41 @@ describe DeploymentsFinder do
         end
       end
     end
+
+    describe 'transform `created_at` sorting to `id` sorting' do
+      let(:params) { { order_by: 'created_at', sort: 'asc' } }
+
+      it 'sorts by only one column' do
+        expect(subject.order_values.size).to eq(1)
+      end
+
+      it 'sorts by `id`' do
+        expect(subject.order_values.first.to_sql).to eq(Deployment.arel_table[:id].asc.to_sql)
+      end
+    end
+
+    describe 'tie-breaker for `updated_at` sorting' do
+      let(:params) { { order_by: 'updated_at', sort: 'asc' } }
+
+      it 'sorts by two columns' do
+        expect(subject.order_values.size).to eq(2)
+      end
+
+      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)
+      end
+
+      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
 end

spec/requests/api/deployments_spec.rb

@@ -332,4 +332,40 @@ describe API::Deployments do
       end
     end
   end
+
+  context 'prevent N + 1 queries' do
+    context 'when the endpoint returns multiple records' do
+      let(:project) { create(:project) }
+
+      def create_record
+        create(:deployment, :success, project: project)
+      end
+
+      def request_with_query_count
+        ActiveRecord::QueryRecorder.new { trigger_request }.count
+      end
+
+      def trigger_request
+        get api("/projects/#{project.id}/deployments?order_by=updated_at&sort=asc", user)
+      end
+
+      before do
+        create_record
+      end
+
+      it 'succeeds' do
+        trigger_request
+
+        expect(response).to have_gitlab_http_status(200)
+
+        expect(json_response.size).to eq(1)
+      end
+
+      it 'does not increase the query count' do
+        expect { create_record }.not_to change { request_with_query_count }
+
+        expect(json_response.size).to eq(2)
+      end
+    end
+  end
 end