diff --git a/.changeset/quiet-wolves-fix.md b/.changeset/quiet-wolves-fix.md new file mode 100644 index 0000000..8c11811 --- /dev/null +++ b/.changeset/quiet-wolves-fix.md @@ -0,0 +1,6 @@ +--- +"posthog-ruby": patch +"posthog-rails": patch +--- + +Stop duplicating `distinct_id` inside `/flags` person properties. diff --git a/.github/workflows/sdk-compliance.yml b/.github/workflows/sdk-compliance.yml index df88af8..1dfcb90 100644 --- a/.github/workflows/sdk-compliance.yml +++ b/.github/workflows/sdk-compliance.yml @@ -14,8 +14,8 @@ on: jobs: compliance: name: PostHog SDK compliance tests - uses: PostHog/posthog-sdk-test-harness/.github/workflows/test-sdk-action.yml@68498bcf3ae95ac941476e6ca3d42d6086e1c7fd + uses: PostHog/posthog-sdk-test-harness/.github/workflows/test-sdk-action.yml@02c049e529001d02f37a534745678e057d371fb0 with: adapter-dockerfile: "sdk_compliance_adapter/Dockerfile" adapter-context: "." - test-harness-version: "0.9.0" + test-harness-version: "0.10.0" diff --git a/lib/posthog/client.rb b/lib/posthog/client.rb index ab3ef87..8f49169 100644 --- a/lib/posthog/client.rb +++ b/lib/posthog/client.rb @@ -558,7 +558,7 @@ def evaluate_flags( return FeatureFlagEvaluations.new(host: host, distinct_id: distinct_id, flags: {}, groups: groups) if @disabled person_properties, group_properties = add_local_person_and_group_properties( - distinct_id, groups, person_properties, group_properties + groups, person_properties, group_properties ) records = {} @@ -672,8 +672,8 @@ def get_all_flags( ) return {} if @disabled - person_properties, group_properties = add_local_person_and_group_properties(distinct_id, groups, - person_properties, group_properties) + person_properties, group_properties = add_local_person_and_group_properties(groups, person_properties, + group_properties) @feature_flags_poller.get_all_flags(distinct_id, groups, person_properties, group_properties, only_evaluate_locally) end @@ -712,8 +712,8 @@ def get_feature_flag_payload( return nil if @disabled key = key.to_s - person_properties, group_properties = add_local_person_and_group_properties(distinct_id, groups, - person_properties, group_properties) + person_properties, group_properties = add_local_person_and_group_properties(groups, person_properties, + group_properties) @feature_flags_poller.get_feature_flag_payload(key, distinct_id, match_value, groups, person_properties, group_properties, only_evaluate_locally) end @@ -740,7 +740,7 @@ def get_all_flags_and_payloads( return { featureFlags: {}, featureFlagPayloads: {} } if @disabled person_properties, group_properties = add_local_person_and_group_properties( - distinct_id, groups, person_properties, group_properties + groups, person_properties, group_properties ) response = @feature_flags_poller.get_all_flags_and_payloads( distinct_id, groups, person_properties, group_properties, only_evaluate_locally @@ -902,7 +902,7 @@ def _get_feature_flag_result( key = key.to_s person_properties, group_properties = add_local_person_and_group_properties( - distinct_id, groups, person_properties, group_properties + groups, person_properties, group_properties ) feature_flag_response, flag_was_locally_evaluated, request_id, evaluated_at, feature_flag_error, payload = @feature_flags_poller.get_feature_flag( @@ -1048,7 +1048,7 @@ def shutdown? @shutdown_mutex.synchronize { @shutdown } end - def add_local_person_and_group_properties(distinct_id, groups, person_properties, group_properties) + def add_local_person_and_group_properties(groups, person_properties, group_properties) groups ||= {} person_properties ||= {} group_properties ||= {} @@ -1061,8 +1061,6 @@ def add_local_person_and_group_properties(distinct_id, groups, person_properties symbolize_keys! value end - all_person_properties = { distinct_id: distinct_id }.merge(person_properties) - all_group_properties = {} groups&.each do |group_name, group_key| all_group_properties[group_name] = { @@ -1070,7 +1068,7 @@ def add_local_person_and_group_properties(distinct_id, groups, person_properties }.merge((group_properties && group_properties[group_name]) || {}) end - [all_person_properties, all_group_properties] + [person_properties, all_group_properties] end end end diff --git a/lib/posthog/feature_flags.rb b/lib/posthog/feature_flags.rb index 9e82229..e42537d 100644 --- a/lib/posthog/feature_flags.rb +++ b/lib/posthog/feature_flags.rb @@ -921,7 +921,12 @@ def _compute_flag_locally(flag, distinct_id, groups = {}, person_properties = {} aggregation_group_type_index = flag_filters[:aggregation_group_type_index] if aggregation_group_type_index.nil? - return match_feature_flag_properties(flag, distinct_id, person_properties, evaluation_cache, @cohorts, + local_person_properties = person_properties || {} + unless local_person_properties.key?(:distinct_id) || local_person_properties.key?('distinct_id') + 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) end diff --git a/sdk_compliance_adapter/docker-compose.yml b/sdk_compliance_adapter/docker-compose.yml index 127a3f6..b56ec36 100644 --- a/sdk_compliance_adapter/docker-compose.yml +++ b/sdk_compliance_adapter/docker-compose.yml @@ -7,7 +7,7 @@ services: - test-network test-harness: - image: ghcr.io/posthog/sdk-test-harness:0.9.0 + image: ghcr.io/posthog/sdk-test-harness:0.10.0 command: ["run", "--adapter-url", "http://sdk-adapter:8080", "--mock-url", "http://test-harness:8081"] networks: - test-network diff --git a/spec/posthog/client_spec.rb b/spec/posthog/client_spec.rb index ce83868..77a5d2e 100644 --- a/spec/posthog/client_spec.rb +++ b/spec/posthog/client_spec.rb @@ -1700,7 +1700,7 @@ def run 'company' => { '$group_key' => 'id:5', 'x' => 'y' }, 'instance' => { '$group_key' => 'app.posthog.com' } }, - 'person_properties' => { 'distinct_id' => 'some_id', 'x1' => 'y1' }, 'token' => 'testsecret' + 'person_properties' => { 'x1' => 'y1' }, 'token' => 'testsecret' } ) @@ -1734,7 +1734,7 @@ def run assert_requested :post, flags_endpoint, times: 1 expect(WebMock).to have_requested(:post, flags_endpoint).with( body: { 'distinct_id' => 'some_id', 'groups' => {}, 'group_properties' => {}, - 'person_properties' => { 'distinct_id' => 'some_id' }, 'token' => 'testsecret' } + 'person_properties' => {}, 'token' => 'testsecret' } ) WebMock.reset_executed_requests! @@ -1745,7 +1745,7 @@ def run 'distinct_id' => 'some_id', 'groups' => { 'company' => 'id:5' }, 'group_properties' => { 'company' => { '$group_key' => 'id:5' } }, - 'person_properties' => { 'distinct_id' => 'some_id' }, + 'person_properties' => {}, 'token' => 'testsecret' } ) @@ -1761,7 +1761,7 @@ def run assert_requested :post, flags_endpoint, times: 1 expect(WebMock).to have_requested(:post, flags_endpoint).with( body: { 'distinct_id' => 'some_id', 'groups' => {}, 'group_properties' => {}, - 'person_properties' => { 'distinct_id' => 'some_id' }, 'token' => 'testsecret' } + 'person_properties' => {}, 'token' => 'testsecret' } ) WebMock.reset_executed_requests! @@ -1769,7 +1769,7 @@ def run assert_requested :post, flags_endpoint, times: 1 expect(WebMock).to have_requested(:post, flags_endpoint).with( body: { 'distinct_id' => 'some_id', 'groups' => {}, 'group_properties' => {}, - 'person_properties' => { 'distinct_id' => 'some_id' }, 'token' => 'testsecret' } + 'person_properties' => {}, 'token' => 'testsecret' } ) WebMock.reset_executed_requests! end diff --git a/spec/posthog/feature_flag_spec.rb b/spec/posthog/feature_flag_spec.rb index 56dc740..9396309 100644 --- a/spec/posthog/feature_flag_spec.rb +++ b/spec/posthog/feature_flag_spec.rb @@ -57,6 +57,50 @@ module PostHog person_properties: { 'region' => 'Canada' })).to eq(false) end + it 'evaluates distinct_id locally without sending it in person_properties' do + api_feature_flag_res = { + 'flags' => [ + { + 'id' => 1, + 'name' => 'Distinct ID Feature', + 'key' => 'distinct-id-flag', + 'is_simple_flag' => true, + 'active' => true, + 'filters' => { + 'groups' => [ + { + 'properties' => [ + { + 'key' => 'distinct_id', + 'operator' => 'exact', + 'value' => 'some-distinct-id', + 'type' => 'person' + } + ], + 'rollout_percentage' => 100 + } + ] + } + } + ] + } + stub_request( + :get, + 'https://us.i.posthog.com/flags/definitions?token=testsecret&send_cohorts=true' + ).to_return(status: 200, body: api_feature_flag_res.to_json) + stub_request(:post, flags_endpoint).to_return(status: 400) + + c = Client.new(api_key: API_KEY, personal_api_key: API_KEY, test_mode: true) + person_properties = { region: 'USA' } + + expect(c.get_feature_flag('distinct-id-flag', 'some-distinct-id', + person_properties: person_properties, + only_evaluate_locally: true, + send_feature_flag_events: false)).to eq(true) + expect(person_properties).not_to have_key(:distinct_id) + expect(a_request(:post, flags_endpoint)).not_to have_been_made + end + it 'accepts symbol flag keys when evaluating locally' do api_feature_flag_res = { 'flags' => [ @@ -320,7 +364,7 @@ module PostHog 'distinct_id' => 'some-distinct-id_outside_rollout?', 'groups' => {}, 'group_properties' => {}, - 'person_properties' => { 'distinct_id' => 'some-distinct-id_outside_rollout?', 'region' => 'USA', + 'person_properties' => { 'region' => 'USA', 'email' => 'a@b.com' }, 'token' => 'testsecret' } @@ -338,7 +382,7 @@ module PostHog 'distinct_id' => 'some-distinct-id', 'groups' => {}, 'group_properties' => {}, - 'person_properties' => { 'distinct_id' => 'some-distinct-id', 'doesnt_matter' => '1' }, + 'person_properties' => { 'doesnt_matter' => '1' }, 'token' => 'testsecret' } ) @@ -4815,7 +4859,7 @@ module PostHog # Add the exact stub for the `/flags` endpoint as recommended in the error stub_request(:post, 'https://us.i.posthog.com/flags/?v=2').with( - body: '{"distinct_id":"distinct_id","groups":{},"person_properties":{"distinct_id":"distinct_id","region":"USA"},"group_properties":{},"token":"testsecret"}', # rubocop:disable Layout/LineLength + body: '{"distinct_id":"distinct_id","groups":{},"person_properties":{"region":"USA"},"group_properties":{},"token":"testsecret"}', # rubocop:disable Layout/LineLength headers: { 'Accept' => '*/*', 'Accept-Encoding' => 'gzip;q=1.0,deflate;q=0.6,identity;q=0.3',