Reduce the number of submodule calls for diff filesid-submodule-url-graphql

Now 1 request is performed instead of 2 per submodule

10 files changed, 81 insertions, 27 deletions

diff --git a/app/graphql/types/tree/tree_type.rb b/app/graphql/types/tree/tree_type.rb
index 20513e0118c..99f2a6c0235 100644
--- a/app/graphql/types/tree/tree_type.rb
+++ b/app/graphql/types/tree/tree_type.rb
@@ -16,7 +16,7 @@ module Types
       end
 
       field :submodules, Types::Tree::SubmoduleType.connection_type, null: false, calls_gitaly: true, resolve: -> (obj, args, ctx) do
-        Gitlab::Graphql::Representation::SubmoduleTreeEntry.decorate(obj)
+        Gitlab::Graphql::Representation::SubmoduleTreeEntry.decorate(obj.submodules, obj)
       end
 
       field :blobs, Types::Tree::BlobType.connection_type, null: false, calls_gitaly: true, resolve: -> (obj, args, ctx) do
diff --git a/app/serializers/diff_file_base_entity.rb b/app/serializers/diff_file_base_entity.rb
index d8630165e49..ee68b4b98e0 100644
--- a/app/serializers/diff_file_base_entity.rb
+++ b/app/serializers/diff_file_base_entity.rb
@@ -3,7 +3,6 @@
 class DiffFileBaseEntity < Grape::Entity
   include RequestAwareEntity
   include BlobHelper
-  include SubmoduleHelper
   include DiffHelper
   include TreeHelper
   include ChecksCollaboration
@@ -12,12 +11,12 @@ class DiffFileBaseEntity < Grape::Entity
   expose :content_sha
   expose :submodule?, as: :submodule
 
-  expose :submodule_link do |diff_file|
-    memoized_submodule_links(diff_file).first
+  expose :submodule_link do |diff_file, options|
+    memoized_submodule_links(diff_file, options).first
   end
 
   expose :submodule_tree_url do |diff_file|
-    memoized_submodule_links(diff_file).last
+    memoized_submodule_links(diff_file, options).last
   end
 
   expose :edit_path, if: -> (_, options) { options[:merge_request] } do |diff_file|
@@ -92,10 +91,10 @@ class DiffFileBaseEntity < Grape::Entity
 
   private
 
-  def memoized_submodule_links(diff_file)
+  def memoized_submodule_links(diff_file, options)
     strong_memoize(:submodule_links) do
       if diff_file.submodule?
-        submodule_links(diff_file.blob, diff_file.content_sha, diff_file.repository)
+        options[:submodule_links].for(diff_file.blob, diff_file.content_sha)
       else
         []
       end
diff --git a/app/serializers/diffs_entity.rb b/app/serializers/diffs_entity.rb
index b51e4a7e6d0..1763fe5b6ab 100644
--- a/app/serializers/diffs_entity.rb
+++ b/app/serializers/diffs_entity.rb
@@ -64,7 +64,10 @@ class DiffsEntity < Grape::Entity
     merge_request_path(merge_request, format: :diff)
   end
 
-  expose :diff_files, using: DiffFileEntity
+  expose :diff_files do |diffs, options|
+    submodule_links = Gitlab::SubmoduleLinks.new(merge_request.project.repository)
+    DiffFileEntity.represent(diffs.diff_files, options.merge(submodule_links: submodule_links))
+  end
 
   expose :merge_request_diffs, using: MergeRequestDiffEntity, if: -> (_, options) { options[:merge_request_diffs]&.any? } do |diffs|
     options[:merge_request_diffs]
diff --git a/app/serializers/discussion_serializer.rb b/app/serializers/discussion_serializer.rb
index 5be40e74175..8bb7e93c033 100644
--- a/app/serializers/discussion_serializer.rb
+++ b/app/serializers/discussion_serializer.rb
@@ -2,4 +2,18 @@
 
 class DiscussionSerializer < BaseSerializer
   entity DiscussionEntity
+
+  def represent(resource, opts = {}, entity_class = nil)
+    super(resource, with_additional_opts(opts), entity_class)
+  end
+
+  private
+
+  def with_additional_opts(opts)
+    additional_opts = {
+      submodule_links: Gitlab::SubmoduleLinks.new(@request.project.repository)
+    }
+
+    opts.merge(additional_opts)
+  end
 end
diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb
index 87284776db0..a7d9ba51277 100644
--- a/lib/gitlab/git/repository.rb
+++ b/lib/gitlab/git/repository.rb
@@ -1071,7 +1071,8 @@ module Gitlab
 
         return unless commit_object && commit_object.type == :COMMIT
 
-        gitaly_submodule_urls_for(ref)[path]
+        urls = gitaly_submodule_urls_for(ref)
+        urls && urls[path]
       end
 
       def gitaly_submodule_urls_for(ref)
diff --git a/lib/gitlab/graphql/representation/submodule_tree_entry.rb b/lib/gitlab/graphql/representation/submodule_tree_entry.rb
index 80d254ce68a..65716dff75d 100644
--- a/lib/gitlab/graphql/representation/submodule_tree_entry.rb
+++ b/lib/gitlab/graphql/representation/submodule_tree_entry.rb
@@ -5,35 +5,28 @@ module Gitlab
     module Representation
       class SubmoduleTreeEntry < SimpleDelegator
         class << self
