feat: add is_deleted soft-delete to ClickHouse IDENTITIES table#7710
feat: add is_deleted soft-delete to ClickHouse IDENTITIES table#771010done wants to merge 12 commits into
Conversation
|
@10done is attempting to deploy a commit to the Flagsmith Team on Vercel. A member of the Team first needs to authorize it. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7710 +/- ##
==========================================
- Coverage 98.52% 98.39% -0.13%
==========================================
Files 1444 1453 +9
Lines 54971 55870 +899
==========================================
+ Hits 54161 54976 +815
- Misses 810 894 +84 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to handle deleted identities in ClickHouse by writing a tombstone row (with is_deleted=True) upon identity deletion, ensuring they are excluded from segment membership counts. It includes a ClickHouse migration, updates to the mapping logic, a new Celery task, and corresponding unit tests. The review feedback highlights a potential issue with write amplification and table bloat in multi-tenant environments, suggesting that we should verify if segment membership is enabled for the organization before writing the tombstone row.
| if not settings.CLICKHOUSE_ENABLED: | ||
| logger.info( | ||
| "tombstone.skipped", | ||
| reason="clickhouse_not_configured", | ||
| env_key=env_key, | ||
| identifier=identifier, | ||
| ) | ||
| return |
There was a problem hiding this comment.
In a multi-tenant environment, writing tombstone rows for every deleted identity across all organizations—even those that do not have segment_membership_inspection enabled—will lead to unnecessary ClickHouse write amplification and table bloat. We should check if the organization has segment membership enabled before writing the tombstone.
if not settings.CLICKHOUSE_ENABLED:
logger.info(
"tombstone.skipped",
reason="clickhouse_not_configured",
env_key=env_key,
identifier=identifier,
)
return
from environments.models import Environment
try:
environment = Environment.objects.select_related(
"project__organisation"
).get(api_key=env_key)
except Environment.DoesNotExist:
logger.info(
"tombstone.skipped",
reason="environment_not_found",
env_key=env_key,
identifier=identifier,
)
return
if not is_membership_enabled(environment.project.organisation):
logger.info(
"tombstone.skipped",
reason="segment_membership_disabled",
env_key=env_key,
identifier=identifier,
)
returnThere was a problem hiding this comment.
I have addressed this raised concern.
This comment was marked as spam.
This comment was marked as spam.
📝 WalkthroughWalkthroughAdds an Estimated code review effort: 3 (Moderate) | ~25 minutes 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 |
for more information, see https://pre-commit.ci
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/segment_membership/tasks.py (1)
42-48: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick winAlign the insert contract with the new five-value mapper.
The mapper and tombstone row now produce five values, but this SQL still declares
inserted_atas a sixth column. Additionally, the existing backfill call still passesscan_started_atpositionally, which the new keyword-only mapper signature rejects. Backfills will fail before writing, and tombstone inserts have the wrong shape. Removeinserted_atfrom this column list and update the stale mapper call to use the five-field contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 157acc4f-dcdd-4d85-b48e-8d7b27f7597b
📒 Files selected for processing (9)
api/clickhouse/migrations/0002_identities_add_is_deleted.pyapi/edge_api/identities/models.pyapi/segment_membership/mappers.pyapi/segment_membership/services.pyapi/segment_membership/tasks.pyapi/tests/unit/edge_api/identities/test_edge_identity_models.pyapi/tests/unit/segment_membership/test_unit_segment_membership_mappers.pyapi/tests/unit/segment_membership/test_unit_segment_membership_tasks.pydocs/docs/deployment-self-hosting/observability/_events-catalogue.md
| # (environment_id, identifier, identity_key, traits, is_deleted) | ||
| ClickHouseIdentityRow = tuple[str, str, str, dict[str, object] | None, bool] | ||
| from segment_membership.constants import INT64_MAX, INT64_MIN | ||
| from segment_membership.types import ClickHouseIdentityRow |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Remove the shadowed alias and restore import ordering.
ClickHouseIdentityRow is immediately replaced by the import on Line 8, and the intervening alias makes both imports violate E402. Keep the canonical type from segment_membership.types and move imports into the module import block.
🧰 Tools
🪛 Ruff (0.15.21)
[error] 7-7: Module level import not at top of file
(E402)
[error] 8-8: Module level import not at top of file
(E402)
Source: Linters/SAST tools
| select_clauses.append( | ||
| f"SELECT {seg.id} AS segment_id, " | ||
| f"i.environment_id AS env_key, count() AS c " | ||
| f"FROM IDENTITIES AS i FINAL " | ||
| f"WHERE i.environment_id IN %(env_keys)s " | ||
| f"AND i.is_deleted = false " | ||
| f"AND ({predicate}) " | ||
| f"GROUP BY i.environment_id" | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Remove the undefined select_clauses write.
select_clauses is never initialised, so this raises NameError for every translatable segment and prevents count refreshes. The count_columns query below is the active aggregation path; remove this obsolete block.
Proposed fix
- select_clauses.append(
- f"SELECT {seg.id} AS segment_id, "
- f"i.environment_id AS env_key, count() AS c "
- f"FROM IDENTITIES AS i FINAL "
- f"WHERE i.environment_id IN %(env_keys)s "
- f"AND i.is_deleted = false "
- f"AND ({predicate}) "
- f"GROUP BY i.environment_id"
- )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| select_clauses.append( | |
| f"SELECT {seg.id} AS segment_id, " | |
| f"i.environment_id AS env_key, count() AS c " | |
| f"FROM IDENTITIES AS i FINAL " | |
| f"WHERE i.environment_id IN %(env_keys)s " | |
| f"AND i.is_deleted = false " | |
| f"AND ({predicate}) " | |
| f"GROUP BY i.environment_id" | |
| ) |
🧰 Tools
🪛 Ruff (0.15.21)
[error] 156-156: Undefined name select_clauses
(F821)
Source: Linters/SAST tools
| open_cursor.assert_called_with( | ||
| log_comment=( | ||
| f"flagsmith:segment_membership:backfill" | ||
| f":org_{project.organisation_id}" | ||
| f":project_{project.id}" | ||
| ) | ||
| ) | ||
| sql, rows_arg = cursor.executemany.call_args.args | ||
| assert sql == ( | ||
| "INSERT INTO IDENTITIES " | ||
| "(environment_id, identifier, identity_key, traits, is_deleted) VALUES" | ||
| ) | ||
| assert {row[0] for row in rows_arg} == {environment.api_key} | ||
| assert {row[1] for row in rows_arg} == {"a", "b"} | ||
| assert any( | ||
| e["event"] == "backfill.environment.completed" and e["rows__count"] == 2 | ||
| for e in log.events | ||
| ) | ||
| refresh_dispatch.delay.assert_called_once_with(args=(project.id,)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Make the forced-failure assertions internally consistent.
This test injects an insert failure, then requires a completion event while also requiring log.events to contain only seed.environment.failed; both cannot be true. It also dereferences undefined refresh_dispatch. Remove the success-path assertion and bind/assert the intended dispatch mock.
🧰 Tools
🪛 Ruff (0.15.21)
[error] 160-160: Undefined name refresh_dispatch
(F821)
Source: Linters/SAST tools
| ### `segment_membership.tombstone.skipped` | ||
|
|
||
| Logged at `info` from: | ||
| - `api/segment_membership/tasks.py:211` | ||
| - `api/segment_membership/tasks.py:224` | ||
| - `api/segment_membership/tasks.py:233` | ||
|
|
||
| Attributes: | ||
| - `env_key` | ||
| - `identifier` | ||
| - `reason` | ||
|
|
||
| ### `segment_membership.tombstone.written` | ||
|
|
||
| Logged at `info` from: | ||
| - `api/segment_membership/tasks.py:247` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Regenerate the tombstone event locations.
These references are stale: the current task logs tombstone skips at Lines 293, 306, and 315, and the successful write at Line 329.
4 participants
Thanks for submitting a PR! Please check the boxes below:
docs/if required so people know about the feature.Changes
Fixes #7593
is_deleted Bool DEFAULT falsecolumn to the ClickHouseIDENTITIEStable via a new migrationis_deletedon every backfill row (defaults tofalse)write_identity_deletion_tombstone_to_clickhouseasync task, dispatched fromEdgeIdentity.delete()to tombstone deleted identities in real timeAND i.is_deleted = falseto the segment membership count query so deleted identities never appear in countsHow did you test this code?