Make 1 Gitaly request to get submodule urls

Now 1 request is performed instead of 2 per submodule

8 files changed, 124 insertions, 43 deletions

diff --git a/app/graphql/types/tree/submodule_type.rb b/app/graphql/types/tree/submodule_type.rb
index 4564fc0d162..2b47e5c0161 100644
--- a/app/graphql/types/tree/submodule_type.rb
+++ b/app/graphql/types/tree/submodule_type.rb
@@ -8,16 +8,8 @@ module Types
 
       graphql_name 'Submodule'
 
-      field :web_url, type: GraphQL::STRING_TYPE, null: true, resolve: -> (obj, args, ctx) do
-        submodule_links(obj).first
-      end
-      field :tree_url, type: GraphQL::STRING_TYPE, null: true, resolve: -> (obj, args, ctx) do
-        submodule_links(obj).last
-      end
-
-      def self.submodule_links(submodule)
-        @links ||= SubmoduleHelper.submodule_links(submodule, submodule.commit_id, submodule.repository)
-      end
+      field :web_url, type: GraphQL::STRING_TYPE, null: true
+      field :tree_url, type: GraphQL::STRING_TYPE, null: true
     end
     # rubocop: enable Graphql/AuthorizeTypes
   end
diff --git a/app/graphql/types/tree/tree_type.rb b/app/graphql/types/tree/tree_type.rb
index 0e98a643cc3..20513e0118c 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::TreeEntry.decorate(obj.submodules, obj.repository)
+        Gitlab::Graphql::Representation::SubmoduleTreeEntry.decorate(obj)
       end
 
       field :blobs, Types::Tree::BlobType.connection_type, null: false, calls_gitaly: true, resolve: -> (obj, args, ctx) do
diff --git a/app/helpers/submodule_helper.rb b/app/helpers/submodule_helper.rb
index 164c69ca50b..35e04b0ced3 100644
--- a/app/helpers/submodule_helper.rb
+++ b/app/helpers/submodule_helper.rb
@@ -9,6 +9,10 @@ module SubmoduleHelper
   def submodule_links(submodule_item, ref = nil, repository = @repository)
     url = repository.submodule_url_for(ref, submodule_item.path)
 
+    submodule_links_for_url(submodule_item.id, url, repository)
+  end
+
+  def submodule_links_for_url(submodule_item_id, url, repository)
     if url == '.' || url == './'
       url = File.join(Gitlab.config.gitlab.url, repository.project.full_path)
     end
@@ -31,13 +35,13 @@ module SubmoduleHelper
 
       if self_url?(url, namespace, project)
         [namespace_project_path(namespace, project),
-         namespace_project_tree_path(namespace, project, submodule_item.id)]
+         namespace_project_tree_path(namespace, project, submodule_item_id)]
       elsif relative_self_url?(url)
-        relative_self_links(url, submodule_item.id, repository.project)
+        relative_self_links(url, submodule_item_id, repository.project)
       elsif github_dot_com_url?(url)
-        standard_links('github.com', namespace, project, submodule_item.id)
+        standard_links('github.com', namespace, project, submodule_item_id)
       elsif gitlab_dot_com_url?(url)
-        standard_links('gitlab.com', namespace, project, submodule_item.id)
+        standard_links('gitlab.com', namespace, project, submodule_item_id)
       else
         [sanitize_submodule_url(url), nil]
       end
diff --git a/app/serializers/submodule_entity.rb b/app/serializers/submodule_entity.rb
deleted file mode 100644
index e475a4f301f..00000000000
--- a/app/serializers/submodule_entity.rb
+++ /dev/null
@@ -1,25 +0,0 @@
-# frozen_string_literal: true
-
-class SubmoduleEntity < Grape::Entity
-  include RequestAwareEntity
-
-  expose :id, :path, :name, :mode
-
-  expose :icon do |blob|
-    'archive'
-  end
-
-  expose :url do |blob|
-    submodule_links(blob, request).first
-  end
-
-  expose :tree_url do |blob|
-    submodule_links(blob, request).last
-  end
-
-  private
-
-  def submodule_links(blob, request)
-    @submodule_links ||= SubmoduleHelper.submodule_links(blob, request.ref, request.repository)
-  end
-end
diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb
index b7b7578cef9..87284776db0 100644
--- a/lib/gitlab/git/repository.rb
+++ b/lib/gitlab/git/repository.rb
@@ -464,6 +464,18 @@ module Gitlab
         end
       end
 
