Clear unused id for target noteable54924-fix-commenting-on-large-commits-sha1

Sets `noteable_id` to `nil` if target is a Commit

Also clears `commit_id` when target is not a Commit / MergeRequest

5 files changed, 72 insertions, 2 deletions

diff --git a/app/models/note.rb b/app/models/note.rb
index 1578ae9c4cc..b2930ada5f7 100644
--- a/app/models/note.rb
+++ b/app/models/note.rb
@@ -142,7 +142,7 @@ class Note < ActiveRecord::Base
   scope :with_metadata, -> { includes(:system_note_metadata) }
 
   after_initialize :ensure_discussion_id
-  before_validation :nullify_blank_type, :nullify_blank_line_code
+  before_validation :nullify_blank_type, :nullify_blank_line_code, :nullify_unused_id
   before_validation :set_discussion_id, on: :create
   after_save :keep_around_commit, if: :for_project_noteable?
   after_save :expire_etag_cache
@@ -474,6 +474,14 @@ class Note < ActiveRecord::Base
     self.line_code = nil if self.line_code.blank?
   end
 
+  def nullify_unused_id
+    if for_commit?
+      self.noteable_id = nil
+    elsif !for_merge_request?
+      self.commit_id = nil
+    end
+  end
+
   def ensure_discussion_id
     return unless self.persisted?
     # Needed in case the SELECT statement doesn't ask for `discussion_id`
diff --git a/changelogs/unreleased/54924-fix-commenting-on-large-commits-sha1.yml b/changelogs/unreleased/54924-fix-commenting-on-large-commits-sha1.yml
new file mode 100644
index 00000000000..e9b04540374
--- /dev/null
+++ b/changelogs/unreleased/54924-fix-commenting-on-large-commits-sha1.yml
@@ -0,0 +1,5 @@
+---
+title: Fix commenting on commits having SHA1 starting with a large number
+merge_request: 25090
+author:
+type: fixed
diff --git a/spec/controllers/projects/notes_controller_spec.rb b/spec/controllers/projects/notes_controller_spec.rb
index 81892575889..2b32657a747 100644
--- a/spec/controllers/projects/notes_controller_spec.rb
+++ b/spec/controllers/projects/notes_controller_spec.rb
@@ -266,6 +266,22 @@ describe Projects::NotesController do
       end
     end
 
+    context 'when creating a comment on a commit with large SHA1' do
+      let(:commit) { create(:commit, project: project, id: '842616594688d2351480dfebd67b3d8d15571e6d') }
+
+      it 'creates a note successfully' do
+        expect do
+          post :create, params: {
+            note: { note: 'some note', commit_id: commit.id },
+            namespace_id: project.namespace,
+            project_id: project,
+            target_type: 'commit',
+            target_id: commit.id
+          }
+        end.to change { Note.count }.by(1)
+      end
+    end
+
     context 'when creating a commit comment from an MR fork' do
       let(:project) { create(:project, :repository) }
 
diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb
index 385b8a7959f..1787ab1310f 100644
--- a/spec/models/note_spec.rb
+++ b/spec/models/note_spec.rb
@@ -19,6 +19,46 @@ describe Note do
     it { is_expected.to include_module(Awardable) }
   end
 
+  describe 'nullify unused id before validation' do
+    subject { described_class.new(noteable_id: 123, commit_id: 123) }
+
+    context 'when note is on commit' do
+      before do
+        subject.noteable_type = "Commit"
+        subject.valid?
+      end
+
+      it 'sets noteable_id to nil' do
+        expect(subject.noteable_id).to eq(nil)
+        expect(subject.commit_id).not_to eq(nil)
+      end
+    end
+
+    context 'when note is on merge request' do
+      before do
+        subject.noteable_type = "MergeRequest"
+        subject.valid?
+      end
+
+      it 'does not set ids to nil' do
+        expect(subject.noteable_id).not_to eq(nil)
+        expect(subject.commit_id).not_to eq(nil)
+      end
+    end
+
+    context 'when note is on other noteable' do
+      before do
+        subject.noteable_type = "PersonalSnippet"
+        subject.valid?
+      end
+
+      it 'sets commit_id to nil' do
+        expect(subject.noteable_id).not_to eq(nil)
+        expect(subject.commit_id).to eq(nil)
+      end
+    end
+  end
+
   describe 'validation' do
     it { is_expected.to validate_presence_of(:note) }
     it { is_expected.to validate_presence_of(:project) }
diff --git a/spec/support/helpers/test_env.rb b/spec/support/helpers/test_env.rb
index f485eb7b0eb..06bcf4f8013 100644
--- a/spec/support/helpers/test_env.rb
+++ b/spec/support/helpers/test_env.rb
@@ -63,7 +63,8 @@ module TestEnv
     'after-create-delete-modify-move'    => 'ba3faa7',
     'with-codeowners'                    => '219560e',
     'submodule_inside_folder'            => 'b491b92',
-    'png-lfs'                            => 'fe42f41'
+    'png-lfs'                            => 'fe42f41',
+    'sha-starting-with-large-number'     => '8426165'
   }.freeze
 
   # gitlab-test-fork is a fork of gitlab-fork, but we don't necessarily