Commit c259d6af authored by Allen Cook's avatar Allen Cook Committed by Shinya Maeda

Improve performance of group releases endpoints

Changelog: performance
parent 99e7779d
......@@ -15,11 +15,17 @@ module Groups
private
def releases
ReleasesFinder
.new(@group, current_user, { include_subgroups: true })
.execute(preload: false)
.page(params[:page])
.per(30)
if Feature.enabled?(:group_releases_finder_inoperator)
Releases::GroupReleasesFinder
.new(@group, current_user, { include_subgroups: true, page: params[:page], per: 30 })
.execute(preload: false)
else
ReleasesFinder
.new(@group, current_user, { include_subgroups: true })
.execute(preload: false)
.page(params[:page])
.per(30)
end
end
end
end
# frozen_string_literal: true
module Releases
##
# The GroupReleasesFinder does not support all the options of ReleasesFinder
# due to use of InOperatorOptimization for finding subprojects/subgroups
#
# order_by - only ordering by released_at is supported
# filter by tag - currently not supported
class GroupReleasesFinder
include Gitlab::Utils::StrongMemoize
attr_reader :parent, :current_user, :params
def initialize(parent, current_user = nil, params = {})
@parent = parent
@current_user = current_user
@params = params
params[:order_by] ||= 'released_at'
params[:sort] ||= 'desc'
params[:page] ||= 0
params[:per] ||= 30
end
def execute(preload: true)
return Release.none unless Ability.allowed?(current_user, :read_release, parent)
releases = get_releases(preload: preload)
paginate_releases(releases)
end
private
def include_subgroups?
params.fetch(:include_subgroups, false)
end
def accessible_projects_scope
if include_subgroups?
Project.for_group_and_its_subgroups(parent)
else
parent.projects
end
end
# rubocop: disable CodeReuse/ActiveRecord
def get_releases(preload: true)
Gitlab::Pagination::Keyset::InOperatorOptimization::QueryBuilder.new(
scope: releases_scope(preload: preload),
array_scope: accessible_projects_scope.select(:id),
array_mapping_scope: -> (project_id_expression) { Release.where(Release.arel_table[:project_id].eq(project_id_expression)) },
finder_query: -> (order_by, id_expression) { Release.where(Release.arel_table[:id].eq(id_expression)) }
)
.execute
end
def releases_scope(preload: true)
scope = Release.all
scope = order_releases(scope)
scope = scope.preloaded if preload
scope
end
def order_releases(scope)
scope.sort_by_attribute("released_at_#{params[:sort]}").order(id: params[:sort])
end
def paginate_releases(releases)
releases.page(params[:page].to_i).per(params[:per])
end
# rubocop: enable CodeReuse/ActiveRecord
end
end
......@@ -6,6 +6,7 @@ class Release < ApplicationRecord
include Importable
include Gitlab::Utils::StrongMemoize
include EachBatch
include FromUnion
cache_markdown_field :description
......
---
name: group_releases_finder_inoperator
introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/80093
rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/355463
milestone: '14.9'
type: development
group: group::release
default_enabled: false
......@@ -20,11 +20,11 @@ RSpec.describe Groups::ReleasesController do
context 'as json' do
let(:format) { :json }
subject { get :index, params: { group_id: group }, format: format }
subject(:index) { get :index, params: { group_id: group }, format: format }
context 'json_response' do
before do
subject
index
end
it 'returns an application/json content_type' do
......@@ -38,7 +38,7 @@ RSpec.describe Groups::ReleasesController do
context 'the user is not authorized' do
before do
subject
index
end
it 'does not return any releases' do
......@@ -54,12 +54,38 @@ RSpec.describe Groups::ReleasesController do
it "returns all group's public and private project's releases as JSON, ordered by released_at" do
sign_in(guest)
subject
index
expect(json_response.map {|r| r['tag'] } ).to match_array(%w(p2 p1 v2 v1))
end
end
context 'group_releases_finder_inoperator feature flag' do
before do
sign_in(guest)
end
it 'calls old code when disabled' do
stub_feature_flags(group_releases_finder_inoperator: false)
allow(ReleasesFinder).to receive(:new).and_call_original
index
expect(ReleasesFinder).to have_received(:new)
end
it 'calls new code when enabled' do
stub_feature_flags(group_releases_finder_inoperator: true)
allow(Releases::GroupReleasesFinder).to receive(:new).and_call_original
index
expect(Releases::GroupReleasesFinder).to have_received(:new)
end
end
context 'N+1 queries' do
it 'avoids N+1 database queries' do
control_count = ActiveRecord::QueryRecorder.new { subject }.count
......
# frozen_string_literal: true
require 'spec_helper'
RSpec.describe Releases::GroupReleasesFinder do
let(:user) { create(:user) }
let(:group) { create(:group) }
let(:project) { create(:project, :repository, group: group) }
let(:params) { {} }
let(:args) { {} }
let(:repository) { project.repository }
let(:v1_0_0) { create(:release, project: project, tag: 'v1.0.0') }
let(:v1_1_0) { create(:release, project: project, tag: 'v1.1.0') }
let(:v1_1_1) { create(:release, project: project, tag: 'v1.1.1') }
before do
v1_0_0.update_attribute(:released_at, 2.days.ago)
v1_1_0.update_attribute(:released_at, 1.day.ago)
v1_1_1.update_attribute(:released_at, 0.5.days.ago)
end
shared_examples_for 'when the user is not part of the project' do
it 'returns no releases' do
is_expected.to be_empty
end
end
shared_examples_for 'when the user is not part of the group' do
before do
allow(Ability).to receive(:allowed?).with(user, :read_release, group).and_return(false)
end
it 'returns no releases' do
is_expected.to be_empty
end
end
shared_examples_for 'preload' do
before do
allow(Ability).to receive(:allowed?).with(user, :read_release, group).and_return(true)
end
it 'preloads associations' do
expect(Release).to receive(:preloaded).once.and_call_original
releases
end
context 'when preload is false' do
let(:args) { { preload: false } }
it 'does not preload associations' do
expect(Release).not_to receive(:preloaded)
releases
end
end
end
describe 'when parent is a group' do
context 'without subgroups' do
let(:project2) { create(:project, :repository, namespace: group) }
let!(:v6) { create(:release, project: project2, tag: 'v6') }
subject(:releases) { described_class.new(group, user, params).execute(**args) }
it_behaves_like 'preload'
it_behaves_like 'when the user is not part of the group'
context 'when the user is a project guest on one sibling project' do
before do
project.add_guest(user)
end
it 'does not return any releases' do
expect(releases.size).to eq(0)
expect(releases).to eq([])
end
end
context 'when the user is a guest on the group' do
before do
group.add_guest(user)
v1_0_0.update_attribute(:released_at, 3.days.ago)
v6.update_attribute(:released_at, 2.days.ago)
v1_1_0.update_attribute(:released_at, 1.day.ago)
v1_1_1.update_attribute(:released_at, v1_1_0.released_at)
end
it 'sorts by release date and id' do
expect(releases.size).to eq(4)
expect(releases).to eq([v1_1_1, v1_1_0, v6, v1_0_0])
end
end
end
describe 'with subgroups' do
let(:params) { { include_subgroups: true } }
subject(:releases) { described_class.new(group, user, params).execute(**args) }
context 'with a single-level subgroup' do
let(:subgroup) { create(:group, parent: group) }
let(:project2) { create(:project, :repository, namespace: subgroup) }
let!(:v6) { create(:release, project: project2, tag: 'v6') }
it_behaves_like 'when the user is not part of the group'
context 'when the user a project guest in the subgroup project' do
before do
project2.add_guest(user)
end
it 'does not return any releases' do
expect(releases).to match_array([])
end
end
context 'when the user is a guest on the group' do
before do
group.add_guest(user)
v6.update_attribute(:released_at, 2.days.ago)
end
it 'returns all releases' do
expect(releases).to match_array([v1_1_1, v1_1_0, v1_0_0, v6])
end
end
end
context 'with a multi-level subgroup' do
let(:subgroup) { create(:group, parent: group) }
let(:subsubgroup) { create(:group, parent: subgroup) }
let(:project2) { create(:project, :repository, namespace: subgroup) }
let(:project3) { create(:project, :repository, namespace: subsubgroup) }
let!(:v6) { create(:release, project: project2, tag: 'v6') }
let!(:p3) { create(:release, project: project3, tag: 'p3') }
before do
v6.update_attribute(:released_at, 2.days.ago)
p3.update_attribute(:released_at, 3.days.ago)
end
it_behaves_like 'when the user is not part of the group'
context 'when the user a project guest in the subgroup and subsubgroup project' do
before do
project2.add_guest(user)
project3.add_guest(user)
end
it 'does not return any releases' do
expect(releases).to match_array([])
end
end
context 'when the user a project guest in the subsubgroup project' do
before do
project3.add_guest(user)
end
it 'does not return any releases' do
expect(releases).to match_array([])
end
end
context 'when the user a guest on the group' do
before do
group.add_guest(user)
end
it 'returns all releases' do
expect(releases).to match_array([v1_1_1, v1_1_0, v6, v1_0_0, p3])
end
end
context 'performance testing' do
shared_examples 'avoids N+1 queries' do |query_params = {}|
context 'with subgroups' do
let(:params) { query_params }
it 'include_subgroups avoids N+1 queries' do
control_count = ActiveRecord::QueryRecorder.new(skip_cached: false) do
releases
end.count
subgroups = create_list(:group, 10, parent: group)
projects = create_list(:project, 10, namespace: subgroups[0])
create_list(:release, 10, project: projects[0], author: user)
expect do
releases
end.not_to exceed_all_query_limit(control_count)
end
end
end
it_behaves_like 'avoids N+1 queries'
it_behaves_like 'avoids N+1 queries', { simple: true }
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