From 5171e2f3d4fdc681a58e11f9615afa968324a278 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alejandro=20Rodr=C3=ADguez?= Date: Tue, 6 Mar 2018 21:12:29 -0300 Subject: Refactor RepositoryCache to make it usable in other classes --- app/models/repository.rb | 74 +-------------------- lib/gitlab/repository_cache.rb | 32 +++++++++ lib/gitlab/repository_cache_adapter.rb | 84 ++++++++++++++++++++++++ lib/repository_cache.rb | 30 --------- spec/lib/gitlab/repository_cache_adapter_spec.rb | 76 +++++++++++++++++++++ spec/lib/gitlab/repository_cache_spec.rb | 36 ++++++++++ spec/lib/repository_cache_spec.rb | 34 ---------- spec/models/repository_spec.rb | 69 ------------------- 8 files changed, 230 insertions(+), 205 deletions(-) create mode 100644 lib/gitlab/repository_cache.rb create mode 100644 lib/gitlab/repository_cache_adapter.rb delete mode 100644 lib/repository_cache.rb create mode 100644 spec/lib/gitlab/repository_cache_adapter_spec.rb create mode 100644 spec/lib/gitlab/repository_cache_spec.rb delete mode 100644 spec/lib/repository_cache_spec.rb diff --git a/app/models/repository.rb b/app/models/repository.rb index 1a14afb951a..f5b223b8b8f 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -16,6 +16,7 @@ class Repository ].freeze include Gitlab::ShellAdapter + include Gitlab::RepositoryCacheAdapter attr_accessor :full_path, :disk_path, :project, :is_wiki @@ -57,22 +58,6 @@ class Repository merge_request_template: :merge_request_template_names }.freeze - # Wraps around the given method and caches its output in Redis and an instance - # variable. - # - # This only works for methods that do not take any arguments. - def self.cache_method(name, fallback: nil, memoize_only: false) - original = :"_uncached_#{name}" - - alias_method(original, name) - - define_method(name) do - cache_method_output(name, fallback: fallback, memoize_only: memoize_only) do - __send__(original) # rubocop:disable GitlabSecurity/PublicSend - end - end - end - def initialize(full_path, project, disk_path: nil, is_wiki: false) @full_path = full_path @disk_path = disk_path || full_path @@ -302,17 +287,6 @@ class Repository expire_method_caches(CACHED_METHODS) end - # Expires the caches of a specific set of methods - def expire_method_caches(methods) - methods.each do |key| - cache.expire(key) - - ivar = cache_instance_variable_name(key) - - remove_instance_variable(ivar) if instance_variable_defined?(ivar) - end - end - def expire_avatar_cache expire_method_caches(%i(avatar)) end @@ -921,49 +895,6 @@ class Repository end end - # Caches the supplied block both in a cache and in an instance variable. - # - # The cache key and instance variable are named the same way as the value of - # the `key` argument. - # - # This method will return `nil` if the corresponding instance variable is also - # set to `nil`. This ensures we don't keep yielding the block when it returns - # `nil`. - # - # key - The name of the key to cache the data in. - # fallback - A value to fall back to in the event of a Git error. - def cache_method_output(key, fallback: nil, memoize_only: false, &block) - ivar = cache_instance_variable_name(key) - - if instance_variable_defined?(ivar) - instance_variable_get(ivar) - else - # If the repository doesn't exist and a fallback was specified we return - # that value inmediately. This saves us Rugged/gRPC invocations. - return fallback unless fallback.nil? || exists? - - begin - value = - if memoize_only - yield - else - cache.fetch(key, &block) - end - - instance_variable_set(ivar, value) - rescue Gitlab::Git::Repository::NoRepository - # Even if the above `#exists?` check passes these errors might still - # occur (for example because of a non-existing HEAD). We want to - # gracefully handle this and not cache anything - fallback - end - end - end - - def cache_instance_variable_name(key) - :"@#{key.to_s.tr('?!', '')}" - end - def file_on_head(type) if head = tree(:head) head.blobs.find do |blob| @@ -1018,8 +949,7 @@ class Repository end def cache - # TODO: should we use UUIDs here? We could move repositories without clearing this cache - @cache ||= RepositoryCache.new(full_path, @project.id) + @cache ||= Gitlab::RepositoryCache.new(self) end def tags_sorted_by_committed_date diff --git a/lib/gitlab/repository_cache.rb b/lib/gitlab/repository_cache.rb new file mode 100644 index 00000000000..b7650f6ed81 --- /dev/null +++ b/lib/gitlab/repository_cache.rb @@ -0,0 +1,32 @@ +# Interface to the Redis-backed cache store +module Gitlab + class RepositoryCache + attr_reader :repository, :namespace, :backend + + def initialize(repository, backend = Rails.cache) + @repository = repository + @namespace = "#{repository.full_path}:#{repository.project.id}" + @backend = backend + end + + def cache_key(type) + "#{type}:#{namespace}" + end + + def expire(key) + backend.delete(cache_key(key)) + end + + def fetch(key, &block) + backend.fetch(cache_key(key), &block) + end + + def exist?(key) + backend.exist?(cache_key(key)) + end + + def read(key) + backend.read(cache_key(key)) + end + end +end diff --git a/lib/gitlab/repository_cache_adapter.rb b/lib/gitlab/repository_cache_adapter.rb new file mode 100644 index 00000000000..7f64a8c9e46 --- /dev/null +++ b/lib/gitlab/repository_cache_adapter.rb @@ -0,0 +1,84 @@ +module Gitlab + module RepositoryCacheAdapter + extend ActiveSupport::Concern + + class_methods do + # Wraps around the given method and caches its output in Redis and an instance + # variable. + # + # This only works for methods that do not take any arguments. + def cache_method(name, fallback: nil, memoize_only: false) + original = :"_uncached_#{name}" + + alias_method(original, name) + + define_method(name) do + cache_method_output(name, fallback: fallback, memoize_only: memoize_only) do + __send__(original) # rubocop:disable GitlabSecurity/PublicSend + end + end + end + end + + # RepositoryCache to be used. Should be overridden by the including class + def cache + raise NotImplementedError + end + + # Caches the supplied block both in a cache and in an instance variable. + # + # The cache key and instance variable are named the same way as the value of + # the `key` argument. + # + # This method will return `nil` if the corresponding instance variable is also + # set to `nil`. This ensures we don't keep yielding the block when it returns + # `nil`. + # + # key - The name of the key to cache the data in. + # fallback - A value to fall back to in the event of a Git error. + def cache_method_output(key, fallback: nil, memoize_only: false, &block) + ivar = cache_instance_variable_name(key) + + if instance_variable_defined?(ivar) + instance_variable_get(ivar) + else + # If the repository doesn't exist and a fallback was specified we return + # that value inmediately. This saves us Rugged/gRPC invocations. + return fallback unless fallback.nil? || cache.repository.exists? + + begin + value = + if memoize_only + yield + else + cache.fetch(key, &block) + end + + instance_variable_set(ivar, value) + rescue Gitlab::Git::Repository::NoRepository + # Even if the above `#exists?` check passes these errors might still + # occur (for example because of a non-existing HEAD). We want to + # gracefully handle this and not cache anything + fallback + end + end + end + + # Expires the caches of a specific set of methods + def expire_method_caches(methods) + methods.each do |key| + cache.expire(key) + + ivar = cache_instance_variable_name(key) + + remove_instance_variable(ivar) if instance_variable_defined?(ivar) + end + end + + private + + def cache_instance_variable_name(key) + :"@#{key.to_s.tr('?!', '')}" + end + end +end diff --git a/lib/repository_cache.rb b/lib/repository_cache.rb deleted file mode 100644 index 068a95790c0..00000000000 --- a/lib/repository_cache.rb +++ /dev/null @@ -1,30 +0,0 @@ -# Interface to the Redis-backed cache store used by the Repository model -class RepositoryCache - attr_reader :namespace, :backend, :project_id - - def initialize(namespace, project_id, backend = Rails.cache) - @namespace = namespace - @backend = backend - @project_id = project_id - end - - def cache_key(type) - "#{type}:#{namespace}:#{project_id}" - end - - def expire(key) - backend.delete(cache_key(key)) - end - - def fetch(key, &block) - backend.fetch(cache_key(key), &block) - end - - def exist?(key) - backend.exist?(cache_key(key)) - end - - def read(key) - backend.read(cache_key(key)) - end -end diff --git a/spec/lib/gitlab/repository_cache_adapter_spec.rb b/spec/lib/gitlab/repository_cache_adapter_spec.rb new file mode 100644 index 00000000000..85971f2a7ef --- /dev/null +++ b/spec/lib/gitlab/repository_cache_adapter_spec.rb @@ -0,0 +1,76 @@ +require 'spec_helper' + +describe Gitlab::RepositoryCacheAdapter do + let(:project) { create(:project, :repository) } + let(:repository) { project.repository } + let(:cache) { repository.send(:cache) } + + describe '#cache_method_output', :use_clean_rails_memory_store_caching do + let(:fallback) { 10 } + + context 'with a non-existing repository' do + let(:project) { create(:project) } # No repository + + subject do + repository.cache_method_output(:cats, fallback: fallback) do + repository.cats_call_stub + end + end + + it 'returns the fallback value' do + expect(subject).to eq(fallback) + end + + it 'avoids calling the original method' do + expect(repository).not_to receive(:cats_call_stub) + + subject + end + end + + context 'with a method throwing a non-existing-repository error' do + subject do + repository.cache_method_output(:cats, fallback: fallback) do + raise Gitlab::Git::Repository::NoRepository + end + end + + it 'returns the fallback value' do + expect(subject).to eq(fallback) + end + + it 'does not cache the data' do + subject + + expect(repository.instance_variable_defined?(:@cats)).to eq(false) + expect(cache.exist?(:cats)).to eq(false) + end + end + + context 'with an existing repository' do + it 'caches the output' do + object = double + + expect(object).to receive(:number).once.and_return(10) + + 2.times do + val = repository.cache_method_output(:cats) { object.number } + + expect(val).to eq(10) + end + + expect(repository.send(:cache).exist?(:cats)).to eq(true) + expect(repository.instance_variable_get(:@cats)).to eq(10) + end + end + end + + describe '#expire_method_caches' do + it 'expires the caches of the given methods' do + expect(cache).to receive(:expire).with(:readme) + expect(cache).to receive(:expire).with(:gitignore) + + repository.expire_method_caches(%i(readme gitignore)) + end + end +end diff --git a/spec/lib/gitlab/repository_cache_spec.rb b/spec/lib/gitlab/repository_cache_spec.rb new file mode 100644 index 00000000000..f4789421663 --- /dev/null +++ b/spec/lib/gitlab/repository_cache_spec.rb @@ -0,0 +1,36 @@ +require 'spec_helper' + +describe Gitlab::RepositoryCache do + let(:backend) { double('backend').as_null_object } + let(:project) { create(:project) } + let(:repository) { project.repository } + let(:namespace) { "#{repository.full_path}:#{project.id}" } + let(:cache) { described_class.new(repository, backend) } + + describe '#cache_key' do + it 'includes the namespace' do + expect(cache.cache_key(:foo)).to eq "foo:#{namespace}" + end + end + + describe '#expire' do + it 'expires the given key from the cache' do + cache.expire(:foo) + expect(backend).to have_received(:delete).with("foo:#{namespace}") + end + end + + describe '#fetch' do + it 'fetches the given key from the cache' do + cache.fetch(:bar) + expect(backend).to have_received(:fetch).with("bar:#{namespace}") + end + + it 'accepts a block' do + p = -> {} + + cache.fetch(:baz, &p) + expect(backend).to have_received(:fetch).with("baz:#{namespace}", &p) + end + end +end diff --git a/spec/lib/repository_cache_spec.rb b/spec/lib/repository_cache_spec.rb deleted file mode 100644 index 8b0c7254b5e..00000000000 --- a/spec/lib/repository_cache_spec.rb +++ /dev/null @@ -1,34 +0,0 @@ -require 'spec_helper' - -describe RepositoryCache do - let(:project) { create(:project) } - let(:backend) { double('backend').as_null_object } - let(:cache) { described_class.new('example', project.id, backend) } - - describe '#cache_key' do - it 'includes the namespace' do - expect(cache.cache_key(:foo)).to eq "foo:example:#{project.id}" - end - end - - describe '#expire' do - it 'expires the given key from the cache' do - cache.expire(:foo) - expect(backend).to have_received(:delete).with("foo:example:#{project.id}") - end - end - - describe '#fetch' do - it 'fetches the given key from the cache' do - cache.fetch(:bar) - expect(backend).to have_received(:fetch).with("bar:example:#{project.id}") - end - - it 'accepts a block' do - p = -> {} - - cache.fetch(:baz, &p) - expect(backend).to have_received(:fetch).with("baz:example:#{project.id}", &p) - end - end -end diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index 38653e18306..0808997a31f 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -2171,15 +2171,6 @@ describe Repository do end end - describe '#expire_method_caches' do - it 'expires the caches of the given methods' do - expect_any_instance_of(RepositoryCache).to receive(:expire).with(:readme) - expect_any_instance_of(RepositoryCache).to receive(:expire).with(:gitignore) - - repository.expire_method_caches(%i(readme gitignore)) - end - end - describe '#expire_all_method_caches' do it 'expires the caches of all methods' do expect(repository).to receive(:expire_method_caches) @@ -2325,66 +2316,6 @@ describe Repository do end end - describe '#cache_method_output', :use_clean_rails_memory_store_caching do - let(:fallback) { 10 } - - context 'with a non-existing repository' do - let(:project) { create(:project) } # No repository - - subject do - repository.cache_method_output(:cats, fallback: fallback) do - repository.cats_call_stub - end - end - - it 'returns the fallback value' do - expect(subject).to eq(fallback) - end - - it 'avoids calling the original method' do - expect(repository).not_to receive(:cats_call_stub) - - subject - end - end - - context 'with a method throwing a non-existing-repository error' do - subject do - repository.cache_method_output(:cats, fallback: fallback) do - raise Gitlab::Git::Repository::NoRepository - end - end - - it 'returns the fallback value' do - expect(subject).to eq(fallback) - end - - it 'does not cache the data' do - subject - - expect(repository.instance_variable_defined?(:@cats)).to eq(false) - expect(repository.send(:cache).exist?(:cats)).to eq(false) - end - end - - context 'with an existing repository' do - it 'caches the output' do - object = double - - expect(object).to receive(:number).once.and_return(10) - - 2.times do - val = repository.cache_method_output(:cats) { object.number } - - expect(val).to eq(10) - end - - expect(repository.send(:cache).exist?(:cats)).to eq(true) - expect(repository.instance_variable_get(:@cats)).to eq(10) - end - end - end - describe '#refresh_method_caches' do it 'refreshes the caches of the given types' do expect(repository).to receive(:expire_method_caches) -- cgit v1.2.1 From bb2bf39ddfc05db887d4a02ad3a38dbf81beed1a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alejandro=20Rodr=C3=ADguez?= Date: Tue, 6 Mar 2018 22:25:12 -0300 Subject: Cache `#can_be_resolved_in_ui?` git operations --- .../merge_requests/conflicts/list_service.rb | 10 +------ changelogs/unreleased/cache-refactor.yml | 5 ++++ lib/gitlab/conflict/file_collection.rb | 32 ++++++++++++++++++++-- lib/gitlab/repository_cache.rb | 3 +- spec/lib/gitlab/conflict/file_collection_spec.rb | 32 ++++++++++++++++++++++ spec/lib/gitlab/repository_cache_spec.rb | 18 ++++++++++-- 6 files changed, 86 insertions(+), 14 deletions(-) create mode 100644 changelogs/unreleased/cache-refactor.yml diff --git a/app/services/merge_requests/conflicts/list_service.rb b/app/services/merge_requests/conflicts/list_service.rb index ca9a33678e4..72cbc49adb2 100644 --- a/app/services/merge_requests/conflicts/list_service.rb +++ b/app/services/merge_requests/conflicts/list_service.rb @@ -17,15 +17,7 @@ module MergeRequests return @conflicts_can_be_resolved_in_ui = false unless merge_request.has_complete_diff_refs? return @conflicts_can_be_resolved_in_ui = false if merge_request.branch_missing? - begin - # Try to parse each conflict. If the MR's mergeable status hasn't been - # updated, ensure that we don't say there are conflicts to resolve - # when there are no conflict files. - conflicts.files.each(&:lines) - @conflicts_can_be_resolved_in_ui = conflicts.files.length > 0 - rescue Gitlab::Git::CommandError, Gitlab::Git::Conflict::Parser::UnresolvableError, Gitlab::Git::Conflict::Resolver::ConflictSideMissing - @conflicts_can_be_resolved_in_ui = false - end + @conflicts_can_be_resolved_in_ui = conflicts.can_be_resolved_in_ui? end def conflicts diff --git a/changelogs/unreleased/cache-refactor.yml b/changelogs/unreleased/cache-refactor.yml new file mode 100644 index 00000000000..dec7a0392a5 --- /dev/null +++ b/changelogs/unreleased/cache-refactor.yml @@ -0,0 +1,5 @@ +--- +title: Cache MergeRequests can_be_resolved_in_ui? git operations +merge_request: 17589 +author: +type: performance diff --git a/lib/gitlab/conflict/file_collection.rb b/lib/gitlab/conflict/file_collection.rb index 0a3ae2c3760..3ccfd9a739d 100644 --- a/lib/gitlab/conflict/file_collection.rb +++ b/lib/gitlab/conflict/file_collection.rb @@ -1,14 +1,18 @@ module Gitlab module Conflict class FileCollection + include Gitlab::RepositoryCacheAdapter + attr_reader :merge_request, :resolver def initialize(merge_request) our_commit = merge_request.source_branch_head.raw their_commit = merge_request.target_branch_head.raw - target_repo = merge_request.target_project.repository.raw + @target_repo = merge_request.target_project.repository @source_repo = merge_request.source_project.repository.raw - @resolver = Gitlab::Git::Conflict::Resolver.new(target_repo, our_commit.id, their_commit.id) + @our_commit_id = our_commit.id + @their_commit_id = their_commit.id + @resolver = Gitlab::Git::Conflict::Resolver.new(@target_repo.raw, @our_commit_id, @their_commit_id) @merge_request = merge_request end @@ -30,6 +34,17 @@ module Gitlab end end + def can_be_resolved_in_ui? + # Try to parse each conflict. If the MR's mergeable status hasn't been + # updated, ensure that we don't say there are conflicts to resolve + # when there are no conflict files. + files.each(&:lines) + files.any? + rescue Gitlab::Git::CommandError, Gitlab::Git::Conflict::Parser::UnresolvableError, Gitlab::Git::Conflict::Resolver::ConflictSideMissing + false + end + cache_method :can_be_resolved_in_ui? + def file_for_path(old_path, new_path) files.find { |file| file.their_path == old_path && file.our_path == new_path } end @@ -56,6 +71,19 @@ Merge branch '#{merge_request.target_branch}' into '#{merge_request.source_branc #{conflict_filenames.join("\n")} EOM end + + private + + def cache + @cache ||= begin + # Use the commit ids as a namespace so if the MR branches get + # updated we instantiate the cache under a different namespace. That + # way don't have to worry about explicitly invalidating the cache + namespace = "#{@our_commit_id}:#{@their_commit_id}" + + Gitlab::RepositoryCache.new(@target_repo, extra_namespace: namespace) + end + end end end end diff --git a/lib/gitlab/repository_cache.rb b/lib/gitlab/repository_cache.rb index b7650f6ed81..b1bf3ca4143 100644 --- a/lib/gitlab/repository_cache.rb +++ b/lib/gitlab/repository_cache.rb @@ -3,9 +3,10 @@ module Gitlab class RepositoryCache attr_reader :repository, :namespace, :backend - def initialize(repository, backend = Rails.cache) + def initialize(repository, extra_namespace: nil, backend: Rails.cache) @repository = repository @namespace = "#{repository.full_path}:#{repository.project.id}" + @namespace += ":#{extra_namespace}" if extra_namespace @backend = backend end diff --git a/spec/lib/gitlab/conflict/file_collection_spec.rb b/spec/lib/gitlab/conflict/file_collection_spec.rb index 5944ce8049a..c93912db411 100644 --- a/spec/lib/gitlab/conflict/file_collection_spec.rb +++ b/spec/lib/gitlab/conflict/file_collection_spec.rb @@ -10,6 +10,38 @@ describe Gitlab::Conflict::FileCollection do end end + describe '#cache' do + it 'specifies a custom namespace with the merge request commit ids' do + our_commit = merge_request.source_branch_head.raw + their_commit = merge_request.target_branch_head.raw + custom_namespace = "#{our_commit.id}:#{their_commit.id}" + + expect(file_collection.send(:cache).namespace).to include(custom_namespace) + end + end + + describe '#can_be_resolved_in_ui?' do + it 'returns true if conflicts for this collection can be resolved in the UI' do + expect(file_collection.can_be_resolved_in_ui?).to be true + end + + it "returns false if conflicts for this collection can't be resolved in the UI" do + expect(file_collection).to receive(:files).and_raise(Gitlab::Git::Conflict::Resolver::ConflictSideMissing) + + expect(file_collection.can_be_resolved_in_ui?).to be false + end + + it 'caches the result' do + expect(file_collection).to receive(:files).twice.and_call_original + + expect(file_collection.can_be_resolved_in_ui?).to be true + + expect(file_collection).not_to receive(:files) + + expect(file_collection.can_be_resolved_in_ui?).to be true + end + end + describe '#default_commit_message' do it 'matches the format of the git CLI commit message' do expect(file_collection.default_commit_message).to eq(<