Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: Detect undefined variations #2563

Merged
merged 2 commits into from
Oct 24, 2024
Merged

feat: Detect undefined variations #2563

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
18f2563
feat: Detect undefined variations
thomaspoignant Oct 23, 2024
ba8abd6
Change typo
thomaspoignant Oct 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions internal/flag/internal_flag.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -295,13 +295,13 @@ func (f *InternalFlag) IsValid() error {

const isDefaultRule = true
// Validate rules
if err := f.GetDefaultRule().IsValid(isDefaultRule); err != nil {
if err := f.GetDefaultRule().IsValid(isDefaultRule, f.GetVariations()); err != nil {
return err
}

ruleNames := map[string]interface{}{}
for _, rule := range f.GetRules() {
if err := rule.IsValid(!isDefaultRule); err != nil {
if err := rule.IsValid(!isDefaultRule, f.GetVariations()); err != nil {
return err
}

Expand Down
190 changes: 190 additions & 0 deletions internal/flag/internal_flag_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2210,6 +2210,7 @@ func TestInternalFlag_IsValid(t *testing.T) {
fields: fields{
Variations: &map[string]*interface{}{
"A": testconvert.Interface("A"),
"B": testconvert.Interface("B"),
},
DefaultRule: &flag.Rule{
Percentages: &map[string]float64{
Expand Down Expand Up @@ -2264,6 +2265,7 @@ func TestInternalFlag_IsValid(t *testing.T) {
fields: fields{
Variations: &map[string]*interface{}{
"A": testconvert.Interface("A"),
"B": testconvert.Interface("B"),
},
DefaultRule: &flag.Rule{
Percentages: &map[string]float64{
Expand Down Expand Up @@ -2366,6 +2368,194 @@ func TestInternalFlag_IsValid(t *testing.T) {
errorMsg: "",
wantErr: assert.NoError,
},
{
name: "should error if default rule referencing a variation that does not exist",
fields: fields{
Variations: &map[string]*interface{}{
"A": testconvert.Interface("A"),
"B": testconvert.Interface("B"),
},
DefaultRule: &flag.Rule{
VariationResult: testconvert.String("C"),
},
},
errorMsg: "invalid variation: C does not exists",
wantErr: assert.Error,
},
{
name: "should error if default percentage rule referencing a variation that does not exist",
fields: fields{
Variations: &map[string]*interface{}{
"A": testconvert.Interface("A"),
"B": testconvert.Interface("B"),
},
DefaultRule: &flag.Rule{
Percentages: &map[string]float64{
"A": 90,
"B": 5,
"C": 5,
},
},
},
errorMsg: "invalid percentage: variation C does not exists",
wantErr: assert.Error,
},
{
name: "should error if default progressive rule end rollout step referencing a variation that does not exist",
fields: fields{
Variations: &map[string]*interface{}{
"A": testconvert.Interface("A"),
"B": testconvert.Interface("B"),
},
DefaultRule: &flag.Rule{
ProgressiveRollout: &flag.ProgressiveRollout{
Initial: &flag.ProgressiveRolloutStep{
Variation: testconvert.String("A"),
Percentage: testconvert.Float64(0),
Date: testconvert.Time(time.Now().Add(1 * time.Second)),
},
End: &flag.ProgressiveRolloutStep{
Variation: testconvert.String("C"),
Percentage: testconvert.Float64(100),
Date: testconvert.Time(time.Now().Add(2 * time.Second)),
},
},
},
},
errorMsg: "invalid progressive rollout, end variation C does not exists",
wantErr: assert.Error,
},
{
name: "should error if default progressive rule initial rollout step referencing a variation that does not exist",
fields: fields{
Variations: &map[string]*interface{}{
"A": testconvert.Interface("A"),
"B": testconvert.Interface("B"),
},
DefaultRule: &flag.Rule{
ProgressiveRollout: &flag.ProgressiveRollout{
Initial: &flag.ProgressiveRolloutStep{
Variation: testconvert.String("C"),
Percentage: testconvert.Float64(0),
Date: testconvert.Time(time.Now().Add(1 * time.Second)),
},
End: &flag.ProgressiveRolloutStep{
Variation: testconvert.String("A"),
Percentage: testconvert.Float64(100),
Date: testconvert.Time(time.Now().Add(2 * time.Second)),
},
},
},
},
errorMsg: "invalid progressive rollout, initial variation C does not exists",
wantErr: assert.Error,
},
{
name: "should error if targeting rule referencing a variation that does not exist",
fields: fields{
Variations: &map[string]*interface{}{
"A": testconvert.Interface("A"),
"B": testconvert.Interface("B"),
},
Rules: &[]flag.Rule{
{
Query: testconvert.String("targetingKey eq 1"),
VariationResult: testconvert.String("C"),
},
},
DefaultRule: &flag.Rule{
VariationResult: testconvert.String("A"),
},
},
errorMsg: "invalid variation: C does not exists",
wantErr: assert.Error,
},
{
name: "should error if percentage in targeting rule referencing a variation that does not exist",
fields: fields{
Variations: &map[string]*interface{}{
"A": testconvert.Interface("A"),
"B": testconvert.Interface("B"),
},
Rules: &[]flag.Rule{
{
Query: testconvert.String("targetingKey eq 1"),
Percentages: &map[string]float64{
"A": 90,
"B": 5,
"C": 5,
},
},
},
DefaultRule: &flag.Rule{
VariationResult: testconvert.String("A"),
},
},
errorMsg: "invalid percentage: variation C does not exists",
wantErr: assert.Error,
},
{
name: "should error if progressive rollout in targeting rule referencing an initial variation that does not exist",
fields: fields{
Variations: &map[string]*interface{}{
"A": testconvert.Interface("A"),
"B": testconvert.Interface("B"),
},
Rules: &[]flag.Rule{
{
Query: testconvert.String("targetingKey eq 1"),
ProgressiveRollout: &flag.ProgressiveRollout{
Initial: &flag.ProgressiveRolloutStep{
Variation: testconvert.String("C"),
Percentage: testconvert.Float64(0),
Date: testconvert.Time(time.Now().Add(1 * time.Second)),
},
End: &flag.ProgressiveRolloutStep{
Variation: testconvert.String("A"),
Percentage: testconvert.Float64(100),
Date: testconvert.Time(time.Now().Add(2 * time.Second)),
},
},
},
},
DefaultRule: &flag.Rule{
VariationResult: testconvert.String("A"),
},
},
errorMsg: "invalid progressive rollout, initial variation C does not exists",
wantErr: assert.Error,
},
{
name: "should error if progressive rollout in targeting rule referencing an end variation that does not exist",
fields: fields{
Variations: &map[string]*interface{}{
"A": testconvert.Interface("A"),
"B": testconvert.Interface("B"),
},
Rules: &[]flag.Rule{
{
Query: testconvert.String("targetingKey eq 1"),
ProgressiveRollout: &flag.ProgressiveRollout{
Initial: &flag.ProgressiveRolloutStep{
Variation: testconvert.String("A"),
Percentage: testconvert.Float64(0),
Date: testconvert.Time(time.Now().Add(1 * time.Second)),
},
End: &flag.ProgressiveRolloutStep{
Variation: testconvert.String("C"),
Percentage: testconvert.Float64(100),
Date: testconvert.Time(time.Now().Add(2 * time.Second)),
},
},
},
},
DefaultRule: &flag.Rule{
VariationResult: testconvert.String("A"),
},
},
errorMsg: "invalid progressive rollout, end variation C does not exists",
wantErr: assert.Error,
},
}

for _, tt := range tests {
Expand Down
35 changes: 28 additions & 7 deletions internal/flag/rule.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,7 @@ func (r *Rule) MergeRules(updatedRule Rule) {
}

// IsValid is checking if the rule is valid
func (r *Rule) IsValid(defaultRule bool) error {
func (r *Rule) IsValid(defaultRule bool, variations map[string]*interface{}) error {
if !defaultRule && r.IsDisable() {
return nil
}
Expand All @@ -240,8 +240,11 @@ func (r *Rule) IsValid(defaultRule bool) error {
// Validate the percentage of the rule
if r.Percentages != nil {
count := float64(0)
for _, p := range r.GetPercentages() {
for k, p := range r.GetPercentages() {
count += p
if _, ok := variations[k]; !ok {
return fmt.Errorf("invalid percentage: variation %s does not exists", k)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be better to change like this?

Suggested change
return fmt.Errorf("invalid percentage: variation %s does not exists", k)
return fmt.Errorf("invalid percentage: variation %s does not exist", k)

}
}

if len(r.GetPercentages()) == 0 {
Expand All @@ -254,11 +257,29 @@ func (r *Rule) IsValid(defaultRule bool) error {
}

// Progressive rollout: check that initial is lower than end
if r.ProgressiveRollout != nil &&
(r.GetProgressiveRollout().End.getPercentage() < r.GetProgressiveRollout().Initial.getPercentage()) {
return fmt.Errorf("invalid progressive rollout, initial percentage should be lower "+
"than end percentage: %v/%v",
r.GetProgressiveRollout().Initial.getPercentage(), r.GetProgressiveRollout().End.getPercentage())
if r.ProgressiveRollout != nil {
if r.GetProgressiveRollout().End.getPercentage() < r.GetProgressiveRollout().Initial.getPercentage() {
return fmt.Errorf("invalid progressive rollout, initial percentage should be lower "+
"than end percentage: %v/%v",
r.GetProgressiveRollout().Initial.getPercentage(), r.GetProgressiveRollout().End.getPercentage())
}

endVar := r.GetProgressiveRollout().End.getVariation()
if _, ok := variations[endVar]; !ok {
return fmt.Errorf("invalid progressive rollout, end variation %s does not exists", endVar)
}

initialVar := r.GetProgressiveRollout().Initial.getVariation()
if _, ok := variations[initialVar]; !ok {
return fmt.Errorf("invalid progressive rollout, initial variation %s does not exists", initialVar)
}
}

// Check that the variation exists
if r.Percentages == nil && r.ProgressiveRollout == nil && r.VariationResult != nil {
if _, ok := variations[r.GetVariationResult()]; !ok {
return fmt.Errorf("invalid variation: %s does not exists", r.GetVariationResult())
}
}
return nil
}
Expand Down
11 changes: 0 additions & 11 deletions variation_all_flags_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,17 +31,6 @@ func TestAllFlagsState(t *testing.T) {
jsonOutput: "./testdata/ffclient/all_flags/marshal_json/valid_multiple_types.json",
initModule: true,
},
{
name: "Error in flag-0",
config: Config{
Retriever: &fileretriever.Retriever{
Path: "./testdata/ffclient/all_flags/config_flag/flag-config-with-error.yaml",
},
},
valid: false,
jsonOutput: "./testdata/ffclient/all_flags/marshal_json/error_in_flag_0.json",
initModule: true,
},
{
name: "module not init",
config: Config{
Expand Down
Loading