[FSSDK-11132] make entity validation configurable in bucketer #1071
Conversation
for cmab, we are using a dummy entityId which is not a real variation id. We need to skip entity validation for cmab experiments.
raju-opti
requested review from
Copilot and
junaed-optimizely
and removed request for
Copilot
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a configurable flag to skip entity (variation) validation for CMAB experiments.
- Adds an optional
validateEntityparameter toBucketerParams. - Defaults
validateEntitytofalsefor CMAB experiments in the DecisionService. - Updates the bucketer logic to only warn on invalid variation IDs when
validateEntityistrue.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| lib/shared_types.ts | Added validateEntity? to the BucketerParams interface |
| lib/core/decision_service/index.ts | Initialized and set validateEntity based on CMAB flag |
| lib/core/bucketer/index.ts | Guarded invalid-variation checks on validateEntity |
Comments suppressed due to low confidence (3)
lib/shared_types.ts:67
- Consider adding a doc comment explaining the default behavior and purpose of
validateEntityso consumers understand when to enable or disable entity validation.
validateEntity?: boolean;
lib/shared_types.ts:67
- [nitpick] The name
validateEntitycould be clearer if inverted (e.g.,skipEntityValidation) to more directly reflect thatfalseskips validation.
validateEntity?: boolean;
lib/core/bucketer/index.ts:142
- It would be helpful to add a unit test covering the case when
validateEntityisfalseto ensure invalid variation IDs in CMAB experiments do not trigger warnings.
if (bucketerParams.validateEntity && entityId !== null && !bucketerParams.variationIdMap[entityId]) {
| @@ -594,12 +594,16 @@ export class DecisionService { | |||
| bucketingId: string, | |||
| userId: string | |||
| ): BucketerParams { | |||
| let validateEntity = true; | |||
There was a problem hiding this comment.
[nitpick] Consider using a const with a ternary or default assignment pattern to keep the variable declaration and its final value closer together, improving readability.
| let validateEntity = true; | |
| const validateEntity = experiment.cmab ? false : true; |
Copilot uses AI. Check for mistakes.
| reasons: decideReasons, | ||
| }; | ||
|
|
||
| if (bucketerParams.validateEntity && entityId !== null && !bucketerParams.variationIdMap[entityId]) { |
There was a problem hiding this comment.
The nested entityId !== null and truthy entityId check inside could be consolidated—removing the inner if (entityId)—to simplify the condition since entityId !== null already guards it.
Copilot uses AI. Check for mistakes.
| @@ -613,6 +617,7 @@ export class DecisionService { | |||
| trafficAllocationConfig, | |||
| userId, | |||
| variationIdMap: configObj.variationIdMap, | |||
| validateEntity, | |||
There was a problem hiding this comment.
NIT: OPTIONAL: We could simplify this by doing validateEntity: !experiment.cmab .
| }; | ||
|
|
||
| if (bucketerParams.validateEntity && entityId !== null && !bucketerParams.variationIdMap[entityId]) { | ||
| if (entityId) { |
There was a problem hiding this comment.
Since the id value could be either string or null, I am confused with copilot here as well! 😅 Why we need this check ?
Summary
for cmab, we are using a dummy entityId which is not a real variation id. We need to skip entity validation for cmab experiments.
Test plan
Issues