diff options
-rw-r--r-- | app/models/repository.rb | 74 | ||||
-rw-r--r-- | lib/gitlab/repository_cache.rb | 32 | ||||
-rw-r--r-- | lib/gitlab/repository_cache_adapter.rb | 84 | ||||
-rw-r--r-- | lib/repository_cache.rb | 30 | ||||
-rw-r--r-- | spec/lib/gitlab/repository_cache_adapter_spec.rb | 76 | ||||
-rw-r--r-- | spec/lib/gitlab/repository_cache_spec.rb (renamed from spec/lib/repository_cache_spec.rb) | 16 | ||||
-rw-r--r-- | spec/models/repository_spec.rb | 69 |
7 files changed, 203 insertions, 178 deletions
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/repository_cache_spec.rb b/spec/lib/gitlab/repository_cache_spec.rb index 8b0c7254b5e..f4789421663 100644 --- a/spec/lib/repository_cache_spec.rb +++ b/spec/lib/gitlab/repository_cache_spec.rb @@ -1,34 +1,36 @@ require 'spec_helper' -describe RepositoryCache do - let(:project) { create(:project) } +describe Gitlab::RepositoryCache do let(:backend) { double('backend').as_null_object } - let(:cache) { described_class.new('example', project.id, backend) } + 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:example:#{project.id}" + 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:example:#{project.id}") + 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:example:#{project.id}") + 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:example:#{project.id}", &p) + expect(backend).to have_received(:fetch).with("baz:#{namespace}", &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) |