feat: cohort CSV sync endpoint and cohort summary on segments - #8294
feat: cohort CSV sync endpoint and cohort summary on segments#8294Zaimwa9 wants to merge 2 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 3 Skipped Deployments
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds multipart CSV synchronisation for cohorts. The change validates CSV size and content, extracts identifiers, updates memberships in batches, tracks ignored rows, increments cohort versions, and queues delta application. Cohort creation now accepts and persists metadata. Segment responses expose related cohort details. The change adds metrics, event catalogue entries, tests, and OpenAPI schemas. Estimated code review effort: 4 (Complex) | ~60 minutes Merge Risk: 🟠 High · up to The CSV synchronization feature can strand membership updates, accept identifiers that downstream processing cannot handle, or leave external membership state inconsistent with the database under concurrent sync and application. These correctness and data-integrity risks make the PR unsafe to merge until the synchronization and validation paths are fixed. 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 |
github-actions
Bot
added
api
Docker builds report
|
|
@themis-blindfold review |
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #19370 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
🗂️ Previous results✅ private-cloud · depot-ubuntu-latest-16 — run #19370 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
✅ oss · depot-ubuntu-latest-arm-16 — run #19370 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
✅ oss · depot-ubuntu-latest-arm-16 — run #19373 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
✅ oss · depot-ubuntu-latest-16 — run #19370 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
✅ oss · depot-ubuntu-latest-16 — run #19373 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
✅ private-cloud · depot-ubuntu-latest-16 — run #19365 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
✅ oss · depot-ubuntu-latest-arm-16 — run #19366 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #19365 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
✅ oss · depot-ubuntu-latest-16 — run #19366 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
✅ oss · depot-ubuntu-latest-arm-16 — run #19365 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
✅ oss · depot-ubuntu-latest-16 — run #19365 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
|
Visual Regression19 screenshots compared. See report for details. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6fc61ada-1f5d-43a2-8a9d-94781de30573
📒 Files selected for processing (17)
api/cohorts/constants.pyapi/cohorts/dataclasses.pyapi/cohorts/exceptions.pyapi/cohorts/metrics.pyapi/cohorts/serializers.pyapi/cohorts/services.pyapi/cohorts/views.pyapi/segments/serializers.pyapi/segments/views.pyapi/tests/unit/cohorts/conftest.pyapi/tests/unit/cohorts/test_services.pyapi/tests/unit/cohorts/test_views.pyapi/tests/unit/segments/test_unit_segments_views.pydocs/docs/deployment-self-hosting/observability/_events-catalogue.mddocs/docs/deployment-self-hosting/observability/_metrics-catalogue.mdmcp/src/flagsmith_mcp/openapi.jsonopenapi.yaml
| max_length: int = CohortMembership._meta.get_field("identifier").max_length # type: ignore[assignment] | ||
| # The upload size cap keeps a full read cheap; utf-8-sig strips any BOM. | ||
| text = io.StringIO(file.read().decode("utf-8-sig", errors="replace"), newline="") | ||
| reader = csv.reader(text) | ||
| seen: set[str] = set() | ||
| identifiers: list[str] = [] | ||
| empty_count = duplicate_count = too_long_count = 0 | ||
| try: | ||
| for row_number, row in enumerate(reader): | ||
| if has_header and row_number == 0: | ||
| continue | ||
| if not row: | ||
| continue | ||
| value = ( | ||
| row[identifier_column].strip() if identifier_column < len(row) else "" | ||
| ) | ||
| if not value: | ||
| empty_count += 1 | ||
| elif len(value) > max_length: | ||
| too_long_count += 1 |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Enforce the Edge identifier byte limit before persistence.
Line 135 checks the model character limit. It does not enforce the 1,024-byte Edge limit. For example, a 600-emoji identifier passes this check but uses 2,400 UTF-8 bytes.
Reject an identifier above 1,024 UTF-8 bytes with HTTP 400. Do not classify it only as an ignored row. Otherwise, the service can persist memberships that the DynamoDB identity path cannot process.
Based on learnings: “validate cohort identifiers before persistence and reject identifiers exceeding 1024 bytes with HTTP 400, matching Edge DynamoDB constraints.”
Source: Learnings
| elif membership.state == CohortMembershipState.PENDING_ADD: | ||
| # Never applied, so no identity data to drain: drop the row. | ||
| discard_ids.append(membership.id) | ||
| # Rows already pending removal stay on their way out. | ||
|
|
||
| added += CohortMembership.objects.filter(id__in=readd_ids).update( | ||
| state=CohortMembershipState.PENDING_ADD, updated_at=timezone.now() | ||
| ) | ||
| removed += CohortMembership.objects.filter(id__in=remove_ids).update( | ||
| state=CohortMembershipState.PENDING_REMOVE, updated_at=timezone.now() | ||
| ) | ||
| discarded, _ = CohortMembership.objects.filter(id__in=discard_ids).delete() | ||
| removed += discarded |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Coordinate sync deletion with membership application.
The sync deletes a PENDING_ADD row when its identifier is absent. apply_pending_memberships() can already have selected that row, write its DynamoDB trait, then find that its state update affects zero rows. The worker then sees no pending row, so the trait remains set without a membership record.
Use one shared claim or locking protocol for the sync path and applier. The sync must not discard a pending add while an applier can write its external side effect.
| locked_cohort.version += 1 | ||
| locked_cohort.save(update_fields=["version"]) | ||
| apply_cohort_membership_deltas.delay(kwargs={"cohort_id": cohort.id}) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Queue the applier after the transaction commits.
apply_cohort_membership_deltas.delay() runs before the membership rows and version commit. A separate worker can read no pending rows, return without rescheduling, and leave the committed rows pending indefinitely.
Register the task with transaction.on_commit() so the worker can only run after this synchronisation is durable.
Based on learnings: “do not rely on ATOMIC_REQUESTS being enabled” and “treat transaction.atomic() in service code as defining the transaction boundary itself.”
Source: Learnings
| '/api/v1/environments/{environment_api_key}/cohorts/{cohort_id}/sync-csv/': | ||
| post: | ||
| operationId: api_v1_environments_cohorts_sync_csv_create | ||
| description: Replace the cohort's members with the identifiers found in the uploaded CSV file and trigger a sync to identity data. `identifier_column` is the 0-based index of the column holding the identifiers; `has_header` skips the first row when true. | ||
| parameters: | ||
| - name: cohort_id | ||
| in: path | ||
| description: A unique integer value identifying this cohort. | ||
| required: true | ||
| schema: | ||
| type: integer | ||
| - name: environment_api_key | ||
| in: path | ||
| required: true | ||
| schema: | ||
| type: string | ||
| requestBody: | ||
| required: true | ||
| content: | ||
| multipart/form-data: | ||
| schema: | ||
| $ref: '#/components/schemas/CohortCsvSync' | ||
| responses: | ||
| '202': | ||
| description: '' | ||
| content: | ||
| application/json: | ||
| schema: | ||
| $ref: '#/components/schemas/CohortCsvSyncResult' | ||
| security: | ||
| - tokenAuth: [] | ||
| - Master API Key: [] | ||
| tags: | ||
| - Environments | ||
| x-flagsmith-minimum-plan: START_UP |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Consider documenting error responses for the CSV size/validation limits.
The PR summary states that files larger than 10 MB are rejected, and rows are validated (empty, duplicate, too-long identifiers). The operation only documents a 202 response. Add a 400 (or 413) response schema so API consumers can distinguish a rejected upload from a successful sync without relying on undocumented behaviour.
| _SegmentCohort: | ||
| type: object | ||
| properties: | ||
| id: | ||
| type: integer | ||
| readOnly: true | ||
| environment: | ||
| type: integer | ||
| source_type: | ||
| $ref: '#/components/schemas/SourceTypeEnum' | ||
| version: | ||
| type: integer | ||
| maximum: 2147483647 | ||
| minimum: 0 | ||
| deletion_requested_at: | ||
| type: | ||
| - string | ||
| - 'null' | ||
| format: date-time | ||
| required: | ||
| - environment |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Compare `version` field declarations across cohort serializers.
set -euo pipefail
fd -e py . api/cohorts api/segments 2>/dev/null | xargs rg -n -B3 -A3 "version" 2>/dev/nullRepository: Flagsmith/flagsmith
Length of output: 21068
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- cohort serializers ---'
cat -n api/cohorts/serializers.py | sed -n '1,125p'
printf '%s\n' '--- segment serializer cohort fields ---'
cat -n api/segments/serializers.py | sed -n '90,125p'
printf '%s\n' '--- cohort schema declarations ---'
rg -n -A18 -B3 '^ (Cohort|CohortCsvSyncResult|_SegmentCohort):$' openapi.yamlRepository: Flagsmith/flagsmith
Length of output: 7342
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- complete Cohort schema ---'
sed -n '18841,18903p' openapi.yaml
printf '%s\n' '--- cohort model and sync result construction ---'
cat -n api/cohorts/models.py | sed -n '1,45p'
cat -n api/cohorts/services.py | sed -n '210,255p'Repository: Flagsmith/flagsmith
Length of output: 5694
Align the documented version constraints.
_SegmentCohort.version declares minimum: 0 and maximum: 2147483647, but Cohort.version and CohortCsvSyncResult.version do not. Expose the same constraints in each schema to prevent inconsistent client validation.
⚖️ Themis review: ✅ Ship itClean, well-structured addition of the CSV sync endpoint and cohort summary on segments. The delta computation in
📝 Walkthrough
🧪 How to verify
Product take: This closes the loop on CSV-based cohort management by giving the dashboard everything it needs to upload, re-sync, and display cohort membership. Solid capability addition that unlocks the full create-from-CSV workflow. 🧭 Assumptions & unverified claims
A CSV walks into a bar; the parser orders one identifier, neat — no duplicates, no empties, hold the BOM. · reviewed at aa61a35 |
05b2fcf to
033c67d
Compare
aa61a35 to
4083994
Compare
github-actions
Bot
added
feature
71a387d to
3a5f36a
Compare
github-actions
Bot
added
feature
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## feat/create-segment-from-csv #8294 +/- ##
===============================================================
Coverage ? 98.70%
===============================================================
Files ? 1586
Lines ? 63478
Branches ? 0
===============================================================
Hits ? 62657
Misses ? 821
Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1 participant
Thanks for submitting a PR! Please check the boxes below:
docs/if required so people know about the feature.Changes
Adds the API surface the dashboard needs to create segments from a CSV of identifiers, on top of the cohort CRUD from #8248.
CSV sync endpoint
POST /api/v1/environments/{api_key}/cohorts/{id}/sync-csv/: multipart upload withfile,identifier_column(0-based, defaults to the first column) andhas_header(defaults to true).202with{version, added, removed, unchanged, ignored: {empty, duplicates, too_long}}and queues the membership delta application inside the transaction, with a row lock on the cohort and a version bump.413.Cohort creation
POST .../cohorts/now accepts segment metadata (custom fields), applied to the managed segment.Segments API
cohortsummary (id,environment,source_type,version,deletion_requested_at), prefetched on the list view. This lets the dashboard tag CSV segments, route their deletion through the cohorts endpoint, and grey out segments awaiting drain.Observability
flagsmith_cohorts_csv_syncs_totalandflagsmith_cohorts_csv_sync_identifiers, newcohorts.csv.syncedevent, both documented in the observability catalogues.How did you test this code?