Refactor Project.with_feature_available_for_user
This method used to use a UNION, which would lead to it performing the same query twice; producing less than ideal performance. Further, in certain cases ActiveRecord could get confused and mess up the variable bindings, though it's not clear how/why exactly this happens. Fortunately we can work around all of this by building some of the WHERE conditions manually, allowing us to use a simple OR statement to get all the data we want without any of the above problems.
This commit is contained in:
parent
d29347220c
commit
73bf9413b9
3 changed files with 38 additions and 7 deletions
|
@ -269,17 +269,29 @@ class Project < ActiveRecord::Base
|
||||||
# project features may be "disabled", "internal" or "enabled". If "internal",
|
# project features may be "disabled", "internal" or "enabled". If "internal",
|
||||||
# they are only available to team members. This scope returns projects where
|
# they are only available to team members. This scope returns projects where
|
||||||
# the feature is either enabled, or internal with permission for the user.
|
# the feature is either enabled, or internal with permission for the user.
|
||||||
|
#
|
||||||
|
# This method uses an optimised version of `with_feature_access_level` for
|
||||||
|
# logged in users to more efficiently get private projects with the given
|
||||||
|
# feature.
|
||||||
def self.with_feature_available_for_user(feature, user)
|
def self.with_feature_available_for_user(feature, user)
|
||||||
return with_feature_enabled(feature) if user.try(:admin?)
|
visible = [nil, ProjectFeature::ENABLED]
|
||||||
|
|
||||||
unconditional = with_feature_access_level(feature, [nil, ProjectFeature::ENABLED])
|
if user&.admin?
|
||||||
return unconditional if user.nil?
|
with_feature_enabled(feature)
|
||||||
|
elsif user
|
||||||
|
column = ProjectFeature.quoted_access_level_column(feature)
|
||||||
|
|
||||||
conditional = with_feature_access_level(feature, ProjectFeature::PRIVATE)
|
authorized = user.project_authorizations.select(1).
|
||||||
authorized = user.authorized_projects.merge(conditional.reorder(nil))
|
where('project_authorizations.project_id = projects.id')
|
||||||
|
|
||||||
union = Gitlab::SQL::Union.new([unconditional.select(:id), authorized.select(:id)])
|
with_project_feature.
|
||||||
where(arel_table[:id].in(Arel::Nodes::SqlLiteral.new(union.to_sql)))
|
where("#{column} IN (?) OR (#{column} = ? AND EXISTS (?))",
|
||||||
|
visible,
|
||||||
|
ProjectFeature::PRIVATE,
|
||||||
|
authorized)
|
||||||
|
else
|
||||||
|
with_feature_access_level(feature, visible)
|
||||||
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
scope :active, -> { joins(:issues, :notes, :merge_requests).order('issues.created_at, notes.created_at, merge_requests.created_at DESC') }
|
scope :active, -> { joins(:issues, :notes, :merge_requests).order('issues.created_at, notes.created_at, merge_requests.created_at DESC') }
|
||||||
|
|
|
@ -27,6 +27,13 @@ class ProjectFeature < ActiveRecord::Base
|
||||||
|
|
||||||
"#{feature}_access_level".to_sym
|
"#{feature}_access_level".to_sym
|
||||||
end
|
end
|
||||||
|
|
||||||
|
def quoted_access_level_column(feature)
|
||||||
|
attribute = connection.quote_column_name(access_level_attribute(feature))
|
||||||
|
table = connection.quote_table_name(table_name)
|
||||||
|
|
||||||
|
"#{table}.#{attribute}"
|
||||||
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
# Default scopes force us to unscope here since a service may need to check
|
# Default scopes force us to unscope here since a service may need to check
|
||||||
|
|
|
@ -4,6 +4,18 @@ describe ProjectFeature do
|
||||||
let(:project) { create(:empty_project) }
|
let(:project) { create(:empty_project) }
|
||||||
let(:user) { create(:user) }
|
let(:user) { create(:user) }
|
||||||
|
|
||||||
|
describe '.quoted_access_level_column' do
|
||||||
|
it 'returns the table name and quoted column name for a feature' do
|
||||||
|
expected = if Gitlab::Database.postgresql?
|
||||||
|
'"project_features"."issues_access_level"'
|
||||||
|
else
|
||||||
|
'`project_features`.`issues_access_level`'
|
||||||
|
end
|
||||||
|
|
||||||
|
expect(described_class.quoted_access_level_column(:issues)).to eq(expected)
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
describe '#feature_available?' do
|
describe '#feature_available?' do
|
||||||
let(:features) { %w(issues wiki builds merge_requests snippets repository) }
|
let(:features) { %w(issues wiki builds merge_requests snippets repository) }
|
||||||
|
|
||||||
|
|
Loading…
Reference in a new issue