fix(segments): Segment Change Requests bypassable via segment deletion - #8358
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 (4)
💤 Files with no reviewable changes (2)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe change centralises Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🟠 High · up to The change is intended to prevent direct segment edits and deletion from bypassing change-request approval, but current implementation risks still allowing protected segments to be deleted—especially for versioned segments or during concurrent override changes—and the rejection test may not verify the required response. These issues should be addressed before merge. 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 |
7a3b5a9 to
fb022d1
Compare
Docker builds report
|
✅ private-cloud · depot-ubuntu-latest-16 — run #19839 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)Details
🗂️ Previous results✅ private-cloud · depot-ubuntu-latest-arm-16 — run #19839 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)Details
✅ oss · depot-ubuntu-latest-arm-16 — run #19840 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)Details
✅ oss · depot-ubuntu-latest-16 — run #19839 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)Details
✅ oss · depot-ubuntu-latest-arm-16 — run #19839 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)Details
✅ oss · depot-ubuntu-latest-16 — run #19840 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)Details
✅ private-cloud · depot-ubuntu-latest-16 — run #19835 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)Details
✅ oss · depot-ubuntu-latest-arm-16 — run #19837 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)Details
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #19835 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)Details
✅ oss · depot-ubuntu-latest-16 — run #19837 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)Details
✅ oss · depot-ubuntu-latest-16 — run #19835 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)Details
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #19831 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)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: 2f7bc5d1-6781-4961-bd6c-07616b1340b0
📒 Files selected for processing (6)
api/segments/serializers.pyapi/segments/views.pyapi/tests/unit/segments/test_unit_segments_views.pydocs/docs/deployment-self-hosting/observability/_events-catalogue.mdmcp/src/flagsmith_mcp/openapi.jsonopenapi.yaml
Included review availability: Your plan provides up to 8 included reviews per hour; 5 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 (2)
api/segments/views.py (2)
105-111: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftKeep override detection aligned with
delete_segment().
FeatureSegment.segmentaccepts anySegment, and override creation stores the supplied segment ID. If an effective override targets a versioned segment, the view checks only the canonical segment ID, whiledelete_segment()removes overrides for the canonical segment and its versions. The view can therefore allow deletion without raisingChangeRequestsEnabledError. Share the expanded segment-ID set, or perform the check insidedelete_segment().
286-288: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake the override check and deletion atomic.
When change requests are enabled, an override can be committed after
_check_segment_is_deletable()and beforedelete_segment()starts its transaction.delete_segment()then removes the override without rejecting the deletion. Re-check within the deletion transaction and share serialisation with override writers. Add a concurrent test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 09597a5a-6ad4-445c-9bec-57ac572dfcfc
📒 Files selected for processing (1)
api/segments/views.py
Included review availability: Your plan provides up to 8 included reviews per hour; 4 remain after this review.
matthewelwell
left a comment
There was a problem hiding this comment.
Mostly LGTM - one minor comment on function naming.
Co-authored-by: Matthew Elwell <matthew.elwell@flagsmith.com>
for more information, see https://pre-commit.ci
Thanks for submitting a PR! Please check the boxes below:
docs/if required so people know about the feature.Changes
Closes #7532.
In this PR, we prevent the following when Segment Change Requests are enabled:
PUTandPATCHon a segment are refused, including metadata-only writes. Drafts held by a change request are exempt, since editing those is the workflow rather than a way around it.Supporting changes:
ChangeRequestsEnabledErrormoves fromfeatures/future/exceptions.pytocore/exceptions.pyso both features and segments raise the same 409 and error code. Its detail message is now generic.Project.is_workflow_enabled, mirroringEnvironment.is_workflow_enabled.EnvironmentFeatureVersionManager.get_superseded_versions(), which answers "has a later version already replaced this one" as a correlated subquery. Ordering by go-live rather than creation is deliberate, per Enabled features can disagree with the flags actually served after a scheduled change #8127.has_overrides, so the UI is able to gray out submission buttons based on it.segments/views.pymoves to structlog, emittingsegments.update_rejectedandsegments.delete_rejected.Deliberately out of scope:
FeatureSegmentViewSetandEnvironmentFeatureVersionViewSet.create/publishhave no change request check, so the API route around Feature Change Requests remains open. The documentation added here is careful not to claim otherwise.How did you test this code?
Added unit tests, and benchmarked
EnvironmentFeatureVersionManager.get_superseded_versions().