Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 3 Skipped Deployments
|
📝 WalkthroughWalkthroughThe change adds a Django dependencies application with persistent segment-to-feature references. It indexes supported Priority: ⬇️ Low Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🟡 Moderate · up to Supported workflows can commit stale references or circular dependencies, especially through change requests, versioning, and deeply nested rules. These gaps should be fixed 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 |
github-actions
Bot
added
api
36dde6b to
b28b065
Compare
github-actions
Bot
added
feature
b28b065 to
a819d0c
Compare
github-actions
Bot
added
feature
github-actions
Bot
added
feature
66804ae to
3608b5e
Compare
github-actions
Bot
added
feature
github-actions
Bot
added
feature
github-actions
Bot
added
feature
Docker builds report
|
✅ private-cloud · depot-ubuntu-latest-16 — run #20475 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
🗂️ Previous results✅ private-cloud · depot-ubuntu-latest-arm-16 — run #20475 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
✅ oss · depot-ubuntu-latest-arm-16 — run #20475 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
✅ oss · depot-ubuntu-latest-16 — run #20475 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8525 +/- ##
==========================================
- Coverage 98.82% 98.82% -0.01%
==========================================
Files 1622 1629 +7
Lines 66727 66983 +256
==========================================
+ Hits 65942 66193 +251
- Misses 785 790 +5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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: 4
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Advanced
Run ID: cadb677c-0a35-48fb-b5d4-24bb58dc9ca3
📒 Files selected for processing (15)
api/app/settings/common.pyapi/features/dependencies/__init__.pyapi/features/dependencies/apps.pyapi/features/dependencies/exceptions.pyapi/features/dependencies/migrations/0001_initial.pyapi/features/dependencies/migrations/__init__.pyapi/features/dependencies/models.pyapi/features/dependencies/services.pyapi/features/dependencies/types.pyapi/features/views.pyapi/segments/serializers.pyapi/segments/services.pyapi/tests/integration/features/dependencies/test_segment_management.pyapi/tests/unit/segments/test_unit_segments_services.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.
| with transaction.atomic(): | ||
| feature_state = serializer.save(environment=environment, feature=feature) # type: ignore[no-untyped-call] | ||
| if feature_segment := feature_state.feature_segment: | ||
| validate_segment_flag_dependencies(feature_segment.segment) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
# Find other creation sites for segment overrides.
rg -nP -C 5 'FeatureSegment\.objects\.(create|get_or_create|bulk_create)' --type=py -g '!**/tests/**'
rg -nP -C 5 'class \w*(FeatureSegment|SegmentOverride)\w*(ViewSet|View|Serializer)\b' --type=py -g '!**/tests/**'
# Confirm the current call sites of the validation service.
rg -nP '\bvalidate_segment_flag_dependencies\s*\(' --type=pyRepository: Flagsmith/flagsmith
Length of output: 15239
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- feature segment views ---'
sed -n '1,150p' api/features/feature_segments/views.py
printf '%s\n' '--- feature segment serializers ---'
sed -n '1,115p' api/features/feature_segments/serializers.py
printf '%s\n' '--- feature segment model ---'
rg -n -C 8 'class FeatureSegment\b|class FeatureState\b' api/features/models.py
printf '%s\n' '--- feature segment route registration ---'
rg -n -C 5 'FeatureSegmentViewSet|feature_segments|FeatureSegment' api/features -g '*.py' -g '!**/migrations/**' | head -240
printf '%s\n' '--- versioning service creation context ---'
sed -n '130,245p' api/features/versioning/versioning_service.py
sed -n '285,330p' api/features/versioning/versioning_service.py
printf '%s\n' '--- versioning service callers ---'
rg -n -C 8 'create_feature_state|create_feature_segment|apply.*change|change_set|versioning_service' api/features -g '*.py' -g '!**/migrations/**' | head -320
printf '%s\n' '--- future service and callers ---'
sed -n '150,205p' api/features/future/services.py
rg -n -C 8 'future\.services|create_feature_state|SegmentOverrideAPIView|SegmentOverrideSerializer' api/features/future api -g '*.py' -g '!**/migrations/**' | head -260
printf '%s\n' '--- validator and all call sites ---'
sed -n '90,145p' api/features/dependencies/services.py
rg -n -C 5 'validate_segment_flag_dependencies\s*\(' api -g '*.py' -g '!**/tests/**'Repository: Flagsmith/flagsmith
Length of output: 50375
🏁 Script executed:
set -eu
sed -n '1,105p' api/features/feature_segments/views.py
sed -n '1,90p' api/features/feature_segments/serializers.py
sed -n '100,140p' api/features/dependencies/services.py
sed -n '150,190p' api/features/versioning/versioning_service.py
sed -n '210,235p' api/features/versioning/versioning_service.py
sed -n '295,325p' api/features/versioning/versioning_service.py
sed -n '165,195p' api/features/future/services.py
rg -n -C 6 'FeatureSegmentViewSet|feature_segments' api -g '*.py' -g '!**/migrations/**'
rg -n -C 5 'validate_segment_flag_dependencies\s*\(' api -g '*.py' -g '!**/tests/**'Repository: Flagsmith/flagsmith
Length of output: 50375
🏁 Script executed:
set -eu
printf '%s\n' '--- dependency validator ---'
sed -n '100,190p' api/features/dependencies/services.py
printf '%s\n' '--- future service helpers and callers ---'
sed -n '150,245p' api/features/future/services.py
rg -n -C 12 '_create_segment_override\(' api/features/future api/features/versioning api -g '*.py' -g '!**/tests/**' | head -220
printf '%s\n' '--- versioning serializer save flow ---'
sed -n '1,180p' api/features/versioning/serializers.py
sed -n '250,340p' api/features/versioning/serializers.pyRepository: Flagsmith/flagsmith
Length of output: 34995
🏁 Script executed:
set -eu
rg -n -C 8 'def get_overrides_in_effect|get_overrides_in_effect\s*=' api/features api/segments -g '*.py'
sed -n '248,460p' api/features/models.py
rg -n -C 8 'feature_states.*FeatureSegment|feature_segment.*related_name|class FeatureState' api/features/models.pyRepository: Flagsmith/flagsmith
Length of output: 10459
Validate dependency cycles in every segment override creation path. FeatureSegmentViewSet creates active FeatureSegment records without calling validate_segment_flag_dependencies. The environment-version serializer and the v1/v2 versioning helpers also create segment overrides without this check. These paths can persist an override that closes a circular dependency. Call the validator after each creation and keep creation plus validation atomic.
| index_segment_flag_references(segment) | ||
| validate_segment_flag_dependencies(segment) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
# Locate segment revision/change-request commit paths and check for indexing calls.
rg -nP -C 6 '\b(is_revision|version_of|change_request)\b' --type=py -g '!**/tests/**' api/segments api/features/workflows | head -200
# Check which modules call the new services.
rg -nP '\b(index_segment_flag_references|validate_segment_flag_dependencies)\s*\(' --type=pyRepository: Flagsmith/flagsmith
Length of output: 20082
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- workflow service candidates ---'
fd -t f . api/features/workflows api/segments | rg -i 'service|model|serializ|view'
printf '%s\n' '--- commit service definitions and callers ---'
rg -n -C 12 'class ChangeRequestCommitService|def commit\(|ChangeRequestCommitService|is_revision|version_of|clone\(' api/features api/segments --type py -g '!**/tests/**' | head -320
printf '%s\n' '--- dependency service implementation ---'
cat -n api/features/dependencies/services.py | sed -n '1,180p'Repository: Flagsmith/flagsmith
Length of output: 31418
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- exact commit service binding ---'
rg -n -C 20 'ChangeRequestCommitService|class .*CommitService|def copy_rules_and_conditions_from|copy_rules_and_conditions_from|restore.*revision|revision.*restore' . --type py -g '!**/tests/**' | head -500
printf '%s\n' '--- rules_data writes and dependency-service call sites ---'
rg -n -C 8 'rules_data\s*=|update\(.*rules_data|update_fields=.*rules_data|index_segment_flag_references|validate_segment_flag_dependencies' . --type py -g '!**/tests/**' | head -500Repository: Flagsmith/flagsmith
Length of output: 32750
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- commit segment publication ---'
cat -n api/core/workflows_services.py | sed -n '23,155p'
printf '%s\n' '--- segment copy helper binding ---'
rg -n -C 12 'copy_segment_rules_and_conditions|def copy_|rules_data' api/segments/models.py api/segments/services.py | head -220Repository: Flagsmith/flagsmith
Length of output: 13866
🏁 Script executed:
cat -n api/segments/services.py | sed -n '127,205p'Repository: Flagsmith/flagsmith
Length of output: 3283
Rebuild dependency references when committing a change request.
ChangeRequestCommitService._publish_segments copies only the draft SegmentRule and Condition rows. The bound helper does not copy draft_segment.rules_data. The commit path also does not call index_segment_flag_references or validate_segment_flag_dependencies.
The index therefore remains based on the previous live_segment.rules_data. Removed prerequisites can remain indexed, and cycles in the committed draft rules bypass dependency validation.
Copy draft_segment.rules_data to live_segment, persist it, then rebuild and validate the references inside the existing _publish_segments transaction.
There was a problem hiding this comment.
This is deferred to https://github.com/Flagsmith/flagsmith-private/issues/296.
There was a problem hiding this comment.
@emyller, acknowledged. Issue flagsmith-private#296 tracks the change-request publication work. This finding remains deferred for this pull request.
🧠 Learnings used
Learnt from: emyller
Repo: Flagsmith/flagsmith PR: 8525
File: api/features/dependencies/services.py:178-179
Timestamp: 2026-09-16T01:27:42.961Z
Learning: In the Flagsmith segment rule data model, rule depth is limited to two levels. `api/features/dependencies/services.py::_get_rules_by_json_path` only needs to process root rules and their direct child rules.
You are interacting with an AI system.
| assert list( | ||
| SegmentFlagReference.objects.values( | ||
| "segment", | ||
| "prerequisite_feature", | ||
| "condition_json_path", | ||
| ) | ||
| ) == [ |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Unordered SegmentFlagReference querysets compared to ordered lists. SegmentFlagReference declares no Meta.ordering, and neither assertion adds order_by, so the database can return the rows in any order and both assertions are unstable. Both expected lists are already sorted by condition_json_path, so adding that ordering keeps the expected values unchanged.
api/tests/integration/features/dependencies/test_segment_management.py#L76-L82: replaceSegmentFlagReference.objects.values(withSegmentFlagReference.objects.order_by("condition_json_path").values(.api/tests/integration/features/dependencies/test_segment_management.py#L197-L203: apply the sameorder_by("condition_json_path")call; this case deletes and recreates the rows, so the order is even less predictable.
📍 Affects 1 file
api/tests/integration/features/dependencies/test_segment_management.py#L76-L82(this comment)api/tests/integration/features/dependencies/test_segment_management.py#L197-L203
2 participants
I have added information todocs/if required so people know about the feature.Changes
Closes #8412
This implements the support foundation for flag dependency in the Management API:
How did you test this code?
Functional tests included.