diff --git a/.claude/commands/create-pr-current.md b/.claude/commands/create-pr-current.md new file mode 100644 index 000000000..4a4b2159b --- /dev/null +++ b/.claude/commands/create-pr-current.md @@ -0,0 +1,253 @@ +--- +name: create-pr-current +displayName: Create PR for Current Branch +description: Creates a pull request for the current branch in java-sdk repository. Must be explicitly invoked with /create-pr-current to avoid confusion with create-prs agent. +version: 1.0.0 +disable-model-invocation: true +requiredTools: + - Bash + - Read + - mcp__github__create_pull_request +--- + +# Create PR for Current Branch + +Creates a pull request for the current branch in the java-sdk repository. + +## Instructions + +When invoked, follow these steps: + +**🚨 CRITICAL: Before starting the workflow, use TodoWrite to set up all steps as pending todos.** + +**Step Setup (Use TodoWrite tool immediately):** +``` +1. Get current branch information +2. Check for merge conflicts with master branch +3. Get repository information +4. Push current branch +5. Generate PR title and description +6. Create pull request +7. Report results +``` + +**TodoWrite Setup Example:** +```json +{ + "todos": [ + { + "content": "Get current branch information", + "activeForm": "Getting current branch information", + "status": "pending" + }, + { + "content": "Check for merge conflicts with master branch", + "activeForm": "Checking for merge conflicts with master branch", + "status": "pending" + }, + { + "content": "Get repository information", + "activeForm": "Getting repository information", + "status": "pending" + }, + { + "content": "Push current branch", + "activeForm": "Pushing current branch", + "status": "pending" + }, + { + "content": "Generate PR title and description", + "activeForm": "Generating PR title and description", + "status": "pending" + }, + { + "content": "Create pull request", + "activeForm": "Creating pull request", + "status": "pending" + }, + { + "content": "Report results", + "activeForm": "Reporting results", + "status": "pending" + } + ] +} +``` + +**After completing each step, use TodoWrite to mark it as completed before proceeding to the next step.** + +--- + +### 1. Get Current Branch Information +**Mark this step as in_progress using TodoWrite** +- Use `git branch --show-current` to get the current branch name +- Use `git status` to verify there are no uncommitted changes +- If uncommitted changes exist, warn user and ask if they want to commit first + +**Mark Step 1 as completed using TodoWrite before proceeding** + +--- + +### 2. Check for Merge Conflicts with Master Branch +**Mark this step as in_progress using TodoWrite** + +#### a. Detect Potential Conflicts (Physical and Logical) +- Fetch latest master: `git fetch origin master` +- Check for physical conflicts: `git merge-tree $(git merge-base HEAD origin/master) HEAD origin/master` +- Check for .md file changes: `git diff --name-only origin/master...HEAD | grep '\.md$'` +- **Decision logic**: + - If .md files changed → ALWAYS proceed to conflict analysis (even if no physical conflicts) + - Reason: Logical conflicts in agent configs/prompts cannot be detected by git + - If only non-.md files changed AND no physical conflicts → Skip to step 3 + - If physical conflicts detected → Proceed to conflict analysis + +#### b-h. Resolve Conflicts with Semantic Evaluation + +**🚨 CRITICAL: Follow the detailed process in [prompts/rules/git-logical-merge.md](../../prompts/rules/git-logical-merge.md)** + +This process handles both physical conflicts (detected by git) and logical conflicts (semantic incompatibilities in .md files that git cannot detect). + +**The process includes these critical steps:** +- **Step b**: Analyze files with semantic evaluation (understand what EACH side added) +- **Step c**: Categorize conflict severity (CRITICAL/HIGH/MEDIUM/LOW) +- **Step d**: Present critical conflicts to user with impact assessment +- **Step e**: **BLOCKING** - Use AskUserQuestion tool for user decision (MANDATORY) +- **Step f**: Execute resolution strategy based on user choice +- **Step g**: **MANDATORY** - Verify resolution preserves intent using Read tool +- **Step h**: **GATE CHECK** - Pre-commit checklist (all answers must be YES) + +**⚠️ You MUST follow every step in git-logical-merge.md. Do NOT skip steps e, g, or h - they contain blocking requirements and verification gates.** + +See [git-logical-merge.md](../../prompts/rules/git-logical-merge.md) for complete step-by-step instructions with examples. + +**Mark Step 2 as completed using TodoWrite before proceeding** + +--- + +### 3. Get Repository Information +**Mark this step as in_progress using TodoWrite** +- Repository owner: Extract from git remote (e.g., "optimizely") +- Repository name: "java-sdk" +- Base branch: Typically "master" (verify with `git remote show origin | grep "HEAD branch"`) + +**Mark Step 3 as completed using TodoWrite before proceeding** + +--- + +### 4. Push Current Branch +**Mark this step as in_progress using TodoWrite** +- Push branch to remote: `git push -u origin ` +- Verify push succeeded + +**Mark Step 4 as completed using TodoWrite before proceeding** + +--- + +### 5. Generate PR Title and Description +**Mark this step as in_progress using TodoWrite** + +#### PR Title (Local Rule - java-sdk Repository) +- **Format:** `[TICKET-ID] Brief description of changes` +- **Ticket ID:** Use uppercase format (e.g., `[FSSDK-12345]`) or `[FSSDK-0000]` if no ticket +- **Example:** `[FSSDK-12345] Add feature rollout support` + +#### PR Body/Description (Follow pr-format.md) + +**🚨 CRITICAL WORKFLOW - Follow these steps exactly:** + +**Step 1: Read the Template** +- ALWAYS read `prompts/rules/pr-format.md` first (lines 55-70 for template) +- The template has ONLY these sections: + - ## Summary + - ## Changes + - ## Jira Ticket + - ## Notes (Optional - only for breaking changes) + +**Step 2: Read Forbidden Sections** +- Read pr-format.md lines 48-53 for what NOT to include +- NEVER add: Testing, Test Coverage, Quality Assurance, Files Modified, Implementation Details +- NEVER add: Commits section (similar to Files Modified) + +**Step 3: Generate PR Description** +- Use ONLY the template sections from pr-format.md +- Summary: 2-3 sentences max +- Changes: 3-5 bullet points, high-level only +- Jira Ticket: `[FSSDK-0000](https://optimizely-ext.atlassian.net/browse/FSSDK-0000)` or actual ticket +- Notes: Only if breaking changes exist + +**Step 4: Verify Before Sending** +- Check: Does output have exactly Summary, Changes, Jira Ticket (and optionally Notes)? +- Check: Does output have ANY sections not in the template (Commits, Files, Tests, etc.)? +- If ANY extra sections exist → REMOVE THEM +- Only proceed when output matches template exactly + +**Mark Step 5 as completed using TodoWrite before proceeding** + +--- + +### 6. Create or Update Pull Request +**Mark this step as in_progress using TodoWrite** + +#### a. Check if PR already exists +- Use `mcp__github__list_pull_requests` with `head` parameter to check for existing PR +- Search for PRs with head branch matching current branch + +#### b. If PR exists - Update it +- Use `mcp__github__update_pull_request` with PR number +- Parameters: + - `owner`: Repository owner + - `repo`: "java-sdk" + - `pullNumber`: Existing PR number + - `title`: New PR title + - `body`: New PR description +- Report: "Updated existing PR #X" + +#### c. If PR does not exist - Create it +- **CRITICAL:** Use GitHub MCP tool `mcp__github__create_pull_request` +- **NEVER** use `gh pr create` via Bash +- Parameters: + - `owner`: Repository owner + - `repo`: "java-sdk" + - `title`: PR title + - `head`: Current branch name + - `base`: Base branch (usually "master") + - `body`: PR description +- Report: "Created new PR #X" + +**Mark Step 6 as completed using TodoWrite before proceeding** + +--- + +### 7. Report Results +**Mark this step as in_progress using TodoWrite** +- Display PR URL +- Show PR title and base branch +- Confirm PR was created successfully + +**Mark Step 7 as completed using TodoWrite - PR creation complete!** + +--- + +## Example Usage + +User: "/create-pr-current" + +Assistant executes: +1. Gets current branch: `jae/add-feature` +2. Checks for merge conflicts with master (if any, resolves interactively) +3. Gets repository information +4. Pushes: `git push -u origin jae/add-feature` +5. Generates PR title and description +6. Creates PR using `mcp__github__create_pull_request` +7. Returns PR URL + +**Note:** Triggers are disabled to avoid confusion with create-prs agent. +Must be explicitly invoked with `/create-pr-current` command. + +## Error Handling + +- If no git repository: Report error +- If on master/main branch: Warn and ask for confirmation +- If uncommitted changes: Offer to show status and ask to commit first +- If push fails: Report error and abort +- If GitHub MCP fails: Report error (do NOT fall back to gh CLI) diff --git a/core-api/src/main/java/com/optimizely/ab/config/DatafileProjectConfig.java b/core-api/src/main/java/com/optimizely/ab/config/DatafileProjectConfig.java index e8dea8e90..a0c8f0d93 100644 --- a/core-api/src/main/java/com/optimizely/ab/config/DatafileProjectConfig.java +++ b/core-api/src/main/java/com/optimizely/ab/config/DatafileProjectConfig.java @@ -194,6 +194,10 @@ public DatafileProjectConfig(String accountId, List allExperiments = new ArrayList(); allExperiments.addAll(experiments); allExperiments.addAll(aggregateGroupExperiments(groups)); + + // Inject "everyone else" variation into feature_rollout experiments + allExperiments = injectFeatureRolloutVariations(allExperiments, this.featureFlags, this.rollouts); + this.experiments = Collections.unmodifiableList(allExperiments); if (holdouts == null) { @@ -357,6 +361,110 @@ public Experiment getExperimentForVariationId(String variationId) { return this.variationIdToExperimentMapping.get(variationId); } + /** + * Injects the "everyone else" variation from the flag's rollout into any experiment + * with type "feature_rollout". This enables Feature Rollout experiments to fall back + * to the everyone else variation when users are outside the rollout percentage. + */ + private List injectFeatureRolloutVariations( + List allExperiments, + List featureFlags, + List rollouts + ) { + if (featureFlags == null || featureFlags.isEmpty()) { + return allExperiments; + } + + // Build rollout ID to Rollout mapping + Map rolloutMap = new HashMap<>(); + if (rollouts != null) { + for (Rollout rollout : rollouts) { + rolloutMap.put(rollout.getId(), rollout); + } + } + + // Build experiment ID to index mapping for quick lookup + Map experimentIndexMap = new HashMap<>(); + for (int i = 0; i < allExperiments.size(); i++) { + experimentIndexMap.put(allExperiments.get(i).getId(), i); + } + + List result = new ArrayList<>(allExperiments); + + for (FeatureFlag flag : featureFlags) { + Variation everyoneElseVariation = getEveryoneElseVariation(flag, rolloutMap); + if (everyoneElseVariation == null) { + continue; + } + + for (String experimentId : flag.getExperimentIds()) { + Integer index = experimentIndexMap.get(experimentId); + if (index == null) { + continue; + } + Experiment experiment = result.get(index); + if (!Experiment.TYPE_FR.equals(experiment.getType())) { + continue; + } + + // Create new experiment with injected variation and traffic allocation + List newVariations = new ArrayList<>(experiment.getVariations()); + newVariations.add(everyoneElseVariation); + + List newTrafficAllocation = new ArrayList<>(experiment.getTrafficAllocation()); + newTrafficAllocation.add(new TrafficAllocation(everyoneElseVariation.getId(), 10000)); + + Experiment updatedExperiment = new Experiment( + experiment.getId(), + experiment.getKey(), + experiment.getStatus(), + experiment.getLayerId(), + experiment.getAudienceIds(), + experiment.getAudienceConditions(), + newVariations, + experiment.getUserIdToVariationKeyMap(), + newTrafficAllocation, + experiment.getGroupId(), + experiment.getCmab(), + experiment.getType() + ); + + result.set(index, updatedExperiment); + } + } + + return result; + } + + /** + * Gets the "everyone else" variation from the flag's rollout. + * The everyone else rule is the last experiment in the rollout, + * and the variation is the first variation of that rule. + * + * @return the everyone else variation, or null if it cannot be resolved + */ + @Nullable + private Variation getEveryoneElseVariation(FeatureFlag flag, Map rolloutMap) { + String rolloutId = flag.getRolloutId(); + if (rolloutId == null || rolloutId.isEmpty()) { + return null; + } + Rollout rollout = rolloutMap.get(rolloutId); + if (rollout == null) { + return null; + } + List rolloutExperiments = rollout.getExperiments(); + if (rolloutExperiments == null || rolloutExperiments.isEmpty()) { + return null; + } + Experiment everyoneElseRule = rolloutExperiments.get(rolloutExperiments.size() - 1); + List variations = everyoneElseRule.getVariations(); + if (variations == null || variations.isEmpty()) { + return null; + } + return variations.get(0); + } + private List aggregateGroupExperiments(List groups) { List groupExperiments = new ArrayList(); for (Group group : groups) { diff --git a/core-api/src/main/java/com/optimizely/ab/config/Experiment.java b/core-api/src/main/java/com/optimizely/ab/config/Experiment.java index 7d687e9e9..51d4fbb18 100644 --- a/core-api/src/main/java/com/optimizely/ab/config/Experiment.java +++ b/core-api/src/main/java/com/optimizely/ab/config/Experiment.java @@ -41,6 +41,7 @@ public class Experiment implements ExperimentCore { private final String status; private final String layerId; private final String groupId; + private final String type; private final Cmab cmab; private final List audienceIds; @@ -52,6 +53,12 @@ public class Experiment implements ExperimentCore { private final Map variationIdToVariationMap; private final Map userIdToVariationKeyMap; + public static final String TYPE_AB = "ab"; + public static final String TYPE_MAB = "mab"; + public static final String TYPE_CMAB = "cmab"; + public static final String TYPE_TD = "td"; + public static final String TYPE_FR = "fr"; + public enum ExperimentStatus { RUNNING("Running"), LAUNCHED("Launched"), @@ -72,7 +79,7 @@ public String toString() { @VisibleForTesting public Experiment(String id, String key, String layerId) { - this(id, key, null, layerId, Collections.emptyList(), null, Collections.emptyList(), Collections.emptyMap(), Collections.emptyList(), "", null); + this(id, key, null, layerId, Collections.emptyList(), null, Collections.emptyList(), Collections.emptyMap(), Collections.emptyList(), "", null, null); } @VisibleForTesting @@ -81,7 +88,7 @@ public Experiment(String id, String key, String status, String layerId, List variations, Map userIdToVariationKeyMap, List trafficAllocation, String groupId) { this(id, key, status, layerId, audienceIds, audienceConditions, variations, - userIdToVariationKeyMap, trafficAllocation, groupId, null); // Default cmab=null + userIdToVariationKeyMap, trafficAllocation, groupId, null, null); // Default cmab=null, type=null } @VisibleForTesting @@ -90,7 +97,27 @@ public Experiment(String id, String key, String status, String layerId, List variations, Map userIdToVariationKeyMap, List trafficAllocation) { this(id, key, status, layerId, audienceIds, audienceConditions, variations, - userIdToVariationKeyMap, trafficAllocation, "", null); // Default groupId="" and cmab=null + userIdToVariationKeyMap, trafficAllocation, "", null, null); // Default groupId="", cmab=null, type=null + } + + @VisibleForTesting + public Experiment(String id, String key, String status, String layerId, + List audienceIds, Condition audienceConditions, + List variations, Map userIdToVariationKeyMap, + List trafficAllocation, + Cmab cmab) { + this(id, key, status, layerId, audienceIds, audienceConditions, variations, + userIdToVariationKeyMap, trafficAllocation, "", cmab, null); // Default groupId="" and type=null + } + + @VisibleForTesting + public Experiment(String id, String key, String status, String layerId, + List audienceIds, Condition audienceConditions, + List variations, Map userIdToVariationKeyMap, + List trafficAllocation, String groupId, + Cmab cmab) { + this(id, key, status, layerId, audienceIds, audienceConditions, variations, + userIdToVariationKeyMap, trafficAllocation, groupId, cmab, null); // Default type=null } @JsonCreator @@ -103,8 +130,9 @@ public Experiment(@JsonProperty("id") String id, @JsonProperty("variations") List variations, @JsonProperty("forcedVariations") Map userIdToVariationKeyMap, @JsonProperty("trafficAllocation") List trafficAllocation, - @JsonProperty("cmab") Cmab cmab) { - this(id, key, status, layerId, audienceIds, audienceConditions, variations, userIdToVariationKeyMap, trafficAllocation, "", cmab); + @JsonProperty("cmab") Cmab cmab, + @JsonProperty("type") String type) { + this(id, key, status, layerId, audienceIds, audienceConditions, variations, userIdToVariationKeyMap, trafficAllocation, "", cmab, type); } public Experiment(@Nonnull String id, @@ -117,7 +145,8 @@ public Experiment(@Nonnull String id, @Nonnull Map userIdToVariationKeyMap, @Nonnull List trafficAllocation, @Nonnull String groupId, - @Nullable Cmab cmab) { + @Nullable Cmab cmab, + @Nullable String type) { this.id = id; this.key = key; this.status = status == null ? ExperimentStatus.NOT_STARTED.toString() : status; @@ -131,6 +160,7 @@ public Experiment(@Nonnull String id, this.variationKeyToVariationMap = ProjectConfigUtils.generateNameMapping(variations); this.variationIdToVariationMap = ProjectConfigUtils.generateIdMapping(variations); this.cmab = cmab; + this.type = type; } public String getId() { @@ -181,6 +211,11 @@ public String getGroupId() { return groupId; } + @Nullable + public String getType() { + return type; + } + public Cmab getCmab() { return cmab; } @@ -211,6 +246,7 @@ public String toString() { ", variationKeyToVariationMap=" + variationKeyToVariationMap + ", userIdToVariationKeyMap=" + userIdToVariationKeyMap + ", trafficAllocation=" + trafficAllocation + + ", type='" + type + '\'' + ", cmab=" + cmab + '}'; } diff --git a/core-api/src/main/java/com/optimizely/ab/config/Group.java b/core-api/src/main/java/com/optimizely/ab/config/Group.java index d0d9ff364..1e8cefbd7 100644 --- a/core-api/src/main/java/com/optimizely/ab/config/Group.java +++ b/core-api/src/main/java/com/optimizely/ab/config/Group.java @@ -63,7 +63,8 @@ public Group(@JsonProperty("id") String id, experiment.getUserIdToVariationKeyMap(), experiment.getTrafficAllocation(), id, - experiment.getCmab() + experiment.getCmab(), + experiment.getType() ); } this.experiments.add(experiment); diff --git a/core-api/src/main/java/com/optimizely/ab/config/parser/DatafileGsonDeserializer.java b/core-api/src/main/java/com/optimizely/ab/config/parser/DatafileGsonDeserializer.java index 99b06c447..767fe428b 100644 --- a/core-api/src/main/java/com/optimizely/ab/config/parser/DatafileGsonDeserializer.java +++ b/core-api/src/main/java/com/optimizely/ab/config/parser/DatafileGsonDeserializer.java @@ -27,8 +27,7 @@ import com.optimizely.ab.config.audience.TypedAudience; import java.lang.reflect.Type; -import java.util.Collections; -import java.util.List; +import java.util.*; /** * GSON {@link DatafileProjectConfig} deserializer to allow the constructor to be used. @@ -127,6 +126,28 @@ public ProjectConfig deserialize(JsonElement json, Type typeOfT, JsonDeserializa region = jsonObject.get("region").getAsString(); } + // Validate experiment types + Set validExperimentTypes = new HashSet<>(Arrays.asList( + Experiment.TYPE_AB, Experiment.TYPE_MAB, Experiment.TYPE_CMAB, + Experiment.TYPE_TD, Experiment.TYPE_FR + )); + for (Experiment experiment : experiments) { + if (experiment.getType() != null && !validExperimentTypes.contains(experiment.getType())) { + throw new JsonParseException( + String.format("Experiment \"%s\" has invalid type \"%s\". Valid types: %s.", + experiment.getKey(), experiment.getType(), validExperimentTypes)); + } + } + for (Group group : groups) { + for (Experiment experiment : group.getExperiments()) { + if (experiment.getType() != null && !validExperimentTypes.contains(experiment.getType())) { + throw new JsonParseException( + String.format("Experiment \"%s\" has invalid type \"%s\". Valid types: %s.", + experiment.getKey(), experiment.getType(), validExperimentTypes)); + } + } + } + return new DatafileProjectConfig( accountId, anonymizeIP, diff --git a/core-api/src/main/java/com/optimizely/ab/config/parser/DatafileJacksonDeserializer.java b/core-api/src/main/java/com/optimizely/ab/config/parser/DatafileJacksonDeserializer.java index 2dfc60b24..4cdbbb483 100644 --- a/core-api/src/main/java/com/optimizely/ab/config/parser/DatafileJacksonDeserializer.java +++ b/core-api/src/main/java/com/optimizely/ab/config/parser/DatafileJacksonDeserializer.java @@ -26,8 +26,7 @@ import com.optimizely.ab.config.audience.TypedAudience; import java.io.IOException; -import java.util.Collections; -import java.util.List; +import java.util.*; class DatafileJacksonDeserializer extends JsonDeserializer { @Override @@ -101,6 +100,28 @@ public DatafileProjectConfig deserialize(JsonParser parser, DeserializationConte region = node.get("region").textValue(); } + // Validate experiment types + Set validExperimentTypes = new HashSet<>(Arrays.asList( + Experiment.TYPE_AB, Experiment.TYPE_MAB, Experiment.TYPE_CMAB, + Experiment.TYPE_TD, Experiment.TYPE_FR + )); + for (Experiment experiment : experiments) { + if (experiment.getType() != null && !validExperimentTypes.contains(experiment.getType())) { + throw new IOException( + String.format("Experiment \"%s\" has invalid type \"%s\". Valid types: %s.", + experiment.getKey(), experiment.getType(), validExperimentTypes)); + } + } + for (Group group : groups) { + for (Experiment experiment : group.getExperiments()) { + if (experiment.getType() != null && !validExperimentTypes.contains(experiment.getType())) { + throw new IOException( + String.format("Experiment \"%s\" has invalid type \"%s\". Valid types: %s.", + experiment.getKey(), experiment.getType(), validExperimentTypes)); + } + } + } + return new DatafileProjectConfig( accountId, anonymizeIP, diff --git a/core-api/src/main/java/com/optimizely/ab/config/parser/GsonHelpers.java b/core-api/src/main/java/com/optimizely/ab/config/parser/GsonHelpers.java index 624f9f159..cfebbd9c8 100644 --- a/core-api/src/main/java/com/optimizely/ab/config/parser/GsonHelpers.java +++ b/core-api/src/main/java/com/optimizely/ab/config/parser/GsonHelpers.java @@ -168,8 +168,16 @@ static Experiment parseExperiment(JsonObject experimentJson, String groupId, Jso } } + String type = null; + if (experimentJson.has("type")) { + JsonElement typeElement = experimentJson.get("type"); + if (!typeElement.isJsonNull()) { + type = typeElement.getAsString(); + } + } + return new Experiment(id, key, status, layerId, audienceIds, conditions, variations, userIdToVariationKeyMap, - trafficAllocations, groupId, cmab); + trafficAllocations, groupId, cmab, type); } static Experiment parseExperiment(JsonObject experimentJson, JsonDeserializationContext context) { diff --git a/core-api/src/main/java/com/optimizely/ab/config/parser/JsonConfigParser.java b/core-api/src/main/java/com/optimizely/ab/config/parser/JsonConfigParser.java index 10ca9685f..96a5d1c58 100644 --- a/core-api/src/main/java/com/optimizely/ab/config/parser/JsonConfigParser.java +++ b/core-api/src/main/java/com/optimizely/ab/config/parser/JsonConfigParser.java @@ -105,6 +105,28 @@ public ProjectConfig parseProjectConfig(@Nonnull String json) throws ConfigParse String regionString = rootObject.getString("region"); } + // Validate experiment types + Set validExperimentTypes = new HashSet<>(Arrays.asList( + Experiment.TYPE_AB, Experiment.TYPE_MAB, Experiment.TYPE_CMAB, + Experiment.TYPE_TD, Experiment.TYPE_FR + )); + for (Experiment experiment : experiments) { + if (experiment.getType() != null && !validExperimentTypes.contains(experiment.getType())) { + throw new ConfigParseException( + String.format("Experiment \"%s\" has invalid type \"%s\". Valid types: %s.", + experiment.getKey(), experiment.getType(), validExperimentTypes)); + } + } + for (Group group : groups) { + for (Experiment experiment : group.getExperiments()) { + if (experiment.getType() != null && !validExperimentTypes.contains(experiment.getType())) { + throw new ConfigParseException( + String.format("Experiment \"%s\" has invalid type \"%s\". Valid types: %s.", + experiment.getKey(), experiment.getType(), validExperimentTypes)); + } + } + } + return new DatafileProjectConfig( accountId, anonymizeIP, @@ -127,6 +149,8 @@ public ProjectConfig parseProjectConfig(@Nonnull String json) throws ConfigParse rollouts, integrations ); + } catch (ConfigParseException e) { + throw e; } catch (RuntimeException e) { throw new ConfigParseException("Unable to parse datafile: " + json, e); } catch (Exception e) { @@ -179,8 +203,10 @@ private List parseExperiments(JSONArray experimentJson, String group cmab = parseCmab(cmabObject); } + String type = experimentObject.optString("type", null); + experiments.add(new Experiment(id, key, status, layerId, audienceIds, conditions, variations, userIdToVariationKeyMap, - trafficAllocations, groupId, cmab)); + trafficAllocations, groupId, cmab, type)); } return experiments; diff --git a/core-api/src/main/java/com/optimizely/ab/config/parser/JsonSimpleConfigParser.java b/core-api/src/main/java/com/optimizely/ab/config/parser/JsonSimpleConfigParser.java index 56215acc3..2fda0344a 100644 --- a/core-api/src/main/java/com/optimizely/ab/config/parser/JsonSimpleConfigParser.java +++ b/core-api/src/main/java/com/optimizely/ab/config/parser/JsonSimpleConfigParser.java @@ -108,6 +108,28 @@ public ProjectConfig parseProjectConfig(@Nonnull String json) throws ConfigParse String regionString = (String) rootObject.get("region"); } + // Validate experiment types + Set validExperimentTypes = new HashSet<>(Arrays.asList( + Experiment.TYPE_AB, Experiment.TYPE_MAB, Experiment.TYPE_CMAB, + Experiment.TYPE_TD, Experiment.TYPE_FR + )); + for (Experiment experiment : experiments) { + if (experiment.getType() != null && !validExperimentTypes.contains(experiment.getType())) { + throw new ConfigParseException( + String.format("Experiment \"%s\" has invalid type \"%s\". Valid types: %s.", + experiment.getKey(), experiment.getType(), validExperimentTypes)); + } + } + for (Group group : groups) { + for (Experiment experiment : group.getExperiments()) { + if (experiment.getType() != null && !validExperimentTypes.contains(experiment.getType())) { + throw new ConfigParseException( + String.format("Experiment \"%s\" has invalid type \"%s\". Valid types: %s.", + experiment.getKey(), experiment.getType(), validExperimentTypes)); + } + } + } + return new DatafileProjectConfig( accountId, anonymizeIP, @@ -130,6 +152,8 @@ public ProjectConfig parseProjectConfig(@Nonnull String json) throws ConfigParse rollouts, integrations ); + } catch (ConfigParseException e) { + throw e; } catch (RuntimeException ex) { throw new ConfigParseException("Unable to parse datafile: " + json, ex); } catch (Exception e) { @@ -189,8 +213,17 @@ private List parseExperiments(JSONArray experimentJson, String group } } - experiments.add(new Experiment(id, key, status, layerId, audienceIds, conditions, variations, - userIdToVariationKeyMap, trafficAllocations, groupId, cmab)); + // Parse type field + String type = null; + if (experimentObject.containsKey("type")) { + Object typeObj = experimentObject.get("type"); + if (typeObj != null) { + type = (String) typeObj; + } + } + + experiments.add(new Experiment(id, key, status, layerId, audienceIds, conditions, variations, + userIdToVariationKeyMap, trafficAllocations, groupId, cmab, type)); } return experiments; diff --git a/core-api/src/test/java/com/optimizely/ab/config/FeatureRolloutConfigTest.java b/core-api/src/test/java/com/optimizely/ab/config/FeatureRolloutConfigTest.java new file mode 100644 index 000000000..fbbda6410 --- /dev/null +++ b/core-api/src/test/java/com/optimizely/ab/config/FeatureRolloutConfigTest.java @@ -0,0 +1,155 @@ +/** + * + * Copyright 2026, Optimizely and contributors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package com.optimizely.ab.config; + +import com.optimizely.ab.config.parser.ConfigParseException; + +import java.io.IOException; +import java.io.InputStream; +import java.nio.charset.StandardCharsets; +import java.util.List; +import java.util.Map; + +import static org.junit.Assert.*; + +import org.junit.Before; +import org.junit.Test; + +/** + * Tests for Feature Rollout support in {@link DatafileProjectConfig}. + */ +public class FeatureRolloutConfigTest { + + private ProjectConfig projectConfig; + + @Before + public void setUp() throws ConfigParseException, IOException { + InputStream is = getClass().getClassLoader().getResourceAsStream("config/feature-rollout-config.json"); + assertNotNull("Test fixture not found", is); + byte[] bytes = is.readAllBytes(); + String datafile = new String(bytes, StandardCharsets.UTF_8); + projectConfig = new DatafileProjectConfig.Builder().withDatafile(datafile).build(); + } + + /** + * Test 1: Backward compatibility - experiments without type field have type=null. + */ + @Test + public void experimentWithoutTypeFieldHasNullType() { + Experiment experiment = projectConfig.getExperimentKeyMapping().get("no_type_experiment"); + assertNotNull("Experiment should exist", experiment); + assertNull("Type should be null for experiments without type field", experiment.getType()); + } + + /** + * Test 2: Core injection - feature_rollout experiments get everyone else variation + * and trafficAllocation (endOfRange=10000) injected. + */ + @Test + public void featureRolloutExperimentGetsEveryoneElseVariationInjected() { + Experiment experiment = projectConfig.getExperimentKeyMapping().get("feature_rollout_experiment"); + assertNotNull("Experiment should exist", experiment); + assertEquals(Experiment.TYPE_FR, experiment.getType()); + + // Should have 2 variations: original + everyone else + assertEquals("Should have 2 variations after injection", 2, experiment.getVariations().size()); + + // Check the injected variation + Variation injectedVariation = experiment.getVariations().get(1); + assertEquals("everyone_else_var", injectedVariation.getId()); + assertEquals("everyone_else_variation", injectedVariation.getKey()); + + // Check the injected traffic allocation + List trafficAllocations = experiment.getTrafficAllocation(); + assertEquals("Should have 2 traffic allocations after injection", 2, trafficAllocations.size()); + TrafficAllocation injectedAllocation = trafficAllocations.get(1); + assertEquals("everyone_else_var", injectedAllocation.getEntityId()); + assertEquals(10000, injectedAllocation.getEndOfRange()); + } + + /** + * Test 3: Variation maps updated - all variation lookup maps contain the injected variation. + */ + @Test + public void variationMapsContainInjectedVariation() { + Experiment experiment = projectConfig.getExperimentKeyMapping().get("feature_rollout_experiment"); + assertNotNull("Experiment should exist", experiment); + + // Check variationKeyToVariationMap + Map keyMap = experiment.getVariationKeyToVariationMap(); + assertTrue("Key map should contain injected variation", + keyMap.containsKey("everyone_else_variation")); + + // Check variationIdToVariationMap + Map idMap = experiment.getVariationIdToVariationMap(); + assertTrue("ID map should contain injected variation", + idMap.containsKey("everyone_else_var")); + } + + /** + * Test 4: Non-rollout unchanged - A/B experiments are not modified by injection logic. + */ + @Test + public void abTestExperimentNotModified() { + Experiment experiment = projectConfig.getExperimentKeyMapping().get("ab_test_experiment"); + assertNotNull("Experiment should exist", experiment); + assertEquals(Experiment.TYPE_AB, experiment.getType()); + + // Should still have exactly 2 original variations + assertEquals("A/B test should keep original 2 variations", 2, experiment.getVariations().size()); + assertEquals("control", experiment.getVariations().get(0).getKey()); + assertEquals("treatment", experiment.getVariations().get(1).getKey()); + + // Should still have exactly 2 original traffic allocations + assertEquals("A/B test should keep original 2 traffic allocations", + 2, experiment.getTrafficAllocation().size()); + } + + /** + * Test 5: No rollout edge case - feature_rollout experiment with empty rolloutId + * does not crash (silent skip). + */ + @Test + public void featureRolloutWithEmptyRolloutIdDoesNotCrash() { + Experiment experiment = projectConfig.getExperimentKeyMapping().get("rollout_no_rollout_id_experiment"); + assertNotNull("Experiment should exist", experiment); + assertEquals(Experiment.TYPE_FR, experiment.getType()); + + // Should keep only original variation since rollout cannot be resolved + assertEquals("Should keep only original variation", 1, experiment.getVariations().size()); + assertEquals("rollout_no_rollout_variation", experiment.getVariations().get(0).getKey()); + } + + /** + * Test 6: Type field parsed - experiments with type field in the datafile + * have the value correctly preserved after config parsing. + */ + @Test + public void typeFieldCorrectlyParsed() { + Experiment rolloutExp = projectConfig.getExperimentKeyMapping().get("feature_rollout_experiment"); + assertNotNull(rolloutExp); + assertEquals(Experiment.TYPE_FR, rolloutExp.getType()); + + Experiment abExp = projectConfig.getExperimentKeyMapping().get("ab_test_experiment"); + assertNotNull(abExp); + assertEquals(Experiment.TYPE_AB, abExp.getType()); + + Experiment noTypeExp = projectConfig.getExperimentKeyMapping().get("no_type_experiment"); + assertNotNull(noTypeExp); + assertNull(noTypeExp.getType()); + } +} diff --git a/core-api/src/test/resources/config/feature-rollout-config.json b/core-api/src/test/resources/config/feature-rollout-config.json new file mode 100644 index 000000000..bbe396516 --- /dev/null +++ b/core-api/src/test/resources/config/feature-rollout-config.json @@ -0,0 +1,213 @@ +{ + "accountId": "12345", + "anonymizeIP": false, + "sendFlagDecisions": true, + "botFiltering": false, + "projectId": "67890", + "revision": "1", + "sdkKey": "FeatureRolloutTest", + "environmentKey": "production", + "version": "4", + "audiences": [], + "typedAudiences": [], + "attributes": [], + "events": [], + "groups": [], + "integrations": [], + "experiments": [ + { + "id": "exp_rollout_1", + "key": "feature_rollout_experiment", + "status": "Running", + "layerId": "layer_1", + "audienceIds": [], + "forcedVariations": {}, + "type": "fr", + "variations": [ + { + "id": "var_rollout_1", + "key": "rollout_variation", + "featureEnabled": true + } + ], + "trafficAllocation": [ + { + "entityId": "var_rollout_1", + "endOfRange": 5000 + } + ] + }, + { + "id": "exp_ab_1", + "key": "ab_test_experiment", + "status": "Running", + "layerId": "layer_2", + "audienceIds": [], + "forcedVariations": {}, + "type": "ab", + "variations": [ + { + "id": "var_ab_1", + "key": "control", + "featureEnabled": false + }, + { + "id": "var_ab_2", + "key": "treatment", + "featureEnabled": true + } + ], + "trafficAllocation": [ + { + "entityId": "var_ab_1", + "endOfRange": 5000 + }, + { + "entityId": "var_ab_2", + "endOfRange": 10000 + } + ] + }, + { + "id": "exp_no_type", + "key": "no_type_experiment", + "status": "Running", + "layerId": "layer_3", + "audienceIds": [], + "forcedVariations": {}, + "variations": [ + { + "id": "var_notype_1", + "key": "variation_1", + "featureEnabled": true + } + ], + "trafficAllocation": [ + { + "entityId": "var_notype_1", + "endOfRange": 10000 + } + ] + }, + { + "id": "exp_rollout_no_rollout_id", + "key": "rollout_no_rollout_id_experiment", + "status": "Running", + "layerId": "layer_4", + "audienceIds": [], + "forcedVariations": {}, + "type": "fr", + "variations": [ + { + "id": "var_no_rollout_1", + "key": "rollout_no_rollout_variation", + "featureEnabled": true + } + ], + "trafficAllocation": [ + { + "entityId": "var_no_rollout_1", + "endOfRange": 5000 + } + ] + } + ], + "featureFlags": [ + { + "id": "flag_1", + "key": "feature_with_rollout", + "rolloutId": "rollout_1", + "experimentIds": ["exp_rollout_1"], + "variables": [] + }, + { + "id": "flag_2", + "key": "feature_with_ab", + "rolloutId": "rollout_2", + "experimentIds": ["exp_ab_1"], + "variables": [] + }, + { + "id": "flag_3", + "key": "feature_no_rollout_id", + "rolloutId": "", + "experimentIds": ["exp_rollout_no_rollout_id"], + "variables": [] + } + ], + "rollouts": [ + { + "id": "rollout_1", + "experiments": [ + { + "id": "rollout_exp_1", + "key": "rollout_rule_1", + "status": "Running", + "layerId": "rollout_layer_1", + "audienceIds": [], + "forcedVariations": {}, + "variations": [ + { + "id": "rollout_var_1", + "key": "rollout_enabled", + "featureEnabled": true + } + ], + "trafficAllocation": [ + { + "entityId": "rollout_var_1", + "endOfRange": 10000 + } + ] + }, + { + "id": "rollout_exp_everyone", + "key": "everyone_else_rule", + "status": "Running", + "layerId": "rollout_layer_everyone", + "audienceIds": [], + "forcedVariations": {}, + "variations": [ + { + "id": "everyone_else_var", + "key": "everyone_else_variation", + "featureEnabled": false + } + ], + "trafficAllocation": [ + { + "entityId": "everyone_else_var", + "endOfRange": 10000 + } + ] + } + ] + }, + { + "id": "rollout_2", + "experiments": [ + { + "id": "rollout_exp_2", + "key": "rollout_rule_2", + "status": "Running", + "layerId": "rollout_layer_2", + "audienceIds": [], + "forcedVariations": {}, + "variations": [ + { + "id": "rollout_var_2", + "key": "rollout_variation_2", + "featureEnabled": true + } + ], + "trafficAllocation": [ + { + "entityId": "rollout_var_2", + "endOfRange": 10000 + } + ] + } + ] + } + ] +}