Merge branch 'caching-project-avatars' into 'master'

Cache project avatars stored in Git

Related issue: https://gitlab.com/gitlab-org/gitlab-ce/issues/14363

See merge request !3272

5 files changed, 120 insertions, 13 deletions

diff --git a/app/models/project.rb b/app/models/project.rb
index ab4913e99a8..412c6c6732d 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -571,10 +571,7 @@ class Project < ActiveRecord::Base
   end
 
   def avatar_in_git
-    @avatar_file ||= 'logo.png' if repository.blob_at_branch('master', 'logo.png')
-    @avatar_file ||= 'logo.jpg' if repository.blob_at_branch('master', 'logo.jpg')
-    @avatar_file ||= 'logo.gif' if repository.blob_at_branch('master', 'logo.gif')
-    @avatar_file
+    repository.avatar
   end
 
   def avatar_url
diff --git a/app/models/repository.rb b/app/models/repository.rb
index e555e97689d..036919c27b2 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -3,6 +3,10 @@ require 'securerandom'
 class Repository
   class CommitError < StandardError; end
 
+  # Files to use as a project avatar in case no avatar was uploaded via the web
+  # UI.
+  AVATAR_FILES = %w{logo.png logo.jpg logo.gif}
+
   include Gitlab::ShellAdapter
 
   attr_accessor :path_with_namespace, :project
@@ -241,12 +245,13 @@ class Repository
     @branches = nil
   end
 
-  def expire_cache(branch_name = nil)
+  def expire_cache(branch_name = nil, revision = nil)
     cache_keys.each do |key|
       cache.expire(key)
     end
 
     expire_branch_cache(branch_name)
+    expire_avatar_cache(branch_name, revision)
 
     # This ensures this particular cache is flushed after the first commit to a
     # new repository.
@@ -316,6 +321,23 @@ class Repository
     cache.expire(:branch_names)
   end
 
+  def expire_avatar_cache(branch_name = nil, revision = nil)
+    # Avatars are pulled from the default branch, thus if somebody pushes to a
+    # different branch there's no need to expire anything.
+    return if branch_name && branch_name != root_ref
+
+    # We don't want to flush the cache if the commit didn't actually make any
+    # changes to any of the possible avatar files.
+    if revision && commit = self.commit(revision)
+      return unless commit.diffs.
+        any? { |diff| AVATAR_FILES.include?(diff.new_path) }
+    end
+
+    cache.expire(:avatar)
+
+    @avatar = nil
+  end
+
   # Runs code just before a repository is deleted.
   def before_delete
     expire_cache if exists?
@@ -350,8 +372,8 @@ class Repository
   end
 
   # Runs code after a new commit has been pushed.
-  def after_push_commit(branch_name)
-    expire_cache(branch_name)
+  def after_push_commit(branch_name, revision)
+    expire_cache(branch_name, revision)
   end
 
   # Runs code after a new branch has been created.
@@ -857,6 +879,14 @@ class Repository
     end
   end
 
+  def avatar
+    @avatar ||= cache.fetch(:avatar) do
+      AVATAR_FILES.find do |file|
+        blob_at_branch('master', file)
+      end
+    end
+  end
+
   private
 
   def cache
diff --git a/app/services/git_push_service.rb b/app/services/git_push_service.rb
index d840ab5e340..14e2a2c0699 100644
--- a/app/services/git_push_service.rb
+++ b/app/services/git_push_service.rb
@@ -17,7 +17,7 @@ class GitPushService < BaseService
   #  6. Checks if the project's main language has changed
   #
   def execute
-    @project.repository.after_push_commit(branch_name)
+    @project.repository.after_push_commit(branch_name, params[:newrev])
 
     if push_remove_branch?
       @project.repository.after_remove_branch
diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb
index fc2ab2d9931..536fe66b21b 100644
--- a/spec/models/repository_spec.rb
+++ b/spec/models/repository_spec.rb
@@ -597,9 +597,9 @@ describe Repository, models: true do
 
   describe '#after_push_commit' do
     it 'flushes the cache' do
