diff --git a/.changeset/versioned-property-matching.md b/.changeset/versioned-property-matching.md new file mode 100644 index 0000000..2757d43 --- /dev/null +++ b/.changeset/versioned-property-matching.md @@ -0,0 +1,5 @@ +--- +"posthog-ruby": patch +--- + +Honor the definitions response's `property_matching_version` during local flag evaluation, including groups, cohorts, dependencies, and external definition caches. Version 2 uses explicit equality with the service's empty-filter truthiness rule and JSON null/composite representations. Missing/1 now intentionally matches the service's legacy aggregate boolean truthiness instead of Ruby's previous unversioned explicit behavior (for example, `false` matches `"banana"` in legacy mode). Definition refreshes retain matching rules with their snapshot and version-only changes take effect on the next evaluation. Cache providers must preserve the version alongside definitions; older entries default to legacy. diff --git a/lib/posthog/client.rb b/lib/posthog/client.rb index 96ea536..a2dcb73 100644 --- a/lib/posthog/client.rb +++ b/lib/posthog/client.rb @@ -656,7 +656,8 @@ def evaluate_flags( flag_keys_set = flag_keys&.to_set(&:to_s) @feature_flags_poller.load_feature_flags - poller_flags_by_key = @feature_flags_poller.feature_flags_by_key || {} + definition_snapshot = @feature_flags_poller._evaluation_snapshot + poller_flags_by_key = definition_snapshot[:flags_by_key] || {} poller_flags_by_key.each do |key, definition| next if flag_keys_set && !flag_keys_set.include?(key.to_s) @@ -664,7 +665,7 @@ def evaluate_flags( begin match = @feature_flags_poller.send( :_compute_flag_locally, - definition, distinct_id, groups, person_properties, group_properties + definition, distinct_id, groups, person_properties, group_properties, snapshot: definition_snapshot ) rescue PostHog::RequiresServerEvaluation, PostHog::InconclusiveMatchError, StandardError next @@ -677,7 +678,7 @@ def evaluate_flags( enabled: match.is_a?(String) || (match ? true : false), variant: match.is_a?(String) ? match : nil, payload: FeatureFlagResult.parse_payload( - @feature_flags_poller.send(:_compute_flag_payload_locally, key, match) + @feature_flags_poller.send(:_compute_flag_payload_locally, key, match, snapshot: definition_snapshot) ), id: definition[:id], version: nil, @@ -696,7 +697,7 @@ def evaluate_flags( # the snapshot uses a remote /flags response, the response's top-level # `minimalFlagCalledEvents` field governs; a local-only snapshot reads # the gate polled with the flag definitions. - minimal_flag_called_events = @feature_flags_poller.minimal_flag_called_events + minimal_flag_called_events = definition_snapshot[:minimal_flag_called_events] # Skip the remote `/flags` round-trip when the caller scoped the request # to a fixed set of `flag_keys` and we've already resolved every one of diff --git a/lib/posthog/feature_flags.rb b/lib/posthog/feature_flags.rb index bae1698..4b86d86 100644 --- a/lib/posthog/feature_flags.rb +++ b/lib/posthog/feature_flags.rb @@ -79,6 +79,11 @@ 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_snapshot = Concurrent::AtomicReference.new({ + flags: @feature_flags, flags_by_key: @feature_flags_by_key, + group_type_mapping: @group_type_mapping, cohorts: @cohorts, + minimal_flag_called_events: @minimal_flag_called_events, property_matching_version: 1 + }.freeze) @flag_definition_cache_provider = flag_definition_cache_provider FlagDefinitionCacheProvider.validate!(@flag_definition_cache_provider) if @flag_definition_cache_provider @@ -214,12 +219,14 @@ def get_feature_flag( groups = {}, person_properties = {}, group_properties = {}, - only_evaluate_locally = false + only_evaluate_locally = false, + snapshot: nil ) key = key.to_s # make sure they're loaded on first run load_feature_flags + snapshot ||= _evaluation_snapshot symbolize_keys! groups symbolize_keys! person_properties @@ -231,12 +238,13 @@ def get_feature_flag( response = nil payload = nil - feature_flag = @feature_flags_by_key&.[](key) + feature_flag = snapshot[:flags_by_key]&.[](key) unless feature_flag.nil? begin - response = _compute_flag_locally(feature_flag, distinct_id, groups, person_properties, group_properties) - payload = _compute_flag_payload_locally(key, response) unless response.nil? + response = _compute_flag_locally(feature_flag, distinct_id, groups, person_properties, group_properties, + snapshot: snapshot) + payload = _compute_flag_payload_locally(key, response, snapshot: snapshot) unless response.nil? logger.debug "Successfully computed flag locally: #{key} -> #{response}" rescue RequiresServerEvaluation, InconclusiveMatchError => e logger.debug "Failed to compute flag #{key} locally: #{e}" @@ -255,7 +263,7 @@ def get_feature_flag( # Locally-evaluated flags read it from the definitions payload; remotely # evaluated flags read it from the /flags response. nil when the signal # is unavailable, which fails safe to the full event. - minimal_flag_called_events = @minimal_flag_called_events if flag_was_locally_evaluated + minimal_flag_called_events = snapshot[:minimal_flag_called_events] if flag_was_locally_evaluated request_id = nil evaluated_at = nil @@ -342,17 +350,19 @@ def get_all_flags_and_payloads( raise_on_error = false ) load_feature_flags + snapshot = _evaluation_snapshot flags = {} payloads = {} - fallback_to_server = @feature_flags.empty? + fallback_to_server = snapshot[:flags].empty? request_id = nil # Only for /flags requests evaluated_at = nil # Only for /flags requests - @feature_flags.each do |flag| - match_value = _compute_flag_locally(flag, distinct_id, groups, person_properties, group_properties) + snapshot[:flags].each do |flag| + match_value = _compute_flag_locally(flag, distinct_id, groups, person_properties, group_properties, + snapshot: snapshot) flags[flag[:key]] = match_value - match_payload = _compute_flag_payload_locally(flag[:key], match_value) + match_payload = _compute_flag_payload_locally(flag[:key], match_value, snapshot: snapshot) payloads[flag[:key]] = match_payload if match_payload rescue RequiresServerEvaluation, InconclusiveMatchError fallback_to_server = true @@ -432,6 +442,8 @@ def get_feature_flag_payload( only_evaluate_locally = false ) key = key.to_s + load_feature_flags + snapshot = _evaluation_snapshot if match_value.nil? match_value = get_feature_flag( @@ -440,11 +452,12 @@ def get_feature_flag_payload( groups, person_properties, group_properties, - true + true, + snapshot: snapshot )[0] end response = nil - response = _compute_flag_payload_locally(key, match_value) unless match_value.nil? + response = _compute_flag_payload_locally(key, match_value, snapshot: snapshot) unless match_value.nil? if response.nil? && !only_evaluate_locally flags_payloads = get_feature_payloads(distinct_id, groups, person_properties, group_properties) response = flags_payloads[key.downcase] || nil @@ -615,14 +628,69 @@ def self.semver_wildcard_bounds(value) end end - def self.match_property(property, property_values, cohort_properties = {}) + # Service legacy classifies the entire filter, not individual array members. + def self.boolean_like?(value) + case value + when true, false then true + when String then %w[true false].include?(value.downcase) + when Array then value.all? { |member| boolean_like?(member) } + else false + end + end + + def self.legacy_truthy?(value) + case value + when true then true + when String then value.downcase == 'true' + when Array then value.all? { |member| legacy_truthy?(member) } + else false + end + end + + def self.sorted_composite(value) + case value + when Hash + value.transform_keys(&:to_s).sort.to_h.transform_values { |member| sorted_composite(member) } + when Array + value.map { |member| sorted_composite(member) } + else + value + end + end + + def self.property_string(value) + # Keep numeric normalization unchanged; null and composites need JSON rather + # than Ruby's nil.to_s / Array#to_s representations. + case value + when nil, Array, Hash then JSON.generate(sorted_composite(value)) + else value.to_s + end.downcase + end + + def self.exact_property_match?(filter, value, property_matching_version) + return legacy_truthy?(filter) == legacy_truthy?(value) if property_matching_version != 2 && boolean_like?(filter) + return legacy_truthy?(value) if filter.is_a?(Array) && filter.empty? + + if filter.is_a?(Array) + filter.any? { |member| property_string(member) == property_string(value) } + else + property_string(filter) == property_string(value) + end + end + + private_class_method :boolean_like?, :legacy_truthy?, :sorted_composite, :property_string, :exact_property_match? + + def self.match_property(property, property_values, cohort_properties = {}, property_matching_version: 1) # only looks for matches where key exists in property_values PostHog::Utils.symbolize_keys! property PostHog::Utils.symbolize_keys! property_values # Handle cohort properties - return match_cohort(property, property_values, cohort_properties) if extract_value(property, :type) == 'cohort' + if extract_value(property, :type) == 'cohort' + return match_cohort(property, property_values, cohort_properties, + property_matching_version: property_matching_version) + end key = property[:key].to_sym value = property[:value] @@ -638,18 +706,8 @@ def self.match_property(property, property_values, cohort_properties = {}) case operator when 'exact', 'is_not' - if value.is_a?(Array) - values_stringified = value.map { |val| val.to_s.downcase } - return values_stringified.any?(override_value.to_s.downcase) if operator == 'exact' - - return values_stringified.none?(override_value.to_s.downcase) - - end - if operator == 'exact' - value.to_s.downcase == override_value.to_s.downcase - else - value.to_s.downcase != override_value.to_s.downcase - end + matches = exact_property_match?(value, override_value, property_matching_version) + operator == 'exact' ? matches : !matches when 'is_set' property_values.key?(key) when 'icontains' @@ -731,7 +789,7 @@ def self.match_property(property, property_values, cohort_properties = {}) end end - def self.match_cohort(property, property_values, cohort_properties) + def self.match_cohort(property, property_values, cohort_properties, property_matching_version: 1) # Cohort properties are in the form of property groups like this: # { # "cohort_id" => { @@ -749,10 +807,11 @@ def self.match_cohort(property, property_values, cohort_properties) "cohort #{cohort_id} not found in local cohorts - likely a static cohort that requires server evaluation" end - match_property_group(property_group, property_values, cohort_properties) + match_property_group(property_group, property_values, cohort_properties, + property_matching_version: property_matching_version) end - def self.match_property_group(property_group, property_values, cohort_properties) + def self.match_property_group(property_group, property_values, cohort_properties, property_matching_version: 1) return true if property_group.nil? || property_group.empty? group_type = extract_value(property_group, :type) @@ -761,9 +820,11 @@ 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) + match_nested_property_group(properties, group_type, property_values, cohort_properties, + property_matching_version: property_matching_version) else - match_regular_property_group(properties, group_type, property_values, cohort_properties) + match_regular_property_group(properties, group_type, property_values, cohort_properties, + property_matching_version: property_matching_version) end end @@ -788,16 +849,19 @@ 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) + 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) + 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) + return true if match_property_group(property, property_values, cohort_properties, + property_matching_version: property_matching_version) end false else @@ -805,7 +869,8 @@ def self.match_nested_property_group(properties, group_type, property_values, co end end - def self.match_regular_property_group(properties, group_type, property_values, cohort_properties) + 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) @@ -814,7 +879,8 @@ def self.match_regular_property_group(properties, group_type, property_values, c properties.each do |prop| PostHog::Utils.symbolize_keys!(prop) - matches = match_property(prop, property_values, cohort_properties) + matches = match_property(prop, property_values, cohort_properties, + property_matching_version: property_matching_version) negated = prop[:negation] || false final_result = negated ? !matches : matches @@ -847,13 +913,14 @@ def self.match_regular_property_group(properties, group_type, property_values, c # @param properties [Hash] Person properties for evaluation # @param cohort_properties [Hash] Cohort properties for evaluation # @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) + 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 @feature_flags_by_key.nil? || evaluation_cache.nil? + 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'}' " \ @@ -881,7 +948,7 @@ def evaluate_flag_dependency(property, evaluation_cache, distinct_id, properties dependency_chain.each do |dep_flag_key| unless evaluation_cache.key?(dep_flag_key) # Need to evaluate this dependency first - dep_flag = @feature_flags_by_key[dep_flag_key] + 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 @@ -898,7 +965,8 @@ def evaluate_flag_dependency(property, evaluation_cache, distinct_id, properties distinct_id, properties, evaluation_cache, - cohort_properties + cohort_properties, + snapshot: snapshot ) evaluation_cache[dep_flag_key] = dep_result rescue InconclusiveMatchError => e @@ -964,7 +1032,14 @@ def self.matches_dependency_value(expected_value, actual_value) private_class_method :extract_value, :find_cohort_property, :nested_property_group?, :match_nested_property_group, :match_regular_property_group - def _compute_flag_locally(flag, distinct_id, groups = {}, person_properties = {}, group_properties = {}) + # Publish/capture all matching state together so a poll cannot switch semantics + # (or dependency/cohort definitions) halfway through one local evaluation. + def _evaluation_snapshot + @definition_snapshot.value + end + + 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] return false unless flag[:active] @@ -981,11 +1056,12 @@ def _compute_flag_locally(flag, distinct_id, groups = {}, person_properties = {} local_person_properties = local_person_properties.merge(distinct_id: distinct_id) end - return match_feature_flag_properties(flag, distinct_id, local_person_properties, evaluation_cache, @cohorts, - groups: groups, group_properties: group_properties) + return match_feature_flag_properties(flag, distinct_id, local_person_properties, evaluation_cache, + snapshot[:cohorts], groups: groups, group_properties: group_properties, + snapshot: snapshot) end - group_name = @group_type_mapping[aggregation_group_type_index.to_s.to_sym] + group_name = snapshot[:group_type_mapping][aggregation_group_type_index.to_s.to_sym] if group_name.nil? logger.warn( @@ -1006,24 +1082,25 @@ def _compute_flag_locally(flag, distinct_id, groups = {}, person_properties = {} focused_group_properties = group_properties[group_name_symbol] match_feature_flag_properties(flag, groups[group_name_symbol], focused_group_properties, evaluation_cache, - @cohorts, groups: groups, group_properties: group_properties) + snapshot[:cohorts], groups: groups, group_properties: group_properties, + snapshot: snapshot) end - def _compute_flag_payload_locally(key, match_value) - return nil if @feature_flags_by_key.nil? + def _compute_flag_payload_locally(key, match_value, snapshot: _evaluation_snapshot) + return nil if snapshot[:flags_by_key].nil? key = key.to_s response = nil if [true, false].include? match_value - response = @feature_flags_by_key.dig(key, :filters, :payloads, match_value.to_s.to_sym) + response = snapshot[:flags_by_key].dig(key, :filters, :payloads, match_value.to_s.to_sym) elsif match_value.is_a? String - response = @feature_flags_by_key.dig(key, :filters, :payloads, match_value.to_sym) + response = snapshot[:flags_by_key].dig(key, :filters, :payloads, match_value.to_sym) end response end def match_feature_flag_properties(flag, distinct_id, properties, evaluation_cache, cohort_properties = {}, - groups: {}, group_properties: {}) + groups: {}, group_properties: {}, snapshot: _evaluation_snapshot) flag_filters = flag[:filters] || {} flag_conditions = flag_filters[:groups] || [] @@ -1048,7 +1125,7 @@ def match_feature_flag_properties(flag, distinct_id, properties, evaluation_cach if condition_aggregation.nil? # Person condition under a mixed flag — caller already passed person props/bucketing. else - group_name = @group_type_mapping[condition_aggregation.to_s.to_sym] + group_name = snapshot[:group_type_mapping][condition_aggregation.to_s.to_sym] if group_name.nil? || !groups.key?(group_name.to_sym) logger.debug do "[FEATURE FLAGS] Skipping group condition for flag '#{flag[:key]}': " \ @@ -1066,7 +1143,7 @@ def match_feature_flag_properties(flag, distinct_id, properties, evaluation_cach end case condition_match_outcome(flag, effective_bucketing, condition, effective_properties, evaluation_cache, - cohort_properties) + cohort_properties, snapshot: snapshot) when :match variant_override = condition[:variant] flag_multivariate = flag_filters[:multivariate] || {} @@ -1100,9 +1177,10 @@ def match_feature_flag_properties(flag, distinct_id, properties, evaluation_cach false end - def condition_match(flag, distinct_id, condition, properties, evaluation_cache, cohort_properties = {}) + def condition_match(flag, distinct_id, condition, properties, evaluation_cache, cohort_properties = {}, + snapshot: _evaluation_snapshot) condition_match_outcome(flag, distinct_id, condition, properties, evaluation_cache, - cohort_properties) == :match + cohort_properties, snapshot: snapshot) == :match end # Evaluates a single condition group and returns a tri-state outcome: @@ -1112,15 +1190,18 @@ def condition_match(flag, distinct_id, condition, properties, evaluation_cache, # rollout percentage excluded the user # Distinguishing :no_match from :out_of_rollout_bound lets the caller implement the # early_exit behavior (mirrors the server-side Rust evaluation engine). - def condition_match_outcome(flag, distinct_id, condition, properties, evaluation_cache, cohort_properties = {}) + def condition_match_outcome(flag, distinct_id, condition, properties, evaluation_cache, cohort_properties = {}, + snapshot: _evaluation_snapshot) 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) + evaluate_flag_dependency(prop, evaluation_cache, distinct_id, properties, cohort_properties, + snapshot: snapshot) else - FeatureFlagsPoller.match_property(prop, properties, cohort_properties) + FeatureFlagsPoller.match_property(prop, properties, cohort_properties, + property_matching_version: snapshot[:property_matching_version]) end end return :no_match @@ -1221,12 +1302,8 @@ def _fetch_and_apply_flag_definitions '[FEATURE FLAGS] Feature flags quota limit exceeded - unsetting all local flags. ' \ 'Learn more about billing limits at https://posthog.com/docs/billing/limits-alerts' ) - @feature_flags = Concurrent::Array.new - @feature_flags_by_key = {} - @group_type_mapping = Concurrent::Hash.new - @cohorts = Concurrent::Hash.new + _apply_flag_definitions({}) @flag_definitions_loaded_at.value = nil - @minimal_flag_called_events = false @loaded_flags_successfully_once.make_false @quota_limited.make_true return @@ -1247,11 +1324,13 @@ def _store_in_cache_provider return unless @flag_definition_cache_provider begin + snapshot = _evaluation_snapshot data = { - flags: @feature_flags.to_a, - group_type_mapping: @group_type_mapping.to_h, - cohorts: @cohorts.to_h, - minimal_flag_called_events: @minimal_flag_called_events + flags: snapshot[:flags].to_a, + group_type_mapping: snapshot[:group_type_mapping].to_h, + cohorts: snapshot[:cohorts].to_h, + minimal_flag_called_events: snapshot[:minimal_flag_called_events], + property_matching_version: snapshot[:property_matching_version] } @flag_definition_cache_provider.on_flag_definitions_received(data) rescue StandardError => e @@ -1265,16 +1344,25 @@ def _apply_flag_definitions(data) cohorts = get_by_symbol_or_string_key(data, 'cohorts') || {} minimal_flag_called_events = get_by_symbol_or_string_key(data, 'minimal_flag_called_events') - @feature_flags = Concurrent::Array.new(flags.map { |f| deep_symbolize_keys(f) }) + property_matching_version = get_by_symbol_or_string_key(data, 'property_matching_version') || 1 + new_flags = Concurrent::Array.new(flags.map { |f| deep_symbolize_keys(f) }) new_by_key = {} - @feature_flags.each do |flag| + new_flags.each do |flag| new_by_key[flag[:key]] = flag unless flag[:key].nil? end + new_group_type_mapping = Concurrent::Hash[deep_symbolize_keys(group_type_mapping)] + new_cohorts = Concurrent::Hash[deep_symbolize_keys(cohorts)] + @definition_snapshot.value = { + flags: new_flags, flags_by_key: new_by_key, + group_type_mapping: new_group_type_mapping, cohorts: new_cohorts, + minimal_flag_called_events: minimal_flag_called_events == true, + property_matching_version: property_matching_version + }.freeze + @feature_flags = new_flags @feature_flags_by_key = new_by_key - - @group_type_mapping = Concurrent::Hash[deep_symbolize_keys(group_type_mapping)] - @cohorts = Concurrent::Hash[deep_symbolize_keys(cohorts)] + @group_type_mapping = new_group_type_mapping + @cohorts = new_cohorts @minimal_flag_called_events = minimal_flag_called_events == true logger.debug "Loaded #{@feature_flags.length} feature flags and #{@cohorts.length} cohorts" diff --git a/lib/posthog/flag_definition_cache.rb b/lib/posthog/flag_definition_cache.rb index dfdd394..9e127d1 100644 --- a/lib/posthog/flag_definition_cache.rb +++ b/lib/posthog/flag_definition_cache.rb @@ -14,11 +14,14 @@ module PostHog # # @!method flag_definitions # Retrieve cached flag definitions. Return a Hash with +:flags+, - # +:group_type_mapping+, +:cohorts+, and +:minimal_flag_called_events+ + # +:group_type_mapping+, +:cohorts+, +:minimal_flag_called_events+, and +:property_matching_version+ # keys, or +nil+ if the cache is empty. Returning +nil+ triggers an API # fetch when no flags are loaded yet (emergency fallback). Providers # written before +:minimal_flag_called_events+ existed continue to work; - # a missing key is treated as +false+. + # a missing key is treated as +false+. Preserve +:property_matching_version+ + # with the definitions: exactly +2+ selects explicit equality; missing/1 + # (including older cache entries) selects service legacy boolean matching. + # A fresh entry without the version resets to legacy, even after version 2. # @return [Hash, nil] # # @!method should_fetch_flag_definitions? @@ -29,9 +32,10 @@ module PostHog # # @!method on_flag_definitions_received(data) # Called after successfully fetching new definitions from the API. - # +data+ is a Hash with +:flags+, +:group_type_mapping+, +:cohorts+, and - # +:minimal_flag_called_events+ keys (plain Ruby types, not Concurrent:: - # wrappers). Store it in your external cache. + # +data+ is a Hash with +:flags+, +:group_type_mapping+, +:cohorts+, + # +:minimal_flag_called_events+, and +:property_matching_version+ keys + # (plain Ruby types, not Concurrent:: wrappers). Store the entire snapshot + # together in your external cache, including version-only updates. # @param data [Hash] # @return [void] # diff --git a/spec/posthog/feature_flags_property_matching_version_spec.rb b/spec/posthog/feature_flags_property_matching_version_spec.rb new file mode 100644 index 0000000..78c627f --- /dev/null +++ b/spec/posthog/feature_flags_property_matching_version_spec.rb @@ -0,0 +1,243 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe PostHog::FeatureFlagsPoller, 'property matching versions' do + rows = [ + [false, 'banana', true, false], + [false, 0, true, false], + [%w[true false], 'true', false, true], + [%w[true false], 'pro', true, false], + [[], true, true, true], + [[], [], true, true], + [true, [true], true, false], + [false, 'FALSE', true, true], + [false, nil, true, false], + [false, {}, true, false], + [[], [true, 'TRUE', []], true, true], + [[], [true, 0], false, false], + [[], false, false, false], + [[], 0, false, false], + [[], 'banana', false, false], + [false, '', true, false], + ['null', nil, true, true], + [nil, nil, true, true], + ['', nil, false, false], + [[nil, 'PRO'], 'null', true, true], + [[true, 'PRO'], 'TRUE', true, true], + [[1, 'PRO'], '1', true, true], + [[[true, true], 'PRO'], [true, true], true, true], + ['[true,true]', [true, true], true, true], + ['{"a":null,"b":[true,{"c":false}]}', { b: [true, { c: false }], a: nil }, true, true], + ['ÄBC', 'äbc', true, true] + ] + + [nil, 0, 1, 2, 3, '2'].each do |version| + rows.each do |filter, value, legacy, explicit| + %w[exact is_not].each do |operator| + it "matches #{filter.inspect} against #{value.inspect} with #{operator}, version #{version.inspect}" do + expected = version == 2 ? explicit : legacy + expected = !expected if operator == 'is_not' + expect(described_class.match_property( + { key: 'value', value: filter, operator: operator }, { value: value }, {}, + property_matching_version: version + )).to eq(expected) + end + end + end + + it "keeps missing properties inconclusive for version #{version.inspect}" do + %w[exact is_not].each do |operator| + expect do + described_class.match_property({ key: 'value', value: false, operator: operator }, {}, {}, + property_matching_version: version) + end.to raise_error(PostHog::InconclusiveMatchError) + end + end + end + + it 'defaults public matching helpers to service legacy semantics' do + expect(described_class.match_property({ key: 'value', value: false }, { value: 'banana' })).to be(true) + end + + let(:url) { 'https://us.i.posthog.com/flags/definitions?token=testsecret&send_cohorts=true' } + let(:leaf) { { key: 'value', value: false, operator: 'exact', type: 'person' } } + let(:definitions) do + { + flags: [ + flag('person', [leaf]), + flag('group', [leaf], aggregation_group_type_index: 0), + flag('mixed', [leaf]).tap { |f| f[:filters][:groups][0][:aggregation_group_type_index] = 0 }, + flag('cohort', [{ key: 'id', value: 1, type: 'cohort' }]), + flag('dependency', [{ key: 'person', value: true, operator: 'flag_evaluates_to', type: 'flag', + dependency_chain: ['person'] }]) + ], + group_type_mapping: { '0' => 'company' }, + cohorts: { '1' => { type: 'AND', values: [{ type: 'OR', values: [leaf] }] } } + } + end + + def flag(key, properties, **filters) + { key: key, active: true, version: 2, filters: { groups: [{ properties: properties }] }.merge(filters) } + end + + def local_flags(poller) + poller.get_all_flags_and_payloads('person-id', { company: 'company-id' }, { value: 'banana' }, + { company: { value: 'banana' } }, true)[:featureFlags] + end + + def expect_local_flags(poller, expected) + expect(local_flags(poller)).to eq(definitions[:flags].to_h { |f| [f[:key], expected] }) + definitions[:flags].each do |f| + result = poller.get_feature_flag(f[:key], 'person-id', { company: 'company-id' }, { value: 'banana' }, + { company: { value: 'banana' } }, true) + expect(result[0..1]).to eq([expected, true]) + end + end + + let(:poller) { described_class.new(60, nil, API_KEY, 'https://us.i.posthog.com', 3) } + + after { poller.shutdown_poller } + + it 'propagates version-only reloads to person, group, mixed, recursive cohort and dependency evaluations' do + %w[exact is_not].each do |operator| + leaf[:operator] = operator + [1, 2, 1, 2, nil, 3].each do |version| + data = definitions.merge(property_matching_version: version) + data.delete(:property_matching_version) if version.nil? + poller._apply_flag_definitions(data) + expect_local_flags(poller, operator == 'exact' ? version != 2 : version == 2) + end + end + expect(WebMock).not_to have_requested(:post, %r{/flags/}) + end + + it 'uses one snapshot in the full-result API and observes version-only reloads on the next call' do + client = PostHog::Client.new(api_key: API_KEY, test_mode: true) + client_poller = client.instance_variable_get(:@feature_flags_poller) + options = { groups: { company: 'company-id' }, person_properties: { value: 'banana' }, + group_properties: { company: { value: 'banana' } }, only_evaluate_locally: true } + client_poller._apply_flag_definitions(definitions.merge(property_matching_version: 1)) + refreshed = false + allow(client_poller).to receive(:_compute_flag_locally).and_wrap_original do |original, *args, **kwargs| + unless refreshed + refreshed = true + client_poller._apply_flag_definitions(definitions.merge(property_matching_version: 2)) + end + original.call(*args, **kwargs) + end + first = client.evaluate_flags('person-id', **options) + second = client.evaluate_flags('person-id', **options) + client_poller._apply_flag_definitions(definitions.merge(property_matching_version: 1)) + third = client.evaluate_flags('person-id', **options) + definitions[:flags].each do |f| + expect([first.get_flag(f[:key]), second.get_flag(f[:key]), third.get_flag(f[:key])]).to eq([true, false, true]) + end + expect(WebMock).not_to have_requested(:post, %r{/flags/}) + ensure + client&.shutdown + end + + it 'preserves the evaluation snapshot when definitions refresh during a dependency evaluation' do + poller._apply_flag_definitions(definitions.merge(property_matching_version: 1)) + allow(poller).to receive(:evaluate_flag_dependency).and_wrap_original do |original, *args, **kwargs| + poller._apply_flag_definitions(definitions.merge(property_matching_version: 2)) + original.call(*args, **kwargs) + end + result = poller.get_feature_flag('dependency', 'person-id', {}, { value: 'banana' }, {}, true) + expect(result[0..1]).to eq([true, true]) + expect(local_flags(poller).values).to all(be(false)) + end + + it 'retains the matching snapshot for payload lookup during a refresh' do + data = definitions.merge(property_matching_version: 1) + data[:flags][0][:filters][:payloads] = { 'true' => 'old-payload' } + poller._apply_flag_definitions(data) + allow(poller).to receive(:_compute_flag_payload_locally).and_wrap_original do |original, *args, **kwargs| + data[:flags][0][:filters][:payloads] = { 'true' => 'new-payload' } + poller._apply_flag_definitions(data.merge(property_matching_version: 2)) + original.call(*args, **kwargs) + end + expect(poller.get_feature_flag_payload('person', 'person-id', nil, {}, { value: 'banana' }, {}, true)) + .to eq('old-payload') + end + + it 'captures the empty initial snapshot when the first load publishes immediately after the atomic read' do + reference = poller.instance_variable_get(:@definition_snapshot) + published = false + allow(reference).to receive(:value).and_wrap_original do |original| + snapshot = original.call + unless published + published = true + # Deterministically schedule first-load publication between the read and its caller resuming. + poller._apply_flag_definitions(definitions.merge(property_matching_version: 2)) + end + snapshot + end + + expect(local_flags(poller)).to eq({}) + expect_local_flags(poller, false) + expect(WebMock).not_to have_requested(:post, %r{/flags/}) + end + + it 'loads matching versions on the asynchronous poller and observes an omitted version on reload' do + stub_request(:get, url).to_return(status: 200, body: definitions.merge(property_matching_version: 2).to_json) + async_poller = described_class.new(60, API_KEY, API_KEY, 'https://us.i.posthog.com', 3, async_load: true) + eventually { expect(async_poller.definitions_loaded?).to be(true) } + expect_local_flags(async_poller, false) + stub_request(:get, url).to_return(status: 200, body: definitions.to_json) + async_poller.load_feature_flags(true) + expect_local_flags(async_poller, true) + ensure + async_poller&.shutdown_poller + end + + it 'keeps version and definitions across 304 and failures, resetting an omitted version on fresh responses' do + stub_request(:get, url).to_return( + { status: 200, body: definitions.merge(property_matching_version: 2).to_json, headers: { 'ETag' => 'v2' } }, + { status: 304 }, + { status: 500, body: '{}' }, + { status: 200, body: 'invalid json' }, + { status: 200, body: definitions.to_json } + ) + poller._load_feature_flags + expect_local_flags(poller, false) + poller._load_feature_flags + expect_local_flags(poller, false) + poller._load_feature_flags + expect_local_flags(poller, false) + poller._load_feature_flags + expect_local_flags(poller, false) + poller._load_feature_flags + expect_local_flags(poller, true) + end + + it 'round trips version through JSON external caches and defaults older entries to legacy' do + provider = double('cache', should_fetch_flag_definitions?: true, shutdown: nil) + allow(provider).to receive(:flag_definitions) + stored = nil + allow(provider).to receive(:on_flag_definitions_received) { |data| stored = JSON.parse(JSON.generate(data)) } + poller.instance_variable_set(:@flag_definition_cache_provider, provider) + cached_poller = described_class.new(60, nil, API_KEY, 'https://us.i.posthog.com', 3) + cached_poller.instance_variable_set(:@flag_definition_cache_provider, provider) + allow(provider).to receive(:flag_definitions) { stored } + + [1, 2, 1, 2].each do |version| + allow(provider).to receive(:should_fetch_flag_definitions?).and_return(true) + stub_request(:get, url).to_return( + status: 200, body: definitions.merge(property_matching_version: version).to_json + ) + poller._load_feature_flags + expect(stored['property_matching_version']).to eq(version) + allow(provider).to receive(:should_fetch_flag_definitions?).and_return(false) + cached_poller._load_feature_flags + expect_local_flags(cached_poller, version != 2) + end + + stored.delete('property_matching_version') + cached_poller._load_feature_flags + expect_local_flags(cached_poller, true) + ensure + cached_poller&.shutdown_poller + end +end diff --git a/spec/posthog/flags_spec.rb b/spec/posthog/flags_spec.rb index b3fa4e2..85a2ab0 100644 --- a/spec/posthog/flags_spec.rb +++ b/spec/posthog/flags_spec.rb @@ -1683,11 +1683,7 @@ module PostHog end def stub_feature_flags(flags) - poller.instance_variable_set(:@feature_flags, flags) - flags_by_key = {} - flags.each { |flag| flags_by_key[flag[:key]] = flag } - poller.instance_variable_set(:@feature_flags_by_key, flags_by_key) - poller.instance_variable_get(:@loaded_flags_successfully_once).make_true + poller._apply_flag_definitions(flags: flags) end end