Clear unused id for target noteable

Sets `noteable_id` to `nil` if target is a Commit Also clears `commit_id` when target is not a Commit / MergeRequest

Clear unused id for target noteable
Sets `noteable_id` to `nil` if target is a Commit Also clears `commit_id` when target is not a Commit / MergeRequest
9e34dbc2 · Heinrich Lee Yu · ed7144ad · 9e34dbc2 · 9e34dbc2 · 9e34dbc2
--- a/app/models/note.rb
+++ b/app/models/note.rb
@@ -142,7 +142,7 @@ def values
  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 @@ def nullify_blank_line_code
    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`

--- a/changelogs/unreleased/54924-fix-commenting-on-large-commits-sha1.yml
+++ b/changelogs/unreleased/54924-fix-commenting-on-large-commits-sha1.yml
+---
+title: Fix commenting on commits having SHA1 starting with a large number
+merge_request: 25090
+author:
+type: fixed
--- a/spec/controllers/projects/notes_controller_spec.rb
+++ b/spec/controllers/projects/notes_controller_spec.rb
@@ -266,6 +266,22 @@
      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) }


--- a/spec/models/note_spec.rb
+++ b/spec/models/note_spec.rb
@@ -19,6 +19,46 @@
    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) }

--- 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