From 82ec8eafab2aa66eaf6fe7c9bc6a25bfbb291596 Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Thu, 1 Mar 2018 17:06:42 +0100 Subject: Revert Project.public_or_visible_to_user changes These changes were introduced in MR https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/17088. In https://gitlab.com/gitlab-com/infrastructure/issues/3772 we discovered these changes lead to a pretty drastic increase in SQL response timings. We'll revert these changes so we can work on a better solution in the mean time without GitLab.com (or other installations) experiecing reduced performance in the mean time. --- app/finders/snippets_finder.rb | 5 +-- app/models/project.rb | 46 ++++++---------------- .../revert-project-visibility-changes.yml | 5 +++ 3 files changed, 18 insertions(+), 38 deletions(-) create mode 100644 changelogs/unreleased/revert-project-visibility-changes.yml diff --git a/app/finders/snippets_finder.rb b/app/finders/snippets_finder.rb index a73c573736e..75e827b7f6e 100644 --- a/app/finders/snippets_finder.rb +++ b/app/finders/snippets_finder.rb @@ -62,9 +62,8 @@ class SnippetsFinder < UnionFinder # Don't return any project related snippets if the user cannot read cross project return table[:id].eq(nil) unless Ability.allowed?(current_user, :read_cross_project) - projects = Project.public_or_visible_to_user(current_user, use_where_in: false) do |part| - part.with_feature_available_for_user(:snippets, current_user) - end.select(:id) + projects = Project.public_or_visible_to_user(current_user) + .with_feature_available_for_user(:snippets, current_user).select(:id) arel_query = Arel::Nodes::SqlLiteral.new(projects.to_sql) table[:project_id].in(arel_query) diff --git a/app/models/project.rb b/app/models/project.rb index ba278a49688..ad4315e1601 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -317,42 +317,18 @@ class Project < ActiveRecord::Base # Returns a collection of projects that is either public or visible to the # logged in user. - # - # A caller may pass in a block to modify individual parts of - # the query, e.g. to apply .with_feature_available_for_user on top of it. - # This is useful for performance as we can stick those additional filters - # at the bottom of e.g. the UNION. - # - # Optionally, turning `use_where_in` off leads to returning a - # relation using #from instead of #where. This can perform much better - # but leads to trouble when used in conjunction with AR's #merge method. - def self.public_or_visible_to_user(user = nil, use_where_in: true, &block) - # If we don't get a block passed, use identity to avoid if/else repetitions - block = ->(part) { part } unless block_given? - - return block.call(public_to_user) unless user - - # If the user is allowed to see all projects, - # we can shortcut and just return. - return block.call(all) if user.full_private_access? - - authorized = user - .project_authorizations - .select(1) - .where('project_authorizations.project_id = projects.id') - authorized_projects = block.call(where('EXISTS (?)', authorized)) - - levels = Gitlab::VisibilityLevel.levels_for_user(user) - visible_projects = block.call(where(visibility_level: levels)) - - # We use a UNION here instead of OR clauses since this results in better - # performance. - union = Gitlab::SQL::Union.new([authorized_projects.select('projects.id'), visible_projects.select('projects.id')]) - - if use_where_in - where("projects.id IN (#{union.to_sql})") # rubocop:disable GitlabSecurity/SqlInjection + def self.public_or_visible_to_user(user = nil) + if user + authorized = user + .project_authorizations + .select(1) + .where('project_authorizations.project_id = projects.id') + + levels = Gitlab::VisibilityLevel.levels_for_user(user) + + where('EXISTS (?) OR projects.visibility_level IN (?)', authorized, levels) else - from("(#{union.to_sql}) AS #{table_name}") + public_to_user end end diff --git a/changelogs/unreleased/revert-project-visibility-changes.yml b/changelogs/unreleased/revert-project-visibility-changes.yml new file mode 100644 index 00000000000..86bab00b9b1 --- /dev/null +++ b/changelogs/unreleased/revert-project-visibility-changes.yml @@ -0,0 +1,5 @@ +--- +title: Revert Project.public_or_visible_to_user changes +merge_request: +author: +type: fixed -- cgit v1.2.1 From 82a32e2763db3196421412a537e9fd344b12c831 Mon Sep 17 00:00:00 2001 From: Andreas Brandl Date: Fri, 2 Mar 2018 09:29:12 +0100 Subject: Apply query changes to snippets only. This applies the changes introduced in `Project.public_or_visible_to_user` to the snippets finder only. We know that for `SnippetsFinder`, this change improves SQL timing from 5/23/25s to 0.7/2/4s (mean/p95/p99). At the same time, the scope was too broad, (negatively) affecting SQL timings in various other places: https://gitlab.com/gitlab-com/infrastructure/issues/3772 With this commit, the snippets dashboard stays usuable as we generally don't run into statement timeouts. In contrast to the earlier change in !17088, the scope of the change is limited to `SnippetsFinder` only, thus not affecting other places. --- app/finders/snippets_finder.rb | 35 ++++++++++++++++++++-- .../revert-project-visibility-changes.yml | 2 +- 2 files changed, 34 insertions(+), 3 deletions(-) diff --git a/app/finders/snippets_finder.rb b/app/finders/snippets_finder.rb index 75e827b7f6e..fc257933e57 100644 --- a/app/finders/snippets_finder.rb +++ b/app/finders/snippets_finder.rb @@ -58,12 +58,43 @@ class SnippetsFinder < UnionFinder .public_or_visible_to_user(current_user) end + # Returns a collection of projects that is either public or visible to the + # logged in user. + # + # A caller may pass in a block to modify individual parts of + # the query, e.g. to apply .with_feature_available_for_user on top of it. + # This is useful for performance as we can stick those additional filters + # at the bottom of e.g. the UNION. + def projects_for_user(&block) + return block.call(Project.public_to_user) unless current_user + + # If the current_user is allowed to see all projects, + # we can shortcut and just return. + return block.call(Project.all) if current_user.full_private_access? + + authorized = current_user + .project_authorizations + .select(1) + .where('project_authorizations.project_id = projects.id') + authorized_projects = block.call(Project.where('EXISTS (?)', authorized)) + + levels = Gitlab::VisibilityLevel.levels_for_user(current_user) + visible_projects = block.call(Project.where(visibility_level: levels)) + + # We use a UNION here instead of OR clauses since this results in better + # performance. + union = Gitlab::SQL::Union.new([authorized_projects.select('projects.id'), visible_projects.select('projects.id')]) + + Project.from("(#{union.to_sql}) AS #{Project.table_name}") + end + def feature_available_projects # Don't return any project related snippets if the user cannot read cross project return table[:id].eq(nil) unless Ability.allowed?(current_user, :read_cross_project) - projects = Project.public_or_visible_to_user(current_user) - .with_feature_available_for_user(:snippets, current_user).select(:id) + projects = projects_for_user do |part| + part.with_feature_available_for_user(:snippets, current_user) + end.select(:id) arel_query = Arel::Nodes::SqlLiteral.new(projects.to_sql) table[:project_id].in(arel_query) diff --git a/changelogs/unreleased/revert-project-visibility-changes.yml b/changelogs/unreleased/revert-project-visibility-changes.yml index 86bab00b9b1..df44fdb79b1 100644 --- a/changelogs/unreleased/revert-project-visibility-changes.yml +++ b/changelogs/unreleased/revert-project-visibility-changes.yml @@ -1,5 +1,5 @@ --- -title: Revert Project.public_or_visible_to_user changes +title: Revert Project.public_or_visible_to_user changes and only apply to snippets merge_request: author: type: fixed -- cgit v1.2.1 From 0a3fc7e3c283c10ef8415a719e83e80f0122af7d Mon Sep 17 00:00:00 2001 From: Andreas Brandl Date: Fri, 2 Mar 2018 10:15:52 +0100 Subject: Use yield instead of block.call. --- app/finders/snippets_finder.rb | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/app/finders/snippets_finder.rb b/app/finders/snippets_finder.rb index fc257933e57..565955ffc05 100644 --- a/app/finders/snippets_finder.rb +++ b/app/finders/snippets_finder.rb @@ -61,25 +61,25 @@ class SnippetsFinder < UnionFinder # Returns a collection of projects that is either public or visible to the # logged in user. # - # A caller may pass in a block to modify individual parts of + # A caller must pass in a block to modify individual parts of # the query, e.g. to apply .with_feature_available_for_user on top of it. # This is useful for performance as we can stick those additional filters # at the bottom of e.g. the UNION. - def projects_for_user(&block) - return block.call(Project.public_to_user) unless current_user + def projects_for_user + return yield(Project.public_to_user) unless current_user # If the current_user is allowed to see all projects, # we can shortcut and just return. - return block.call(Project.all) if current_user.full_private_access? + return yield(Project.all) if current_user.full_private_access? authorized = current_user .project_authorizations .select(1) .where('project_authorizations.project_id = projects.id') - authorized_projects = block.call(Project.where('EXISTS (?)', authorized)) + authorized_projects = yield(Project.where('EXISTS (?)', authorized)) levels = Gitlab::VisibilityLevel.levels_for_user(current_user) - visible_projects = block.call(Project.where(visibility_level: levels)) + visible_projects = yield(Project.where(visibility_level: levels)) # We use a UNION here instead of OR clauses since this results in better # performance. -- cgit v1.2.1 From 39011be53daee921dac1648044d1e68b9706197c Mon Sep 17 00:00:00 2001 From: Andreas Brandl Date: Fri, 2 Mar 2018 16:01:06 +0100 Subject: Extract method User#authorizations_for_projects. --- app/finders/snippets_finder.rb | 6 +----- app/models/project.rb | 16 ++++------------ app/models/user.rb | 9 +++++++++ spec/models/user_spec.rb | 26 ++++++++++++++++++++++++++ 4 files changed, 40 insertions(+), 17 deletions(-) diff --git a/app/finders/snippets_finder.rb b/app/finders/snippets_finder.rb index 565955ffc05..d498a2d6d11 100644 --- a/app/finders/snippets_finder.rb +++ b/app/finders/snippets_finder.rb @@ -72,11 +72,7 @@ class SnippetsFinder < UnionFinder # we can shortcut and just return. return yield(Project.all) if current_user.full_private_access? - authorized = current_user - .project_authorizations - .select(1) - .where('project_authorizations.project_id = projects.id') - authorized_projects = yield(Project.where('EXISTS (?)', authorized)) + authorized_projects = yield(Project.where('EXISTS (?)', current_user.authorizations_for_projects)) levels = Gitlab::VisibilityLevel.levels_for_user(current_user) visible_projects = yield(Project.where(visibility_level: levels)) diff --git a/app/models/project.rb b/app/models/project.rb index ad4315e1601..5b1f8b2658b 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -319,14 +319,9 @@ class Project < ActiveRecord::Base # logged in user. def self.public_or_visible_to_user(user = nil) if user - authorized = user - .project_authorizations - .select(1) - .where('project_authorizations.project_id = projects.id') - - levels = Gitlab::VisibilityLevel.levels_for_user(user) - - where('EXISTS (?) OR projects.visibility_level IN (?)', authorized, levels) + where('EXISTS (?) OR projects.visibility_level IN (?)', + user.authorizations_for_projects, + Gitlab::VisibilityLevel.levels_for_user(user)) else public_to_user end @@ -347,14 +342,11 @@ class Project < ActiveRecord::Base elsif user column = ProjectFeature.quoted_access_level_column(feature) - authorized = user.project_authorizations.select(1) - .where('project_authorizations.project_id = projects.id') - with_project_feature .where("#{column} IN (?) OR (#{column} = ? AND EXISTS (?))", visible, ProjectFeature::PRIVATE, - authorized) + user.authorizations_for_projects) else with_feature_access_level(feature, visible) end diff --git a/app/models/user.rb b/app/models/user.rb index 9547506d33d..9c60adf0c90 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -601,6 +601,15 @@ class User < ActiveRecord::Base authorized_projects(min_access_level).exists?({ id: project.id }) end + # Typically used in conjunction with projects table to get projects + # a user has been given access to. + # + # Example use: + # `Project.where('EXISTS(?)', user.authorizations_for_projects)` + def authorizations_for_projects + project_authorizations.select(1).where('project_authorizations.project_id = projects.id') + end + # Returns the projects this user has reporter (or greater) access to, limited # to at most the given projects. # diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 3531de244bd..00b5226d874 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -1635,6 +1635,32 @@ describe User do end end + describe '#authorizations_for_projects' do + let!(:user) { create(:user) } + subject { Project.where("EXISTS (?)", user.authorizations_for_projects) } + + it 'includes projects that belong to a user, but no other projects' do + owned = create(:project, :private, namespace: user.namespace) + member = create(:project, :private).tap { |p| p.add_master(user) } + other = create(:project) + + expect(subject).to include(owned) + expect(subject).to include(member) + expect(subject).not_to include(other) + end + + it 'includes projects a user has access to, but no other projects' do + other_user = create(:user) + accessible = create(:project, :private, namespace: other_user.namespace) do |project| + project.add_developer(user) + end + other = create(:project) + + expect(subject).to include(accessible) + expect(subject).not_to include(other) + end + end + describe '#authorized_projects', :delete do context 'with a minimum access level' do it 'includes projects for which the user is an owner' do -- cgit v1.2.1