Merge branch 'fix_es_for_non-default-branches' into 'master'

Fix ES for for non-default branches

Closes #1349

See merge request !999

app/views/search/results/_blob.html.haml

@@ -6,8 +6,7 @@
   - blob = parse_search_result(blob)
   - file_name = blob.filename
 
-- ref = blob.ref
-- blob_link = namespace_project_blob_path(project.namespace, project, tree_join(ref, file_name))
+- blob_link = namespace_project_blob_path(project.namespace, project, tree_join(blob.ref, file_name))
 .blob-result
   .file-holder
     .file-title
@@ -19,6 +18,6 @@
           - else
             #{project.name_with_namespace}:
             %i= file_name
-    - if blob
+    - if blob.data
       .file-content.code.term
         = render 'shared/file_highlight', blob: blob, first_line_number: blob.startline, blob_link: blob_link

changelogs/unreleased-ee/fix_es_for_non-default-branches.yml

+---
+title: Fix ES search for non-default branches
+merge_request:
+author:

lib/gitlab/elastic/project_search_results.rb

@@ -9,12 +9,7 @@ module Gitlab
       def initialize(current_user, query, project_id, repository_ref = nil)
         @current_user = current_user
         @project = Project.find(project_id)
-
-        @repository_ref = if repository_ref.present?
-                            repository_ref
-                          else
-                            nil
-                          end
+        @repository_ref = repository_ref.presence || project.default_branch
         @query = query
         @public_and_internal_projects = false
       end
@@ -65,7 +60,7 @@ module Gitlab
             )[:blobs][:results].response
           else
             Kaminari.paginate_array(
-              project.repository.search_files(query, repository_ref)
+              Gitlab::FileFinder.new(project, repository_ref).find(query)
             )
           end
         end
@@ -105,10 +100,12 @@ module Gitlab
               per_page: per_page
             )
           else
+            offset = per_page * ((page || 1) - 1)
+
             Kaminari.paginate_array(
-              project.repository.find_commits_by_message(query).compact,
-              page: (page || 1).to_i,
-              per_page: per_page
+              project.repository.find_commits_by_message(query),
+              offset: offset,
+              limit: per_page
             )
           end
         end
@@ -119,7 +116,7 @@ module Gitlab
       end
 
       def root_ref?
-        !repository_ref || project.root_ref?(repository_ref)
+        project.root_ref?(repository_ref)
       end
     end
   end

lib/gitlab/file_finder.rb

+# This class finds files in a repository by name and content
+# the result is joined and sorted by file name
+module Gitlab
+  class FileFinder
+    BATCH_SIZE = 100
+
+    attr_reader :project, :ref
+
+    def initialize(project, ref)
+      @project = project
+      @ref = ref
+    end
+
+    def find(query)
+      blobs = project.repository.search_files_by_content(query, ref).first(BATCH_SIZE)
+      found_file_names = Set.new
+
+      results = blobs.map do |blob|
+        blob = Gitlab::ProjectSearchResults.parse_search_result(blob)
+        found_file_names << blob.filename
+
+        [blob.filename, blob]
+      end
+
+      project.repository.search_files_by_name(query, ref).first(BATCH_SIZE).each do |filename|
+        results << [filename, OpenStruct.new(ref: ref)] unless found_file_names.include?(filename)
+      end
+
+      results.sort_by(&:first)
+    end
+  end
+end

lib/gitlab/project_search_results.rb

@@ -74,23 +74,7 @@ module Gitlab
     private
 
     def blobs
-      @blobs ||= begin
-        blobs = project.repository.search_files_by_content(query, repository_ref).first(100)
-        found_file_names = Set.new
-
-        results = blobs.map do |blob|
-          blob = self.class.parse_search_result(blob)
-          found_file_names << blob.filename
-
-          [blob.filename, blob]
-        end
-
-        project.repository.search_files_by_name(query, repository_ref).first(100).each do |filename|
-          results << [filename, nil] unless found_file_names.include?(filename)
-        end
-
-        results.sort_by(&:first)
-      end
+      @blobs ||= Gitlab::FileFinder.new(project, repository_ref).find(query)
     end
 
     def wiki_blobs

spec/lib/gitlab/elastic/project_search_results_spec.rb

