Skip to content

[FSSDK-11132] make entity validation configurable in bucketer #1071

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 2 commits into from
Jun 23, 2025

Conversation

raju-opti
Copy link
Contributor

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

  • all existing tests should pass

Issues

  • FSSDK-11132

for cmab, we are using a dummy entityId which is not a real
variation id. We need to skip entity validation for cmab experiments.
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces a configurable flag to skip entity (variation) validation for CMAB experiments.

  • Adds an optional validateEntity parameter to BucketerParams.
  • Defaults validateEntity to false for CMAB experiments in the DecisionService.
  • Updates the bucketer logic to only warn on invalid variation IDs when validateEntity is true.

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 validateEntity so consumers understand when to enable or disable entity validation.
  validateEntity?: boolean;

lib/shared_types.ts:67

  • [nitpick] The name validateEntity could be clearer if inverted (e.g., skipEntityValidation) to more directly reflect that false skips validation.
  validateEntity?: boolean;

lib/core/bucketer/index.ts:142

  • It would be helpful to add a unit test covering the case when validateEntity is false to ensure invalid variation IDs in CMAB experiments do not trigger warnings.
  if (bucketerParams.validateEntity && entityId !== null && !bucketerParams.variationIdMap[entityId]) {

Copy link
Contributor

@junaed-optimizely junaed-optimizely left a comment

Choose a reason for hiding this comment

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

LGTM! Just a suggestion and a confusion 😅

@raju-opti raju-opti merged commit 41fed0d into master Jun 23, 2025
19 checks passed
@raju-opti raju-opti deleted the raju/bucket branch June 23, 2025 11:25
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.

2 participants