Commit ab9d6cd1 authored by James Fargher's avatar James Fargher

Fix N+1 in storage move API

Also improves permissions coverage
parent 07885680
...@@ -54,4 +54,5 @@ class ProjectRepositoryStorageMove < ApplicationRecord ...@@ -54,4 +54,5 @@ class ProjectRepositoryStorageMove < ApplicationRecord
end end
scope :order_created_at_desc, -> { order(created_at: :desc) } scope :order_created_at_desc, -> { order(created_at: :desc) }
scope :with_projects, -> { includes(project: :route) }
end end
...@@ -15,7 +15,7 @@ module API ...@@ -15,7 +15,7 @@ module API
use :pagination use :pagination
end end
get do get do
storage_moves = ProjectRepositoryStorageMove.order_created_at_desc storage_moves = ProjectRepositoryStorageMove.with_projects.order_created_at_desc
present paginate(storage_moves), with: Entities::ProjectRepositoryStorageMove, current_user: current_user present paginate(storage_moves), with: Entities::ProjectRepositoryStorageMove, current_user: current_user
end end
......
...@@ -3,6 +3,8 @@ ...@@ -3,6 +3,8 @@
require 'spec_helper' require 'spec_helper'
describe API::ProjectRepositoryStorageMoves do describe API::ProjectRepositoryStorageMoves do
include AccessMatchersForRequest
let(:user) { create(:admin) } let(:user) { create(:admin) }
let!(:storage_move) { create(:project_repository_storage_move, :scheduled) } let!(:storage_move) { create(:project_repository_storage_move, :scheduled) }
...@@ -23,6 +25,9 @@ describe API::ProjectRepositoryStorageMoves do ...@@ -23,6 +25,9 @@ describe API::ProjectRepositoryStorageMoves do
end end
it 'avoids N+1 queries', :request_store do it 'avoids N+1 queries', :request_store do
# prevent `let` from polluting the control
get_project_repository_storage_moves
control = ActiveRecord::QueryRecorder.new { get_project_repository_storage_moves } control = ActiveRecord::QueryRecorder.new { get_project_repository_storage_moves }
create(:project_repository_storage_move, :scheduled) create(:project_repository_storage_move, :scheduled)
...@@ -43,16 +48,42 @@ describe API::ProjectRepositoryStorageMoves do ...@@ -43,16 +48,42 @@ describe API::ProjectRepositoryStorageMoves do
storage_move_oldest.id storage_move_oldest.id
]) ])
end end
describe 'permissions' do
it { expect { get_project_repository_storage_moves }.to be_allowed_for(:admin) }
it { expect { get_project_repository_storage_moves }.to be_denied_for(:user) }
end
end end
describe 'GET /project_repository_storage_moves/:id' do describe 'GET /project_repository_storage_moves/:id' do
let(:project_repository_storage_move_id) { storage_move.id }
def get_project_repository_storage_move
get api("/project_repository_storage_moves/#{project_repository_storage_move_id}", user)
end
it 'returns a project repository storage move' do it 'returns a project repository storage move' do
get api("/project_repository_storage_moves/#{storage_move.id}", user) get_project_repository_storage_move
expect(response).to have_gitlab_http_status(:ok) expect(response).to have_gitlab_http_status(:ok)
expect(response).to match_response_schema('public_api/v4/project_repository_storage_move') expect(response).to match_response_schema('public_api/v4/project_repository_storage_move')
expect(json_response['id']).to eq(storage_move.id) expect(json_response['id']).to eq(storage_move.id)
expect(json_response['state']).to eq(storage_move.human_state_name) expect(json_response['state']).to eq(storage_move.human_state_name)
end end
context 'non-existent project repository storage move' do
let(:project_repository_storage_move_id) { non_existing_record_id }
it 'returns not found' do
get_project_repository_storage_move
expect(response).to have_gitlab_http_status(:not_found)
end
end
describe 'permissions' do
it { expect { get_project_repository_storage_move }.to be_allowed_for(:admin) }
it { expect { get_project_repository_storage_move }.to be_denied_for(:user) }
end
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