feat(cohorts): apply cohort membership to Postgres identities - #8315
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 3 Skipped Deployments
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe change adds Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🟡 Moderate · up to Cohort-gated flags may be enabled or remain enabled based on user-controlled traits rather than actual cohort membership, creating incorrect flag exposure. This concrete correctness and security risk should be resolved or explicitly accepted before merging. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8315 +/- ##
========================================
Coverage 98.78% 98.78%
========================================
Files 1604 1609 +5
Lines 64426 64575 +149
========================================
+ Hits 63642 63791 +149
Misses 784 784 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
✅ private-cloud · depot-ubuntu-latest-16 — run #19530 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)Details
🗂️ Previous results✅ private-cloud · depot-ubuntu-latest-arm-16 — run #19530 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)Details
✅ oss · depot-ubuntu-latest-arm-16 — run #19530 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)Details
✅ oss · depot-ubuntu-latest-16 — run #19530 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)Details
✅ private-cloud · depot-ubuntu-latest-16 — run #19529 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)Details
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #19529 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)Details
✅ oss · depot-ubuntu-latest-arm-16 — run #19529 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)Details
✅ oss · depot-ubuntu-latest-16 — run #19529 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)Details
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #19528 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)Details
✅ private-cloud · depot-ubuntu-latest-16 — run #19528 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)Details
✅ oss · depot-ubuntu-latest-arm-16 — run #19528 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)Details
✅ oss · depot-ubuntu-latest-16 — run #19528 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)Details
|
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 65178f22-fee3-41dc-8c9f-298c38385826
📒 Files selected for processing (11)
api/cohorts/services.pyapi/cohorts/tasks.pyapi/cohorts/views.pyapi/environments/identities/migrations/0007_identity_system_traits.pyapi/environments/identities/models.pyapi/tests/unit/cohorts/test_evaluation.pyapi/tests/unit/cohorts/test_services.pyapi/tests/unit/cohorts/test_tasks.pyapi/tests/unit/cohorts/test_views.pyapi/util/mappers/engine.pydocs/docs/deployment-self-hosting/observability/_events-catalogue.md
💤 Files with no reviewable changes (2)
- api/cohorts/tasks.py
- api/cohorts/views.py
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.
Visual Regression19 screenshots compared. See report for details. |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b1d8df87-bc8f-404b-8f34-e6b595f287e9
📒 Files selected for processing (6)
api/cohorts/services.pyapi/environments/identities/system_traits.pyapi/tests/unit/cohorts/test_services.pyapi/tests/unit/cohorts/test_tasks.pyapi/tests/unit/environments/identities/test_system_traits.pydocs/docs/deployment-self-hosting/observability/_events-catalogue.md
Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/tests/unit/environments/identities/test_system_traits.py (1)
96-115: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert against the selected identity backend.
These parametrised tests run for PostgreSQL and DynamoDB, but both assertions only inspect
Identity.objects. Foredge_environment, an incorrect DynamoDB write would still pass because the DynamoDB document is not checked. Assert that_stored_system_traits(target_environment, "sentinel") is Nonein both tests, or inspect the DynamoDB wrapper for the edge case.Proposed fix
def test_set_system_trait__no_identifiers__creates_no_identity( target_environment: Environment, ) -> None: + identifier = "sentinel" set_system_trait(target_environment, _TRAIT_KEY, []) - assert not Identity.objects.filter(environment=target_environment).exists() + assert _stored_system_traits(target_environment, identifier) is Nonedef test_unset_system_trait__no_identifiers__creates_no_identity( target_environment: Environment, ) -> None: + identifier = "sentinel" unset_system_trait(target_environment, _TRAIT_KEY, []) - assert not Identity.objects.filter(environment=target_environment).exists() + assert _stored_system_traits(target_environment, identifier) is None
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 13bbd081-5a57-4b92-bbfc-be7531ce9958
📒 Files selected for processing (3)
api/tests/unit/cohorts/test_evaluation.pyapi/tests/unit/cohorts/test_services.pyapi/tests/unit/environments/identities/test_system_traits.py
💤 Files with no reviewable changes (1)
- api/tests/unit/cohorts/test_evaluation.py
Included review availability: Your plan provides up to 8 included reviews per hour; 4 remain after this review.
docs/if required so people know about the feature.Changes
Cohorts only worked on edge projects. Now they work when identities are in Postgres too, which covers self-hosted.
Identity.system_traits, same shape as thesystem_traitsmap on edge identity documents.environments/identities/system_traits.pysets and clears a system trait and picks DynamoDB or Postgres itself.system_traitsalongside the user's traits, so theIS_SETcondition on the cohort's segment works as before. It's a column on a row we already load, soget_segmentsdoesn't do any more queries.jsonboperators, so two cohorts writing to the same identity can't wipe each other's keys.How did you test this code?
Unit tests run the same cases against both stores. On top of that: membership turns a segment-gated flag on, removal turns it off, retries don't break anything, and one identity can be in two cohorts.
I also broke the code on purpose to check the tests catch it. Making the Postgres write do nothing fails 8 tests. Skipping the trait merge in evaluation fails 2.
Migration
0007on a 3M row table takes under 2ms because Postgres only writes metadata for a constant default (the same thing with a volatile default takes ~1.5s). Taking the lock on the production identities table was near instant.