diff --git a/.changeset/unresolved-local-flags.md b/.changeset/unresolved-local-flags.md new file mode 100644 index 0000000..98e0c72 --- /dev/null +++ b/.changeset/unresolved-local-flags.md @@ -0,0 +1,15 @@ +--- +"posthog-ruby": minor +--- + +Report flags that local evaluation could not resolve on `evaluate_flags` snapshots. `FeatureFlagEvaluations#unresolved_flags` maps each such flag key to a `PostHog::UnresolvedFlagReason` value (`experience_continuity`, `unsupported_definition`, `unresolved_dependency` or `missing_context`). `InconclusiveMatchError` and `RequiresServerEvaluation` have a new `reason` with the same values. When flag definitions load, log one warning for each flag that local evaluation can never resolve (experience continuity, a static cohort, an unknown operator, a malformed value, a regex that only the server can evaluate, or a missing or circular flag dependency), and log it again only when the definition of that flag changes. + +Behavior changes (these apply to every flag method, including `get_feature_flag`, `is_feature_enabled` and `get_all_flags`, not only to `evaluate_flags`): + +- A condition or a cohort property group no longer stops at the first filter that local evaluation can't check, unless that filter is a static cohort inside a cohort. If another filter does not match, the flag can now resolve locally. Before, such a flag fell back to a remote request, or stayed unresolved with `only_evaluate_locally: true`. +- An inactive flag with experience continuity enabled now resolves to `false` locally instead of falling back to a remote request. +- A flag that depends on a flag with experience continuity enabled is no longer evaluated locally. It falls back to a remote request, or stays unresolved with `only_evaluate_locally: true`. +- A flag that depends on a flag that needs the server, for example one with a static cohort, no longer forces a remote request at that condition. A later condition can now resolve the flag locally. +- `$feature_flag_called` events from snapshot reads report `local_evaluation_inconclusive` instead of `flag_missing` for flags in `unresolved_flags`. +- When local evaluation left a flag unresolved and the remote fallback marks it as failed, the snapshot no longer holds it as `false`. The flag is absent from the snapshot and listed in `unresolved_flags`, so `enabled?` returns the `default_value:` you pass (this reverses the 3.27.0 rule for these flags) and `get_flag` returns `nil`. A failed flag that has no local definition keeps its `false` value and still ignores `default_value:`. +- The load-time warning is logged at WARN level, so it shows without debug logging. diff --git a/.rubocop.yml b/.rubocop.yml index b57466e..dd664ba 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -20,7 +20,6 @@ Naming/PredicateMethod: - condition_match - enqueue - flush - - match_regular_property_group Style/OneClassPerFile: Exclude: diff --git a/lib/posthog.rb b/lib/posthog.rb index d4227e6..04f6b90 100644 --- a/lib/posthog.rb +++ b/lib/posthog.rb @@ -11,6 +11,7 @@ require 'posthog/logging' require 'posthog/exception_capture' require 'posthog/feature_flag_error' +require 'posthog/unresolved_flag_reason' require 'posthog/feature_flag_result' require 'posthog/feature_flag_evaluations' require 'posthog/flag_definition_cache' diff --git a/lib/posthog/client.rb b/lib/posthog/client.rb index fa7fc7f..abd23a0 100644 --- a/lib/posthog/client.rb +++ b/lib/posthog/client.rb @@ -640,7 +640,8 @@ def get_feature_flag_result( # @param [Hash] groups # @param [Hash] person_properties key-value pairs of properties to associate with the user # @param [Hash] group_properties - # @param [Boolean] only_evaluate_locally Skip the remote /flags call entirely + # @param [Boolean] only_evaluate_locally Skip the remote /flags call entirely. Flags that local evaluation + # cannot resolve are left out of the snapshot and listed in {FeatureFlagEvaluations#unresolved_flags}. # @param [Boolean, nil] disable_geoip When true, disables GeoIP lookup for remote evaluation and stamps captured # access events. # @param [Array, nil] flag_keys When set, scopes evaluation to only these flag keys. @@ -676,6 +677,7 @@ def evaluate_flags( records = {} locally_evaluated_keys = Set.new + unresolved_flags = {} flag_keys_set = flag_keys&.to_set(&:to_s) @feature_flags_poller.load_feature_flags @@ -690,7 +692,10 @@ def evaluate_flags( :_compute_flag_locally, definition, distinct_id, groups, person_properties, group_properties, snapshot: definition_snapshot ) - rescue PostHog::RequiresServerEvaluation, PostHog::InconclusiveMatchError, StandardError + rescue PostHog::RequiresServerEvaluation, PostHog::InconclusiveMatchError => e + unresolved_flags[key.to_s] = e.reason + next + rescue StandardError next end @@ -747,6 +752,8 @@ def evaluate_flags( remote_flags.each do |key, ff| key_str = key.to_s next if locally_evaluated_keys.include?(key_str) + # A failed server evaluation has no value, so a flag local evaluation left unresolved stays so. + next if ff.failed == true && unresolved_flags.key?(key_str) metadata = ff.metadata reason = ff.reason @@ -778,7 +785,8 @@ def evaluate_flags( flag_definitions_loaded_at: @feature_flags_poller.flag_definitions_loaded_at, errors_while_computing: errors_while_computing, quota_limited: quota_limited, - minimal_flag_called_events: minimal_flag_called_events + minimal_flag_called_events: minimal_flag_called_events, + unresolved_flags: unresolved_flags.reject { |key, _reason| records.key?(key) } ) end diff --git a/lib/posthog/feature_flag_error.rb b/lib/posthog/feature_flag_error.rb index 6a4421f..c425145 100644 --- a/lib/posthog/feature_flag_error.rb +++ b/lib/posthog/feature_flag_error.rb @@ -10,6 +10,7 @@ module PostHog # Error values: # ERRORS_WHILE_COMPUTING: Server returned errorsWhileComputingFlags=true # FLAG_MISSING: Requested flag not in API response + # LOCAL_EVALUATION_INCONCLUSIVE: Flag has a loaded local definition but local evaluation could not resolve it # QUOTA_LIMITED: Rate/quota limit exceeded # TIMEOUT: Request timed out # CONNECTION_ERROR: Network connectivity issue @@ -21,6 +22,7 @@ module PostHog class FeatureFlagError ERRORS_WHILE_COMPUTING = 'errors_while_computing_flags' FLAG_MISSING = 'flag_missing' + LOCAL_EVALUATION_INCONCLUSIVE = 'local_evaluation_inconclusive' QUOTA_LIMITED = 'quota_limited' TIMEOUT = 'timeout' CONNECTION_ERROR = 'connection_error' diff --git a/lib/posthog/feature_flag_evaluations.rb b/lib/posthog/feature_flag_evaluations.rb index f6d8d78..4310a77 100644 --- a/lib/posthog/feature_flag_evaluations.rb +++ b/lib/posthog/feature_flag_evaluations.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true require 'set' +require 'posthog/feature_flag_error' module PostHog # A snapshot of feature flag evaluations for one distinct_id, returned by @@ -34,6 +35,15 @@ class FeatureFlagEvaluations # @return [Time, nil] When local flag definitions were loaded. attr_reader :flag_definitions_loaded_at + # 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 not + # in {#keys}, and their accessors follow the missing-flag contract. Reading this does not count + # as an access and does not send any event. Always empty on snapshots returned by {#only} and + # {#only_accessed}. + # + # @return [Hash{String => String}] Frozen hash of flag key to an {UnresolvedFlagReason} value. + attr_reader :unresolved_flags + # @param host [Host, nil] Internal host callbacks used to record flag access events. # @param distinct_id [String, nil] The distinct id these evaluations belong to. # @param flags [Hash] Evaluated flags keyed by flag key. @@ -47,6 +57,8 @@ class FeatureFlagEvaluations # @param minimal_flag_called_events [Boolean] Server-controlled gate for minimal # `$feature_flag_called` events. # @param accessed [Array, Set, nil] Flag keys already accessed by this snapshot. + # @param unresolved_flags [Hash{String => String}, nil] Unresolved flag keys mapped to an + # {UnresolvedFlagReason} value. def initialize( host: nil, distinct_id: nil, @@ -59,7 +71,8 @@ def initialize( errors_while_computing: false, quota_limited: false, minimal_flag_called_events: false, - accessed: nil + accessed: nil, + unresolved_flags: nil ) @host = host @distinct_id = distinct_id || '' @@ -73,6 +86,7 @@ def initialize( @quota_limited = quota_limited @minimal_flag_called_events = minimal_flag_called_events == true @accessed = Set.new(accessed || []) + @unresolved_flags = (unresolved_flags || {}).dup.freeze end # @return [Array] The evaluated flag keys in this snapshot. @@ -85,8 +99,8 @@ def keys # snapshot — it was never loaded, the `/flags` request failed, or no flag with that # key exists. A flag that does have a value, including `false` and variant strings, # always wins over this default; so does a flag the server returned but marked as - # failed, which resolves to `false`. Defaults to `nil`, which keeps the historical - # `false` result for a missing flag. + # failed, which resolves to `false` unless it is in {#unresolved_flags}. Defaults to + # `nil`, which keeps the historical `false` result for a missing flag. # @return [Boolean] true when the flag is enabled, false when disabled, and the # caller-supplied default (or false when none was supplied) when the flag is missing. def enabled?(key, default_value: nil) @@ -198,7 +212,11 @@ def _record_access(key, flag) errors = [] errors << 'errors_while_computing_flags' if @errors_while_computing errors << 'quota_limited' if @quota_limited - errors << 'flag_missing' if flag.nil? + if flag.nil? && @unresolved_flags.key?(key) + errors << FeatureFlagError::LOCAL_EVALUATION_INCONCLUSIVE + elsif flag.nil? + errors << FeatureFlagError::FLAG_MISSING + end properties['$feature_flag_error'] = errors.join(',') unless errors.empty? # Emit a minimal event only when the server gate is on and the flag is diff --git a/lib/posthog/feature_flags.rb b/lib/posthog/feature_flags.rb index 595c6f9..53af982 100644 --- a/lib/posthog/feature_flags.rb +++ b/lib/posthog/feature_flags.rb @@ -9,19 +9,12 @@ require 'posthog/backoff_policy' require 'posthog/feature_flag' require 'posthog/flag_definition_cache' +require 'posthog/unresolved_flag_reason' +require 'posthog/flag_evaluation_errors' require 'digest' +require 'set' module PostHog - class InconclusiveMatchError < StandardError - end - - # RequiresServerEvaluation is raised when feature flag evaluation requires - # server-side data that is not available locally (e.g., static cohorts, - # experience continuity). This error should propagate immediately to trigger - # API fallback, unlike InconclusiveMatchError which allows trying other conditions. - class RequiresServerEvaluation < StandardError - end - # Polls and evaluates feature flag definitions for {PostHog::Client}. # # @api private @@ -29,6 +22,22 @@ class FeatureFlagsPoller include PostHog::Logging include PostHog::Utils + # When several causes apply to one flag, the spec reports the first reason in this order, so + # every SDK picks the same reason whatever order it inspects the filters in. + UNRESOLVED_REASON_PRECEDENCE = [ + UnresolvedFlagReason::EXPERIENCE_CONTINUITY, + UnresolvedFlagReason::UNSUPPORTED_DEFINITION, + UnresolvedFlagReason::UNRESOLVED_DEPENDENCY, + UnresolvedFlagReason::MISSING_CONTEXT + ].freeze + + # Operator groups that share one branch in both match_property_value and assert_filter_supported, + # so the load-time check stays in step with evaluation. + REGEX_OPERATORS = %w[regex not_regex].freeze + NUMERIC_OPERATORS = %w[gt gte lt lte].freeze + DATE_OPERATORS = %w[is_date_before is_date_after].freeze + SEMVER_COMPARISON_OPERATORS = %w[semver_eq semver_neq semver_gt semver_gte semver_lt semver_lte].freeze + # @param polling_interval [Integer, nil] Seconds between local feature flag definition polls. # Defaults to {Defaults::FeatureFlags::POLLING_INTERVAL_SECONDS}. # @param secret_key [String, nil] Credential used to fetch local evaluation definitions. Accepts either a @@ -86,6 +95,8 @@ def initialize( # from the top-level `minimal_flag_called_events` key of the local # evaluation definitions payload. false when the server does not send it. @minimal_flag_called_events = false + # Definition fingerprint per flag key already warned about as never locally resolvable. + @warned_unresolvable_flags = {} @definition_snapshot = Concurrent::AtomicReference.new({ flags: @feature_flags, flags_by_key: @feature_flags_by_key, group_type_mapping: @group_type_mapping, cohorts: @cohorts, @@ -485,6 +496,12 @@ def shutdown_poller # Class methods + def self.pick_unresolved_reason(current, reason) + return reason if current.nil? + + UNRESOLVED_REASON_PRECEDENCE.index(reason) < UNRESOLVED_REASON_PRECEDENCE.index(current) ? reason : current + end + def self.compare(lhs, rhs, operator) case operator when 'gt' @@ -547,33 +564,36 @@ def self.relative_date_parse_for_feature_flag_matching(value) end # Parse a single semver numeric identifier, rejecting empty, non-digit, or - # leading-zero values per semver 2.0.0 §2. - def self.parse_semver_numeric(part) - raise InconclusiveMatchError, 'Invalid semver format' if part.nil? || part.empty? || part !~ /^\d+$/ + # leading-zero values per semver 2.0.0 §2. `reason` names the owner of the value: + # UNSUPPORTED_DEFINITION for a filter value, MISSING_CONTEXT for a value the call passed. + def self.parse_semver_numeric(part, reason: UnresolvedFlagReason::UNSUPPORTED_DEFINITION) + if part.nil? || part.empty? || part !~ /^\d+$/ + raise InconclusiveMatchError.new('Invalid semver format', reason: reason) + end # Semver 2.0.0 §2: numeric identifiers MUST NOT include leading zeros. - raise InconclusiveMatchError, 'Invalid semver format' if part.length > 1 && part[0] == '0' + raise InconclusiveMatchError.new('Invalid semver format', reason: reason) if part.length > 1 && part[0] == '0' part.to_i end # Parse a semver string into a comparable [major, minor, patch] integer array. # Handles v-prefix, whitespace, pre-release suffixes. Defaults missing components to 0. - def self.parse_semver(value) - raise InconclusiveMatchError, 'Invalid semver format' if value.nil? + def self.parse_semver(value, reason: UnresolvedFlagReason::UNSUPPORTED_DEFINITION) + raise InconclusiveMatchError.new('Invalid semver format', reason: reason) if value.nil? text = value.to_s.strip.sub(/^[vV]/, '') - raise InconclusiveMatchError, 'Invalid semver format' if text.empty? + raise InconclusiveMatchError.new('Invalid semver format', reason: reason) if text.empty? # Strip pre-release and build metadata suffixes text = text.split('-')[0].split('+')[0] parts = text.split('.') - raise InconclusiveMatchError, 'Invalid semver format' if parts.empty? || parts[0].to_s.empty? + raise InconclusiveMatchError.new('Invalid semver format', reason: reason) if parts.empty? || parts[0].to_s.empty? - major = parse_semver_numeric(parts[0]) - minor = parts.length > 1 ? parse_semver_numeric(parts[1]) : 0 - patch = parts.length > 2 ? parse_semver_numeric(parts[2]) : 0 + major = parse_semver_numeric(parts[0], reason: reason) + minor = parts.length > 1 ? parse_semver_numeric(parts[1], reason: reason) : 0 + patch = parts.length > 2 ? parse_semver_numeric(parts[2], reason: reason) : 0 [major, minor, patch] end @@ -628,12 +648,16 @@ def self.semver_wildcard_bounds(value) cleaned = value.to_s.strip.sub(/^[vV]/, '').gsub('*', '').chomp('.') parts = cleaned.split('.').reject(&:empty?) - raise InconclusiveMatchError, 'Invalid semver wildcard format' if parts.empty? + if parts.empty? + raise InconclusiveMatchError.new('Invalid semver wildcard format', + reason: UnresolvedFlagReason::UNSUPPORTED_DEFINITION) + end numeric = parts.map do |part| parse_semver_numeric(part) rescue InconclusiveMatchError - raise InconclusiveMatchError, 'Invalid semver wildcard format' + raise InconclusiveMatchError.new('Invalid semver wildcard format', + reason: UnresolvedFlagReason::UNSUPPORTED_DEFINITION) end major = numeric[0] @@ -719,12 +743,23 @@ def self.match_property(property, property_values, cohort_properties = {}, prope property_matching_version: property_matching_version) end + begin + match_property_value(property, property_values, property_matching_version) + rescue InconclusiveMatchError => e + # An unusable filter outranks missing context, so check the filter before blaming the context. + assert_filter_supported(property) if e.reason == UnresolvedFlagReason::MISSING_CONTEXT + raise + end + end + + def self.match_property_value(property, property_values, property_matching_version) key = property[:key].to_sym value = property[:value] operator = property[:operator] || 'exact' if !property_values.key?(key) - raise InconclusiveMatchError, "Property #{key} not found in property_values" + raise InconclusiveMatchError.new("Property #{key} not found in property_values", + reason: UnresolvedFlagReason::MISSING_CONTEXT) elsif operator == 'is_not_set' return false end @@ -749,35 +784,32 @@ def self.match_property(property, property_values, cohort_properties = {}, prope string_operand(override_value).downcase(:ascii).end_with?(value.to_s.downcase(:ascii)) when 'not_ends_with' !string_operand(override_value).downcase(:ascii).end_with?(value.to_s.downcase(:ascii)) - when 'regex', 'not_regex' + when *REGEX_OPERATORS matched = match_regex(value.to_s, string_operand(override_value)) return false if matched.nil? operator == 'regex' ? matched : !matched - when 'gt', 'gte', 'lt', 'lte' - parsed_value = parse_finite_number(value) - raise InconclusiveMatchError, 'Invalid numeric condition' if parsed_value.nil? - + when *NUMERIC_OPERATORS + parsed_value = parse_numeric_filter(value) parsed_override = parse_finite_number(override_value) return false if parsed_override.nil? compare(parsed_override, parsed_value, operator) - when 'is_date_before', 'is_date_after' - override_date = PostHog::Utils.convert_to_datetime(override_value.to_s) - parsed_date = relative_date_parse_for_feature_flag_matching(value.to_s) - - parsed_date = PostHog::Utils.convert_to_datetime(value.to_s) if parsed_date.nil? - - raise InconclusiveMatchError, 'Invalid date format' unless parsed_date + when *DATE_OPERATORS + # Parses the filter before the context, so a bad filter reports unsupported_definition even + # when the context value is bad too. + parsed_date = parse_date_filter(value) + override_date = PostHog::Utils.convert_to_datetime(override_value.to_s, + reason: UnresolvedFlagReason::MISSING_CONTEXT) if operator == 'is_date_before' override_date < parsed_date elsif operator == 'is_date_after' override_date > parsed_date end - when 'semver_eq', 'semver_neq', 'semver_gt', 'semver_gte', 'semver_lt', 'semver_lte' - override_parsed = parse_semver(override_value) - flag_parsed = parse_semver(value) + when *SEMVER_COMPARISON_OPERATORS + flag_parsed = parse_semver(value, reason: UnresolvedFlagReason::UNSUPPORTED_DEFINITION) + override_parsed = parse_semver(override_value, reason: UnresolvedFlagReason::MISSING_CONTEXT) case operator when 'semver_eq' @@ -794,44 +826,116 @@ def self.match_property(property, property_values, cohort_properties = {}, prope (override_parsed <=> flag_parsed) <= 0 end when 'semver_tilde' - override_parsed = parse_semver(override_value) lower, upper = semver_tilde_bounds(value) + override_parsed = parse_semver(override_value, reason: UnresolvedFlagReason::MISSING_CONTEXT) (override_parsed <=> lower) >= 0 && (override_parsed <=> upper) == -1 when 'semver_caret' - override_parsed = parse_semver(override_value) lower, upper = semver_caret_bounds(value) + override_parsed = parse_semver(override_value, reason: UnresolvedFlagReason::MISSING_CONTEXT) (override_parsed <=> lower) >= 0 && (override_parsed <=> upper) == -1 when 'semver_wildcard' - override_parsed = parse_semver(override_value) lower, upper = semver_wildcard_bounds(value) + override_parsed = parse_semver(override_value, reason: UnresolvedFlagReason::MISSING_CONTEXT) (override_parsed <=> lower) >= 0 && (override_parsed <=> upper) == -1 else - raise InconclusiveMatchError, "Unknown operator: #{operator}" + raise InconclusiveMatchError.new("Unknown operator: #{operator}", + reason: UnresolvedFlagReason::UNSUPPORTED_DEFINITION) end end + private_class_method :match_property_value + + # Raises an unsupported_definition error when local evaluation cannot use the filter, whatever + # the call passes: an unknown operator, or a filter value that the operator cannot parse. + def self.assert_filter_supported(property) + value = property[:value] + case property[:operator] || 'exact' + when 'exact', 'is_not', 'is_set', 'is_not_set', 'icontains', 'not_icontains', 'starts_with', + 'not_starts_with', 'ends_with', 'not_ends_with' + nil + when *REGEX_OPERATORS + # An empty input is the most portable one, so this raises only for a pattern that needs the + # server whatever the value. Compile without matching, so a server-supplied pattern never runs + # while definitions load. + pattern = value.to_s + compile_regex(pattern, '') unless pattern.empty? + when *NUMERIC_OPERATORS + parse_numeric_filter(value) + when *DATE_OPERATORS + parse_date_filter(value) + when *SEMVER_COMPARISON_OPERATORS + parse_semver(value, reason: UnresolvedFlagReason::UNSUPPORTED_DEFINITION) + when 'semver_tilde' + semver_tilde_bounds(value) + when 'semver_caret' + semver_caret_bounds(value) + when 'semver_wildcard' + semver_wildcard_bounds(value) + else + raise InconclusiveMatchError.new("Unknown operator: #{property[:operator]}", + reason: UnresolvedFlagReason::UNSUPPORTED_DEFINITION) + end + end + + def self.parse_numeric_filter(value) + parsed_value = parse_finite_number(value) + if parsed_value.nil? + raise InconclusiveMatchError.new('Invalid numeric condition', + reason: UnresolvedFlagReason::UNSUPPORTED_DEFINITION) + end + + parsed_value + end + private_class_method :parse_numeric_filter + + def self.parse_date_filter(value) + parsed_date = relative_date_parse_for_feature_flag_matching(value.to_s) || + PostHog::Utils.convert_to_datetime(value.to_s, + reason: UnresolvedFlagReason::UNSUPPORTED_DEFINITION) + return parsed_date if parsed_date + + raise InconclusiveMatchError.new('Invalid date format', reason: UnresolvedFlagReason::UNSUPPORTED_DEFINITION) + end + private_class_method :parse_date_filter def self.match_regex(pattern, value) return true if pattern.empty? + regex = compile_regex(pattern, value) + return nil if regex.nil? + + !regex.match(value).nil? + rescue InconclusiveMatchError + raise + rescue StandardError + raise InconclusiveMatchError.new('Regex execution requires server evaluation', + reason: UnresolvedFlagReason::UNSUPPORTED_DEFINITION) + end + private_class_method :match_regex + + # Returns nil for a pattern that Ruby cannot compile. + def self.compile_regex(pattern, value) unless portable_regex?(pattern, value) - raise InconclusiveMatchError, 'Regex compatibility requires server evaluation' + raise InconclusiveMatchError.new('Regex compatibility requires server evaluation', + reason: UnresolvedFlagReason::UNSUPPORTED_DEFINITION) end - begin - regex = Regexp.new(pattern) - rescue RegexpError => e - # Ruby's repeat-count limit is smaller than the service's syntax limit. - raise InconclusiveMatchError, 'Regex compilation requires server evaluation' if e.message.include?('too big') - - return nil + Regexp.new(pattern) + rescue RegexpError => e + # Ruby's repeat-count limit is smaller than the service's syntax limit. + if e.message.include?('too big') + raise InconclusiveMatchError.new('Regex compilation requires server evaluation', + reason: UnresolvedFlagReason::UNSUPPORTED_DEFINITION) end - !regex.match(value).nil? + + nil rescue InconclusiveMatchError raise rescue StandardError - raise InconclusiveMatchError, 'Regex execution requires server evaluation' + # For example, a pattern with an invalid byte sequence. + raise InconclusiveMatchError.new('Regex compatibility requires server evaluation', + reason: UnresolvedFlagReason::UNSUPPORTED_DEFINITION) end - private_class_method :match_regex + private_class_method :compile_regex # Native compilation alone cannot establish compatibility with the flags service's Rust engine. # Keep ordinary scalar matching local, but defer engine-specific syntax and backtracking assertions. @@ -957,8 +1061,10 @@ def self.match_cohort(property, property_values, cohort_properties, property_mat property_group = find_cohort_property(cohort_properties, cohort_id) unless property_group - raise RequiresServerEvaluation, - "cohort #{cohort_id} not found in local cohorts - likely a static cohort that requires server evaluation" + raise RequiresServerEvaluation.new( + "cohort #{cohort_id} not found in local cohorts - likely a static cohort that requires server evaluation", + reason: UnresolvedFlagReason::UNSUPPORTED_DEFINITION + ) end match_property_group(property_group, property_values, cohort_properties, @@ -974,11 +1080,17 @@ def self.match_property_group(property_group, property_values, cohort_properties return true if properties.nil? || properties.empty? if nested_property_group?(properties) - match_nested_property_group(properties, group_type, property_values, cohort_properties, - property_matching_version: property_matching_version) + property_group_items_match?(properties, group_type) do |group| + match_property_group(group, property_values, cohort_properties, + property_matching_version: property_matching_version) + end else - match_regular_property_group(properties, group_type, property_values, cohort_properties, - property_matching_version: property_matching_version) + property_group_items_match?(properties, group_type) do |prop| + PostHog::Utils.symbolize_keys!(prop) + matches = match_property(prop, property_values, cohort_properties, + property_matching_version: property_matching_version) + prop[:negation] ? !matches : matches + end end end @@ -1003,59 +1115,32 @@ def self.nested_property_group?(properties) first_property.key?(:values) || first_property.key?('values') end - def self.match_nested_property_group(properties, group_type, property_values, cohort_properties, - property_matching_version: 1) - case group_type - when 'AND' - properties.each do |property| - return false unless match_property_group(property, property_values, cohort_properties, - property_matching_version: property_matching_version) - end - true - when 'OR' - properties.each do |property| - return true if match_property_group(property, property_values, cohort_properties, - property_matching_version: property_matching_version) - end - false - else - raise InconclusiveMatchError, "Unknown property group type: #{group_type}" + # Combines the items of a cohort property group with AND or OR. The block evaluates one item. + # RequiresServerEvaluation is not an InconclusiveMatchError, so it propagates at once. + def self.property_group_items_match?(items, group_type) + unless %w[AND OR].include?(group_type) + raise InconclusiveMatchError.new("Unknown property group type: #{group_type}", + reason: UnresolvedFlagReason::UNSUPPORTED_DEFINITION) end - end - - def self.match_regular_property_group(properties, group_type, property_values, cohort_properties, - property_matching_version: 1) - # Validate group type upfront - raise InconclusiveMatchError, "Unknown property group type: #{group_type}" unless %w[AND OR].include?(group_type) - - error_matching_locally = false - properties.each do |prop| - PostHog::Utils.symbolize_keys!(prop) - - matches = match_property(prop, property_values, cohort_properties, - property_matching_version: property_matching_version) - - negated = prop[:negation] || false - final_result = negated ? !matches : matches - - # Short-circuit based on group type + inconclusive_reason = nil + items.each do |item| + matches = yield(item) if group_type == 'AND' - return false unless final_result - elsif final_result # group_type == 'OR' + return false unless matches + elsif matches return true end - rescue RequiresServerEvaluation - # Immediately propagate - this condition requires server-side data - raise rescue InconclusiveMatchError => e - PostHog::Logging.logger&.debug("Failed to compute property #{prop} locally: #{e}") - error_matching_locally = true + PostHog::Logging.logger&.debug("Failed to compute property #{item} locally: #{e}") + inconclusive_reason = pick_unresolved_reason(inconclusive_reason, e.reason) end - raise InconclusiveMatchError, "can't match cohort without a given cohort property value" if error_matching_locally + unless inconclusive_reason.nil? + raise InconclusiveMatchError.new("can't match cohort without a given cohort property value", + reason: inconclusive_reason) + end - # If we reach here, return default based on group type group_type == 'AND' end @@ -1069,48 +1154,38 @@ def self.match_regular_property_group(properties, group_type, property_values, c # @return [Boolean] True if all dependencies in the chain evaluate to true, false otherwise def evaluate_flag_dependency(property, evaluation_cache, distinct_id, properties, cohort_properties, snapshot: _evaluation_snapshot) - if property[:operator] != 'flag_evaluates_to' - # Should never happen, but just in case - raise InconclusiveMatchError, "Operator #{property[:operator]} not supported for flag dependencies" - end - if snapshot[:flags_by_key].nil? || evaluation_cache.nil? # Cannot evaluate flag dependencies without required context - raise InconclusiveMatchError, - "Cannot evaluate flag dependency on '#{property[:key] || 'unknown'}' " \ - 'without feature flags loaded or evaluation_cache' + raise InconclusiveMatchError.new( + "Cannot evaluate flag dependency on '#{property[:key] || 'unknown'}' " \ + 'without feature flags loaded or evaluation_cache', + reason: UnresolvedFlagReason::UNSUPPORTED_DEFINITION + ) end - # Check if dependency_chain is present - it should always be provided for flag dependencies - unless property.key?(:dependency_chain) - # Missing dependency_chain indicates malformed server data - raise InconclusiveMatchError, - "Flag dependency property for '#{property[:key] || 'unknown'}' " \ - "is missing required 'dependency_chain' field" + # A dependency with no definition outranks one that is only inconclusive, so this checks the whole + # chain before evaluating any of it. Nothing is cached for it, so a later lookup reports the same. + definition_error = FeatureFlagsPoller.flag_dependency_definition_error(property, snapshot[:flags_by_key]) + unless definition_error.nil? + raise InconclusiveMatchError.new(definition_error, reason: UnresolvedFlagReason::UNSUPPORTED_DEFINITION) end dependency_chain = property[:dependency_chain] - # Handle circular dependency (empty chain means circular) - if dependency_chain.empty? - PostHog::Logging.logger&.debug("Circular dependency detected for flag: #{property[:key]}") - raise InconclusiveMatchError, - "Circular dependency detected for flag '#{property[:key] || 'unknown'}'" - end - # Evaluate all dependencies in the chain order dependency_chain.each do |dep_flag_key| unless evaluation_cache.key?(dep_flag_key) # Need to evaluate this dependency first dep_flag = snapshot[:flags_by_key][dep_flag_key] - if dep_flag.nil? - # Missing flag dependency - cannot evaluate locally - evaluation_cache[dep_flag_key] = nil - raise InconclusiveMatchError, - "Cannot evaluate flag dependency '#{dep_flag_key}' - flag not found in local flags" - elsif !dep_flag[:active] + if !dep_flag[:active] # Check if the flag is active (same check as in _compute_flag_locally) evaluation_cache[dep_flag_key] = false + elsif dep_flag[:ensure_experience_continuity] + evaluation_cache[dep_flag_key] = nil + raise InconclusiveMatchError.new( + "Cannot evaluate flag dependency '#{dep_flag_key}' - flag has experience continuity enabled", + reason: UnresolvedFlagReason::UNRESOLVED_DEPENDENCY + ) else # Recursively evaluate the dependency using existing instance method begin @@ -1123,11 +1198,11 @@ def evaluate_flag_dependency(property, evaluation_cache, distinct_id, properties snapshot: snapshot ) evaluation_cache[dep_flag_key] = dep_result - rescue InconclusiveMatchError => e + rescue InconclusiveMatchError, RequiresServerEvaluation => e # If we can't evaluate a dependency, store nil and propagate the error evaluation_cache[dep_flag_key] = nil - raise InconclusiveMatchError, - "Cannot evaluate flag dependency '#{dep_flag_key}': #{e.message}" + raise InconclusiveMatchError.new("Cannot evaluate flag dependency '#{dep_flag_key}': #{e.message}", + reason: UnresolvedFlagReason::UNRESOLVED_DEPENDENCY) end end end @@ -1136,8 +1211,8 @@ def evaluate_flag_dependency(property, evaluation_cache, distinct_id, properties cached_result = evaluation_cache[dep_flag_key] if cached_result.nil? # Previously inconclusive - raise error again - raise InconclusiveMatchError, - "Flag dependency '#{dep_flag_key}' was previously inconclusive" + raise InconclusiveMatchError.new("Flag dependency '#{dep_flag_key}' was previously inconclusive", + reason: UnresolvedFlagReason::UNRESOLVED_DEPENDENCY) elsif !cached_result # Definitive False result - dependency failed return false @@ -1183,8 +1258,96 @@ def self.matches_dependency_value(expected_value, actual_value) false end + # The unresolved reason that comes from the flag's definition alone, so it is known when + # definitions load. Matches the reason local evaluation reports for the same cause. + def self.definition_unresolved_reason(flag, flags_by_key, cohorts, group_type_mapping) + return nil unless flag[:active] + return UnresolvedFlagReason::EXPERIENCE_CONTINUITY if flag[:ensure_experience_continuity] + + flag_filters = flag[:filters] || {} + aggregation = flag_filters[:aggregation_group_type_index] + if !aggregation.nil? && group_type_mapping[aggregation.to_s.to_sym].nil? + return UnresolvedFlagReason::UNSUPPORTED_DEFINITION + end + + unsupported = (flag_filters[:groups] || []).any? do |condition| + (condition[:properties] || []).any? do |property| + unsupported_property?(property, flags_by_key, cohorts, Set.new) + end + end + unsupported ? UnresolvedFlagReason::UNSUPPORTED_DEFINITION : nil + end + + # Why a flag dependency filter can never evaluate locally, whatever the call passes, or nil. + # Evaluation and the load-time warning both use it, so the warning matches the reason evaluation reports. + def self.flag_dependency_definition_error(property, flags_by_key) + operator = property[:operator] + return "Operator #{operator} not supported for flag dependencies" if operator != 'flag_evaluates_to' + + key = property[:key] || 'unknown' + dependency_chain = property[:dependency_chain] + unless property.key?(:dependency_chain) + return "Flag dependency property for '#{key}' is missing required 'dependency_chain' field" + end + unless dependency_chain.is_a?(Array) + return "Flag dependency property for '#{key}' has an invalid 'dependency_chain' field" + end + return "Circular dependency detected for flag '#{key}'" if dependency_chain.empty? + + missing_dependency = dependency_chain.find { |dep_flag_key| flags_by_key[dep_flag_key].nil? } + return nil if missing_dependency.nil? + + "Cannot evaluate flag dependency '#{missing_dependency}' - flag not found in local flags" + end + + def self.unsupported_property?(property, flags_by_key, cohorts, visited_cohort_ids) + case property[:type] + when 'flag' + !flag_dependency_definition_error(property, flags_by_key).nil? + when 'cohort' + cohort_id = property[:value].to_s + property_group = find_cohort_property(cohorts, cohort_id) + return true if property_group.nil? + return false unless visited_cohort_ids.add?(cohort_id) + + unsupported_property_group?(property_group, flags_by_key, cohorts, visited_cohort_ids) + else + unsupported_filter?(property) + end + end + + def self.unsupported_property_group?(property_group, flags_by_key, cohorts, visited_cohort_ids) + properties = extract_value(property_group, :values) + return false if properties.nil? || properties.empty? + return true unless %w[AND OR].include?(extract_value(property_group, :type)) + + if nested_property_group?(properties) + return properties.any? do |group| + unsupported_property_group?(group, flags_by_key, cohorts, visited_cohort_ids) + end + end + + properties.any? do |property| + filter = PostHog::Utils.symbolize_keys(property) + if filter[:type] == 'cohort' + unsupported_property?(filter, flags_by_key, cohorts, visited_cohort_ids) + else + # Like evaluation, a cohort group checks a flag filter as an ordinary property. + unsupported_filter?(filter) + end + end + end + + def self.unsupported_filter?(property) + assert_filter_supported(property) + false + rescue InconclusiveMatchError + true + end + private_class_method :extract_value, :find_cohort_property, :nested_property_group?, - :match_nested_property_group, :match_regular_property_group + :property_group_items_match?, :assert_filter_supported, + :unsupported_property?, :unsupported_property_group?, :unsupported_filter? # Publish/capture all matching state together so a poll cannot switch semantics # (or dependency/cohort definitions) halfway through one local evaluation. @@ -1194,10 +1357,14 @@ def _evaluation_snapshot def _compute_flag_locally(flag, distinct_id, groups = {}, person_properties = {}, group_properties = {}, snapshot: _evaluation_snapshot) - raise RequiresServerEvaluation, 'Flag has experience continuity enabled' if flag[:ensure_experience_continuity] - + # An inactive flag is false whatever its experience continuity setting. return false unless flag[:active] + if flag[:ensure_experience_continuity] + raise RequiresServerEvaluation.new('Flag has experience continuity enabled', + reason: UnresolvedFlagReason::EXPERIENCE_CONTINUITY) + end + # Create evaluation cache for flag dependencies evaluation_cache = {} @@ -1222,7 +1389,8 @@ def _compute_flag_locally(flag, distinct_id, groups = {}, person_properties = {} "[FEATURE FLAGS] Unknown group type index #{aggregation_group_type_index} for feature flag #{flag[:key]}" ) # failover to `/flags/` - raise InconclusiveMatchError, 'Flag has unknown group type index' + raise InconclusiveMatchError.new('Flag has unknown group type index', + reason: UnresolvedFlagReason::UNSUPPORTED_DEFINITION) end group_name_symbol = group_name.to_sym @@ -1267,7 +1435,7 @@ def match_feature_flag_properties(flag, distinct_id, properties, evaluation_cach flag_conditions = flag_filters[:groups] || [] flag_aggregation = flag_filters[:aggregation_group_type_index] early_exit = flag_filters[:early_exit] == true - is_inconclusive = false + inconclusive_reason = nil result = nil # NOTE: This NEEDS to be `each` because `each_key` breaks @@ -1295,7 +1463,9 @@ def match_feature_flag_properties(flag, distinct_id, properties, evaluation_cach next end unless group_properties.key?(group_name.to_sym) - is_inconclusive = true + inconclusive_reason = FeatureFlagsPoller.pick_unresolved_reason( + inconclusive_reason, UnresolvedFlagReason::MISSING_CONTEXT + ) next end effective_properties = group_properties[group_name.to_sym] @@ -1322,16 +1492,17 @@ def match_feature_flag_properties(flag, distinct_id, properties, evaluation_cach rescue RequiresServerEvaluation # Static cohort or other missing server-side data - must fallback to API raise - rescue InconclusiveMatchError + rescue InconclusiveMatchError => e # Evaluation error (bad regex, invalid date, missing property, etc.) # Track that we had an inconclusive match, but try other conditions - is_inconclusive = true + inconclusive_reason = FeatureFlagsPoller.pick_unresolved_reason(inconclusive_reason, e.reason) end if !result.nil? return result - elsif is_inconclusive - raise InconclusiveMatchError, "Can't determine if feature flag is enabled or not with given properties" + elsif !inconclusive_reason.nil? + raise InconclusiveMatchError.new("Can't determine if feature flag is enabled or not with given properties", + reason: inconclusive_reason) end # We can only return False when all conditions are False @@ -1356,16 +1527,42 @@ def condition_match_outcome(flag, distinct_id, condition, properties, evaluation rollout_percentage = condition[:rollout_percentage] unless (condition[:properties] || []).empty? - unless condition[:properties].all? do |prop| - if prop[:type] == 'flag' - evaluate_flag_dependency(prop, evaluation_cache, distinct_id, properties, cohort_properties, - snapshot: snapshot) - else - FeatureFlagsPoller.match_property(prop, properties, cohort_properties, - property_matching_version: snapshot[:property_matching_version]) - end + # 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. + inconclusive_reason = nil + server_evaluation_error = nil + condition[:properties].each do |prop| + matches = + if prop[:type] == 'flag' + evaluate_flag_dependency(prop, evaluation_cache, distinct_id, properties, cohort_properties, + snapshot: snapshot) + else + FeatureFlagsPoller.match_property(prop, properties, cohort_properties, + property_matching_version: snapshot[:property_matching_version]) + end + return :no_match unless matches + rescue RequiresServerEvaluation => e + # 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. + server_evaluation_error ||= e if inconclusive_reason.nil? + inconclusive_reason = FeatureFlagsPoller.pick_unresolved_reason(inconclusive_reason, e.reason) + rescue InconclusiveMatchError => e + inconclusive_reason = FeatureFlagsPoller.pick_unresolved_reason(inconclusive_reason, e.reason) + rescue StandardError + raise if inconclusive_reason.nil? + + # The condition is already inconclusive, so an unexpected error in a later filter must not + # fail the whole flag. Later conditions can still resolve it. + break end - return :no_match + + # Raise the original error type: match_feature_flag_properties stops at it and falls back to the + # server instead of trying later conditions. + raise server_evaluation_error unless server_evaluation_error.nil? + unless inconclusive_reason.nil? + raise InconclusiveMatchError.new("Can't determine if a condition of flag #{flag[:key]} matches", + reason: inconclusive_reason) end return :match if rollout_percentage.nil? @@ -1566,6 +1763,8 @@ def _apply_flag_definitions(data) end new_group_type_mapping = Concurrent::Hash[deep_symbolize_keys(group_type_mapping)] new_cohorts = Concurrent::Hash[deep_symbolize_keys(cohorts)] + unresolvable_flags = _unresolvable_flags(new_flags, new_by_key, new_cohorts, new_group_type_mapping) + unwarned_flags = nil # A read of the new definitions must not record its event before the tracker reset. @flag_definitions_update_mutex.synchronize do @@ -1582,11 +1781,57 @@ def _apply_flag_definitions(data) @minimal_flag_called_events = minimal_flag_called_events == true @flag_definitions_loaded_at.value = (Time.now.to_f * 1000).to_i @loaded_flags_successfully_once.make_true if @loaded_flags_successfully_once.false? + # Rebuilt from the current definitions, so a flag that is deleted or becomes resolvable is + # forgotten and warns again if it later returns to an unresolvable definition. + unwarned_flags = unresolvable_flags.reject { |key, entry| @warned_unresolvable_flags[key] == entry } + @warned_unresolvable_flags = unresolvable_flags @on_flag_definitions_updated&.call end + _warn_about_unresolvable_flags(unwarned_flags) logger.debug "Loaded #{new_flags.length} feature flags and #{new_cohorts.length} cohorts" end + # Flag key => [reason, definition] for each flag that local evaluation never resolves. Hash#== compares + # the definition by content and ignores key order, so a reordered copy does not warn again. + def _unresolvable_flags(flags, flags_by_key, cohorts, group_type_mapping) + flags.each_with_object({}) do |flag, unresolvable| + reason = self.class.definition_unresolved_reason(flag, flags_by_key, cohorts, group_type_mapping) + next if reason.nil? || flag[:key].nil? + + unresolvable[flag[:key]] = [reason, flag] + rescue StandardError + # Evaluation reports a definition this malformed when the flag is used. + next + end + end + + def _warn_about_unresolvable_flags(unresolvable_flags) + keys_by_reason = unresolvable_flags.keys.group_by { |key| unresolvable_flags[key].first } + + experience_continuity_keys = keys_by_reason[UnresolvedFlagReason::EXPERIENCE_CONTINUITY] + if experience_continuity_keys + logger.warn( + "[FEATURE FLAGS] #{experience_continuity_keys.length} flag(s) have experience continuity enabled and " \ + "can't be evaluated locally (reason: #{UnresolvedFlagReason::EXPERIENCE_CONTINUITY}): " \ + "#{experience_continuity_keys.join(', ')}. Evaluating them makes a remote request, or leaves them " \ + 'unresolved with only_evaluate_locally: true. To evaluate them locally, disable experience continuity ' \ + 'on these flags in PostHog.' + ) + end + + unsupported_definition_keys = keys_by_reason[UnresolvedFlagReason::UNSUPPORTED_DEFINITION] + return unless unsupported_definition_keys + + logger.warn( + "[FEATURE FLAGS] #{unsupported_definition_keys.length} flag(s) use something local evaluation doesn't " \ + 'support, such as a static cohort, an unknown operator, a malformed value, or a missing or circular flag ' \ + "dependency (reason: #{UnresolvedFlagReason::UNSUPPORTED_DEFINITION}): " \ + "#{unsupported_definition_keys.join(', ')}. Evaluating them makes a remote request when evaluation " \ + 'reaches that part, or leaves them unresolved with only_evaluate_locally: true. To evaluate them ' \ + 'locally, change these flags in PostHog or upgrade posthog-ruby.' + ) + end + def _request_feature_flag_definitions(etag: nil) uri = URI("#{@host}/flags/definitions") uri.query = URI.encode_www_form([['token', @project_api_key], %w[send_cohorts true]]) diff --git a/lib/posthog/flag_evaluation_errors.rb b/lib/posthog/flag_evaluation_errors.rb new file mode 100644 index 0000000..56a2602 --- /dev/null +++ b/lib/posthog/flag_evaluation_errors.rb @@ -0,0 +1,35 @@ +# frozen_string_literal: true + +require 'posthog/unresolved_flag_reason' + +module PostHog + # InconclusiveMatchError is raised when local evaluation cannot determine a flag's + # value. {#reason} says why, as reported by {FeatureFlagEvaluations#unresolved_flags}. + class InconclusiveMatchError < StandardError + # @return [String] One of the {UnresolvedFlagReason} values. + attr_reader :reason + + # @param message [String, nil] + # @param reason [String] One of the {UnresolvedFlagReason} values. + def initialize(message = nil, reason: UnresolvedFlagReason::UNSUPPORTED_DEFINITION) + super(message) + @reason = reason + end + end + + # RequiresServerEvaluation is raised when feature flag evaluation requires + # server-side data that is not available locally (e.g., static cohorts, + # experience continuity). This error should propagate immediately to trigger + # API fallback, unlike InconclusiveMatchError which allows trying other conditions. + class RequiresServerEvaluation < StandardError + # @return [String] One of the {UnresolvedFlagReason} values. + attr_reader :reason + + # @param message [String, nil] + # @param reason [String] One of the {UnresolvedFlagReason} values. + def initialize(message = nil, reason: UnresolvedFlagReason::UNSUPPORTED_DEFINITION) + super(message) + @reason = reason + end + end +end diff --git a/lib/posthog/unresolved_flag_reason.rb b/lib/posthog/unresolved_flag_reason.rb new file mode 100644 index 0000000..188596e --- /dev/null +++ b/lib/posthog/unresolved_flag_reason.rb @@ -0,0 +1,20 @@ +# frozen_string_literal: true + +module PostHog + # Why local evaluation left a flag without a value, as reported by + # {FeatureFlagEvaluations#unresolved_flags}. + # + # Reason values: + # EXPERIENCE_CONTINUITY: The flag has experience continuity enabled, which local evaluation never resolves + # UNSUPPORTED_DEFINITION: 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 + # UNRESOLVED_DEPENDENCY: A flag this flag depends on has a loaded definition but was unresolved + # MISSING_CONTEXT: The call did not pass a property or group property the flag needs, or passed it in a + # form local evaluation cannot use + module UnresolvedFlagReason + EXPERIENCE_CONTINUITY = 'experience_continuity' + UNSUPPORTED_DEFINITION = 'unsupported_definition' + UNRESOLVED_DEPENDENCY = 'unresolved_dependency' + MISSING_CONTEXT = 'missing_context' + end +end diff --git a/lib/posthog/utils.rb b/lib/posthog/utils.rb index 0ce5d6f..141ee77 100644 --- a/lib/posthog/utils.rb +++ b/lib/posthog/utils.rb @@ -1,9 +1,8 @@ # frozen_string_literal: true -module PostHog - class InconclusiveMatchError < StandardError - end +require 'posthog/flag_evaluation_errors' +module PostHog # Utility helpers used internally by the SDK. # # @api private @@ -92,7 +91,7 @@ def seconds_to_utc_offset(seconds, colon = true) seconds.abs / 3600, (seconds.abs % 3600) / 60) end - def convert_to_datetime(value) + def convert_to_datetime(value, reason:) if value.respond_to?(:strftime) value @@ -100,10 +99,10 @@ def convert_to_datetime(value) begin DateTime.parse(value) rescue ArgumentError - raise InconclusiveMatchError, "#{value} is not in a valid date format" + raise InconclusiveMatchError.new("#{value} is not in a valid date format", reason: reason) end else - raise InconclusiveMatchError, 'The date provided must be a string or date object' + raise InconclusiveMatchError.new('The date provided must be a string or date object', reason: reason) end end diff --git a/public_api_snapshot.txt b/public_api_snapshot.txt index 277f876..97e7eb5 100644 --- a/public_api_snapshot.txt +++ b/public_api_snapshot.txt @@ -71,6 +71,7 @@ instance_method PostHog::FeatureFlagEvaluations#keys() instance_method PostHog::FeatureFlagEvaluations#only(keys) instance_method PostHog::FeatureFlagEvaluations#only_accessed() instance_method PostHog::FeatureFlagEvaluations#request_id() +instance_method PostHog::FeatureFlagEvaluations#unresolved_flags() constant PostHog::FeatureFlagEvaluations::EVALUATED_LOCALLY_REASON: String class PostHog::FeatureFlagEvaluations::EvaluatedFlagRecord < Struct instance_method PostHog::FeatureFlagEvaluations::EvaluatedFlagRecord#enabled() @@ -118,6 +119,7 @@ module PostHog::FlagDefinitionCacheProvider class_method PostHog::FlagDefinitionCacheProvider.validate!(provider) constant PostHog::FlagDefinitionCacheProvider::REQUIRED_METHODS: Array class PostHog::InconclusiveMatchError < StandardError +instance_method PostHog::InconclusiveMatchError#reason() module PostHog::Logging instance_method PostHog::Logging#logger() class_method PostHog::Logging.included(base) @@ -269,12 +271,18 @@ class_method PostHog::MCP::UserIdentity.new(*arg0) module PostHog::Rails constant PostHog::Rails::VERSION: String class PostHog::RequiresServerEvaluation < StandardError +instance_method PostHog::RequiresServerEvaluation#reason() class PostHog::SendFeatureFlagsOptions instance_method PostHog::SendFeatureFlagsOptions#group_properties() instance_method PostHog::SendFeatureFlagsOptions#only_evaluate_locally() instance_method PostHog::SendFeatureFlagsOptions#person_properties() instance_method PostHog::SendFeatureFlagsOptions#to_h() class_method PostHog::SendFeatureFlagsOptions.from_hash(hash) +module PostHog::UnresolvedFlagReason +constant PostHog::UnresolvedFlagReason::EXPERIENCE_CONTINUITY: String +constant PostHog::UnresolvedFlagReason::MISSING_CONTEXT: String +constant PostHog::UnresolvedFlagReason::UNRESOLVED_DEPENDENCY: String +constant PostHog::UnresolvedFlagReason::UNSUPPORTED_DEFINITION: String constant PostHog::VERSION: String # posthog-rails diff --git a/spec/posthog/feature_flag_evaluations_spec.rb b/spec/posthog/feature_flag_evaluations_spec.rb index 3660fd7..fd8dd46 100644 --- a/spec/posthog/feature_flag_evaluations_spec.rb +++ b/spec/posthog/feature_flag_evaluations_spec.rb @@ -690,5 +690,253 @@ def capture_stderr expect(snapshot.get_flag_payload('variant-flag')).to eq('key' => 'value') end end + + # Mirrors the @unresolved_flags_capable scenarios of the evaluate-flags spec. + describe 'unresolved flags' do + let(:definitions_endpoint) { 'https://us.i.posthog.com/flags/definitions?token=testsecret&send_cohorts=true' } + let(:plan_is_pro) { { key: 'plan', operator: 'exact', value: 'pro', type: 'person' } } + let(:static_cohort) { { key: 'id', value: 42, type: 'cohort' } } + let(:beta_ui) { flag_definition('beta-ui') } + let(:checkout_with_continuity) { flag_definition('checkout', ensure_experience_continuity: true) } + + def flag_definition(key, *properties, **extra) + { id: 1, key: key, active: true, filters: { groups: [{ properties: properties, rollout_percentage: 100 }] } } + .merge(extra) + end + + def depends_on(key) + { key: key, type: 'flag', value: true, operator: 'flag_evaluates_to', dependency_chain: [key] } + end + + def local_client(*flags) + stub_request(:get, definitions_endpoint).to_return(status: 200, body: { flags: flags }.to_json) + Client.new(api_key: API_KEY, personal_api_key: API_KEY, test_mode: true) + end + + def flag_called_events(client) + drain_messages(client) + .select { |m| m[:event] == '$feature_flag_called' } + .to_h { |m| [m[:properties]['$feature_flag'], m[:properties]] } + end + + it 'reports an experience continuity flag as unresolved in local-only evaluation' do + c = local_client(beta_ui, checkout_with_continuity) + + snapshot = c.evaluate_flags('user-123', only_evaluate_locally: true) + + expect(snapshot.keys).to eq(['beta-ui']) + expect(snapshot.unresolved_flags).to eq('checkout' => UnresolvedFlagReason::EXPERIENCE_CONTINUITY) + expect(snapshot.unresolved_flags).to be_frozen + expect(WebMock).not_to have_requested(:post, FLAGS_ENDPOINT) + expect(flag_called_events(c)).to eq({}) + expect(snapshot.only_accessed.keys).to eq([]) + expect(snapshot.only(%w[beta-ui checkout]).unresolved_flags).to eq({}) + expect(snapshot.get_flag('beta-ui')).to be(true) + end + + # The spec's device-id row does not apply: this SDK has no device-id bucketing. + [ + ['a missing person property', :plan_only, {}, UnresolvedFlagReason::MISSING_CONTEXT], + ['an operator this SDK does not recognize', :unknown_operator, { plan: 'enterprise' }, + UnresolvedFlagReason::UNSUPPORTED_DEFINITION], + ['a static cohort', :static_cohort_only, {}, UnresolvedFlagReason::UNSUPPORTED_DEFINITION], + ['a static cohort and a missing person property', :static_cohort_and_plan, {}, + UnresolvedFlagReason::UNSUPPORTED_DEFINITION], + ['a missing person property and a static cohort', :plan_and_static_cohort, {}, + UnresolvedFlagReason::UNSUPPORTED_DEFINITION] + ].each do |cause, filters, person_properties, reason| + it "reports #{reason} for #{cause}" do + properties = { + plan_only: [plan_is_pro], + unknown_operator: [{ key: 'plan', operator: 'bogus', value: 'x', type: 'person' }], + static_cohort_only: [static_cohort], + static_cohort_and_plan: [static_cohort, plan_is_pro], + plan_and_static_cohort: [plan_is_pro, static_cohort] + }.fetch(filters) + c = local_client(flag_definition('checkout', *properties, ensure_experience_continuity: false)) + + snapshot = c.evaluate_flags('user-123', person_properties: person_properties, only_evaluate_locally: true) + + expect(snapshot.keys).not_to include('checkout') + expect(snapshot.unresolved_flags).to eq('checkout' => reason) + end + end + + it 'reports a flag blocked by an unresolved dependency as unresolved_dependency' do + c = local_client(checkout_with_continuity, flag_definition('beta-ui', depends_on('checkout'))) + + snapshot = c.evaluate_flags('user-123', flag_keys: ['beta-ui'], only_evaluate_locally: true) + + expect(snapshot.keys).not_to include('beta-ui') + expect(snapshot.unresolved_flags).to eq('beta-ui' => UnresolvedFlagReason::UNRESOLVED_DEPENDENCY) + end + + it 'reports the dependency with its own reason when it is in scope' do + c = local_client(flag_definition('checkout', plan_is_pro), flag_definition('beta-ui', depends_on('checkout'))) + + snapshot = c.evaluate_flags('user-123', only_evaluate_locally: true) + + expect(snapshot.unresolved_flags).to eq( + 'checkout' => UnresolvedFlagReason::MISSING_CONTEXT, + 'beta-ui' => UnresolvedFlagReason::UNRESOLVED_DEPENDENCY + ) + end + + it 'reports a flag that depends on a flag with no loaded definition as unsupported_definition' do + c = local_client(flag_definition('beta-ui', depends_on('checkout'))) + + snapshot = c.evaluate_flags('user-123', flag_keys: ['beta-ui'], only_evaluate_locally: true) + + expect(snapshot.keys).not_to include('beta-ui') + expect(snapshot.unresolved_flags).to eq('beta-ui' => UnresolvedFlagReason::UNSUPPORTED_DEFINITION) + end + + it 'does not report a flag the remote fallback resolved' do + c = local_client(checkout_with_continuity) + stub_flags(flags: { 'checkout' => { key: 'checkout', enabled: true, variant: nil } }) + + snapshot = c.evaluate_flags('user-123') + + expect(snapshot.get_flag('checkout')).to be(true) + expect(snapshot.unresolved_flags).to eq({}) + end + + { + 'the remote request fails' => -> { stub_request(:post, FLAGS_ENDPOINT).to_return(status: 500, body: '{}') }, + 'the remote request raises' => -> { stub_request(:post, FLAGS_ENDPOINT).to_raise(Errno::ECONNREFUSED) }, + 'the remote response returns no flags' => -> { stub_flags(flags: {}) }, + 'the remote response marks the flag as failed' => lambda { + stub_flags( + flags: { 'checkout' => { key: 'checkout', enabled: false, variant: nil, failed: true } }, + errorsWhileComputingFlags: true + ) + } + }.each do |outcome, stub_outcome| + it "leaves the flag unresolved when #{outcome}" do + c = local_client(checkout_with_continuity) + instance_exec(&stub_outcome) + + snapshot = c.evaluate_flags('user-123') + + expect(snapshot.keys).not_to include('checkout') + expect(snapshot.unresolved_flags).to eq('checkout' => UnresolvedFlagReason::EXPERIENCE_CONTINUITY) + expect(snapshot.enabled?('checkout')).to be(false) + error = flag_called_events(c)['checkout']['$feature_flag_error'] + expect(error.split(',')).to include('local_evaluation_inconclusive') + expect(error.split(',')).not_to include('flag_missing') + end + end + + it 'keeps a failed remote flag without a local definition as false' do + c = local_client(beta_ui) + stub_flags( + flags: { 'remote-only' => { key: 'remote-only', enabled: false, variant: nil, failed: true } }, + errorsWhileComputingFlags: true + ) + + snapshot = c.evaluate_flags('user-123') + + expect(snapshot.keys).to include('remote-only') + expect(snapshot.get_flag('remote-only')).to be(false) + expect(snapshot.unresolved_flags).to eq({}) + end + + it 'reports a key without a local definition as missing, not unresolved' do + c = local_client(beta_ui) + stub_flags(flags: {}) + + snapshot = c.evaluate_flags('user-123', flag_keys: %w[beta-ui typo-flag]) + snapshot.enabled?('typo-flag') + + expect(snapshot.unresolved_flags).to eq({}) + expect(flag_called_events(c)['typo-flag']['$feature_flag_error']).to eq('flag_missing') + end + + it 'does not report a flag outside the requested keys' do + c = local_client(beta_ui, checkout_with_continuity) + + snapshot = c.evaluate_flags('user-123', flag_keys: ['beta-ui']) + + expect(snapshot.keys).to eq(['beta-ui']) + expect(snapshot.get_flag('beta-ui')).to be(true) + expect(snapshot.unresolved_flags).to eq({}) + expect(WebMock).not_to have_requested(:post, FLAGS_ENDPOINT) + end + + it 'resolves an inactive flag with experience continuity to false' do + c = local_client(checkout_with_continuity.merge(active: false)) + + snapshot = c.evaluate_flags('user-123', only_evaluate_locally: true) + + expect(snapshot.keys).to eq(['checkout']) + expect(snapshot.get_flag('checkout')).to be(false) + expect(snapshot.unresolved_flags).to eq({}) + end + + it 'reports no error when reading a flag the remote fallback resolved' do + c = local_client(checkout_with_continuity) + stub_flags(flags: { 'checkout' => { key: 'checkout', enabled: true, variant: nil } }) + + snapshot = c.evaluate_flags('user-123') + + expect(snapshot.enabled?('checkout')).to be(true) + properties = flag_called_events(c)['checkout'] + expect(properties['$feature_flag_response']).to be(true) + expect(properties).not_to have_key('$feature_flag_error') + end + + it 'reports local_evaluation_inconclusive when reading an unresolved flag' do + c = local_client(checkout_with_continuity) + + snapshot = c.evaluate_flags('user-123', only_evaluate_locally: true) + + expect(snapshot.enabled?('checkout')).to be(false) + expect(flag_called_events(c)['checkout']['$feature_flag_error']).to eq('local_evaluation_inconclusive') + end + + it 'keeps response-level errors next to local_evaluation_inconclusive' do + c = local_client(checkout_with_continuity) + stub_flags(flags: {}, errorsWhileComputingFlags: true) + + c.evaluate_flags('user-123').enabled?('checkout') + + expect(flag_called_events(c)['checkout']['$feature_flag_error']) + .to eq('errors_while_computing_flags,local_evaluation_inconclusive') + end + + it 'carries no unresolved flags on filtered snapshots' do + c = local_client(beta_ui, checkout_with_continuity) + snapshot = c.evaluate_flags('user-123', only_evaluate_locally: true) + snapshot.enabled?('beta-ui') + + expect(snapshot.only_accessed.unresolved_flags).to eq({}) + filtered = snapshot.only(['beta-ui']) + expect(filtered.unresolved_flags).to eq({}) + filtered.get_flag('checkout') + expect(flag_called_events(c)['checkout']['$feature_flag_error']).to eq('flag_missing') + expect(snapshot.unresolved_flags).to eq('checkout' => UnresolvedFlagReason::EXPERIENCE_CONTINUITY) + end + + it 'does not report a flag that fails with an unexpected error' do + c = local_client(flag_definition('checkout').merge(filters: { groups: [{ properties: 'oops' }] })) + + snapshot = c.evaluate_flags('user-123', only_evaluate_locally: true) + snapshot.enabled?('checkout') + + expect(snapshot.unresolved_flags).to eq({}) + expect(flag_called_events(c)['checkout']['$feature_flag_error']).to eq('flag_missing') + end + + it 'resolves a flag locally when a filter does not match next to one local evaluation cannot check' do + free_plan = { key: 'plan', operator: 'exact', value: 'free', type: 'person' } + c = local_client(flag_definition('checkout', static_cohort, free_plan)) + + snapshot = c.evaluate_flags('user-123', person_properties: { plan: 'pro' }, flag_keys: ['checkout']) + + expect(snapshot.get_flag('checkout')).to be(false) + expect(snapshot.unresolved_flags).to eq({}) + expect(WebMock).not_to have_requested(:post, FLAGS_ENDPOINT) + end + end end end diff --git a/spec/posthog/flags_spec.rb b/spec/posthog/flags_spec.rb index 98c2ab9..fe3f672 100644 --- a/spec/posthog/flags_spec.rb +++ b/spec/posthog/flags_spec.rb @@ -2120,6 +2120,466 @@ def build_flag(early_exit:) end end + describe 'Unresolved flag reasons' do + let(:client) { Client.new(api_key: API_KEY, personal_api_key: API_KEY, test_mode: true) } + let(:poller) { client.instance_variable_get(:@feature_flags_poller) } + let(:plan_is_pro) { { key: 'plan', operator: 'exact', value: 'pro', type: 'person' } } + let(:plan_is_free) { { key: 'plan', operator: 'exact', value: 'free', type: 'person' } } + let(:email_is_set) { { key: 'email', operator: 'is_set', type: 'person' } } + let(:unknown_operator) { { key: 'plan', operator: 'bogus', value: 'x', type: 'person' } } + let(:static_cohort) { { key: 'id', value: 42, type: 'cohort' } } + + before do + stub_request(:get, 'https://us.i.posthog.com/flags/definitions?token=testsecret&send_cohorts=true') + .to_return(status: 200, body: { flags: [] }.to_json) + end + + def flag(key, *conditions, **extra) + { + key: key, active: true, + filters: { groups: conditions.map { |properties| { properties: properties, rollout_percentage: 100 } } } + }.merge(extra) + end + + def depends_on(key) + { key: key, type: 'flag', value: true, operator: 'flag_evaluates_to', dependency_chain: [key] } + end + + def evaluate(flag, properties, flags: [], cohorts: {}, **options) + poller._apply_flag_definitions(flags: [flag, *flags], cohorts: cohorts) + snapshot = poller._evaluation_snapshot + poller.match_feature_flag_properties(snapshot[:flags_by_key][flag[:key]], 'user-123', properties, {}, + snapshot[:cohorts], **options) + end + + def expect_unresolved(error_class, reason, &block) + expect(&block).to raise_error(error_class) { |error| expect(error.reason).to eq(reason) } + end + + it 'defaults the reason of both errors to unsupported_definition' do + expect(InconclusiveMatchError.new('message').reason).to eq(UnresolvedFlagReason::UNSUPPORTED_DEFINITION) + expect(RequiresServerEvaluation.new('message').reason).to eq(UnresolvedFlagReason::UNSUPPORTED_DEFINITION) + expect(InconclusiveMatchError.new('message', reason: UnresolvedFlagReason::MISSING_CONTEXT).message) + .to eq('message') + end + + [ + [nil, UnresolvedFlagReason::EXPERIENCE_CONTINUITY, UnresolvedFlagReason::EXPERIENCE_CONTINUITY], + [UnresolvedFlagReason::MISSING_CONTEXT, UnresolvedFlagReason::UNSUPPORTED_DEFINITION, + UnresolvedFlagReason::UNSUPPORTED_DEFINITION], + [UnresolvedFlagReason::MISSING_CONTEXT, UnresolvedFlagReason::UNRESOLVED_DEPENDENCY, + UnresolvedFlagReason::UNRESOLVED_DEPENDENCY], + [UnresolvedFlagReason::UNSUPPORTED_DEFINITION, UnresolvedFlagReason::UNRESOLVED_DEPENDENCY, + UnresolvedFlagReason::UNSUPPORTED_DEFINITION], + [UnresolvedFlagReason::EXPERIENCE_CONTINUITY, UnresolvedFlagReason::UNSUPPORTED_DEFINITION, + UnresolvedFlagReason::EXPERIENCE_CONTINUITY] + ].each do |current, reason, expected| + it "keeps #{expected} when #{reason.inspect} follows #{current.inspect}" do + expect(FeatureFlagsPoller.pick_unresolved_reason(current, reason)).to eq(expected) + end + end + + [ + ['a missing property', 'exact', 'x', {}, UnresolvedFlagReason::MISSING_CONTEXT], + ['an unknown operator', 'bogus', 'x', { key: 'x' }, UnresolvedFlagReason::UNSUPPORTED_DEFINITION], + ['an unknown operator and a missing property', 'bogus', 'x', {}, UnresolvedFlagReason::UNSUPPORTED_DEFINITION], + ['an invalid numeric filter', 'gt', 'abc', { key: 5 }, UnresolvedFlagReason::UNSUPPORTED_DEFINITION], + ['an invalid numeric filter and a missing property', 'gt', 'abc', {}, + UnresolvedFlagReason::UNSUPPORTED_DEFINITION], + ['an invalid date filter', 'is_date_before', 'not-a-date', { key: '2024-01-01' }, + UnresolvedFlagReason::UNSUPPORTED_DEFINITION], + ['a boolean date filter', 'is_date_after', true, { key: '2024-01-01' }, + UnresolvedFlagReason::UNSUPPORTED_DEFINITION], + ['an invalid context date', 'is_date_before', '2024-01-01', { key: 'not-a-date' }, + UnresolvedFlagReason::MISSING_CONTEXT], + ['an invalid date filter and context', 'is_date_after', 'not-a-date', { key: 'nope' }, + UnresolvedFlagReason::UNSUPPORTED_DEFINITION], + ['an invalid semver filter', 'semver_gt', 'x.y', { key: '1.2.3' }, UnresolvedFlagReason::UNSUPPORTED_DEFINITION], + ['an invalid context semver', 'semver_gt', '1.2.3', { key: 'x.y' }, UnresolvedFlagReason::MISSING_CONTEXT], + ['an invalid semver filter and context', 'semver_gt', 'x.y', { key: 'a.b' }, + UnresolvedFlagReason::UNSUPPORTED_DEFINITION], + ['an invalid semver filter and a missing property', 'semver_gt', 'x.y', {}, + UnresolvedFlagReason::UNSUPPORTED_DEFINITION], + ['an invalid wildcard filter', 'semver_wildcard', '1x.*', { key: '1.8.0' }, + UnresolvedFlagReason::UNSUPPORTED_DEFINITION], + ['an invalid wildcard filter and context', 'semver_wildcard', '1x.*', { key: 'a.b' }, + UnresolvedFlagReason::UNSUPPORTED_DEFINITION], + ['an invalid context semver for a range', 'semver_tilde', '1.2.3', { key: 'x' }, + UnresolvedFlagReason::MISSING_CONTEXT], + ['a regex the server must evaluate', 'regex', '\bfoo', { key: 'foo' }, UnresolvedFlagReason::UNSUPPORTED_DEFINITION], + ['a regex the server must evaluate and a missing property', 'regex', '\bfoo', {}, + UnresolvedFlagReason::UNSUPPORTED_DEFINITION], + ['a regex the server must evaluate for the context value', 'regex', '\w+', { key: 'é' }, + UnresolvedFlagReason::UNSUPPORTED_DEFINITION], + ['a regex with an invalid byte and a missing property', 'regex', "\\\xFF", {}, + UnresolvedFlagReason::UNSUPPORTED_DEFINITION], + ['a portable regex and a missing property', 'regex', 'foo', {}, UnresolvedFlagReason::MISSING_CONTEXT] + ].each do |cause, operator, value, property_values, reason| + it "reports #{reason} for #{cause}" do + property = { key: 'key', operator: operator, value: value } + + expect_unresolved(InconclusiveMatchError, reason) do + FeatureFlagsPoller.match_property(property, property_values) + end + end + end + + [ + ['a missing property first', :email_is_set, :plan_is_free], + ['a missing property last', :plan_is_free, :email_is_set], + ['a static cohort first', :static_cohort, :plan_is_free], + ['an unknown operator first', :unknown_operator, :plan_is_free] + ].each do |description, first, second| + it "rules a condition out when a filter does not match, with #{description}" do + checkout = flag('checkout', [send(first), send(second)]) + + expect(evaluate(checkout, { plan: 'pro' })).to be(false) + expect { evaluate(checkout, { plan: 'free' }) }.to(raise_error do |error| + expect(error).to be_a(InconclusiveMatchError).or be_a(RequiresServerEvaluation) + end) + end + end + + it 'lets a later condition resolve when an unexpected error follows a filter that cannot be checked' do + keyless_filter = { operator: 'exact', value: 'x', type: 'person' } + + expect(evaluate(flag('checkout', [email_is_set, keyless_filter], []), {})).to be(true) + expect { evaluate(flag('checkout', [keyless_filter], []), {}) }.to raise_error(NoMethodError) + end + + it 'reports the highest-precedence reason of a condition' do + expect_unresolved(InconclusiveMatchError, UnresolvedFlagReason::UNSUPPORTED_DEFINITION) do + evaluate(flag('checkout', [plan_is_pro, unknown_operator]), {}) + end + end + + it 'requires the server when a static cohort is the first filter that cannot be checked' do + expect_unresolved(RequiresServerEvaluation, UnresolvedFlagReason::UNSUPPORTED_DEFINITION) do + evaluate(flag('checkout', [static_cohort, plan_is_pro]), {}) + end + end + + it 'lets later conditions resolve when a static cohort follows a filter that cannot be checked' do + checkout = flag('checkout', [plan_is_pro, static_cohort], [email_is_set]) + + expect(evaluate(checkout, { email: 'a@b.c' })).to be(true) + expect_unresolved(InconclusiveMatchError, UnresolvedFlagReason::UNSUPPORTED_DEFINITION) do + evaluate(checkout, {}) + end + end + + it 'keeps the static cohort reason when a later condition requires the server' do + expect_unresolved(RequiresServerEvaluation, UnresolvedFlagReason::UNSUPPORTED_DEFINITION) do + evaluate(flag('checkout', [plan_is_pro], [static_cohort]), {}) + end + end + + it 'reports a dependency that requires the server as unresolved_dependency' do + base = flag('base', [static_cohort]) + + expect_unresolved(InconclusiveMatchError, UnresolvedFlagReason::UNRESOLVED_DEPENDENCY) do + evaluate(flag('checkout', [depends_on('base')]), {}, flags: [base]) + end + expect_unresolved(InconclusiveMatchError, UnresolvedFlagReason::UNSUPPORTED_DEFINITION) do + evaluate(flag('checkout', [unknown_operator], [depends_on('base')]), { plan: 'pro' }, flags: [base]) + end + end + + it 'reports a dependency with experience continuity as unresolved_dependency' do + base = flag('base', [], ensure_experience_continuity: true) + + expect_unresolved(InconclusiveMatchError, UnresolvedFlagReason::UNRESOLVED_DEPENDENCY) do + evaluate(flag('checkout', [depends_on('base')]), {}, flags: [base]) + end + end + + it 'resolves an inactive dependency with experience continuity as false' do + base = flag('base', [], active: false, ensure_experience_continuity: true) + + expect(evaluate(flag('checkout', [depends_on('base')]), {}, flags: [base])).to be(false) + end + + it 'reports a missing dependency before an inconclusive one earlier in the chain' do + dependency = depends_on('missing').merge(dependency_chain: %w[base missing]) + + expect_unresolved(InconclusiveMatchError, UnresolvedFlagReason::UNSUPPORTED_DEFINITION) do + evaluate(flag('checkout', [dependency]), {}, flags: [flag('base', [plan_is_pro])]) + end + end + + it 'reports a missing dependency the same way on a later lookup' do + evaluation_cache = {} + + 2.times do + expect_unresolved(InconclusiveMatchError, UnresolvedFlagReason::UNSUPPORTED_DEFINITION) do + poller.evaluate_flag_dependency(depends_on('missing'), evaluation_cache, 'user-123', {}, {}) + end + end + expect(evaluation_cache).to eq({}) + end + + [['a circular chain', []], ['a chain that is not a list', nil]].each do |description, dependency_chain| + it "reports #{description} as unsupported_definition" do + dependency = depends_on('base').merge(dependency_chain: dependency_chain) + + expect_unresolved(InconclusiveMatchError, UnresolvedFlagReason::UNSUPPORTED_DEFINITION) do + evaluate(flag('checkout', [dependency]), {}, flags: [flag('base', [])]) + end + end + end + + it 'reports missing group properties as missing_context' do + checkout = flag('checkout', []) + checkout[:filters][:groups][0][:aggregation_group_type_index] = 0 + poller._apply_flag_definitions(flags: [checkout], group_type_mapping: { '0' => 'company' }) + snapshot = poller._evaluation_snapshot + + expect_unresolved(InconclusiveMatchError, UnresolvedFlagReason::MISSING_CONTEXT) do + poller.match_feature_flag_properties(snapshot[:flags_by_key]['checkout'], 'user-123', {}, {}, {}, + groups: { company: 'acme' }, group_properties: {}) + end + end + + it 'reports the highest-precedence reason of a cohort property group' do + expect_unresolved(InconclusiveMatchError, UnresolvedFlagReason::UNSUPPORTED_DEFINITION) do + FeatureFlagsPoller.match_property_group({ type: 'AND', values: [plan_is_pro, unknown_operator] }, {}, {}) + end + end + + it 'resolves a nested cohort property group when another group decides it' do + inconclusive_group = { type: 'AND', values: [plan_is_pro] } + country_group = { type: 'AND', values: [{ key: 'country', operator: 'exact', value: 'US' }] } + + expect(FeatureFlagsPoller.match_property_group( + { type: 'AND', values: [inconclusive_group, country_group] }, { country: 'CA' }, {} + )).to be(false) + expect(FeatureFlagsPoller.match_property_group( + { type: 'OR', values: [inconclusive_group, country_group] }, { country: 'US' }, {} + )).to be(true) + expect_unresolved(InconclusiveMatchError, UnresolvedFlagReason::MISSING_CONTEXT) do + FeatureFlagsPoller.match_property_group({ type: 'OR', values: [inconclusive_group, country_group] }, + { country: 'CA' }, {}) + end + end + + it 'resolves an inactive flag with experience continuity as false' do + checkout = flag('checkout', [static_cohort], active: false, ensure_experience_continuity: true) + + expect(poller._compute_flag_locally(checkout, 'user-123')).to be(false) + expect_unresolved(RequiresServerEvaluation, UnresolvedFlagReason::EXPERIENCE_CONTINUITY) do + poller._compute_flag_locally(checkout.merge(active: true), 'user-123') + end + end + + it 'reports an unknown group type as unsupported_definition' do + checkout = flag('checkout', []) + checkout[:filters][:aggregation_group_type_index] = 9 + + expect_unresolved(InconclusiveMatchError, UnresolvedFlagReason::UNSUPPORTED_DEFINITION) do + poller._compute_flag_locally(checkout, 'user-123', { company: 'acme' }) + end + end + + describe '.definition_unresolved_reason' do + let(:country_is_us) { { key: 'country', operator: 'exact', value: 'US', type: 'person' } } + let(:cohorts) do + { + '1': { type: 'OR', values: [static_cohort] }, + '2': { type: 'OR', values: [{ type: 'AND', values: [country_is_us] }] }, + '3': { type: 'OR', values: [{ type: 'AND', values: [static_cohort] }] } + } + end + + { + 'a resolvable flag' => [-> { flag('checkout', [plan_is_pro]) }, nil], + 'an inactive flag' => [-> { flag('checkout', [static_cohort], active: false) }, nil], + 'experience continuity' => [ + -> { flag('checkout', [static_cohort], ensure_experience_continuity: true) }, + UnresolvedFlagReason::EXPERIENCE_CONTINUITY + ], + 'a static cohort' => [ + -> { flag('checkout', [plan_is_pro], [static_cohort]) }, UnresolvedFlagReason::UNSUPPORTED_DEFINITION + ], + 'a static cohort nested in a cohort' => [ + -> { flag('checkout', [{ key: 'id', value: 1, type: 'cohort' }]) }, + UnresolvedFlagReason::UNSUPPORTED_DEFINITION + ], + 'a dynamic cohort' => [-> { flag('checkout', [{ key: 'id', value: 2, type: 'cohort' }]) }, nil], + 'a static cohort in a nested cohort group' => [ + -> { flag('checkout', [{ key: 'id', value: 3, type: 'cohort' }]) }, + UnresolvedFlagReason::UNSUPPORTED_DEFINITION + ], + 'an unknown operator' => [-> { flag('checkout', [unknown_operator]) }, UnresolvedFlagReason::UNSUPPORTED_DEFINITION], + 'a malformed value' => [ + -> { flag('checkout', [{ key: 'n', operator: 'gt', value: 'abc', type: 'person' }]) }, + UnresolvedFlagReason::UNSUPPORTED_DEFINITION + ], + 'a regex the server must evaluate' => [ + -> { flag('checkout', [{ key: 'email', operator: 'regex', value: '\bfoo', type: 'person' }]) }, + UnresolvedFlagReason::UNSUPPORTED_DEFINITION + ], + 'a missing dependency' => [ + -> { flag('checkout', [depends_on('missing')]) }, UnresolvedFlagReason::UNSUPPORTED_DEFINITION + ], + 'a circular dependency' => [ + -> { flag('checkout', [depends_on('checkout').merge(dependency_chain: [])]) }, + UnresolvedFlagReason::UNSUPPORTED_DEFINITION + ], + 'a dependency with a loaded definition' => [-> { flag('checkout', [depends_on('checkout')]) }, nil], + 'an unknown group type' => [ + -> { flag('checkout', []).merge(filters: { aggregation_group_type_index: 9, groups: [] }) }, + UnresolvedFlagReason::UNSUPPORTED_DEFINITION + ], + 'a known group type' => [ + -> { flag('checkout', []).merge(filters: { aggregation_group_type_index: 0, groups: [] }) }, nil + ] + }.each do |description, (build_flag, expected)| + it "returns #{expected.inspect} for #{description}" do + checkout = instance_exec(&build_flag) + + expect(FeatureFlagsPoller.definition_unresolved_reason(checkout, { 'checkout' => checkout }, cohorts, + { '0': 'company' })).to eq(expected) + end + end + end + end + + # Mirrors the @unresolvable_flag_warning_capable scenarios of the evaluate-flags spec. + describe 'Unresolvable flag warning' do + let(:feature_flag_endpoint) { 'https://us.i.posthog.com/flags/definitions?token=testsecret&send_cohorts=true' } + let(:log) { StringIO.new } + let(:checkout) do + { id: 1, key: 'checkout', active: true, ensure_experience_continuity: true, + filters: { groups: [{ properties: [], rollout_percentage: 100 }] } } + end + let(:beta_ui) do + { id: 2, key: 'beta-ui', active: true, filters: { groups: [{ properties: [], rollout_percentage: 100 }] } } + end + + before do + # Same level as the default SDK logger, so these warnings show without debug logging. + PostHog::Logging.logger = Logger.new(log, level: Logger::WARN) + end + + def definitions(*flags) + { flags: flags, group_type_mapping: {}, cohorts: {} }.to_json + end + + def refresh_with(*responses) + stub_request(:get, feature_flag_endpoint).to_return(*responses) + client = Client.new(api_key: API_KEY, secret_key: API_KEY, test_mode: true) + poller = client.instance_variable_get(:@feature_flags_poller) + (responses.length - 1).times { poller.load_feature_flags(true) } + poller + end + + def warnings(key, reason) + log.string.lines.select { |line| line.include?(key) && line.include?("(reason: #{reason})") } + end + + { + 'unchanged definitions' => lambda { + refresh_with({ status: 200, body: definitions(checkout, beta_ui), headers: { 'ETag' => '"1"' } }, + { status: 200, body: definitions(checkout, beta_ui), headers: { 'ETag' => '"2"' } }) + }, + '304 not modified' => lambda { + refresh_with({ status: 200, body: definitions(checkout, beta_ui), headers: { 'ETag' => '"1"' } }, + { status: 304, body: '', headers: { 'ETag' => '"1"' } }) + }, + 'unchanged definitions with reordered keys' => lambda { + refresh_with({ status: 200, body: definitions(checkout, beta_ui) }, + { status: 200, body: definitions(checkout.to_a.reverse.to_h, beta_ui) }) + }, + 'successful shared-cache load' => lambda { + provider = double( + 'cache provider', + flag_definitions: JSON.parse(definitions(checkout, beta_ui)), + should_fetch_flag_definitions?: false, on_flag_definitions_received: nil, shutdown: nil + ) + client = Client.new(api_key: API_KEY, secret_key: API_KEY, test_mode: true, + flag_definition_cache_provider: provider) + client.instance_variable_get(:@feature_flags_poller).load_feature_flags(true) + } + }.each do |refresh_result, refresh| + it "logs one warning for a flag local evaluation never resolves after #{refresh_result}" do + instance_exec(&refresh) + + logged = warnings('checkout', UnresolvedFlagReason::EXPERIENCE_CONTINUITY) + expect(logged.length).to eq(1) + expect(logged.first).to include('WARN') + expect(logged.first).not_to include('beta-ui') + expect(WebMock).not_to have_requested(:post, 'https://us.i.posthog.com/flags/?v=2') + end + end + + it 'logs the warning again when the definition changes' do + refresh_with({ status: 200, body: definitions(checkout) }, + { status: 200, body: definitions(checkout.merge(id: 3)) }) + + expect(warnings('checkout', UnresolvedFlagReason::EXPERIENCE_CONTINUITY).length).to eq(2) + expect(WebMock).not_to have_requested(:post, 'https://us.i.posthog.com/flags/?v=2') + end + + it 'does not log the warning again when another flag changes' do + refresh_with({ status: 200, body: definitions(checkout, beta_ui) }, + { status: 200, body: definitions(checkout, beta_ui.merge(id: 3)) }) + + expect(warnings('checkout', UnresolvedFlagReason::EXPERIENCE_CONTINUITY).length).to eq(1) + end + + it 'forgets a removed flag and warns again when it returns' do + poller = refresh_with({ status: 200, body: definitions(checkout) }, + { status: 200, body: definitions(beta_ui) }, + { status: 200, body: definitions(checkout) }) + + expect(warnings('checkout', UnresolvedFlagReason::EXPERIENCE_CONTINUITY).length).to eq(2) + expect(poller.instance_variable_get(:@warned_unresolvable_flags).keys).to eq(['checkout']) + end + + { + 'a static cohort' => { key: 'id', value: 42, type: 'cohort' }, + 'an unknown operator' => { key: 'plan', operator: 'bogus', value: 'x', type: 'person' }, + 'a malformed value' => { key: 'since', operator: 'is_date_after', value: 'x', type: 'person' }, + 'a missing dependency' => { + key: 'missing', type: 'flag', value: true, operator: 'flag_evaluates_to', dependency_chain: ['missing'] + }, + 'a circular dependency' => { + key: 'checkout', type: 'flag', value: true, operator: 'flag_evaluates_to', dependency_chain: [] + } + }.each do |cause, property| + it "logs one unsupported_definition warning for #{cause}" do + unsupported = { id: 1, key: 'checkout', active: true, + filters: { groups: [{ properties: [property], rollout_percentage: 100 }] } } + + refresh_with({ status: 200, body: definitions(unsupported) }, { status: 200, body: definitions(unsupported) }) + + expect(warnings('checkout', UnresolvedFlagReason::UNSUPPORTED_DEFINITION).length).to eq(1) + end + end + + it 'logs one line per reason and none for flags that depend on the call' do + flags = [ + checkout, + checkout.merge(id: 3, key: 'inactive', active: false), + beta_ui.merge(id: 4, key: 'needs-plan', + filters: { groups: [{ properties: [{ key: 'plan', value: 'pro', type: 'person' }] }] }), + beta_ui.merge(id: 5, key: 'dependent', filters: { groups: [{ properties: [ + { key: 'checkout', type: 'flag', value: true, operator: 'flag_evaluates_to', + dependency_chain: ['checkout'] } + ] }] }), + beta_ui.merge(id: 6, key: 'static', + filters: { groups: [{ properties: [{ key: 'id', value: 42, type: 'cohort' }] }] }) + ] + + refresh_with({ status: 200, body: definitions(*flags) }) + + expect(log.string.lines.length).to eq(2) + expect(warnings('checkout', UnresolvedFlagReason::EXPERIENCE_CONTINUITY).length).to eq(1) + expect(warnings('static', UnresolvedFlagReason::UNSUPPORTED_DEFINITION).length).to eq(1) + %w[inactive needs-plan dependent].each { |key| expect(log.string).not_to include(key) } + end + end + describe FeatureFlagsPoller do it 'applies open, read, and write timeouts to feature flag requests' do poller = described_class.allocate