Persist truncated note diffs on a new table45190-create-notes-diff-files

We request Gitaly in a N+1 manner to build discussion diffs. Once the diffs are from different revisions, it's hard to make a single request to the service in order to build the whole response.
With this change we solve this problem and simplify a lot fetching this piece of info.

5 files changed, 72 insertions, 22 deletions

diff --git a/app/models/concerns/diff_file.rb b/app/models/concerns/diff_file.rb
new file mode 100644
index 00000000000..72332072012
--- /dev/null
+++ b/app/models/concerns/diff_file.rb
@@ -0,0 +1,9 @@
+module DiffFile
+  extend ActiveSupport::Concern
+
+  def to_hash
+    keys = Gitlab::Git::Diff::SERIALIZE_KEYS - [:diff]
+
+    as_json(only: keys).merge(diff: diff).with_indifferent_access
+  end
+end
diff --git a/app/models/diff_note.rb b/app/models/diff_note.rb
index 616a626419b..d752d5bcdee 100644
--- a/app/models/diff_note.rb
+++ b/app/models/diff_note.rb
@@ -3,6 +3,7 @@
 # A note of this type can be resolvable.
 class DiffNote < Note
   include NoteOnDiff
+  include Gitlab::Utils::StrongMemoize
 
   NOTEABLE_TYPES = %w(MergeRequest Commit).freeze
 
@@ -12,7 +13,6 @@ class DiffNote < Note
 
   validates :original_position, presence: true
   validates :position, presence: true
-  validates :diff_line, presence: true, if: :on_text?
   validates :line_code, presence: true, line_code: true, if: :on_text?
   validates :noteable_type, inclusion: { in: NOTEABLE_TYPES }
   validate :positions_complete
@@ -23,6 +23,7 @@ class DiffNote < Note
   before_validation :update_position, on: :create, if: :on_text?
   before_validation :set_line_code, if: :on_text?
   after_save :keep_around_commits
+  after_commit :create_diff_file, on: :create
 
   def discussion_class(*)
     DiffDiscussion
@@ -53,21 +54,25 @@ class DiffNote < Note
     position.position_type == "image"
   end
 
+  def create_diff_file
+    return unless should_create_diff_file?
+
+    diff_file = fetch_diff_file
+    diff_line = diff_file.line_for_position(self.original_position)
+
+    creation_params = diff_file.diff.to_hash
+      .except(:too_large)
+      .merge(diff: diff_file.diff_hunk(diff_line))
+
+    create_note_diff_file(creation_params)
+  end
+
   def diff_file
-    @diff_file ||=
-      begin
-        if created_at_diff?(noteable.diff_refs)
-          # We're able to use the already persisted diffs (Postgres) if we're
-          # presenting a "current version" of the MR discussion diff.
-          # So no need to make an extra Gitaly diff request for it.
-          # As an extra benefit, the returned `diff_file` already
-          # has `highlighted_diff_lines` data set from Redis on
-          # `Diff::FileCollection::MergeRequestDiff`.
-          noteable.diffs(paths: original_position.paths, expanded: true).diff_files.first
-        else
-          original_position.diff_file(self.project.repository)
-        end
-      end
+    strong_memoize(:diff_file) do
+      enqueue_diff_file_creation_job if should_create_diff_file?
+
+      fetch_diff_file
+    end
   end
 
   def diff_line
@@ -98,6 +103,38 @@ class DiffNote < Note
 
   private
 
+  def enqueue_diff_file_creation_job
+    # Avoid enqueuing multiple file creation jobs at once for a note (i.e.
+    # parallel calls to `DiffNote#diff_file`).
+    lease = Gitlab::ExclusiveLease.new("note_diff_file_creation:#{id}", timeout: 1.hour.to_i)
+    return unless lease.try_obtain
+
+    CreateNoteDiffFileWorker.perform_async(id)
+  end
+
+  def should_create_diff_file?
+    on_text? && note_diff_file.nil? && self == discussion.first_note
+  end
+
+  def fetch_diff_file
+    if note_diff_file
+      diff = Gitlab::Git::Diff.new(note_diff_file.to_hash)
+      Gitlab::Diff::File.new(diff,
+                             repository: project.repository,
+                             diff_refs: original_position.diff_refs)
+    elsif created_at_diff?(noteable.diff_refs)
+      # We're able to use the already persisted diffs (Postgres) if we're
+      # presenting a "current version" of the MR discussion diff.
+      # So no need to make an extra Gitaly diff request for it.
+      # As an extra benefit, the returned `diff_file` already
+      # has `highlighted_diff_lines` data set from Redis on
+      # `Diff::FileCollection::MergeRequestDiff`.
+      noteable.diffs(paths: original_position.paths, expanded: true).diff_files.first
+    else
+      original_position.diff_file(self.project.repository)
+    end
+  end
+
   def supported?
     for_commit? || self.noteable.has_complete_diff_refs?
   end
diff --git a/app/models/merge_request_diff_file.rb b/app/models/merge_request_diff_file.rb
index 1199ff5af22..cd8ba6b904d 100644
--- a/app/models/merge_request_diff_file.rb
+++ b/app/models/merge_request_diff_file.rb
@@ -1,5 +1,6 @@
 class MergeRequestDiffFile < ActiveRecord::Base
   include Gitlab::EncodingHelper
+  include DiffFile
 
   belongs_to :merge_request_diff
 
@@ -12,10 +13,4 @@ class MergeRequestDiffFile < ActiveRecord::Base
   def diff
     binary? ? super.unpack('m0').first : super
   end
-
-  def to_hash
-    keys = Gitlab::Git::Diff::SERIALIZE_KEYS - [:diff]
-
-    as_json(only: keys).merge(diff: diff).with_indifferent_access
-  end
 end
diff --git a/app/models/note.rb b/app/models/note.rb
index 109405d3f17..02f7a9b1e4f 100644
--- a/app/models/note.rb
+++ b/app/models/note.rb
@@ -63,6 +63,7 @@ class Note < ActiveRecord::Base
   has_many :todos
   has_many :events, as: :target, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
   has_one :system_note_metadata
+  has_one :note_diff_file, inverse_of: :diff_note, foreign_key: :diff_note_id
 
   delegate :gfm_reference, :local_reference, to: :noteable
   delegate :name, to: :project, prefix: true
@@ -100,7 +101,8 @@ class Note < ActiveRecord::Base
   scope :inc_author_project, -> { includes(:project, :author) }
   scope :inc_author, -> { includes(:author) }
   scope :inc_relations_for_view, -> do
-    includes(:project, :author, :updated_by, :resolved_by, :award_emoji, :system_note_metadata)
+    includes(:project, :author, :updated_by, :resolved_by, :award_emoji,
+             :system_note_metadata, :note_diff_file)
   end
 
   scope :diff_notes, -> { where(type: %w(LegacyDiffNote DiffNote)) }
diff --git a/app/models/note_diff_file.rb b/app/models/note_diff_file.rb
new file mode 100644
index 00000000000..e688018a6d9
--- /dev/null
+++ b/app/models/note_diff_file.rb
@@ -0,0 +1,7 @@
+class NoteDiffFile < ActiveRecord::Base
+  include DiffFile
+
+  belongs_to :diff_note, inverse_of: :note_diff_file
+
+  validates :diff_note, presence: true
+end