Add empty group detection and pruning to Expression, guard enables - #1036
Open
jnunemaker wants to merge 1 commit into
Open
Add empty group detection and pruning to Expression, guard enables#1036jnunemaker wants to merge 1 commit into
jnunemaker wants to merge 1 commit into
Conversation
An expression containing an empty All group evaluates true for every
actor, so persisting one silently enables the feature for everyone.
- Expression#empty_groups? detects empty Any/All groups anywhere in
the tree, #prune_empty_groups removes them wherever removal cannot
broaden the expression, and #always_true? reports expressions that
are guaranteed to match every actor.
- Feature#enable_expression and #add_expression raise ArgumentError
when the expression contains empty groups. flipper-api already
rescues ArgumentError into the expression_invalid error response,
so API clients get a 422 instead of persisting the footgun.
- Feature#remove_expression of the last condition now disables the
expression instead of persisting an empty group (previously it
left {"Any":[]} — or worse, {"All":[]}, enabled-for-everyone —
in the adapter).
- Includes a seeded property spec verifying against the evaluator
that pruning never broadens an expression.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
An expression containing an empty
Allgroup evaluates true for every actor, so persisting one silently enables the feature for everyone. This PR detects and guards against that footgun.Expression#empty_groups?detects emptyAny/Allgroups anywhere in the tree,#prune_empty_groupsremoves them wherever removal cannot broaden the expression, and#always_true?reports expressions that are guaranteed to match every actor.Feature#enable_expressionand#add_expressionraiseArgumentErrorwhen the expression contains empty groups. flipper-api already rescuesArgumentErrorinto theexpression_invaliderror response, so API clients get a 422 instead of persisting the footgun.Feature#remove_expressionof the last condition now disables the expression instead of persisting an empty group (previously it left{"Any":[]}— or worse,{"All":[]}, enabled-for-everyone — in the adapter).Test plan
empty_groups?,prune_empty_groups, andalways_true?inspec/flipper/expression_spec.rbspec/flipper/feature_spec.rbspec/flipper/expression_pruning_property_spec.rb) verifying against the evaluator that pruning never broadens an expression🤖 Generated with Claude Code