+      # Returns path to url mappings for submodules
+      #
+      # Ex.
+      #   @repository.submodule_urls_for('master')
+      #   # => { 'rack' => 'git@localhost:rack.git' }
+      #
+      def submodule_urls_for(ref)
+        wrapped_gitaly_errors do
+          gitaly_submodule_urls_for(ref)
+        end
+      end
+
       # Return total commits count accessible from passed ref
       def commit_count(ref)
         wrapped_gitaly_errors do
@@ -1059,12 +1071,15 @@ module Gitlab
 
         return unless commit_object && commit_object.type == :COMMIT
 
+        gitaly_submodule_urls_for(ref)[path]
+      end
+
+      def gitaly_submodule_urls_for(ref)
         gitmodules = gitaly_commit_client.tree_entry(ref, '.gitmodules', Gitlab::Git::Blob::MAX_DATA_DISPLAY_SIZE)
         return unless gitmodules
 
-        found_module = GitmodulesParser.new(gitmodules.data).parse[path]
-
-        found_module && found_module['url']
+        submodules = GitmodulesParser.new(gitmodules.data).parse
+        submodules.transform_values { |submodule| submodule['url'] }
       end
 
       # Returns true if the given ref name exists
diff --git a/lib/gitlab/graphql/representation/submodule_tree_entry.rb b/lib/gitlab/graphql/representation/submodule_tree_entry.rb
new file mode 100644
index 00000000000..80d254ce68a
--- /dev/null
+++ b/lib/gitlab/graphql/representation/submodule_tree_entry.rb
@@ -0,0 +1,41 @@
+# frozen_string_literal: true
+
+module Gitlab
+  module Graphql
+    module Representation
+      class SubmoduleTreeEntry < SimpleDelegator
+        class << self
+          def decorate(tree)
+            repository = tree.repository
+            submodule_urls = repository.submodule_urls_for(tree.sha)
+
+            tree.submodules.map do |submodule|
+              self.new(submodule, submodule_urls[submodule.path], repository)
+            end
+          end
+        end
+
+        def initialize(submodule, submodule_url, repository)
+          @submodule_url = submodule_url
+          @repository = repository
+
+          super(submodule)
+        end
+
+        def web_url
+          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)
+        end
+      end
+    end
+  end
+end
diff --git a/spec/lib/gitlab/graphql/representation/submodule_tree_entry_spec.rb b/spec/lib/gitlab/graphql/representation/submodule_tree_entry_spec.rb
new file mode 100644
index 00000000000..a3ff75f17e6
--- /dev/null
+++ b/spec/lib/gitlab/graphql/representation/submodule_tree_entry_spec.rb
@@ -0,0 +1,29 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::Graphql::Representation::SubmoduleTreeEntry do
+  let(:project) { create(:project, :repository) }
+  let(:repository) { project.repository }
+
+  describe '.decorate' do
+    it 'returns array of SubmoduleTreeEntry' do
+      entries = described_class.decorate(repository.tree)
+
+      expect(entries.first).to be_a(described_class)
+
+      expect(entries.map(&:web_url)).to contain_exactly(
+        "https://gitlab.com/gitlab-org/gitlab-grack",
+        "https://github.com/gitlabhq/gitlab-shell",
+        "https://github.com/randx/six"
+      )
+
+      expect(entries.map(&:tree_url)).to contain_exactly(
+        "https://gitlab.com/gitlab-org/gitlab-grack/tree/645f6c4c82fd3f5e06f67134450a570b795e55a6",
+        "https://github.com/gitlabhq/gitlab-shell/tree/79bceae69cb5750d6567b223597999bfa91cb3b9",
+        "https://github.com/randx/six/tree/409f37c4f05865e4fb208c771485f211a22c4c2d"
+      )
+    end
+  end
+end
+
diff --git a/spec/serializers/diff_file_base_entity_spec.rb b/spec/serializers/diff_file_base_entity_spec.rb
new file mode 100644
index 00000000000..0c434f739c9
--- /dev/null
+++ b/spec/serializers/diff_file_base_entity_spec.rb
@@ -0,0 +1,25 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe DiffFileBaseEntity do
+  let(:project) { create(:project, :repository) }
+  let(:repository) { project.repository }
+
+  context 'diff for a changed submodule' do
+    let(:commit_sha_with_changed_submodule) do
+      "cfe32cf61b73a0d5e9f13e774abde7ff789b1660"
+    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 }
+
+    it do
+      expect(entity[:submodule]).to eq(true)
+      expect(entity[:submodule_link]).to eq("https://github.com/randx/six")
+      expect(entity[:submodule_tree_url]).to eq(
+        "https://github.com/randx/six/tree/409f37c4f05865e4fb208c771485f211a22c4c2d"
+      )
+    end
+  end
+end