-          def decorate(tree)
+          def decorate(submodules, tree)
             repository = tree.repository
-            submodule_urls = repository.submodule_urls_for(tree.sha)
+            submodule_links = Gitlab::SubmoduleLinks.new(repository)
 
-            tree.submodules.map do |submodule|
-              self.new(submodule, submodule_urls[submodule.path], repository)
+            submodules.map do |submodule|
+              self.new(submodule, submodule_links.for(submodule, tree.sha))
             end
           end
         end
 
-        def initialize(submodule, submodule_url, repository)
-          @submodule_url = submodule_url
-          @repository = repository
+        def initialize(submodule, submodule_links)
+          @submodule_links = submodule_links
 
           super(submodule)
         end
 
         def web_url
-          submodule_links.first
+          @submodule_links.first
         end
 
         def tree_url
-          submodule_links.last
-        end
-
-        private
-
-        def submodule_links
-          @links ||= SubmoduleHelper.submodule_links_for_url(id, @submodule_url, @repository)
+          @submodule_links.last
         end
       end
     end
diff --git a/lib/gitlab/submodule_links.rb b/lib/gitlab/submodule_links.rb
new file mode 100644
index 00000000000..a6c0369d864
--- /dev/null
+++ b/lib/gitlab/submodule_links.rb
@@ -0,0 +1,26 @@
+# frozen_string_literal: true
+
+module Gitlab
+  class SubmoduleLinks
+    include Gitlab::Utils::StrongMemoize
+
+    def initialize(repository)
+      @repository = repository
+    end
+
+    def for(submodule, sha)
+      submodule_url = submodule_url_for(sha)[submodule.path]
+      SubmoduleHelper.submodule_links_for_url(submodule.id, submodule_url, repository)
+    end
+
+    private
+
+    attr_reader :repository
+
+    def submodule_url_for(sha)
+      strong_memoize(:"submodule_links_for_#{sha}") do
+        repository.submodule_urls_for(sha)
+      end
+    end
+  end
+end
diff --git a/spec/lib/gitlab/git/repository_spec.rb b/spec/lib/gitlab/git/repository_spec.rb
index a28b95e5bff..41b898df112 100644
--- a/spec/lib/gitlab/git/repository_spec.rb
+++ b/spec/lib/gitlab/git/repository_spec.rb
@@ -256,6 +256,22 @@ describe Gitlab::Git::Repository, :seed_helper do
     end
   end
 
+  describe '#submodule_urls_for' do
+    let(:ref) { 'master' }
+
+    it 'returns url mappings for submodules' do
+      urls = repository.submodule_urls_for(ref)
+
+      expect(urls).to eq({
+        "deeper/nested/six" => "git://github.com/randx/six.git",
+               "gitlab-grack" => "https://gitlab.com/gitlab-org/gitlab-grack.git",
+       "gitlab-shell" => "https://github.com/gitlabhq/gitlab-shell.git",
+        "nested/six" => "git://github.com/randx/six.git",
+        "six" => "git://github.com/randx/six.git"
+      })
+    end
+  end
+
   describe '#commit_count' do
     it { expect(repository.commit_count("master")).to eq(25) }
     it { expect(repository.commit_count("feature")).to eq(9) }
diff --git a/spec/lib/gitlab/graphql/representation/submodule_tree_entry_spec.rb b/spec/lib/gitlab/graphql/representation/submodule_tree_entry_spec.rb
index a3ff75f17e6..28056a6085d 100644
--- a/spec/lib/gitlab/graphql/representation/submodule_tree_entry_spec.rb
+++ b/spec/lib/gitlab/graphql/representation/submodule_tree_entry_spec.rb
@@ -7,8 +7,10 @@ describe Gitlab::Graphql::Representation::SubmoduleTreeEntry do
   let(:repository) { project.repository }
 
   describe '.decorate' do
+    let(:submodules) { repository.tree.submodules }
+
     it 'returns array of SubmoduleTreeEntry' do
-      entries = described_class.decorate(repository.tree)
+      entries = described_class.decorate(submodules, repository.tree)
 
       expect(entries.first).to be_a(described_class)
 
@@ -26,4 +28,3 @@ describe Gitlab::Graphql::Representation::SubmoduleTreeEntry do
     end
   end
 end
-
diff --git a/spec/serializers/diff_file_base_entity_spec.rb b/spec/serializers/diff_file_base_entity_spec.rb
index 0c434f739c9..68c5c665ed6 100644
--- a/spec/serializers/diff_file_base_entity_spec.rb
+++ b/spec/serializers/diff_file_base_entity_spec.rb
@@ -12,7 +12,8 @@ describe DiffFileBaseEntity do
     end
     let(:commit) { project.commit(commit_sha_with_changed_submodule) }
     let(:diff_file) { commit.diffs.diff_files.to_a.last }
-    let(:entity) { described_class.new(diff_file, request: {}).as_json }
+    let(:options) { { request: {}, submodule_links: Gitlab::SubmoduleLinks.new(repository) } }
+    let(:entity) { described_class.new(diff_file, options).as_json }
 
     it do
       expect(entity[:submodule]).to eq(true)