summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorAndreas Brandl <abrandl@gitlab.com>2018-03-02 09:29:12 +0100
committerAndreas Brandl <abrandl@gitlab.com>2018-03-02 09:35:21 +0100
commit82a32e2763db3196421412a537e9fd344b12c831 (patch)
treeb041575fb982deb3870cbfa4c440039191f224e7
parent82ec8eafab2aa66eaf6fe7c9bc6a25bfbb291596 (diff)
downloadgitlab-ce-82a32e2763db3196421412a537e9fd344b12c831.tar.gz
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.
-rw-r--r--app/finders/snippets_finder.rb35
-rw-r--r--changelogs/unreleased/revert-project-visibility-changes.yml2
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