optimize Cedar migration: replace N+1 queries with single query#1656
optimize Cedar migration: replace N+1 queries with single query#1656
Conversation
Fetch all groups needing migration in one query instead of iterating over every connection and querying groups per connection. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR refactors the Cedar policy migration script to migrate all groups missing a cedarPolicy in a single query/loop rather than iterating connection-by-connection.
Changes:
- Removes per-connection iteration and instead fetches all groups with missing/empty
cedarPolicy. - Generates Cedar policies using each group’s associated connection and permissions, then saves them back to the group.
- Simplifies repositories used by removing now-unneeded
ConnectionEntity/PermissionEntityrepositories.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| .leftJoinAndSelect('group.connection', 'connection') | ||
| .leftJoinAndSelect('group.permissions', 'permission') | ||
| .where('group.cedarPolicy IS NULL OR group.cedarPolicy = :empty', { empty: '' }) | ||
| .getMany(); | ||
|
|
||
| for (const group of groups) { | ||
| const permissions = group.permissions || []; | ||
| for (const group of groups) { | ||
| const connection = group.connection; | ||
| if (!connection) continue; |
There was a problem hiding this comment.
leftJoinAndSelect('group.connection', ...) plus if (!connection) continue; silently skips groups with missing connections. Consider using an innerJoinAndSelect (or adding a connection.id IS NOT NULL condition) so the query only returns migratable groups, and/or log/count skipped groups so the final message isn't misleading.
| const groups = await groupRepository | ||
| .createQueryBuilder('group') | ||
| .leftJoinAndSelect('group.connection', 'connection') | ||
| .leftJoinAndSelect('group.permissions', 'permission') | ||
| .where('group.cedarPolicy IS NULL OR group.cedarPolicy = :empty', { empty: '' }) | ||
| .getMany(); |
There was a problem hiding this comment.
This loads all groups lacking cedarPolicy (plus all their permissions) into memory at once via getMany(). If this runs during app bootstrap and the dataset is large, it can cause long startup times or OOM. Consider processing in pages/chunks (take/skip) or iterating by connection/group ids to bound memory.
| for (const tp of tablePermissions) { | ||
| const existing = tableMap.get(tp.tableName) || { | ||
| tableName: tp.tableName, | ||
| accessLevel: { visibility: false, readonly: false, add: false, delete: false, edit: false }, | ||
| }; |
There was a problem hiding this comment.
tp.tableName can be an empty string (PermissionEntity defaults tableName to ''), which would collapse multiple table permissions into the same map key and generate an incorrect policy. Add a guard to skip/log table permissions with missing/blank tableName (or treat them as invalid data).
Fetch all groups needing migration in one query instead of iterating over every connection and querying groups per connection.