Merge branch 'id-submodule-url-graphql' into 'master'

Reduce the number of Gitaly calls for submodules

See merge request gitlab-org/gitlab-ce!30735

18 files changed, 213 insertions, 43 deletions

diff --git a/app/assets/javascripts/repository/components/table/index.vue b/app/assets/javascripts/repository/components/table/index.vue
index 0d9e992e596..610c7e8d99e 100644
--- a/app/assets/javascripts/repository/components/table/index.vue
+++ b/app/assets/javascripts/repository/components/table/index.vue
@@ -137,6 +137,7 @@ export default {
               :path="entry.flatPath"
               :type="entry.type"
               :url="entry.webUrl"
+              :submodule-tree-url="entry.treeUrl"
               :lfs-oid="entry.lfsOid"
             />
           </template>
diff --git a/app/assets/javascripts/repository/components/table/row.vue b/app/assets/javascripts/repository/components/table/row.vue
index 3e060e9ecb6..6029460d975 100644
--- a/app/assets/javascripts/repository/components/table/row.vue
+++ b/app/assets/javascripts/repository/components/table/row.vue
@@ -62,6 +62,11 @@ export default {
       required: false,
       default: null,
     },
+    submoduleTreeUrl: {
+      type: String,
+      required: false,
+      default: null,
+    },
   },
   data() {
     return {
@@ -112,7 +117,7 @@ export default {
       </component>
       <gl-badge v-if="lfsOid" variant="default" class="label-lfs ml-1">LFS</gl-badge>
       <template v-if="isSubmodule">
-        @ <gl-link href="#" class="commit-sha">{{ shortSha }}</gl-link>
+        @ <gl-link :href="submoduleTreeUrl" class="commit-sha">{{ shortSha }}</gl-link>
       </template>
     </td>
     <td class="d-none d-sm-table-cell tree-commit">
diff --git a/app/assets/javascripts/repository/queries/getFiles.query.graphql b/app/assets/javascripts/repository/queries/getFiles.query.graphql
index 4c24fc4087f..b3cc0878cad 100644
--- a/app/assets/javascripts/repository/queries/getFiles.query.graphql
+++ b/app/assets/javascripts/repository/queries/getFiles.query.graphql
@@ -35,6 +35,8 @@ query getFiles(
           edges {
             node {
               ...TreeEntry
+              webUrl
+              treeUrl
             }
           }
           pageInfo {
diff --git a/app/graphql/types/tree/submodule_type.rb b/app/graphql/types/tree/submodule_type.rb
index 8cb1e04f5ba..2b47e5c0161 100644
--- a/app/graphql/types/tree/submodule_type.rb
+++ b/app/graphql/types/tree/submodule_type.rb
@@ -7,6 +7,9 @@ module Types
       implements Types::Tree::EntryType
 
       graphql_name 'Submodule'
+
+      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 fbdc1597461..99f2a6c0235 100644
--- a/app/graphql/types/tree/tree_type.rb
+++ b/app/graphql/types/tree/tree_type.rb
@@ -15,7 +15,9 @@ module Types
         Gitlab::Graphql::Representation::TreeEntry.decorate(obj.trees, obj.repository)
       end
 
-      field :submodules, Types::Tree::SubmoduleType.connection_type, null: false, calls_gitaly: true
+      field :submodules, Types::Tree::SubmoduleType.connection_type, null: false, calls_gitaly: true, resolve: -> (obj, args, ctx) do
+        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
         Gitlab::Graphql::Representation::TreeEntry.decorate(obj.blobs, obj.repository)
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/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/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..a7d9ba51277 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,16 @@ module Gitlab
 
         return unless commit_object && commit_object.type == :COMMIT
 
+        urls = gitaly_submodule_urls_for(ref)
+        urls && urls[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..65716dff75d
--- /dev/null
+++ b/lib/gitlab/graphql/representation/submodule_tree_entry.rb
@@ -0,0 +1,34 @@
+# frozen_string_literal: true
+
+module Gitlab
+  module Graphql
+    module Representation
+      class SubmoduleTreeEntry < SimpleDelegator
+        class << self
+          def decorate(submodules, tree)
+            repository = tree.repository
+            submodule_links = Gitlab::SubmoduleLinks.new(repository)
+
+            submodules.map do |submodule|
+              self.new(submodule, submodule_links.for(submodule, tree.sha))
+            end
+          end
+        end
+
+        def initialize(submodule, submodule_links)
+          @submodule_links = submodule_links
+
+          super(submodule)
+        end
+
+        def web_url
+          @submodule_links.first
+        end
+
+        def tree_url
+          @submodule_links.last
+        end
+      end
+    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/frontend/repository/components/table/row_spec.js b/spec/frontend/repository/components/table/row_spec.js
index c566057ad3f..e539c560975 100644
--- a/spec/frontend/repository/components/table/row_spec.js
+++ b/spec/frontend/repository/components/table/row_spec.js
@@ -1,5 +1,5 @@
 import { shallowMount, RouterLinkStub } from '@vue/test-utils';
-import { GlBadge } from '@gitlab/ui';
+import { GlBadge, GlLink } from '@gitlab/ui';
 import { visitUrl } from '~/lib/utils/url_utility';
 import TableRow from '~/repository/components/table/row.vue';
 
@@ -142,4 +142,18 @@ describe('Repository table row component', () => {
 
     expect(vm.find(GlBadge).exists()).toBe(true);
   });
+
+  it('renders commit and web links with href for submodule', () => {
+    factory({
+      id: '1',
+      path: 'test',
+      type: 'commit',
+      url: 'https://test.com',
+      submoduleTreeUrl: 'https://test.com/commit',
+      currentPath: '/',
+    });
+
+    expect(vm.find('a').attributes('href')).toEqual('https://test.com');
+    expect(vm.find(GlLink).attributes('href')).toEqual('https://test.com/commit');
+  });
 });
diff --git a/spec/graphql/types/tree/submodule_type_spec.rb b/spec/graphql/types/tree/submodule_type_spec.rb
index bdb3149b41c..768eccba68c 100644
--- a/spec/graphql/types/tree/submodule_type_spec.rb
+++ b/spec/graphql/types/tree/submodule_type_spec.rb
@@ -5,5 +5,5 @@ require 'spec_helper'
 describe Types::Tree::SubmoduleType do
   it { expect(described_class.graphql_name).to eq('Submodule') }
 
-  it { expect(described_class).to have_graphql_fields(:id, :name, :type, :path, :flat_path) }
+  it { expect(described_class).to have_graphql_fields(:id, :name, :type, :path, :flat_path, :web_url, :tree_url) }
 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
new file mode 100644
index 00000000000..28056a6085d
--- /dev/null
+++ b/spec/lib/gitlab/graphql/representation/submodule_tree_entry_spec.rb
@@ -0,0 +1,30 @@
+# 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
+    let(:submodules) { repository.tree.submodules }
+
+    it 'returns array of SubmoduleTreeEntry' do
+      entries = described_class.decorate(submodules, 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..68c5c665ed6
--- /dev/null
+++ b/spec/serializers/diff_file_base_entity_spec.rb
@@ -0,0 +1,26 @@
+# 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(: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)
+      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