Commit 2b323f6b authored by Adam Hegyi's avatar Adam Hegyi

More consistent deployment API ordering

- Add tie-breaker `id desc` sorting to deployments API.
- Preload association to prevent n+1 queries.
- Turn `created_at` sorting to `id` sorting.
- Add index to speed up `updated_at` sorting.
- Add index to speed up `id` sorting.
parent bfd6f3db
......@@ -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
---
title: Optimize Deployments endpoint by preloading associations and make record ordering more consistent
merge_request: 20848
author:
type: changed
# 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
......@@ -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"
......@@ -1338,10 +1338,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|
......
......@@ -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
......@@ -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
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment