Refactor Entities::Snippet to use files not blobs

In order to improve performance, this commit refactors
the entity to use repository.ls_files over blobs

app/helpers/gitlab_routing_helper.rb

@@ -271,21 +271,17 @@ module GitlabRoutingHelper
     end
   end
 
-  def gitlab_raw_snippet_blob_url(blob)
-    snippet = blob.container
+  def gitlab_raw_snippet_blob_url(snippet, path, ref = nil)
+    params = {
+      snippet_id: snippet,
+      ref: ref || snippet.repository.root_ref,
+      path: path
+    }
 
     if snippet.is_a?(ProjectSnippet)
-      namespace_project_snippet_blob_raw_url(
-        {
-          namespace_id: snippet.project.namespace,
-          project_id: snippet.project,
-          snippet_id: snippet,
-          ref: blob.repository.root_ref,
-          path: blob.path
-        }
-      )
+      project_snippet_blob_raw_url(snippet.project, params)
     else
-      snippet_blob_raw_url(snippet, ref: blob.repository.root_ref, path: blob.path)
+      snippet_blob_raw_url(params)
     end
   end
 

app/models/snippet.rb

@@ -334,7 +334,13 @@ class Snippet < ApplicationRecord
   def file_name_on_repo
     return if repository.empty?
 
-    repository.ls_files(repository.root_ref).first
+    list_files(repository.root_ref).first
+  end
+
+  def list_files(ref = nil)
+    return [] if repository.empty?
+
+    repository.ls_files(ref)
   end
 
   class << self

lib/api/entities/snippet.rb

@@ -17,7 +17,14 @@ module API
       expose :file_name do |snippet|
         snippet.file_name_on_repo || snippet.file_name
       end
-      expose :blobs, as: :files, using: Entities::SnippetBlob, if: ->(snippet, options) { snippet_multiple_files?(snippet, options[:current_user]) }
+      expose :files, if: ->(snippet, options) { snippet_multiple_files?(snippet, options[:current_user]) } do |snippet, options|
+        snippet.list_files.map do |file|
+          {
+            path: file,
+            raw_url: Gitlab::UrlBuilder.build(snippet, file: file, ref: snippet.repository.root_ref)
+          }
+        end
+      end
 
       def snippet_multiple_files?(snippet, current_user)
         ::Feature.enabled?(:snippet_multiple_files, current_user) && snippet.repository_exists?

lib/api/entities/snippet_blob.rb

-# frozen_string_literal: true
-
-module API
-  module Entities
-    class SnippetBlob < Grape::Entity
-      expose :path
-      expose :raw_url do |blob|
-        Gitlab::UrlBuilder.build(blob)
-      end
-    end
-  end
-end

lib/gitlab/url_builder.rb

@@ -18,8 +18,6 @@ module Gitlab
       def build(object, **options)
         # Objects are sometimes wrapped in a BatchLoader instance
         case object.itself
-        when Blob
-          snippet_blob_raw_url(object)
         when ::Ci::Build
           instance.project_job_url(object.project, object, **options)
         when Commit
@@ -73,17 +71,17 @@ module Gitlab
       end
 
       def snippet_url(snippet, **options)
-        if options.delete(:raw).present?
+        if options[:file].present?
+          file, ref = options.values_at(:file, :ref)
+
+          instance.gitlab_raw_snippet_blob_url(snippet, file, ref)
+        elsif options.delete(:raw).present?
           instance.gitlab_raw_snippet_url(snippet, **options)
         else
           instance.gitlab_snippet_url(snippet, **options)
         end
       end
 
-      def snippet_blob_raw_url(blob)
-        instance.gitlab_raw_snippet_blob_url(blob)
-      end
-
       def wiki_url(wiki, **options)
         return wiki_page_url(wiki, Wiki::HOMEPAGE, **options) unless options[:action]
 

spec/helpers/gitlab_routing_helper_spec.rb

@@ -193,18 +193,27 @@ RSpec.describe GitlabRoutingHelper do
 
     describe '#gitlab_raw_snippet_blob_url' do
       let(:blob) { snippet.blobs.first }
-      let(:ref)  { blob.repository.root_ref }
+      let(:ref)  { 'snippet-test-ref' }
 
-      context 'with PersonalSnippet' do
+      context 'for a PersonalSnippet' do
         let(:snippet) { personal_snippet }
 
-        it { expect(gitlab_raw_snippet_blob_url(blob)).to eq("http://test.host/-/snippets/#{snippet.id}/raw/#{ref}/#{blob.path}") }
+        it { expect(gitlab_raw_snippet_blob_url(snippet, blob.path, ref)).to eq("http://test.host/-/snippets/#{snippet.id}/raw/#{ref}/#{blob.path}") }
       end
 
-      context 'with ProjectSnippet' do
+      context 'for a ProjectSnippet' do
         let(:snippet) { project_snippet }
 
-        it { expect(gitlab_raw_snippet_blob_url(blob)).to eq("http://test.host/#{snippet.project.full_path}/-/snippets/#{snippet.id}/raw/#{ref}/#{blob.path}") }
+        it { expect(gitlab_raw_snippet_blob_url(snippet, blob.path, ref)).to eq("http://test.host/#{snippet.project.full_path}/-/snippets/#{snippet.id}/raw/#{ref}/#{blob.path}") }
+      end
+
+      context 'without a ref' do
+        let(:snippet) { personal_snippet }
+        let(:ref) { snippet.repository.root_ref }
+
+        it 'uses the root ref' do
+          expect(gitlab_raw_snippet_blob_url(snippet, blob.path)).to eq("http://test.host/-/snippets/#{snippet.id}/raw/#{ref}/#{blob.path}")
+        end
       end
     end
 

spec/lib/api/entities/snippet_blob_spec.rb

-# frozen_string_literal: true
-
-require 'spec_helper'
-
-RSpec.describe ::API::Entities::SnippetBlob do
-  let_it_be(:personal_snippet) { create(:personal_snippet, :repository) }
-  let_it_be(:project_snippet)  { create(:project_snippet, :repository) }
-
-  let(:blob)   { snippet.blobs.first }
-  let(:entity) { described_class.new(blob) }
-  let(:ref)    { blob.repository.root_ref }
-
-  subject { entity.as_json }
-
-  context 'with PersonalSnippet blob' do
-    let(:snippet) { personal_snippet }
-
-    it { expect(subject[:path]).to eq blob.path }
-    it { expect(subject[:raw_url]).to match("/-/snippets/#{snippet.id}/raw/#{ref}/#{blob.path}") }
-  end
-
-  context 'with ProjectSnippet blob' do
-    let(:snippet) { project_snippet }
-
-    it { expect(subject[:path]).to eq blob.path }
-    it { expect(subject[:raw_url]).to match("#{snippet.project.full_path}/-/snippets/#{snippet.id}/raw/#{ref}/#{blob.path}") }
-  end
-end

spec/lib/api/entities/snippet_spec.rb

@@ -74,10 +74,8 @@ RSpec.describe ::API::Entities::Snippet do
     end
 
     describe 'files' do
-      it 'returns snippet blob data in files' do
-        expect(subject[:files].count).to eq snippet.blobs.count
-        expect(subject[:files].first[:path]).to eq snippet.blobs.first.path
-      end
+      let(:blob)   { snippet.blobs.first }
+      let(:ref)    { blob.repository.root_ref }
 
       context 'when repository does not exist' do
         it 'does not include the files attribute' do
@@ -86,6 +84,36 @@ RSpec.describe ::API::Entities::Snippet do
           expect(subject).not_to include(:files)
         end
       end
+
+      shared_examples 'snippet files' do
+        let(:file) { subject[:files].first }
+
+        it 'returns all snippet files' do
+          expect(subject[:files].count).to eq snippet.blobs.count
+        end
+
+        it 'has the file path' do
+          expect(file[:path]).to eq blob.path
+        end
+
+        it 'has the raw url' do
+          expect(file[:raw_url]).to match(raw_url)
+        end
+      end
+
+      context 'with PersonalSnippet' do
+        it_behaves_like 'snippet files' do
+          let(:snippet) { personal_snippet }
+          let(:raw_url) { "/-/snippets/#{snippet.id}/raw/#{ref}/#{blob.path}" }
+        end
+      end
+
+      context 'with ProjectSnippet' do
+        it_behaves_like 'snippet files' do
+          let(:snippet) { project_snippet }
+          let(:raw_url) { "#{snippet.project.full_path}/-/snippets/#{snippet.id}/raw/#{ref}/#{blob.path}" }
+        end
+      end
     end
   end
 

