[FSSDK-11170] update: decision service methods for cmab #583
Conversation
…e to support CMAB UUID handling
…and include CMAB UUIDs in responses
…ils for CMAB configuration
…ations over CMAB service decisions in DecisionService
…tly verify error state
…izely and DecisionService
…text and related fetcher classes
| @Nullable ODPManager odpManager | ||
| ) { | ||
| @Nullable ODPManager odpManager, | ||
| @Nonnull CmabService cmabService |
There was a problem hiding this comment.
Looks like breaking change, can we make nullable?
There was a problem hiding this comment.
Javascript and all other server side implementations have cmabService as a mandatory field (non null). It shouldn't be breaking.
| private String cmabUUID; | ||
|
|
||
| public DecisionResponse(@Nullable T result, @Nonnull DecisionReasons reasons) { | ||
| public DecisionResponse(@Nullable T result, @Nonnull DecisionReasons reasons, @Nonnull boolean error, @Nullable String cmabUUID) { |
There was a problem hiding this comment.
Does it impact the existing user?
There was a problem hiding this comment.
No, existing users can still use this class as they were. In that case cmabUUID will be set null and error will be false. Class behaviour will be same.
core-api/src/main/java/com/optimizely/ab/optimizelydecision/AsyncDecisionsFetcher.java
Outdated
Show resolved
Hide resolved
…ecision responses
…ent key retrieval
…eation in CMAB client
…hance decision handling
| .replace("\"", "\\\"") | ||
| .replace("\n", "\\n") | ||
| .replace("\r", "\\r") | ||
| .replace("\t", "\\t"); |
There was a problem hiding this comment.
Is there something built-in that does this so we don't have to write our own?
Also, this doesn't cover every legal case for JSON escaping. And don't we escape \ with \? Why three?
There was a problem hiding this comment.
The SDK supports multiple parsers (e.g., Jackson, Gson) but avoids hard-binding to any one to prevent dependency lock-in. Instead, we kept payload building minimal and manual, which ensures flexibility and avoids unnecessary dependencies.
Regarding triple ,
s.replace("\"", "\\""); would create a compilation error because the string literal ends prematurely.
s.replace("\"", "\\\""); is correct (produces the string \")
| String flagKey = flagsWithoutForcedDecision.get(i).getKey(); | ||
|
|
||
| if (error) { | ||
| OptimizelyDecision optimizelyDecision = OptimizelyDecision.newErrorDecision(flagKey, user, DecisionMessage.CMAB_ERROR.reason(experimentKey)); |
There was a problem hiding this comment.
Do we always report CMAB error for any decision errors? Is this safe?
|
|
||
| /** | ||
| * Returns a decision result ({@link OptimizelyDecision}) for a given flag key and a user context, | ||
| * skipping CMAB logic and using only traditional A/B testing. |
There was a problem hiding this comment.
Add more comments about why we need this like -
"This will be called by mobile apps which will use non-blocking legacy ab-tests only (for backward compatibility with android-sdk)"
| @Nonnull List<String> keys, | ||
| @Nonnull List<OptimizelyDecideOption> options, | ||
| boolean ignoreDefaultOptions) { | ||
| Map<String, OptimizelyDecision> decisionMap = new HashMap<>(); |
There was a problem hiding this comment.
We have full separate copies of decideForKeys and decideForKeysSync
It won't be easy to maintain these identical copies with small changes.
Can we consider refactor decideForKeysSync to reuse decideForKeys (or the other way). We can pass "needAsync=True" param to skip async ops (including CMAB).
There was a problem hiding this comment.
Same for decideSync and decideAllSync?
| /** | ||
| * Returns a decision result ({@link OptimizelyDecision}) for a given flag key and a user context, | ||
| * which contains all data required to deliver the flag. This method skips CMAB logic. | ||
| * | ||
| * @param key A flag key for which a decision will be made. | ||
| * @return A decision result. | ||
| */ | ||
| public OptimizelyDecision decideSync(@Nonnull String key) { | ||
| return decideSync(key, Collections.emptyList()); | ||
| } |
There was a problem hiding this comment.
I think we can remove this - this is only for android-sdk and it can call decideSync(key, options) always.
| return optimizely.decideForKeysSync(copy(), keys, options); | ||
| } | ||
|
|
||
| /** |
| /** | ||
| * Returns a decision result asynchronously for a given flag key and a user context. | ||
| * | ||
| * @param key A flag key for which a decision will be made. | ||
| * @param callback A callback to invoke when the decision is available. | ||
| */ | ||
| public void decideAsync(@Nonnull String key, @Nonnull OptimizelyDecisionCallback callback) { | ||
| decideAsync(key, callback, Collections.emptyList()); | ||
| } |
| /** | ||
| * Returns decision results asynchronously for multiple flag keys. | ||
| * | ||
| * @param keys A list of flag keys for which decisions will be made. | ||
| * @param callback A callback to invoke when decisions are available. | ||
| */ | ||
| public void decideForKeysAsync(@Nonnull List<String> keys, @Nonnull OptimizelyDecisionsCallback callback) { | ||
| decideForKeysAsync(keys, callback, Collections.emptyList()); | ||
| } |
| /** | ||
| * Returns decision results asynchronously for all active flag keys. | ||
| * | ||
| * @param callback A callback to invoke when decisions are available. | ||
| */ | ||
| public void decideAllAsync(@Nonnull OptimizelyDecisionsCallback callback) { | ||
| decideAllAsync(callback, Collections.emptyList()); | ||
| } |
| @Nullable UserProfileTracker userProfileTracker, | ||
| @Nullable DecisionReasons reasons) { | ||
| @Nullable DecisionReasons reasons, | ||
| @Nonnull boolean useCmab) { |
There was a problem hiding this comment.
can we change this param useCmab to needAsync (or similar)?
I understand the purpose of this is differentiate sync vs async not specific for cmab.
We can keep more it more generatic for future extension.
| if (decisionMeetAudience.getResult()) { | ||
| String bucketingId = getBucketingId(user.getUserId(), user.getAttributes()); | ||
| String cmabUUID = null; | ||
| if (useCmab && isCmabExperiment(experiment)) { |
There was a problem hiding this comment.
We can consider to bring useCmab (needAsync flag) to decouple from cmab so can use to cover other async ops too.
| * @return A {@link DecisionResponse} including the entity ID ("$" if bucketed to CMAB, null otherwise) and decision reasons | ||
| */ | ||
| @Nonnull | ||
| public DecisionResponse<String> bucketForCmab(@Nonnull Experiment experiment, |
There was a problem hiding this comment.
I do not see if we need this separate bucketing for cmab. We can reuse the current bucket().
| DecisionReasons reasons = DefaultDecisionReasons.newInstance(); | ||
|
|
||
| // Check if user is in CMAB traffic allocation | ||
| DecisionResponse<String> bucketResponse = bucketer.bucketForCmab(experiment, bucketingId, projectConfig); |
There was a problem hiding this comment.
Can't we reuse existing bucket() here?
4 participants
Summary
Decision Service methods to handle CMAB
Test plan
Added unit tests
Issues
FSSDK-11170