Skip to content

[FSSDK-11373] add holdout support and refactor decision logic in DefaultDecisionService #587

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

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
50 changes: 50 additions & 0 deletions OptimizelySwiftSDK.xcodeproj/project.pbxproj

Large diffs are not rendered by default.

2 changes: 0 additions & 2 deletions Sources/Data Model/FeatureFlag.swift
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,6 @@ struct FeatureFlag: Codable, Equatable, OptimizelyFeature {
case variables
}

// var holdoutIds: [String] = []

// MARK: - OptimizelyConfig

var experimentsMap: [String: OptimizelyExperiment] = [:]
Expand Down
1 change: 0 additions & 1 deletion Sources/Data Model/HoldoutConfig.swift
Original file line number Diff line number Diff line change
Expand Up @@ -115,4 +115,3 @@ struct HoldoutConfig {
return holdoutIdMap[id]
}
}

4 changes: 2 additions & 2 deletions Sources/Implementation/DecisionInfo.swift
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ struct DecisionInfo {
let decisionType: Constants.DecisionType

/// The experiment that the decision variation belongs to.
var experiment: Experiment?
var experiment: ExperimentCore?

/// The variation selected by the decision.
var variation: Variation?
Expand Down Expand Up @@ -58,7 +58,7 @@ struct DecisionInfo {
var decisionEventDispatched: Bool

init(decisionType: Constants.DecisionType,
experiment: Experiment? = nil,
experiment: ExperimentCore? = nil,
variation: Variation? = nil,
source: String? = nil,
feature: FeatureFlag? = nil,
Expand Down
2 changes: 1 addition & 1 deletion Sources/Implementation/DefaultBucketer.swift
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ class DefaultBucketer: OPTBucketer {
return DecisionResponse(result: nil, reasons: reasons)
}

func bucketToVariation(experiment: Experiment,
func bucketToVariation(experiment: ExperimentCore,
bucketingId: String) -> DecisionResponse<Variation> {
let reasons = DecisionReasons()

Expand Down
356 changes: 271 additions & 85 deletions Sources/Implementation/DefaultDecisionService.swift

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion Sources/Implementation/Events/BatchEventBuilder.swift
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ class BatchEventBuilder {
// MARK: - Impression Event

static func createImpressionEvent(config: ProjectConfig,
experiment: Experiment?,
experiment: ExperimentCore?,
variation: Variation?,
userId: String,
attributes: OptimizelyAttributes?,
Expand Down
4 changes: 2 additions & 2 deletions Sources/Optimizely/OptimizelyClient.swift
Original file line number Diff line number Diff line change
Expand Up @@ -804,7 +804,7 @@ extension OptimizelyClient {
return (source == Constants.DecisionSource.featureTest.rawValue && decision?.variation != nil) || config.sendFlagDecisions
}

func sendImpressionEvent(experiment: Experiment?,
func sendImpressionEvent(experiment: ExperimentCore?,
variation: Variation?,
userId: String,
attributes: OptimizelyAttributes? = nil,
Expand Down Expand Up @@ -892,7 +892,7 @@ extension OptimizelyClient {

extension OptimizelyClient {

func sendActivateNotification(experiment: Experiment,
func sendActivateNotification(experiment: ExperimentCore,
variation: Variation,
userId: String,
attributes: OptimizelyAttributes?,
Expand Down
9 changes: 9 additions & 0 deletions Sources/Protocols/OPTBucketer.swift
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,15 @@ protocol OPTBucketer {
func bucketExperiment(config: ProjectConfig,
experiment: Experiment,
bucketingId: String) -> DecisionResponse<Variation>

/**
Bucket a bucketingId into an experiment.
- Parameter experiment: The rule in which to bucket the bucketingId.
- Parameter bucketingId: The ID to bucket. This must be a non-null, non-empty string.
- Returns: The variation the bucketingId was bucketed into.
*/
func bucketToVariation(experiment: ExperimentCore,
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we adding new signature to this public protocol? This may break existing custom implementations on client side.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right that adding a new method signature could potentially break clients' custom implementations. To mitigate this, we could consider either making the method optional or removing the new signature altogether.

Which approach do you think would better align with our clients' needs and use cases?

bucketingId: String) -> DecisionResponse<Variation>

/**
Hash the bucketing ID and map it to the range [0, 10000).
Expand Down
1 change: 1 addition & 0 deletions Sources/Utils/Constants.swift
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ struct Constants {
case experiment = "experiment"
case featureTest = "feature-test"
case rollout = "rollout"
case holdout = "holdout"
}

struct DecisionInfoKeys {
Expand Down
18 changes: 14 additions & 4 deletions Sources/Utils/LogMessage.swift
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import Foundation

enum LogMessage {
case experimentNotRunning(_ key: String)
case holdoutNotRunning(_ key: String)
case featureEnabledForUser(_ key: String, _ userId: String)
case featureNotEnabledForUser(_ key: String, _ userId: String)
case featureHasNoExperiments(_ key: String)
Expand All @@ -34,7 +35,9 @@ enum LogMessage {
case userAssignedToBucketValue(_ bucket: Int, _ userId: String)
case userMappedToForcedVariation(_ userId: String, _ expId: String, _ varId: String)
case userMeetsConditionsForTargetingRule(_ userId: String, _ rule: String)
case userMeetsConditionsForHoldout(_ userId: String, _ holdoutKey: String)
case userDoesntMeetConditionsForTargetingRule(_ userId: String, _ rule: String)
case userDoesntMeetConditionsForHoldout(_ userId: String, _ holdoutKey: String)
case userBucketedIntoTargetingRule(_ userId: String, _ rule: String)
case userNotBucketedIntoTargetingRule(_ userId: String, _ rule: String)
case userHasForcedDecision(_ userId: String, _ flagKey: String, _ ruleKey: String?, _ varKey: String)
Expand All @@ -44,8 +47,10 @@ enum LogMessage {
case userHasNoForcedVariation(_ userId: String)
case userHasNoForcedVariationForExperiment(_ userId: String, _ expKey: String)
case userBucketedIntoVariationInExperiment(_ userId: String, _ expKey: String, _ varKey: String)
case userBucketedIntoVariationInHoldout(_ userId: String, _ expKey: String, _ varKey: String)
case userNotBucketedIntoVariation(_ userId: String)
case userBucketedIntoInvalidVariation(_ id: String)
case userNotBucketedIntoHoldoutVariation(_ userId: String)
case userBucketedIntoExperimentInGroup(_ userId: String, _ expKey: String, _ group: String)
case userNotBucketedIntoExperimentInGroup(_ userId: String, _ expKey: String, _ group: String)
case userNotBucketedIntoAnyExperimentInGroup(_ userId: String, _ group: String)
Expand Down Expand Up @@ -76,6 +81,7 @@ extension LogMessage: CustomStringConvertible {

switch self {
case .experimentNotRunning(let key): message = "Experiment (\(key)) is not running."
case .holdoutNotRunning(let key): message = "Holdout (\(key)) is not running."
case .featureEnabledForUser(let key, let userId): message = "Feature (\(key)) is enabled for user (\(userId))."
case .featureNotEnabledForUser(let key, let userId): message = "Feature (\(key)) is not enabled for user (\(userId))."
case .featureHasNoExperiments(let key): message = "Feature (\(key)) is not attached to any experiments."
Expand All @@ -91,10 +97,12 @@ extension LogMessage: CustomStringConvertible {
case .savedVariationInUserProfile(let varId, let expId, let userId): message = "Saved variation (\(varId)) of experiment (\(expId)) for user (\(userId))."
case .userAssignedToBucketValue(let bucket, let userId): message = "Assigned bucket (\(bucket)) to user with bucketing ID (\(userId))."
case .userMappedToForcedVariation(let userId, let expId, let varId): message = "Set variation (\(varId)) for experiment (\(expId)) and user (\(userId)) in the forced variation map."
case .userMeetsConditionsForTargetingRule(let userId, let rule): message = "User (\(userId)) meets conditions for targeting rule (\(rule))."
case .userDoesntMeetConditionsForTargetingRule(let userId, let rule): message = "User (\(userId)) does not meet conditions for targeting rule (\(rule))."
case .userBucketedIntoTargetingRule(let userId, let rule): message = "User (\(userId)) is in the traffic group of targeting rule (\(rule))."
case .userNotBucketedIntoTargetingRule(let userId, let rule): message = "User (\(userId)) is not in the traffic group for targeting rule (\(rule)). Checking (Everyone Else) rule now."
case .userMeetsConditionsForTargetingRule(let userId, let rule): message = "User (\(userId)) meets conditions for targeting rule (\(rule))."
case .userMeetsConditionsForHoldout(let userId, let holdoutKey): message = "User (\(userId)) meets conditions for holdout(\(holdoutKey))."
case .userDoesntMeetConditionsForTargetingRule(let userId, let rule): message = "User (\(userId)) does not meet conditions for targeting rule (\(rule))."
case .userDoesntMeetConditionsForHoldout(let userId, let holdoutKey): message = "User (\(userId)) does not meet conditions for holdout (\(holdoutKey))."
case .userBucketedIntoTargetingRule(let userId, let rule): message = "User (\(userId)) is in the traffic group of targeting rule (\(rule))."
case .userNotBucketedIntoTargetingRule(let userId, let rule): message = "User (\(userId)) is not in the traffic group for targeting rule (\(rule)). Checking (Everyone Else) rule now."
case .userHasForcedDecision(let userId, let flagKey, let ruleKey, let varKey):
let target = (ruleKey != nil) ? "flag (\(flagKey)), rule (\(ruleKey!))" : "flag (\(flagKey))"
message = "Variation (\(varKey)) is mapped to \(target) and user (\(userId)) in the forced decision map."
Expand All @@ -106,7 +114,9 @@ extension LogMessage: CustomStringConvertible {
case .userHasNoForcedVariation(let userId): message = "User (\(userId)) is not in the forced variation map."
case .userHasNoForcedVariationForExperiment(let userId, let expKey): message = "No experiment (\(expKey)) mapped to user (\(userId)) in the forced variation map."
case .userBucketedIntoVariationInExperiment(let userId, let expKey, let varKey): message = "User (\(userId)) is in variation (\(varKey)) of experiment (\(expKey))"
case .userBucketedIntoVariationInHoldout(let userId, let holdoutKey, let varKey): message = "User (\(userId)) is in variation (\(varKey)) of holdout (\(holdoutKey))"
case .userNotBucketedIntoVariation(let userId): message = "User (\(userId)) is in no variation."
case .userNotBucketedIntoHoldoutVariation(let userId): message = "User (\(userId)) is in no holdout variation."
case .userBucketedIntoInvalidVariation(let id): message = "Bucketed into an invalid variation id (\(id))"
case .userBucketedIntoExperimentInGroup(let userId, let expId, let group): message = "User (\(userId)) is in experiment (\(expId)) of group (\(group))."
case .userNotBucketedIntoExperimentInGroup(let userId, let expKey, let group): message = "User (\(userId)) is not in experiment (\(expKey)) of group (\(group))."
Expand Down
143 changes: 143 additions & 0 deletions Tests/OptimizelyTests-Common/BucketTests_HoldoutToVariation.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,143 @@
//
// Copyright 2019, 2021, Optimizely, Inc. 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.
//

import XCTest

class BucketTests_HoldoutToVariation: XCTestCase {
var optimizely: OptimizelyClient!
var config: ProjectConfig!
var bucketer: DefaultBucketer!

var kUserId = "123456"
var kHoldoutId = "4444444"
var kHoldoutKey = "holdout_key"

var kVariationKeyA = "a"
var kVariationIdA = "a11"

var kAudienceIdCountry = "10"
var kAudienceIdAge = "20"
var kAudienceIdInvalid = "9999999"

var kAttributesCountryMatch: [String: Any] = ["country": "us"]
var kAttributesCountryNotMatch: [String: Any] = ["country": "ca"]
var kAttributesAgeMatch: [String: Any] = ["age": 30]
var kAttributesAgeNotMatch: [String: Any] = ["age": 10]
var kAttributesEmpty: [String: Any] = [:]

var holdout: Holdout!

// MARK: - Sample datafile data

var sampleHoldoutData: [String: Any] {
return [
"status": "Running",
"id": kHoldoutId,
"key": kHoldoutKey,
"layerId": "10420273888",
"trafficAllocation": [
["entityId": kVariationIdA, "endOfRange": 1000] // 10% traffic allocation (0-1000 out of 10000)
],
"audienceIds": [kAudienceIdCountry],
"variations": [
["variables": [], "id": kVariationIdA, "key": kVariationKeyA]
],
]
}

// MARK: - Setup

override func setUp() {
super.setUp()

self.optimizely = OTUtils.createOptimizely(datafileName: "empty_datafile",
clearUserProfileService: true)
self.config = self.optimizely.config!
self.bucketer = ((optimizely.decisionService as! DefaultDecisionService).bucketer as! DefaultBucketer)

// Initialize holdout
holdout = try! OTUtils.model(from: sampleHoldoutData)
}

// MARK: - Tests for bucketToVariation

func testBucketToVariation_ValidBucketingWithinAllocation() {
// Test users that should bucket into the single variation (within 0-1000 range)
let testCases = [
["userId": "user1", "expectedVariation": kVariationKeyA], // Buckets to variation A
["userId": "testuser", "expectedVariation": kVariationKeyA] // Buckets to variation A
]

for (index, test) in testCases.enumerated() {
// Mock bucket value to ensure it falls within 0-1000
let mockBucketValue = 500 // Within 10% allocation
let mockBucketer = MockBucketer(mockBucketValue: mockBucketValue)
let response = mockBucketer.bucketToVariation(experiment: holdout, bucketingId: test["userId"]!)
XCTAssertNotNil(response.result, "Variation should not be nil for test case \(index)")
XCTAssertEqual(response.result?.key, test["expectedVariation"], "Wrong variation for test case \(index)")
}
}

func testBucketToVariation_BucketValueOutsideAllocation() {
// Test users that fall outside the 10% allocation (bucket value > 1000)
let testCases = [
["userId": "user2"],
["userId": "anotheruser"]
]

for (index, test) in testCases.enumerated() {
// Mock bucket value to ensure it falls outside 0-1000
let mockBucketValue = 1500 // Outside 10% allocation
let mockBucketer = MockBucketer(mockBucketValue: mockBucketValue)
let response = mockBucketer.bucketToVariation(experiment: holdout, bucketingId: test["userId"]!)
XCTAssertNil(response.result, "Variation should be nil for test case \(index) when outside allocation")
}
}

func testBucketToVariation_NoTrafficAllocation() {
// Create a holdout with empty traffic allocation
var modifiedHoldoutData = sampleHoldoutData
modifiedHoldoutData["trafficAllocation"] = []
let modifiedHoldout = try! OTUtils.model(from: modifiedHoldoutData) as Holdout

let response = bucketer.bucketToVariation(experiment: modifiedHoldout, bucketingId: kUserId)

XCTAssertNil(response.result, "Variation should be nil when no traffic allocation")
}

func testBucketToVariation_InvalidVariationId() {
// Create a holdout with invalid variation ID in traffic allocation
var modifiedHoldoutData = sampleHoldoutData
modifiedHoldoutData["trafficAllocation"] = [
["entityId": "invalid_variation_id", "endOfRange": 1000]
]
let modifiedHoldout = try! OTUtils.model(from: modifiedHoldoutData) as Holdout

let response = bucketer.bucketToVariation(experiment: modifiedHoldout, bucketingId: kUserId)

XCTAssertNil(response.result, "Variation should be nil for invalid variation ID")
}

func testBucketToVariation_EmptyBucketingId() {
// Test with empty bucketing ID, still within allocation
let mockBucketValue = 500
let mockBucketer = MockBucketer(mockBucketValue: mockBucketValue)
let response = mockBucketer.bucketToVariation(experiment: holdout, bucketingId: "")

XCTAssertNotNil(response.result, "Should still bucket with empty bucketing ID")
XCTAssertEqual(response.result?.key, kVariationKeyA, "Should bucket to variation A")
}
}
2 changes: 1 addition & 1 deletion Tests/OptimizelyTests-Common/DecisionListenerTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -1263,7 +1263,7 @@ class FakeDecisionService: DefaultDecisionService {
return DecisionResponse.responseNoReasons(result: featureDecision)
}

override func getVariationForFeatureExperiment(config: ProjectConfig, featureFlag: FeatureFlag, user: OptimizelyUserContext, userProfileTracker: UserProfileTracker? = nil, options: [OptimizelyDecideOption]? = nil) -> DecisionResponse<FeatureDecision> {
override func getVariationForFeatureExperiments(config: ProjectConfig, featureFlag: FeatureFlag, user: OptimizelyUserContext, userProfileTracker: UserProfileTracker? = nil, options: [OptimizelyDecideOption]? = nil) -> DecisionResponse<FeatureDecision> {
guard let experiment = self.experiment, let tmpVariation = self.variation else {
return DecisionResponse.nilNoReasons()
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -258,7 +258,7 @@ class DecisionServiceTests_Features: XCTestCase {
extension DecisionServiceTests_Features {

func testGetVariationForFeatureExperimentWhenMatched() {
let pair = self.decisionService.getVariationForFeatureExperiment(config: config,
let pair = self.decisionService.getVariationForFeatureExperiments(config: config,
featureFlag: featureFlag,
user: optimizely.createUserContext(userId: kUserId,
attributes: kAttributesCountryMatch)).result
Expand All @@ -268,7 +268,7 @@ extension DecisionServiceTests_Features {
}

func testGetVariationForFeatureExperimentWhenNotMatched() {
let pair = self.decisionService.getVariationForFeatureExperiment(config: config,
let pair = self.decisionService.getVariationForFeatureExperiments(config: config,
featureFlag: featureFlag,
user: optimizely.createUserContext(userId: kUserId,
attributes: kAttributesCountryNotMatch)).result
Expand All @@ -280,7 +280,7 @@ extension DecisionServiceTests_Features {
featureFlag.experimentIds = ["99999"] // not-existing experiment
self.config.project.featureFlags = [featureFlag]

let pair = self.decisionService.getVariationForFeatureExperiment(config: config,
let pair = self.decisionService.getVariationForFeatureExperiments(config: config,
featureFlag: featureFlag,
user: optimizely.createUserContext(userId: kUserId,
attributes: kAttributesCountryMatch)).result
Expand Down
Loading
Loading