Merge branch 'clear-issuable-count-cache-for-states' into 'master'

app/controllers/concerns/issuable_collections.rb

@@ -32,10 +32,10 @@ module IssuableCollections
 
   def filter_params
     set_sort_order_from_cookie
-    set_default_scope
     set_default_state
 
-    @filter_params = params.dup
+    # Skip irrelevant Rails routing params
+    @filter_params = params.dup.except(:controller, :action, :namespace_id)
     @filter_params[:sort] ||= default_sort_order
 
     @sort = @filter_params[:sort]
@@ -55,10 +55,6 @@ module IssuableCollections
     @filter_params
   end
 
-  def set_default_scope
-    params[:scope] = 'all' if params[:scope].blank?
-  end
-
   def set_default_state
     params[:state] = 'opened' if params[:state].blank?
   end

app/finders/issuable_finder.rb

@@ -22,7 +22,7 @@ class IssuableFinder
   include CreatedAtFilter
 
   NONE = '0'.freeze
-  IRRELEVANT_PARAMS_FOR_CACHE_KEY = %i[utf8 sort page].freeze
+  IRRELEVANT_PARAMS_FOR_CACHE_KEY = %i[utf8 sort page state].freeze
 
   attr_accessor :current_user, :params
 
@@ -89,8 +89,14 @@ class IssuableFinder
     execute.find_by!(*params)
   end
 
-  def state_counter_cache_key(state)
-    Digest::SHA1.hexdigest(state_counter_cache_key_components(state).flatten.join('-'))
+  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
@@ -417,12 +423,19 @@ class IssuableFinder
     params[:scope] == 'created-by-me' || params[:scope] == 'authored' || params[:scope] == 'assigned-to-me'
   end
 
-  def state_counter_cache_key_components(state)
+  def state_counter_cache_key_components
     opts = params.with_indifferent_access
-    opts[:state] = state
     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

app/finders/issues_finder.rb

@@ -75,7 +75,7 @@ class IssuesFinder < IssuableFinder
     current_user.blank? || for_counting || params[:for_counting]
   end
 
-  def state_counter_cache_key_components(state)
+  def state_counter_cache_key_components
     extra_components = [
       user_can_see_all_confidential_issues?,
       user_cannot_see_confidential_issues?(for_counting: true)
@@ -84,6 +84,16 @@ class IssuesFinder < IssuableFinder
     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]
+    ]
+  end
+
   def by_assignee(items)
     if assignee
       items.assigned_to(assignee)

app/helpers/issuables_helper.rb

@@ -235,7 +235,7 @@ module IssuablesHelper
 
   def issuables_count_for_state(issuable_type, state, finder: nil)
     finder ||= public_send("#{issuable_type}_finder")
-    cache_key = finder.state_counter_cache_key(state)
+    cache_key = finder.state_counter_cache_key
 
     @counts ||= {}
     @counts[cache_key] ||= Rails.cache.fetch(cache_key, expires_in: 2.minutes) do

app/services/boards/issues/list_service.rb

@@ -33,17 +33,12 @@ module Boards
       end
 
       def filter_params
-        set_default_scope
         set_project
         set_state
 
         params
       end
 
-      def set_default_scope
-        params[:scope] = 'all'
-      end
-
       def set_project
         params[:project_id] = project.id
       end

app/services/issuable_base_service.rb

@@ -183,7 +183,7 @@ class IssuableBaseService < BaseService
       after_create(issuable)
       issuable.create_cross_references!(current_user)
       execute_hooks(issuable)
-      invalidate_cache_counts(issuable.assignees, issuable)
+      invalidate_cache_counts(issuable, users: issuable.assignees)
     end
 
     issuable
@@ -240,12 +240,12 @@ class IssuableBaseService < BaseService
           old_assignees: old_assignees
         )
 
-        if old_assignees != issuable.assignees
         new_assignees = issuable.assignees.to_a
         affected_assignees = (old_assignees + new_assignees) - (old_assignees & new_assignees)
-          invalidate_cache_counts(affected_assignees.compact, issuable)
-        end
 
+        # 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)
         after_update(issuable)
         issuable.create_new_cross_references!(current_user)
         execute_hooks(issuable, 'update')
@@ -339,9 +339,18 @@ class IssuableBaseService < BaseService
     create_labels_note(issuable, old_labels) if issuable.labels != old_labels
   end
 
-  def invalidate_cache_counts(users, issuable)
+  def invalidate_cache_counts(issuable, users: [], skip_project_cache: false)
     users.each do |user|
       user.public_send("invalidate_#{issuable.model_name.singular}_cache_counts")
     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

app/services/issues/close_service.rb

@@ -28,7 +28,7 @@ module Issues
         notification_service.close_issue(issue, current_user) if notifications
         todo_service.close_issue(issue, current_user)
         execute_hooks(issue, 'close')
-        invalidate_cache_counts(issue.assignees, issue)
+        invalidate_cache_counts(issue, users: issue.assignees)
       end
 
       issue

app/services/issues/reopen_service.rb

@@ -8,7 +8,7 @@ module Issues
         create_note(issue)
         notification_service.reopen_issue(issue, current_user)
         execute_hooks(issue, 'reopen')
-        invalidate_cache_counts(issue.assignees, issue)
+        invalidate_cache_counts(issue, users: issue.assignees)
       end
 
       issue

app/services/merge_requests/close_service.rb

