Merge branch 'test-improve-gitlab-identifier' into 'master'

Refactor Gitlab::Identifier

## What does this MR do?

This refactors `Gitlab::Identifier` so that it:

1. Has tests
2. Caches output in an instance variable to reduce queries
3. Uses only a single query to find a user by an SSH key, instead of 2

## Why was this MR needed?

This code was untested and would execute more SQL queries than needed.

See merge request !6680

1 files changed, 50 insertions, 8 deletions

diff --git a/lib/gitlab/identifier.rb b/lib/gitlab/identifier.rb
index 3e5d728f3bc..f8809db21aa 100644
--- a/lib/gitlab/identifier.rb
+++ b/lib/gitlab/identifier.rb
@@ -5,19 +5,61 @@ module Gitlab
     def identify(identifier, project, newrev)
       if identifier.blank?
         # Local push from gitlab
-        email = project.commit(newrev).author_email rescue nil
-        User.find_by(email: email) if email
-
+        identify_using_commit(project, newrev)
       elsif identifier =~ /\Auser-\d+\Z/
         # git push over http
-        user_id = identifier.gsub("user-", "")
-        User.find_by(id: user_id)
-
+        identify_using_user(identifier)
       elsif identifier =~ /\Akey-\d+\Z/
         # git push over ssh
-        key_id = identifier.gsub("key-", "")
-        Key.find_by(id: key_id).try(:user)
+        identify_using_ssh_key(identifier)
+      end
+    end
+
+    # Tries to identify a user based on a commit SHA.
+    def identify_using_commit(project, ref)
+      commit = project.commit(ref)
+
+      return if !commit || !commit.author_email
+
+      email = commit.author_email
+
+      identify_with_cache(:email, email) do
+        User.find_by(email: email)
       end
     end
+
+    # Tries to identify a user based on a user identifier (e.g. "user-123").
+    def identify_using_user(identifier)
+      user_id = identifier.gsub("user-", "")
+
+      identify_with_cache(:user, user_id) do
+        User.find_by(id: user_id)
+      end
+    end
+
+    # Tries to identify a user based on an SSH key identifier (e.g. "key-123").
+    def identify_using_ssh_key(identifier)
+      key_id = identifier.gsub("key-", "")
+
+      identify_with_cache(:ssh_key, key_id) do
+        User.find_by_ssh_key_id(key_id)
+      end
+    end
+
+    def identify_with_cache(category, key)
+      if identification_cache[category].key?(key)
+        identification_cache[category][key]
+      else
+        identification_cache[category][key] = yield
+      end
+    end
+
+    def identification_cache
+      @identification_cache ||= {
+        email: {},
+        user: {},
+        ssh_key: {}
+      }
+    end
   end
 end