Add tenant migration script across organizations#881
Open
nevil-mathew wants to merge 2 commits intodevelopfrom
Open
Add tenant migration script across organizations#881nevil-mathew wants to merge 2 commits intodevelopfrom
nevil-mathew wants to merge 2 commits intodevelopfrom
Conversation
Collaborator
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughA new migration script is introduced that orchestrates moving tenant data across organizations. The script handles configuration validation, database operations, external entity remapping, role management, transactional data copying/moving, post-migration verification, and supports dry-run mode with extensive logging throughout the process. Changes
Sequence DiagramsequenceDiagram
actor User
participant Script as Migration Script
participant DB as Database
participant ExtSvc as External Services
participant TxnMgr as Transaction Manager
User->>Script: Execute migration (source, target tenant, options)
Script->>Script: Validate config & arguments
activate Script
Script->>TxnMgr: Start SERIALIZABLE transaction
activate TxnMgr
Script->>DB: Apply locks on source/target tables
activate DB
DB-->>Script: Locks acquired
deactivate DB
Script->>DB: Query source tenant context (users, roles, orgs)
DB-->>Script: Source context data
Script->>DB: Query target tenant context
DB-->>Script: Target context data
Script->>ExtSvc: Fetch external entity mappings (parallel batches)
activate ExtSvc
ExtSvc-->>Script: External ID mappings
deactivate ExtSvc
Script->>Script: Remap external metadata for users
Script->>Script: Resolve role mappings (strategy-dependent)
alt Dry-Run Mode
Script->>Script: Log planned copy/move/delete operations
Script-->>User: Report dry-run results
else Execute Mode
Script->>DB: Copy forms, entity types, entities to target
DB-->>Script: Copy complete
Script->>DB: Move users, user_organizations, user_roles to target
DB-->>Script: Move complete
Script->>DB: Apply role mappings to moved users
DB-->>Script: Mappings applied
Script->>DB: Delete/soft-delete source rows (per cleanup mode)
DB-->>Script: Cleanup complete
Script->>DB: Verify migrated data counts & role consistency
DB-->>Script: Validation results
Script->>Script: Validate post-migration state
end
alt Validation Successful
Script->>TxnMgr: Commit transaction
TxnMgr->>DB: Commit all changes
TxnMgr-->>Script: Transaction committed
Script-->>User: Migration succeeded
else Validation Failed
Script->>TxnMgr: Rollback transaction
TxnMgr->>DB: Rollback all changes
TxnMgr-->>Script: Transaction rolled back
Script-->>User: Migration failed (error details)
end
deactivate TxnMgr
deactivate Script
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
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
🧪 Generate unit tests (beta)
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
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/scripts/migrateTenantOrgData.js`:
- Around line 2318-2368: The external I/O call remapExternalMetaForUsers is
executed while a SERIALIZABLE transaction and locks are held (tx created and
lockByStrategy called), which can prolong locks; move the
remapExternalMetaForUsers call out of the write transaction by invoking it
before opening the transaction and applying locks (or immediately after
buildContextAndPrecheck but outside any tx), assign its usersForInsert back onto
context, and keep the same external_meta_remap_ready logging; update any call
sites (remapExternalMetaForUsers(context)) so it does not depend on tx or locked
resources and ensure context is passed/returned unchanged except for
usersForInsert.
- Around line 90-109: The usage text in printUsage incorrectly shows "node
scripts/migrateTenantOrgData.js"; update the first usage example string to the
correct entrypoint "node src/scripts/migrateTenantOrgData.js" (i.e., change the
path in the template literal inside the printUsage function) so the help output
points to the actual file location.
- Around line 1220-1254: The target-role SELECT used for strict-id (the
querySelect that populates targetRolesByIdRows) currently includes soft-deleted
rows; modify that SQL to exclude deleted roles (e.g., add a WHERE clause
condition like ur.deleted_at IS NULL or equivalent) so the Map created as
targetRoleById only contains active roles, and then re-run the same change for
the other analogous query that builds targetRolesByIdRows in the later block
(the other strict-id path around the code referenced as applying to 1448-1459)
to ensure strict-id never matches soft-deleted roles.
- Around line 1079-1115: The query that builds sourceRolesFromUsers currently
un-nests users.roles without ensuring those role IDs actually belong to the
source org(s), so stale cross-org role IDs can be pulled in; update the SQL used
for sourceRolesFromUsers (the querySelect that assigns sourceRolesFromUsers) to
only select role IDs that exist in user_roles for the source tenant and allowed
organization scope (e.g., JOIN user_roles ur ON ur.id = unnest(roles) and join
organizations o to ensure ur.organization_id / o.code matches the source/default
org), so requiredRoleIds / requiredRoleIdArray only contains role IDs that truly
belong to the source org before fetching sourceRoleRows and building
sourceRoleById.
- Around line 1395-1446: After rebasing user_roles.id via the loops that use
queryRaw (see nextFreeRoleId, tempSeed, targetToSourceIdMap, tx), run a final
queryRaw inside the same transaction to reset the table's backing sequence to at
least the current max(id) for that tenant; call
setval(pg_get_serial_sequence('user_roles','id'), coalesce((SELECT MAX(id) FROM
user_roles WHERE tenant_code = $targetTenant AND deleted_at IS NULL), 1), true)
(or equivalent) passing targetTenant and tx so subsequent inserts won't reuse
updated ids.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8af91057-976c-4e6e-bdcc-afd837d57ec1
⛔ Files ignored due to path filters (2)
AGENTS.mdis excluded by!**/*.mdsrc/scripts/readme.mdis excluded by!**/*.md
📒 Files selected for processing (1)
src/scripts/migrateTenantOrgData.js
Comment on lines
+90
to
+109
| function printUsage() { | ||
| console.log(` | ||
| Usage: | ||
| node scripts/migrateTenantOrgData.js --current-tenant-code=<sourceTenant> --current-org-code=<orgCode> --new-tenant-code=<targetTenant> [options] | ||
|
|
||
| Required: | ||
| --current-tenant-code | ||
| --current-org-code | ||
| --new-tenant-code | ||
|
|
||
| Optional: | ||
| --role-resolution=strict-id|map-by-title (default: strict-id) | ||
| --strict-id-rebase=if-target-tenant-empty|never (default: if-target-tenant-empty) | ||
| --delete-mode=soft|hard|none (default: soft) | ||
| --delete-scope=users-only|all-copied (default: users-only) | ||
| --session-mode=invalidate|migrate (default: invalidate) | ||
| --lock-strategy=skip|advisory-only|advisory-table-lock (default: skip) | ||
| --dry-run=true|false (default: false) | ||
| --help | ||
| `) |
There was a problem hiding this comment.
Fix the usage example path.
Line 93 tells operators to run node scripts/migrateTenantOrgData.js, but this file lives under src/scripts. The current help text points to the wrong entrypoint.
🛠️ Suggested fix
Usage:
- node scripts/migrateTenantOrgData.js --current-tenant-code=<sourceTenant> --current-org-code=<orgCode> --new-tenant-code=<targetTenant> [options]
+ node src/scripts/migrateTenantOrgData.js --current-tenant-code=<sourceTenant> --current-org-code=<orgCode> --new-tenant-code=<targetTenant> [options]📝 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.
Suggested change
| function printUsage() { | |
| console.log(` | |
| Usage: | |
| node scripts/migrateTenantOrgData.js --current-tenant-code=<sourceTenant> --current-org-code=<orgCode> --new-tenant-code=<targetTenant> [options] | |
| Required: | |
| --current-tenant-code | |
| --current-org-code | |
| --new-tenant-code | |
| Optional: | |
| --role-resolution=strict-id|map-by-title (default: strict-id) | |
| --strict-id-rebase=if-target-tenant-empty|never (default: if-target-tenant-empty) | |
| --delete-mode=soft|hard|none (default: soft) | |
| --delete-scope=users-only|all-copied (default: users-only) | |
| --session-mode=invalidate|migrate (default: invalidate) | |
| --lock-strategy=skip|advisory-only|advisory-table-lock (default: skip) | |
| --dry-run=true|false (default: false) | |
| --help | |
| `) | |
| function printUsage() { | |
| console.log(` | |
| Usage: | |
| node src/scripts/migrateTenantOrgData.js --current-tenant-code=<sourceTenant> --current-org-code=<orgCode> --new-tenant-code=<targetTenant> [options] | |
| Required: | |
| --current-tenant-code | |
| --current-org-code | |
| --new-tenant-code | |
| Optional: | |
| --role-resolution=strict-id|map-by-title (default: strict-id) | |
| --strict-id-rebase=if-target-tenant-empty|never (default: if-target-tenant-empty) | |
| --delete-mode=soft|hard|none (default: soft) | |
| --delete-scope=users-only|all-copied (default: users-only) | |
| --session-mode=invalidate|migrate (default: invalidate) | |
| --lock-strategy=skip|advisory-only|advisory-table-lock (default: skip) | |
| --dry-run=true|false (default: false) | |
| --help | |
| `) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/scripts/migrateTenantOrgData.js` around lines 90 - 109, The usage text in
printUsage incorrectly shows "node scripts/migrateTenantOrgData.js"; update the
first usage example string to the correct entrypoint "node
src/scripts/migrateTenantOrgData.js" (i.e., change the path in the template
literal inside the printUsage function) so the help output points to the actual
file location.
Comment on lines
+1079
to
+1115
| const sourceRolesFromUsers = await querySelect( | ||
| sequelize, | ||
| `SELECT DISTINCT unnest(roles) AS role_id | ||
| FROM users | ||
| WHERE tenant_code = $sourceTenant | ||
| AND id = ANY($userIds);`, | ||
| { sourceTenant, userIds }, | ||
| tx | ||
| ) | ||
|
|
||
| const requiredRoleIds = new Set() | ||
| sourceRolesFromUor.forEach((row) => row.role_id !== null && requiredRoleIds.add(Number(row.role_id))) | ||
| sourceRolesFromUsers.forEach((row) => row.role_id !== null && requiredRoleIds.add(Number(row.role_id))) | ||
| const requiredRoleIdArray = Array.from(requiredRoleIds) | ||
|
|
||
| const sourceRoleRows = | ||
| requiredRoleIdArray.length > 0 | ||
| ? await querySelect( | ||
| sequelize, | ||
| `SELECT ur.id, ur.title, ur.deleted_at, ur.organization_id, o.code AS organization_code | ||
| FROM user_roles ur | ||
| JOIN organizations o | ||
| ON o.id = ur.organization_id | ||
| AND o.tenant_code = ur.tenant_code | ||
| WHERE ur.tenant_code = $sourceTenant | ||
| AND ur.id = ANY($requiredRoleIdArray);`, | ||
| { sourceTenant, requiredRoleIdArray }, | ||
| tx | ||
| ) | ||
| : [] | ||
|
|
||
| assertOrThrow(sourceRoleRows.length === requiredRoleIdArray.length, 'Referenced source role not found', { | ||
| requiredRoleCount: requiredRoleIdArray.length, | ||
| foundRoleCount: sourceRoleRows.length, | ||
| }) | ||
|
|
||
| const sourceRoleById = new Map(sourceRoleRows.map((row) => [Number(row.id), row])) |
There was a problem hiding this comment.
Reject users.roles entries from outside the source/default org scope.
Line 1081 pulls every role id from users.roles, but the only cross-org guard here is against user_organization_roles. Because user_roles are org-scoped, a stale role from some other organization can be copied into the target users.roles array even though only current-org-code membership is inserted.
🛡️ Suggested fix
const sourceRoleById = new Map(sourceRoleRows.map((row) => [Number(row.id), row]))
+ const invalidRoleScope = sourceRoleRows.filter((row) => {
+ const roleOrgCode = String(row.organization_code || '').trim().toLowerCase()
+ return (
+ roleOrgCode !== String(orgCode).trim().toLowerCase() &&
+ roleOrgCode !== String(defaultOrgCode).trim().toLowerCase()
+ )
+ })
+ assertOrThrow(
+ invalidRoleScope.length === 0,
+ 'Scoped users reference roles outside source/default organization',
+ { sample: invalidRoleScope.slice(0, 20) }
+ )🧰 Tools
🪛 Biome (2.4.6)
[error] 1090-1090: This callback passed to forEach() iterable method should not return a value.
(lint/suspicious/useIterableCallbackReturn)
[error] 1091-1091: This callback passed to forEach() iterable method should not return a value.
(lint/suspicious/useIterableCallbackReturn)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/scripts/migrateTenantOrgData.js` around lines 1079 - 1115, The query that
builds sourceRolesFromUsers currently un-nests users.roles without ensuring
those role IDs actually belong to the source org(s), so stale cross-org role IDs
can be pulled in; update the SQL used for sourceRolesFromUsers (the querySelect
that assigns sourceRolesFromUsers) to only select role IDs that exist in
user_roles for the source tenant and allowed organization scope (e.g., JOIN
user_roles ur ON ur.id = unnest(roles) and join organizations o to ensure
ur.organization_id / o.code matches the source/default org), so requiredRoleIds
/ requiredRoleIdArray only contains role IDs that truly belong to the source org
before fetching sourceRoleRows and building sourceRoleById.
Comment on lines
+1220
to
+1254
| const targetRolesByIdRows = await querySelect( | ||
| sequelize, | ||
| `SELECT ur.id, ur.title, ur.deleted_at, o.code AS organization_code | ||
| FROM user_roles ur | ||
| JOIN organizations o | ||
| ON o.id = ur.organization_id | ||
| AND o.tenant_code = ur.tenant_code | ||
| WHERE ur.tenant_code = $targetTenant | ||
| AND ur.id = ANY($requiredRoleIdArray);`, | ||
| { targetTenant, requiredRoleIdArray }, | ||
| tx | ||
| ) | ||
|
|
||
| const targetRoleById = new Map(targetRolesByIdRows.map((row) => [Number(row.id), row])) | ||
|
|
||
| let compatible = true | ||
| for (const roleId of requiredRoleIdArray) { | ||
| const sourceRole = sourceRoleById.get(roleId) | ||
| const targetRole = targetRoleById.get(roleId) | ||
| if (!targetRole) { | ||
| compatible = false | ||
| break | ||
| } | ||
| if ( | ||
| String(sourceRole.title).toLowerCase() !== String(targetRole.title).toLowerCase() || | ||
| String(sourceRole.organization_code).toLowerCase() !== String(targetRole.organization_code).toLowerCase() | ||
| ) { | ||
| compatible = false | ||
| break | ||
| } | ||
| } | ||
|
|
||
| if (compatible) { | ||
| return compatibilityResult | ||
| } |
There was a problem hiding this comment.
Ignore soft-deleted roles in strict-id resolution.
These queries treat a deleted target role as a valid match. If the target tenant already has a soft-deleted (id, title, org) row, strict-id can return an identity map and the migrated users.roles / user_organization_roles end up pointing at a deleted role. The map-by-title path already excludes deleted rows, so strict-id should do the same.
🧩 Suggested fix
`SELECT ur.id, ur.title, ur.deleted_at, o.code AS organization_code
FROM user_roles ur
JOIN organizations o
ON o.id = ur.organization_id
AND o.tenant_code = ur.tenant_code
WHERE ur.tenant_code = $targetTenant
+ AND ur.deleted_at IS NULL
AND ur.id = ANY($requiredRoleIdArray);`, `SELECT ur.id, ur.title, o.code AS organization_code
FROM user_roles ur
JOIN organizations o
ON o.id = ur.organization_id
AND o.tenant_code = ur.tenant_code
WHERE ur.tenant_code = $targetTenant
+ AND ur.deleted_at IS NULL
AND ur.id = ANY($requiredRoleIdArray);`,Also applies to: 1448-1459
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/scripts/migrateTenantOrgData.js` around lines 1220 - 1254, The
target-role SELECT used for strict-id (the querySelect that populates
targetRolesByIdRows) currently includes soft-deleted rows; modify that SQL to
exclude deleted roles (e.g., add a WHERE clause condition like ur.deleted_at IS
NULL or equivalent) so the Map created as targetRoleById only contains active
roles, and then re-run the same change for the other analogous query that builds
targetRolesByIdRows in the later block (the other strict-id path around the code
referenced as applying to 1448-1459) to ensure strict-id never matches
soft-deleted roles.
Comment on lines
+1395
to
+1446
| for (const blocker of blockingRows) { | ||
| await queryRaw( | ||
| sequelize, | ||
| `UPDATE user_roles | ||
| SET id = $newId | ||
| WHERE tenant_code = $targetTenant | ||
| AND deleted_at IS NULL | ||
| AND id = $oldId;`, | ||
| { | ||
| targetTenant, | ||
| oldId: Number(blocker.id), | ||
| newId: nextFreeRoleId, | ||
| }, | ||
| tx | ||
| ) | ||
| nextFreeRoleId += 1 | ||
| } | ||
|
|
||
| let tempSeed = -1000000000 | ||
| for (const mapping of targetToSourceIdMap) { | ||
| if (mapping.oldId === mapping.newId) { | ||
| continue | ||
| } | ||
| await queryRaw( | ||
| sequelize, | ||
| `UPDATE user_roles | ||
| SET id = $tmpId | ||
| WHERE tenant_code = $targetTenant | ||
| AND deleted_at IS NULL | ||
| AND id = $oldId;`, | ||
| { tmpId: tempSeed, targetTenant, oldId: mapping.oldId }, | ||
| tx | ||
| ) | ||
| mapping.tmpId = tempSeed | ||
| tempSeed -= 1 | ||
| } | ||
|
|
||
| for (const mapping of targetToSourceIdMap) { | ||
| if (mapping.oldId === mapping.newId) { | ||
| continue | ||
| } | ||
| await queryRaw( | ||
| sequelize, | ||
| `UPDATE user_roles | ||
| SET id = $newId | ||
| WHERE tenant_code = $targetTenant | ||
| AND deleted_at IS NULL | ||
| AND id = $tmpId;`, | ||
| { newId: mapping.newId, targetTenant, tmpId: mapping.tmpId }, | ||
| tx | ||
| ) | ||
| } |
There was a problem hiding this comment.
Reseed the user_roles.id sequence after rebasing ids.
Lines 1398-1443 rewrite an autoIncrement primary key. PostgreSQL does not advance the backing sequence when rows are updated, so if this tenant currently owns the table max id, the next role insert can reuse one of the newly assigned blocker ids and fail with a duplicate-key error.
🔁 Suggested fix
for (const mapping of targetToSourceIdMap) {
if (mapping.oldId === mapping.newId) {
continue
}
await queryRaw(
sequelize,
`UPDATE user_roles
SET id = $newId
WHERE tenant_code = $targetTenant
AND deleted_at IS NULL
AND id = $tmpId;`,
{ newId: mapping.newId, targetTenant, tmpId: mapping.tmpId },
tx
)
}
+
+ await queryRaw(
+ sequelize,
+ `SELECT setval(
+ pg_get_serial_sequence('user_roles', 'id'),
+ COALESCE((SELECT MAX(id) FROM user_roles), 1)
+ );`,
+ {},
+ tx
+ )
const verifyRows = await querySelect(📝 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.
Suggested change
| for (const blocker of blockingRows) { | |
| await queryRaw( | |
| sequelize, | |
| `UPDATE user_roles | |
| SET id = $newId | |
| WHERE tenant_code = $targetTenant | |
| AND deleted_at IS NULL | |
| AND id = $oldId;`, | |
| { | |
| targetTenant, | |
| oldId: Number(blocker.id), | |
| newId: nextFreeRoleId, | |
| }, | |
| tx | |
| ) | |
| nextFreeRoleId += 1 | |
| } | |
| let tempSeed = -1000000000 | |
| for (const mapping of targetToSourceIdMap) { | |
| if (mapping.oldId === mapping.newId) { | |
| continue | |
| } | |
| await queryRaw( | |
| sequelize, | |
| `UPDATE user_roles | |
| SET id = $tmpId | |
| WHERE tenant_code = $targetTenant | |
| AND deleted_at IS NULL | |
| AND id = $oldId;`, | |
| { tmpId: tempSeed, targetTenant, oldId: mapping.oldId }, | |
| tx | |
| ) | |
| mapping.tmpId = tempSeed | |
| tempSeed -= 1 | |
| } | |
| for (const mapping of targetToSourceIdMap) { | |
| if (mapping.oldId === mapping.newId) { | |
| continue | |
| } | |
| await queryRaw( | |
| sequelize, | |
| `UPDATE user_roles | |
| SET id = $newId | |
| WHERE tenant_code = $targetTenant | |
| AND deleted_at IS NULL | |
| AND id = $tmpId;`, | |
| { newId: mapping.newId, targetTenant, tmpId: mapping.tmpId }, | |
| tx | |
| ) | |
| } | |
| for (const blocker of blockingRows) { | |
| await queryRaw( | |
| sequelize, | |
| `UPDATE user_roles | |
| SET id = $newId | |
| WHERE tenant_code = $targetTenant | |
| AND deleted_at IS NULL | |
| AND id = $oldId;`, | |
| { | |
| targetTenant, | |
| oldId: Number(blocker.id), | |
| newId: nextFreeRoleId, | |
| }, | |
| tx | |
| ) | |
| nextFreeRoleId += 1 | |
| } | |
| let tempSeed = -1000000000 | |
| for (const mapping of targetToSourceIdMap) { | |
| if (mapping.oldId === mapping.newId) { | |
| continue | |
| } | |
| await queryRaw( | |
| sequelize, | |
| `UPDATE user_roles | |
| SET id = $tmpId | |
| WHERE tenant_code = $targetTenant | |
| AND deleted_at IS NULL | |
| AND id = $oldId;`, | |
| { tmpId: tempSeed, targetTenant, oldId: mapping.oldId }, | |
| tx | |
| ) | |
| mapping.tmpId = tempSeed | |
| tempSeed -= 1 | |
| } | |
| for (const mapping of targetToSourceIdMap) { | |
| if (mapping.oldId === mapping.newId) { | |
| continue | |
| } | |
| await queryRaw( | |
| sequelize, | |
| `UPDATE user_roles | |
| SET id = $newId | |
| WHERE tenant_code = $targetTenant | |
| AND deleted_at IS NULL | |
| AND id = $tmpId;`, | |
| { newId: mapping.newId, targetTenant, tmpId: mapping.tmpId }, | |
| tx | |
| ) | |
| } | |
| await queryRaw( | |
| sequelize, | |
| `SELECT setval( | |
| pg_get_serial_sequence('user_roles', 'id'), | |
| COALESCE((SELECT MAX(id) FROM user_roles), 1) | |
| );`, | |
| {}, | |
| tx | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/scripts/migrateTenantOrgData.js` around lines 1395 - 1446, After rebasing
user_roles.id via the loops that use queryRaw (see nextFreeRoleId, tempSeed,
targetToSourceIdMap, tx), run a final queryRaw inside the same transaction to
reset the table's backing sequence to at least the current max(id) for that
tenant; call setval(pg_get_serial_sequence('user_roles','id'), coalesce((SELECT
MAX(id) FROM user_roles WHERE tenant_code = $targetTenant AND deleted_at IS
NULL), 1), true) (or equivalent) passing targetTenant and tx so subsequent
inserts won't reuse updated ids.
Comment on lines
+2318
to
+2368
| tx = await sequelize.transaction({ | ||
| isolationLevel: Transaction.ISOLATION_LEVELS.SERIALIZABLE, | ||
| }) | ||
| log('info', 'transaction_started', { isolationLevel: 'SERIALIZABLE' }) | ||
|
|
||
| await lockByStrategy(sequelize, tx, options) | ||
| log('info', 'lock_strategy_applied', { lockStrategy: options.lockStrategy }) | ||
|
|
||
| const context = await buildContextAndPrecheck(sequelize, tx, options) | ||
| log('info', 'precheck_success', { | ||
| sourceOrgId: context.sourceOrg.id, | ||
| targetOrgId: context.targetOrg.id, | ||
| userCount: context.sourceCounts.users, | ||
| sourceCounts: context.sourceCounts, | ||
| }) | ||
| log('info', 'default_org_resolved', { | ||
| sourceTenant: context.sourceTenant, | ||
| defaultOrgCode: context.defaultOrgCode, | ||
| defaultOrgId: context.sourceDefaultOrg.id, | ||
| }) | ||
| log('info', 'external_entity_type_detection_summary', { | ||
| sourceTenant: context.sourceTenant, | ||
| sourceOrgCode: context.orgCode, | ||
| defaultOrgCode: context.defaultOrgCode, | ||
| sourceScopedCount: context.externalEntityTypeStats.sourceScopedCount, | ||
| defaultScopedCount: context.externalEntityTypeStats.defaultScopedCount, | ||
| dedupedFinalKeyCount: context.externalEntityTypeStats.dedupedCount, | ||
| externalMetaKeys: context.externalMetaKeys, | ||
| }) | ||
|
|
||
| let roleMap = new Map(context.requiredRoleIdArray.map((roleId) => [roleId, roleId])) | ||
| let performedStrictIdRebase = false | ||
|
|
||
| if (options.roleResolution === 'strict-id') { | ||
| const strictResult = await ensureStrictIdCompatibility(sequelize, tx, context, options) | ||
| roleMap = strictResult.roleMap | ||
| performedStrictIdRebase = strictResult.performedRebase | ||
| } else { | ||
| roleMap = await buildMapByTitleRoleMap(sequelize, tx, context) | ||
| } | ||
|
|
||
| log('info', 'role_resolution_ready', { | ||
| roleResolution: options.roleResolution, | ||
| requiredRoleCount: context.requiredRoleIdArray.length, | ||
| mapSize: roleMap.size, | ||
| performedStrictIdRebase, | ||
| }) | ||
|
|
||
| const externalRemapResult = await remapExternalMetaForUsers(context) | ||
| context.usersForInsert = externalRemapResult.usersForInsert | ||
| log('info', 'external_meta_remap_ready', externalRemapResult.stats) |
There was a problem hiding this comment.
Move external remap I/O out of the write transaction.
The SERIALIZABLE transaction and lock strategy are opened on Lines 2318-2324, but Line 2366 can still fan out into paginated external calls before any writes happen. A slow dependency will keep the snapshot and any advisory/table locks open for the full round-trip, which raises lock contention and serialization-retry risk for the rest of the system.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/scripts/migrateTenantOrgData.js` around lines 2318 - 2368, The external
I/O call remapExternalMetaForUsers is executed while a SERIALIZABLE transaction
and locks are held (tx created and lockByStrategy called), which can prolong
locks; move the remapExternalMetaForUsers call out of the write transaction by
invoking it before opening the transaction and applying locks (or immediately
after buildContextAndPrecheck but outside any tx), assign its usersForInsert
back onto context, and keep the same external_meta_remap_ready logging; update
any call sites (remapExternalMetaForUsers(context)) so it does not depend on tx
or locked resources and ensure context is passed/returned unchanged except for
usersForInsert.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit