Add empty group detection and pruning to Expression, guard enables

lib/flipper/adapters/sync/feature_synchronizer.rb

@@ -58,7 +58,9 @@ def sync_expression
           if remote_expression.nil?
             @feature.disable_expression
           else
-            @feature.enable_expression remote_expression
+            # Remote adapters can hold legacy empty-group expressions, which
+            # must be mirrored faithfully without weakening public enables.
+            @feature.__send__ :enable_expression_from_sync, remote_expression
           end
         end
 

lib/flipper/expression.rb

@@ -5,6 +5,9 @@ module Flipper
   class Expression
     include Builder
 
+    PruneResult = Struct.new(:expression, :constant_value, keyword_init: true)
+    private_constant :PruneResult
+
     def self.build(object)
       return object if object.is_a?(self) || object.is_a?(Constant)
 
@@ -50,12 +53,93 @@ def eql?(other)
     end
     alias_method :==, :eql?
 
+    # Public: Returns true if this expression contains an Any/All group with
+    # no arguments anywhere in its tree. An empty All evaluates to true for
+    # every actor and an empty Any to false for every actor, so persisting
+    # one is almost always a mistake.
+    def empty_groups?
+      return true if group? && args.empty?
+
+      args.any? { |arg| arg.is_a?(Expression) && arg.empty_groups? }
+    end
+
+    # Public: Returns a copy of this expression with empty Any/All groups
+    # removed wherever removal cannot broaden the expression, or nil when no
+    # conditions remain. An empty All can be removed from an All parent (it
+    # contributes a constant true) and an empty Any from an Any parent (a
+    # constant false), but an empty Any inside an All is kept because
+    # removing it would broaden the expression from never-true.
+    def prune_empty_groups
+      prune_empty_groups_with_identity.expression
+    end
+
+    # Public: Returns true when this expression is guaranteed to match every
+    # actor, e.g. an empty All group reaches an Any group or the root.
+    def always_true?
+      constant_group_value == true
+    end
+
     def value
       {
         name => args.map(&:value)
       }
     end
 
+    protected
+
+    def prune_empty_groups_with_identity
+      return PruneResult.new(expression: self) unless group?
+      return PruneResult.new(constant_value: all?) if args.empty?
+
+      pruned_args = args.map do |arg|
+        if arg.is_a?(Expression)
+          arg.prune_empty_groups_with_identity
+        else
+          PruneResult.new(expression: arg)
+        end
+      end
+
+      kept = pruned_args.map do |result|
+        if result.constant_value == false
+          result.expression || build("Any" => []) if all?
+        elsif result.constant_value.nil?
+          result.expression
+        end
+      end.compact
+
+      # No condition remains. Preserve this group's constant value for an
+      # enclosing group, which lets an empty All disappear from All without
+      # treating it as the unsafe empty-Any-in-All case.
+      if kept.empty?
+        return PruneResult.new(constant_value: constant_group_value)
+      end
+
+      pruned = build(name => kept)
+      PruneResult.new(
+        expression: pruned,
+        constant_value: pruned.constant_group_value
+      )
+    end
+
+    def constant_group_value
+      return nil unless group?
+      return all? if args.empty?
+
+      values = args.map do |arg|
+        arg.is_a?(Expression) ? arg.constant_group_value : nil
+      end
+
+      if all?
+        return false if values.include?(false)
+        return true if values.all?(true)
+      else
+        return true if values.include?(true)
+        return false if values.all?(false)
+      end
+
+      nil
+    end
+
     private
 
     def call_with_context?

lib/flipper/feature.rb

@@ -40,16 +40,7 @@ def initialize(name, adapter, options = {})
     #
     # Returns the result of Adapter#enable.
     def enable(thing = true)
-      instrument(:enable) do |payload|
-        adapter.add self
-
-        gate = gate_for(thing)
-        wrapped_thing = gate.wrap(thing)
-        payload[:gate_name] = gate.name
-        payload[:thing] = wrapped_thing
-
-        adapter.enable self, gate, wrapped_thing
-      end
+      enable_thing(thing, validate_expression: true)
     end
 
     # Public: Disable this feature for something.
@@ -124,6 +115,8 @@ def enabled?(*actors)
     # expression - an Expression or Hash that can be converted to an expression.
     #
     # Returns result of enable.