@@ -19,7 +19,7 @@ describe Gitlab::Elastic::ProjectSearchResults, lib: true do
     subject(:results) { described_class.new(user, query, project.id, '') }
 
     it { expect(results.project).to eq(project) }
-    it { expect(results.repository_ref).to be_nil }
+    it { expect(results.repository_ref).to eq('master') }
     it { expect(results.query).to eq('hello world') }
   end
 
@@ -46,30 +46,49 @@ describe Gitlab::Elastic::ProjectSearchResults, lib: true do
       create :note, note: 'bla-bla term', project: project1
 
       # Wiki
-      project.wiki.create_page("index_page", "term")
+      project.wiki.create_page('index_page', 'term')
       project.wiki.index_blobs
-      project1.wiki.create_page("index_page", " term")
+      project1.wiki.create_page('index_page', ' term')
       project1.wiki.index_blobs
 
       Gitlab::Elastic::Helper.refresh_index
 
-      result = Gitlab::Elastic::ProjectSearchResults.new(user, "term", project.id)
+      result = Gitlab::Elastic::ProjectSearchResults.new(user, 'term', project.id)
       expect(result.notes_count).to eq(1)
       expect(result.wiki_blobs_count).to eq(1)
       expect(result.blobs_count).to eq(1)
 
-      result1 = Gitlab::Elastic::ProjectSearchResults.new(user, "initial", project.id)
+      result1 = Gitlab::Elastic::ProjectSearchResults.new(user, 'initial', project.id)
       expect(result1.commits_count).to eq(1)
     end
   end
 
   describe "search for commits in non-default branch" do
-    it "finds needed commit" do
+    it 'finds needed commit' do
       project = create :project
 
-      result = Gitlab::Elastic::ProjectSearchResults.new(user, "initial", project.id, 'test')
+      result = Gitlab::Elastic::ProjectSearchResults.new(user, 'initial', project.id, 'test')
       expect(result.commits_count).to eq(1)
     end
+
+    it 'responds to total_pages method' do
+      project = create :project
+
+      result = Gitlab::Elastic::ProjectSearchResults.new(user, 'initial', project.id, 'test')
+      expect(result.objects('commits').total_pages).to eq(1)
+    end
+  end
+
+  describe 'search for blobs in non-default branch' do
+    it 'users FileFinder instead of ES search' do
+      project = create :project
+
+      expect_any_instance_of(Gitlab::FileFinder).to receive(:find).with('initial').and_return([])
+
+      result = Gitlab::Elastic::ProjectSearchResults.new(user, 'initial', project.id, 'test')
+
+      result.blobs_count
+    end
   end
 
   describe 'confidential issues' do

spec/lib/gitlab/file_finder_spec.rb

+require 'spec_helper'
+
+describe Gitlab::FileFinder do
+  let(:project) { create :project }
+  let(:finder) { described_class.new(project, 'master') }
+
+  it 'finds files by name' do
+    filename, blob = finder.find('CHANGELOG').first
+
+    expect(filename).to eq('CHANGELOG')
+    expect(blob.ref).to eq('master')
+  end
+
+  it 'finds files by content' do
+    filename, blob = finder.find('CHANGELOG').last
+
+    expect(filename).to eq('CONTRIBUTING.md')
+    expect(blob.filename).to eq('CONTRIBUTING.md')
+    expect(blob.startline).to be_a(Integer)
+    expect(blob.data).to include('CHANGELOG')
+  end
+end

spec/lib/gitlab/project_search_results_spec.rb

@@ -25,11 +25,12 @@ describe Gitlab::ProjectSearchResults, lib: true do
     let(:results) { described_class.new(user, project, 'files').objects('blobs') }
 
     it 'finds by name' do
-      expect(results).to include(["files/images/wm.svg", nil])
+      blob = results.select { |result| result.first == 'files/images/wm.svg' }.flatten.last
+      expect(blob).to be_a(OpenStruct)
     end
 
     it 'finds by content' do
-      blob = results.select { |result| result.first == "CHANGELOG" }.flatten.last
+      blob = results.select { |result| result.first == 'CHANGELOG' }.flatten.last
 
       expect(blob.filename).to eq("CHANGELOG")
     end