Skip to content

Commit 724f2ef

Browse files
[AI-FSSDK] [FSSDK-12418] Remove experiment type validation from config parsing (#443)
* [AI-FSSDK] [FSSDK-12418] Remove experiment type validation from config parsing * Fix Feature Rollout injection not updating feature.FeatureExperiments The injectFeatureRolloutVariations function was modifying experiments in experimentIDMap but not updating the copies in feature.FeatureExperiments. This caused the decision service to use experiments without the injected "everyone else" variation, leading to incorrect bucketing and fallthrough to rollout rules instead of returning the FR experiment decision. When MapFeatures runs, it creates feature.FeatureExperiments by copying experiments from experimentIDMap. Since Go copies struct values, later modifications to experimentIDMap experiments (like appending to TrafficAllocation slice) don't affect the copies in FeatureExperiments. The fix updates both experimentIDMap AND feature.FeatureExperiments to ensure the decision service sees the complete injected variations. This fixes FSC test failures where users were getting rule_key from rollout rules (e.g., "ee_rule_1002") instead of from FR experiments (e.g., "fr_zero_exp"). * Add test to verify injection updates both experimentIDMap and FeatureExperiments This test ensures that the Feature Rollout injection properly updates both the experimentIDMap and the feature's FeatureExperiments slice, preventing regression of the bug where only experimentIDMap was updated. --------- Co-authored-by: Matjaz Pirnovar <Mat001@users.noreply.github.com>
1 parent 8d64c55 commit 724f2ef

File tree

2 files changed

+187
-16
lines changed

2 files changed

+187
-16
lines changed

pkg/config/datafileprojectconfig/config.go

Lines changed: 16 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -325,21 +325,6 @@ func NewDatafileProjectConfig(jsonDatafile []byte, logger logging.OptimizelyLogP
325325
groupMap, experimentGroupMap := mappers.MapGroups(datafile.Groups)
326326
experimentIDMap, experimentKeyMap := mappers.MapExperiments(allExperiments, experimentGroupMap)
327327

328-
validExperimentTypes := map[entities.ExperimentType]bool{
329-
entities.ExperimentTypeAB: true,
330-
entities.ExperimentTypeMAB: true,
331-
entities.ExperimentTypeCMAB: true,
332-
entities.ExperimentTypeTD: true,
333-
entities.ExperimentTypeFR: true,
334-
}
335-
for _, experiment := range experimentIDMap {
336-
if experiment.Type != "" && !validExperimentTypes[experiment.Type] {
337-
err = fmt.Errorf(`experiment "%s" has invalid type "%s"`, experiment.Key, experiment.Type)
338-
logger.Error(err.Error(), err)
339-
return nil, err
340-
}
341-
}
342-
343328
rollouts, rolloutMap := mappers.MapRollouts(datafile.Rollouts)
344329
integrations := []entities.Integration{}
345330
for _, integration := range datafile.Integrations {
@@ -409,12 +394,13 @@ func NewDatafileProjectConfig(jsonDatafile []byte, logger logging.OptimizelyLogP
409394
// into any experiment with type "feature_rollout". This enables Feature Rollout experiments
410395
// to fall back to the everyone else variation when users are outside the rollout percentage.
411396
func injectFeatureRolloutVariations(featureMap map[string]entities.Feature, experimentMap map[string]entities.Experiment) {
412-
for _, feature := range featureMap {
397+
for featureKey, feature := range featureMap {
413398
everyoneElseVariation := getEveryoneElseVariation(feature)
414399
if everyoneElseVariation == nil {
415400
continue
416401
}
417402

403+
featureModified := false
418404
for _, experimentID := range feature.ExperimentIDs {
419405
experiment, ok := experimentMap[experimentID]
420406
if !ok {
@@ -434,6 +420,20 @@ func injectFeatureRolloutVariations(featureMap map[string]entities.Feature, expe
434420

435421
// Update the experiment in the map
436422
experimentMap[experimentID] = experiment
423+
424+
// Also update the experiment in the feature's FeatureExperiments slice
425+
for j := range feature.FeatureExperiments {
426+
if feature.FeatureExperiments[j].ID == experimentID {
427+
feature.FeatureExperiments[j] = experiment
428+
featureModified = true
429+
break
430+
}
431+
}
432+
}
433+
434+
// Update the feature in the map if it was modified
435+
if featureModified {
436+
featureMap[featureKey] = feature
437437
}
438438
}
439439
}

pkg/config/datafileprojectconfig/feature_rollout_test.go

Lines changed: 171 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -328,3 +328,174 @@ func TestTypeFieldCorrectlyParsed(t *testing.T) {
328328
assert.NoError(t, err)
329329
assert.Empty(t, noTypeExp.Type)
330330
}
331+
332+
// Test 7: Unknown experiment type accepted - config parsing succeeds with unknown type value
333+
func TestUnknownExperimentTypeAccepted(t *testing.T) {
334+
datafile := `{
335+
"accountId": "12345",
336+
"anonymizeIP": false,
337+
"sendFlagDecisions": true,
338+
"botFiltering": false,
339+
"projectId": "67890",
340+
"revision": "1",
341+
"sdkKey": "UnknownTypeTest",
342+
"environmentKey": "production",
343+
"version": "4",
344+
"audiences": [],
345+
"typedAudiences": [],
346+
"attributes": [],
347+
"events": [],
348+
"groups": [],
349+
"integrations": [],
350+
"experiments": [
351+
{
352+
"id": "exp_unknown",
353+
"key": "unknown_type_experiment",
354+
"status": "Running",
355+
"layerId": "layer_1",
356+
"audienceIds": [],
357+
"forcedVariations": {},
358+
"type": "new_unknown_type",
359+
"variations": [
360+
{
361+
"id": "var_1",
362+
"key": "variation_1",
363+
"featureEnabled": true
364+
}
365+
],
366+
"trafficAllocation": [
367+
{
368+
"entityId": "var_1",
369+
"endOfRange": 10000
370+
}
371+
]
372+
}
373+
],
374+
"featureFlags": [
375+
{
376+
"id": "flag_1",
377+
"key": "test_flag",
378+
"rolloutId": "",
379+
"experimentIds": ["exp_unknown"],
380+
"variables": []
381+
}
382+
],
383+
"rollouts": []
384+
}`
385+
386+
logger := logging.GetLogger("test", "TestUnknownExperimentTypeAccepted")
387+
config, err := NewDatafileProjectConfig([]byte(datafile), logger)
388+
require.NoError(t, err, "Config parsing should succeed with unknown experiment type")
389+
require.NotNil(t, config)
390+
391+
experiment, err := config.GetExperimentByKey("unknown_type_experiment")
392+
assert.NoError(t, err)
393+
assert.Equal(t, entities.ExperimentType("new_unknown_type"), experiment.Type)
394+
}
395+
396+
// Test 8: Injection updates both experimentIDMap and feature.FeatureExperiments
397+
func TestFeatureRolloutInjectionUpdatesFeatureExperiments(t *testing.T) {
398+
datafile := `{
399+
"accountId": "12345",
400+
"anonymizeIP": false,
401+
"projectId": "67890",
402+
"revision": "1",
403+
"version": "4",
404+
"audiences": [],
405+
"attributes": [],
406+
"events": [{"id": "1", "key": "test_event", "experimentIds": []}],
407+
"groups": [],
408+
"experiments": [
409+
{
410+
"id": "3002",
411+
"key": "fr_zero_exp",
412+
"status": "Running",
413+
"layerId": "layer_3002",
414+
"audienceIds": [],
415+
"forcedVariations": {},
416+
"type": "fr",
417+
"variations": [
418+
{
419+
"id": "5002",
420+
"key": "rollout_var",
421+
"featureEnabled": true,
422+
"variables": []
423+
}
424+
],
425+
"trafficAllocation": [
426+
{
427+
"entityId": "5002",
428+
"endOfRange": 0
429+
}
430+
]
431+
}
432+
],
433+
"featureFlags": [
434+
{
435+
"id": "1002",
436+
"key": "flag_fr_zero",
437+
"rolloutId": "rollout_1002",
438+
"experimentIds": ["3002"],
439+
"variables": []
440+
}
441+
],
442+
"rollouts": [
443+
{
444+
"id": "rollout_1002",
445+
"experiments": [
446+
{
447+
"id": "ee_rule_1002",
448+
"key": "ee_rule_1002",
449+
"status": "Running",
450+
"layerId": "rollout_1002",
451+
"audienceIds": [],
452+
"variations": [
453+
{
454+
"id": "5102",
455+
"key": "everyone_else_var",
456+
"featureEnabled": true,
457+
"variables": []
458+
}
459+
],
460+
"trafficAllocation": [
461+
{
462+
"entityId": "5102",
463+
"endOfRange": 10000
464+
}
465+
],
466+
"forcedVariations": {}
467+
}
468+
]
469+
}
470+
]
471+
}`
472+
473+
logger := logging.GetLogger("test", "TestFeatureRolloutInjectionUpdatesFeatureExperiments")
474+
config, err := NewDatafileProjectConfig([]byte(datafile), logger)
475+
require.NoError(t, err)
476+
477+
// Get experiment from experimentIDMap
478+
experimentByID, err := config.GetExperimentByID("3002")
479+
assert.NoError(t, err)
480+
assert.Equal(t, 2, len(experimentByID.Variations), "experimentIDMap should have injected variation")
481+
assert.Equal(t, 2, len(experimentByID.TrafficAllocation), "experimentIDMap should have injected traffic allocation")
482+
483+
// Get experiment from feature.FeatureExperiments
484+
feature, err := config.GetFeatureByKey("flag_fr_zero")
485+
assert.NoError(t, err)
486+
assert.Equal(t, 1, len(feature.FeatureExperiments), "Feature should have 1 experiment")
487+
488+
featureExperiment := feature.FeatureExperiments[0]
489+
assert.Equal(t, "3002", featureExperiment.ID)
490+
assert.Equal(t, 2, len(featureExperiment.Variations), "Feature experiment should have injected variation")
491+
assert.Equal(t, 2, len(featureExperiment.TrafficAllocation), "Feature experiment should have injected traffic allocation")
492+
493+
// Verify the injected traffic allocation is correct
494+
lastAllocation := featureExperiment.TrafficAllocation[len(featureExperiment.TrafficAllocation)-1]
495+
assert.Equal(t, "5102", lastAllocation.EntityID, "Last allocation should be everyone else variation")
496+
assert.Equal(t, 10000, lastAllocation.EndOfRange, "Last allocation should have endOfRange 10000")
497+
498+
// Verify both maps have the same traffic allocations
499+
assert.Equal(t, len(experimentByID.TrafficAllocation), len(featureExperiment.TrafficAllocation),
500+
"Traffic allocations should match between experimentIDMap and FeatureExperiments")
501+
}

0 commit comments

Comments
 (0)