Merge branch 'cached-avatars' into 'master'

Add Gitlab::AvatarCache and cache lookups by email [RUN ALL RSPEC] [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!55184

app/helpers/avatars_helper.rb

@@ -22,11 +22,14 @@ module AvatarsHelper
   end
 
   def avatar_icon_for_email(email = nil, size = nil, scale = 2, only_path: true)
-    user = User.find_by_any_email(email)
-    if user
-      avatar_icon_for_user(user, size, scale, only_path: only_path)
+    return gravatar_icon(email, size, scale) if email.nil?
+
+    if Feature.enabled?(:avatar_cache_for_email, @current_user, type: :development)
+      Gitlab::AvatarCache.by_email(email, size, scale, only_path) do
+        avatar_icon_by_user_email_or_gravatar(email, size, scale, only_path: only_path)
+      end
     else
-      gravatar_icon(email, size, scale)
+      avatar_icon_by_user_email_or_gravatar(email, size, scale, only_path: only_path)
     end
   end
 
@@ -101,19 +104,23 @@ module AvatarsHelper
 
   private
 
-  def user_avatar_url_for(only_path: true, **options)
-    return options[:url] if options[:url]
-
-    email = options[:user_email]
-    user = options.key?(:user) ? options[:user] : User.find_by_any_email(email)
+  def avatar_icon_by_user_email_or_gravatar(email, size, scale, only_path:)
+    user = User.find_by_any_email(email)
 
     if user
-      avatar_icon_for_user(user, options[:size], only_path: only_path)
+      avatar_icon_for_user(user, size, scale, only_path: only_path)
     else
-      gravatar_icon(email, options[:size])
+      gravatar_icon(email, size, scale)
     end
   end
 
+  def user_avatar_url_for(only_path: true, **options)
+    return options[:url] if options[:url]
+    return avatar_icon_for_user(options[:user], options[:size], only_path: only_path) if options[:user]
+
+    avatar_icon_for_email(options[:user_email], options[:size], only_path: only_path)
+  end
+
   def source_icon(source, options = {})
     avatar_url = source.try(:avatar_url)
 

app/models/concerns/avatarable.rb

@@ -20,6 +20,7 @@ module Avatarable
     mount_uploader :avatar, AvatarUploader
 
     after_initialize :add_avatar_to_batch
+    after_commit :clear_avatar_caches
   end
 
   module ShadowMethods
@@ -127,4 +128,11 @@ module Avatarable
   def avatar_mounter
     strong_memoize(:avatar_mounter) { _mounter(:avatar) }
   end
+
+  def clear_avatar_caches
+    return unless respond_to?(:verified_emails) && verified_emails.any? && avatar_changed?
+    return unless Feature.enabled?(:avatar_cache_for_email, self, type: :development)
+
+    Gitlab::AvatarCache.delete_by_email(*verified_emails)
+  end
 end

config/feature_flags/development/avatar_cache_for_email.yml

+---
+name: avatar_cache_for_email
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/55184
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/323185
+milestone: '13.10'
+type: development
+group: group::source code
+default_enabled: false

lib/gitlab/avatar_cache.rb

+# frozen_string_literal: true
+
+module Gitlab
+  class AvatarCache
+    class << self
+      # Increment this if a breaking change requires
+      # immediate cache expiry of all avatar caches.
+      #
+      # @return [Integer]
+      VERSION = 1
+
+      # @return [Symbol]
+      BASE_KEY = :avatar_cache
+
+      # @return [ActiveSupport::Duration]
+      DEFAULT_EXPIRY = 7.days
+
+      # Look up cached avatar data by email address.
+      # This accepts a block to provide the value to be
+      # cached in the event nothing is found.
+      #
+      # Multiple calls in the same request will be served from the
+      # request store.
+      #
+      # @param email [String]
+      # @param additional_keys [*Object] all must respond to `#to_s`
+      # @param expires_in [ActiveSupport::Duration, Integer]
+      # @yield [email, *additional_keys] yields the supplied params back to the block
+      # @return [String]
+      def by_email(email, *additional_keys, expires_in: DEFAULT_EXPIRY)
+        key = email_key(email)
+        subkey = additional_keys.join(":")
+
+        Gitlab::SafeRequestStore.fetch([key, subkey]) do
+          with do |redis|
+            # Look for existing cache value
+            cached = redis.hget(key, subkey)
+
+            # Return the cached entry if set
+            break cached unless cached.nil?
+
+            # Otherwise, call the block to get the value
+            to_cache = yield(email, *additional_keys).to_s
+
+            # Set it in the cache
+            redis.hset(key, subkey, to_cache)
+
+            # Update the expiry time
+            redis.expire(key, expires_in)
+
+            # Return this new value
+            break to_cache
+          end
+        end
+      end
+
+      # Remove one or more emails from the cache
+      #
+      # @param emails [String] one or more emails to delete
+      # @return [Integer] the number of keys deleted
+      def delete_by_email(*emails)
+        return 0 if emails.empty?
+
+        with do |redis|
+          keys = emails.map { |email| email_key(email) }
+
+          Gitlab::Instrumentation::RedisClusterValidator.allow_cross_slot_commands do
+            redis.unlink(*keys)
+          end
+        end
+      end
+
+      private
+
+      # @param email [String]
+      # @return [String]
+      def email_key(email)
+        "#{BASE_KEY}:v#{VERSION}:#{email}"
+      end
+
+      def with(&blk)
+        Gitlab::Redis::Cache.with(&blk) # rubocop:disable CodeReuse/ActiveRecord
+      end
+    end
+  end
+end

spec/features/participants_autocomplete_spec.rb

@@ -85,6 +85,7 @@ RSpec.describe 'Member autocomplete', :js do
     let(:note) { create(:note_on_commit, project: project, commit_id: project.commit.id) }
 
     before do
+      allow(User).to receive(:find_by_any_email).and_call_original
       allow(User).to receive(:find_by_any_email)
         .with(noteable.author_email.downcase, confirmed: true).and_return(author)
 

spec/helpers/avatars_helper_spec.rb

@@ -5,7 +5,7 @@ require 'spec_helper'
 RSpec.describe AvatarsHelper do
   include UploadHelpers
 
-  let(:user) { create(:user) }
+  let_it_be(:user) { create(:user) }
 
   describe '#project_icon & #group_icon' do
     shared_examples 'resource with a default avatar' do |source_type|
@@ -89,33 +89,60 @@ RSpec.describe AvatarsHelper do
     end
   end
 
-  describe '#avatar_icon_for_email' do
+  describe '#avatar_icon_for_email', :clean_gitlab_redis_cache do
     let(:user) { create(:user, avatar: File.open(uploaded_image_temp_path)) }
 
-    context 'using an email' do
-      context 'when there is a matching user' do
-        it 'returns a relative URL for the avatar' do
-          expect(helper.avatar_icon_for_email(user.email).to_s)
-            .to eq(user.avatar.url)
+    subject { helper.avatar_icon_for_email(user.email).to_s }
+
+    shared_examples "returns avatar for email" do
+      context 'using an email' do
+        context 'when there is a matching user' do
+          it 'returns a relative URL for the avatar' do
+            expect(subject).to eq(user.avatar.url)
+          end
         end
-      end
 
-      context 'when no user exists for the email' do
-        it 'calls gravatar_icon' do
-          expect(helper).to receive(:gravatar_icon).with('foo@example.com', 20, 2)
+        context 'when no user exists for the email' do
+          it 'calls gravatar_icon' do
+            expect(helper).to receive(:gravatar_icon).with('foo@example.com', 20, 2)
 
-          helper.avatar_icon_for_email('foo@example.com', 20, 2)
+            helper.avatar_icon_for_email('foo@example.com', 20, 2)
+          end
         end
-      end
 
-      context 'without an email passed' do
-        it 'calls gravatar_icon' do
-          expect(helper).to receive(:gravatar_icon).with(nil, 20, 2)
+        context 'without an email passed' do
+          it 'calls gravatar_icon' do
+            expect(helper).to receive(:gravatar_icon).with(nil, 20, 2)
+            expect(User).not_to receive(:find_by_any_email)
 
-          helper.avatar_icon_for_email(nil, 20, 2)
+            helper.avatar_icon_for_email(nil, 20, 2)
+          end
         end
       end
     end
+
+    context "when :avatar_cache_for_email flag is enabled" do
+      before do
+        stub_feature_flags(avatar_cache_for_email: true)
+      end
+
+      it_behaves_like "returns avatar for email"
+
+      it "caches the request" do
+        expect(User).to receive(:find_by_any_email).once.and_call_original
+
+        expect(helper.avatar_icon_for_email(user.email).to_s).to eq(user.avatar.url)
+        expect(helper.avatar_icon_for_email(user.email).to_s).to eq(user.avatar.url)
+      end
+    end
+
+    context "when :avatar_cache_for_email flag is disabled" do
+      before do
+        stub_feature_flags(avatar_cache_for_email: false)
+      end
+
+      it_behaves_like "returns avatar for email"
+    end
   end
 
   describe '#avatar_icon_for_user' do
@@ -346,7 +373,7 @@ RSpec.describe AvatarsHelper do
         is_expected.to eq tag(
           :img,
           alt: "#{options[:user_name]}'s avatar",
-          src: avatar_icon_for_email(options[:user_email], 16),
+          src: helper.avatar_icon_for_email(options[:user_email], 16),
           data: { container: 'body' },
           class: "avatar s16 has-tooltip",
           title: options[:user_name]
@@ -379,7 +406,7 @@ RSpec.describe AvatarsHelper do
           is_expected.to eq tag(
             :img,
             alt: "#{user_with_avatar.username}'s avatar",
-            src: avatar_icon_for_email(user_with_avatar.email, 16, only_path: false),
+            src: helper.avatar_icon_for_email(user_with_avatar.email, 16, only_path: false),
             data: { container: 'body' },
             class: "avatar s16 has-tooltip",
             title: user_with_avatar.username

spec/lib/gitlab/avatar_cache_spec.rb

+# frozen_string_literal: true
+
+require "spec_helper"
+
+RSpec.describe Gitlab::AvatarCache, :clean_gitlab_redis_cache do
+  def with(&blk)
+    Gitlab::Redis::Cache.with(&blk) # rubocop:disable CodeReuse/ActiveRecord
+  end
+
+  def read(key, subkey)
+    with do |redis|
+      redis.hget(key, subkey)
+    end
+  end
+
+  let(:thing) { double("thing", avatar_path: avatar_path) }
+  let(:avatar_path) { "/avatars/my_fancy_avatar.png" }
+  let(:key) { described_class.send(:email_key, "foo@bar.com") }
+
+  let(:perform_fetch) do
+    described_class.by_email("foo@bar.com", 20, 2, true) do
+      thing.avatar_path
+    end
+  end
+
+  describe "#by_email" do
+    it "writes a new value into the cache" do
+      expect(read(key, "20:2:true")).to eq(nil)
+
+      perform_fetch
+
+      expect(read(key, "20:2:true")).to eq(avatar_path)
+    end
+
+    it "finds the cached value and doesn't execute the block" do
+      expect(thing).to receive(:avatar_path).once
+
+      described_class.by_email("foo@bar.com", 20, 2, true) do
+        thing.avatar_path
+      end
+
+      described_class.by_email("foo@bar.com", 20, 2, true) do
+        thing.avatar_path
+      end
+    end
+
+    it "finds the cached value in the request store and doesn't execute the block" do
+      expect(thing).to receive(:avatar_path).once
+
+      Gitlab::WithRequestStore.with_request_store do
+        described_class.by_email("foo@bar.com", 20, 2, true) do
+          thing.avatar_path
+        end
+
+        described_class.by_email("foo@bar.com", 20, 2, true) do
+          thing.avatar_path
+        end
+
+        expect(Gitlab::SafeRequestStore.read([key, "20:2:true"])).to eq(avatar_path)
+      end
+    end
+  end
+
+  describe "#delete_by_email" do
+    subject { described_class.delete_by_email(*emails) }
+
+    before do
+      perform_fetch
+    end
+
+    context "no emails, somehow" do
+      let(:emails) { [] }
+
+      it { is_expected.to eq(0) }
+    end
+
+    context "single email" do
+      let(:emails) { "foo@bar.com" }
+
+      it "removes the email" do
+        expect(read(key, "20:2:true")).to eq(avatar_path)
+
+        expect(subject).to eq(1)
+
+        expect(read(key, "20:2:true")).to eq(nil)
+      end
+    end
+
+    context "multiple emails" do
+      let(:emails) { ["foo@bar.com", "missing@baz.com"] }
+
+      it "removes the emails it finds" do
+        expect(read(key, "20:2:true")).to eq(avatar_path)
+
+        expect(subject).to eq(1)
+
+        expect(read(key, "20:2:true")).to eq(nil)
+      end
+    end
+  end
+end

spec/models/user_spec.rb

@@ -2499,6 +2499,38 @@ RSpec.describe User do
     end
   end
 
+  describe "#clear_avatar_caches" do
+    let(:user) { create(:user) }
+
+    context "when :avatar_cache_for_email flag is enabled" do
+      before do
+        stub_feature_flags(avatar_cache_for_email: true)
+      end
+
+      it "clears the avatar cache when saving" do
+        allow(user).to receive(:avatar_changed?).and_return(true)
+
+        expect(Gitlab::AvatarCache).to receive(:delete_by_email).with(*user.verified_emails)
+
+        user.update(avatar: fixture_file_upload('spec/fixtures/dk.png'))
+      end
+    end
+
+    context "when :avatar_cache_for_email flag is disabled" do
+      before do
+        stub_feature_flags(avatar_cache_for_email: false)
+      end
+
+      it "doesn't attempt to clear the avatar cache" do
+        allow(user).to receive(:avatar_changed?).and_return(true)
+
+        expect(Gitlab::AvatarCache).not_to receive(:delete_by_email)
+
+        user.update(avatar: fixture_file_upload('spec/fixtures/dk.png'))
+      end
+    end
+  end
+
   describe '#accept_pending_invitations!' do
     let(:user) { create(:user, email: 'user@email.com') }
     let!(:project_member_invite) { create(:project_member, :invited, invite_email: user.email) }