Fix handling of default_enabledsh-fix-feature-flags

2 files changed, 53 insertions, 12 deletions

diff --git a/lib/feature.rb b/lib/feature.rb
index c98ffbd9959..f23c316f266 100644
--- a/lib/feature.rb
+++ b/lib/feature.rb
@@ -44,6 +44,7 @@ class Feature
 
     def expire_persisted_names
       l1_cache_backend.delete(PERSISTED_NAMES_CACHE_KEY)
+      Gitlab::SafeRequestStore[:flipper_persisted_names] = nil
     end
 
     def persisted?(feature)
@@ -58,11 +59,28 @@ class Feature
     def enabled?(key, thing = nil, default_enabled: false)
       feature = Feature.get(key)
 
-      # If we're not default enabling the flag or the feature has been set, always evaluate.
-      # `persisted?` can potentially generate DB queries and also checks for inclusion
-      # in an array of feature names (177 at last count), possibly reducing performance by half.
-      # So we only perform the `persisted` check if `default_enabled: true`
-      !default_enabled || Feature.persisted?(feature) ? feature.enabled?(thing) : true
+      return default_enabled unless Feature.persisted?(feature)
+
+      # If this feature were enabled for any actor at some point, then
+      # a gate will exist, and we should check with Flipper whether
+      # the feature is enabled. Otherwise, we should rely on the
+      # default_enabled flag.
+      #
+      # Consider this example:
+      #
+      # Feature.enable(feature, project) # This opens the gate for a project
+      #
+      # Feature.enabled?(feature, project, default_enabled: true) # => true
+      # Feature.enabled?(feature, default_enabled: true) # => false
+      #
+      # The last call doesn't have an associated actor, but it does have
+      # the default_enabled flag on. Flipper will assume the feature is off,
+      # but we need to check the default_enabled value to be sure.
+      if thing
+        feature.enabled?(thing)
+      else
+        default_enabled || feature.enabled?(thing)
+      end
     end
 
     def disabled?(key, thing = nil, default_enabled: false)
@@ -71,15 +89,15 @@ class Feature
     end
 
     def enable(key, thing = true)
-      expire_persisted_names
-
-      get(key).enable(thing)
+      get(key).enable(thing).tap do
+        expire_persisted_names
+      end
     end
 
     def disable(key, thing = false)
-      expire_persisted_names
-
-      get(key).disable(thing)
+      get(key).disable(thing).tap do
+        expire_persisted_names
+      end
     end
 
     def enable_group(key, group)
diff --git a/spec/lib/feature_spec.rb b/spec/lib/feature_spec.rb
index bf92a03b898..0271b8e38b8 100644
--- a/spec/lib/feature_spec.rb
+++ b/spec/lib/feature_spec.rb
@@ -167,6 +167,7 @@ describe Feature do
       described_class.enable(:enabled_feature_flag)
       flipper_key = "flipper/v1/feature/enabled_feature_flag"
 
+      expect(described_class.l1_cache_backend).to receive(:fetch).with('flipper:persisted_names', expires_in: 1.minute).and_call_original
       expect(described_class.l2_cache_backend)
         .to receive(:fetch)
         .once
@@ -184,11 +185,33 @@ describe Feature do
       end
     end
 
+    context 'with actor', :request_store do
+      let(:flag) { :actor_flag }
+      let(:actor) { create(:project) }
+
+      it 'persisted feature prioritizes gate state over default_enabled' do
+        described_class.enable(flag, actor)
+
+        expect(described_class.enabled?(flag, actor, default_enabled: true)).to be_truthy
+        expect(described_class.enabled?(flag, actor, default_enabled: false)).to be_truthy
+        expect(described_class.enabled?(flag, default_enabled: true)).to be_truthy
+        expect(described_class.enabled?(flag, default_enabled: false)).to be_falsey
+      end
+
+      it 'unpersisted feature respects default_enabled' do
+        expect(described_class.enabled?(flag, actor, default_enabled: true)).to be_truthy
+        expect(described_class.enabled?(flag, actor, default_enabled: false)).to be_falsey
+        expect(described_class.enabled?(flag, default_enabled: true)).to be_truthy
+        expect(described_class.enabled?(flag, default_enabled: false)).to be_falsey
+      end
+    end
+
     context 'cached feature flag', :request_store do
       let(:flag) { :some_feature_flag }
 
       before do
         described_class.flipper.memoize = false
+        described_class.enable(flag)
         described_class.enabled?(flag)
       end
 
@@ -206,7 +229,7 @@ describe Feature do
             expect(described_class.l1_cache_backend).to receive(:fetch).once.and_call_original
             expect(described_class.l2_cache_backend).to receive(:fetch).once.and_call_original
             expect(described_class.enabled?(flag)).to be_truthy
-          end.not_to exceed_query_limit(0)
+          end.not_to exceed_query_limit(1) # One query for persisted_names
         end
       end