Commit 353c6895 authored by Yorick Peterse's avatar Yorick Peterse

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
parent d43ac640
...@@ -5,6 +5,10 @@ class Feature ...@@ -5,6 +5,10 @@ class Feature
class FlipperFeature < Flipper::Adapters::ActiveRecord::Feature class FlipperFeature < Flipper::Adapters::ActiveRecord::Feature
# Using `self.table_name` won't work. ActiveRecord bug? # Using `self.table_name` won't work. ActiveRecord bug?
superclass.table_name = 'features' superclass.table_name = 'features'
def self.feature_names
pluck(:key)
end
end end
class FlipperGate < Flipper::Adapters::ActiveRecord::Gate class FlipperGate < Flipper::Adapters::ActiveRecord::Gate
...@@ -22,11 +26,19 @@ class Feature ...@@ -22,11 +26,19 @@ class Feature
flipper.feature(key) flipper.feature(key)
end end
def persisted_names
if RequestStore.active?
RequestStore[:flipper_persisted_names] ||= FlipperFeature.feature_names
else
FlipperFeature.feature_names
end
end
def persisted?(feature) def persisted?(feature)
# Flipper creates on-memory features when asked for a not-yet-created one. # 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 # If we want to check if a feature has been actually set, we look for it
# on the persisted features list. # on the persisted features list.
all.map(&:name).include?(feature.name) persisted_names.include?(feature.name)
end end
def enabled?(key, thing = nil) def enabled?(key, thing = nil)
......
...@@ -13,6 +13,47 @@ describe Feature do ...@@ -13,6 +13,47 @@ describe Feature do
end end
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 describe '.all' do
let(:features) { Set.new } let(:features) { Set.new }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment