From a0edaa9210642f23ef3ea4984c6d6f77cbbba878 Mon Sep 17 00:00:00 2001
From: Stan Hu <stanhu@gmail.com>
Date: Sun, 23 Apr 2017 22:32:50 -0700
Subject: Cache Routable#full_path in RequestStore to reduce duplicate route
 loads

We see in #27387 that a call to `polymorphic_path` will cause duplicate
SELECT route calls for each merge request in a milestone. This happens
because calling `project.namespace.becomes(Namespace)` will instantiate
a new instance of a Namespace for each merge request, which causes a N+1
query on the routes table. This change caches the state of the route by
the specific class and ID, which dramatically eliminates duplicate work.
---
 app/models/concerns/routable.rb                        | 15 +++++++++++++--
 .../sh-optimize-duplicate-routable-full-path.yml       |  4 ++++
 spec/models/concerns/routable_spec.rb                  | 18 ++++++++++++++++++
 3 files changed, 35 insertions(+), 2 deletions(-)
 create mode 100644 changelogs/unreleased/sh-optimize-duplicate-routable-full-path.yml

diff --git a/app/models/concerns/routable.rb b/app/models/concerns/routable.rb
index aca99feee53..b28e05d0c28 100644
--- a/app/models/concerns/routable.rb
+++ b/app/models/concerns/routable.rb
@@ -163,7 +163,20 @@ module Routable
     end
   end
 
+  # Every time `project.namespace.becomes(Namespace)` is called for polymorphic_path,
+  # a new instance is instantiated, and we end up duplicating the same query to retrieve
+  # the route. Caching this per request ensures that even if we have multiple instances,
+  # we will not have to duplicate work, avoiding N+1 queries in some cases.
   def full_path
+    return uncached_full_path unless RequestStore.active?
+
+    key = "routable/full_path/#{self.class.name}/#{self.id}"
+    RequestStore[key] ||= uncached_full_path
+  end
+
+  private
+
+  def uncached_full_path
     if route && route.path.present?
       @full_path ||= route.path
     else
@@ -173,8 +186,6 @@ module Routable
     end
   end
 
-  private
-
   def full_name_changed?
     name_changed? || parent_changed?
   end
diff --git a/changelogs/unreleased/sh-optimize-duplicate-routable-full-path.yml b/changelogs/unreleased/sh-optimize-duplicate-routable-full-path.yml
new file mode 100644
index 00000000000..b1ef00f09b2
--- /dev/null
+++ b/changelogs/unreleased/sh-optimize-duplicate-routable-full-path.yml
@@ -0,0 +1,4 @@
+---
+title: Cache Routable#full_path in RequestStore to reduce duplicate route loads
+merge_request:
+author:
diff --git a/spec/models/concerns/routable_spec.rb b/spec/models/concerns/routable_spec.rb
index f191605dbdb..221647d7a48 100644
--- a/spec/models/concerns/routable_spec.rb
+++ b/spec/models/concerns/routable_spec.rb
@@ -194,6 +194,24 @@ describe Group, 'Routable' do
 
     it { expect(group.full_path).to eq(group.path) }
     it { expect(nested_group.full_path).to eq("#{group.full_path}/#{nested_group.path}") }
+
+    context 'with RequestStore active' do
+      before do
+        RequestStore.begin!
+      end
+
+      after do
+        RequestStore.end!
+        RequestStore.clear!
+      end
+
+      it 'does not load the route table more than once' do
+        expect(group).to receive(:uncached_full_path).once.and_call_original
+
+        3.times { group.full_path }
+        expect(group.full_path).to eq(group.path)
+      end
+    end
   end
 
   describe '#full_name' do
-- 
cgit v1.2.1