Skip to content

Adds Requirement Type All and Any to FeatureFlags #221

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Apr 6, 2023

Conversation

rossgrambo
Copy link
Contributor

In response to issue #28.
And as a rewrite of PR #192.

Add Requirement Type All and Any to FeatureFlags

Filters on Feature Flags have always used "Any" logic. Our execution goes through each filter and when a single one returns "true", we end execution and return true. This PR adds "All" logic. For "All", the execution instead goes through each filter and when a single one returns "false", we end execution, returning false. Otherwise we return true.

In short- this PR:

  • Moves RequirementType from AspNetCore to the main .Net library.
  • Defaults all unspecified Feature Flags to use "Any" logic
  • Adds "All" logic to Feature Flag evaluation

Example

"AllFilterFeature": {
  "RequirementType": "All",
  "EnabledFor": [
    {
      "Name": "Targeting",
      "Parameters": {
        "Audience": {
          "Users": [
            "Jeff",
            "Alicia"
          ]
        }
      }
    },
    {
      "Name": "TimeWindow",
      "Parameters": {
        "Start": "Wed, 1 April 2023 00:00:00 GMT",
        "End": "Wed, 30 April 2023 23:59:59 GMT"
      }
    }
  ]

Evaluation

// Called before TimeWindow:
.IsEnabledAsync(AllFilterFeature, new TargetingContext { UserId = "Jeff" }}) // returns false
.IsEnabledAsync(AllFilterFeature, new TargetingContext { UserId = "Anne" }}) // returns false

// Called during TimeWindow:
.IsEnabledAsync(AllFilterFeature, new TargetingContext { UserId = "Jeff" }}) // returns true
.IsEnabledAsync(AllFilterFeature, new TargetingContext { UserId = "Anne" }}) // returns false

@rossgrambo rossgrambo force-pushed the rossgrambo/requirement-type-all branch from 23b7098 to 13ec4ea Compare March 31, 2023 21:28
@rossgrambo rossgrambo requested a review from avanigupta April 3, 2023 20:50
@rossgrambo
Copy link
Contributor Author

I addressed the comments, but I also re-wrote my changes to the FeatureManager to have less edge-cases.

Both Any and And now have the same default: false
Which allows me to remove the edge case for an empty list of filters with a default of true.
The default changes to "true" when the first filter returns true. In the Any case, we stop and return. In the All case, we now have a true default and we continue.

I also combined some of the inner logic so the Contextual and non-Contextual filters have less duplicated code.

@rossgrambo rossgrambo requested a review from avanigupta April 4, 2023 21:09
…, pulled the RequirementType.All and IgnoreMissingFeatureFilters restriction further up so it happens anytime the feature is evaluated.
@rossgrambo rossgrambo force-pushed the rossgrambo/requirement-type-all branch from 392bfbe to 2511c26 Compare April 4, 2023 21:10
@rossgrambo rossgrambo force-pushed the rossgrambo/requirement-type-all branch from f407d54 to 850b1eb Compare April 5, 2023 02:04
@rossgrambo rossgrambo requested a review from jimmyca15 April 5, 2023 22:14
@rossgrambo rossgrambo force-pushed the rossgrambo/requirement-type-all branch from 7fb0abd to 51377dc Compare April 5, 2023 22:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants