move signature cache read to Gpg::Commit

as we write the cache in the gpg commit class already the read should
also happen there.

This also removes all logic from the main commit class, which just
proxies the call to the Gpg::Commit now.

app/models/commit.rb

@@ -237,11 +237,6 @@ class Commit
   def signature
     return @signature if defined?(@signature)
 
-    @signature = nil
-
-    cached_signature = GpgSignature.find_by(commit_sha: sha)
-    return cached_signature if cached_signature.present?
-
     @signature = Gitlab::Gpg::Commit.new(self).signature
   end
 

lib/gitlab/gpg/commit.rb

@@ -16,6 +16,9 @@ module Gitlab
       def signature
         return unless has_signature?
 
+        cached_signature = GpgSignature.find_by(commit_sha: commit.sha)
+        return cached_signature if cached_signature.present?
+
         Gitlab::Gpg.using_tmp_keychain do
           # first we need to get the keyid from the signature to query the gpg
           # key belonging to the keyid.

spec/lib/gitlab/gpg/commit_spec.rb

@@ -11,19 +11,21 @@ RSpec.describe Gitlab::Gpg::Commit do
     end
 
     context 'known and verified public key' do
-      it 'returns a valid signature' do
-        gpg_key = create :gpg_key, key: GpgHelpers::User1.public_key, user: create(:user, email: GpgHelpers::User1.emails.first)
+      let!(:gpg_key) do
+        create :gpg_key, key: GpgHelpers::User1.public_key, user: create(:user, email: GpgHelpers::User1.emails.first)
+      end
 
+      let!(:commit) do
         raw_commit = double(:raw_commit, signature: [
           GpgHelpers::User1.signed_commit_signature,
           GpgHelpers::User1.signed_commit_base_data
         ], sha: '0beec7b5ea3f0fdbc95d0dd47f3c5bc275da8a33')
         allow(raw_commit).to receive :save!
 
-        commit = create :commit,
-          git_commit: raw_commit,
-          project: project
+        create :commit, git_commit: raw_commit, project: project
+      end
 
+      it 'returns a valid signature' do
         expect(described_class.new(commit).signature).to have_attributes(
           commit_sha: '0beec7b5ea3f0fdbc95d0dd47f3c5bc275da8a33',
           project: project,
@@ -32,22 +34,33 @@ RSpec.describe Gitlab::Gpg::Commit do
           valid_signature: true
         )
       end
+
+      it 'returns the cached signature on second call' do
+        gpg_commit = described_class.new(commit)
+
+        expect(gpg_commit).to receive(:verified_signature).twice.and_call_original
+        gpg_commit.signature
+
+        # consecutive call
+        expect(gpg_commit).not_to receive(:verified_signature).and_call_original
+        gpg_commit.signature
+      end
     end
 
     context 'known but unverified public key' do
-      it 'returns an invalid signature' do
-        gpg_key = create :gpg_key, key: GpgHelpers::User1.public_key
+      let!(:gpg_key) { create :gpg_key, key: GpgHelpers::User1.public_key }
 
+      let!(:commit) do
         raw_commit = double(:raw_commit, signature: [
           GpgHelpers::User1.signed_commit_signature,
           GpgHelpers::User1.signed_commit_base_data
         ], sha: '0beec7b5ea3f0fdbc95d0dd47f3c5bc275da8a33')
         allow(raw_commit).to receive :save!
 
-        commit = create :commit,
-          git_commit: raw_commit,
-          project: project
+        create :commit, git_commit: raw_commit, project: project
+      end
 
+      it 'returns an invalid signature' do
         expect(described_class.new(commit).signature).to have_attributes(
           commit_sha: '0beec7b5ea3f0fdbc95d0dd47f3c5bc275da8a33',
           project: project,
@@ -56,20 +69,33 @@ RSpec.describe Gitlab::Gpg::Commit do
           valid_signature: false
         )
       end
+
+      it 'returns the cached signature on second call' do
+        gpg_commit = described_class.new(commit)
+
+        expect(gpg_commit).to receive(:verified_signature).and_call_original
+        gpg_commit.signature
+
+        # consecutive call
+        expect(gpg_commit).not_to receive(:verified_signature).and_call_original
+        gpg_commit.signature
+      end
     end
 
     context 'unknown public key' do
-      it 'returns an invalid signature', :gpg do
+      let!(:commit) do
         raw_commit = double(:raw_commit, signature: [
           GpgHelpers::User1.signed_commit_signature,
           GpgHelpers::User1.signed_commit_base_data
         ], sha: '0beec7b5ea3f0fdbc95d0dd47f3c5bc275da8a33')
         allow(raw_commit).to receive :save!
 
-        commit = create :commit,
+        create :commit,
           git_commit: raw_commit,
           project: project
+      end
 
+      it 'returns an invalid signature' do
         expect(described_class.new(commit).signature).to have_attributes(
           commit_sha: '0beec7b5ea3f0fdbc95d0dd47f3c5bc275da8a33',
           project: project,
@@ -78,6 +104,17 @@ RSpec.describe Gitlab::Gpg::Commit do
           valid_signature: false
         )
       end
+
+      it 'returns the cached signature on second call' do
+        gpg_commit = described_class.new(commit)
+
+        expect(gpg_commit).to receive(:verified_signature).and_call_original
+        gpg_commit.signature
+
+        # consecutive call
+        expect(gpg_commit).not_to receive(:verified_signature).and_call_original
+        gpg_commit.signature
+      end
     end
   end
 end

spec/models/commit_spec.rb

@@ -414,86 +414,4 @@ eos
       expect(described_class.valid_hash?('a' * 41)).to be false
     end
   end
-
-  describe '#signature' do
-    it 'returns nil if the commit is not signed' do
-      expect(commit.signature).to be_nil
-    end
-
-    context 'signed commit', :gpg do
-      context 'known public key' do
-        it 'returns a valid signature' do
-          create :gpg_key, key: GpgHelpers::User1.public_key
-
-          raw_commit = double(:raw_commit, signature: [
-            GpgHelpers::User1.signed_commit_signature,
-            GpgHelpers::User1.signed_commit_base_data
-          ], sha: '0beec7b5ea3f0fdbc95d0dd47f3c5bc275da8a33')
-          allow(raw_commit).to receive :save!
-
-          commit = create :commit,
-            git_commit: raw_commit,
-            project: project
-
-          expect(commit.signature.valid_signature?).to be_truthy
-        end
-
-        it 'returns the cached validation result on second call', :gpg do
-          create :gpg_key, key: GpgHelpers::User1.public_key
-
-          raw_commit = double(:raw_commit, signature: [
-            GpgHelpers::User1.signed_commit_signature,
-            GpgHelpers::User1.signed_commit_base_data
-          ], sha: '0beec7b5ea3f0fdbc95d0dd47f3c5bc275da8a33')
-          allow(raw_commit).to receive :save!
-
-          commit = create :commit,
-            git_commit: raw_commit,
-            project: project
-
-          expect(Gitlab::Gpg::Commit).to receive(:new).and_call_original
-          expect(commit.signature.valid_signature?).to be_truthy
-
-          # second call returns the cache
-          expect(Gitlab::Gpg::Commit).not_to receive(:new).and_call_original
-          expect(commit.signature.valid_signature?).to be_truthy
-        end
-      end
-
-      context 'unknown public key' do
-        it 'returns an invalid signature if the public key is unknown', :gpg do
-          raw_commit = double(:raw_commit, signature: [
-            GpgHelpers::User1.signed_commit_signature,
-            GpgHelpers::User1.signed_commit_base_data
-          ], sha: '0beec7b5ea3f0fdbc95d0dd47f3c5bc275da8a33')
-          allow(raw_commit).to receive :save!
-
-          commit = create :commit,
-            git_commit: raw_commit,
-            project: project
-
-          expect(commit.signature.valid_signature?).to be_falsey
-        end
-
-        it 'returns the cached validation result on second call', :gpg do
-          raw_commit = double(:raw_commit, signature: [
-            GpgHelpers::User1.signed_commit_signature,
-            GpgHelpers::User1.signed_commit_base_data
-          ], sha: '0beec7b5ea3f0fdbc95d0dd47f3c5bc275da8a33')
-          allow(raw_commit).to receive :save!
-
-          commit = create :commit,
-            git_commit: raw_commit,
-            project: project
-
-          expect(Gitlab::Gpg::Commit).to receive(:new).and_call_original
-          expect(commit.signature.valid_signature?).to be_falsey
-
-          # second call returns the cache
-          expect(Gitlab::Gpg::Commit).not_to receive(:new).and_call_original
-          expect(commit.signature.valid_signature?).to be_falsey
-        end
-      end
-    end
-  end
 end