From 52fdddbd2d1e89a6a41a59a41adf0820cc98c435 Mon Sep 17 00:00:00 2001
From: Stan Hu <stanhu@gmail.com>
Date: Wed, 19 Dec 2018 15:18:20 -0800
Subject: [PATCH] Cache avatar URLs and paths within a request

In a merge request with many discussions, the avatar URL can be called
thousands of times, inflicting a significant performance penalty
especially when avatars are stored in object storage. To mitigate this
problem, we can just cache the generated path any time it is requested.

Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/55355
---
 app/models/concerns/avatarable.rb             | 13 ++++++-
 .../unreleased/sh-cache-avatar-paths.yml      |  5 +++
 spec/models/concerns/avatarable_spec.rb       | 37 +++++++++++++++++++
 3 files changed, 54 insertions(+), 1 deletion(-)
 create mode 100644 changelogs/unreleased/sh-cache-avatar-paths.yml

diff --git a/app/models/concerns/avatarable.rb b/app/models/concerns/avatarable.rb
index b42236c1fa2..4687ec7d166 100644
--- a/app/models/concerns/avatarable.rb
+++ b/app/models/concerns/avatarable.rb
@@ -43,7 +43,18 @@ module Avatarable
   end
 
   def avatar_path(only_path: true, size: nil)
-    return unless self[:avatar].present?
+    unless self.try(:id)
+      return uncached_avatar_path(only_path: only_path, size: size)
+    end
+
+    # Cache this avatar path only within the request because avatars in
+    # object storage may be generated with time-limited, signed URLs.
+    key = "#{self.class.name}:#{self.id}:#{only_path}:#{size}"
+    Gitlab::SafeRequestStore[key] ||= uncached_avatar_path(only_path: only_path, size: size)
+  end
+
+  def uncached_avatar_path(only_path: true, size: nil)
+    return unless self.try(:avatar).present?
 
     asset_host = ActionController::Base.asset_host
     use_asset_host = asset_host.present?
diff --git a/changelogs/unreleased/sh-cache-avatar-paths.yml b/changelogs/unreleased/sh-cache-avatar-paths.yml
new file mode 100644
index 00000000000..b59a4db413d
--- /dev/null
+++ b/changelogs/unreleased/sh-cache-avatar-paths.yml
@@ -0,0 +1,5 @@
+---
+title: Cache avatar URLs and paths within a request
+merge_request: 23950
+author:
+type: performance
diff --git a/spec/models/concerns/avatarable_spec.rb b/spec/models/concerns/avatarable_spec.rb
index 7d617cb7b19..1ea7f2b9985 100644
--- a/spec/models/concerns/avatarable_spec.rb
+++ b/spec/models/concerns/avatarable_spec.rb
@@ -33,6 +33,43 @@ describe Avatarable do
   end
 
   describe '#avatar_path' do
+    context 'with caching enabled', :request_store do
+      let!(:avatar_path) { [relative_url_root, project.avatar.local_url].join }
+      let!(:avatar_url) { [gitlab_host, relative_url_root, project.avatar.local_url].join }
+
+      it 'only calls local_url once' do
+        expect(project.avatar).to receive(:local_url).once.and_call_original
+
+        2.times do
+          expect(project.avatar_path).to eq(avatar_path)
+        end
+      end
+
+      it 'calls local_url twice for path and URLs' do
+        expect(project.avatar).to receive(:local_url).exactly(2).times.and_call_original
+
+        expect(project.avatar_path(only_path: true)).to eq(avatar_path)
+        expect(project.avatar_path(only_path: false)).to eq(avatar_url)
+      end
+
+      it 'calls local_url twice for different sizes' do
+        expect(project.avatar).to receive(:local_url).exactly(2).times.and_call_original
+
+        expect(project.avatar_path).to eq(avatar_path)
+        expect(project.avatar_path(size: 40)).to eq(avatar_path + "?width=40")
+      end
+
+      it 'handles unpersisted objects' do
+        new_project = build(:project, :with_avatar)
+        path = [relative_url_root, new_project.avatar.local_url].join
+        expect(new_project.avatar).to receive(:local_url).exactly(2).times.and_call_original
+
+        2.times do
+          expect(new_project.avatar_path).to eq(path)
+        end
+      end
+    end
+
     using RSpec::Parameterized::TableSyntax
 
     where(:has_asset_host, :visibility_level, :only_path, :avatar_path_prefix) do