From 329a890f8baebb2da3b30d4b2fdfc0cc1f96b5e6 Mon Sep 17 00:00:00 2001
From: Tiago Botelho <tiagonbotelho@hotmail.com>
Date: Mon, 26 Nov 2018 15:44:13 +0000
Subject: [PATCH] Resolves N+1 RPC call for Project#readme_url

Caches repository.path into Repository#readme_path
---
 app/models/project.rb                         |  6 +--
 app/models/repository.rb                      |  9 ++++-
 ...51061-readme-url-n-1-rpc-call-resolved.yml |  5 +++
 spec/models/repository_spec.rb                | 40 ++++++++++++++++++-
 4 files changed, 54 insertions(+), 6 deletions(-)
 create mode 100644 changelogs/unreleased/51061-readme-url-n-1-rpc-call-resolved.yml

diff --git a/app/models/project.rb b/app/models/project.rb
index 4d1917b9ab2..39d5fdd7a7f 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -988,9 +988,9 @@ class Project < ActiveRecord::Base
   end
 
   def readme_url
-    readme = repository.readme
-    if readme
-      Gitlab::Routing.url_helpers.project_blob_url(self, File.join(default_branch, readme.path))
+    readme_path = repository.readme_path
+    if readme_path
+      Gitlab::Routing.url_helpers.project_blob_url(self, File.join(default_branch, readme_path))
     end
   end
 
diff --git a/app/models/repository.rb b/app/models/repository.rb
index fff6d4be275..76e10af51d6 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -35,7 +35,7 @@ class Repository
   #
   # For example, for entry `:commit_count` there's a method called `commit_count` which
   # stores its data in the `commit_count` cache key.
-  CACHED_METHODS = %i(size commit_count rendered_readme contribution_guide
+  CACHED_METHODS = %i(size commit_count rendered_readme readme_path contribution_guide
                       changelog license_blob license_key gitignore
                       gitlab_ci_yml branch_names tag_names branch_count
                       tag_count avatar exists? root_ref has_visible_content?
@@ -48,7 +48,7 @@ class Repository
   # changed. This Hash maps file types (as returned by Gitlab::FileDetector) to
   # the corresponding methods to call for refreshing caches.
   METHOD_CACHES_FOR_FILE_TYPES = {
-    readme: :rendered_readme,
+    readme: %i(rendered_readme readme_path),
     changelog: :changelog,
     license: %i(license_blob license_key license),
     contributing: :contribution_guide,
@@ -585,6 +585,11 @@ class Repository
     head_tree&.readme
   end
 
+  def readme_path
+    readme&.path
+  end
+  cache_method :readme_path
+
   def rendered_readme
     return unless readme
 
diff --git a/changelogs/unreleased/51061-readme-url-n-1-rpc-call-resolved.yml b/changelogs/unreleased/51061-readme-url-n-1-rpc-call-resolved.yml
new file mode 100644
index 00000000000..86f91fcb427
--- /dev/null
+++ b/changelogs/unreleased/51061-readme-url-n-1-rpc-call-resolved.yml
@@ -0,0 +1,5 @@
+---
+title: Improves performance of Project#readme_url by caching the README path
+merge_request: 23357
+author:
+type: performance
diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb
index 187283b284b..f09b4b67061 100644
--- a/spec/models/repository_spec.rb
+++ b/spec/models/repository_spec.rb
@@ -1488,6 +1488,7 @@ describe Repository do
         :size,
         :commit_count,
         :rendered_readme,
+        :readme_path,
         :contribution_guide,
         :changelog,
         :license_blob,
@@ -1874,6 +1875,42 @@ describe Repository do
     end
   end
 
+  describe '#readme_path', :use_clean_rails_memory_store_caching do
+    context 'with a non-existing repository' do
+      let(:project) { create(:project) }
+
+      it 'returns nil' do
+        expect(repository.readme_path).to be_nil
+      end
+    end
+
+    context 'with an existing repository' do
+      context 'when no README exists' do
+        let(:project) { create(:project, :empty_repo) }
+
+        it 'returns nil' do
+          expect(repository.readme_path).to be_nil
+        end
+      end
+
+      context 'when a README exists' do
+        let(:project) { create(:project, :repository) }
+
+        it 'returns the README' do
+          expect(repository.readme_path).to eq("README.md")
+        end
+
+        it 'caches the response' do
+          expect(repository).to receive(:readme).and_call_original.once
+
+          2.times do
+            expect(repository.readme_path).to eq("README.md")
+          end
+        end
+      end
+    end
+  end
+
   describe '#expire_statistics_caches' do
     it 'expires the caches' do
       expect(repository).to receive(:expire_method_caches)
@@ -2042,9 +2079,10 @@ describe Repository do
   describe '#refresh_method_caches' do
     it 'refreshes the caches of the given types' do
       expect(repository).to receive(:expire_method_caches)
-        .with(%i(rendered_readme license_blob license_key license))
+        .with(%i(rendered_readme readme_path license_blob license_key license))
 
       expect(repository).to receive(:rendered_readme)
+      expect(repository).to receive(:readme_path)
       expect(repository).to receive(:license_blob)
       expect(repository).to receive(:license_key)
       expect(repository).to receive(:license)