Fix missing discussion_id in GitHub imports

The discussion_id method can return `nil` if a previously cached value
does not exist. This normally does not happen because we process notes
from GitHub in ID order, so the note we're replying to should already be
processed.

But we've seen production data where the discussion_id is nil. This may
be a GitHub API problem where notes are missing or returned out of order,
or a problem with our cache storage.

This falls back to generating a new discussion_id when this edge
case happens.

Changelog: fixed

lib/gitlab/github_import/representation/diff_note.rb

@@ -129,17 +129,7 @@ module Gitlab
 
         def discussion_id
           strong_memoize(:discussion_id) do
-            if in_reply_to_id.present?
-              current_discussion_id
-            else
-              Discussion.discussion_id(
-                Struct
-                .new(:noteable_id, :noteable_type)
-                .new(merge_request.id, NOTEABLE_TYPE)
-              ).tap do |discussion_id|
-                cache_discussion_id(discussion_id)
-              end
-            end
+            (in_reply_to_id.present? && current_discussion_id) || generate_discussion_id
           end
         end
 
@@ -160,6 +150,16 @@ module Gitlab
           side == 'RIGHT'
         end
 
+        def generate_discussion_id
+          Discussion.discussion_id(
+            Struct
+            .new(:noteable_id, :noteable_type)
+            .new(merge_request.id, NOTEABLE_TYPE)
+          ).tap do |discussion_id|
+            cache_discussion_id(discussion_id)
+          end
+        end
+
         def cache_discussion_id(discussion_id)
           Gitlab::Cache::Import::Caching.write(discussion_id_cache_key(note_id), discussion_id)
         end

spec/lib/gitlab/github_import/representation/diff_note_spec.rb

@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-RSpec.describe Gitlab::GithubImport::Representation::DiffNote, :clean_gitlab_redis_shared_state do
+RSpec.describe Gitlab::GithubImport::Representation::DiffNote, :clean_gitlab_redis_cache do
   let(:hunk) do
     '@@ -1 +1 @@
     -Hello
@@ -166,6 +166,23 @@ RSpec.describe Gitlab::GithubImport::Representation::DiffNote, :clean_gitlab_red
             expect(new_discussion_note.discussion_id)
               .to eq('SECOND_DISCUSSION_ID')
           end
+
+          context 'when cached value does not exist' do
+            it 'falls back to generating a new discussion_id' do
+              expect(Discussion)
+                .to receive(:discussion_id)
+                .and_return('NEW_DISCUSSION_ID')
+
+              reply_note = described_class.from_json_hash(
+                'note_id' => note.note_id + 1,
+                'in_reply_to_id' => note.note_id
+              )
+              reply_note.project = project
+              reply_note.merge_request = merge_request
+
+              expect(reply_note.discussion_id).to eq('NEW_DISCUSSION_ID')
+            end
+          end
         end
       end