spec/lib/gitlab/url_builder_spec.rb

@@ -87,16 +87,6 @@ RSpec.describe Gitlab::UrlBuilder do
     end
 
     context 'when passing a Snippet' do
-      let(:snippet) { build_stubbed(:personal_snippet) }
-
-      it 'returns a raw snippet URL if requested' do
-        url = subject.build(snippet, raw: true)
-
-        expect(url).to eq "#{Gitlab.config.gitlab.url}/snippets/#{snippet.id}/raw"
-      end
-    end
-
-    context 'when passing a Snippet blob' do
       let_it_be(:personal_snippet) { create(:personal_snippet, :repository) }
       let_it_be(:project_snippet)  { create(:project_snippet, :repository) }
       let(:blob)                   { snippet.blobs.first }
@@ -105,8 +95,14 @@ RSpec.describe Gitlab::UrlBuilder do
       context 'for a PersonalSnippet' do
         let(:snippet) { personal_snippet }
 
-        it 'returns a raw snippet blob URL' do
-          url = subject.build(blob)
+        it 'returns a raw snippet URL if requested' do
+          url = subject.build(snippet, raw: true)
+
+          expect(url).to eq "#{Gitlab.config.gitlab.url}/snippets/#{snippet.id}/raw"
+        end
+
+        it 'returns a raw snippet blob URL if requested' do
+          url = subject.build(snippet, file: blob.path, ref: ref)
 
           expect(url).to eq "#{Gitlab.config.gitlab.url}/-/snippets/#{snippet.id}/raw/#{ref}/#{blob.path}"
         end
@@ -115,8 +111,14 @@ RSpec.describe Gitlab::UrlBuilder do
       context 'for a ProjectSnippet' do
         let(:snippet) { project_snippet }
 
-        it 'returns a raw snippet blob URL' do
-          url = subject.build(blob)
+        it 'returns a raw snippet URL if requested' do
+          url = subject.build(snippet, raw: true)
+
+          expect(url).to eq "#{Gitlab.config.gitlab.url}/#{snippet.project.full_path}/snippets/#{snippet.id}/raw"
+        end
+
+        it 'returns a raw snippet blob URL if requested' do
+          url = subject.build(snippet, file: blob.path, ref: ref)
 
           expect(url).to eq "#{Gitlab.config.gitlab.url}/#{snippet.project.full_path}/-/snippets/#{snippet.id}/raw/#{ref}/#{blob.path}"
         end

spec/models/snippet_spec.rb

@@ -762,4 +762,29 @@ RSpec.describe Snippet do
       end
     end
   end
+
+  describe '#list_files' do
+    let_it_be(:snippet) { create(:snippet, :repository) }
+    let(:ref) { 'test-ref' }
+
+    subject { snippet.list_files(ref) }
+
+    context 'when snippet has a repository' do
+      it 'lists files from the repository with the ref' do
+        expect(snippet.repository).to receive(:ls_files).with(ref)
+
+        subject
+      end
+    end
+
+    context 'when snippet does not have a repository' do
+      before do
+        allow(snippet.repository).to receive(:empty?).and_return(true)
+      end
+
+      it 'returns an empty array' do
+        expect(subject).to eq []
+      end
+    end
+  end
 end

spec/support/helpers/snippet_helpers.rb

@@ -8,7 +8,7 @@ module SnippetHelpers
   def snippet_blob_file(blob)
     {
       "path" => blob.path,
-      "raw_url" => gitlab_raw_snippet_blob_url(blob)
+      "raw_url" => gitlab_raw_snippet_blob_url(blob.container, blob.path)
     }
   end
 end