fix: count actual existing owners in _validate_owner_removal validation#8032
fix: count actual existing owners in _validate_owner_removal validation#8032Marnie0415 wants to merge 3 commits into
Conversation
|
@mimocode is attempting to deploy a commit to the Flagsmith Team on Vercel. A member of the Team first needs to authorize it. |
|
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:
📝 WalkthroughWalkthroughFeature owner and group-owner removal validation now accepts requested ID sets and counts only IDs that exist on the feature before enforcing the minimum-owner rule. Tests cover nonexistent IDs alongside real removals, including blocked and permitted outcomes for users and group owners. Estimated code review effort: 3 (Moderate) | ~20 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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: 76b6a08d-e537-44cf-80b3-dd6331d5df49
📒 Files selected for processing (2)
api/features/views.pyapi/tests/unit/features/test_unit_features_views.py
There was a problem hiding this comment.
♻️ Duplicate comments (1)
api/features/views.py (1)
490-503: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUse prefetched relations to avoid additional database queries.
Since both
ownersandgroup_ownersare already prefetched in theget_querysetmethod, using.values_list()and.count()will ignore the prefetch cache and trigger four additional database queries. You can iterate over the prefetched.all()objects and uselen()to perform these checks efficiently in memory.⚡ Proposed refactor to utilise the prefetch cache
- existing_owner_ids = set(feature.owners.values_list("id", flat=True)) - existing_group_owner_ids = set( - feature.group_owners.values_list("id", flat=True) - ) - - actual_owners_to_remove = len(owner_ids & existing_owner_ids) - actual_group_owners_to_remove = len(group_owner_ids & existing_group_owner_ids) - - remaining = ( - feature.owners.count() - - actual_owners_to_remove - + feature.group_owners.count() - - actual_group_owners_to_remove - ) + existing_owners = feature.owners.all() + existing_group_owners = feature.group_owners.all() + + existing_owner_ids = {owner.id for owner in existing_owners} + existing_group_owner_ids = {group.id for group in existing_group_owners} + + actual_owners_to_remove = len(owner_ids & existing_owner_ids) + actual_group_owners_to_remove = len(group_owner_ids & existing_group_owner_ids) + + remaining = ( + len(existing_owners) + - actual_owners_to_remove + + len(existing_group_owners) + - actual_group_owners_to_remove + )
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 879f1ff3-8368-4e65-b21f-4df8248438ae
📒 Files selected for processing (1)
api/features/views.py
dfd4c60 to
eda5fb1
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
api/features/views.py (1)
490-508: 🧹 Nitpick | 🔵 TrivialUtilise prefetched relations to avoid additional database queries.
As noted in a previous review, because both
ownersandgroup_ownersare prefetched in theget_querysetmethod, using.values_list("id", flat=True)and.count()bypasses the prefetch cache and triggers additional database queries.Although the
if owner_idsandif group_owner_idsconditionals attempt to skip queries when the sets are empty, the subsequent.count()calls will still trigger database queries unconditionally. Evaluatingfeature.owners.all()and usinglen()allows these checks to be performed efficiently in memory.⚡ Proposed refactor to utilise the prefetch cache
- actual_owners_to_remove = ( - len(owner_ids & set(feature.owners.values_list("id", flat=True))) - if owner_ids - else 0 - ) - actual_group_owners_to_remove = ( - len( - group_owner_ids & set(feature.group_owners.values_list("id", flat=True)) - ) - if group_owner_ids - else 0 - ) + existing_owners = feature.owners.all() + existing_group_owners = feature.group_owners.all() + + actual_owners_to_remove = ( + len(owner_ids & {owner.id for owner in existing_owners}) + if owner_ids + else 0 + ) + actual_group_owners_to_remove = ( + len(group_owner_ids & {group.id for group in existing_group_owners}) + if group_owner_ids + else 0 + ) remaining = ( - feature.owners.count() + len(existing_owners) - actual_owners_to_remove - + feature.group_owners.count() + len(existing_group_owners) - actual_group_owners_to_remove )
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 273abbe0-f552-414e-96f8-f9259e4c2df0
📒 Files selected for processing (2)
api/features/views.pyapi/tests/unit/features/test_unit_features_views.py
💤 Files with no reviewable changes (1)
- api/tests/unit/features/test_unit_features_views.py
7640828 to
4bfe95b
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
api/features/views.py (1)
490-508: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUse prefetched relations to avoid additional database queries.
Since both
ownersandgroup_ownersare already prefetched in theget_querysetmethod, using.values_list("id", flat=True)and.count()will ignore the prefetch cache and trigger four additional database queries. You can iterate over the prefetched.all()objects and uselen()to perform these checks efficiently in memory.⚡ Proposed refactor to utilise the prefetch cache
- actual_owners_to_remove = ( - len(owner_ids & set(feature.owners.values_list("id", flat=True))) - if owner_ids - else 0 - ) - actual_group_owners_to_remove = ( - len( - group_owner_ids & set(feature.group_owners.values_list("id", flat=True)) - ) - if group_owner_ids - else 0 - ) - - remaining = ( - feature.owners.count() - - actual_owners_to_remove - + feature.group_owners.count() - - actual_group_owners_to_remove - ) + existing_owners = feature.owners.all() + existing_group_owners = feature.group_owners.all() + + actual_owners_to_remove = ( + len(owner_ids & {owner.id for owner in existing_owners}) + if owner_ids + else 0 + ) + actual_group_owners_to_remove = ( + len(group_owner_ids & {group.id for group in existing_group_owners}) + if group_owner_ids + else 0 + ) + + remaining = ( + len(existing_owners) + - actual_owners_to_remove + + len(existing_group_owners) + - actual_group_owners_to_remove + )
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2b8ad9c9-ad39-42bb-bcdc-e9a9e9c3fcf9
📒 Files selected for processing (2)
api/features/views.pyapi/tests/unit/features/test_unit_features_views.py
cc30a3e to
bc18412
Compare
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/tests/unit/features/test_unit_features_views.py (2)
5431-5432: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse an existing, non-owner group for consistency and robustness.
While
FeatureGroupOwnerInputSerializerdoes not currently validate group existence likeUserIdsSerializerdoes, using a hardcoded non-existent group ID (888888) is brittle and inconsistent. Create and use a real group that is not an owner to accurately simulate a stale ID being removed.
5366-5367: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winTests use non-existent IDs that fail at the serializer layer or cause brittle assertions.
Using hardcoded non-existent IDs (like
999999or888888) does not correctly test the new_validate_owner_removallogic. For users,UserIdsSerializer.validatestrictly checks database existence and immediately returns400 Bad Request("Some users not found"). This means one test passes for the wrong reason and another will fail outright, as the view logic is never reached. To correctly simulate "stale" removals, create real entities and pass their IDs without adding them as feature owners (and optionally update the test names to reflect this).
api/tests/unit/features/test_unit_features_views.py#L5366-L5367: Create a realnon_owner_userand use their ID here so the400 Bad Requestis legitimately triggered by_validate_owner_removalinstead of the serializer.api/tests/unit/features/test_unit_features_views.py#L5396-L5397: Use thenon_owner_userID here to bypass serializer validation and ensure the test receives the expected200 OK.api/tests/unit/features/test_unit_features_views.py#L5431-L5432: Create a realnon_owner_groupand use its ID here for consistency and to prevent future serializer changes from breaking the test.🛠️ Proposed fixes
For lines 5366-5367:
- # Request removes the real owner AND a non-existent user - data = {"user_ids": [admin_user.id, 999999]} + # Request removes the real owner AND a user who is not an owner + non_owner = FFAdminUser.objects.create_user(email="nonowner1@example.com") # type: ignore[no-untyped-call] + data = {"user_ids": [admin_user.id, non_owner.id]}For lines 5396-5397:
- # Request removes one real owner and one non-existent user - data = {"user_ids": [admin_user.id, 999999]} + # Request removes one real owner and a user who is not an owner + non_owner = FFAdminUser.objects.create_user(email="nonowner2@example.com") # type: ignore[no-untyped-call] + data = {"user_ids": [admin_user.id, non_owner.id]}For lines 5431-5432:
- # Request removes the real group and a non-existent group - data = {"group_ids": [group.id, 888888]} + # Request removes the real group and a group that is not an owner + non_owner_group = UserPermissionGroup.objects.create(name="Non Owner Group", organisation=organisation) + data = {"group_ids": [group.id, non_owner_group.id]}
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1d0111d9-7c02-4a7a-b3c7-7e8fc9c5191d
📒 Files selected for processing (2)
api/features/views.pyapi/tests/unit/features/test_unit_features_views.py
a0bd65e to
c95ba38
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8032 +/- ##
==========================================
- Coverage 98.63% 98.50% -0.14%
==========================================
Files 1496 1506 +10
Lines 59072 59589 +517
==========================================
+ Hits 58266 58696 +430
- Misses 806 893 +87 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
d6811b2 to
cf02027
Compare
When enforce_feature_owners is enabled, _validate_owner_removal was using the count of IDs provided in the request rather than the count of IDs that actually exist as owners. This caused false rejections when non-existent IDs were included in the group owner removal request. Changed the method to accept ID sets and filter against actual existing owners before counting. Added test cases covering the bug scenario.
a862bba to
03a14eb
Compare
for more information, see https://pre-commit.ci
|
@emyller Hi! This PR is ready for your review. All CI checks pass. The fix addresses a validation count mismatch in _validate_owner_removal for group owners. Thanks! |
emyller
left a comment
emyller
left a comment
There was a problem hiding this comment.
The changes look sound, thanks for this contribution!
I have a few comments to improve overall quality. I'm sorry we're still improving such practices ourselves.
Also: is this work related to an existing issue in GitHub? We'd appreciate if you took a minute to read and follow our contributing guide. Thanks again!
There was a problem hiding this comment.
I understand the code upstream suggests count arithmetic but I believe we could improve this twofold:
- By only fetching IDs from the database
- By using set arithmetic as a lever
| existing_owner_ids = set(feature.owners.values_list("id", flat=True)) | |
| existing_group_owner_ids = set(feature.group_owners.values_list("id", flat=True)) | |
| if not ( | |
| (existing_owner_ids - owner_ids) | |
| or (existing_group_owner_ids - group_owner_ids) | |
| ): | |
| raise serializers.ValidationError( | |
| "This project requires at least one owner or group owner per feature." | |
| ) |
| feature: Feature, | ||
| admin_user: FFAdminUser, | ||
| ) -> None: | ||
| # Given — feature has only one owner, enforce_feature_owners is on |
There was a problem hiding this comment.
Please avoid adding these comments. If code needs clarification, please do so in better naming, unless it can't help. I think none of the code you're adding to tests needs clarification.
| url = reverse( | ||
| "api-v1:projects:project-features-remove-owners", | ||
| args=[project.id, feature.id], | ||
| ) | ||
| # Request removes the real owner AND a user who is not an owner | ||
| data = {"user_ids": [admin_user.id, non_owner.id]} | ||
|
|
||
| # When | ||
| response = admin_client_new.post( | ||
| url, data=json.dumps(data), content_type="application/json" | ||
| ) |
There was a problem hiding this comment.
Please inline the real URL and the payload to the client call. This improves reading tests, and exercises the URL itself — e.g. reverse could help perpetuating a typo in the API spec, because the actual URL isn't exercised.
| url = reverse( | |
| "api-v1:projects:project-features-remove-owners", | |
| args=[project.id, feature.id], | |
| ) | |
| # Request removes the real owner AND a user who is not an owner | |
| data = {"user_ids": [admin_user.id, non_owner.id]} | |
| # When | |
| response = admin_client_new.post( | |
| url, data=json.dumps(data), content_type="application/json" | |
| ) | |
| # When | |
| response = admin_client_new.post( | |
| f"/the/real/url/{project.id}", | |
| data={"user_ids": [admin_user.id, non_owner.id]}, | |
| format="json", | |
| ) |
|
Hi @emyller, thanks for the thorough review! I've addressed all your feedback: Changes made:
Regarding the issue: I searched the existing issues but couldn't find one related to this bug. Would you like me to create a new issue first, or is it fine to proceed without one for this validation fix? |
Use values_list('id', flat=True) to fetch only IDs instead of full objects,
and use set difference to check remaining owners instead of counting.
Also update new tests to use inline URLs and format='json' per review
feedback.
dd5974e to
d08d9fb
Compare
|
URL verification: Confirmed the inline URLs match the actual route definitions: |
2 participants
Problem
When
enforce_feature_ownersis enabled, the_validate_owner_removalmethod validates whether removing feature owners would leave the feature with zero owners. It uses the count of IDs in the request rather than the count of IDs that actually exist as owners on the feature.The group owner serializer (
FeatureGroupOwnerInputSerializer) does not validate that group IDs exist before processing, so non-existent group IDs pass through to_validate_owner_removaland inflate the removal count. This causes the validation to incorrectly block legitimate operations.Note: The user owner serializer (
UserIdsSerializer) already validates all IDs exist before the view runs, so this bug is only reachable via the group owner endpoint.How to reproduce
enforce_feature_owners = TruePOST /api/v1/projects/{id}/features/{id}/remove-group-owners/with{"group_ids": [group_id, 999999]}1 + 1 - 2 = 0and rejectsWhen does this happen in practice?
Solution
Filter requested IDs against actual existing owners before counting.
Files changed
api/features/views.py-_validate_owner_removalnow acceptsowner_ids: set[int]andgroup_owner_ids: set[int]instead of plain counts. Both callers (remove_owners,remove_group_owners) updated accordingly.api/tests/unit/features/test_unit_features_views.py- Added 2 test cases for the non-existent group ID scenarios.Performance
Only queries existing IDs when the corresponding input set is non-empty, avoiding unnecessary DB calls.
Scope
This PR fixes the validation count mismatch for group owners. The user owner path is already safe because
UserIdsSerializervalidates IDs before the view. The behavior where M2Mremove()silently ignores non-existent IDs is by design in Django and is a separate concern.