@@ -13,7 +13,7 @@ module MergeRequests
         notification_service.close_mr(merge_request, current_user)
         todo_service.close_merge_request(merge_request, current_user)
         execute_hooks(merge_request, 'close')
-        invalidate_cache_counts(merge_request.assignees, merge_request)
+        invalidate_cache_counts(merge_request, users: merge_request.assignees)
       end
 
       merge_request

app/services/merge_requests/post_merge_service.rb

@@ -13,7 +13,7 @@ module MergeRequests
       create_note(merge_request)
       notification_service.merge_mr(merge_request, current_user)
       execute_hooks(merge_request, 'merge')
-      invalidate_cache_counts(merge_request.assignees, merge_request)
+      invalidate_cache_counts(merge_request, users: merge_request.assignees)
     end
 
     private

app/services/merge_requests/reopen_service.rb

@@ -10,7 +10,7 @@ module MergeRequests
         execute_hooks(merge_request, 'reopen')
         merge_request.reload_diff(current_user)
         merge_request.mark_as_unchecked
-        invalidate_cache_counts(merge_request.assignees, merge_request)
+        invalidate_cache_counts(merge_request, users: merge_request.assignees)
       end
 
       merge_request

spec/features/dashboard/issues_spec.rb

@@ -62,7 +62,7 @@ RSpec.describe 'Dashboard Issues', feature: true do
 
     it 'state filter tabs work' do
       find('#state-closed').click
-      expect(page).to have_current_path(issues_dashboard_url(assignee_id: current_user.id, scope: 'all', state: 'closed'), url: true)
+      expect(page).to have_current_path(issues_dashboard_url(assignee_id: current_user.id, state: 'closed'), url: true)
     end
 
     it_behaves_like "it has an RSS button with current_user's RSS token"

spec/features/projects/issuable_counts_caching_spec.rb

+require 'spec_helper'
+
+describe 'Issuable counts caching', :use_clean_rails_memory_store_caching do
+  let!(:member) { create(:user) }
+  let!(:member_2) { create(:user) }
+  let!(:non_member) { create(:user) }
+  let!(:project) { create(:empty_project, :public) }
+  let!(:open_issue) { create(:issue, project: project) }
+  let!(:confidential_issue) { create(:issue, :confidential, project: project, author: non_member) }
+  let!(:closed_issue) { create(:issue, :closed, project: project) }
+
+  before do
+    project.add_developer(member)
+    project.add_developer(member_2)
+  end
+
+  it 'caches issuable counts correctly for non-members' do
+    # We can't use expect_any_instance_of because that uses a single instance.
+    counts = 0
+
+    allow_any_instance_of(IssuesFinder).to receive(:count_by_state).and_wrap_original do |m, *args|
+      counts += 1
+
+      m.call(*args)
+    end
+
+    aggregate_failures 'only counts once on first load with no params, and caches for later loads' do
+      expect { visit project_issues_path(project) }
+        .to change { counts }.by(1)
+
+      expect { visit project_issues_path(project) }
+        .not_to change { counts }
+    end
+
+    aggregate_failures 'uses counts from cache on load from non-member' do
+      sign_in(non_member)
+
+      expect { visit project_issues_path(project) }
+        .not_to change { counts }
+
+      sign_out(non_member)
+    end
+
+    aggregate_failures 'does not use the same cache for a member' do
+      sign_in(member)
+
+      expect { visit project_issues_path(project) }
+        .to change { counts }.by(1)
+
+      sign_out(member)
+    end
+
+    aggregate_failures 'uses the same cache for all members' do
+      sign_in(member_2)
+
+      expect { visit project_issues_path(project) }
+        .not_to change { counts }
+
+      sign_out(member_2)
+    end
+
+    aggregate_failures 'shares caches when params are passed' do
+      expect { visit project_issues_path(project, author_username: non_member.username) }
+        .to change { counts }.by(1)
+
+      sign_in(member)
+
+      expect { visit project_issues_path(project, author_username: non_member.username) }
+        .to change { counts }.by(1)
+
+      sign_in(non_member)
+
+      expect { visit project_issues_path(project, author_username: non_member.username) }
+        .not_to change { counts }
+
+      sign_in(member_2)
+
+      expect { visit project_issues_path(project, author_username: non_member.username) }
+        .not_to change { counts }
+
+      sign_out(member_2)
+    end
+
+    aggregate_failures 'resets caches on issue close' do
+      Issues::CloseService.new(project, member).execute(open_issue)
+
+      expect { visit project_issues_path(project) }
+        .to change { counts }.by(1)
+
+      sign_in(member)
+
+      expect { visit project_issues_path(project) }
+        .to change { counts }.by(1)
+
+      sign_in(non_member)
+
+      expect { visit project_issues_path(project) }
+        .not_to change { counts }
+
+      sign_in(member_2)
+
+      expect { visit project_issues_path(project) }
+        .not_to change { counts }
+
+      sign_out(member_2)
+    end
+
+    aggregate_failures 'does not reset caches on issue update' do
+      Issues::UpdateService.new(project, member, title: 'new title').execute(open_issue)
+
+      expect { visit project_issues_path(project) }
+        .not_to change { counts }
+
+      sign_in(member)
+
+      expect { visit project_issues_path(project) }
+        .not_to change { counts }
+
+      sign_in(non_member)
+
+      expect { visit project_issues_path(project) }
+        .not_to change { counts }
+
+      sign_in(member_2)
+
+      expect { visit project_issues_path(project) }
+        .not_to change { counts }
+
+      sign_out(member_2)
+    end
+  end
+end