Skip to content

Commit 0ce0cd1

Browse files
cre8ivejpcre8ivejp
committed
fix: variation deletion validation
Signed-off-by: Alessandro Yuichi Okimoto <yuichijpn@gmail.com>
1 parent 5cc47c4 commit 0ce0cd1

4 files changed

Lines changed: 52 additions & 28 deletions

File tree

‎pkg/feature/api/validation.go‎

Lines changed: 16 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1192,10 +1192,8 @@ func validateRemoveVariationCommand(cmd *featureproto.RemoveVariationCommand, fs
11921192
}
11931193
}
11941194

1195-
if deletedVariationValue == "" {
1196-
// Variation not found, let domain validation handle this
1197-
return nil
1198-
}
1195+
// Even if we can't find the variation value, we should still check for prerequisites
1196+
// since they reference variation IDs, not values
11991197

12001198
// Optimization: First check if ANY features depend on our target
12011199
// This reuses existing logic to quickly filter relevant features
@@ -1219,13 +1217,19 @@ func validateRemoveVariationCommand(cmd *featureproto.RemoveVariationCommand, fs
12191217
}
12201218

12211219
// Use our precise cross-feature validation only on dependent features
1222-
deletedVariations := map[string]string{
1223-
cmd.Id: deletedVariationValue,
1220+
deletedVariations := map[string]string{}
1221+
if deletedVariationValue != "" {
1222+
deletedVariations[cmd.Id] = deletedVariationValue
1223+
} else {
1224+
// We don't have the variation value, but we still need to check prerequisites
1225+
// For prerequisites, we only need the variation ID (key), not the value
1226+
deletedVariations[cmd.Id] = "" // Empty value, but we'll check keys for prerequisites
12241227
}
12251228

12261229
if err := featuredomain.ValidateVariationUsage(dependentFeaturesSlice, tgt.Id, deletedVariations); err != nil {
12271230
if errors.Is(err, featuredomain.ErrVariationInUse) {
1228-
dt, err := statusVariationInUseByOtherFeatures.WithDetails(&errdetails.LocalizedMessage{
1231+
// Use the legacy error status for RemoveVariationCommand for backward compatibility
1232+
dt, err := statusInvalidChangingVariation.WithDetails(&errdetails.LocalizedMessage{
12291233
Locale: localizer.GetLocale(),
12301234
Message: localizer.MustLocalizeWithTemplate(locale.InvalidArgumentError, "variation"),
12311235
})
@@ -1945,7 +1949,10 @@ func validateVariationDeletion(
19451949
allFeaturesMap[f.Id] = f
19461950
}
19471951

1948-
dependentFeatures := featuredomain.GetFeaturesDependsOnTargets([]*featureproto.Feature{targetFeature}, allFeaturesMap)
1952+
dependentFeatures := featuredomain.GetFeaturesDependsOnTargets(
1953+
[]*featureproto.Feature{targetFeature},
1954+
allFeaturesMap,
1955+
)
19491956
delete(dependentFeatures, targetFeatureID) // Remove the target itself
19501957

19511958
if len(dependentFeatures) == 0 {
@@ -1959,7 +1966,7 @@ func validateVariationDeletion(
19591966
dependentFeaturesSlice = append(dependentFeaturesSlice, f)
19601967
}
19611968

1962-
// Use our precise cross-feature validation only on dependent features
1969+
// Check if the deleted variation is used as a prerequisite or rule in other features
19631970
if err := featuredomain.ValidateVariationUsage(
19641971
dependentFeaturesSlice,
19651972
targetFeatureID,

‎pkg/feature/api/validation_test.go‎

Lines changed: 25 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -98,8 +98,8 @@ func TestValidateVariationDeletion(t *testing.T) {
9898
localizer := locale.NewLocalizer(ctx)
9999

100100
variationID1 := "variation-1"
101+
variationID2 := "variation-2"
101102
variationValue1 := "true"
102-
variationValue2 := "false"
103103

104104
patterns := []struct {
105105
desc string
@@ -158,7 +158,16 @@ func TestValidateVariationDeletion(t *testing.T) {
158158
},
159159
features: []*featureproto.Feature{
160160
{
161-
Id: "feature-2",
161+
Id: "feature-1", // Target feature (must be included)
162+
Variations: []*featureproto.Variation{
163+
{
164+
Id: variationID1,
165+
Value: variationValue1,
166+
},
167+
},
168+
},
169+
{
170+
Id: "feature-2", // Dependent feature
162171
Prerequisites: []*featureproto.Prerequisite{
163172
{
164173
FeatureId: "feature-1",
@@ -172,7 +181,7 @@ func TestValidateVariationDeletion(t *testing.T) {
172181
localizer.MustLocalizeWithTemplate(locale.InvalidArgumentError, "variation"), localizer),
173182
},
174183
{
175-
desc: "error: other feature has FEATURE_FLAG rule using deleted variation value",
184+
desc: "error: other feature has FEATURE_FLAG rule using deleted variation ID",
176185
variationChanges: []*featureproto.VariationChange{
177186
{
178187
ChangeType: featureproto.ChangeType_DELETE,
@@ -184,14 +193,23 @@ func TestValidateVariationDeletion(t *testing.T) {
184193
},
185194
features: []*featureproto.Feature{
186195
{
187-
Id: "feature-2",
196+
Id: "feature-1", // Target feature (must be included)
197+
Variations: []*featureproto.Variation{
198+
{
199+
Id: variationID1,
200+
Value: variationValue1,
201+
},
202+
},
203+
},
204+
{
205+
Id: "feature-2", // Dependent feature
188206
Rules: []*featureproto.Rule{
189207
{
190208
Clauses: []*featureproto.Clause{
191209
{
192210
Operator: featureproto.Clause_FEATURE_FLAG,
193211
Attribute: "feature-1",
194-
Values: []string{variationValue1},
212+
Values: []string{variationID1}, // Fixed: Use variation ID, not value
195213
},
196214
},
197215
},
@@ -228,7 +246,7 @@ func TestValidateVariationDeletion(t *testing.T) {
228246
expected: nil,
229247
},
230248
{
231-
desc: "success: different variation value in FEATURE_FLAG rule",
249+
desc: "success: different variation ID in FEATURE_FLAG rule",
232250
variationChanges: []*featureproto.VariationChange{
233251
{
234252
ChangeType: featureproto.ChangeType_DELETE,
@@ -247,7 +265,7 @@ func TestValidateVariationDeletion(t *testing.T) {
247265
{
248266
Operator: featureproto.Clause_FEATURE_FLAG,
249267
Attribute: "feature-1",
250-
Values: []string{variationValue2}, // Different value
268+
Values: []string{variationID2}, // Fixed: Different variation ID
251269
},
252270
},
253271
},

‎pkg/feature/domain/feature.go‎

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1290,12 +1290,11 @@ func ValidateVariationUsage(
12901290
for _, rule := range f.Rules {
12911291
for _, clause := range rule.Clauses {
12921292
if clause.Operator == feature.Clause_FEATURE_FLAG && clause.Attribute == targetFeatureID {
1293-
// Check if clause values match any deleted variation values
1293+
// FEATURE_FLAG clause values contain variation IDs, not values
1294+
// We should check if any clause values match deleted variation IDs
12941295
for _, clValue := range clause.Values {
1295-
for _, delValue := range deletedVariations {
1296-
if clValue == delValue {
1297-
return ErrVariationInUse
1298-
}
1296+
if _, found := deletedVariations[clValue]; found {
1297+
return ErrVariationInUse
12991298
}
13001299
}
13011300
}

‎pkg/feature/domain/feature_test.go‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -2057,8 +2057,8 @@ func TestValidateVariationUsage(t *testing.T) {
20572057
t.Parallel()
20582058

20592059
variationID1 := "variation-1"
2060+
variationID2 := "variation-2"
20602061
variationValue1 := "true"
2061-
variationValue2 := "false"
20622062

20632063
patterns := []struct {
20642064
desc string
@@ -2115,7 +2115,7 @@ func TestValidateVariationUsage(t *testing.T) {
21152115
expected: ErrVariationInUse,
21162116
},
21172117
{
2118-
desc: "error: other feature has FEATURE_FLAG rule using deleted variation value",
2118+
desc: "error: other feature has FEATURE_FLAG rule using deleted variation ID",
21192119
features: []*ftproto.Feature{
21202120
{
21212121
Id: "feature-2",
@@ -2125,7 +2125,7 @@ func TestValidateVariationUsage(t *testing.T) {
21252125
{
21262126
Operator: ftproto.Clause_FEATURE_FLAG,
21272127
Attribute: "feature-1",
2128-
Values: []string{variationValue1},
2128+
Values: []string{variationID1}, // Fixed: Use variation ID, not value
21292129
},
21302130
},
21312131
},
@@ -2175,7 +2175,7 @@ func TestValidateVariationUsage(t *testing.T) {
21752175
expected: nil,
21762176
},
21772177
{
2178-
desc: "success: different variation value in FEATURE_FLAG rule",
2178+
desc: "success: different variation ID in FEATURE_FLAG rule",
21792179
features: []*ftproto.Feature{
21802180
{
21812181
Id: "feature-2",
@@ -2185,7 +2185,7 @@ func TestValidateVariationUsage(t *testing.T) {
21852185
{
21862186
Operator: ftproto.Clause_FEATURE_FLAG,
21872187
Attribute: "feature-1",
2188-
Values: []string{variationValue2}, // Different value
2188+
Values: []string{variationID2}, // Fixed: Different variation ID
21892189
},
21902190
},
21912191
},
@@ -2217,8 +2217,8 @@ func TestValidateVariationUsage(t *testing.T) {
22172217
{
22182218
Id: "test-clause",
22192219
Operator: ftproto.Clause_FEATURE_FLAG,
2220-
Attribute: "feature-A", // References feature being updated
2221-
Values: []string{"true"}, // References the value we're deleting
2220+
Attribute: "feature-A", // References feature being updated
2221+
Values: []string{"var-true"}, // Fixed: Use variation ID, not value
22222222
},
22232223
},
22242224
},

0 commit comments

Comments
 (0)