diff --git a/.changeset/unresolved-local-flags.md b/.changeset/unresolved-local-flags.md new file mode 100644 index 00000000..5d1518a3 --- /dev/null +++ b/.changeset/unresolved-local-flags.md @@ -0,0 +1,12 @@ +--- +"posthog-go": minor +--- + +Report flags that local evaluation could not resolve on `EvaluateFlags` snapshots. `FeatureFlagEvaluations.UnresolvedFlags()` maps each such flag to an `UnresolvedFlagReason` (`experience_continuity`, `unsupported_definition`, `unresolved_dependency` or `missing_context`), and `InconclusiveMatchError` and `RequiresServerEvaluationError` carry the reason in a new `Reason` field. When definitions load, log one warning per flag that local evaluation can never resolve (experience continuity, a static cohort, an unknown operator, a malformed value, or a missing or circular flag dependency), and again only when its definition changes. + +Behavior changes: + +- A condition with a filter that local evaluation can't check and a filter that does not match now resolves locally as no match. Before, such a flag fell back to a remote request, or stayed unresolved with `OnlyEvaluateLocally`. +- An inactive flag with experience continuity enabled now resolves to `false` locally instead of falling back to a remote request. +- `$feature_flag_called` events from snapshot reads of these flags report `local_evaluation_inconclusive` instead of `flag_missing`. +- The load-time warning is logged with `Logger.Warnf`, so it shows without debug logging. diff --git a/api/public-api.txt b/api/public-api.txt index 8af2eb32..7e1c41f7 100644 --- a/api/public-api.txt +++ b/api/public-api.txt @@ -38,6 +38,8 @@ const ( FeatureFlagErrorFlagMissing = "flag_missing" + FeatureFlagErrorLocalEvaluationInconclusive = "local_evaluation_inconclusive" + FeatureFlagErrorEvaluationFailed = "evaluation_failed" FeatureFlagErrorQuotaLimited = "quota_limited" @@ -551,6 +553,9 @@ func (e *FeatureFlagEvaluations) OnlyAccessed() *FeatureFlagEvaluations +func (e *FeatureFlagEvaluations) UnresolvedFlags() map[string]UnresolvedFlagReason + + type FeatureFlagPayload struct { Key string DistinctId string @@ -783,6 +788,8 @@ type IdentifyInApi struct { type InAppDecider func(frame runtime.Frame) bool type InconclusiveMatchError struct { + + Reason UnresolvedFlagReason } func (e *InconclusiveMatchError) Error() string @@ -852,6 +859,8 @@ type RequestContextOptions struct { } type RequiresServerEvaluationError struct { + + Reason UnresolvedFlagReason } func (e *RequiresServerEvaluationError) Error() string @@ -940,6 +949,14 @@ type Ticker struct { func (t *Ticker) Stop() +type UnresolvedFlagReason string + +const ( + UnresolvedFlagReasonExperienceContinuity UnresolvedFlagReason = "experience_continuity" + UnresolvedFlagReasonUnsupportedDefinition UnresolvedFlagReason = "unsupported_definition" + UnresolvedFlagReasonUnresolvedDependency UnresolvedFlagReason = "unresolved_dependency" + UnresolvedFlagReasonMissingContext UnresolvedFlagReason = "missing_context" +) type Variants struct { Variants []FlagVariant `json:"variants"` } diff --git a/feature_flag_errors.go b/feature_flag_errors.go index 324d1e15..77e1108c 100644 --- a/feature_flag_errors.go +++ b/feature_flag_errors.go @@ -24,6 +24,10 @@ const ( // FeatureFlagErrorFlagMissing indicates the requested flag was not in the API response FeatureFlagErrorFlagMissing = "flag_missing" + // FeatureFlagErrorLocalEvaluationInconclusive indicates the flag has a loaded local + // definition, but neither local evaluation nor a remote fallback resolved it + FeatureFlagErrorLocalEvaluationInconclusive = "local_evaluation_inconclusive" + // FeatureFlagErrorEvaluationFailed indicates the flag was present but failed to evaluate due to a transient server error FeatureFlagErrorEvaluationFailed = "evaluation_failed" diff --git a/feature_flag_evaluations.go b/feature_flag_evaluations.go index cc36a307..1c4014e8 100644 --- a/feature_flag_evaluations.go +++ b/feature_flag_evaluations.go @@ -30,6 +30,10 @@ type FeatureFlagEvaluations struct { // granularity the legacy single-flag path emits. errorsWhileComputing bool quotaLimited bool + // unresolved maps each flag with a loaded local definition that neither + // local evaluation nor the remote fallback resolved to the reason. + // Filtered snapshots leave it empty. + unresolved map[string]UnresolvedFlagReason mu sync.Mutex accessed map[string]struct{} @@ -130,6 +134,24 @@ func (e *FeatureFlagEvaluations) Keys() []string { return keys } +// UnresolvedFlags returns the flags that have a loaded local definition but +// that neither local evaluation nor a remote fallback resolved, mapped to the +// reason local evaluation was inconclusive. These flags are absent from Keys, +// IsEnabled returns false and GetFlag returns nil for them. Reading this does +// not record an access and does not fire an event. The returned map is a copy. +// +// Snapshots returned by Only and OnlyAccessed have no unresolved flags. +func (e *FeatureFlagEvaluations) UnresolvedFlags() map[string]UnresolvedFlagReason { + if e == nil { + return nil + } + unresolved := make(map[string]UnresolvedFlagReason, len(e.unresolved)) + for key, reason := range e.unresolved { + unresolved[key] = reason + } + return unresolved +} + // OnlyAccessed returns a filtered copy of this snapshot containing only the // flags that were accessed via IsEnabled or GetFlag before this call. // @@ -281,8 +303,8 @@ func (e *FeatureFlagEvaluations) recordAccess(key string) { // Build the comma-joined $feature_flag_error matching the single-flag // path's granularity: response-level errors (errors-while-computing, - // quota-limited) are combined with per-flag errors so consumers can - // filter by type. + // quota-limited) are combined with per-flag errors (flag-missing or + // local-evaluation-inconclusive) so consumers can filter by type. var errs []string if e.errorsWhileComputing { errs = append(errs, FeatureFlagErrorErrorsWhileComputing) @@ -291,7 +313,11 @@ func (e *FeatureFlagEvaluations) recordAccess(key string) { errs = append(errs, FeatureFlagErrorQuotaLimited) } if !found { - errs = append(errs, FeatureFlagErrorFlagMissing) + if _, unresolved := e.unresolved[key]; unresolved { + errs = append(errs, FeatureFlagErrorLocalEvaluationInconclusive) + } else { + errs = append(errs, FeatureFlagErrorFlagMissing) + } } else if flag.Error != nil { errs = append(errs, *flag.Error) } @@ -311,7 +337,7 @@ func (e *FeatureFlagEvaluations) recordAccess(key string) { // cloneWith builds a child snapshot with the given flag set. The accessed set // is copied so further access on the child does not leak back into the -// parent's accessed set. +// parent's accessed set. The child carries no unresolved flags. func (e *FeatureFlagEvaluations) cloneWith(flags map[string]evaluatedFlagRecord) *FeatureFlagEvaluations { e.mu.Lock() accessedCopy := make(map[string]struct{}, len(e.accessed)) diff --git a/feature_flag_evaluations_test.go b/feature_flag_evaluations_test.go index 7a965eba..98254102 100644 --- a/feature_flag_evaluations_test.go +++ b/feature_flag_evaluations_test.go @@ -6,6 +6,7 @@ import ( "errors" "net/http" "net/http/httptest" + "strconv" "strings" "sync" "sync/atomic" @@ -13,6 +14,7 @@ import ( "time" json "github.com/goccy/go-json" + "github.com/stretchr/testify/require" ) // flagsServer wraps an httptest server that responds with the v4 flags fixture. @@ -1404,3 +1406,432 @@ func TestCaptureWithFlags_UserPropertiesOverrideGenerated(t *testing.T) { t.Errorf("expected non-overridden $feature/multi-variate-flag to remain 'hello', got %v", event.Properties["$feature/multi-variate-flag"]) } } + +const ( + betaUIDefinition = `{"key": "beta-ui", "active": true, "filters": {"groups": [{"properties": [], "rollout_percentage": 100}]}}` + continuityCheckoutDefinition = `{"key": "checkout", "active": true, "ensure_experience_continuity": true, "filters": {"groups": [{"properties": [], "rollout_percentage": 100}]}}` + remoteCheckoutEnabled = `{"flags": {"checkout": {"key": "checkout", "enabled": true, "metadata": {"id": 1, "version": 1}}}}` + remoteNoFlags = `{"flags": {}}` +) + +func localDefinitions(flags ...string) string { + return localDefinitionsWithCohorts(`{}`, flags...) +} + +func localDefinitionsWithCohorts(cohorts string, flags ...string) string { + return `{"flags": [` + strings.Join(flags, ",") + `], "group_type_mapping": {}, "cohorts": ` + cohorts + `}` +} + +// newUnresolvedFlagsClient serves local definitions and answers /flags with remote, +// or with a 500 when remote is empty. +func newUnresolvedFlagsClient(t *testing.T, definitions, remote string) (Client, *eventCapture, *atomic.Int32) { + t.Helper() + var remoteCalls atomic.Int32 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch { + case strings.HasPrefix(r.URL.Path, "/flags/definitions"): + w.Write([]byte(definitions)) + case strings.HasPrefix(r.URL.Path, "/flags"): + remoteCalls.Add(1) + if remote == "" { + w.WriteHeader(http.StatusInternalServerError) + return + } + w.Write([]byte(remote)) + } + })) + t.Cleanup(server.Close) + + client, capture, _ := newEvalClient(t, server, func(c *Config) { + c.PersonalApiKey = "personal-key" + c.DefaultFeatureFlagsPollingInterval = time.Hour + }) + waitForFlagDefinitions(t, client) + return client, capture, &remoteCalls +} + +func flagCalledError(t *testing.T, client Client, capture *eventCapture, key string) string { + t.Helper() + flushEvaluationEvents(t, client) + event := findEvent(snapshotCapturedEvents(capture), "$feature_flag_called", key) + require.NotNil(t, event, "expected $feature_flag_called for %s", key) + errorString, _ := event.Properties["$feature_flag_error"].(string) + return errorString +} + +func TestEvaluateFlags_LocalOnlyReportsExperienceContinuityFlagAsUnresolved(t *testing.T) { + t.Parallel() + client, capture, remoteCalls := newUnresolvedFlagsClient(t, localDefinitions(betaUIDefinition, continuityCheckoutDefinition), remoteNoFlags) + + snapshot, err := client.EvaluateFlags(EvaluateFlagsPayload{DistinctId: "user-123", OnlyEvaluateLocally: true}) + require.NoError(t, err) + + require.Equal(t, []string{"beta-ui"}, snapshot.Keys()) + require.Equal(t, true, snapshot.flags["beta-ui"].Enabled) + require.Equal(t, map[string]UnresolvedFlagReason{"checkout": UnresolvedFlagReasonExperienceContinuity}, snapshot.UnresolvedFlags()) + require.Zero(t, remoteCalls.Load()) + require.Empty(t, snapshot.OnlyAccessed().Keys()) + flushEvaluationEvents(t, client) + require.Zero(t, countFlagCalledEvents(snapshotCapturedEvents(capture), "checkout")) + require.Zero(t, countFlagCalledEvents(snapshotCapturedEvents(capture), "beta-ui")) + + require.Empty(t, snapshot.Only([]string{"beta-ui", "checkout"}).UnresolvedFlags()) + require.Empty(t, snapshot.OnlyAccessed().UnresolvedFlags()) +} + +func TestEvaluateFlags_EachInconclusiveCauseReportsItsReason(t *testing.T) { + t.Parallel() + const staticCohort = `{"key": "id", "type": "cohort", "value": 404}` + const planIsPro = `{"key": "plan", "type": "person", "operator": "exact", "value": "pro"}` + checkout := func(properties ...string) string { + return `{"key": "checkout", "active": true, "ensure_experience_continuity": false, "filters": {"groups": [{"properties": [` + strings.Join(properties, ",") + `], "rollout_percentage": 100}]}}` + } + // The device-bucketed row of the spec does not apply: without a device id this SDK + // buckets device-bucketed flags by the distinct id, so they resolve locally. + tests := []struct { + name string + definition string + properties Properties + want UnresolvedFlagReason + }{ + {"missing person property", checkout(planIsPro), nil, UnresolvedFlagReasonMissingContext}, + {"unknown operator", checkout(`{"key": "plan", "type": "person", "operator": "is_on_the_moon", "value": "pro"}`), NewProperties().Set("plan", "enterprise"), UnresolvedFlagReasonUnsupportedDefinition}, + {"static cohort", checkout(staticCohort), nil, UnresolvedFlagReasonUnsupportedDefinition}, + {"static cohort and missing person property", checkout(staticCohort, planIsPro), nil, UnresolvedFlagReasonUnsupportedDefinition}, + {"missing person property and static cohort", checkout(planIsPro, staticCohort), nil, UnresolvedFlagReasonUnsupportedDefinition}, + } + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + client, _, _ := newUnresolvedFlagsClient(t, localDefinitions(tt.definition), remoteNoFlags) + + snapshot, err := client.EvaluateFlags(EvaluateFlagsPayload{DistinctId: "user-123", PersonProperties: tt.properties, OnlyEvaluateLocally: true}) + require.NoError(t, err) + + require.Empty(t, snapshot.Keys()) + require.Equal(t, map[string]UnresolvedFlagReason{"checkout": tt.want}, snapshot.UnresolvedFlags()) + }) + } +} + +func TestEvaluateFlags_DependencyReasons(t *testing.T) { + t.Parallel() + const betaUIDependsOnCheckout = `{"key": "beta-ui", "active": true, "filters": {"groups": [{"properties": [{"key": "checkout", "type": "flag", "operator": "flag_evaluates_to", "value": true, "dependency_chain": ["checkout"]}], "rollout_percentage": 100}]}}` + tests := []struct { + name string + definitions string + want UnresolvedFlagReason + }{ + {"dependency is unresolved", localDefinitions(continuityCheckoutDefinition, betaUIDependsOnCheckout), UnresolvedFlagReasonUnresolvedDependency}, + {"dependency has no loaded definition", localDefinitions(betaUIDependsOnCheckout), UnresolvedFlagReasonUnsupportedDefinition}, + } + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + client, _, _ := newUnresolvedFlagsClient(t, tt.definitions, remoteNoFlags) + + snapshot, err := client.EvaluateFlags(EvaluateFlagsPayload{DistinctId: "user-123", FlagKeys: []string{"beta-ui"}, OnlyEvaluateLocally: true}) + require.NoError(t, err) + + require.Empty(t, snapshot.Keys()) + require.Equal(t, map[string]UnresolvedFlagReason{"beta-ui": tt.want}, snapshot.UnresolvedFlags()) + }) + } +} + +func companyCheckoutDefinitions(ensureExperienceContinuity bool, properties ...string) string { + return `{"flags": [{"key": "checkout", "active": true, "ensure_experience_continuity": ` + strconv.FormatBool(ensureExperienceContinuity) + `, + "filters": {"aggregation_group_type_index": 0, "groups": [{"properties": [` + strings.Join(properties, ",") + `], "rollout_percentage": 100}]}}], + "group_type_mapping": {"0": "company"}, "cohorts": {}}` +} + +func TestEvaluateFlags_GroupFlagWithoutGroupKey(t *testing.T) { + t.Parallel() + tests := []struct { + name string + definitions string + groups Groups + localOnly bool + remote string + want interface{} + wantReason UnresolvedFlagReason + wantRemoteCalls int32 + }{ + {"group key passed", companyCheckoutDefinitions(false), Groups{"company": "acme"}, true, remoteNoFlags, true, "", 0}, + {"local only", companyCheckoutDefinitions(false), nil, true, remoteNoFlags, nil, UnresolvedFlagReasonMissingContext, 0}, + {"experience continuity outranks the group key", companyCheckoutDefinitions(true), nil, true, remoteNoFlags, nil, UnresolvedFlagReasonExperienceContinuity, 0}, + {"static cohort outranks the group key", companyCheckoutDefinitions(false, `{"key": "id", "type": "cohort", "value": 404}`), nil, true, remoteNoFlags, nil, UnresolvedFlagReasonUnsupportedDefinition, 0}, + {"remote fallback resolves the flag", companyCheckoutDefinitions(false), nil, false, remoteCheckoutEnabled, true, "", 1}, + {"remote fallback does not return the flag", companyCheckoutDefinitions(false), nil, false, remoteNoFlags, nil, UnresolvedFlagReasonMissingContext, 1}, + } + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + client, capture, remoteCalls := newUnresolvedFlagsClient(t, tt.definitions, tt.remote) + + snapshot, err := client.EvaluateFlags(EvaluateFlagsPayload{DistinctId: "user-123", Groups: tt.groups, OnlyEvaluateLocally: tt.localOnly}) + require.NoError(t, err) + + require.Equal(t, tt.wantRemoteCalls, remoteCalls.Load()) + require.Equal(t, tt.want, snapshot.GetFlag("checkout")) + if tt.wantReason == "" { + require.Empty(t, snapshot.UnresolvedFlags()) + require.Empty(t, flagCalledError(t, client, capture, "checkout")) + return + } + require.Equal(t, map[string]UnresolvedFlagReason{"checkout": tt.wantReason}, snapshot.UnresolvedFlags()) + require.Equal(t, FeatureFlagErrorLocalEvaluationInconclusive, flagCalledError(t, client, capture, "checkout")) + }) + } +} + +// The legacy getters must keep their handling of a group flag without its group key. +func TestLegacyGettersGroupFlagWithoutGroupKey(t *testing.T) { + t.Parallel() + const localError = "[FEATURE FLAGS] Can't compute group feature flag: checkout without group names passed in" + payload := FeatureFlagPayload{Key: "checkout", DistinctId: "user-123"} + t.Run("local only", func(t *testing.T) { + t.Parallel() + client, capture, remoteCalls := newUnresolvedFlagsClient(t, companyCheckoutDefinitions(false), remoteCheckoutEnabled) + localOnly := payload + localOnly.OnlyEvaluateLocally = true + + value, err := client.GetFeatureFlag(localOnly) + require.EqualError(t, err, localError) + require.Nil(t, value) + require.Equal(t, FeatureFlagErrorUnknownError, flagCalledError(t, client, capture, "checkout")) + + enabled, err := client.IsFeatureEnabled(localOnly) + require.EqualError(t, err, localError) + require.Nil(t, enabled) + + flagPayload, err := client.GetFeatureFlagPayload(localOnly) + require.EqualError(t, err, localError) + require.Empty(t, flagPayload) + + all, err := client.GetAllFlags(FeatureFlagPayloadNoKey{DistinctId: "user-123", OnlyEvaluateLocally: true}) + require.NoError(t, err) + require.Empty(t, all) + + require.Zero(t, remoteCalls.Load()) + }) + t.Run("remote fallback", func(t *testing.T) { + t.Parallel() + client, _, remoteCalls := newUnresolvedFlagsClient(t, companyCheckoutDefinitions(false), remoteCheckoutEnabled) + + value, err := client.GetFeatureFlag(payload) + require.NoError(t, err) + require.Equal(t, true, value) + + all, err := client.GetAllFlags(FeatureFlagPayloadNoKey{DistinctId: "user-123"}) + require.NoError(t, err) + require.Equal(t, map[string]interface{}{"checkout": true}, all) + + require.Equal(t, int32(2), remoteCalls.Load()) + }) +} + +func TestEvaluateFlags_DependencyThatNeedsServerForcesRemoteFallback(t *testing.T) { + t.Parallel() + dependency := func(filter string) string { + return `{"key": "dep", "active": true, "filters": {"groups": [{"properties": [` + filter + `], "rollout_percentage": 100}]}}` + } + const target = `{"key": "target", "active": true, "filters": { + "multivariate": {"variants": [{"key": "a", "rollout_percentage": 50}, {"key": "b", "rollout_percentage": 50}]}, + "groups": [ + {"properties": [{"key": "dep", "type": "flag", "value": true, "operator": "flag_evaluates_to", "dependency_chain": ["dep"]}], "rollout_percentage": 100, "variant": "a"}, + {"properties": [], "rollout_percentage": 100, "variant": "b"} + ]}}` + const remoteTargetA = `{"flags": {"target": {"key": "target", "enabled": true, "variant": "a", "metadata": {"id": 2, "version": 1}}}}` + staticCohortDependency := localDefinitions(dependency(`{"key": "id", "type": "cohort", "value": 999}`), target) + propertyDependency := localDefinitions(dependency(`{"key": "plan", "type": "person", "operator": "exact", "value": "pro"}`), target) + tests := []struct { + name string + definitions string + flagKeys []string + localOnly bool + want interface{} + wantReason UnresolvedFlagReason + wantRemoteCalls int32 + }{ + {"target requested alone", staticCohortDependency, []string{"target"}, false, "a", "", 1}, + {"all flags requested", staticCohortDependency, nil, false, "a", "", 1}, + {"local only", staticCohortDependency, []string{"target"}, true, nil, UnresolvedFlagReasonUnresolvedDependency, 0}, + {"dependency resolves locally", propertyDependency, []string{"target"}, false, "a", "", 0}, + } + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + client, _, remoteCalls := newUnresolvedFlagsClient(t, tt.definitions, remoteTargetA) + + snapshot, err := client.EvaluateFlags(EvaluateFlagsPayload{ + DistinctId: "user-123", + FlagKeys: tt.flagKeys, + PersonProperties: NewProperties().Set("plan", "pro"), + OnlyEvaluateLocally: tt.localOnly, + }) + require.NoError(t, err) + + require.Equal(t, tt.wantRemoteCalls, remoteCalls.Load()) + require.Equal(t, tt.want, snapshot.GetFlag("target")) + if tt.wantReason == "" { + require.NotContains(t, snapshot.UnresolvedFlags(), "target") + } else { + require.Equal(t, tt.wantReason, snapshot.UnresolvedFlags()["target"]) + } + }) + } +} + +func TestEvaluateFlags_RemoteFallbackResolvesUnresolvedFlag(t *testing.T) { + t.Parallel() + client, capture, remoteCalls := newUnresolvedFlagsClient(t, localDefinitions(continuityCheckoutDefinition), remoteCheckoutEnabled) + + snapshot, err := client.EvaluateFlags(EvaluateFlagsPayload{DistinctId: "user-123"}) + require.NoError(t, err) + + require.Equal(t, int32(1), remoteCalls.Load()) + require.Equal(t, []string{"checkout"}, snapshot.Keys()) + require.Empty(t, snapshot.UnresolvedFlags()) + + require.True(t, snapshot.IsEnabled("checkout")) + flushEvaluationEvents(t, client) + event := findEvent(snapshotCapturedEvents(capture), "$feature_flag_called", "checkout") + require.NotNil(t, event) + require.Equal(t, true, event.Properties["$feature_flag_response"]) + require.NotContains(t, event.Properties, "$feature_flag_error") +} + +func TestEvaluateFlags_RemoteFallbackThatDoesNotResolveLeavesFlagUnresolved(t *testing.T) { + t.Parallel() + tests := []struct { + name string + remote string + wantErr bool + }{ + {"remote request fails", "", true}, + {"remote returns no flags", remoteNoFlags, false}, + } + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + client, capture, remoteCalls := newUnresolvedFlagsClient(t, localDefinitions(continuityCheckoutDefinition), tt.remote) + + snapshot, err := client.EvaluateFlags(EvaluateFlagsPayload{DistinctId: "user-123"}) + if tt.wantErr { + require.Error(t, err) + } else { + require.NoError(t, err) + } + + require.Equal(t, int32(1), remoteCalls.Load()) + require.Empty(t, snapshot.Keys()) + require.Equal(t, map[string]UnresolvedFlagReason{"checkout": UnresolvedFlagReasonExperienceContinuity}, snapshot.UnresolvedFlags()) + + require.False(t, snapshot.IsEnabled("checkout")) + errorString := flagCalledError(t, client, capture, "checkout") + require.Contains(t, errorString, FeatureFlagErrorLocalEvaluationInconclusive) + require.NotContains(t, errorString, FeatureFlagErrorFlagMissing) + }) + } +} + +func TestEvaluateFlags_KeyWithoutLocalDefinitionIsMissingNotUnresolved(t *testing.T) { + t.Parallel() + client, capture, _ := newUnresolvedFlagsClient(t, localDefinitions(betaUIDefinition), remoteNoFlags) + + snapshot, err := client.EvaluateFlags(EvaluateFlagsPayload{DistinctId: "user-123", FlagKeys: []string{"beta-ui", "typo-flag"}}) + require.NoError(t, err) + + require.False(t, snapshot.IsEnabled("typo-flag")) + require.Empty(t, snapshot.UnresolvedFlags()) + require.Equal(t, FeatureFlagErrorFlagMissing, flagCalledError(t, client, capture, "typo-flag")) +} + +func TestEvaluateFlags_FlagOutsideRequestedKeysIsNotUnresolved(t *testing.T) { + t.Parallel() + client, _, remoteCalls := newUnresolvedFlagsClient(t, localDefinitions(betaUIDefinition, continuityCheckoutDefinition), remoteNoFlags) + + snapshot, err := client.EvaluateFlags(EvaluateFlagsPayload{DistinctId: "user-123", FlagKeys: []string{"beta-ui"}}) + require.NoError(t, err) + + require.Equal(t, []string{"beta-ui"}, snapshot.Keys()) + require.True(t, snapshot.flags["beta-ui"].Enabled) + require.Empty(t, snapshot.UnresolvedFlags()) + require.Zero(t, remoteCalls.Load()) +} + +func TestEvaluateFlags_InactiveFlagIsResolvedAsFalseNotUnresolved(t *testing.T) { + t.Parallel() + inactive := `{"key": "checkout", "active": false, "ensure_experience_continuity": true, "filters": {"groups": [{"properties": [], "rollout_percentage": 100}]}}` + client, _, _ := newUnresolvedFlagsClient(t, localDefinitions(inactive), remoteNoFlags) + + snapshot, err := client.EvaluateFlags(EvaluateFlagsPayload{DistinctId: "user-123", OnlyEvaluateLocally: true}) + require.NoError(t, err) + + require.Equal(t, []string{"checkout"}, snapshot.Keys()) + require.False(t, snapshot.flags["checkout"].Enabled) + require.Empty(t, snapshot.UnresolvedFlags()) +} + +func TestEvaluateFlags_ReadingUnresolvedFlagReportsInconclusiveLocalEvaluation(t *testing.T) { + t.Parallel() + client, capture, _ := newUnresolvedFlagsClient(t, localDefinitions(continuityCheckoutDefinition), remoteNoFlags) + + snapshot, err := client.EvaluateFlags(EvaluateFlagsPayload{DistinctId: "user-123", OnlyEvaluateLocally: true}) + require.NoError(t, err) + + require.False(t, snapshot.IsEnabled("checkout")) + require.Equal(t, FeatureFlagErrorLocalEvaluationInconclusive, flagCalledError(t, client, capture, "checkout")) +} + +func TestEvaluateFlags_UnexpectedLocalErrorIsNotUnresolved(t *testing.T) { + t.Parallel() + // A numeric filter value that is not a number fails with an unexpected error, not an + // inconclusive match. + notOrderable := `{"key": "checkout", "active": true, "filters": {"groups": [{"properties": [{"key": "age", "type": "person", "operator": "gt", "value": "abc"}], "rollout_percentage": 100}]}}` + client, capture, _ := newUnresolvedFlagsClient(t, localDefinitions(notOrderable), remoteNoFlags) + + snapshot, err := client.EvaluateFlags(EvaluateFlagsPayload{DistinctId: "user-123", PersonProperties: NewProperties().Set("age", 30), OnlyEvaluateLocally: true}) + require.NoError(t, err) + + require.Empty(t, snapshot.Keys()) + require.Empty(t, snapshot.UnresolvedFlags()) + require.False(t, snapshot.IsEnabled("checkout")) + require.Equal(t, FeatureFlagErrorFlagMissing, flagCalledError(t, client, capture, "checkout")) +} + +func TestFeatureFlagEvaluations_UnresolvedFlagsErrorKeepsResponseErrors(t *testing.T) { + var errorsReported []string + snapshot := &FeatureFlagEvaluations{ + host: featureFlagEvaluationsHost{captureFlagCalledIfNeeded: func(_, _ string, _ interface{}, _ *string, properties Properties, _ Groups, _ bool) { + errorsReported = append(errorsReported, properties["$feature_flag_error"].(string)) + }}, + distinctId: "user-123", + flags: map[string]evaluatedFlagRecord{"beta-ui": {Key: "beta-ui", Enabled: true}}, + unresolved: map[string]UnresolvedFlagReason{"checkout": UnresolvedFlagReasonExperienceContinuity}, + errorsWhileComputing: true, + } + + unresolved := snapshot.UnresolvedFlags() + unresolved["checkout"] = UnresolvedFlagReasonMissingContext + require.Equal(t, map[string]UnresolvedFlagReason{"checkout": UnresolvedFlagReasonExperienceContinuity}, snapshot.UnresolvedFlags()) + require.Empty(t, errorsReported) + require.Empty(t, snapshot.OnlyAccessed().Keys()) + filtered := snapshot.Only([]string{"beta-ui"}) + + require.Nil(t, snapshot.GetFlag("checkout")) + require.Equal(t, []string{FeatureFlagErrorErrorsWhileComputing + "," + FeatureFlagErrorLocalEvaluationInconclusive}, errorsReported) + + // A filtered snapshot treats the key as any other key absent from its source. + filtered.IsEnabled("checkout") + require.Equal(t, []string{ + FeatureFlagErrorErrorsWhileComputing + "," + FeatureFlagErrorLocalEvaluationInconclusive, + FeatureFlagErrorErrorsWhileComputing + "," + FeatureFlagErrorFlagMissing, + }, errorsReported) +} diff --git a/feature_flag_property_group.go b/feature_flag_property_group.go index a400be9e..72822347 100644 --- a/feature_flag_property_group.go +++ b/feature_flag_property_group.go @@ -35,19 +35,19 @@ func (poller *FeatureFlagsPoller) matchPropertyGroup(propertyGroup PropertyGroup // reconstruction from map[string]any. This is the fast path for cohort matching. func (poller *FeatureFlagsPoller) matchParsedPropertyGroup(groupType string, parsedValues []parsedPropertyValue, properties Properties, cohorts map[string]PropertyGroup, flagsByKey map[string]FeatureFlag, evaluationCache map[string]interface{}, distinctId string, deviceId *string, aggregationGroupTypeIndex *uint8, snapshots ...*flagsState) (bool, error) { state := poller.evaluationState(snapshots) - errorMatchingLocally := false + var inconclusiveReason UnresolvedFlagReason for i := range parsedValues { pv := &parsedValues[i] if pv.IsGroup { matches, err := poller.matchPropertyGroup(pv.Group, properties, cohorts, flagsByKey, evaluationCache, distinctId, deviceId, aggregationGroupTypeIndex, state) if err != nil { - if isServerEvalError(err) { - return false, err - } else if isInconclusiveError(err) { + if serverErr, ok := err.(*RequiresServerEvaluationError); ok { + return false, serverErr.withReason(inconclusiveReason) + } else if inconclusiveErr, ok := err.(*InconclusiveMatchError); ok { // An inconclusive entry is neither a match nor a mismatch, so it // can't decide the group, with or without negation. - errorMatchingLocally = true + inconclusiveReason = pickUnresolvedReason(inconclusiveReason, inconclusiveErr.Reason) continue } else { return false, err @@ -76,12 +76,12 @@ func (poller *FeatureFlagsPoller) matchParsedPropertyGroup(groupType string, par } if err != nil { - if isServerEvalError(err) { - return false, err - } else if isInconclusiveError(err) { + if serverErr, ok := err.(*RequiresServerEvaluationError); ok { + return false, serverErr.withReason(inconclusiveReason) + } else if inconclusiveErr, ok := err.(*InconclusiveMatchError); ok { // An inconclusive entry is neither a match nor a mismatch, so it // can't decide the group, with or without negation. - errorMatchingLocally = true + inconclusiveReason = pickUnresolvedReason(inconclusiveReason, inconclusiveErr.Reason) continue } else { return false, err @@ -107,8 +107,8 @@ func (poller *FeatureFlagsPoller) matchParsedPropertyGroup(groupType string, par } } - if errorMatchingLocally { - return false, errCohortPropertyValue + if inconclusiveReason != "" { + return false, errCohortPropertyValue.withReason(inconclusiveReason) } return groupType == "AND", nil diff --git a/feature_flags_dependencies_test.go b/feature_flags_dependencies_test.go index 4b266fe3..1f765d62 100644 --- a/feature_flags_dependencies_test.go +++ b/feature_flags_dependencies_test.go @@ -558,3 +558,86 @@ func TestMultiLevelMultivariateDependencyChain(t *testing.T) { require.NoError(t, err) require.Equal(t, false, resultBool) } + +func TestFlagDependencyUnresolvedReasons(t *testing.T) { + inactiveContinuity := `{"key": "checkout", "active": false, "ensure_experience_continuity": true, "filters": {"groups": [{"properties": [], "rollout_percentage": 100}]}}` + needsPlan := flagDefinition("checkout", flagCondition(100, planIsProFilter)) + needsServer := flagDefinition("checkout", flagCondition(100, staticCohortFilter)) + groupFlag := `{"key": "checkout", "active": true, "filters": {"aggregation_group_type_index": 0, "groups": [{"properties": [], "rollout_percentage": 100}]}}` + dependent := func(properties ...string) string { + return flagDefinition("beta-ui", flagCondition(100, properties...)) + } + tests := []struct { + name string + flags []string + want interface{} + wantReason UnresolvedFlagReason + wantServer bool + }{ + { + name: "dependency has experience continuity", + flags: []string{continuityCheckoutDefinition, dependent(flagDependencyFilter("checkout", true, "checkout"))}, + wantReason: UnresolvedFlagReasonUnresolvedDependency, + }, + { + name: "inactive dependency with experience continuity is false", + flags: []string{inactiveContinuity, dependent(flagDependencyFilter("checkout", false, "checkout"))}, + want: true, + }, + { + name: "dependency is inconclusive", + flags: []string{needsPlan, dependent(flagDependencyFilter("checkout", true, "checkout"))}, + wantReason: UnresolvedFlagReasonUnresolvedDependency, + }, + { + name: "dependency requires the server", + flags: []string{needsServer, dependent(flagDependencyFilter("checkout", true, "checkout"))}, + wantReason: UnresolvedFlagReasonUnresolvedDependency, + wantServer: true, + }, + { + name: "dependency has no loaded definition", + flags: []string{dependent(flagDependencyFilter("checkout", true, "checkout"))}, + wantReason: UnresolvedFlagReasonUnsupportedDefinition, + }, + { + name: "missing definition later in the chain outranks an unresolved dependency", + flags: []string{continuityCheckoutDefinition, dependent(flagDependencyFilter("gone", true, "checkout", "gone"))}, + wantReason: UnresolvedFlagReasonUnsupportedDefinition, + }, + { + name: "circular dependency", + flags: []string{continuityCheckoutDefinition, dependent(flagDependencyFilter("checkout", true))}, + wantReason: UnresolvedFlagReasonUnsupportedDefinition, + }, + { + name: "dependency needs a different aggregation", + flags: []string{groupFlag, dependent(flagDependencyFilter("checkout", true, "checkout"))}, + wantReason: UnresolvedFlagReasonUnsupportedDefinition, + wantServer: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := computeDefinedFlag(t, localDefinitions(tt.flags...), "beta-ui", nil) + if tt.wantReason == "" { + require.NoError(t, err) + require.Equal(t, tt.want, got) + return + } + require.Equal(t, tt.wantServer, isServerEvalError(err), "%v", err) + requireUnresolvedReason(t, err, tt.wantReason) + }) + } +} + +func TestRepeatedReferenceToMissingDependencyKeepsReason(t *testing.T) { + poller := newDefinitionsPoller(t, localDefinitions(continuityCheckoutDefinition)) + state := poller.state.Load() + property := FlagProperty{Key: "gone", Type: "flag", Operator: "flag_evaluates_to", Value: true, DependencyChain: []string{"gone"}} + cache := map[string]interface{}{} + for i := 0; i < 2; i++ { + _, err := poller.evaluateFlagDependency(property, state.flagsByKey, cache, "user-123", nil, nil, state.cohorts, nil, state) + requireUnresolvedReason(t, err, UnresolvedFlagReasonUnsupportedDefinition) + } +} diff --git a/feature_flags_local_test.go b/feature_flags_local_test.go index d599b512..5676b231 100644 --- a/feature_flags_local_test.go +++ b/feature_flags_local_test.go @@ -709,8 +709,9 @@ func TestFeatureFlagWithDependencies(t *testing.T) { t.Error("Should return error when flag dependencies can't be evaluated locally") } - // Test with email that doesn't match - should also error due to missing dependency - _, err = client.IsFeatureEnabled( + // An email that doesn't match rules the condition out whatever the missing dependency + // would return, so the flag resolves locally. + enabled, err := client.IsFeatureEnabled( FeatureFlagPayload{ Key: "flag-with-dependencies", DistinctId: "test-user-2", @@ -719,8 +720,8 @@ func TestFeatureFlagWithDependencies(t *testing.T) { }, ) - if err == nil { - t.Error("Should return error when flag dependencies can't be evaluated locally") + if err != nil || enabled != false { + t.Errorf("Should resolve to false locally when another filter does not match, got %v, %v", enabled, err) } } diff --git a/feature_flags_property_group_test.go b/feature_flags_property_group_test.go index 760866e2..6f191e7d 100644 --- a/feature_flags_property_group_test.go +++ b/feature_flags_property_group_test.go @@ -41,7 +41,7 @@ func TestMatchPropertyGroupRawAndParsedParity(t *testing.T) { map[string]any{"key": "country", "operator": "exact", "value": "US"}, }}, properties: NewProperties().Set("country", "US"), - wantErr: errCohortPropertyValue, + wantErr: errCohortPropertyValue.withReason(UnresolvedFlagReasonMissingContext), }, { name: "OR match overrides an inconclusive result", @@ -66,7 +66,7 @@ func TestMatchPropertyGroupRawAndParsedParity(t *testing.T) { map[string]any{"key": "missing", "operator": "exact", "value": "value", "negation": true}, }}, properties: NewProperties().Set("country", "US"), - wantErr: errCohortPropertyValue, + wantErr: errCohortPropertyValue.withReason(UnresolvedFlagReasonMissingContext), }, { name: "inconclusive leaf is not an AND mismatch", @@ -75,7 +75,7 @@ func TestMatchPropertyGroupRawAndParsedParity(t *testing.T) { map[string]any{"key": "country", "operator": "exact", "value": "US"}, }}, properties: NewProperties().Set("country", "US"), - wantErr: errCohortPropertyValue, + wantErr: errCohortPropertyValue.withReason(UnresolvedFlagReasonMissingContext), }, { name: "inconclusive nested group is not an AND mismatch", @@ -86,7 +86,7 @@ func TestMatchPropertyGroupRawAndParsedParity(t *testing.T) { map[string]any{"key": "country", "operator": "exact", "value": "US"}, }}, properties: NewProperties().Set("country", "US"), - wantErr: errCohortPropertyValue, + wantErr: errCohortPropertyValue.withReason(UnresolvedFlagReasonMissingContext), }, { name: "AND mismatch after an inconclusive leaf is still a mismatch", @@ -96,6 +96,15 @@ func TestMatchPropertyGroupRawAndParsedParity(t *testing.T) { }}, properties: NewProperties().Set("country", "CA"), }, + { + name: "inconclusive result keeps the highest reason", + group: PropertyGroup{Type: "AND", Values: []any{ + map[string]any{"key": "missing", "operator": "exact", "value": "value"}, + map[string]any{"key": "country", "operator": "is_on_the_moon", "value": "US"}, + }}, + properties: NewProperties().Set("country", "US"), + wantErr: errCohortPropertyValue.withReason(UnresolvedFlagReasonUnsupportedDefinition), + }, { name: "missing cohort requires server evaluation", group: PropertyGroup{Type: "AND", Values: []any{ diff --git a/featureflags.go b/featureflags.go index 7d1e49c8..2a101260 100644 --- a/featureflags.go +++ b/featureflags.go @@ -38,12 +38,12 @@ var relativeDateRegex = regexp.MustCompile(`^-?([0-9]+)([hdwmy])$`) // Common sentinel errors for matchProperty — reused to avoid allocating new // InconclusiveMatchError structs on every evaluation of flags with missing properties. var ( - errMissingPropertyValue = &InconclusiveMatchError{"Can't match properties without a given property value"} - errInconclusiveMatch = &InconclusiveMatchError{"Can't determine if feature flag is enabled or not with given properties"} - errAmbiguousExactNumber = &InconclusiveMatchError{"Can't match an integral JSON number locally because Go does not preserve whether it was an integer or float"} - errCohortPropertyValue = &InconclusiveMatchError{msg: "Can't match cohort without a given cohort property value"} - errHoldoutMissingDeviceID = &InconclusiveMatchError{msg: "Can't evaluate a device-bucketed holdout without a device id"} - errCohortRequiresServerEval = &RequiresServerEvaluationError{msg: "cohort not found in local cohorts - likely a static cohort that requires server evaluation"} + errMissingPropertyValue = &InconclusiveMatchError{msg: "Can't match properties without a given property value", Reason: UnresolvedFlagReasonMissingContext} + errInconclusiveMatch = newInconclusiveSentinels("Can't determine if feature flag is enabled or not with given properties") + errAmbiguousExactNumber = &InconclusiveMatchError{msg: "Can't match an integral JSON number locally because Go does not preserve whether it was an integer or float", Reason: UnresolvedFlagReasonUnsupportedDefinition} + errCohortPropertyValue = newInconclusiveSentinels("Can't match cohort without a given cohort property value") + errHoldoutMissingDeviceID = newInconclusiveSentinels("Can't evaluate a device-bucketed holdout without a device id") + errCohortRequiresServerEval = &RequiresServerEvaluationError{msg: "cohort not found in local cohorts - likely a static cohort that requires server evaluation", Reason: UnresolvedFlagReasonUnsupportedDefinition} ) // regexCache caches compiled regexps for flag property matching. @@ -52,7 +52,9 @@ var ( var regexCache sync.Map // map[string]*flagRegex // A later matching condition cannot decide the variant while an earlier regex is unresolved. -var errRegexRequiresServer = &RequiresServerEvaluationError{"Regex requires server evaluation"} +var errRegexRequiresServer = &RequiresServerEvaluationError{msg: "Regex requires server evaluation", Reason: UnresolvedFlagReasonUnsupportedDefinition} + +var errDependencyAggregationRequiresServer = &RequiresServerEvaluationError{msg: "Flag dependency requires different aggregation context", Reason: UnresolvedFlagReasonUnsupportedDefinition} type flagRegex struct { *regexp.Regexp @@ -301,6 +303,16 @@ type FeatureFlagsPoller struct { decider decider disableGeoIP bool cacheProvider FlagDefinitionCacheProvider + + // warnedUnresolvableFlags maps each flag key already warned about as never locally + // resolvable to the definition it was warned about at. + warnedUnresolvableMu sync.Mutex + warnedUnresolvableFlags map[string]unresolvableFlagFingerprint +} + +type unresolvableFlagFingerprint struct { + reason UnresolvedFlagReason + definition [sha1.Size]byte } // FeatureFlag is a feature flag definition returned by the local evaluation endpoint. @@ -467,9 +479,98 @@ type DecideResponse struct { FeatureFlagPayloads map[string]json.RawMessage `json:"featureFlagPayloads"` } +// UnresolvedFlagReason explains why local evaluation could not resolve a flag. +// Experience continuity and unsupported definitions come from the flag's +// definition, missing context from the evaluation call, and unresolved +// dependencies from a flag the definition depends on. +type UnresolvedFlagReason string + +const ( + // UnresolvedFlagReasonExperienceContinuity means the flag has experience continuity + // enabled, which local evaluation never resolves. + UnresolvedFlagReasonExperienceContinuity UnresolvedFlagReason = "experience_continuity" + // UnresolvedFlagReasonUnsupportedDefinition means the flag uses something local + // evaluation does not support, such as a static cohort, an unknown operator, a + // malformed value, or a missing or circular flag dependency. + UnresolvedFlagReasonUnsupportedDefinition UnresolvedFlagReason = "unsupported_definition" + // UnresolvedFlagReasonUnresolvedDependency means a flag this flag depends on has a + // loaded definition but local evaluation could not resolve it. + UnresolvedFlagReasonUnresolvedDependency UnresolvedFlagReason = "unresolved_dependency" + // UnresolvedFlagReasonMissingContext means the call did not pass a property, group + // property or device id the flag needs, or passed it in a form local evaluation + // cannot use. + UnresolvedFlagReasonMissingContext UnresolvedFlagReason = "missing_context" +) + +// unresolvedFlagReasons lists the reasons in the order the spec reports them when +// more than one cause applies, so every SDK reports the same reason whatever order +// it inspects the filters in. +var unresolvedFlagReasons = [...]UnresolvedFlagReason{ + UnresolvedFlagReasonExperienceContinuity, + UnresolvedFlagReasonUnsupportedDefinition, + UnresolvedFlagReasonUnresolvedDependency, + UnresolvedFlagReasonMissingContext, +} + +func unresolvedReasonRank(reason UnresolvedFlagReason) int { + for rank, candidate := range unresolvedFlagReasons { + if candidate == reason { + return rank + } + } + return len(unresolvedFlagReasons) +} + +// pickUnresolvedReason returns whichever of current and next comes first in the spec +// order. An empty current means no reason was recorded yet. +func pickUnresolvedReason(current, next UnresolvedFlagReason) UnresolvedFlagReason { + if next == "" { + // An error raised without a reason still leaves the flag unresolved, and the + // loops treat an empty reason as "nothing was inconclusive". + next = UnresolvedFlagReasonUnsupportedDefinition + } + if current == "" || unresolvedReasonRank(next) < unresolvedReasonRank(current) { + return next + } + return current +} + +// unresolvedReasonOf returns the reason an inconclusive or server-required error carries. +func unresolvedReasonOf(err error) (UnresolvedFlagReason, bool) { + switch e := err.(type) { + case *InconclusiveMatchError: + return pickUnresolvedReason("", e.Reason), true + case *RequiresServerEvaluationError: + return pickUnresolvedReason("", e.Reason), true + } + return "", false +} + +// inconclusiveSentinels holds one shared error per reason, so that merging reasons +// does not allocate on every evaluation. +type inconclusiveSentinels [len(unresolvedFlagReasons)]*InconclusiveMatchError + +func newInconclusiveSentinels(msg string) *inconclusiveSentinels { + var sentinels inconclusiveSentinels + for rank, reason := range unresolvedFlagReasons { + sentinels[rank] = &InconclusiveMatchError{msg: msg, Reason: reason} + } + return &sentinels +} + +func (s *inconclusiveSentinels) withReason(reason UnresolvedFlagReason) *InconclusiveMatchError { + rank := unresolvedReasonRank(reason) + if rank == len(s) { + rank = unresolvedReasonRank(UnresolvedFlagReasonUnsupportedDefinition) + } + return s[rank] +} + // InconclusiveMatchError indicates that local evaluation could not conclusively match a condition. type InconclusiveMatchError struct { msg string + // Reason is why local evaluation could not resolve the flag. + Reason UnresolvedFlagReason } // Error returns the inconclusive match message. @@ -483,6 +584,8 @@ func (e *InconclusiveMatchError) Error() string { // API fallback, unlike InconclusiveMatchError which allows trying other conditions. type RequiresServerEvaluationError struct { msg string + // Reason is why local evaluation could not resolve the flag. + Reason UnresolvedFlagReason } // Error returns the reason server-side evaluation is required. @@ -490,6 +593,15 @@ func (e *RequiresServerEvaluationError) Error() string { return e.msg } +// withReason returns e, or a copy of it when reason outranks the reason e carries. +func (e *RequiresServerEvaluationError) withReason(reason UnresolvedFlagReason) *RequiresServerEvaluationError { + merged := pickUnresolvedReason(reason, e.Reason) + if merged == e.Reason { + return e + } + return &RequiresServerEvaluationError{msg: e.msg, Reason: merged} +} + // isServerEvalError returns true if err is a RequiresServerEvaluationError. // Uses type assertion instead of errors.As to avoid pointer-variable heap escapes. func isServerEvalError(err error) bool { @@ -551,76 +663,31 @@ func (poller *FeatureFlagsPoller) evaluateFlagDependency( snapshots ...*flagsState, ) (bool, error) { state := poller.evaluationState(snapshots) - // Some of these conditions should never happen, but we'll check them to be defensive. - if property.Value == nil { - return false, &InconclusiveMatchError{ - msg: fmt.Sprintf("Cannot evaluate flag dependency on '%s' without a value", property.Key), - } - } - - if property.Operator != "flag_evaluates_to" { - return false, &InconclusiveMatchError{ - msg: fmt.Sprintf("Unsupported operator '%s' for flag dependency '%s'", property.Operator, property.Key), - } - } - if flagsByKey == nil || evaluationCache == nil { // Cannot evaluate flag dependencies without required context return false, &InconclusiveMatchError{ - msg: fmt.Sprintf("Cannot evaluate flag dependency on '%s' without flagsByKey and evaluationCache", property.Key), + msg: fmt.Sprintf("Cannot evaluate flag dependency on '%s' without flagsByKey and evaluationCache", property.Key), + Reason: UnresolvedFlagReasonUnsupportedDefinition, } } - // Check if dependency_chain is present - it should always be provided for flag dependencies - if property.DependencyChain == nil { - // Missing dependency_chain indicates malformed server data - return false, &InconclusiveMatchError{ - msg: fmt.Sprintf("Flag dependency property for '%s' is missing required 'dependency_chain' field", property.Key), - } - } - - dependencyChain := property.DependencyChain - - // Handle circular dependency (empty chain means circular) - if len(dependencyChain) == 0 { - if poller.Logger != nil { + // A problem in the definitions of the chain outranks a dependency that is only + // inconclusive, so check the whole chain before evaluating any of it. + if err := flagDependencyDefinitionError(property, flagsByKey, aggregationGroupTypeIndex); err != nil { + if property.DependencyChain != nil && len(property.DependencyChain) == 0 && poller.Logger != nil { poller.Logger.Debugf("Circular dependency detected for flag: %s", property.Key) } - return false, &InconclusiveMatchError{ - msg: fmt.Sprintf("Circular dependency detected for flag '%s'", property.Key), - } + return false, err } + dependencyChain := property.DependencyChain // Evaluate all dependencies in the chain order for _, depFlagKey := range dependencyChain { - // Dependencies can reuse the caller's key and properties only for the - // same aggregation type. Check before the cache so repeated cohort - // references cannot bypass the cross-type server boundary. - depFlag := flagsByKey[depFlagKey] - if depFlag.Active { - if !uint8PtrEqual(depFlag.Filters.AggregationGroupTypeIndex, aggregationGroupTypeIndex) { - return false, &RequiresServerEvaluationError{"Flag dependency requires different aggregation context"} - } - for _, condition := range depFlag.Filters.Groups { - if condition.AggregationGroupTypeIndex != nil && !uint8PtrEqual(condition.AggregationGroupTypeIndex, aggregationGroupTypeIndex) { - return false, &RequiresServerEvaluationError{"Flag dependency requires different aggregation context"} - } - } - } if _, exists := evaluationCache[depFlagKey]; exists { continue } - // Need to evaluate this dependency first - depFlag, flagExists := flagsByKey[depFlagKey] - if !flagExists { - // Missing flag dependency - cannot evaluate locally - evaluationCache[depFlagKey] = nil - return false, &InconclusiveMatchError{ - msg: fmt.Sprintf("Cannot evaluate flag dependency '%s' - flag not found in local flags", depFlagKey), - } - } - + depFlag := flagsByKey[depFlagKey] // Check if the flag is active (same check as in computeFlagLocally) if !depFlag.Active { evaluationCache[depFlagKey] = false @@ -628,7 +695,7 @@ func (poller *FeatureFlagsPoller) evaluateFlagDependency( // Dependencies must preserve the same server boundary as direct evaluation. if depFlag.EnsureExperienceContinuity != nil && *depFlag.EnsureExperienceContinuity { evaluationCache[depFlagKey] = nil - return false, &InconclusiveMatchError{"Flag dependency has experience continuity enabled"} + return false, &InconclusiveMatchError{msg: "Flag dependency has experience continuity enabled", Reason: UnresolvedFlagReasonUnresolvedDependency} } // Recursively evaluate the dependency result, err := poller.matchFeatureFlagProperties(depFlag, distinctId, deviceId, properties, cohorts, flagsByKey, evaluationCache, nil, nil, state) @@ -636,11 +703,15 @@ func (poller *FeatureFlagsPoller) evaluateFlagDependency( // Preserve server-required errors on repeated references instead of // reducing them to a cached inconclusive value that cohorts can negate. if isServerEvalError(err) { - return false, err + return false, &RequiresServerEvaluationError{ + msg: fmt.Sprintf("Cannot evaluate flag dependency '%s': %s", depFlagKey, err.Error()), + Reason: UnresolvedFlagReasonUnresolvedDependency, + } } evaluationCache[depFlagKey] = nil return false, &InconclusiveMatchError{ - msg: fmt.Sprintf("Cannot evaluate flag dependency '%s': %s", depFlagKey, err.Error()), + msg: fmt.Sprintf("Cannot evaluate flag dependency '%s': %s", depFlagKey, err.Error()), + Reason: UnresolvedFlagReasonUnresolvedDependency, } } evaluationCache[depFlagKey] = result @@ -648,7 +719,8 @@ func (poller *FeatureFlagsPoller) evaluateFlagDependency( } // Check if the dependency result matches the expected value and operator - if cachedResult, exists := evaluationCache[property.Key]; exists && cachedResult != nil { + cachedResult, exists := evaluationCache[property.Key] + if exists && cachedResult != nil { match, err := checkFlagDependencyValue(property.Value, cachedResult) if err != nil { return false, err @@ -656,10 +728,63 @@ func (poller *FeatureFlagsPoller) evaluateFlagDependency( return match, nil } - // The main dependency couldn't be evaluated + // The main dependency couldn't be evaluated. A key missing from its own chain is + // malformed server data rather than an unresolved dependency. + reason := UnresolvedFlagReasonUnresolvedDependency + if !exists { + reason = UnresolvedFlagReasonUnsupportedDefinition + } return false, &InconclusiveMatchError{ - msg: fmt.Sprintf("Flag dependency '%s' could not be evaluated for value comparison", property.Key), + msg: fmt.Sprintf("Flag dependency '%s' could not be evaluated for value comparison", property.Key), + Reason: reason, + } +} + +// flagDependencyDefinitionError reports a flag dependency filter that local evaluation +// cannot use whatever the call passes. Evaluation and the load-time warning share it so +// they agree on which dependencies are unsupported. +func flagDependencyDefinitionError(property FlagProperty, flagsByKey map[string]FeatureFlag, aggregationGroupTypeIndex *uint8) error { + switch { + case property.Value == nil: + return &InconclusiveMatchError{msg: fmt.Sprintf("Cannot evaluate flag dependency on '%s' without a value", property.Key), Reason: UnresolvedFlagReasonUnsupportedDefinition} + case property.Operator != "flag_evaluates_to": + return &InconclusiveMatchError{msg: fmt.Sprintf("Unsupported operator '%s' for flag dependency '%s'", property.Operator, property.Key), Reason: UnresolvedFlagReasonUnsupportedDefinition} + case property.DependencyChain == nil: + return &InconclusiveMatchError{msg: fmt.Sprintf("Flag dependency property for '%s' is missing required 'dependency_chain' field", property.Key), Reason: UnresolvedFlagReasonUnsupportedDefinition} + case len(property.DependencyChain) == 0: + // The server sends an empty chain for a circular dependency. + return &InconclusiveMatchError{msg: fmt.Sprintf("Circular dependency detected for flag '%s'", property.Key), Reason: UnresolvedFlagReasonUnsupportedDefinition} + } + return dependencyChainDefinitionError(property.DependencyChain, flagsByKey, aggregationGroupTypeIndex) +} + +// dependencyChainDefinitionError reports a dependency with no loaded definition, or one +// that needs a different aggregation context than the dependent condition. +// Dependencies can reuse the caller's key and properties only for the same +// aggregation type. This runs before the evaluation cache is read so repeated cohort +// references cannot bypass the cross-type server boundary. +func dependencyChainDefinitionError(dependencyChain []string, flagsByKey map[string]FeatureFlag, aggregationGroupTypeIndex *uint8) error { + for _, depFlagKey := range dependencyChain { + depFlag, flagExists := flagsByKey[depFlagKey] + if !flagExists { + return &InconclusiveMatchError{ + msg: fmt.Sprintf("Cannot evaluate flag dependency '%s' - flag not found in local flags", depFlagKey), + Reason: UnresolvedFlagReasonUnsupportedDefinition, + } + } + if !depFlag.Active { + continue + } + if !uint8PtrEqual(depFlag.Filters.AggregationGroupTypeIndex, aggregationGroupTypeIndex) { + return errDependencyAggregationRequiresServer + } + for _, condition := range depFlag.Filters.Groups { + if condition.AggregationGroupTypeIndex != nil && !uint8PtrEqual(condition.AggregationGroupTypeIndex, aggregationGroupTypeIndex) { + return errDependencyAggregationRequiresServer + } + } } + return nil } // checkFlagDependencyValue checks if a flag dependency result matches the expected value and operator @@ -892,7 +1017,7 @@ func (poller *FeatureFlagsPoller) applyFlagDefinitions(data json.RawMessage, dec groups = *decoded.GroupTypeMapping } - poller.state.Store(&flagsState{ + state := &flagsState{ featureFlags: flags, flagsByKey: buildFlagsByKey(flags), cohorts: preParseCohortValues(decoded.Cohorts), @@ -901,7 +1026,149 @@ func (poller *FeatureFlagsPoller) applyFlagDefinitions(data json.RawMessage, dec minimalFlagCalledEvents: decoded.MinimalFlagCalledEvents, propertyMatchingVersion: decoded.PropertyMatchingVersion, definitions: data, - }) + } + poller.state.Store(state) + poller.warnAboutUnresolvableFlags(state) +} + +// warnAboutUnresolvableFlags logs the flags whose definition local evaluation never +// resolves. Each flag is warned about once until its definition changes, because the +// cache provider applies the same definitions again on every poll. +func (poller *FeatureFlagsPoller) warnAboutUnresolvableFlags(state *flagsState) { + if poller.Logger == nil { + return + } + poller.warnedUnresolvableMu.Lock() + defer poller.warnedUnresolvableMu.Unlock() + // Rebuilt from the current definitions, so a flag that is deleted or becomes + // resolvable is forgotten and warns again if it later returns. + warned := make(map[string]unresolvableFlagFingerprint) + var experienceContinuityKeys, unsupportedDefinitionKeys []string + for _, flag := range state.featureFlags { + reason := definitionUnresolvedReason(flag, state.flagsByKey, state.cohorts, state.propertyMatchingVersion) + if reason == "" { + continue + } + fingerprint := unresolvableFlagFingerprint{reason: reason, definition: flagDefinitionHash(flag)} + warned[flag.Key] = fingerprint + if poller.warnedUnresolvableFlags[flag.Key] == fingerprint { + continue + } + if reason == UnresolvedFlagReasonExperienceContinuity { + experienceContinuityKeys = append(experienceContinuityKeys, flag.Key) + } else { + unsupportedDefinitionKeys = append(unsupportedDefinitionKeys, flag.Key) + } + } + poller.warnedUnresolvableFlags = warned + + if len(experienceContinuityKeys) > 0 { + poller.Logger.Warnf("[FEATURE FLAGS] You are using local evaluation but %d flag(s) have experience continuity enabled and cannot be resolved locally (reason: %s): %s. "+ + "Evaluating these flags makes a /flags request, or leaves them unresolved with OnlyEvaluateLocally. "+ + "To evaluate them locally, disable experience continuity on these flags in PostHog.", + len(experienceContinuityKeys), UnresolvedFlagReasonExperienceContinuity, strings.Join(experienceContinuityKeys, ", ")) + } + if len(unsupportedDefinitionKeys) > 0 { + poller.Logger.Warnf("[FEATURE FLAGS] You are using local evaluation but %d flag(s) use something local evaluation does not support, such as a static cohort, an unknown operator, a malformed value, or a missing or circular flag dependency (reason: %s): %s. "+ + "Evaluating these flags makes a /flags request when evaluation reaches that part, or leaves them unresolved with OnlyEvaluateLocally. "+ + "To evaluate them locally, change these flags in PostHog or upgrade posthog-go.", + len(unsupportedDefinitionKeys), UnresolvedFlagReasonUnsupportedDefinition, strings.Join(unsupportedDefinitionKeys, ", ")) + } +} + +// flagDefinitionHash hashes the decoded flag. Struct fields encode in a fixed order and +// map keys sorted, so a cache provider that serializes the same definition differently +// does not repeat the warning. +func flagDefinitionHash(flag FeatureFlag) [sha1.Size]byte { + // Payloads stay raw JSON with the key order of their source, so sort their keys too. + if len(flag.Filters.Payloads) > 0 { + payloads := make(map[string]json.RawMessage, len(flag.Filters.Payloads)) + for key, raw := range flag.Filters.Payloads { + payloads[key] = canonicalJSON(raw) + } + flag.Filters.Payloads = payloads + } + encoded, _ := json.Marshal(flag) + return sha1.Sum(encoded) +} + +// canonicalJSON re-encodes raw JSON with sorted object keys. Numbers keep their text. +func canonicalJSON(raw json.RawMessage) json.RawMessage { + decoder := json.NewDecoder(bytes.NewReader(raw)) + decoder.UseNumber() + var value interface{} + if err := decoder.Decode(&value); err != nil { + return raw + } + canonical, err := json.Marshal(value) + if err != nil { + return raw + } + return canonical +} + +// definitionUnresolvedReason returns the unresolved reason that comes from the flag's +// definition alone, so it is known when definitions load. It matches the reason local +// evaluation reports for the same cause. +func definitionUnresolvedReason(flag FeatureFlag, flagsByKey map[string]FeatureFlag, cohorts map[string]PropertyGroup, matchingVersion int) UnresolvedFlagReason { + if !flag.Active { + return "" + } + if flag.EnsureExperienceContinuity != nil && *flag.EnsureExperienceContinuity { + return UnresolvedFlagReasonExperienceContinuity + } + // A filter in any condition that cannot be checked locally whatever the call passes + // makes the definition unsupported. + visitedCohorts := map[string]struct{}{} + for _, condition := range flag.Filters.Groups { + aggregation := condition.AggregationGroupTypeIndex + if aggregation == nil { + aggregation = flag.Filters.AggregationGroupTypeIndex + } + for _, property := range condition.Properties { + if isUnsupportedProperty(property, aggregation, flagsByKey, cohorts, matchingVersion, visitedCohorts) { + return UnresolvedFlagReasonUnsupportedDefinition + } + } + } + return "" +} + +func isUnsupportedProperty(property FlagProperty, aggregation *uint8, flagsByKey map[string]FeatureFlag, cohorts map[string]PropertyGroup, matchingVersion int, visitedCohorts map[string]struct{}) bool { + switch property.Type { + case "flag": + return flagDependencyDefinitionError(property, flagsByKey, aggregation) != nil + case "cohort": + cohortID := valueToString(property.Value) + cohort, ok := cohorts[cohortID] + if !ok { + return true + } + if _, visited := visitedCohorts[cohortID]; visited { + return false + } + visitedCohorts[cohortID] = struct{}{} + if len(cohort.ParsedValues) == 0 { + cohort = preParsePG(cohort) + } + return isUnsupportedPropertyGroup(cohort, aggregation, flagsByKey, cohorts, matchingVersion, visitedCohorts) + default: + return filterDefinitionError(property, matchingVersion) != nil + } +} + +func isUnsupportedPropertyGroup(group PropertyGroup, aggregation *uint8, flagsByKey map[string]FeatureFlag, cohorts map[string]PropertyGroup, matchingVersion int, visitedCohorts map[string]struct{}) bool { + for i := range group.ParsedValues { + value := &group.ParsedValues[i] + if value.IsGroup { + if isUnsupportedPropertyGroup(value.Group, aggregation, flagsByKey, cohorts, matchingVersion, visitedCohorts) { + return true + } + } else if isUnsupportedProperty(value.Property, aggregation, flagsByKey, cohorts, matchingVersion, visitedCohorts) { + return true + } + } + return false } // publishFlagDefinitions stores definitions in the cache provider. @@ -985,12 +1252,14 @@ func (poller *FeatureFlagsPoller) fetchFlagDefinitions(publish bool) { // Handle quota limit response (HTTP 402) if res.StatusCode == http.StatusPaymentRequired { // Clear existing flags when quota limited - atomic swap - poller.state.Store(&flagsState{ + cleared := &flagsState{ featureFlags: []FeatureFlag{}, cohorts: map[string]PropertyGroup{}, groups: map[string]string{}, flagsEtag: "", - }) + } + poller.state.Store(cleared) + poller.warnAboutUnresolvableFlags(cleared) poller.Logger.Warnf("[FEATURE FLAGS] PostHog feature flags quota limited, resetting feature flag data. Learn more about billing limits at https://posthog.com/docs/billing/limits-alerts") return } @@ -1275,14 +1544,14 @@ func (poller *FeatureFlagsPoller) computeFlagLocally( snapshots ...*flagsState, ) (interface{}, error) { state := poller.evaluationState(snapshots) - if flag.EnsureExperienceContinuity != nil && *flag.EnsureExperienceContinuity { - return nil, &InconclusiveMatchError{"Flag has experience continuity enabled"} - } - if !flag.Active { return false, nil } + if flag.EnsureExperienceContinuity != nil && *flag.EnsureExperienceContinuity { + return nil, &InconclusiveMatchError{msg: "Flag has experience continuity enabled", Reason: UnresolvedFlagReasonExperienceContinuity} + } + // Use pre-built flagsByKey index (built once when flags are fetched, not per evaluation) flagsByKey := state.flagsByKey @@ -1304,8 +1573,12 @@ func (poller *FeatureFlagsPoller) computeFlagLocally( groupKey, exists := groups[groupType] if !exists { - errMessage := fmt.Sprintf("[FEATURE FLAGS] Can't compute group feature flag: %s without group names passed in", flag.Key) - return nil, errors.New(errMessage) + // No condition can be checked without the group key, so a cause in the + // definition outranks the missing key. + return nil, &InconclusiveMatchError{ + msg: fmt.Sprintf("[FEATURE FLAGS] Can't compute group feature flag: %s without group names passed in", flag.Key), + Reason: pickUnresolvedReason(definitionUnresolvedReason(flag, flagsByKey, cohorts, state.propertyMatchingVersion), UnresolvedFlagReasonMissingContext), + } } groupKeyStr, ok := groupKeyToString(groupKey) @@ -1427,7 +1700,10 @@ func (poller *FeatureFlagsPoller) matchFeatureFlagProperties( // Per-condition aggregation below never changes holdout membership. holdoutVariant, held, err := getHoldoutVariant(flag, distinctId, deviceId) if err != nil { - return nil, err + // No condition can be checked without the bucketing value, so a cause in the + // definition outranks the missing device id. + reason := pickUnresolvedReason(definitionUnresolvedReason(flag, flagsByKey, cohorts, state.propertyMatchingVersion), UnresolvedFlagReasonMissingContext) + return nil, errHoldoutMissingDeviceID.withReason(reason) } if held { return holdoutVariant, nil @@ -1436,7 +1712,7 @@ func (poller *FeatureFlagsPoller) matchFeatureFlagProperties( flagAggregation := flag.Filters.AggregationGroupTypeIndex groupTypeMapping := state.groups - isInconclusive := false + var inconclusiveReason UnresolvedFlagReason for _, condition := range conditions { // Per-condition aggregation overrides only when the condition explicitly @@ -1465,7 +1741,7 @@ func (poller *FeatureFlagsPoller) matchFeatureFlagProperties( } focusedGroupProperties, hasProps := groupProperties[groupType] if !hasProps { - isInconclusive = true + inconclusiveReason = pickUnresolvedReason(inconclusiveReason, UnresolvedFlagReasonMissingContext) continue } groupKeyStr, ok := groupKeyToString(groupKey) @@ -1484,14 +1760,20 @@ func (poller *FeatureFlagsPoller) matchFeatureFlagProperties( if err != nil { // Use direct type switch instead of errors.As to avoid pointer escape allocations. // Our error types are returned directly (not wrapped), so type assertion suffices. - switch err.(type) { + switch e := err.(type) { case *RequiresServerEvaluationError: - // Static cohort or other missing server-side data - must fallback to API - return nil, err + // Static cohort or other missing server-side data - must fallback to API. + // Evaluation stops here, and filters it skipped in any condition or cohort + // can still carry a higher reason. + reason := inconclusiveReason + if definitionReason := definitionUnresolvedReason(flag, flagsByKey, cohorts, state.propertyMatchingVersion); definitionReason != "" { + reason = pickUnresolvedReason(reason, definitionReason) + } + return nil, e.withReason(reason) case *InconclusiveMatchError: // Evaluation error (invalid date, missing property, etc.) // Track that we had an inconclusive match, but try other conditions - isInconclusive = true + inconclusiveReason = pickUnresolvedReason(inconclusiveReason, e.Reason) default: return nil, err } @@ -1515,8 +1797,8 @@ func (poller *FeatureFlagsPoller) matchFeatureFlagProperties( // inconclusive we can't decide locally, so return errInconclusiveMatch // to force a server fallback rather than continuing evaluation. if flag.Filters.EarlyExit { - if isInconclusive { - return false, errInconclusiveMatch + if inconclusiveReason != "" { + return false, errInconclusiveMatch.withReason(inconclusiveReason) } return false, nil } @@ -1526,8 +1808,8 @@ func (poller *FeatureFlagsPoller) matchFeatureFlagProperties( } } - if isInconclusive { - return false, errInconclusiveMatch + if inconclusiveReason != "" { + return false, errInconclusiveMatch.withReason(inconclusiveReason) } return false, nil @@ -1585,6 +1867,11 @@ func (poller *FeatureFlagsPoller) isConditionMatch( // original person's ID or device, for dependent flag bucketing. dependencyDistinctId, dependencyDeviceId = bucketingId, nil } + // Filters in a condition have no defined order, so an inconclusive filter does not + // stop the loop: a later filter can still rule the condition out or carry a + // higher-precedence reason. + var inconclusiveReason UnresolvedFlagReason + var serverEvaluationErr *RequiresServerEvaluationError for _, prop := range condition.Properties { if prop.Type == "cohort" { isMatch, err = poller.matchCohort(prop, properties, cohorts, flagsByKey, evaluationCache, dependencyDistinctId, dependencyDeviceId, aggregationGroupTypeIndex, state) @@ -1595,12 +1882,40 @@ func (poller *FeatureFlagsPoller) isConditionMatch( } if err != nil { - return conditionNoMatch, err + switch e := err.(type) { + case *RequiresServerEvaluationError: + // Fall back to the server only when this is the first filter that could + // not be checked, as when the loop stopped there. After another + // inconclusive filter, later conditions must still be able to resolve + // the flag locally. + if inconclusiveReason == "" { + serverEvaluationErr = e + } + inconclusiveReason = pickUnresolvedReason(inconclusiveReason, e.Reason) + continue + case *InconclusiveMatchError: + inconclusiveReason = pickUnresolvedReason(inconclusiveReason, e.Reason) + continue + default: + // An earlier filter already left the condition undecided, so an + // unexpected error here must not fail the whole flag. + if inconclusiveReason != "" { + continue + } + return conditionNoMatch, err + } } if !isMatch { return conditionNoMatch, nil } } + + if serverEvaluationErr != nil { + return conditionNoMatch, serverEvaluationErr.withReason(inconclusiveReason) + } + if inconclusiveReason != "" { + return conditionNoMatch, errInconclusiveMatch.withReason(inconclusiveReason) + } } if condition.RolloutPercentage != nil { @@ -1616,6 +1931,88 @@ func (poller *FeatureFlagsPoller) isConditionMatch( } func matchProperty(property FlagProperty, properties Properties, matchingVersions ...int) (bool, error) { + matched, err := matchPropertyValue(property, properties, matchingVersions...) + // An unusable filter outranks missing context, so check the filter before blaming the context. + if inconclusiveErr, ok := err.(*InconclusiveMatchError); ok && inconclusiveErr.Reason == UnresolvedFlagReasonMissingContext { + if filterErr := filterDefinitionError(property, matchingVersions...); filterErr != nil { + return false, filterErr + } + } + return matched, err +} + +// propertyOperators lists the operators matchPropertyValue handles. An operator missing +// here makes filterDefinitionError report a working operator as unknown. +var propertyOperators = map[string]struct{}{ + "exact": {}, "is_not": {}, "is_set": {}, "is_not_set": {}, "icontains": {}, "not_icontains": {}, + "starts_with": {}, "not_starts_with": {}, "ends_with": {}, "not_ends_with": {}, "regex": {}, "not_regex": {}, + "gt": {}, "gte": {}, "lt": {}, "lte": {}, "is_date_before": {}, "is_date_after": {}, + "semver_eq": {}, "semver_neq": {}, "semver_gt": {}, "semver_gte": {}, "semver_lt": {}, "semver_lte": {}, + "semver_tilde": {}, "semver_caret": {}, "semver_wildcard": {}, +} + +// filterDefinitionError returns an unsupported_definition error when local evaluation +// cannot use a property filter whatever the call passes: an unknown operator, or a +// filter value the operator cannot use. +func filterDefinitionError(property FlagProperty, matchingVersions ...int) *InconclusiveMatchError { + if _, ok := propertyOperators[property.Operator]; !ok { + return &InconclusiveMatchError{msg: "Unknown operator: " + property.Operator, Reason: UnresolvedFlagReasonUnsupportedDefinition} + } + value := property.Value + var err error + switch property.Operator { + case "exact", "is_not": + if exactFilterAlwaysAmbiguous(value, matchingVersions...) { + return errAmbiguousExactNumber + } + case "regex", "not_regex": + if _, regexErr := getOrCompileRegex(valueToString(value)); isServerEvalError(regexErr) { + return &InconclusiveMatchError{msg: regexErr.Error(), Reason: UnresolvedFlagReasonUnsupportedDefinition} + } + case "semver_eq", "semver_neq", "semver_gt", "semver_gte", "semver_lt", "semver_lte", "semver_tilde", "semver_caret", "semver_wildcard": + valueStr, ok := value.(string) + if !ok { + return &InconclusiveMatchError{msg: fmt.Sprintf("flag semver value must be a string, got %T", value), Reason: UnresolvedFlagReasonUnsupportedDefinition} + } + switch property.Operator { + case "semver_tilde": + _, _, err = computeTildeBounds(valueStr) + case "semver_caret": + _, _, err = computeCaretBounds(valueStr) + case "semver_wildcard": + _, _, err = computeWildcardBounds(valueStr) + default: + _, err = parseSemver(valueStr) + } + if err != nil { + return &InconclusiveMatchError{msg: fmt.Sprintf("flag semver value %q is not valid for operator %s", valueStr, property.Operator), Reason: UnresolvedFlagReasonUnsupportedDefinition} + } + } + return nil +} + +// exactFilterAlwaysAmbiguous reports an exact filter that is inconclusive for any +// value, because each number it compares with is an integral JSON number. +// It runs whenever an exact filter's property is missing, so it checks the numbers +// first to skip the lowercasing in isTruthyOrFalsyPropertyValue for string filters. +func exactFilterAlwaysAmbiguous(value interface{}, matchingVersions ...int) bool { + if list, ok := value.([]interface{}); ok { + if len(list) == 0 { + return false + } + for _, item := range list { + if !containsAmbiguousExactNumber(item) { + return false + } + } + } else if !containsAmbiguousExactNumber(value) { + return false + } + explicitMatching := len(matchingVersions) > 0 && matchingVersions[0] == 2 + return explicitMatching || !isTruthyOrFalsyPropertyValue(value) +} + +func matchPropertyValue(property FlagProperty, properties Properties, matchingVersions ...int) (bool, error) { key := property.Key operator := property.Operator value := property.Value @@ -1760,22 +2157,22 @@ func matchProperty(property FlagProperty, properties Properties, matchingVersion overrideStr, ok := override_value.(string) if !ok { - return false, &InconclusiveMatchError{fmt.Sprintf("semver comparison requires string value, got %T", override_value)} + return false, &InconclusiveMatchError{msg: fmt.Sprintf("semver comparison requires string value, got %T", override_value), Reason: UnresolvedFlagReasonMissingContext} } overrideParsed, err := parseSemver(overrideStr) if err != nil { - return false, &InconclusiveMatchError{fmt.Sprintf("property value %q is not a valid semver", overrideStr)} + return false, &InconclusiveMatchError{msg: fmt.Sprintf("property value %q is not a valid semver", overrideStr), Reason: UnresolvedFlagReasonMissingContext} } valueStr, ok := value.(string) if !ok { - return false, &InconclusiveMatchError{fmt.Sprintf("flag semver value must be a string, got %T", value)} + return false, &InconclusiveMatchError{msg: fmt.Sprintf("flag semver value must be a string, got %T", value), Reason: UnresolvedFlagReasonUnsupportedDefinition} } valueParsed, err := parseSemver(valueStr) if err != nil { - return false, &InconclusiveMatchError{fmt.Sprintf("flag semver value %q is not a valid semver", valueStr)} + return false, &InconclusiveMatchError{msg: fmt.Sprintf("flag semver value %q is not a valid semver", valueStr), Reason: UnresolvedFlagReasonUnsupportedDefinition} } cmp := overrideParsed.compareTo(valueParsed) @@ -1799,22 +2196,22 @@ func matchProperty(property FlagProperty, properties Properties, matchingVersion if operator == "semver_tilde" { overrideStr, ok := override_value.(string) if !ok { - return false, &InconclusiveMatchError{fmt.Sprintf("semver comparison requires string value, got %T", override_value)} + return false, &InconclusiveMatchError{msg: fmt.Sprintf("semver comparison requires string value, got %T", override_value), Reason: UnresolvedFlagReasonMissingContext} } overrideParsed, err := parseSemver(overrideStr) if err != nil { - return false, &InconclusiveMatchError{fmt.Sprintf("property value %q is not a valid semver", overrideStr)} + return false, &InconclusiveMatchError{msg: fmt.Sprintf("property value %q is not a valid semver", overrideStr), Reason: UnresolvedFlagReasonMissingContext} } valueStr, ok := value.(string) if !ok { - return false, &InconclusiveMatchError{fmt.Sprintf("flag semver value must be a string, got %T", value)} + return false, &InconclusiveMatchError{msg: fmt.Sprintf("flag semver value must be a string, got %T", value), Reason: UnresolvedFlagReasonUnsupportedDefinition} } lower, upper, err := computeTildeBounds(valueStr) if err != nil { - return false, &InconclusiveMatchError{fmt.Sprintf("flag semver value %q is not valid for tilde operator", valueStr)} + return false, &InconclusiveMatchError{msg: fmt.Sprintf("flag semver value %q is not valid for tilde operator", valueStr), Reason: UnresolvedFlagReasonUnsupportedDefinition} } // Check: lower <= override < upper @@ -1824,22 +2221,22 @@ func matchProperty(property FlagProperty, properties Properties, matchingVersion if operator == "semver_caret" { overrideStr, ok := override_value.(string) if !ok { - return false, &InconclusiveMatchError{fmt.Sprintf("semver comparison requires string value, got %T", override_value)} + return false, &InconclusiveMatchError{msg: fmt.Sprintf("semver comparison requires string value, got %T", override_value), Reason: UnresolvedFlagReasonMissingContext} } overrideParsed, err := parseSemver(overrideStr) if err != nil { - return false, &InconclusiveMatchError{fmt.Sprintf("property value %q is not a valid semver", overrideStr)} + return false, &InconclusiveMatchError{msg: fmt.Sprintf("property value %q is not a valid semver", overrideStr), Reason: UnresolvedFlagReasonMissingContext} } valueStr, ok := value.(string) if !ok { - return false, &InconclusiveMatchError{fmt.Sprintf("flag semver value must be a string, got %T", value)} + return false, &InconclusiveMatchError{msg: fmt.Sprintf("flag semver value must be a string, got %T", value), Reason: UnresolvedFlagReasonUnsupportedDefinition} } lower, upper, err := computeCaretBounds(valueStr) if err != nil { - return false, &InconclusiveMatchError{fmt.Sprintf("flag semver value %q is not valid for caret operator", valueStr)} + return false, &InconclusiveMatchError{msg: fmt.Sprintf("flag semver value %q is not valid for caret operator", valueStr), Reason: UnresolvedFlagReasonUnsupportedDefinition} } // Check: lower <= override < upper @@ -1849,29 +2246,30 @@ func matchProperty(property FlagProperty, properties Properties, matchingVersion if operator == "semver_wildcard" { overrideStr, ok := override_value.(string) if !ok { - return false, &InconclusiveMatchError{fmt.Sprintf("semver comparison requires string value, got %T", override_value)} + return false, &InconclusiveMatchError{msg: fmt.Sprintf("semver comparison requires string value, got %T", override_value), Reason: UnresolvedFlagReasonMissingContext} } overrideParsed, err := parseSemver(overrideStr) if err != nil { - return false, &InconclusiveMatchError{fmt.Sprintf("property value %q is not a valid semver", overrideStr)} + return false, &InconclusiveMatchError{msg: fmt.Sprintf("property value %q is not a valid semver", overrideStr), Reason: UnresolvedFlagReasonMissingContext} } valueStr, ok := value.(string) if !ok { - return false, &InconclusiveMatchError{fmt.Sprintf("flag semver value must be a string, got %T", value)} + return false, &InconclusiveMatchError{msg: fmt.Sprintf("flag semver value must be a string, got %T", value), Reason: UnresolvedFlagReasonUnsupportedDefinition} } lower, upper, err := computeWildcardBounds(valueStr) if err != nil { - return false, &InconclusiveMatchError{fmt.Sprintf("flag semver value %q is not valid for wildcard operator", valueStr)} + return false, &InconclusiveMatchError{msg: fmt.Sprintf("flag semver value %q is not valid for wildcard operator", valueStr), Reason: UnresolvedFlagReasonUnsupportedDefinition} } // Check: lower <= override < upper return overrideParsed.compareTo(lower) >= 0 && overrideParsed.compareTo(upper) < 0, nil } - return false, &InconclusiveMatchError{"Unknown operator: " + operator} + // A new operator above also needs an entry in propertyOperators. + return false, &InconclusiveMatchError{msg: "Unknown operator: " + operator, Reason: UnresolvedFlagReasonUnsupportedDefinition} } @@ -2662,7 +3060,7 @@ func getHoldoutVariant(flag FeatureFlag, distinctId string, deviceId *string) (v percentage := math.Min(math.Max(*holdout.ExclusionPercentage, 0), 100) if percentage != 100 { if flag.Filters.AggregationGroupTypeIndex == nil && isDeviceBucketed(flag) && (deviceId == nil || *deviceId == "") { - return "", false, errHoldoutMissingDeviceID + return "", false, errHoldoutMissingDeviceID.withReason(UnresolvedFlagReasonMissingContext) } if holdoutHash(getBucketingID(flag, distinctId, deviceId)) > percentage/100 { return "", false, nil diff --git a/featureflags_test.go b/featureflags_test.go index deebcd3d..7f925898 100644 --- a/featureflags_test.go +++ b/featureflags_test.go @@ -1,8 +1,16 @@ package posthog import ( + "fmt" "math" + "net/http" + "net/http/httptest" + "strings" + "sync/atomic" "testing" + + json "github.com/goccy/go-json" + "github.com/stretchr/testify/require" ) func TestCalculateHash(t *testing.T) { @@ -50,3 +58,443 @@ func TestFeatureFlagsPollerStateMapGetters(t *testing.T) { t.Fatalf("getGroups() = %#v, want stored group mapping", gotGroups) } } + +const ( + planIsProFilter = `{"key": "plan", "type": "person", "operator": "exact", "value": "pro"}` + countryIsUSFilter = `{"key": "country", "type": "person", "operator": "exact", "value": "US"}` + unknownOperatorFilter = `{"key": "country", "type": "person", "operator": "is_on_the_moon", "value": "US"}` + staticCohortFilter = `{"key": "id", "type": "cohort", "value": 404}` + ageOver18Filter = `{"key": "age", "type": "person", "operator": "gt", "value": 18}` +) + +func flagDefinition(key string, conditions ...string) string { + return `{"key": "` + key + `", "active": true, "filters": {"groups": [` + strings.Join(conditions, ",") + `]}}` +} + +func flagCondition(rollout int, properties ...string) string { + return fmt.Sprintf(`{"properties": [%s], "rollout_percentage": %d}`, strings.Join(properties, ","), rollout) +} + +func flagDependencyFilter(key string, value interface{}, chain ...string) string { + encodedValue, _ := json.Marshal(value) + encodedChain, _ := json.Marshal(chain) + return fmt.Sprintf(`{"key": %q, "type": "flag", "operator": "flag_evaluates_to", "value": %s, "dependency_chain": %s}`, key, encodedValue, encodedChain) +} + +func newDefinitionsPoller(t *testing.T, definitions string) *FeatureFlagsPoller { + t.Helper() + poller := newTestPoller(t, "http://localhost") + poller.Logger = silentLogger{} + decoded, err := decodeFlagDefinitions([]byte(definitions)) + require.NoError(t, err) + poller.applyFlagDefinitions([]byte(definitions), decoded, "") + return poller +} + +func computeDefinedFlag(t *testing.T, definitions, key string, personProperties Properties) (interface{}, error) { + t.Helper() + poller := newDefinitionsPoller(t, definitions) + state := poller.state.Load() + flag, ok := state.flagsByKey[key] + require.True(t, ok, "no definition for %s", key) + return poller.computeFlagLocally(flag, "user-123", nil, Groups{}, personProperties, nil, state.cohorts, state) +} + +func requireUnresolvedReason(t *testing.T, err error, want UnresolvedFlagReason) { + t.Helper() + reason, ok := unresolvedReasonOf(err) + require.True(t, ok, "expected an inconclusive or server-required error, got %v", err) + require.Equal(t, want, reason) +} + +func TestPickUnresolvedReasonFollowsSpecPrecedence(t *testing.T) { + specOrder := []UnresolvedFlagReason{ + UnresolvedFlagReasonExperienceContinuity, + UnresolvedFlagReasonUnsupportedDefinition, + UnresolvedFlagReasonUnresolvedDependency, + UnresolvedFlagReasonMissingContext, + } + for i, higher := range specOrder { + require.Equal(t, higher, pickUnresolvedReason("", higher)) + for _, lower := range specOrder[i:] { + require.Equal(t, higher, pickUnresolvedReason(higher, lower), "%s then %s", higher, lower) + require.Equal(t, higher, pickUnresolvedReason(lower, higher), "%s then %s", lower, higher) + } + } + require.Equal(t, UnresolvedFlagReasonUnsupportedDefinition, pickUnresolvedReason(UnresolvedFlagReasonMissingContext, "")) + requireUnresolvedReason(t, &InconclusiveMatchError{msg: "no reason"}, UnresolvedFlagReasonUnsupportedDefinition) +} + +func TestMatchPropertyUnresolvedReasons(t *testing.T) { + semver := func(operator string, value interface{}) FlagProperty { + return FlagProperty{Key: "version", Operator: operator, Value: value} + } + tests := []struct { + name string + property FlagProperty + properties Properties + version int + want UnresolvedFlagReason + wantServer bool + }{ + {name: "missing property", property: FlagProperty{Key: "plan", Operator: "exact", Value: "pro"}, want: UnresolvedFlagReasonMissingContext}, + {name: "unknown operator", property: FlagProperty{Key: "plan", Operator: "is_on_the_moon", Value: "pro"}, properties: Properties{"plan": "pro"}, want: UnresolvedFlagReasonUnsupportedDefinition}, + {name: "unknown operator with missing property", property: FlagProperty{Key: "plan", Operator: "is_on_the_moon", Value: "pro"}, want: UnresolvedFlagReasonUnsupportedDefinition}, + {name: "person semver value is not a string", property: semver("semver_gt", "1.0.0"), properties: Properties{"version": 1}, want: UnresolvedFlagReasonMissingContext}, + {name: "person semver value is invalid", property: semver("semver_gt", "1.0.0"), properties: Properties{"version": "latest"}, want: UnresolvedFlagReasonMissingContext}, + {name: "filter semver value is not a string", property: semver("semver_gt", 1), properties: Properties{"version": "1.0.0"}, want: UnresolvedFlagReasonUnsupportedDefinition}, + {name: "filter semver value is invalid", property: semver("semver_tilde", "x.y"), properties: Properties{"version": "1.0.0"}, want: UnresolvedFlagReasonUnsupportedDefinition}, + {name: "invalid person and filter semver values", property: semver("semver_eq", "x.y"), properties: Properties{"version": "latest"}, want: UnresolvedFlagReasonUnsupportedDefinition}, + {name: "invalid filter semver value with missing property", property: semver("semver_caret", "x.y"), want: UnresolvedFlagReasonUnsupportedDefinition}, + {name: "ambiguous number", property: FlagProperty{Key: "age", Operator: "exact", Value: float64(30)}, properties: Properties{"age": "30"}, want: UnresolvedFlagReasonUnsupportedDefinition}, + {name: "ambiguous number with missing property", property: FlagProperty{Key: "age", Operator: "is_not", Value: float64(30)}, want: UnresolvedFlagReasonUnsupportedDefinition}, + {name: "partly ambiguous list with missing property", property: FlagProperty{Key: "age", Operator: "exact", Value: []interface{}{float64(30), "thirty"}}, want: UnresolvedFlagReasonMissingContext}, + {name: "regex requiring the server", property: FlagProperty{Key: "email", Operator: "regex", Value: `\bpro`}, properties: Properties{"email": "pro@example.com"}, want: UnresolvedFlagReasonUnsupportedDefinition, wantServer: true}, + {name: "regex requiring the server with missing property", property: FlagProperty{Key: "email", Operator: "regex", Value: `\bpro`}, want: UnresolvedFlagReasonUnsupportedDefinition}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + matched, err := matchProperty(tt.property, tt.properties, tt.version) + require.False(t, matched) + require.Equal(t, tt.wantServer, isServerEvalError(err), "%v", err) + requireUnresolvedReason(t, err, tt.want) + }) + } +} + +func TestConditionLoopDoesNotStopAtFirstInconclusiveFilter(t *testing.T) { + const dependsOnStaticCohort = `{"key": "cohort-dependency", "active": true, "filters": {"groups": [{"properties": [` + staticCohortFilter + `], "rollout_percentage": 100}]}}` + earlyExit := func(conditions ...string) string { + return `{"key": "checkout", "active": true, "filters": {"early_exit": true, "groups": [` + strings.Join(conditions, ",") + `]}}` + } + dependencyOnStaticCohort := flagDependencyFilter("cohort-dependency", true, "cohort-dependency") + andCohort := func(filters ...string) string { + return `{"7": {"type": "AND", "values": [` + strings.Join(filters, ",") + `]}}` + } + const cohortFilter = `{"key": "id", "type": "cohort", "value": 7}` + canada := NewProperties().Set("country", "CA") + tests := []struct { + name string + flags []string + cohorts string + properties Properties + want interface{} + wantReason UnresolvedFlagReason + wantServer bool + }{ + { + name: "inconclusive filter then non-matching filter", + flags: []string{flagDefinition("checkout", flagCondition(100, planIsProFilter, countryIsUSFilter))}, + properties: canada, + want: false, + }, + { + name: "static cohort then non-matching filter", + flags: []string{flagDefinition("checkout", flagCondition(100, staticCohortFilter, countryIsUSFilter))}, + properties: canada, + want: false, + }, + { + name: "static cohort first in its condition requires the server", + flags: []string{flagDefinition("checkout", flagCondition(100, staticCohortFilter, planIsProFilter), flagCondition(100))}, + wantReason: UnresolvedFlagReasonUnsupportedDefinition, + wantServer: true, + }, + { + name: "static cohort after another inconclusive filter lets later conditions resolve", + flags: []string{flagDefinition("checkout", flagCondition(100, planIsProFilter, staticCohortFilter), flagCondition(100))}, + want: true, + }, + { + name: "plain error after an inconclusive filter lets later conditions resolve", + flags: []string{flagDefinition("checkout", flagCondition(100, planIsProFilter, ageOver18Filter), flagCondition(100))}, + properties: NewProperties().Set("age", "unknown"), + want: true, + }, + { + name: "plain error after a static cohort still requires the server", + flags: []string{flagDefinition("checkout", flagCondition(100, staticCohortFilter, ageOver18Filter), flagCondition(100))}, + properties: NewProperties().Set("age", "unknown"), + wantReason: UnresolvedFlagReasonUnsupportedDefinition, + wantServer: true, + }, + { + name: "highest reason in a condition", + flags: []string{flagDefinition("checkout", flagCondition(100, planIsProFilter, unknownOperatorFilter))}, + properties: canada, + wantReason: UnresolvedFlagReasonUnsupportedDefinition, + }, + { + name: "highest reason in a condition in the reverse order", + flags: []string{flagDefinition("checkout", flagCondition(100, unknownOperatorFilter, planIsProFilter))}, + properties: canada, + wantReason: UnresolvedFlagReasonUnsupportedDefinition, + }, + { + name: "highest reason across conditions", + flags: []string{flagDefinition("checkout", flagCondition(100, unknownOperatorFilter), flagCondition(100, planIsProFilter))}, + properties: canada, + wantReason: UnresolvedFlagReasonUnsupportedDefinition, + }, + { + name: "server-required dependency does not override a higher reason", + flags: []string{ + dependsOnStaticCohort, + flagDefinition("checkout", flagCondition(100, unknownOperatorFilter), flagCondition(100, dependencyOnStaticCohort)), + }, + properties: canada, + wantReason: UnresolvedFlagReasonUnsupportedDefinition, + wantServer: true, + }, + { + name: "server-required dependency does not hide a higher reason in a later condition", + flags: []string{ + dependsOnStaticCohort, + flagDefinition("checkout", flagCondition(100, dependencyOnStaticCohort), flagCondition(100, unknownOperatorFilter)), + }, + properties: canada, + wantReason: UnresolvedFlagReasonUnsupportedDefinition, + wantServer: true, + }, + { + name: "server-required dependency then a higher reason in the same condition", + flags: []string{ + dependsOnStaticCohort, + flagDefinition("checkout", flagCondition(100, dependencyOnStaticCohort, unknownOperatorFilter)), + }, + properties: canada, + wantReason: UnresolvedFlagReasonUnsupportedDefinition, + wantServer: true, + }, + { + name: "server-required dependency then a higher reason in a cohort", + flags: []string{dependsOnStaticCohort, flagDefinition("checkout", flagCondition(100, cohortFilter))}, + cohorts: andCohort(dependencyOnStaticCohort, unknownOperatorFilter), + properties: canada, + wantReason: UnresolvedFlagReasonUnsupportedDefinition, + wantServer: true, + }, + { + name: "higher reason then a server-required dependency in a cohort", + flags: []string{dependsOnStaticCohort, flagDefinition("checkout", flagCondition(100, cohortFilter))}, + cohorts: andCohort(unknownOperatorFilter, dependencyOnStaticCohort), + properties: canada, + wantReason: UnresolvedFlagReasonUnsupportedDefinition, + wantServer: true, + }, + { + name: "server-required dependency does not hide a higher reason in a condition ruled out earlier", + flags: []string{ + dependsOnStaticCohort, + flagDefinition("checkout", flagCondition(100, countryIsUSFilter, unknownOperatorFilter), flagCondition(100, dependencyOnStaticCohort)), + }, + properties: canada, + wantReason: UnresolvedFlagReasonUnsupportedDefinition, + wantServer: true, + }, + { + name: "early exit after a condition ruled out by a later filter", + flags: []string{earlyExit(flagCondition(100, planIsProFilter, countryIsUSFilter), flagCondition(0))}, + properties: canada, + want: false, + }, + { + name: "early exit after an inconclusive condition", + flags: []string{earlyExit(flagCondition(100, planIsProFilter), flagCondition(0))}, + wantReason: UnresolvedFlagReasonMissingContext, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cohorts := tt.cohorts + if cohorts == "" { + cohorts = `{}` + } + got, err := computeDefinedFlag(t, localDefinitionsWithCohorts(cohorts, tt.flags...), "checkout", tt.properties) + if tt.wantReason == "" { + require.NoError(t, err) + require.Equal(t, tt.want, got) + return + } + require.Equal(t, tt.wantServer, isServerEvalError(err), "%v", err) + requireUnresolvedReason(t, err, tt.wantReason) + }) + } +} + +func TestInactiveExperienceContinuityFlagResolvesToFalse(t *testing.T) { + inactive := `{"key": "checkout", "active": false, "ensure_experience_continuity": true, "filters": {"groups": [{"properties": [], "rollout_percentage": 100}]}}` + got, err := computeDefinedFlag(t, localDefinitions(inactive), "checkout", nil) + require.NoError(t, err) + require.Equal(t, false, got) + + _, err = computeDefinedFlag(t, localDefinitions(continuityCheckoutDefinition), "checkout", nil) + requireUnresolvedReason(t, err, UnresolvedFlagReasonExperienceContinuity) +} + +func TestHoldoutWithoutDeviceIDReportsHighestReason(t *testing.T) { + holdoutFlag := func(properties ...string) string { + return `{"key": "checkout", "active": true, "bucketing_identifier": "device_id", "filters": {"holdout": {"id": 1, "exclusion_percentage": 50}, "groups": [` + flagCondition(100, properties...) + `]}}` + } + _, err := computeDefinedFlag(t, localDefinitions(holdoutFlag()), "checkout", nil) + requireUnresolvedReason(t, err, UnresolvedFlagReasonMissingContext) + + _, err = computeDefinedFlag(t, localDefinitions(holdoutFlag(staticCohortFilter)), "checkout", nil) + requireUnresolvedReason(t, err, UnresolvedFlagReasonUnsupportedDefinition) +} + +// definitionsSequenceServer serves each body in turn on /flags/definitions, repeating +// the last one, and counts remote evaluation requests. A body of "304" answers with +// Not Modified, and "402" with a quota limit. +func definitionsSequenceServer(t *testing.T, bodies ...string) (*httptest.Server, *atomic.Int32) { + t.Helper() + var definitionRequests atomic.Int32 + var remoteRequests atomic.Int32 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if !strings.HasPrefix(r.URL.Path, "/flags/definitions") { + remoteRequests.Add(1) + w.WriteHeader(http.StatusInternalServerError) + return + } + index := int(definitionRequests.Add(1)) - 1 + if index >= len(bodies) { + index = len(bodies) - 1 + } + if bodies[index] == "304" { + w.WriteHeader(http.StatusNotModified) + return + } + if bodies[index] == "402" { + w.WriteHeader(http.StatusPaymentRequired) + return + } + w.Header().Set("ETag", fmt.Sprintf(`"etag-%d"`, index)) + w.Write([]byte(bodies[index])) + })) + t.Cleanup(server.Close) + return server, &remoteRequests +} + +func countUnresolvableWarnings(logger *captureLogger, key string, reason UnresolvedFlagReason) int { + count := 0 + for _, warning := range logger.snapshot() { + if strings.Contains(warning, key) && strings.Contains(warning, string(reason)) { + count++ + } + } + return count +} + +func TestUnresolvableFlagWarningIsLoggedOncePerDefinition(t *testing.T) { + definitions := localDefinitions(`{"key": "checkout", "active": true, "ensure_experience_continuity": true, "filters": {"groups": [{"properties": [], "rollout_percentage": 100}], "payloads": {"true": {"color": "blue", "size": 2}}}}`) + // The same definition serialized differently, as a shared cache may store it. + reserialized := `{"cohorts":{},"flags":[{"filters":{"payloads":{"true":{"size":2,"color":"blue"}},"groups":[{"rollout_percentage":100,"properties":[]}]},"ensure_experience_continuity":true,"active":true,"key":"checkout"}],"group_type_mapping":{}}` + tests := []struct { + name string + bodies []string + cache bool + }{ + {name: "unchanged definitions", bodies: []string{definitions, definitions, definitions}}, + {name: "304 not modified", bodies: []string{definitions, "304", "304"}}, + {name: "successful shared-cache load", bodies: []string{definitions}, cache: true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + server, remoteRequests := definitionsSequenceServer(t, tt.bodies...) + logger := &captureLogger{} + poller := newTestPoller(t, server.URL) + provider := &fakeFlagDefinitionCache{shouldFetch: true} + if tt.cache { + poller = newCachingTestPoller(t, server.URL, provider) + } + poller.Logger = logger + poller.fetchNewFeatureFlags() + // With a cache provider, later polls load from the shared cache instead of the API. + provider.mu.Lock() + provider.shouldFetch = false + provider.cached = json.RawMessage(reserialized) + provider.mu.Unlock() + + for i := 0; i < 2; i++ { + poller.fetchNewFeatureFlags() + } + + if tt.cache { + _, cacheLoads, _, _ := provider.calls() + require.Equal(t, 2, cacheLoads) + } + require.Equal(t, 1, countUnresolvableWarnings(logger, "checkout", UnresolvedFlagReasonExperienceContinuity), logger.snapshot()) + require.Len(t, logger.snapshot(), 1) + require.Zero(t, remoteRequests.Load()) + }) + } +} + +func TestUnresolvableFlagWarningRepeatsWhenDefinitionChanges(t *testing.T) { + changed := `{"key": "checkout", "active": true, "ensure_experience_continuity": true, "filters": {"groups": [{"properties": [], "rollout_percentage": 50}]}}` + server, _ := definitionsSequenceServer(t, localDefinitions(continuityCheckoutDefinition), localDefinitions(changed), localDefinitions(changed)) + logger := &captureLogger{} + poller := newTestPoller(t, server.URL) + poller.Logger = logger + + for i := 0; i < 3; i++ { + poller.fetchNewFeatureFlags() + } + + require.Equal(t, 2, countUnresolvableWarnings(logger, "checkout", UnresolvedFlagReasonExperienceContinuity), logger.snapshot()) +} + +func TestUnresolvableFlagWarningForgetsRemovedFlags(t *testing.T) { + withCheckout := localDefinitions(betaUIDefinition, continuityCheckoutDefinition) + tests := []struct { + name string + removal string + }{ + {name: "removed from the definitions", removal: localDefinitions(betaUIDefinition)}, + {name: "cleared by a quota limit", removal: "402"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + server, _ := definitionsSequenceServer(t, withCheckout, tt.removal, withCheckout) + logger := &captureLogger{} + poller := newTestPoller(t, server.URL) + poller.Logger = logger + + for i := 0; i < 3; i++ { + poller.fetchNewFeatureFlags() + } + + require.Equal(t, 2, countUnresolvableWarnings(logger, "checkout", UnresolvedFlagReasonExperienceContinuity), logger.snapshot()) + require.Len(t, poller.warnedUnresolvableFlags, 1) + }) + } +} + +func TestUnresolvableFlagWarningCoversUnsupportedDefinitions(t *testing.T) { + inactiveContinuity := `{"key": "inactive-continuity", "active": false, "ensure_experience_continuity": true, "filters": {"groups": [{"properties": [], "rollout_percentage": 100}]}}` + definitions := localDefinitions( + continuityCheckoutDefinition, + inactiveContinuity, + flagDefinition("static-cohort", flagCondition(100, staticCohortFilter)), + flagDefinition("unknown-operator", flagCondition(100, unknownOperatorFilter)), + flagDefinition("malformed-semver", flagCondition(100, `{"key": "version", "type": "person", "operator": "semver_gt", "value": "x.y"}`)), + flagDefinition("missing-dependency", flagCondition(100, flagDependencyFilter("gone", true, "gone"))), + flagDefinition("circular-dependency", flagCondition(100, flagDependencyFilter("circular-dependency", true))), + flagDefinition("needs-context", flagCondition(100, planIsProFilter)), + flagDefinition("depends-on-continuity", flagCondition(100, flagDependencyFilter("checkout", true, "checkout"))), + ) + server, _ := definitionsSequenceServer(t, definitions) + logger := &captureLogger{} + poller := newTestPoller(t, server.URL) + poller.Logger = logger + + poller.fetchNewFeatureFlags() + poller.fetchNewFeatureFlags() + + warnings := logger.snapshot() + require.Len(t, warnings, 2) + require.Contains(t, warnings[0], `"checkout"`) + require.Contains(t, warnings[0], string(UnresolvedFlagReasonExperienceContinuity)) + require.Contains(t, warnings[1], string(UnresolvedFlagReasonUnsupportedDefinition)) + require.Contains(t, warnings[1], `"static-cohort, unknown-operator, malformed-semver, missing-dependency, circular-dependency"`) +} diff --git a/posthog.go b/posthog.go index 35020174..0e1bef14 100644 --- a/posthog.go +++ b/posthog.go @@ -1173,7 +1173,8 @@ type EvaluateFlagsPayload struct { // GroupProperties overrides group properties used during flag evaluation, keyed by group type. GroupProperties map[string]Properties // OnlyEvaluateLocally prevents fallback to remote /flags requests. Flags - // that cannot be resolved from loaded local definitions are omitted. + // that cannot be resolved from loaded local definitions are omitted, and + // FeatureFlagEvaluations.UnresolvedFlags lists them with a reason. OnlyEvaluateLocally bool // DisableGeoIP, when non-nil, overrides the client-level DisableGeoIP for // this evaluation only. @@ -1242,10 +1243,11 @@ func (c *client) evaluateFlagsWithContext(ctx context.Context, payload EvaluateF records := make(map[string]evaluatedFlagRecord) locallyEvaluated := make(map[string]struct{}) + unresolved := make(map[string]UnresolvedFlagReason) fallbackToRemote := true if c.featureFlagsPoller != nil { - fallbackToRemote = c.populateLocalEvaluations(records, locallyEvaluated, payload) + fallbackToRemote = c.populateLocalEvaluations(records, locallyEvaluated, unresolved, payload) } else if payload.OnlyEvaluateLocally { c.warnPersonalAPIKeyMissing("EvaluateFlags") return noopFeatureFlagEvaluations, ErrNoSecretKey @@ -1296,12 +1298,20 @@ func (c *client) evaluateFlagsWithContext(ctx context.Context, payload EvaluateF } } + // A flag the remote fallback resolved has a value, so it is not unresolved. + for key := range unresolved { + if _, resolved := records[key]; resolved { + delete(unresolved, key) + } + } + return &FeatureFlagEvaluations{ host: host, distinctId: payload.DistinctId, deviceId: payload.DeviceId, groups: payload.Groups, flags: records, + unresolved: unresolved, requestId: requestId, evaluatedAt: evaluatedAt, errorsWhileComputing: errorsWhileComputing, @@ -1310,12 +1320,13 @@ func (c *client) evaluateFlagsWithContext(ctx context.Context, payload EvaluateF }, remoteErr } -// populateLocalEvaluations fills records with locally-resolved flags. It -// returns whether the caller should fall back to a remote /flags request to +// populateLocalEvaluations fills records with locally-resolved flags and +// unresolved with the reason of each flag local evaluation found inconclusive. +// It returns whether the caller should fall back to a remote /flags request to // fill in the rest. The local-evaluation loop here mirrors // FeatureFlagsPoller.GetAllFlags but stores the rich record needed to power // $feature_flag_called events with locally_evaluated=true. -func (c *client) populateLocalEvaluations(records map[string]evaluatedFlagRecord, locallyEvaluated map[string]struct{}, payload EvaluateFlagsPayload) bool { +func (c *client) populateLocalEvaluations(records map[string]evaluatedFlagRecord, locallyEvaluated map[string]struct{}, unresolved map[string]UnresolvedFlagReason, payload EvaluateFlagsPayload) bool { poller := c.featureFlagsPoller state, err := poller.getLoadedState() if err != nil { @@ -1357,6 +1368,9 @@ func (c *client) populateLocalEvaluations(records map[string]evaluatedFlagRecord ) if err != nil { c.debugf("Unable to compute flag '%s' locally - %s", storedFlag.Key, err) + if reason, ok := unresolvedReasonOf(err); ok { + unresolved[storedFlag.Key] = reason + } fallbackToRemote = true continue }