Stop creating duplicated roles when description is changed - #8391
Stop creating duplicated roles when description is changed#8391melton-jason wants to merge 8 commits into
Conversation
|
Warning One or more dependencies are approaching or past End-of-Life. |
|
Warning Review limit reached
Next review available in: 56 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughRole initialization now uses declarative library-role definitions, shared user-type mappings, case-insensitive role lookup, bulk policy creation, and shared handling for library and collection roles. ChangesRole initialization
Sequence Diagram(s)sequenceDiagram
participant create_roles
participant create_missing_library_roles
participant _create_role_and_policies
participant Role
participant RolePolicy
create_roles->>create_missing_library_roles: initialize configured library roles
create_missing_library_roles->>Role: case-insensitive role lookup
create_missing_library_roles->>_create_role_and_policies: create missing role
_create_role_and_policies->>Role: create or reuse role
_create_role_and_policies->>RolePolicy: bulk-create policies
create_roles->>_create_role_and_policies: create collection roles
🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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: 3
🧹 Nitpick comments (4)
specifyweb/backend/permissions/initialize.py (4)
391-391: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace the mutable default argument.
Ruff flags
role_filters: dict = dict()(B006). The dictionary is not mutated today, but a default ofNoneremoves the hazard. Please also add a return type annotation.♻️ Proposed fix
-def _create_role_and_policies(role_model, role_policy_model, role_name: str, role_filters: dict = dict()): +def _create_role_and_policies(role_model, role_policy_model, role_name: str, role_filters: Optional[dict] = None): + role_filters = role_filters or {}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@specifyweb/backend/permissions/initialize.py` at line 391, Update _create_role_and_policies to use None instead of the mutable dict() default for role_filters, normalize it to an empty dictionary inside the function before use, and add an appropriate return type annotation.Source: Linters/SAST tools
450-476: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the redundant
.get()fallbacks.
_USERTYPES_TO_ROLE_NAMESdefinesGuest,FullAccess, andManagerat Lines 83-88, so the fallback values never apply. They also hide a real failure: if a mapping key is ever renamed, the fallback name is looked up inLIBRARY_ROLESand raisesKeyErrorinside_create_role_and_policies. Use direct subscripts for a clearer failure point.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@specifyweb/backend/permissions/initialize.py` around lines 450 - 476, The role creation loop should use direct subscripting for the Guest, FullAccess, and Manager entries in _USERTYPES_TO_ROLE_NAMES instead of .get() fallbacks. Update each _create_role_and_policies call while preserving the existing conditional behavior and role creation flow.
445-448: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winMaterialize
user_typesonce.
user_typesis a lazy queryset. Eachintest on Lines 447-448 runs a separateSELECT DISTINCT. Wrap the query in asetto run it once.♻️ Proposed fix
- user_types = Specifyuser.objects.all().values_list("usertype", flat=True).distinct() + user_types = set(Specifyuser.objects.values_list("usertype", flat=True).distinct())🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@specifyweb/backend/permissions/initialize.py` around lines 445 - 448, Materialize the lazy queryset assigned to user_types by wrapping the distinct values_list result in a set, so the subsequent has_guest and has_full_access membership checks reuse one fetched collection and issue only one query.
133-156: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider sourcing the descriptions from
LIBRARY_ROLES.The descriptions in
ROLE_DESCRIPTIONS(Lines 97-102) duplicate the descriptions inLIBRARY_ROLES(Lines 346-388). If the two copies drift, a newly created collection role gets a different description than the same role created bycreate_roles. Resolve the description through_USERTYPES_TO_ROLE_NAMESandLIBRARY_ROLESinstead.Also, the fallback
f"{user_type} - {collection_name}"on Line 136 is unreachable because Line 133 already rejects unmapped user types. A plain subscript is clearer.♻️ Proposed simplification
- role_name = _USERTYPES_TO_ROLE_NAMES.get(user_type, f"{user_type} - {collection_name}") - role_description = ROLE_DESCRIPTIONS.get(user_type, "No description available.") + role_name = _USERTYPES_TO_ROLE_NAMES[user_type] + role_description = LIBRARY_ROLES[role_name]["description"]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@specifyweb/backend/permissions/initialize.py` around lines 133 - 156, Update the role initialization block to use a direct subscript on _USERTYPES_TO_ROLE_NAMES for role_name, removing the unreachable fallback. Resolve role_description from the corresponding role entry in LIBRARY_ROLES using the mapped role name, and stop using the duplicated ROLE_DESCRIPTIONS mapping so descriptions remain consistent with create_roles.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@specifyweb/backend/permissions/initialize.py`:
- Around line 421-427: Update the role lookup before missing_roles so the __in
operand is a materialized list of lowercased all_roles values, and normalize the
returned existing_role_names to lowercase before computing the set difference.
Ensure missing_roles compares normalized names case-insensitively while
preserving the existing role creation flow.
- Around line 403-414: Update the role policy creation comprehension in the
role-initialization function to iterate directly over the resource/action
mapping in resolved_role["policies"], producing each resource and action without
calling items() on individual resource strings. Also return the created role
after bulk_create, matching the existing reuse path so every execution returns
role.
- Around line 393-401: Update role creation in create_roles to avoid
get_or_create, which fails when duplicate matching roles exist. Query matching
roles ordered by the earliest record, reuse the first role when present, and
create a new role with the existing defaults only when none match, preserving
the current role_filters and return behavior.
---
Nitpick comments:
In `@specifyweb/backend/permissions/initialize.py`:
- Line 391: Update _create_role_and_policies to use None instead of the mutable
dict() default for role_filters, normalize it to an empty dictionary inside the
function before use, and add an appropriate return type annotation.
- Around line 450-476: The role creation loop should use direct subscripting for
the Guest, FullAccess, and Manager entries in _USERTYPES_TO_ROLE_NAMES instead
of .get() fallbacks. Update each _create_role_and_policies call while preserving
the existing conditional behavior and role creation flow.
- Around line 445-448: Materialize the lazy queryset assigned to user_types by
wrapping the distinct values_list result in a set, so the subsequent has_guest
and has_full_access membership checks reuse one fetched collection and issue
only one query.
- Around line 133-156: Update the role initialization block to use a direct
subscript on _USERTYPES_TO_ROLE_NAMES for role_name, removing the unreachable
fallback. Resolve role_description from the corresponding role entry in
LIBRARY_ROLES using the mapped role name, and stop using the duplicated
ROLE_DESCRIPTIONS mapping so descriptions remain consistent with create_roles.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8f57a289-3f49-4bc6-ae0b-3c6b360ae4a1
📒 Files selected for processing (1)
specifyweb/backend/permissions/initialize.py
github-project-automation
Bot
moved this from 📋Back Log
to Dev Attention Needed
in General Tester Board
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
♻️ Duplicate comments (2)
specifyweb/backend/permissions/initialize.py (2)
396-406: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
get_or_createstill fails withMultipleObjectsReturnedon pre-existing duplicates.This diff only adds the
DefaultRoletype annotation at line 397. Theget_or_createcall at lines 398-404 is unchanged.get_or_createcallsget()first, so a collection that already holds two rows matchingrole_nameandrole_filtersraisesMultipleObjectsReturned. Since this PR's objective is to fix duplicate default roles including cases where duplicates already exist, this call will crashrun_key_migration_functionson any database that already has the duplication this PR is meant to resolve.assign_users_to_roles(Lines 147-150) already uses the safer pattern of filtering, ordering bypk, and taking the first match.🐛 Proposed fix
resolved_role: DefaultRole = LIBRARY_ROLES[role_name] - role, is_new = role_model.objects.get_or_create( - name=role_name, - **role_filters, - defaults={ - "description": resolved_role["description"] - } - ) - if not is_new: - return role + role = role_model.objects.filter( + name=role_name, + **role_filters + ).order_by("pk").first() + if role is not None: + return role + + role = role_model.objects.create( + name=role_name, + description=resolved_role["description"], + **role_filters + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@specifyweb/backend/permissions/initialize.py` around lines 396 - 406, Update _create_role_and_policies to avoid get_or_create when matching duplicate roles may already exist: filter by role_name and role_filters, order by pk, and reuse the first matching role. Only create a new role with the resolved description when no match exists, preserving the existing return behavior and aligning with assign_users_to_roles.
408-418: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winPolicy iteration is fixed, but the "newly created role" branch still returns
Noneimplicitly.Line 415 now correctly iterates
resolved_role["policies"].items(), fixing theAttributeErrorflagged in the previous review. However, afterbulk_createruns, the function has no explicitreturn rolefor the branch where a role is newly created. The function returnsroleonly via the early return at line 406 when reusing an existing role; the "created" path falls through and returnsNone. CodeQL also flags this at line 396 ("Explicit returns mixed with implicit (fall through) returns"). Any caller increate_rolesthat expects aRoleobject back from this helper after creating a new role receivesNoneinstead.🐛 Proposed fix
role_policy_model.objects.bulk_create( [ role_policy_model( role=role, resource=resource, action=action ) for resource, actions in resolved_role["policies"].items() for action in actions ] ) + return role🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@specifyweb/backend/permissions/initialize.py` around lines 408 - 418, Update the newly created role path in the role-creation helper to explicitly return role after bulk_create completes. Preserve the existing early return for reused roles and ensure every branch returns the Role object expected by create_roles, eliminating the implicit fall-through.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@specifyweb/backend/permissions/initialize.py`:
- Around line 396-406: Update _create_role_and_policies to avoid get_or_create
when matching duplicate roles may already exist: filter by role_name and
role_filters, order by pk, and reuse the first matching role. Only create a new
role with the resolved description when no match exists, preserving the existing
return behavior and aligning with assign_users_to_roles.
- Around line 408-418: Update the newly created role path in the role-creation
helper to explicitly return role after bulk_create completes. Preserve the
existing early return for reused roles and ensure every branch returns the Role
object expected by create_roles, eliminating the implicit fall-through.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9d571de9-3250-407a-9fc0-ce22aba16809
📒 Files selected for processing (1)
specifyweb/backend/permissions/initialize.py
2 participants
Fixes #8390
A more ideal solution would probably be to have a uniqueness rule defined for:
SpRole must have unique Name in Collection, and/or ever better a constraint at the database level to enforce uniqueness.Checklist
self-explanatory (or properly documented)
specify7/specifyweb/specify/management/commands/run_key_migration_functions.py
Line 50 in ea04665
Testing instructions
Read Only - Legacy,Full Access - Legacy, orCollection AdminRolesrun_key_migration_functionscommandSummary by CodeRabbit
Bug Fixes
Refactor