From 53801b1206ff9e165b30117e3c3bcf543db7ad2c Mon Sep 17 00:00:00 2001
From: Andreas Brandl <abrandl@gitlab.com>
Date: Tue, 27 Aug 2019 19:15:28 +0200
Subject: [PATCH] Preload routes information

This fixes a high frequency N+1 issue:

`RoutableActions#find_routable!` is used across many controllers to
retrieve e.g. the Project or Namespace by path. The `#find_routable!`
method calls `#ensure_canonical_path` which in turn retrieves
`#full_path` from the given Routable.

This in turn triggers a lookup on `routes`, leading to a high frequency
of these queries:

```sql
SELECT  "routes".* FROM "routes" WHERE "routes"."source_id" = $1 AND
"routes"."source_type" = $2 LIMIT $3
```

This is unnecessary as we already join `routes` in
`Routable#find_by_full_path` anyways.
---
 app/controllers/admin/groups_controller.rb   | 6 +++++-
 app/models/concerns/routable.rb              | 4 ++--
 changelogs/unreleased/ab-routable-nplus1.yml | 5 +++++
 spec/models/concerns/routable_spec.rb        | 7 +++++++
 4 files changed, 19 insertions(+), 3 deletions(-)
 create mode 100644 changelogs/unreleased/ab-routable-nplus1.yml

diff --git a/app/controllers/admin/groups_controller.rb b/app/controllers/admin/groups_controller.rb
index 6317fa7c8d1..32a36da56fe 100644
--- a/app/controllers/admin/groups_controller.rb
+++ b/app/controllers/admin/groups_controller.rb
@@ -14,7 +14,11 @@ class Admin::GroupsController < Admin::ApplicationController
 
   # rubocop: disable CodeReuse/ActiveRecord
   def show
-    @group = Group.with_statistics.joins(:route).group('routes.path').find_by_full_path(params[:id])
+    # Group.with_statistics doesn't behave nicely when including other relations.
+    # Group.find_by_full_path includes the routes relation to avoid a common N+1
+    # (at the expense of this action: there are two queries here to find and retrieve
+    # the Group with statistics).
+    @group = Group.with_statistics.find(group&.id)
     @members = present_members(
       @group.members.order("access_level DESC").page(params[:members_page]))
     @requesters = present_members(
diff --git a/app/models/concerns/routable.rb b/app/models/concerns/routable.rb
index 3a486632800..07d22641a0a 100644
--- a/app/models/concerns/routable.rb
+++ b/app/models/concerns/routable.rb
@@ -38,7 +38,7 @@ module Routable
       if Feature.enabled?(:routable_two_step_lookup)
         # Case sensitive match first (it's cheaper and the usual case)
         # If we didn't have an exact match, we perform a case insensitive search
-        found = joins(:route).find_by(routes: { path: path }) || where_full_path_in([path]).take
+        found = includes(:route).find_by(routes: { path: path }) || where_full_path_in([path]).take
       else
         order_sql = Arel.sql("(CASE WHEN routes.path = #{connection.quote(path)} THEN 0 ELSE 1 END)")
         found = where_full_path_in([path]).reorder(order_sql).take
@@ -67,7 +67,7 @@ module Routable
         "(LOWER(routes.path) = LOWER(#{connection.quote(path)}))"
       end
 
-      joins(:route).where(wheres.join(' OR '))
+      includes(:route).where(wheres.join(' OR ')).references(:routes)
     end
 
     # Temporary instrumentation of method calls
diff --git a/changelogs/unreleased/ab-routable-nplus1.yml b/changelogs/unreleased/ab-routable-nplus1.yml
new file mode 100644
index 00000000000..7b59bc78dae
--- /dev/null
+++ b/changelogs/unreleased/ab-routable-nplus1.yml
@@ -0,0 +1,5 @@
+---
+title: Preload routes information to fix N+1 issue
+merge_request: 32352
+author:
+type: performance
diff --git a/spec/models/concerns/routable_spec.rb b/spec/models/concerns/routable_spec.rb
index cff86afe768..cad705ee594 100644
--- a/spec/models/concerns/routable_spec.rb
+++ b/spec/models/concerns/routable_spec.rb
@@ -66,6 +66,13 @@ describe Group, 'Routable' do
       it { expect(described_class.find_by_full_path(group.to_param.upcase)).to eq(group) }
       it { expect(described_class.find_by_full_path(nested_group.to_param)).to eq(nested_group) }
       it { expect(described_class.find_by_full_path('unknown')).to eq(nil) }
+
+      it 'includes route information when loading a record' do
+        path = group.to_param
+        control_count = ActiveRecord::QueryRecorder.new { described_class.find_by_full_path(path) }.count
+
+        expect { described_class.find_by_full_path(path).route }.not_to exceed_all_query_limit(control_count)
+      end
     end
 
     context 'with redirect routes' do