Commit 733d5df3 authored by Alexandru Croitor's avatar Alexandru Croitor

Reduce number of queries for finding iterations

Reduce number of queries made when finding iterations, that is
due to the fact that iterations are accessible throughout the
group hierarchy and not available at project level. So we do not
need to check for project iterations and it is eniough to check
that user can read iterations at the hierarchy leaf group.
parent 0fc3ec38
......@@ -126,10 +126,13 @@ module EE
end
def iterations_finder_params
IterationsFinder.params_for_parent(params.parent, include_ancestors: true).merge!(
{
parent: params.parent,
include_ancestors: true,
state: 'opened',
start_date: Date.today,
end_date: Date.today)
end_date: Date.today
}
end
end
end
......@@ -3,8 +3,8 @@
# Search for iterations
#
# params - Hash
# project_ids: Array of project ids or single project id or ActiveRecord relation.
# group_ids: Array of group ids or single group id or ActiveRecord relation.
# parent - The group in which to look-up iterations.
# include_ancestors - whether to look-up iterations in group ancestors.
# order - Orders by field default due date asc.
# title - Filter by title.
# state - Filters by state.
......@@ -15,39 +15,16 @@ class IterationsFinder
attr_reader :params, :current_user
class << self
def params_for_parent(parent, include_ancestors: false)
case parent
when Group
if include_ancestors
{ group_ids: parent.self_and_ancestors.select(:id) }
else
{ group_ids: parent.id }
end
when Project
if include_ancestors && parent.parent_id.present?
{ group_ids: parent.parent.self_and_ancestors.select(:id), project_ids: parent.id }
else
{ project_ids: parent.id }
end
else
raise ArgumentError, 'Invalid parent class. Only Project and Group are supported.'
end
end
end
def initialize(current_user, params = {})
@params = params
@current_user = current_user
end
def execute
filter_permissions
items = Iteration.all
items = by_id(items)
items = by_iid(items)
items = by_groups_and_projects(items)
items = by_groups(items)
items = by_title(items)
items = by_search_title(items)
items = by_state(items)
......@@ -59,33 +36,10 @@ class IterationsFinder
private
def filter_permissions
filter_allowed_projects
filter_allowed_groups
# Only allow either one project_id or one group_id when filtering by `iid`
if params[:iid] && params.slice(:project_ids, :group_ids).keys.count > 1
raise ArgumentError, 'You can specify only one scope if you use iid filter'
end
end
def filter_allowed_projects
return unless params[:project_ids].present?
projects = Project.id_in(params[:project_ids])
params[:project_ids] = Project.projects_user_can(projects, current_user, :read_iteration)
end
def filter_allowed_groups
return unless params[:group_ids].present?
def by_groups(items)
return Iteration.none unless Ability.allowed?(current_user, :read_iteration, params[:parent])
groups = Group.id_in(params[:group_ids])
params[:group_ids] = Group.groups_user_can(groups, current_user, :read_iteration)
end
def by_groups_and_projects(items)
items.for_projects_and_groups(params[:project_ids], params[:group_ids])
items.of_groups(groups)
end
def by_id(items)
......@@ -128,4 +82,19 @@ class IterationsFinder
items.reorder(order_statement).order(:title)
end
# rubocop: enable CodeReuse/ActiveRecord
def groups
parent = params[:parent]
group = case parent
when Group
parent
when Project
parent.parent
else
raise ArgumentError, 'Invalid parent class. Only Project and Group are supported.'
end
params[:include_ancestors] ? group.self_and_ancestors : group
end
end
......@@ -67,7 +67,7 @@ module Mutations
private
def find_object(parent:, id:)
params = IterationsFinder.params_for_parent(parent).merge!(id: id)
params = { parent: parent, id: id }
IterationsFinder.new(context[:current_user], params).execute.first
end
......
......@@ -51,7 +51,9 @@ module Resolvers
private
def iterations_finder_params(args)
IterationsFinder.params_for_parent(parent, include_ancestors: args[:include_ancestors]).merge!(
{
parent: parent,
include_ancestors: args[:include_ancestors],
id: args[:id],
iid: args[:iid],
iteration_cadence_ids: args[:iteration_cadence_ids],
......@@ -59,7 +61,7 @@ module Resolvers
start_date: args.dig(:timeframe, :start) || args[:start_date],
end_date: args.dig(:timeframe, :end) || args[:end_date],
search_title: args[:title]
)
}
end
def parent
......
......@@ -53,7 +53,7 @@ module EE
end
def iterations_finder_params
IterationsFinder.params_for_parent(parent, include_ancestors: true).merge(state: 'all')
{ parent: parent, include_ancestors: true, state: 'all' }
end
end
end
......
......@@ -83,7 +83,7 @@ module EE
end
def find_iteration(board)
parent_params = ::IterationsFinder.params_for_parent(board.resource_parent, include_ancestors: true)
parent_params = { parent: board.resource_parent, include_ancestors: true }
::IterationsFinder.new(current_user, parent_params).find_by(id: params['iteration_id']) # rubocop: disable CodeReuse/ActiveRecord
end
......
......@@ -23,10 +23,12 @@ module API
end
def iterations_finder_params(parent)
IterationsFinder.params_for_parent(parent, include_ancestors: params[:include_ancestors]).merge!(
{
parent: parent,
include_ancestors: params[:include_ancestors],
state: params[:state],
search_title: params[:search]
)
}
end
end
......
......@@ -131,7 +131,7 @@ module EE
end
def find_iterations(project, params = {})
parent_params = ::IterationsFinder.params_for_parent(project, include_ancestors: true)
parent_params = { parent: project, include_ancestors: true }
::IterationsFinder.new(current_user, params.merge(parent_params)).execute
end
......
......@@ -3,27 +3,37 @@
require 'spec_helper'
RSpec.describe IterationsFinder do
let(:now) { Time.now }
let_it_be(:group) { create(:group, :private) }
let_it_be(:root) { create(:group, :private) }
let_it_be(:group) { create(:group, :private, parent: root) }
let_it_be(:project_1) { create(:project, namespace: group) }
let_it_be(:project_2) { create(:project, namespace: group) }
let_it_be(:user) { create(:user) }
let_it_be(:iteration_cadence1) { create(:iterations_cadence, group: group, active: true, duration_in_weeks: 1, title: 'one week iterations') }
let_it_be(:iteration_cadence2) { create(:iterations_cadence, group: group, active: true, duration_in_weeks: 2, title: 'two week iterations') }
let_it_be(:iteration_cadence3) { create(:iterations_cadence, group: root, active: true, duration_in_weeks: 3, title: 'three week iterations') }
let_it_be(:closed_iteration) { create(:closed_iteration, :skip_future_date_validation, iterations_cadence: iteration_cadence2, group: iteration_cadence2.group, start_date: 7.days.ago, due_date: 2.days.ago) }
let_it_be(:started_group_iteration) { create(:started_iteration, :skip_future_date_validation, iterations_cadence: iteration_cadence2, group: iteration_cadence2.group, title: 'one test', start_date: 1.day.ago, due_date: Date.today) }
let_it_be(:upcoming_group_iteration) { create(:iteration, iterations_cadence: iteration_cadence1, group: iteration_cadence1.group, start_date: 1.day.from_now, due_date: 3.days.from_now) }
let_it_be(:root_group_iteration) { create(:started_iteration, iterations_cadence: iteration_cadence3, group: iteration_cadence3.group, start_date: 1.day.from_now, due_date: 2.days.from_now) }
let_it_be(:root_closed_iteration) { create(:closed_iteration, iterations_cadence: iteration_cadence3, group: iteration_cadence3.group, start_date: 1.week.ago, due_date: 1.day.ago) }
let!(:started_group_iteration) { create(:started_iteration, :skip_future_date_validation, iterations_cadence: iteration_cadence2, group: iteration_cadence2.group, title: 'one test', start_date: now - 1.day, due_date: now) }
let!(:upcoming_group_iteration) { create(:iteration, iterations_cadence: iteration_cadence1, group: iteration_cadence1.group, start_date: 1.day.from_now, due_date: 2.days.from_now) }
let!(:iteration_from_project_1) { create(:started_iteration, :skip_project_validation, project: project_1, start_date: 3.days.from_now, due_date: 4.days.from_now) }
let!(:iteration_from_project_2) { create(:started_iteration, :skip_project_validation, project: project_2, start_date: 5.days.from_now, due_date: 6.days.from_now) }
let(:project_ids) { [project_1.id, project_2.id] }
let(:parent) { project_1 }
let(:params) { { parent: parent, include_ancestors: true } }
subject { described_class.new(user, params).execute }
context 'without permissions' do
context 'groups and projects' do
let(:params) { { project_ids: project_ids, group_ids: group.id } }
context 'with project as parent' do
let(:params) { { parent: parent } }
it 'returns iterations for groups and projects' do
it 'returns none' do
expect(subject).to be_empty
end
end
context 'with group as parent' do
let(:params) { { parent: group } }
it 'returns none' do
expect(subject).to be_empty
end
end
......@@ -33,76 +43,59 @@ RSpec.describe IterationsFinder do
before do
group.add_reporter(user)
project_1.add_reporter(user)
project_2.add_reporter(user)
end
context 'iterations for projects' do
let(:params) { { project_ids: project_ids } }
context 'iterations fetched from project' do
let(:params) { { parent: parent } }
it 'returns iterations for projects' do
expect(subject).to contain_exactly(iteration_from_project_1, iteration_from_project_2)
expect(subject).to contain_exactly(closed_iteration, started_group_iteration, upcoming_group_iteration)
end
end
context 'iterations for groups' do
let(:params) { { group_ids: group.id } }
context 'iterations fetched from group' do
let(:params) { { parent: group } }
it 'returns iterations for groups' do
expect(subject).to contain_exactly(started_group_iteration, upcoming_group_iteration)
expect(subject).to contain_exactly(closed_iteration, started_group_iteration, upcoming_group_iteration)
end
end
context 'iterations for groups and project' do
let(:params) { { project_ids: project_ids, group_ids: group.id } }
it 'returns iterations for groups and projects' do
expect(subject).to contain_exactly(started_group_iteration, upcoming_group_iteration, iteration_from_project_1, iteration_from_project_2)
context 'iterations for project with ancestors' do
it 'returns iterations for project and ancestor groups' do
expect(subject).to contain_exactly(root_closed_iteration, root_group_iteration, closed_iteration, started_group_iteration, upcoming_group_iteration)
end
it 'orders iterations by due date' do
iteration = create(:iteration, :skip_future_date_validation, group: group, start_date: now - 3.days, due_date: now - 2.days)
iteration = create(:iteration, :skip_future_date_validation, group: group, start_date: 5.days.ago, due_date: 3.days.ago)
expect(subject.first).to eq(iteration)
expect(subject.second).to eq(started_group_iteration)
expect(subject.third).to eq(upcoming_group_iteration)
expect(subject.to_a).to eq([iteration, closed_iteration, root_closed_iteration, started_group_iteration, root_group_iteration, upcoming_group_iteration])
end
end
context 'with filters' do
let(:params) do
{
project_ids: project_ids,
group_ids: group.id
}
end
before do
started_group_iteration.close
iteration_from_project_1.close
end
it 'filters by all states' do
params[:state] = 'all'
expect(subject).to contain_exactly(started_group_iteration, upcoming_group_iteration, iteration_from_project_1, iteration_from_project_2)
expect(subject).to contain_exactly(root_closed_iteration, root_group_iteration, closed_iteration, started_group_iteration, upcoming_group_iteration)
end
it 'filters by started state' do
params[:state] = 'started'
expect(subject).to contain_exactly(iteration_from_project_2)
expect(subject).to contain_exactly(root_group_iteration, started_group_iteration)
end
it 'filters by opened state' do
params[:state] = 'opened'
expect(subject).to contain_exactly(upcoming_group_iteration, iteration_from_project_2)
expect(subject).to contain_exactly(upcoming_group_iteration, root_group_iteration, started_group_iteration)
end
it 'filters by closed state' do
params[:state] = 'closed'
expect(subject).to contain_exactly(started_group_iteration, iteration_from_project_1)
expect(subject).to contain_exactly(root_closed_iteration, closed_iteration)
end
it 'filters by title' do
......@@ -118,9 +111,9 @@ RSpec.describe IterationsFinder do
end
it 'filters by ID' do
params[:id] = iteration_from_project_1.id
params[:id] = upcoming_group_iteration.id
expect(subject).to contain_exactly(iteration_from_project_1)
expect(subject).to contain_exactly(upcoming_group_iteration)
end
it 'filters by cadence' do
......@@ -132,25 +125,25 @@ RSpec.describe IterationsFinder do
it 'filters by multiple cadences' do
params[:iteration_cadence_ids] = [iteration_cadence1.id, iteration_cadence2.id]
expect(subject).to contain_exactly(started_group_iteration, upcoming_group_iteration)
expect(subject).to contain_exactly(closed_iteration, started_group_iteration, upcoming_group_iteration)
end
context 'by timeframe' do
it 'returns iterations with start_date and due_date between timeframe' do
params.merge!(start_date: now - 1.day, end_date: 3.days.from_now)
params.merge!(start_date: 1.day.ago, end_date: 3.days.from_now)
expect(subject).to match_array([started_group_iteration, upcoming_group_iteration, iteration_from_project_1])
expect(subject).to match_array([started_group_iteration, upcoming_group_iteration, root_group_iteration, root_closed_iteration])
end
it 'returns iterations which start before the timeframe' do
iteration = create(:iteration, :skip_project_validation, :skip_future_date_validation, project: project_2, start_date: now - 5.days, due_date: now - 3.days)
params.merge!(start_date: now - 3.days, end_date: now - 2.days)
iteration = create(:iteration, :skip_project_validation, :skip_future_date_validation, group: group, start_date: 5.days.ago, due_date: 3.days.ago)
params.merge!(start_date: 3.days.ago, end_date: 2.days.ago)
expect(subject).to match_array([iteration])
expect(subject).to match_array([iteration, closed_iteration, root_closed_iteration])
end
it 'returns iterations which end after the timeframe' do
iteration = create(:iteration, :skip_project_validation, project: project_2, start_date: 9.days.from_now, due_date: 2.weeks.from_now)
iteration = create(:iteration, :skip_project_validation, group: group, start_date: 9.days.from_now, due_date: 2.weeks.from_now)
params.merge!(start_date: 9.days.from_now, end_date: 10.days.from_now)
expect(subject).to match_array([iteration])
......@@ -172,79 +165,11 @@ RSpec.describe IterationsFinder do
end
end
describe 'iid' do
let(:params) do
{
project_ids: project_ids,
group_ids: group.id,
iid: iteration_from_project_1.iid
}
end
it 'only accepts one of project_id or group_id' do
expect { subject }.to raise_error(ArgumentError, 'You can specify only one scope if you use iid filter')
end
end
describe '#find_by' do
it 'finds a single iteration' do
finder = described_class.new(user, project_ids: [project_1.id])
expect(finder.find_by(iid: iteration_from_project_1.iid)).to eq(iteration_from_project_1)
end
end
describe '.params_for_parent' do
let_it_be(:parent_group) { create(:group) }
let_it_be(:group) { create(:group, parent: parent_group) }
let_it_be(:project) { create(:project, group: group) }
finder = described_class.new(user, parent: project_1)
context 'when parent is a project' do
subject { described_class.params_for_parent(project, include_ancestors: include_ancestors) }
context 'when include_ancestors is true' do
let(:include_ancestors) { true }
it 'returns project and ancestor group ids' do
expect(subject).to match(group_ids: contain_exactly(group, parent_group), project_ids: project.id)
end
end
context 'when include_ancestors is false' do
let(:include_ancestors) { false }
it 'returns project id' do
expect(subject).to eq(project_ids: project.id)
end
end
end
context 'when parent is a group' do
subject { described_class.params_for_parent(group, include_ancestors: include_ancestors) }
context 'when include_ancestors is true' do
let(:include_ancestors) { true }
it 'returns group and ancestor ids' do
expect(subject).to match(group_ids: contain_exactly(group, parent_group))
end
end
context 'when include_ancestors is false' do
let(:include_ancestors) { false }
it 'returns group id' do
expect(subject).to eq(group_ids: group.id)
end
end
end
context 'when parent is invalid' do
subject { described_class.params_for_parent(double(User)) }
it 'raises an ArgumentError' do
expect { subject }.to raise_error(ArgumentError, 'Invalid parent class. Only Project and Group are supported.')
end
expect(finder.find_by(iid: upcoming_group_iteration.iid)).to eq(upcoming_group_iteration)
end
end
end
......
......@@ -13,7 +13,7 @@ RSpec.describe Resolvers::IterationsResolver do
id: nil,
iid: nil,
iteration_cadence_ids: nil,
group_ids: nil,
parent: nil,
state: nil,
start_date: nil,
end_date: nil,
......@@ -43,7 +43,7 @@ RSpec.describe Resolvers::IterationsResolver do
context 'without parameters' do
it 'calls IterationsFinder to retrieve all iterations' do
params = params_list.merge(group_ids: Group.where(id: group.id).select(:id), state: 'all')
params = params_list.merge(parent: group, include_ancestors: true, state: 'all')
expect(IterationsFinder).to receive(:new).with(current_user, params).and_call_original
......@@ -60,7 +60,7 @@ RSpec.describe Resolvers::IterationsResolver do
iid = 2
iteration_cadence_ids = ['5']
params = params_list.merge(id: id, iid: iid, iteration_cadence_ids: iteration_cadence_ids, group_ids: group.id, state: 'closed', start_date: start_date, end_date: end_date, search_title: search)
params = params_list.merge(id: id, iid: iid, iteration_cadence_ids: iteration_cadence_ids, parent: group, include_ancestors: nil, state: 'closed', start_date: start_date, end_date: end_date, search_title: search)
expect(IterationsFinder).to receive(:new).with(current_user, params).and_call_original
......@@ -75,7 +75,7 @@ RSpec.describe Resolvers::IterationsResolver do
iid = 2
iteration_cadence_ids = ['5']
params = params_list.merge(id: id, iid: iid, iteration_cadence_ids: iteration_cadence_ids, group_ids: group.id, state: 'closed', start_date: start_date, end_date: end_date, search_title: search)
params = params_list.merge(id: id, iid: iid, iteration_cadence_ids: iteration_cadence_ids, parent: group, include_ancestors: nil, state: 'closed', start_date: start_date, end_date: end_date, search_title: search)
expect(IterationsFinder).to receive(:new).with(current_user, params).and_call_original
......@@ -85,7 +85,7 @@ RSpec.describe Resolvers::IterationsResolver do
it 'accepts a raw model id for backward compatibility' do
id = 1
iid = 2
params = params_list.merge(id: id, iid: iid, group_ids: group.id, state: 'all')
params = params_list.merge(id: id, iid: iid, parent: group, include_ancestors: nil, state: 'all')
expect(IterationsFinder).to receive(:new).with(current_user, params).and_call_original
......@@ -97,7 +97,7 @@ RSpec.describe Resolvers::IterationsResolver do
let_it_be(:subgroup) { create(:group, :private, parent: group) }
it 'defaults to include_ancestors' do
params = params_list.merge(group_ids: subgroup.self_and_ancestors.select(:id), state: 'all')
params = params_list.merge(parent: subgroup, include_ancestors: true, state: 'all')
expect(IterationsFinder).to receive(:new).with(current_user, params).and_call_original
......@@ -105,7 +105,7 @@ RSpec.describe Resolvers::IterationsResolver do
end
it 'does not default to include_ancestors if IID is supplied' do
params = params_list.merge(iid: 1, group_ids: subgroup.id, state: 'all')
params = params_list.merge(iid: 1, parent: subgroup, include_ancestors: false, state: 'all')
expect(IterationsFinder).to receive(:new).with(current_user, params).and_call_original
......@@ -113,7 +113,7 @@ RSpec.describe Resolvers::IterationsResolver do
end
it 'accepts include_ancestors false' do
params = params_list.merge(group_ids: subgroup.id, state: 'all')
params = params_list.merge(parent: subgroup, include_ancestors: false, state: 'all')
expect(IterationsFinder).to receive(:new).with(current_user, params).and_call_original
......
......@@ -107,16 +107,6 @@ RSpec.describe 'Querying an Iteration' do
let(:expected_web_url) { /#{expected_web_path}$/ }
end
end
describe 'project-owned iteration' do
it_behaves_like 'scoped path' do
let(:queried_iteration) { project_iteration }
let(:expected_scope_path) { project_iteration_path(project, project_iteration.id) }
let(:expected_scope_url) { /#{expected_scope_path}$/ }
let(:expected_web_path) { project_iteration_path(project, project_iteration.id) }
let(:expected_web_url) { /#{expected_web_path}$/ }
end
end
end
context 'inside a group context' do
......
......@@ -78,11 +78,11 @@ RSpec.describe API::Iterations do
it_behaves_like 'iterations list'
it 'excludes ancestor iterations when include_ancestors is set to false' do
it 'return direct parent group iterations when include_ancestors is set to false' do
get api(api_path, user), params: { include_ancestors: false }
expect(response).to have_gitlab_http_status(:ok)
expect(json_response.size).to eq(0)
expect(json_response.map { |i| i['id'] }).to contain_exactly(iteration.id, closed_iteration.id)
end
end
end
......@@ -34,12 +34,24 @@ RSpec.describe Boards::CreateService, services: true do
end
end
context 'when setting a timebox' do
let(:user) { create(:user) }
before do
parent.add_reporter(user)
end
it_behaves_like 'setting a milestone scope' do
subject { described_class.new(parent, double, milestone_id: milestone.id).execute.payload }
before do
parent.add_reporter(user)
end
subject { described_class.new(parent, user, milestone_id: milestone.id).execute.payload }
end
it_behaves_like 'setting an iteration scope' do
subject { described_class.new(parent, nil, iteration_id: iteration.id).execute.payload }
subject { described_class.new(parent, user, iteration_id: iteration.id).execute.payload }
end
end
end
end
......@@ -34,6 +34,10 @@ RSpec.describe Boards::UpdateService, services: true do
hide_backlog_list: true, hide_closed_list: true }
end
before do
project.add_reporter(user)
end
context 'with group board' do
let!(:board) { create(:board, group: group, name: 'Backend') }
......@@ -46,11 +50,18 @@ RSpec.describe Boards::UpdateService, services: true do
it_behaves_like 'board update service'
end
context 'when setting a timebox' do
let(:user) { create(:user) }
before do
parent.add_reporter(user)
end
it_behaves_like 'setting a milestone scope' do
subject { board.reload }
before do
described_class.new(parent, double, milestone_id: milestone.id).execute(board)
described_class.new(parent, user, milestone_id: milestone.id).execute(board)
end
end
......@@ -58,7 +69,8 @@ RSpec.describe Boards::UpdateService, services: true do
subject { board.reload }
before do
described_class.new(parent, nil, iteration_id: iteration.id).execute(board)
described_class.new(parent, user, iteration_id: iteration.id).execute(board)
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