diff options
author | Douwe Maan <douwe@gitlab.com> | 2018-11-07 16:20:59 +0300 |
---|---|---|
committer | Douwe Maan <douwe@gitlab.com> | 2018-11-07 16:20:59 +0300 |
commit | 0cd22a382ef390cda6dab001ab5e4738dd25b3a5 (patch) | |
tree | 625d02d37686529cbd19cb90bc003550cae5d70e /app | |
parent | 681d927f18f81755d88c56c0b6e267c9866c7bb4 (diff) | |
parent | 1208d55206128266690f46f0165df0fc10c24941 (diff) |
Merge branch 'refactor-snippets-finder' into 'master'
Rewrite SnippetsFinder to improve performance
Closes #52639
See merge request gitlab-org/gitlab-ce!22606
Diffstat (limited to 'app')
-rw-r--r-- | app/finders/snippets_finder.rb | 209 | ||||
-rw-r--r-- | app/models/project.rb | 4 | ||||
-rw-r--r-- | app/models/snippet.rb | 77 |
3 files changed, 170 insertions, 120 deletions
diff --git a/app/finders/snippets_finder.rb b/app/finders/snippets_finder.rb index f90971bb9f6..d3774746cb8 100644 --- a/app/finders/snippets_finder.rb +++ b/app/finders/snippets_finder.rb @@ -1,138 +1,149 @@ # frozen_string_literal: true -# Snippets Finder +# Finder for retrieving snippets that a user can see, optionally scoped to a +# project or snippets author. # -# Used to filter Snippets collections by a set of params +# Basic usage: # -# Arguments. +# user = User.find(1) # -# current_user - The current user, nil also can be used. -# params: -# visibility (integer) - Individual snippet visibility: Public(20), internal(10) or private(0). -# project (Project) - Project related. -# author (User) - Author related. +# SnippetsFinder.new(user).execute # -# params are optional +# To limit the snippets to a specific project, supply the `project:` option: +# +# user = User.find(1) +# project = Project.find(1) +# +# SnippetsFinder.new(user, project: project).execute +# +# Limiting snippets to an author can be done by supplying the `author:` option: +# +# user = User.find(1) +# project = Project.find(1) +# +# SnippetsFinder.new(user, author: user).execute +# +# To filter snippets using a specific visibility level, you can provide the +# `scope:` option: +# +# user = User.find(1) +# project = Project.find(1) +# +# SnippetsFinder.new(user, author: user, scope: :are_public).execute +# +# Valid `scope:` values are: +# +# * `:are_private` +# * `:are_internal` +# * `:are_public` +# +# Any other value will be ignored. class SnippetsFinder < UnionFinder - include Gitlab::Allowable include FinderMethods - attr_accessor :current_user, :project, :params + attr_accessor :current_user, :project, :author, :scope - def initialize(current_user, params = {}) + def initialize(current_user = nil, params = {}) @current_user = current_user - @params = params @project = params[:project] - end - - def execute - items = init_collection - items = by_author(items) - items = by_visibility(items) - - items.fresh - end - - private - - def init_collection - if project.present? - authorized_snippets_from_project - else - authorized_snippets + @author = params[:author] + @scope = params[:scope].to_s + + if project && author + raise( + ArgumentError, + 'Filtering by both an author and a project is not supported, ' \ + 'as this finder is not optimised for this use case' + ) end end - def authorized_snippets_from_project - if can?(current_user, :read_project_snippet, project) - if project.team.member?(current_user) - project.snippets + def execute + base = + if project + snippets_for_a_single_project else - project.snippets.public_to_user(current_user) + snippets_for_multiple_projects end - else - Snippet.none - end - end - # rubocop: disable CodeReuse/ActiveRecord - def authorized_snippets - # This query was intentionally converted to a raw one to get it work in Rails 5.0. - # In Rails 5.0 and 5.1 there's a bug: https://github.com/rails/arel/issues/531 - # Please convert it back when on rails 5.2 as it works again as expected since 5.2. - Snippet.where("#{feature_available_projects} OR #{not_project_related}") - .public_or_visible_to_user(current_user) + base.with_optional_visibility(visibility_from_scope).fresh end - # rubocop: enable CodeReuse/ActiveRecord - # Returns a collection of projects that is either public or visible to the - # logged in user. + # Produces a query that retrieves snippets from multiple projects. # - # 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. - # rubocop: disable CodeReuse/ActiveRecord - 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 yield(Project.all) if current_user.full_private_access? - - 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)) - - # We use a UNION here instead of OR clauses since this results in better - # performance. - Project.from_union([authorized_projects, visible_projects]) - end - # rubocop: enable CodeReuse/ActiveRecord - - def feature_available_projects - # Don't return any project related snippets if the user cannot read cross project - return table[:id].eq(nil).to_sql unless Ability.allowed?(current_user, :read_cross_project) - - projects = projects_for_user do |part| - part.with_feature_available_for_user(:snippets, current_user) - end.select(:id) + # The resulting query will, depending on the user's permissions, include the + # following collections of snippets: + # + # 1. Snippets that don't belong to any project. + # 2. Snippets of projects that are visible to the current user (e.g. snippets + # in public projects). + # 3. Snippets of projects that the current user is a member of. + # + # Each collection is constructed in isolation, allowing for greater control + # over the resulting SQL query. + def snippets_for_multiple_projects + queries = [global_snippets] + + if Ability.allowed?(current_user, :read_cross_project) + queries << snippets_of_visible_projects + queries << snippets_of_authorized_projects if current_user + end - # This query was intentionally converted to a raw one to get it work in Rails 5.0. - # In Rails 5.0 and 5.1 there's a bug: https://github.com/rails/arel/issues/531 - # Please convert it back when on rails 5.2 as it works again as expected since 5.2. - "snippets.project_id IN (#{projects.to_sql})" + find_union(queries, Snippet) end - def not_project_related - table[:project_id].eq(nil).to_sql + def snippets_for_a_single_project + Snippet.for_project_with_user(project, current_user) end - def table - Snippet.arel_table + def global_snippets + snippets_for_author_or_visible_to_user.only_global_snippets end - # rubocop: disable CodeReuse/ActiveRecord - def by_visibility(items) - visibility = params[:visibility] || visibility_from_scope + # Returns the snippets that the current user (logged in or not) can view. + def snippets_of_visible_projects + snippets_for_author_or_visible_to_user + .only_include_projects_visible_to(current_user) + .only_include_projects_with_snippets_enabled + end - return items unless visibility + # Returns the snippets that the currently logged in user has access to by + # being a member of the project the snippets belong to. + # + # This method requires that `current_user` returns a `User` instead of `nil`, + # and is optimised for this specific scenario. + def snippets_of_authorized_projects + base = author ? snippets_for_author : Snippet.all + + base + .only_include_projects_with_snippets_enabled(include_private: true) + .only_include_authorized_projects(current_user) + end - items.where(visibility_level: visibility) + def snippets_for_author_or_visible_to_user + if author + snippets_for_author + elsif current_user + Snippet.visible_to_or_authored_by(current_user) + else + Snippet.public_to_user + end end - # rubocop: enable CodeReuse/ActiveRecord - # rubocop: disable CodeReuse/ActiveRecord - def by_author(items) - return items unless params[:author] + def snippets_for_author + base = author.snippets - items.where(author_id: params[:author].id) + if author == current_user + # If the current user is also the author of all snippets, then we can + # include private snippets. + base + else + base.public_to_user(current_user) + end end - # rubocop: enable CodeReuse/ActiveRecord def visibility_from_scope - case params[:scope].to_s + case scope when 'are_private' Snippet::PRIVATE when 'are_internal' diff --git a/app/models/project.rb b/app/models/project.rb index d5a4ae79c47..48905547ab4 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -2073,6 +2073,10 @@ class Project < ActiveRecord::Base storage_version != LATEST_STORAGE_VERSION end + def snippets_visible?(user = nil) + Ability.allowed?(user, :read_project_snippet, self) + end + private def use_hashed_storage diff --git a/app/models/snippet.rb b/app/models/snippet.rb index 1c5846b4023..11856b55902 100644 --- a/app/models/snippet.rb +++ b/app/models/snippet.rb @@ -63,6 +63,62 @@ class Snippet < ActiveRecord::Base attr_spammable :title, spam_title: true attr_spammable :content, spam_description: true + def self.with_optional_visibility(value = nil) + if value + where(visibility_level: value) + else + all + end + end + + def self.only_global_snippets + where(project_id: nil) + end + + def self.only_include_projects_visible_to(current_user = nil) + levels = Gitlab::VisibilityLevel.levels_for_user(current_user) + + joins(:project).where('projects.visibility_level IN (?)', levels) + end + + def self.only_include_projects_with_snippets_enabled(include_private: false) + column = ProjectFeature.access_level_attribute(:snippets) + levels = [ProjectFeature::ENABLED, ProjectFeature::PUBLIC] + + levels << ProjectFeature::PRIVATE if include_private + + joins(project: :project_feature) + .where(project_features: { column => levels }) + end + + def self.only_include_authorized_projects(current_user) + where( + 'EXISTS (?)', + ProjectAuthorization + .select(1) + .where('project_id = snippets.project_id') + .where(user_id: current_user.id) + ) + end + + def self.for_project_with_user(project, user = nil) + return none unless project.snippets_visible?(user) + + if user && project.team.member?(user) + project.snippets + else + project.snippets.public_to_user(user) + end + end + + def self.visible_to_or_authored_by(user) + where( + 'snippets.visibility_level IN (?) OR snippets.author_id = ?', + Gitlab::VisibilityLevel.levels_for_user(user), + user.id + ) + end + def self.reference_prefix '$' end @@ -81,27 +137,6 @@ class Snippet < ActiveRecord::Base @link_reference_pattern ||= super("snippets", /(?<snippet>\d+)/) end - # Returns a collection of snippets that are either public or visible to the - # logged in user. - # - # This method does not verify the user actually has the access to the project - # the snippet is in, so it should be only used on a relation that's already scoped - # for project access - def self.public_or_visible_to_user(user = nil) - if user - authorized = user - .project_authorizations - .select(1) - .where('project_authorizations.project_id = snippets.project_id') - - levels = Gitlab::VisibilityLevel.levels_for_user(user) - - where('EXISTS (?) OR snippets.visibility_level IN (?) or snippets.author_id = (?)', authorized, levels, user.id) - else - public_to_user - end - end - def to_reference(from = nil, full: false) reference = "#{self.class.reference_prefix}#{id}" |