-      expect(repository).to receive(:expire_cache).with('master')
+      expect(repository).to receive(:expire_cache).with('master', '123')
 
-      repository.after_push_commit('master')
+      repository.after_push_commit('master', '123')
     end
   end
 
@@ -703,4 +703,81 @@ describe Repository, models: true do
       repository.rm_tag('8.5')
     end
   end
+
+  describe '#avatar' do
+    it 'returns the first avatar file found in the repository' do
+      expect(repository).to receive(:blob_at_branch).
+        with('master', 'logo.png').
+        and_return(true)
+
+      expect(repository.avatar).to eq('logo.png')
+    end
+
+    it 'caches the output' do
+      allow(repository).to receive(:blob_at_branch).
+        with('master', 'logo.png').
+        and_return(true)
+
+      expect(repository.avatar).to eq('logo.png')
+
+      expect(repository).to_not receive(:blob_at_branch)
+      expect(repository.avatar).to eq('logo.png')
+    end
+  end
+
+  describe '#expire_avatar_cache' do
+    let(:cache) { repository.send(:cache) }
+
+    before do
+      allow(repository).to receive(:cache).and_return(cache)
+    end
+
+    context 'without a branch or revision' do
+      it 'flushes the cache' do
+        expect(cache).to receive(:expire).with(:avatar)
+
+        repository.expire_avatar_cache
+      end
+    end
+
+    context 'with a branch' do
+      it 'does not flush the cache if the branch is not the default branch' do
+        expect(cache).not_to receive(:expire)
+
+        repository.expire_avatar_cache('cats')
+      end
+
+      it 'flushes the cache if the branch equals the default branch' do
+        expect(cache).to receive(:expire).with(:avatar)
+
+        repository.expire_avatar_cache(repository.root_ref)
+      end
+    end
+
+    context 'with a branch and revision' do
+      let(:commit) { double(:commit) }
+
+      before do
+        allow(repository).to receive(:commit).and_return(commit)
+      end
+
+      it 'does not flush the cache if the commit does not change any logos' do
+        diff = double(:diff, new_path: 'test.txt')
+
+        expect(commit).to receive(:diffs).and_return([diff])
+        expect(cache).not_to receive(:expire)
+
+        repository.expire_avatar_cache(repository.root_ref, '123')
+      end
+
+      it 'flushes the cache if the commit changes any of the logos' do
+        diff = double(:diff, new_path: Repository::AVATAR_FILES[0])
+
+        expect(commit).to receive(:diffs).and_return([diff])
+        expect(cache).to receive(:expire).with(:avatar)
+
+        repository.expire_avatar_cache(repository.root_ref, '123')
+      end
+    end
+  end
 end
diff --git a/spec/services/git_push_service_spec.rb b/spec/services/git_push_service_spec.rb
index 145bc937560..b49ca96e8e8 100644
--- a/spec/services/git_push_service_spec.rb
+++ b/spec/services/git_push_service_spec.rb
@@ -29,7 +29,8 @@ describe GitPushService, services: true do
       it { is_expected.to be_truthy }
 
       it 'flushes general cached data' do
-        expect(project.repository).to receive(:expire_cache).with('master')
+        expect(project.repository).to receive(:expire_cache).
+          with('master', newrev)
 
         subject
       end
@@ -46,7 +47,8 @@ describe GitPushService, services: true do
       it { is_expected.to be_truthy }
 
       it 'flushes general cached data' do
-        expect(project.repository).to receive(:expire_cache).with('master')
+        expect(project.repository).to receive(:expire_cache).
+          with('master', newrev)
 
         subject
       end
@@ -65,7 +67,8 @@ describe GitPushService, services: true do
       end
 
       it 'flushes general cached data' do
-        expect(project.repository).to receive(:expire_cache).with('master')
+        expect(project.repository).to receive(:expire_cache).
+          with('master', newrev)
 
         subject
       end