+    # Raises ArgumentError if the expression contains empty Any/All groups,
+    # because an empty All matches every actor.
     def enable_expression(expression)
       enable Expression.build(expression)
     end
@@ -133,12 +126,18 @@ def enable_expression(expression)
     # expression_to_add - an expression or Hash that can be converted to an expression.
     #
     # Returns result of enable.
+    # Raises ArgumentError if the resulting expression contains empty Any/All
+    # groups, because an empty All matches every actor.
     def add_expression(expression_to_add)
-      if (current_expression = expression)
-        enable current_expression.add(expression_to_add)
+      expression_to_add = Expression.build(expression_to_add)
+
+      new_expression = if (current_expression = expression)
+        current_expression.add(expression_to_add)
       else
-        enable expression_to_add
+        expression_to_add
       end
+
+      enable new_expression
     end
 
     # Public: Enables a feature for an actor.
@@ -191,14 +190,27 @@ def disable_expression
     end
 
     # Public: Remove an expression from a feature. Does nothing if no expression is
-    # currently enabled.
+    # currently enabled. Removing the last condition disables the expression
+    # rather than persisting an empty group.
     #
     # expression - an Expression or Hash that can be converted to an expression.
     #
-    # Returns result of enable or nil (if no expression enabled).
+    # Returns result of enable or disable or nil (if no expression enabled).
     def remove_expression(expression_to_remove)
       if (current_expression = expression)
-        enable current_expression.remove(expression_to_remove)
+        remaining = current_expression
+          .remove(Expression.build(expression_to_remove))
+          .prune_empty_groups
+
+        if remaining.nil?
+          disable_expression
+        else
+          # remove and prune only delete nodes, so any empty group left in
+          # remaining was already stored (e.g. mirrored by sync). Persist it
+          # unvalidated: rejecting it here would block removing unrelated
+          # conditions from legacy data.
+          enable_thing(remaining, validate_expression: false)
+        end
       end
     end
 
@@ -427,6 +439,42 @@ def gate_for(actor)
 
     private
 
+    # Internal: Mirrors a trusted remote expression during adapter sync,
+    # including legacy expressions that predate empty-group validation.
+    #
+    # Returns the result of Adapter#enable.
+    def enable_expression_from_sync(expression)
+      enable_thing(Expression.build(expression), validate_expression: false)
+    end
+
+    def enable_thing(thing, validate_expression:)
+      instrument(:enable) do |payload|
+        gate = gate_for(thing)
+        wrapped_thing = gate.wrap(thing)
+        validate_expression!(wrapped_thing) if validate_expression
+
+        adapter.add self
+
+        payload[:gate_name] = gate.name
+        payload[:thing] = wrapped_thing
+
+        adapter.enable self, gate, wrapped_thing
+      end
+    end
+
+    # Private: Raises if an expression about to be enabled contains empty
+    # Any/All groups. An empty All evaluates to true for every actor, so
+    # persisting one silently enables the feature for everyone.
+    #
+    # Returns the expression.
+    def validate_expression!(expression)
+      if expression.is_a?(Expression) && expression.empty_groups?
+        raise ArgumentError, "#{expression.value.inspect} contains empty Any/All groups and cannot be enabled (an empty All matches every actor, an empty Any matches none). Remove the empty groups or add conditions to them."
+      end
+
+      expression
+    end
+
     # Private: Wrap the actors passed to enabled? in a single pass to avoid
     # intermediate array allocations on the hot path. Arrays are flattened one
     # level; the explicit is_a?(Array) check (instead of flatten) avoids the

spec/flipper/adapters/active_record_spec.rb

@@ -1,4 +1,5 @@
 SpecHelpers.silence { require 'flipper/adapters/active_record' }
+require 'active_record/connection_adapters/sqlite3_adapter'
 
 # Turn off migration logging for specs
 ActiveRecord::Migration.verbose = false

spec/flipper/adapters/sync/feature_synchronizer_spec.rb

@@ -94,6 +94,17 @@
       expect_only_enable
     end
 
+    it "mirrors a remote expression containing empty groups without raising" do
+      remote = Flipper::GateValues.new(expression: {"Any" => []})
+      feature.enable_expression(plan_expression)
+      adapter.reset
+
+      described_class.new(feature, feature.gate_values, remote).call
+
+      expect(feature.expression_value).to eq({"Any" => []})
+      expect_only_enable
+    end
+
     it "does nothing to expression if in sync" do
       remote = Flipper::GateValues.new(expression: plan_expression.value)
       feature.enable_expression(plan_expression)

spec/flipper/api/v1/actions/expression_gate_spec.rb

@@ -121,6 +121,32 @@
     end
   end
 
+  describe 'enable with empty group' do
+    before do
+      data = {"Any" => []}
+      post '/features/my_feature/expression', Flipper::Typecast.to_json(data),
+        "CONTENT_TYPE" => "application/json"
+    end
+
+    it 'returns correct error response' do
+      expect(last_response.status).to eq(422)
+      expect(json_response).to eq(api_expression_invalid_response)
+    end
+  end
+
+  describe 'enable with nested empty group' do
+    before do
+      data = {"All" => [{"All" => []}]}
+      post '/features/my_feature/expression', Flipper::Typecast.to_json(data),
+        "CONTENT_TYPE" => "application/json"
+    end
+
+    it 'returns correct error response' do
+      expect(last_response.status).to eq(422)
+      expect(json_response).to eq(api_expression_invalid_response)
+    end
+  end
+
   describe 'enable missing feature' do
     before do
       post '/features/my_feature/expression', JSON.dump(expression.value), "CONTENT_TYPE" => "application/json"

spec/flipper/dsl_spec.rb

@@ -216,6 +216,15 @@
     end
   end
 
+  describe '#enable with empty groups' do
+    it "raises instead of enabling an empty All for everyone" do
+      expect {
+        subject.enable(:stats, Flipper.all)
+      }.to raise_error(ArgumentError, /empty Any\/All groups/)
+      expect(subject[:stats].expression).to be(nil)
+    end
+  end
+
   describe '#add_expression/remove_expression' do
     it 'enables and disables the feature for the expression' do
       expression = Flipper.property(:plan).eq("basic")
@@ -226,7 +235,7 @@
       expect(subject[:stats].expression).to eq(any_expression)
 
       subject.remove_expression(:stats, expression)
-      expect(subject[:stats].expression).to eq(Flipper.any)
+      expect(subject[:stats].expression).to be(nil)
     end
   end
 

spec/flipper/engine_spec.rb

@@ -8,12 +8,22 @@
       config.eager_load = false
       config.logger = ActiveSupport::Logger.new($stdout)
       config.active_support.remove_deprecated_time_with_zone_name = false
+
+      # These specs boot a new application for each example. Rails main's
+      # inflection freezer registers a global after_initialize hook on every
+      # boot, so exclude that unrelated initializer from these test apps.
+      def initializers
+        Rails::Initializable::Collection.new(
+          super.reject { |initializer| initializer.name == "active_support.freeze_inflections" }
+        )
+      end
     end.instance
   end
 
   before do
     stub_request(:get, /flippercloud\.io/).to_return(status: 200, body: "{}")
     Rails.application = nil
+    reset_frozen_inflections
     ActiveSupport::Dependencies.autoload_paths = ActiveSupport::Dependencies.autoload_paths.dup
     ActiveSupport::Dependencies.autoload_once_paths = ActiveSupport::Dependencies.autoload_once_paths.dup
   end
@@ -371,4 +381,21 @@ def initializer(&block)
       block.call
     end
   end
+
+  # Rails main freezes global inflection instances after an application boots.
+  # This spec boots a fresh application for each example, so give each one an
+  # unfrozen copy of the previous application's inflections.
+  def reset_frozen_inflections
+    inflections = ActiveSupport::Inflector::Inflections
+    return unless inflections.respond_to?(:all_instances)
+    return unless inflections.all_instances.compact.any?(&:frozen?)
+
+    english = inflections.instance_variable_get(:@__en_instance__)
+    instances = inflections.instance_variable_get(:@__instance__)
+    copied_instances = instances.dup
+    instances.each_pair { |locale, instance| copied_instances[locale] = instance.dup }
+
+    inflections.instance_variable_set(:@__en_instance__, english.dup) if english
+    inflections.instance_variable_set(:@__instance__, copied_instances)
+  end
 end