Commit 9f72d3ed authored by Robert Speicher's avatar Robert Speicher

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
parents d123b8ad cd05d3f7
...@@ -571,10 +571,7 @@ class Project < ActiveRecord::Base ...@@ -571,10 +571,7 @@ class Project < ActiveRecord::Base
end end
def avatar_in_git def avatar_in_git
@avatar_file ||= 'logo.png' if repository.blob_at_branch('master', 'logo.png') repository.avatar
@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
end end
def avatar_url def avatar_url
......
...@@ -3,6 +3,10 @@ require 'securerandom' ...@@ -3,6 +3,10 @@ require 'securerandom'
class Repository class Repository
class CommitError < StandardError; end 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 include Gitlab::ShellAdapter
attr_accessor :path_with_namespace, :project attr_accessor :path_with_namespace, :project
...@@ -241,12 +245,13 @@ class Repository ...@@ -241,12 +245,13 @@ class Repository
@branches = nil @branches = nil
end end
def expire_cache(branch_name = nil) def expire_cache(branch_name = nil, revision = nil)
cache_keys.each do |key| cache_keys.each do |key|
cache.expire(key) cache.expire(key)
end end
expire_branch_cache(branch_name) expire_branch_cache(branch_name)
expire_avatar_cache(branch_name, revision)
# This ensures this particular cache is flushed after the first commit to a # This ensures this particular cache is flushed after the first commit to a
# new repository. # new repository.
...@@ -316,6 +321,23 @@ class Repository ...@@ -316,6 +321,23 @@ class Repository
cache.expire(:branch_names) cache.expire(:branch_names)
end 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. # Runs code just before a repository is deleted.
def before_delete def before_delete
expire_cache if exists? expire_cache if exists?
...@@ -350,8 +372,8 @@ class Repository ...@@ -350,8 +372,8 @@ class Repository
end end
# Runs code after a new commit has been pushed. # Runs code after a new commit has been pushed.
def after_push_commit(branch_name) def after_push_commit(branch_name, revision)
expire_cache(branch_name) expire_cache(branch_name, revision)
end end
# Runs code after a new branch has been created. # Runs code after a new branch has been created.
...@@ -857,6 +879,14 @@ class Repository ...@@ -857,6 +879,14 @@ class Repository
end end
end end
def avatar
@avatar ||= cache.fetch(:avatar) do
AVATAR_FILES.find do |file|
blob_at_branch('master', file)
end
end
end
private private
def cache def cache
......
...@@ -17,7 +17,7 @@ class GitPushService < BaseService ...@@ -17,7 +17,7 @@ class GitPushService < BaseService
# 6. Checks if the project's main language has changed # 6. Checks if the project's main language has changed
# #
def execute def execute
@project.repository.after_push_commit(branch_name) @project.repository.after_push_commit(branch_name, params[:newrev])
if push_remove_branch? if push_remove_branch?
@project.repository.after_remove_branch @project.repository.after_remove_branch
......
...@@ -597,9 +597,9 @@ describe Repository, models: true do ...@@ -597,9 +597,9 @@ describe Repository, models: true do
describe '#after_push_commit' do describe '#after_push_commit' do
it 'flushes the cache' 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
end end
...@@ -703,4 +703,81 @@ describe Repository, models: true do ...@@ -703,4 +703,81 @@ describe Repository, models: true do
repository.rm_tag('8.5') repository.rm_tag('8.5')
end end
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 end
...@@ -29,7 +29,8 @@ describe GitPushService, services: true do ...@@ -29,7 +29,8 @@ describe GitPushService, services: true do
it { is_expected.to be_truthy } it { is_expected.to be_truthy }
it 'flushes general cached data' do 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 subject
end end
...@@ -46,7 +47,8 @@ describe GitPushService, services: true do ...@@ -46,7 +47,8 @@ describe GitPushService, services: true do
it { is_expected.to be_truthy } it { is_expected.to be_truthy }
it 'flushes general cached data' do 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 subject
end end
...@@ -65,7 +67,8 @@ describe GitPushService, services: true do ...@@ -65,7 +67,8 @@ describe GitPushService, services: true do
end end
it 'flushes general cached data' do 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 subject
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment