From 90be53c5d39bff5e371cf8a6e11a39bf5dad7bcc Mon Sep 17 00:00:00 2001
From: Yorick Peterse <yorickpeterse@gmail.com>
Date: Mon, 30 Oct 2017 16:10:31 +0100
Subject: [PATCH] Cache feature names in RequestStore

The GitHub importer (and probably other parts of our code) ends up
calling Feature.persisted? many times (via Gitaly). By storing this data
in RequestStore we can save ourselves _a lot_ of database queries.

Fixes https://gitlab.com/gitlab-org/gitlab-ce/issues/39361
---
 lib/feature.rb           | 14 +++++++++++++-
 spec/lib/feature_spec.rb | 41 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 54 insertions(+), 1 deletion(-)

diff --git a/lib/feature.rb b/lib/feature.rb
index 4bd29aed687..ac3bc65c0d5 100644
--- a/lib/feature.rb
+++ b/lib/feature.rb
@@ -5,6 +5,10 @@ class Feature
   class FlipperFeature < Flipper::Adapters::ActiveRecord::Feature
     # Using `self.table_name` won't work. ActiveRecord bug?
     superclass.table_name = 'features'
+
+    def self.feature_names
+      pluck(:key)
+    end
   end
 
   class FlipperGate < Flipper::Adapters::ActiveRecord::Gate
@@ -22,11 +26,19 @@ class Feature
       flipper.feature(key)
     end
 
+    def persisted_names
+      if RequestStore.active?
+        RequestStore[:flipper_persisted_names] ||= FlipperFeature.feature_names
+      else
+        FlipperFeature.feature_names
+      end
+    end
+
     def persisted?(feature)
       # Flipper creates on-memory features when asked for a not-yet-created one.
       # If we want to check if a feature has been actually set, we look for it
       # on the persisted features list.
-      all.map(&:name).include?(feature.name)
+      persisted_names.include?(feature.name)
     end
 
     def enabled?(key, thing = nil)
diff --git a/spec/lib/feature_spec.rb b/spec/lib/feature_spec.rb
index 1076c63b5f2..10020511bf8 100644
--- a/spec/lib/feature_spec.rb
+++ b/spec/lib/feature_spec.rb
@@ -13,6 +13,47 @@ describe Feature do
     end
   end
 
+  describe '.persisted_names' do
+    it 'returns the names of the persisted features' do
+      Feature::FlipperFeature.create!(key: 'foo')
+
+      expect(described_class.persisted_names).to eq(%w[foo])
+    end
+
+    it 'returns an empty Array when no features are presisted' do
+      expect(described_class.persisted_names).to be_empty
+    end
+
+    it 'caches the feature names when request store is active', :request_store do
+      Feature::FlipperFeature.create!(key: 'foo')
+
+      expect(Feature::FlipperFeature)
+        .to receive(:feature_names)
+        .once
+        .and_call_original
+
+      2.times do
+        expect(described_class.persisted_names).to eq(%w[foo])
+      end
+    end
+  end
+
+  describe '.persisted?' do
+    it 'returns true for a persisted feature' do
+      Feature::FlipperFeature.create!(key: 'foo')
+
+      feature = double(:feature, name: 'foo')
+
+      expect(described_class.persisted?(feature)).to eq(true)
+    end
+
+    it 'returns false for a feature that is not persisted' do
+      feature = double(:feature, name: 'foo')
+
+      expect(described_class.persisted?(feature)).to eq(false)
+    end
+  end
+
   describe '.all' do
     let(:features) { Set.new }