[Draft] Optimize role creation signal with schema version tracking #1097
Conversation
This change dramatically reduces database overhead during migrations by implementing smart schema version tracking for role permissions. Key improvements: - Add RoleSchemaVersion model to track permission schema changes via MD5 hash - Update create_roles() signal to only run when schema actually changes - Optimize create_roles_for_project() with bulk database operations - Add update_roles management command for manual control Performance impact: - Normal migrations: 99.9% reduction in overhead (1-2 queries vs ~21,500 for 100 projects) - Schema change migrations: 14x faster via bulk operations (~1,500 vs ~21,500 operations for 100 projects) - New projects: Unchanged, still automatic via post_save signal Technical details: - Schema version is MD5 hash of all Role class names and permissions - Migration creates RoleSchemaVersion table and performs initial update - Bulk operations: Permission.bulk_create(), group.permissions.set(), GroupObjectPermission.bulk_create() - Backwards compatible: All existing functionality preserved Files modified: - ami/users/models.py: Add RoleSchemaVersion model with version tracking - ami/users/roles.py:197: Optimize create_roles_for_project() with bulk operations - ami/users/signals.py:14: Add conditional schema version check to create_roles() - ami/users/management/commands/update_roles.py: New management command - ami/users/migrations/0004_roleschemaversion.py: New migration - CLAUDE.md: Add Role Permission Management documentation section Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
✅ Deploy Preview for antenna-preview canceled.
|
✅ Deploy Preview for antenna-ssec canceled.
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughThe changes introduce role schema version tracking to the users app. A new RoleSchemaVersion model persists schema versions, a Django management command enables manual per-project or global role updates with dry-run support, and role creation logic now supports forced updates with bulk permission operations. Signals trigger updates only when schema changes are detected. Changes
Sequence Diagram(s)sequenceDiagram
participant Admin as Admin/CLI
participant Cmd as Management<br/>Command
participant RoleVer as RoleSchemaVersion
participant Project as Project
participant RoleFunc as create_roles_<br/>for_project()
participant DB as Database
Admin->>Cmd: Run update_roles (--dry-run/--force)
Cmd->>Project: Query target projects
Project-->>Cmd: Project list
loop For each project
Cmd->>RoleFunc: Call with force_update flag
RoleFunc->>DB: Fetch Permission codenames
RoleFunc->>DB: Create missing Permissions (bulk)
RoleFunc->>DB: Update Group.permissions
RoleFunc->>DB: Update Guardian object permissions (bulk)
DB-->>RoleFunc: Success
RoleFunc-->>Cmd: Complete
end
alt Not dry-run
Cmd->>RoleVer: mark_updated()
RoleVer->>DB: Create RoleSchemaVersion record
DB-->>RoleVer: Persisted
RoleVer-->>Cmd: Updated
end
Cmd->>Cmd: Print summary (updated/failed counts)
Cmd-->>Admin: Done
sequenceDiagram
participant Migration as Post-Migrate<br/>Signal
participant RoleVer as RoleSchemaVersion
participant RoleFunc as create_roles_<br/>for_project()
participant Project as Project
participant DB as Database
Migration->>RoleVer: needs_update()?
RoleVer->>DB: Compare get_current_version() with latest
DB-->>RoleVer: True/False
alt Schema needs update
RoleVer-->>Migration: True
Migration->>Project: Query all projects
Project-->>Migration: Project list
loop For each project
Migration->>RoleFunc: Call with force_update=True
RoleFunc->>DB: Update roles/permissions/groups
DB-->>RoleFunc: Success
end
Migration->>RoleVer: mark_updated()
RoleVer->>DB: Persist version
DB-->>RoleVer: Recorded
else No update needed
RoleVer-->>Migration: False
Migration->>Migration: Exit (log skip)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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
🤖 Fix all issues with AI agents
In `@ami/users/models.py`:
- Around line 89-93: The mark_updated classmethod can raise IntegrityError
because version is unique; change it to atomically ensure a single row per
version by using cls.objects.get_or_create(version=current,
defaults={'description': description}) after computing current via
cls.get_current_version(), or wrap the cls.objects.create call in a transaction
and catch IntegrityError to ignore duplicate inserts; update mark_updated to use
cls.objects.get_or_create(...) (or handle IntegrityError) so concurrent calls
for the same version do not crash.
In `@ami/users/roles.py`:
- Around line 250-260: The delete-then-bulk_create sequence for
GroupObjectPermission can leave permissions missing if bulk_create fails; wrap
the critical section in an atomic transaction (using transaction.atomic) around
the deletion and bulk_create so they succeed or roll back together, e.g., in the
function that builds group_obj_perms and calls
GroupObjectPermission.objects.filter(...).delete() and
GroupObjectPermission.objects.bulk_create(group_obj_perms); ensure to import
django.db.transaction and apply the atomic context there (or add a
savepoint/atomic in the signal handler that invokes this code) so the operation
is executed as one transactional unit.
In `@ami/users/signals.py`:
- Around line 43-49: The code currently calls RoleSchemaVersion.mark_updated()
unconditionally even if some projects failed; change the migration flow in the
function handling the project loop to track failures (e.g., a failed_projects
list or a boolean flag updated_failed) while iterating projects, and only call
RoleSchemaVersion.mark_updated(description="Post-migration role update") when no
failures were recorded; if failures exist, log the failed project ids via
logger.error/info and skip calling mark_updated (or re-raise/return a non-zero
status) so failed projects can be retried or handled manually. Ensure you
reference the existing logger and RoleSchemaVersion.mark_updated symbols and
preserve the existing exception logging behavior for per-project errors.
🧹 Nitpick comments (3)
ami/users/models.py (1)
78-87: Broad exception catch is intentional but could mask unexpected errors.The
except Exceptionis designed to handle the case where the table doesn't exist during initial migration. However, catching all exceptions may mask other database errors (e.g., connection issues). Consider catchingProgrammingErrororOperationalErrorspecifically.♻️ Suggested improvement
`@classmethod` def needs_update(cls): """Check if roles need updating based on schema version.""" current = cls.get_current_version() try: latest = cls.objects.first() return latest is None or latest.version != current - except Exception: + except (ProgrammingError, OperationalError): # Table doesn't exist yet (first migration) return FalseAdd the import at the top:
from django.db import ProgrammingError, OperationalErrorami/users/signals.py (1)
48-49: Uselogger.exceptionto include traceback.As flagged by static analysis (TRY400),
logger.exceptionshould be used instead oflogger.errorwhen logging within an exception handler to include the full traceback.♻️ Proposed fix
except Exception as e: - logger.error(f"Failed to create roles during migration: {e}") + logger.exception(f"Failed to create roles during migration: {e}")ami/users/management/commands/update_roles.py (1)
13-32: The--forceflag defaults toTrue, making it effectively a no-op.Since
--forcehasdefault=True, the flag has no effect unless explicitly set to--no-force(which isn't defined). If the intent is to always force updates by default, consider removing the flag or changing it to--no-force/--skip-existingto allow opting out.♻️ Option A: Remove redundant flag
- parser.add_argument( - "--force", - action="store_true", - default=True, - help="Force update even if groups already exist (default: True)", - )And update handle:
- force = options.get("force", True) + force = True # Always force updates in management command♻️ Option B: Add --skip-existing to opt out
parser.add_argument( - "--force", - action="store_true", - default=True, - help="Force update even if groups already exist (default: True)", + "--skip-existing", + action="store_true", + help="Skip groups that already exist (default: update all)", )- force = options.get("force", True) + force = not options.get("skip_existing", False)
| @classmethod | ||
| def mark_updated(cls, description="Schema updated"): | ||
| """Mark schema as updated to current version.""" | ||
| current = cls.get_current_version() | ||
| cls.objects.create(version=current, description=description) |
There was a problem hiding this comment.
Potential IntegrityError if mark_updated is called concurrently with same version.
The version field has a unique=True constraint. If mark_updated is called twice with the same schema version (e.g., concurrent migrations or re-runs), it will raise an IntegrityError. Consider using get_or_create or handling the conflict.
🐛 Proposed fix
`@classmethod`
def mark_updated(cls, description="Schema updated"):
"""Mark schema as updated to current version."""
current = cls.get_current_version()
- cls.objects.create(version=current, description=description)
+ cls.objects.get_or_create(
+ version=current,
+ defaults={"description": description}
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @classmethod | |
| def mark_updated(cls, description="Schema updated"): | |
| """Mark schema as updated to current version.""" | |
| current = cls.get_current_version() | |
| cls.objects.create(version=current, description=description) | |
| `@classmethod` | |
| def mark_updated(cls, description="Schema updated"): | |
| """Mark schema as updated to current version.""" | |
| current = cls.get_current_version() | |
| cls.objects.get_or_create( | |
| version=current, | |
| defaults={"description": description} | |
| ) |
🤖 Prompt for AI Agents
In `@ami/users/models.py` around lines 89 - 93, The mark_updated classmethod can
raise IntegrityError because version is unique; change it to atomically ensure a
single row per version by using cls.objects.get_or_create(version=current,
defaults={'description': description}) after computing current via
cls.get_current_version(), or wrap the cls.objects.create call in a transaction
and catch IntegrityError to ignore duplicate inserts; update mark_updated to use
cls.objects.get_or_create(...) (or handle IntegrityError) so concurrent calls
for the same version do not crash.
| # Bulk update Guardian object permissions | ||
| # Remove all existing, then bulk create new ones | ||
| GroupObjectPermission.objects.filter(group=group, content_type=project_ct, object_pk=project.pk).delete() | ||
|
|
||
| group_obj_perms = [ | ||
| GroupObjectPermission( | ||
| group=group, permission=existing_perms[codename], content_type=project_ct, object_pk=project.pk | ||
| ) | ||
| for codename in permissions | ||
| ] | ||
| GroupObjectPermission.objects.bulk_create(group_obj_perms) |
There was a problem hiding this comment.
Delete-then-bulk-create pattern could leave inconsistent state on failure.
The delete operation at line 252 followed by bulk_create at line 260 is not atomic within this function. If bulk_create fails (e.g., database error), the existing permissions will already be deleted, leaving the group without object permissions.
The caller (signals.py and management command) wraps this in a transaction for the management command, but the signal handler processes each project in a separate iteration without transaction wrapping around the delete+create.
🐛 Consider wrapping in a savepoint or documenting caller responsibility
+ # Note: Caller should wrap in transaction.atomic() for consistency
# Bulk update Guardian object permissions
# Remove all existing, then bulk create new ones
GroupObjectPermission.objects.filter(group=group, content_type=project_ct, object_pk=project.pk).delete()
group_obj_perms = [
GroupObjectPermission(
group=group, permission=existing_perms[codename], content_type=project_ct, object_pk=project.pk
)
for codename in permissions
]
GroupObjectPermission.objects.bulk_create(group_obj_perms)Alternatively, wrap the critical section in a savepoint:
from django.db import transaction
with transaction.atomic():
GroupObjectPermission.objects.filter(...).delete()
GroupObjectPermission.objects.bulk_create(group_obj_perms)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Bulk update Guardian object permissions | |
| # Remove all existing, then bulk create new ones | |
| GroupObjectPermission.objects.filter(group=group, content_type=project_ct, object_pk=project.pk).delete() | |
| group_obj_perms = [ | |
| GroupObjectPermission( | |
| group=group, permission=existing_perms[codename], content_type=project_ct, object_pk=project.pk | |
| ) | |
| for codename in permissions | |
| ] | |
| GroupObjectPermission.objects.bulk_create(group_obj_perms) | |
| # Note: Caller should wrap in transaction.atomic() for consistency | |
| # Bulk update Guardian object permissions | |
| # Remove all existing, then bulk create new ones | |
| GroupObjectPermission.objects.filter(group=group, content_type=project_ct, object_pk=project.pk).delete() | |
| group_obj_perms = [ | |
| GroupObjectPermission( | |
| group=group, permission=existing_perms[codename], content_type=project_ct, object_pk=project.pk | |
| ) | |
| for codename in permissions | |
| ] | |
| GroupObjectPermission.objects.bulk_create(group_obj_perms) |
🤖 Prompt for AI Agents
In `@ami/users/roles.py` around lines 250 - 260, The delete-then-bulk_create
sequence for GroupObjectPermission can leave permissions missing if bulk_create
fails; wrap the critical section in an atomic transaction (using
transaction.atomic) around the deletion and bulk_create so they succeed or roll
back together, e.g., in the function that builds group_obj_perms and calls
GroupObjectPermission.objects.filter(...).delete() and
GroupObjectPermission.objects.bulk_create(group_obj_perms); ensure to import
django.db.transaction and apply the atomic context there (or add a
savepoint/atomic in the signal handler that invokes this code) so the operation
is executed as one transactional unit.
|
|
||
| # Mark schema as updated | ||
| RoleSchemaVersion.mark_updated(description="Post-migration role update") | ||
| logger.info(f"Successfully updated roles for {project_count} projects") | ||
|
|
||
| except Exception as e: | ||
| logger.warning( | ||
| f"Failed to create roles during migration: {e}. This can be run manually via management command." | ||
| ) | ||
| logger.error(f"Failed to create roles during migration: {e}") |
There was a problem hiding this comment.
Schema marked as updated even when some projects fail.
If some projects fail during the loop (line 40-41), mark_updated is still called at line 45. This means subsequent migrations won't retry the failed projects. Consider either:
- Not marking as updated if any failures occurred
- Or documenting that failed projects require manual intervention via the management command
🐛 Suggested approach
+ failed_count = 0
for project in Project.objects.all():
try:
create_roles_for_project(project, force_update=True)
except Exception as e:
logger.warning(f"Failed to create roles for project {project.pk} ({project.name}): {e}")
+ failed_count += 1
continue
- # Mark schema as updated
- RoleSchemaVersion.mark_updated(description="Post-migration role update")
- logger.info(f"Successfully updated roles for {project_count} projects")
+ # Mark schema as updated only if all projects succeeded
+ if failed_count == 0:
+ RoleSchemaVersion.mark_updated(description="Post-migration role update")
+ logger.info(f"Successfully updated roles for {project_count} projects")
+ else:
+ logger.warning(
+ f"Roles updated for {project_count - failed_count}/{project_count} projects. "
+ f"Run 'manage.py update_roles' to retry failed projects."
+ )🧰 Tools
🪛 Ruff (0.14.13)
48-48: Do not catch blind exception: Exception
(BLE001)
49-49: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
🤖 Prompt for AI Agents
In `@ami/users/signals.py` around lines 43 - 49, The code currently calls
RoleSchemaVersion.mark_updated() unconditionally even if some projects failed;
change the migration flow in the function handling the project loop to track
failures (e.g., a failed_projects list or a boolean flag updated_failed) while
iterating projects, and only call
RoleSchemaVersion.mark_updated(description="Post-migration role update") when no
failures were recorded; if failures exist, log the failed project ids via
logger.error/info and skip calling mark_updated (or re-raise/return a non-zero
status) so failed projects can be retried or handled manually. Ensure you
reference the existing logger and RoleSchemaVersion.mark_updated symbols and
preserve the existing exception logging behavior for per-project errors.
There was a problem hiding this comment.
Pull request overview
This PR optimizes role permission management during Django migrations by introducing schema version tracking to avoid unnecessary database operations. Instead of updating roles for all projects on every migration, the system now only performs updates when role permission schemas actually change.
Changes:
- Adds RoleSchemaVersion model with MD5-based schema tracking to detect when role permissions change
- Optimizes create_roles_for_project() function with bulk database operations and conditional updates based on force_update parameter
- Updates create_roles() signal to check schema version before processing projects and marks schema as updated after completion
- Adds update_roles management command for manual role updates with options for specific projects, dry-run mode, and forced updates
Reviewed changes
Copilot reviewed 5 out of 7 changed files in this pull request and generated 18 comments.
Show a summary per file
| File | Description |
|---|---|
| ami/users/models.py | Adds RoleSchemaVersion model with version tracking methods |
| ami/users/roles.py | Optimizes create_roles_for_project() with bulk operations and adds force_update parameter |
| ami/users/signals.py | Updates create_roles() to conditionally execute based on schema version changes |
| ami/users/management/commands/update_roles.py | New management command for manual role updates |
| ami/users/migrations/0004_roleschemaversion.py | Creates RoleSchemaVersion table |
| ami/users/management/commands/init.py | Empty init file for management commands module |
| ami/users/management/init.py | Empty init file for management module |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Create any missing permissions | ||
| missing_perms = [] | ||
| for codename in all_perm_codenames: | ||
| if codename not in existing_perms: | ||
| missing_perms.append( | ||
| Permission(codename=codename, content_type=project_ct, name=f"Can {codename.replace('_', ' ')}") | ||
| ) | ||
|
|
||
| if missing_perms: | ||
| Permission.objects.bulk_create(missing_perms, ignore_conflicts=True) | ||
| # Refresh existing_perms dict after bulk create | ||
| existing_perms = { | ||
| perm.codename: perm | ||
| for perm in Permission.objects.filter(codename__in=all_perm_codenames, content_type=project_ct) | ||
| } | ||
|
|
There was a problem hiding this comment.
When creating missing permissions with bulk_create, if there's a collision due to ignore_conflicts=True, the newly created permission objects won't have their IDs set. This could cause a KeyError on line 256 when trying to access existing_perms[codename] if the permission was created by another concurrent process but not yet in the refreshed existing_perms dict. Consider using get_or_create for each missing permission, or handle potential KeyErrors when accessing existing_perms.
| # Create any missing permissions | |
| missing_perms = [] | |
| for codename in all_perm_codenames: | |
| if codename not in existing_perms: | |
| missing_perms.append( | |
| Permission(codename=codename, content_type=project_ct, name=f"Can {codename.replace('_', ' ')}") | |
| ) | |
| if missing_perms: | |
| Permission.objects.bulk_create(missing_perms, ignore_conflicts=True) | |
| # Refresh existing_perms dict after bulk create | |
| existing_perms = { | |
| perm.codename: perm | |
| for perm in Permission.objects.filter(codename__in=all_perm_codenames, content_type=project_ct) | |
| } | |
| # Ensure all required permissions exist, handling concurrency safely | |
| for codename in all_perm_codenames: | |
| if codename not in existing_perms: | |
| perm, _ = Permission.objects.get_or_create( | |
| codename=codename, | |
| content_type=project_ct, | |
| defaults={"name": f"Can {codename.replace('_', ' ')}"}, | |
| ) | |
| existing_perms[codename] = perm |
| group_obj_perms = [ | ||
| GroupObjectPermission( | ||
| group=group, permission=existing_perms[codename], content_type=project_ct, object_pk=project.pk |
There was a problem hiding this comment.
Similar to line 247, if a permission codename is not in existing_perms, this will raise a KeyError when building the GroupObjectPermission objects. This could happen due to race conditions or if the permission creation failed silently.
| if success > 0 and not project_id: | ||
| RoleSchemaVersion.mark_updated(description="Manual update via management command") | ||
| current_version = RoleSchemaVersion.get_current_version() | ||
| self.stdout.write(f"Schema version updated to: {current_version}") |
There was a problem hiding this comment.
When updating a specific project with --project-id, the schema version is not updated (line 85 condition checks "not project_id"). This means if you manually update roles for a specific project when the schema has changed, the schema version won't be marked as updated, and the next migration will still try to update all projects. Consider either always updating the schema version when successful, or adding a warning that single-project updates don't affect schema version tracking.
| self.stdout.write(f"Schema version updated to: {current_version}") | |
| self.stdout.write(f"Schema version updated to: {current_version}") | |
| elif success > 0 and project_id: | |
| # When updating a single project, the global role schema version is not advanced. | |
| # This means future migrations that rely on RoleSchemaVersion may still run for all projects. | |
| self.stdout.write( | |
| self.style.WARNING( | |
| "Roles were updated for the specified project, but the global role schema " | |
| "version was NOT updated because --project-id was used. " | |
| "Run this command without --project-id to advance the global schema version " | |
| "after all projects have been updated." | |
| ) | |
| ) |
| return latest is None or latest.version != current | ||
| except Exception: | ||
| # Table doesn't exist yet (first migration) | ||
| return False |
There was a problem hiding this comment.
When schema version tracking is first introduced, needs_update() will return False during the initial migration because the table doesn't exist yet (line 87 returns False on exception). This means the first migration won't create roles for existing projects. The PR description mentions "Initial migration updates all projects" but the code logic prevents this from happening. The schema update will only run after the table is created and on subsequent deployments when the schema actually changes.
| return False | |
| return True |
| def create_roles_for_project(project, force_update=False): | ||
| """ | ||
| Creates role-based permission groups for a given project. | ||
| Args: | ||
| project: The project to create roles for | ||
| force_update: If False, skip updates for existing groups (default: False) | ||
| If True, always update permissions even if group exists | ||
| """ | ||
| from guardian.models import GroupObjectPermission | ||
|
|
||
| project_ct = ContentType.objects.get_for_model(Project) | ||
|
|
||
| # Pre-fetch all permissions we might need (single query) | ||
| all_perm_codenames = set() | ||
| for role_class in Role.__subclasses__(): | ||
| all_perm_codenames.update(role_class.permissions) | ||
|
|
||
| existing_perms = { | ||
| perm.codename: perm | ||
| for perm in Permission.objects.filter(codename__in=all_perm_codenames, content_type=project_ct) | ||
| } | ||
|
|
||
| # Create any missing permissions | ||
| missing_perms = [] | ||
| for codename in all_perm_codenames: | ||
| if codename not in existing_perms: | ||
| missing_perms.append( | ||
| Permission(codename=codename, content_type=project_ct, name=f"Can {codename.replace('_', ' ')}") | ||
| ) | ||
|
|
||
| if missing_perms: | ||
| Permission.objects.bulk_create(missing_perms, ignore_conflicts=True) | ||
| # Refresh existing_perms dict after bulk create | ||
| existing_perms = { | ||
| perm.codename: perm | ||
| for perm in Permission.objects.filter(codename__in=all_perm_codenames, content_type=project_ct) | ||
| } | ||
|
|
||
| for role_class in Role.__subclasses__(): | ||
| role_name = f"{project.pk}_{project.name}_{role_class.__name__}" | ||
| permissions = role_class.permissions | ||
| group, created = Group.objects.get_or_create(name=role_name) | ||
|
|
||
| if created: | ||
| logger.debug(f"Role created {role_class} for project {project}") | ||
| else: | ||
| # Reset permissions to make sure permissions are updated | ||
| # every time we call this function | ||
| group.permissions.clear() | ||
| assigned_perms = get_perms(group, project) | ||
| for perm_codename in assigned_perms: | ||
| remove_perm(perm_codename, group, project) | ||
| for perm_codename in permissions: | ||
| permission, perm_created = Permission.objects.get_or_create( | ||
| codename=perm_codename, | ||
| content_type=project_ct, | ||
| defaults={"name": f"Can {perm_codename.replace('_', ' ')}"}, | ||
| ) | ||
| elif not force_update: | ||
| # Skip updates for existing groups unless force_update=True | ||
| continue | ||
|
|
||
| group.permissions.add(permission) # Assign the permission group to the project | ||
| assign_perm(perm_codename, group, project) | ||
| # Use set() instead of clear() + add() loop (single query) | ||
| role_perm_objects = [existing_perms[codename] for codename in permissions] | ||
| group.permissions.set(role_perm_objects) | ||
|
|
||
| # Bulk update Guardian object permissions | ||
| # Remove all existing, then bulk create new ones | ||
| GroupObjectPermission.objects.filter(group=group, content_type=project_ct, object_pk=project.pk).delete() | ||
|
|
||
| group_obj_perms = [ | ||
| GroupObjectPermission( | ||
| group=group, permission=existing_perms[codename], content_type=project_ct, object_pk=project.pk | ||
| ) | ||
| for codename in permissions | ||
| ] | ||
| GroupObjectPermission.objects.bulk_create(group_obj_perms) |
There was a problem hiding this comment.
The bulk operations optimization in create_roles_for_project lacks test coverage. Tests should verify that bulk_create works correctly with ignore_conflicts, that permissions are properly assigned to groups, and that the force_update parameter behaves as expected.
| "--force", | ||
| action="store_true", | ||
| default=True, | ||
| help="Force update even if groups already exist (default: True)", |
There was a problem hiding this comment.
The --force argument uses action="store_true" with default=True, which is incorrect. With store_true, the default is False, and passing the flag sets it to True. Having default=True means it's always True regardless of whether the flag is passed. To make force default to True but allow users to disable it, use action="store_false" with dest="force" and default=True, or change the flag to --no-force with store_false action.
| "--force", | |
| action="store_true", | |
| default=True, | |
| help="Force update even if groups already exist (default: True)", | |
| "--no-force", | |
| action="store_false", | |
| dest="force", | |
| default=True, | |
| help="Do not force update if groups already exist (default: force=True)", |
| class RoleSchemaVersion(models.Model): | ||
| """ | ||
| Tracks the current role/permission schema version. | ||
| Updated when Role classes or Project.Permissions change. | ||
| """ | ||
|
|
||
| version = models.CharField(max_length=100, unique=True) | ||
| description = models.TextField() | ||
| updated_at = models.DateTimeField(auto_now=True) | ||
|
|
||
| class Meta: | ||
| ordering = ["-updated_at"] | ||
|
|
||
| def __str__(self): | ||
| return f"RoleSchemaVersion {self.version}" | ||
|
|
||
| @classmethod | ||
| def get_current_version(cls): | ||
| """Get the current schema version from code.""" | ||
| import hashlib | ||
|
|
||
| from ami.users.roles import Role | ||
|
|
||
| role_data = [] | ||
| for role_class in sorted(Role.__subclasses__(), key=lambda r: r.__name__): | ||
| perms = sorted(role_class.permissions) | ||
| role_data.append(f"{role_class.__name__}:{','.join(perms)}") | ||
|
|
||
| schema_str = "|".join(role_data) | ||
| return hashlib.md5(schema_str.encode()).hexdigest()[:16] | ||
|
|
||
| @classmethod | ||
| def needs_update(cls): | ||
| """Check if roles need updating based on schema version.""" | ||
| current = cls.get_current_version() | ||
| try: | ||
| latest = cls.objects.first() | ||
| return latest is None or latest.version != current | ||
| except Exception: | ||
| # Table doesn't exist yet (first migration) | ||
| return False | ||
|
|
||
| @classmethod | ||
| def mark_updated(cls, description="Schema updated"): | ||
| """Mark schema as updated to current version.""" | ||
| current = cls.get_current_version() | ||
| cls.objects.create(version=current, description=description) |
There was a problem hiding this comment.
The new RoleSchemaVersion model and its methods (get_current_version, needs_update, mark_updated) lack test coverage. Given that this is core functionality for the optimization and affects when role updates occur, comprehensive tests should be added to verify version detection, schema change detection, and the update marking behavior.
| def create_roles(sender, **kwargs): | ||
| """Creates predefined roles with specific permissions .""" | ||
| """ | ||
| Creates predefined roles with specific permissions. | ||
| Only runs when role schema version has changed. | ||
| """ | ||
| from ami.users.models import RoleSchemaVersion | ||
|
|
||
| # Quick check - does schema need updating? | ||
| if not RoleSchemaVersion.needs_update(): | ||
| logger.debug("Role schema is up to date, skipping role creation") | ||
| return | ||
|
|
||
| logger.info("Role schema version changed - updating roles for all projects") | ||
| project_count = Project.objects.count() | ||
|
|
||
| if project_count > 100: | ||
| logger.warning( | ||
| f"Updating roles for {project_count} projects. " | ||
| f"This may take a while. Consider running 'python manage.py update_roles' " | ||
| f"separately for better control." | ||
| ) | ||
|
|
||
| logger.info("Creating roles for all projects") | ||
| try: | ||
| for project in Project.objects.all(): | ||
| try: | ||
| create_roles_for_project(project) | ||
| create_roles_for_project(project, force_update=True) | ||
| except Exception as e: | ||
| logger.warning(f"Failed to create roles for project {project.pk} ({project.name}): {e}") | ||
| continue | ||
|
|
||
| # Mark schema as updated | ||
| RoleSchemaVersion.mark_updated(description="Post-migration role update") | ||
| logger.info(f"Successfully updated roles for {project_count} projects") | ||
|
|
||
| except Exception as e: | ||
| logger.warning( | ||
| f"Failed to create roles during migration: {e}. This can be run manually via management command." | ||
| ) | ||
| logger.error(f"Failed to create roles during migration: {e}") | ||
|
|
There was a problem hiding this comment.
The updated create_roles signal with schema version checking lacks test coverage. Tests should verify that roles are only created when the schema changes, that the schema version is properly marked after updates, and that the signal behaves correctly when the RoleSchemaVersion table doesn't exist yet.
| try: | ||
| latest = cls.objects.first() | ||
| return latest is None or latest.version != current | ||
| except Exception: | ||
| # Table doesn't exist yet (first migration) | ||
| return False |
There was a problem hiding this comment.
The exception handling here catches all exceptions generically and returns False. This means if the table doesn't exist (expected during first migration), the schema update will be skipped. However, this also silently catches and ignores other database errors or unexpected exceptions. Consider being more specific about which exception to catch (e.g., OperationalError, ProgrammingError for table not existing) and logging unexpected exceptions.
| @classmethod | ||
| def mark_updated(cls, description="Schema updated"): | ||
| """Mark schema as updated to current version.""" | ||
| current = cls.get_current_version() | ||
| cls.objects.create(version=current, description=description) |
There was a problem hiding this comment.
There's a potential race condition in mark_updated where multiple processes could call this concurrently. Since version has unique=True, if two processes both detect the schema needs updating and both try to create the same version record, one will fail with an IntegrityError. Consider using get_or_create instead of create, or wrapping in try-except to handle the IntegrityError gracefully.
mihow
changed the title
|
@copilot @coderabbitai Is this overkill? We may change the role / permission schema once per year. |
|
Tip For best results, initiate chat on the files or code changes.
The key insight: Even though the role schema changes ~once per year, the current code runs
Then you're paying the full role-update cost on every single migration, even though 99% of those migrations don't touch roles at all. When this optimization matters:
When it might be overkill:
Middle ground options:
Given the PR also includes the bulk operations optimization (which is valuable regardless), you could consider whether the |
|
@coderabbitai another consideration is that we need to refactor how the permission groups are assigned. Currently we construct special names (strings) with the project name and role to query the right permission group. I propose overriding the |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 10010 🏁 Script executed: Length of output: 1541 🏁 Script executed: Length of output: 3787
Components by Impact Level✅ MINIMALLY AFFECTED (Core optimization preserved)RoleSchemaVersion model & version tracking logic:
Management command (
🔧 MODERATELY AFFECTED (Implementation details change)
🔨 SIGNIFICANTLY AFFECTED (Would become simpler!)Role class methods in # Current (string-based):
def get_group_name(cls, project):
return f"{project.pk}_{project.name}_{cls.__name__}"
def has_role(cls, user, project):
group_name = cls.get_group_name(project)
return user.groups.filter(name=group_name).exists()
# After refactor (FK-based):
def has_role(cls, user, project):
return user.groups.filter(
project=project,
role_type=cls.__name__
).exists()Signal handler in
Migration ConsiderationsWhen you do this refactor, you'd need:
Architecture CompatibilityThe key insight: This PR optimizes "when/how often to update" while your refactor optimizes "how to store/lookup groups." These are orthogonal concerns:
Both can coexist perfectly. In fact, doing this PR first gives you:
RecommendationOrder of operations:
The string-based naming is definitely tech debt (as the TODO comments acknowledge), but this PR's optimization architecture is future-proof for that refactor. 🐰 |
|
See more auto-generated reflection on this here #1098 |
Summary
This PR dramatically reduces database overhead during migrations by implementing smart schema version tracking for role permissions.
Key Improvements
Performance Impact
Before Optimization
After Optimization
Normal migrations (99% of cases - no schema changes):
Schema change migrations (rare - when permissions actually change):
New projects:
Technical Details
Permission.objects.bulk_create()group.permissions.set()GroupObjectPermission.objects.bulk_create()Files Modified
ami/users/models.py: Add RoleSchemaVersion model with version trackingami/users/roles.py:197: Optimize create_roles_for_project() with bulk operationsami/users/signals.py:14: Add conditional schema version check to create_roles()ami/users/management/commands/update_roles.py: New management commandami/users/migrations/0004_roleschemaversion.py: New migrationCLAUDE.md: Add Role Permission Management documentation sectionTesting
Migration Strategy
First deployment with this change:
Subsequent deployments:
Manual Updates
If needed, force update roles for all projects:
Options:
--project-id=N- Update specific project only--dry-run- Preview changes without applying--force- Force update even if groups exist (default: True)Backwards Compatibility
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.