Commit 1e4a0de1 authored by Heinrich Lee Yu's avatar Heinrich Lee Yu

Fix N+1 when validating groups

Removes the validates_associated call which calls `#valid?` on each
runner of the group which triggers an N+1 query

This improves the validation for RunnerNamespace so that we don't need
to fire multiple SQL queries
parent 4fe894dc
......@@ -4,10 +4,17 @@ module Ci
class RunnerNamespace < ApplicationRecord
extend Gitlab::Ci::Model
belongs_to :runner, inverse_of: :runner_namespaces, validate: true
belongs_to :runner, inverse_of: :runner_namespaces
belongs_to :namespace, inverse_of: :runner_namespaces, class_name: '::Namespace'
belongs_to :group, class_name: '::Group', foreign_key: :namespace_id
validates :runner_id, uniqueness: { scope: :namespace_id }
validate :group_runner_type
private
def group_runner_type
errors.add(:runner, 'is not a group runner') unless runner&.group_type?
end
end
end
......@@ -67,8 +67,6 @@ class Namespace < ApplicationRecord
validate :changing_shared_runners_enabled_is_allowed
validate :changing_allow_descendants_override_disabled_shared_runners_is_allowed
validates_associated :runners
delegate :name, to: :owner, allow_nil: true, prefix: true
delegate :avatar_url, to: :owner, allow_nil: true
......
---
title: Improve performance of validations when a group has a lot of runners
merge_request: 54774
author:
type: performance
......@@ -40,41 +40,39 @@ RSpec.describe Ci::Runner do
context 'runner_type validations' do
let_it_be(:group) { create(:group) }
let_it_be(:project) { create(:project) }
let(:group_runner) { create(:ci_runner, :group, groups: [group]) }
let(:project_runner) { create(:ci_runner, :project, projects: [project]) }
let(:instance_runner) { create(:ci_runner, :instance) }
it 'disallows assigning group to project_type runner' do
project_runner.groups << build(:group)
project_runner = build(:ci_runner, :project, groups: [group])
expect(project_runner).not_to be_valid
expect(project_runner.errors.full_messages).to include('Runner cannot have groups assigned')
end
it 'disallows assigning group to instance_type runner' do
instance_runner.groups << build(:group)
instance_runner = build(:ci_runner, :instance, groups: [group])
expect(instance_runner).not_to be_valid
expect(instance_runner.errors.full_messages).to include('Runner cannot have groups assigned')
end
it 'disallows assigning project to group_type runner' do
group_runner.projects << build(:project)
group_runner = build(:ci_runner, :instance, projects: [project])
expect(group_runner).not_to be_valid
expect(group_runner.errors.full_messages).to include('Runner cannot have projects assigned')
end
it 'disallows assigning project to instance_type runner' do
instance_runner.projects << build(:project)
instance_runner = build(:ci_runner, :instance, projects: [project])
expect(instance_runner).not_to be_valid
expect(instance_runner.errors.full_messages).to include('Runner cannot have projects assigned')
end
it 'fails to save a group assigned to a project runner even if the runner is already saved' do
group.runners << project_runner
expect { group.save! }
project_runner = create(:ci_runner, :project, projects: [project])
expect { create(:group, runners: [project_runner]) }
.to raise_error(ActiveRecord::RecordInvalid)
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