diff options
author | Sean McGivern <sean@gitlab.com> | 2017-08-31 14:21:39 +0300 |
---|---|---|
committer | Sean McGivern <sean@gitlab.com> | 2017-08-31 14:21:39 +0300 |
commit | e7817fc1e0877efd59f0442934bbb0ad91fbb20a (patch) | |
tree | 6d0502e8a9c2af5a9d962deb40ad4b2216b84b69 /app | |
parent | f35d7d7f6ea04a38da822db902ad24108dfe94a2 (diff) |
Remove issuable finder count caching
We're going to cache the total open count separately, and then just perform
these counts on the list. We already do that to get the pagination information,
through Kaminari, and a future change will make Kaminari reuse the query results
from earlier in the request.
Diffstat (limited to 'app')
-rw-r--r-- | app/finders/issuable_finder.rb | 29 | ||||
-rw-r--r-- | app/finders/issues_finder.rb | 38 | ||||
-rw-r--r-- | app/helpers/issuables_helper.rb | 13 | ||||
-rw-r--r-- | app/services/issuable_base_service.rb | 15 |
4 files changed, 8 insertions, 87 deletions
diff --git a/app/finders/issuable_finder.rb b/app/finders/issuable_finder.rb index 7e0d3b5c979..c8dd2275730 100644 --- a/app/finders/issuable_finder.rb +++ b/app/finders/issuable_finder.rb @@ -24,7 +24,6 @@ class IssuableFinder include CreatedAtFilter NONE = '0'.freeze - IRRELEVANT_PARAMS_FOR_CACHE_KEY = %i[utf8 sort page state].freeze attr_accessor :current_user, :params @@ -68,7 +67,7 @@ class IssuableFinder # grouping and counting within that query. # def count_by_state - count_params = params.merge(state: nil, sort: nil, for_counting: true) + count_params = params.merge(state: nil, sort: nil) labels_count = label_names.any? ? label_names.count : 1 finder = self.class.new(current_user, count_params) counts = Hash.new(0) @@ -91,16 +90,6 @@ class IssuableFinder execute.find_by!(*params) end - def state_counter_cache_key - cache_key(state_counter_cache_key_components) - end - - def clear_caches! - state_counter_cache_key_components_permutations.each do |components| - Rails.cache.delete(cache_key(components)) - end - end - def group return @group if defined?(@group) @@ -432,20 +421,4 @@ class IssuableFinder def current_user_related? params[:scope] == 'created-by-me' || params[:scope] == 'authored' || params[:scope] == 'assigned-to-me' end - - def state_counter_cache_key_components - opts = params.with_indifferent_access - opts.except!(*IRRELEVANT_PARAMS_FOR_CACHE_KEY) - opts.delete_if { |_, value| value.blank? } - - ['issuables_count', klass.to_ability_name, opts.sort] - end - - def state_counter_cache_key_components_permutations - [state_counter_cache_key_components] - end - - def cache_key(components) - Digest::SHA1.hexdigest(components.flatten.join('-')) - end end diff --git a/app/finders/issues_finder.rb b/app/finders/issues_finder.rb index 0ec42a4e6eb..aa9cef6b08c 100644 --- a/app/finders/issues_finder.rb +++ b/app/finders/issues_finder.rb @@ -54,44 +54,10 @@ class IssuesFinder < IssuableFinder project.team.max_member_access(current_user.id) >= CONFIDENTIAL_ACCESS_LEVEL end - # Anonymous users can't see any confidential issues. - # - # Users without access to see _all_ confidential issues (as in - # `user_can_see_all_confidential_issues?`) are more complicated, because they - # can see confidential issues where: - # 1. They are an assignee. - # 2. They are an author. - # - # That's fine for most cases, but if we're just counting, we need to cache - # effectively. If we cached this accurately, we'd have a cache key for every - # authenticated user without sufficient access to the project. Instead, when - # we are counting, we treat them as if they can't see any confidential issues. - # - # This does mean the counts may be wrong for those users, but avoids an - # explosion in cache keys. - def user_cannot_see_confidential_issues?(for_counting: false) + def user_cannot_see_confidential_issues? return false if user_can_see_all_confidential_issues? - current_user.blank? || for_counting || params[:for_counting] - end - - def state_counter_cache_key_components - extra_components = [ - user_can_see_all_confidential_issues?, - user_cannot_see_confidential_issues?(for_counting: true) - ] - - super + extra_components - end - - def state_counter_cache_key_components_permutations - # Ignore the last two, as we'll provide both options for them. - components = super.first[0..-3] - - [ - components + [false, true], - components + [true, false] - ] + current_user.blank? end def by_assignee(items) diff --git a/app/helpers/issuables_helper.rb b/app/helpers/issuables_helper.rb index 2a748ce0a75..5d7d33da30a 100644 --- a/app/helpers/issuables_helper.rb +++ b/app/helpers/issuables_helper.rb @@ -240,16 +240,9 @@ module IssuablesHelper } end - def issuables_count_for_state(issuable_type, state, finder: nil) - finder ||= public_send("#{issuable_type}_finder") # rubocop:disable GitlabSecurity/PublicSend - cache_key = finder.state_counter_cache_key - - @counts ||= {} - @counts[cache_key] ||= Rails.cache.fetch(cache_key, expires_in: 2.minutes) do - finder.count_by_state - end - - @counts[cache_key][state] + def issuables_count_for_state(issuable_type, state) + finder = public_send("#{issuable_type}_finder") # rubocop:disable GitlabSecurity/PublicSend + finder.count_by_state end def close_issuable_url(issuable) diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb index 1486db046b5..063f7bc28d0 100644 --- a/app/services/issuable_base_service.rb +++ b/app/services/issuable_base_service.rb @@ -244,9 +244,7 @@ class IssuableBaseService < BaseService new_assignees = issuable.assignees.to_a affected_assignees = (old_assignees + new_assignees) - (old_assignees & new_assignees) - # Don't clear the project cache, because it will be handled by the - # appropriate service (close / reopen / merge / etc.). - invalidate_cache_counts(issuable, users: affected_assignees.compact, skip_project_cache: true) + invalidate_cache_counts(issuable, users: affected_assignees.compact) after_update(issuable) issuable.create_new_cross_references!(current_user) execute_hooks(issuable, 'update') @@ -340,18 +338,9 @@ class IssuableBaseService < BaseService create_labels_note(issuable, old_labels) if issuable.labels != old_labels end - def invalidate_cache_counts(issuable, users: [], skip_project_cache: false) + def invalidate_cache_counts(issuable, users: []) users.each do |user| user.public_send("invalidate_#{issuable.model_name.singular}_cache_counts") # rubocop:disable GitlabSecurity/PublicSend end - - unless skip_project_cache - case issuable - when Issue - IssuesFinder.new(nil, project_id: issuable.project_id).clear_caches! - when MergeRequest - MergeRequestsFinder.new(nil, project_id: issuable.target_project_id).clear_caches! - end - end end end |