Cache project avatars stored in Git
The avatar logic has been moved from Project to Repository as this makes caching easier. The logic itself in turn has been changed so that the logo file names are cached in Redis. This cache is flushed upon pushing a commit but _only_ if: 1. The commit was pushed to the default branch 2. The commit actually changes any of the logo files If no branch or commit is given the cache is flushed anyway, this ensures that calling Repository#expire_cache without any arguments still flushes the avatar cache (e.g. this is used when removing a project). Fixes gitlab-org/gitlab-ce#14363
This commit is contained in:
parent
f728e4b519
commit
cd05d3f78d
|
@ -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
|
||||
|
|
|
@ -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
|
||||
|
|
|
@ -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
|
||||
|
|
|
@ -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
|
||||
|
|
|
@ -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
|
||||
|
|
Loading…
Reference in New Issue