diff options
25 files changed, 301 insertions, 71 deletions
diff --git a/app/models/issue.rb b/app/models/issue.rb index 1c948c8957e..d89b1c96a36 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -53,7 +53,10 @@ class Issue < ActiveRecord::Base scope :preload_associations, -> { preload(:labels, project: :namespace) } + scope :public_only, -> { where(confidential: false) } + after_save :expire_etag_cache + after_commit :update_project_counter_caches, on: :destroy attr_spammable :title, spam_title: true attr_spammable :description, spam_description: true @@ -269,6 +272,10 @@ class Issue < ActiveRecord::Base end end + def update_project_counter_caches + Projects::OpenIssuesCountService.new(project).refresh_cache + end + private # Returns `true` if the given User can read the current Issue. diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index ac08dc0ee1f..7fe7df75944 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -32,6 +32,7 @@ class MergeRequest < ActiveRecord::Base after_create :ensure_merge_request_diff, unless: :importing? after_update :reload_diff_if_branch_changed + after_commit :update_project_counter_caches, on: :destroy # When this attribute is true some MR validation is ignored # It allows us to close or modify broken merge requests @@ -937,6 +938,10 @@ class MergeRequest < ActiveRecord::Base true end + def update_project_counter_caches + Projects::OpenMergeRequestsCountService.new(target_project).refresh_cache + end + private def write_ref diff --git a/app/models/project.rb b/app/models/project.rb index be248bc99e1..ddef8b82dee 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -1158,7 +1158,11 @@ class Project < ActiveRecord::Base end def open_issues_count - issues.opened.count + Projects::OpenIssuesCountService.new(self).count + end + + def open_merge_requests_count + Projects::OpenMergeRequestsCountService.new(self).count end def visibility_level_allowed_as_fork?(level = self.visibility_level) diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb index 4a4f2b91182..1486db046b5 100644 --- a/app/services/issuable_base_service.rb +++ b/app/services/issuable_base_service.rb @@ -192,6 +192,8 @@ class IssuableBaseService < BaseService def after_create(issuable) # To be overridden by subclasses + + issuable.update_project_counter_caches end def before_update(issuable) @@ -200,6 +202,8 @@ class IssuableBaseService < BaseService def after_update(issuable) # To be overridden by subclasses + + issuable.update_project_counter_caches end def update(issuable) diff --git a/app/services/issues/create_service.rb b/app/services/issues/create_service.rb index 234fcbede03..0307634c0b6 100644 --- a/app/services/issues/create_service.rb +++ b/app/services/issues/create_service.rb @@ -27,6 +27,8 @@ module Issues todo_service.new_issue(issuable, current_user) user_agent_detail_service.create resolve_discussions_with_issue(issuable) + + super end def resolve_discussions_with_issue(issue) diff --git a/app/services/merge_requests/create_service.rb b/app/services/merge_requests/create_service.rb index 194413bf321..3d53fe0646b 100644 --- a/app/services/merge_requests/create_service.rb +++ b/app/services/merge_requests/create_service.rb @@ -28,6 +28,8 @@ module MergeRequests todo_service.new_merge_request(issuable, current_user) issuable.cache_merge_request_closes_issues!(current_user) update_merge_requests_head_pipeline(issuable) + + super end private diff --git a/app/services/projects/count_service.rb b/app/services/projects/count_service.rb new file mode 100644 index 00000000000..5e633c37bf8 --- /dev/null +++ b/app/services/projects/count_service.rb @@ -0,0 +1,43 @@ +module Projects + # Base class for the various service classes that count project data (e.g. + # issues or forks). + class CountService + def initialize(project) + @project = project + end + + def relation_for_count + raise( + NotImplementedError, + '"relation_for_count" must be implemented and return an ActiveRecord::Relation' + ) + end + + def count + Rails.cache.fetch(cache_key) { uncached_count } + end + + def refresh_cache + Rails.cache.write(cache_key, uncached_count) + end + + def uncached_count + relation_for_count.count + end + + def delete_cache + Rails.cache.delete(cache_key) + end + + def cache_key_name + raise( + NotImplementedError, + '"cache_key_name" must be implemented and return a String' + ) + end + + def cache_key + ['projects', @project.id, cache_key_name] + end + end +end diff --git a/app/services/projects/forks_count_service.rb b/app/services/projects/forks_count_service.rb index e2e2b1da91d..3a0fa84b868 100644 --- a/app/services/projects/forks_count_service.rb +++ b/app/services/projects/forks_count_service.rb @@ -1,30 +1,12 @@ module Projects # Service class for getting and caching the number of forks of a project. - class ForksCountService - def initialize(project) - @project = project + class ForksCountService < CountService + def relation_for_count + @project.forks end - def count - Rails.cache.fetch(cache_key) { uncached_count } - end - - def refresh_cache - Rails.cache.write(cache_key, uncached_count) - end - - def delete_cache - Rails.cache.delete(cache_key) - end - - private - - def uncached_count - @project.forks.count - end - - def cache_key - ['projects', @project.id, 'forks_count'] + def cache_key_name + 'forks_count' end end end diff --git a/app/services/projects/open_issues_count_service.rb b/app/services/projects/open_issues_count_service.rb new file mode 100644 index 00000000000..3c0d186a73c --- /dev/null +++ b/app/services/projects/open_issues_count_service.rb @@ -0,0 +1,15 @@ +module Projects + # Service class for counting and caching the number of open issues of a + # project. + class OpenIssuesCountService < CountService + def relation_for_count + # We don't include confidential issues in this number since this would + # expose the number of confidential issues to non project members. + @project.issues.opened.public_only + end + + def cache_key_name + 'open_issues_count' + end + end +end diff --git a/app/services/projects/open_merge_requests_count_service.rb b/app/services/projects/open_merge_requests_count_service.rb new file mode 100644 index 00000000000..2a90f78b90d --- /dev/null +++ b/app/services/projects/open_merge_requests_count_service.rb @@ -0,0 +1,13 @@ +module Projects + # Service class for counting and caching the number of open merge requests of + # a project. + class OpenMergeRequestsCountService < CountService + def relation_for_count + @project.merge_requests.opened + end + + def cache_key_name + 'open_merge_requests_count' + end + end +end diff --git a/app/views/layouts/nav/_new_project_sidebar.html.haml b/app/views/layouts/nav/_new_project_sidebar.html.haml index 0ef81375c3a..81c84756ff8 100644 --- a/app/views/layouts/nav/_new_project_sidebar.html.haml +++ b/app/views/layouts/nav/_new_project_sidebar.html.haml @@ -86,7 +86,8 @@ %span.nav-item-name Issues - if @project.issues_enabled? - %span.badge.count.issue_counter= number_with_delimiter(IssuesFinder.new(current_user, project_id: @project.id).execute.opened.count) + %span.badge.count.issue_counter + = number_with_delimiter(@project.open_issues_count) %ul.sidebar-sub-level-items = nav_link(controller: :issues) do @@ -116,7 +117,8 @@ = custom_icon('mr_bold') %span.nav-item-name Merge Requests - %span.badge.count.merge_counter.js-merge-counter= number_with_delimiter(MergeRequestsFinder.new(current_user, project_id: @project.id).execute.opened.count) + %span.badge.count.merge_counter.js-merge-counter + = number_with_delimiter(@project.open_merge_requests_count) - if project_nav_tab? :pipelines = nav_link(controller: [:pipelines, :builds, :jobs, :pipeline_schedules, :environments, :artifacts]) do diff --git a/app/views/layouts/nav/_project.html.haml b/app/views/layouts/nav/_project.html.haml index 924cd2e9681..b88465848e3 100644 --- a/app/views/layouts/nav/_project.html.haml +++ b/app/views/layouts/nav/_project.html.haml @@ -28,7 +28,8 @@ %span Issues - if @project.issues_enabled? - %span.badge.count.issue_counter= number_with_delimiter(issuables_count_for_state(:issues, :opened, finder: IssuesFinder.new(current_user, project_id: @project.id))) + %span.badge.count.issue_counter + = number_with_delimiter(@project.open_issues_count) - if project_nav_tab? :merge_requests - controllers = [:merge_requests, 'projects/merge_requests/conflicts'] @@ -37,7 +38,8 @@ = link_to project_merge_requests_path(@project), title: 'Merge Requests', class: 'shortcuts-merge_requests' do %span Merge Requests - %span.badge.count.merge_counter.js-merge-counter= number_with_delimiter(issuables_count_for_state(:merge_requests, :opened, finder: MergeRequestsFinder.new(current_user, project_id: @project.id))) + %span.badge.count.merge_counter.js-merge-counter + = number_with_delimiter(@project.open_merge_requests_count) - if project_nav_tab? :pipelines = nav_link(controller: [:pipelines, :builds, :environments, :artifacts]) do diff --git a/changelogs/unreleased/cache-issue-and-mr-counts.yml b/changelogs/unreleased/cache-issue-and-mr-counts.yml new file mode 100644 index 00000000000..fe3fe3be976 --- /dev/null +++ b/changelogs/unreleased/cache-issue-and-mr-counts.yml @@ -0,0 +1,5 @@ +--- +title: Cache the number of open issues and merge requests +merge_request: +author: +type: other diff --git a/spec/models/issue_spec.rb b/spec/models/issue_spec.rb index 9203f6562f2..de86788d142 100644 --- a/spec/models/issue_spec.rb +++ b/spec/models/issue_spec.rb @@ -751,4 +751,22 @@ describe Issue do end end end + + describe 'removing an issue' do + it 'refreshes the number of open issues of the project' do + project = subject.project + + expect { subject.destroy } + .to change { project.open_issues_count }.from(1).to(0) + end + end + + describe '.public_only' do + it 'only returns public issues' do + public_issue = create(:issue) + create(:issue, confidential: true) + + expect(described_class.public_only).to eq([public_issue]) + end + end end diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 026bdbd26d1..1c72513520d 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -1692,4 +1692,13 @@ describe MergeRequest do expect(subject.ref_fetched?).to be_falsey end end + + describe 'removing a merge request' do + it 'refreshes the number of open merge requests of the target project' do + project = subject.target_project + + expect { subject.destroy } + .to change { project.open_merge_requests_count }.from(1).to(0) + end + end end diff --git a/spec/services/issues/close_service_spec.rb b/spec/services/issues/close_service_spec.rb index a03f68434de..171f70c32a8 100644 --- a/spec/services/issues/close_service_spec.rb +++ b/spec/services/issues/close_service_spec.rb @@ -42,6 +42,11 @@ describe Issues::CloseService do service.execute(issue) end + it 'refreshes the number of open issues' do + expect { service.execute(issue) } + .to change { project.open_issues_count }.from(1).to(0) + end + it 'invalidates counter cache for assignees' do expect_any_instance_of(User).to receive(:invalidate_issue_cache_counts) diff --git a/spec/services/issues/create_service_spec.rb b/spec/services/issues/create_service_spec.rb index 9b2d9e79f4f..78b11cd7991 100644 --- a/spec/services/issues/create_service_spec.rb +++ b/spec/services/issues/create_service_spec.rb @@ -35,6 +35,10 @@ describe Issues::CreateService do expect(issue.due_date).to eq Date.tomorrow end + it 'refreshes the number of open issues' do + expect { issue }.to change { project.open_issues_count }.from(0).to(1) + end + context 'when current user cannot admin issues in the project' do let(:guest) { create(:user) } diff --git a/spec/services/issues/reopen_service_spec.rb b/spec/services/issues/reopen_service_spec.rb index 205e9ebd237..48fc98b3b2f 100644 --- a/spec/services/issues/reopen_service_spec.rb +++ b/spec/services/issues/reopen_service_spec.rb @@ -34,6 +34,13 @@ describe Issues::ReopenService do described_class.new(project, user).execute(issue) end + it 'refreshes the number of opened issues' do + service = described_class.new(project, user) + + expect { service.execute(issue) } + .to change { project.open_issues_count }.from(0).to(1) + end + context 'when issue is not confidential' do it 'executes issue hooks' do expect(project).to receive(:execute_hooks).with(an_instance_of(Hash), :issue_hooks) diff --git a/spec/services/merge_requests/close_service_spec.rb b/spec/services/merge_requests/close_service_spec.rb index 04bf267d167..7e65369762c 100644 --- a/spec/services/merge_requests/close_service_spec.rb +++ b/spec/services/merge_requests/close_service_spec.rb @@ -52,6 +52,13 @@ describe MergeRequests::CloseService do end end + it 'refreshes the number of open merge requests for a valid MR' do + service = described_class.new(project, user, {}) + + expect { service.execute(merge_request) } + .to change { project.open_merge_requests_count }.from(1).to(0) + end + context 'current user is not authorized to close merge request' do before do perform_enqueued_jobs do diff --git a/spec/services/merge_requests/create_service_spec.rb b/spec/services/merge_requests/create_service_spec.rb index a1f3bec42cc..d6409c0d625 100644 --- a/spec/services/merge_requests/create_service_spec.rb +++ b/spec/services/merge_requests/create_service_spec.rb @@ -18,31 +18,35 @@ describe MergeRequests::CreateService do end let(:service) { described_class.new(project, user, opts) } + let(:merge_request) { service.execute } before do project.team << [user, :master] project.team << [assignee, :developer] allow(service).to receive(:execute_hooks) - - @merge_request = service.execute end it 'creates an MR' do - expect(@merge_request).to be_valid - expect(@merge_request.title).to eq('Awesome merge_request') - expect(@merge_request.assignee).to be_nil - expect(@merge_request.merge_params['force_remove_source_branch']).to eq('1') + expect(merge_request).to be_valid + expect(merge_request.title).to eq('Awesome merge_request') + expect(merge_request.assignee).to be_nil + expect(merge_request.merge_params['force_remove_source_branch']).to eq('1') end it 'executes hooks with default action' do - expect(service).to have_received(:execute_hooks).with(@merge_request) + expect(service).to have_received(:execute_hooks).with(merge_request) + end + + it 'refreshes the number of open merge requests' do + expect { service.execute } + .to change { project.open_merge_requests_count }.from(0).to(1) end it 'does not creates todos' do attributes = { project: project, - target_id: @merge_request.id, - target_type: @merge_request.class.name + target_id: merge_request.id, + target_type: merge_request.class.name } expect(Todo.where(attributes).count).to be_zero @@ -51,8 +55,8 @@ describe MergeRequests::CreateService do it 'creates exactly 1 create MR event' do attributes = { action: Event::CREATED, - target_id: @merge_request.id, - target_type: @merge_request.class.name + target_id: merge_request.id, + target_type: merge_request.class.name } expect(Event.where(attributes).count).to eq(1) @@ -69,15 +73,15 @@ describe MergeRequests::CreateService do } end - it { expect(@merge_request.assignee).to eq assignee } + it { expect(merge_request.assignee).to eq assignee } it 'creates a todo for new assignee' do attributes = { project: project, author: user, user: assignee, - target_id: @merge_request.id, - target_type: @merge_request.class.name, + target_id: merge_request.id, + target_type: merge_request.class.name, action: Todo::ASSIGNED, state: :pending } diff --git a/spec/services/merge_requests/reopen_service_spec.rb b/spec/services/merge_requests/reopen_service_spec.rb index f02af0c582e..fa652611c6b 100644 --- a/spec/services/merge_requests/reopen_service_spec.rb +++ b/spec/services/merge_requests/reopen_service_spec.rb @@ -47,6 +47,13 @@ describe MergeRequests::ReopenService do end end + it 'refreshes the number of open merge requests for a valid MR' do + service = described_class.new(project, user, {}) + + expect { service.execute(merge_request) } + .to change { project.open_merge_requests_count }.from(0).to(1) + end + context 'current user is not authorized to reopen merge request' do before do perform_enqueued_jobs do diff --git a/spec/services/projects/count_service_spec.rb b/spec/services/projects/count_service_spec.rb new file mode 100644 index 00000000000..79b01e7620e --- /dev/null +++ b/spec/services/projects/count_service_spec.rb @@ -0,0 +1,73 @@ +require 'spec_helper' + +describe Projects::CountService do + let(:project) { build(:project, id: 1) } + let(:service) { described_class.new(project) } + + describe '#relation_for_count' do + it 'raises NotImplementedError' do + expect { service.relation_for_count }.to raise_error(NotImplementedError) + end + end + + describe '#count' do + before do + allow(service).to receive(:cache_key_name).and_return('count_service') + end + + it 'returns the number of rows' do + allow(service).to receive(:uncached_count).and_return(1) + + expect(service.count).to eq(1) + end + + it 'caches the number of rows', :use_clean_rails_memory_store_caching do + expect(service).to receive(:uncached_count).once.and_return(1) + + 2.times do + expect(service.count).to eq(1) + end + end + end + + describe '#refresh_cache', :use_clean_rails_memory_store_caching do + before do + allow(service).to receive(:cache_key_name).and_return('count_service') + end + + it 'refreshes the cache' do + expect(service).to receive(:uncached_count).once.and_return(1) + + service.refresh_cache + + expect(service.count).to eq(1) + end + end + + describe '#delete_cache', :use_clean_rails_memory_store_caching do + before do + allow(service).to receive(:cache_key_name).and_return('count_service') + end + + it 'removes the cache' do + expect(service).to receive(:uncached_count).twice.and_return(1) + + service.count + service.delete_cache + service.count + end + end + + describe '#cache_key_name' do + it 'raises NotImplementedError' do + expect { service.cache_key_name }.to raise_error(NotImplementedError) + end + end + + describe '#cache_key' do + it 'returns the cache key as an Array' do + allow(service).to receive(:cache_key_name).and_return('count_service') + expect(service.cache_key).to eq(['projects', 1, 'count_service']) + end + end +end diff --git a/spec/services/projects/forks_count_service_spec.rb b/spec/services/projects/forks_count_service_spec.rb index cf299c5d09b..9f8e7ee18a8 100644 --- a/spec/services/projects/forks_count_service_spec.rb +++ b/spec/services/projects/forks_count_service_spec.rb @@ -1,40 +1,14 @@ require 'spec_helper' describe Projects::ForksCountService do - let(:project) { build(:project, id: 42) } - let(:service) { described_class.new(project) } - describe '#count' do it 'returns the number of forks' do - allow(service).to receive(:uncached_count).and_return(1) - - expect(service.count).to eq(1) - end - - it 'caches the forks count', :use_clean_rails_memory_store_caching do - expect(service).to receive(:uncached_count).once.and_return(1) + project = build(:project, id: 42) + service = described_class.new(project) - 2.times { service.count } - end - end - - describe '#refresh_cache', :use_clean_rails_memory_store_caching do - it 'refreshes the cache' do - expect(service).to receive(:uncached_count).once.and_return(1) - - service.refresh_cache + allow(service).to receive(:uncached_count).and_return(1) expect(service.count).to eq(1) end end - - describe '#delete_cache', :use_clean_rails_memory_store_caching do - it 'removes the cache' do - expect(service).to receive(:uncached_count).twice.and_return(1) - - service.count - service.delete_cache - service.count - end - end end diff --git a/spec/services/projects/open_issues_count_service_spec.rb b/spec/services/projects/open_issues_count_service_spec.rb new file mode 100644 index 00000000000..f964f9972cd --- /dev/null +++ b/spec/services/projects/open_issues_count_service_spec.rb @@ -0,0 +1,21 @@ +require 'spec_helper' + +describe Projects::OpenIssuesCountService do + describe '#count' do + it 'returns the number of open issues' do + project = create(:project) + create(:issue, :opened, project: project) + + expect(described_class.new(project).count).to eq(1) + end + + it 'does not include confidential issues in the issue count' do + project = create(:project) + + create(:issue, :opened, project: project) + create(:issue, :opened, confidential: true, project: project) + + expect(described_class.new(project).count).to eq(1) + end + end +end diff --git a/spec/services/projects/open_merge_requests_count_service_spec.rb b/spec/services/projects/open_merge_requests_count_service_spec.rb new file mode 100644 index 00000000000..9f49b9ec6a2 --- /dev/null +++ b/spec/services/projects/open_merge_requests_count_service_spec.rb @@ -0,0 +1,15 @@ +require 'spec_helper' + +describe Projects::OpenMergeRequestsCountService do + describe '#count' do + it 'returns the number of open merge requests' do + project = create(:project) + create(:merge_request, + :opened, + source_project: project, + target_project: project) + + expect(described_class.new(project).count).to eq(1) + end + end +end |