Merge branch 'cache-refactor' into 'master'

Cache `#can_be_resolved_in_ui?` git operations

Closes gitaly#1051

See merge request gitlab-org/gitlab-ce!17589

12 files changed, 313 insertions, 216 deletions

diff --git a/app/models/repository.rb b/app/models/repository.rb
index 386fd0b1c9a..42f1ac43e29 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
@@ -924,49 +898,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|
@@ -1021,8 +952,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/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
new file mode 100644
index 00000000000..b1bf3ca4143
--- /dev/null
+++ b/lib/gitlab/repository_cache.rb
@@ -0,0 +1,33 @@
+# Interface to the Redis-backed cache store
+module Gitlab
+  class RepositoryCache
+    attr_reader :repository, :namespace, :backend
+
+    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
+
+    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/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(<<EOM.chomp)
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..fc259cf1208
--- /dev/null
+++ b/spec/lib/gitlab/repository_cache_spec.rb
@@ -0,0 +1,50 @@
+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: backend) }
+
+  describe '#cache_key' do
+    subject { cache.cache_key(:foo) }
+
+    it 'includes the namespace' do
+      expect(subject).to eq "foo:#{namespace}"
+    end
+
+    context 'with a given namespace' do
+      let(:extra_namespace) { 'my:data' }
+      let(:cache) do
+        described_class.new(repository, extra_namespace: extra_namespace,
+                                        backend: backend)
+      end
+
+      it 'includes the full namespace' do
+        expect(subject).to eq "foo:#{namespace}:#{extra_namespace}"
+      end
+    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 93a61c6ea71..5bc972bca14 100644
--- a/spec/models/repository_spec.rb
+++ b/spec/models/repository_spec.rb
@@ -2169,15 +2169,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)
@@ -2323,66 +2314,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)