feat(analytics): add Source discriminated union with Fallback.Reason (SDK-79)#89
Conversation
SelectedVariant now carries two source fields: `variant_source` (local | remote | fallback) and `fallback_reason` (FLAG_NOT_FOUND | MISSING_CONTEXT_KEY | NO_ROLLOUT_MATCH | BACKEND_ERROR | NOT_READY, set only when source is fallback). Three behaviorally distinct outcomes — flag-not-found, no-rollout-match, and missing-context-key — previously all returned the bare fallback. The OpenFeature wrapper collapsed them to FLAG_NOT_FOUND, sending callers chasing the flag name when the real cause was usually a rule miss or absent context. The wrapper now dispatches on fallback_reason and maps each to the spec-correct OpenFeature response. Most notably, NO_ROLLOUT_MATCH becomes `reason: DEFAULT` with no error code instead of FLAG_NOT_FOUND. Constant names align with mixpanel-php for consistency across SDKs. Linear: SDK-79 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
rahul-mixpanel
left a comment
There was a problem hiding this comment.
Four recommendations left as inline comments — one high, one medium, two low priority.
The OpenFeature wrapper short-circuits to PROVIDER_NOT_READY at the top of resolve via the areFlagsReady check, so no producer ever constructs a Source.Fallback with Reason.NOT_READY — the case was dead, same pattern Swift PR #745 / Android PR #981 / Python PR #180 / Ruby PR #153 / Node PR #277 cleaned up. Trivial to add back the moment a real call site needs it. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- Source.Fallback now overrides equals/hashCode so two Fallback(REASON) instances compare structurally — otherwise SelectedVariant.equals() returns false for any two fallbacks with the same reason. - Source subclasses get toString() (Local / Remote / Fallback(REASON)) so SelectedVariant.toString() doesn't print Source\$Fallback@1a2b3c. - Restore @param Javadoc on both 5-arg and 6-arg SelectedVariant constructors. - Clarify isSuccess()'s source-based check vs the old variantKey-based check via a brief Javadoc note. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Thanks for the review — pushed
Mirrored on mixpanel-android (PR #981, commit |
rahul-mixpanel
left a comment
There was a problem hiding this comment.
Core design is solid — the discriminated union, singleton pattern for Local/Remote, and immutable withSource() copy are all well done. Prior high-priority feedback (missing equals/hashCode on Fallback, missing toString()) has been addressed.
Four items remaining for your consideration:
- Medium:
SelectedVariantoverridesequals()but nothashCode()— pre-existing but good to fix now - Medium (carried over):
isSuccess()/isFallback()semantic change still needs explicit sign-off re: downstream impact - Low:
sourcedocumented as "never null" but not enforced;equals()defensively handles null anyway - Low:
Fallback(Reason)constructor doesn't null-guardreason— deferred NPE risk
Addresses review feedback on PR #89: - Add SelectedVariant.hashCode() to match the existing equals() (Java contract; the missing pair was pre-existing in v1.9.0). - Deprecate the 5-arg SelectedVariant constructor. Retained for source compatibility with v1.9.0; new callers use the 6-arg overload for an explicit Source, or the 1-arg constructor for consumer-side fallbacks. - 6-arg constructor: coalesce null source to Source.local() so hashCode() / isSuccess() / isFallback() never depend on a null field. Drops the corresponding defensive null-guard in equals(). - Fallback constructor: throw IllegalArgumentException on a null Reason. SDK-79 explicitly eliminated placeholder reasons; silently defaulting to FLAG_NOT_FOUND would mislabel observability data.
|
Pushed 1. 2.
3. 4. Tests: 172 base + 35 openfeature-provider, all green locally. CI will re-run. |
Confidence Score: 4/5Safe to merge with one logic fix: the broken One code path in src/main/java/com/mixpanel/mixpanelapi/featureflags/provider/LocalFlagsProvider.java — the post-loop fallback reason.
|
| Filename | Overview |
|---|---|
| src/main/java/com/mixpanel/mixpanelapi/featureflags/model/Source.java | New discriminated union; well-structured. Enum is missing NOT_READY (flagged in prior thread). Local/Remote singletons correctly rely on identity equality. |
| src/main/java/com/mixpanel/mixpanelapi/featureflags/model/SelectedVariant.java | Source field added, equals/hashCode updated correctly, withSource() copy pattern is clean. |
| src/main/java/com/mixpanel/mixpanelapi/featureflags/provider/LocalFlagsProvider.java | Most fallback reasons correctly classified, but the post-loop NO_ROLLOUT_MATCH is also returned when a rollout matched and variant assignment failed — that path should be BACKEND_ERROR. |
| src/main/java/com/mixpanel/mixpanelapi/featureflags/provider/RemoteFlagsProvider.java | Now correctly distinguishes FLAG_NOT_FOUND from NO_ROLLOUT_MATCH (null variant_key). Source.remote() tagged on success. |
| src/test/java/com/mixpanel/mixpanelapi/featureflags/provider/RemoteFlagsProviderTest.java | New tests cover the null-variantKey → NO_ROLLOUT_MATCH path and FLAG_NOT_FOUND; existing tests preserved. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[getVariant called] --> B{flag in definitions?}
B -- No --> C[Source.fallback\nFLAG_NOT_FOUND]
B -- Yes --> D{context key present?}
D -- No --> E[Source.fallback\nMISSING_CONTEXT_KEY]
D -- Yes --> F{rollout matches?}
F -- No --> G[Source.fallback\nNO_ROLLOUT_MATCH]
F -- Yes --> H{selectedVariant != null?}
H -- Yes --> I[Source.local\nmatched variant]
H -- No --> J[break → falls to\nNO_ROLLOUT_MATCH ⚠️\nshould be BACKEND_ERROR]
A2[Exception] --> K[Source.fallback\nBACKEND_ERROR]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[getVariant called] --> B{flag in definitions?}
B -- No --> C[Source.fallback\nFLAG_NOT_FOUND]
B -- Yes --> D{context key present?}
D -- No --> E[Source.fallback\nMISSING_CONTEXT_KEY]
D -- Yes --> F{rollout matches?}
F -- No --> G[Source.fallback\nNO_ROLLOUT_MATCH]
F -- Yes --> H{selectedVariant != null?}
H -- Yes --> I[Source.local\nmatched variant]
H -- No --> J[break → falls to\nNO_ROLLOUT_MATCH ⚠️\nshould be BACKEND_ERROR]
A2[Exception] --> K[Source.fallback\nBACKEND_ERROR]
Reviews (2): Last reviewed commit: "fix(flags): remote null variant_key -> N..." | Re-trigger Greptile
…_FOUND When /flags returns the flag key but sets variant_key to null, the flag exists on the server — the user just wasn't assigned a variant (e.g. no rollout matched). Previously the SDK conflated that with the "flag absent from response" case and returned FLAG_NOT_FOUND, which downstream (the upcoming OpenFeature wrapper) would surface as the wrong error code. The truly-missing-key path stays FLAG_NOT_FOUND — the remote SDK still can't distinguish "flag doesn't exist" from "user in no rollout" when the server omits the entry entirely, and the existing comment documents that limitation. Added a test that pins the new NO_ROLLOUT_MATCH mapping for the null-variant-key path, and tightened the existing testReturnFallbackWhenFlagNotFoundInSuccessfulResponse to assert Source.Fallback.Reason.FLAG_NOT_FOUND for the missing-key path. All 173 tests pass.
|
Pushed @rahul-mixpanel — @rahul-mixpanel — @rahul-mixpanel — @greptile-apps — @greptile-apps — Full suite: 173 tests pass ( |
|
All five fixes look correct. Quick verification of each:
One small nit: the 1-arg constructor hardcodes Everything else is in good shape. LGTM. ✅ Tip: You can customize Greptile's behavior for this repo with |
|
Ack on the Every current provider path ( Worth revisiting only if a future code path ever returns the caller's fallback object without routing through a provider — at that point the fix belongs across all SDKs at once, not just Java. |
Summary
Sourcediscriminated union (com.mixpanel.mixpanelapi.featureflags.model.Source):Source.Local|Source.Remote|Source.Fallback(Reason). Abstract class + nested static finals — Java 8 source level so nosealedkeyword.Source.Fallback.Reasonenum:FLAG_NOT_FOUND|MISSING_CONTEXT_KEY|NO_ROLLOUT_MATCH|BACKEND_ERROR. Constants align with mixpanel-php.SelectedVariant.getSource()replaces the priorvariantSourcestring field.isFallback()now checkssource instanceof Source.Fallback.LocalFlagsProviderandRemoteFlagsProvidertag every returned variant: matches withSource.local()/Source.remote(), fallbacks withSource.fallback(reason)carrying the specific reason.RemoteFlagsProvidernow surfacesNO_ROLLOUT_MATCH(notFLAG_NOT_FOUND) when the server includes the flag key but setsvariant_keytonull— the flag exists, the user just wasn't assigned a variant.Why
Three behaviorally distinct outcomes — flag-not-found, no-rollout-match, and missing-context-key — previously all returned the bare fallback. A future OpenFeature wrapper (in
openfeature-provider/) can now dispatch on the reason and map each to the spec-correct error code instead of collapsing them all toFLAG_NOT_FOUND.Discriminated union rather than two flat fields because the language is type-safe enough to model it that way; invalid states like "successful evaluation with a fallback reason" are unrepresentable.
Cross-SDK fix tracked at Linear SDK-79. Constant set matches
mixpanel-phpso callers across SDKs see the same vocabulary.NOT_READYis intentionally NOT in the Java enum — the OpenFeature wrapper short-circuits toPROVIDER_NOT_READYviaareFlagsReady()before invoking the provider, so no producer would ever construct that reason. Same cleanup landed across the Python / Ruby / Node / Go PRs.Follow-up PR
The
openfeature-providermodule needs a matching update to dispatch onSource.Fallback.Reason— that PR depends on this one shipping in a newmixpanel-javarelease first (the provider consumes the published Maven coordinate). It'll come right after release.Test plan
mvn test -Dgpg.skip=true)LocalFlagsProviderTest(53) andRemoteFlagsProviderTest(9, +1 for theNO_ROLLOUT_MATCHnull-variant-key path) continue passing — theisFallback()contract is preservedSelectedVariant.equals()updated to includesource🤖 Generated with Claude Code