Feature/org id code migration#1644
Conversation
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…into feature/org_id_code_migration
|
@coderabbitai full review and check if everything is okay and approved from your side. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Get PR details
gh pr view 1644 --repo ELEVATE-Project/mentoring --json title,body,state,additions,deletions,changedFiles,files,reviews,comments,reviewRequests,labels,assignees
echo "---FILES---"
gh pr diff 1644 --repo ELEVATE-Project/mentoring
echo "---REVIEW COMMENTS---"
gh api repos/ELEVATE-Project/mentoring/pulls/1644/comments --paginate | jq '[.[] | {id, path, line, body, user: .user.login}]'
echo "---REVIEWS---"
gh api repos/ELEVATE-Project/mentoring/pulls/1644/reviews | jq '[.[] | {id, state, body, user: .user.login}]'Length of output: 63657 🏁 Script executed: #!/bin/bash
echo "=== processEntityTypesToAddValueLabels signature ==="
rg -n "processEntityTypesToAddValueLabels" --type=js -C 5 | head -80
echo ""
echo "=== organisationExtensionQueries.findAll ==="
ast-grep --pattern 'static async findAll($$$) { $$$ }' | head -60
echo ""
echo "=== findAll in organisationExtension queries ==="
fd -i "organisationExtension" --type f | xargs grep -n "findAll" 2>/dev/null | head -30
echo ""
echo "=== updateRelatedOrgs function signature in services/org-admin ==="
rg -n "updateRelatedOrgs" --type=js -C 3 | head -60
echo ""
echo "=== getEntityTypesWithMentorOrg callers ==="
rg -n "getEntityTypesWithMentorOrg" --type=js -C 2
echo ""
echo "=== PUSH and POP constants ==="
rg -n "PUSH\|POP" src/constants/common.jsLength of output: 12145
🔴 Critical — SQL Injection Risks1. Migration batch update —
|
|
Warning Review limit reached
More reviews will be available in 34 minutes and 35 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughReplaces numeric organization IDs with organization code strings across cache, helper, DB schema/migrations, queries, services, Kafka consumer, and a cleanup script. ChangesOrganization Code Migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/database/queries/userExtension.js (1)
120-121:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRaw interpolation in array SQL literals is unsafe for org codes.
Line 120 and Line 150 embed org-code values directly into SQL. This can fail on quoted values and introduces injection risk.
Also applies to: 150-150
🤖 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 `@src/database/queries/userExtension.js` around lines 120 - 121, The code currently injects org codes directly into SQL via the raw interpolation used in the ARRAY[${newRelatedOrgsSql}] expression (affecting the array_cat/COALESCE update of visible_to_organizations and the similar occurrence around line 150), which is unsafe; instead, change the SQL to use a parameterized array (e.g. array_cat(COALESCE("visible_to_organizations", ARRAY[]::varchar[]), $X::varchar[])) and pass the org codes as a bound array parameter (e.g. relatedOrgs) from the query builder/execution call; update both places that reference newRelatedOrgsSql to use the parameter placeholder and add the corresponding parameter in the query parameters list so values are safely escaped and not raw-interpolated.
🧹 Nitpick comments (2)
src/services/mentors.js (1)
492-496: ⚖️ Poor tradeoffMentor creation sets minimal
visible_to_organizationscompared to role change.When creating a mentor extension directly,
visible_to_organizationsis set to only[orgCode]. However, when a mentee is changed to mentor viachangeRoleToMentorinorg-admin.js(lines 235-241), thevisible_to_organizationsincludes bothrelated_orgscodes and the current org code.This inconsistency means mentors created through different flows will have different visibility scopes. If this is intentional (e.g., direct creation should have minimal visibility, role change inherits existing visibility), consider adding a comment to clarify.
🤖 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 `@src/services/mentors.js` around lines 492 - 496, The mentor creation path in src/services/mentors.js currently sets visible_to_organizations to only [orgCode], which differs from the behavior in changeRoleToMentor (org-admin.js) that sets visible_to_organizations to include related_orgs plus the current org; update the mentor creation logic (the code that builds data and assigns visible_to_organizations) to compute the same visibility set as changeRoleToMentor (include related_orgs codes and orgCode) or, if minimal visibility is intentional, add a clear inline comment near the assignment of visible_to_organizations explaining the deliberate difference and why it should not inherit related_orgs—refer to visible_to_organizations, orgCode, and changeRoleToMentor to locate the relevant code paths.src/services/org-admin.js (1)
686-694: 💤 Low valueParameter names are misleading after migration.
The function receives organization codes from the controller, but the parameter names still say
deltaOrganizationIdsandorgId. While.toString()and.map(String)are harmless on strings, the naming creates confusion about what data type is expected.♻️ Suggested rename for clarity
-static async updateRelatedOrgs(deltaOrganizationIds, orgId, action, tenantCode) { +static async updateRelatedOrgs(deltaOrgCodes, orgCode, action, tenantCode) { try { - orgId = orgId.toString() - deltaOrganizationIds = deltaOrganizationIds.map(String) + orgCode = String(orgCode) + deltaOrgCodes = deltaOrgCodes.map(String) if (action?.toLowerCase() === common.PUSH.toLowerCase()) { - await menteeQueries.addVisibleToOrg(orgId, deltaOrganizationIds, tenantCode) + await menteeQueries.addVisibleToOrg(orgCode, deltaOrgCodes, tenantCode) } else if (action?.toLowerCase() === common.POP.toLowerCase()) { - await menteeQueries.removeVisibleToOrg(orgId, deltaOrganizationIds, tenantCode) + await menteeQueries.removeVisibleToOrg(orgCode, deltaOrgCodes, tenantCode) }🤖 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 `@src/services/org-admin.js` around lines 686 - 694, The parameter names in updateRelatedOrgs are misleading: rename orgId to orgCode and deltaOrganizationIds to deltaOrganizationCodes (function: updateRelatedOrgs) and update all internal uses (replace orgId.toString() with orgCode = String(orgCode) or just use orgCode if already a string, and deltaOrganizationIds = deltaOrganizationIds.map(String) → deltaOrganizationCodes = deltaOrganizationCodes.map(String)); also update calls to menteeQueries.addVisibleToOrg and removeVisibleToOrg to pass orgCode and deltaOrganizationCodes, and update any external callers of updateRelatedOrgs to use the new parameter names to keep semantics clear.
🤖 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 `@src/controllers/v1/org-admin.js`:
- Around line 139-145: The conversion result for organization_id may be empty
causing orgCode to be undefined; in the controller code around
convertOrgIdsToOrgCodes and before calling orgAdminService.updateRelatedOrgs,
validate that the returned [orgCode] is present and valid (e.g., not
undefined/null/empty) and if missing respond with an appropriate error (HTTP 400
or throw a ValidationError) including a clear message referencing
req.body.organization_id; ensure deltaOrgCodes is still computed but
short-circuit the updateRelatedOrgs call when orgCode is invalid to avoid
downstream failures.
In `@src/database/migrations/20260525000001-backfill-org-codes-from-ids.js`:
- Around line 101-108: The migration is doing per-row DB updates in the sessions
loop (sessionRows, toCodeArray, pgArr, queryInterface.sequelize.query) which
causes many round-trips; change it to a set-based update: build a single
in-memory list of (id, visible_to_organization_codes) updates using toCodeArray
and pgArr, then execute one SQL statement to apply them (for example using a
single UPDATE with a CASE ... WHEN id = ... THEN ... END or by bulk inserting
the tuples into a temp table and running an UPDATE ... FROM temp_table) so
sessionArrUpdated is updated accordingly; apply the same refactor to the other
per-row loop referenced in the comment (the block around the second loop).
- Around line 52-53: The current pgArr helper builds Postgres array literals via
arr.join(',') which breaks on org codes containing commas/quotes/braces; replace
use of pgArr by passing JS arrays as parameterized query parameters (e.g. use
$n::text[] and supply the JS array to node-postgres) instead of
string-concatenating, or implement a safe escaping routine that produces a valid
Postgres array literal (escaping quotes/backslashes and wrapping elements in
double quotes) and use that in place of pgArr; update every place that calls
pgArr (the pgArr function and the query sites that use it in this file) to use
parameterized array binding or the safe-escaping function.
- Around line 68-74: The migration is interpolating org codes into SQL via the
values string (sessionCodeUpdates → values) which can break on quotes and allow
injection; instead build a parameterized VALUES list and pass the parameters to
queryInterface.sequelize.query (or use binds) so no raw codes are concatenated.
Concretely, produce a VALUES clause of placeholders like ($1,$2),($3,$4),...
corresponding to each sessionCodeUpdates entry, collect ids and codes into a
single replacements/bind array, and call queryInterface.sequelize.query(sql, {
replacements: params }) (or { bind: params }) so the UPDATE of sessions SET
mentor_organization_code = v.code FROM (VALUES ...) AS v(id, code) uses
parameterized values rather than interpolated u.code.
In `@src/database/queries/mentorExtension.js`:
- Around line 360-363: The SQL literal is vulnerable because newRelatedOrgs
values are interpolated directly into sequelize.literal for array_cat/COALESCE;
instead, stop direct string interpolation of newRelatedOrgs and parameterize or
escape each org value before building the array expression. Locate the
array_cat(...) usage that references "visible_to_organizations" and
newRelatedOrgs and either (a) build the array via Sequelize helpers (e.g.,
sequelize.fn/ sequelize.cast) so values are passed as parameters, or (b) escape
each value with sequelize.escape (or use query bind/replacements) when
constructing the ARRAY[...] expression so quotes and special chars cannot break
the SQL or allow injection.
In `@src/scripts/dropOrgNumericBackupColumns.js`:
- Around line 106-107: The ALTER TABLE drop loop currently exits on the first
error which prevents the final materialized view rebuilds; modify the block that
runs await db.sequelize.query(`ALTER TABLE "${table}" DROP COLUMN IF EXISTS
"${column}" CASCADE`) / console.log(...) so it catches and records errors (do
not call process.exit or return from inside the loop), continue processing
remaining drops, and after the loop run the materialized view rebuild logic
regardless, then if any drops failed return a non-zero exit or throw a
summarized error; update any early-exit calls near the drop loop to instead
defer failure propagation until after the rebuild step.
---
Outside diff comments:
In `@src/database/queries/userExtension.js`:
- Around line 120-121: The code currently injects org codes directly into SQL
via the raw interpolation used in the ARRAY[${newRelatedOrgsSql}] expression
(affecting the array_cat/COALESCE update of visible_to_organizations and the
similar occurrence around line 150), which is unsafe; instead, change the SQL to
use a parameterized array (e.g. array_cat(COALESCE("visible_to_organizations",
ARRAY[]::varchar[]), $X::varchar[])) and pass the org codes as a bound array
parameter (e.g. relatedOrgs) from the query builder/execution call; update both
places that reference newRelatedOrgsSql to use the parameter placeholder and add
the corresponding parameter in the query parameters list so values are safely
escaped and not raw-interpolated.
---
Nitpick comments:
In `@src/services/mentors.js`:
- Around line 492-496: The mentor creation path in src/services/mentors.js
currently sets visible_to_organizations to only [orgCode], which differs from
the behavior in changeRoleToMentor (org-admin.js) that sets
visible_to_organizations to include related_orgs plus the current org; update
the mentor creation logic (the code that builds data and assigns
visible_to_organizations) to compute the same visibility set as
changeRoleToMentor (include related_orgs codes and orgCode) or, if minimal
visibility is intentional, add a clear inline comment near the assignment of
visible_to_organizations explaining the deliberate difference and why it should
not inherit related_orgs—refer to visible_to_organizations, orgCode, and
changeRoleToMentor to locate the relevant code paths.
In `@src/services/org-admin.js`:
- Around line 686-694: The parameter names in updateRelatedOrgs are misleading:
rename orgId to orgCode and deltaOrganizationIds to deltaOrganizationCodes
(function: updateRelatedOrgs) and update all internal uses (replace
orgId.toString() with orgCode = String(orgCode) or just use orgCode if already a
string, and deltaOrganizationIds = deltaOrganizationIds.map(String) →
deltaOrganizationCodes = deltaOrganizationCodes.map(String)); also update calls
to menteeQueries.addVisibleToOrg and removeVisibleToOrg to pass orgCode and
deltaOrganizationCodes, and update any external callers of updateRelatedOrgs to
use the new parameter names to keep semantics clear.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 342279b4-2f90-44da-b1e9-ed03d2de5b48
📒 Files selected for processing (18)
src/constants/common.jssrc/controllers/v1/org-admin.jssrc/database/migrations/20260525000001-backfill-org-codes-from-ids.jssrc/database/migrations/20260525000002-rename-visible-to-org-codes.jssrc/database/models/sessions.jssrc/database/queries/mentorExtension.jssrc/database/queries/sessions.jssrc/database/queries/userExtension.jssrc/generics/cacheHelper.jssrc/generics/kafka/consumers/organization.jssrc/helpers/getOrgIdAndEntityTypewithEntitiesBasedOnPolicy.jssrc/helpers/orgUtils.jssrc/helpers/saasUserAccessibility.jssrc/scripts/dropOrgNumericBackupColumns.jssrc/services/mentees.jssrc/services/mentors.jssrc/services/org-admin.jssrc/services/sessions.js
|
@coderabbitai full review and see if everything is okay. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== 1. Migration backfill VALUES interpolation ==="
gh api repos/ELEVATE-Project/mentoring/contents/src/database/migrations/20260525000001-backfill-org-codes-from-ids.js?ref=feature/org_id_code_migration | jq -r '.content' | base64 -d | grep -n "u\.code\|pgArr\|VALUES\|replace\|escape"
echo ""
echo "=== 2. pgArr helper ==="
gh api repos/ELEVATE-Project/mentoring/contents/src/database/migrations/20260525000001-backfill-org-codes-from-ids.js?ref=feature/org_id_code_migration | jq -r '.content' | base64 -d | grep -n "pgArr" | head -10
echo ""
echo "=== 3. mentorExtension.js - array_cat unsafe interpolation ==="
gh api repos/ELEVATE-Project/mentoring/contents/src/database/queries/mentorExtension.js?ref=feature/org_id_code_migration | jq -r '.content' | base64 -d | grep -n "array_cat\|ARRAY\[" | head -20
echo ""
echo "=== 4. userExtension.js - ARRAY interpolation ==="
gh api repos/ELEVATE-Project/mentoring/contents/src/database/queries/userExtension.js?ref=feature/org_id_code_migration | jq -r '.content' | base64 -d | grep -n "ARRAY\[" | head -20
echo ""
echo "=== 5. controller org-admin undefined guard ==="
gh api repos/ELEVATE-Project/mentoring/contents/src/controllers/v1/org-admin.js?ref=feature/org_id_code_migration | jq -r '.content' | base64 -d | grep -n -A5 "organization_id\|convertOrgIds\|failureResponse\|400" | head -40
echo ""
echo "=== 6. information_schema table_schema filter ==="
gh api repos/ELEVATE-Project/mentoring/contents/src/database/migrations/20260525000001-backfill-org-codes-from-ids.js?ref=feature/org_id_code_migration | jq -r '.content' | base64 -d | grep -n "information_schema\|table_schema\|public"
echo ""
echo "=== 7. scripts/dropOrgNumericBackupColumns.js - main() guard ==="
gh api repos/ELEVATE-Project/mentoring/contents/src/scripts/dropOrgNumericBackupColumns.js?ref=feature/org_id_code_migration | jq -r '.content' | base64 -d | tail -20
echo ""
echo "=== 8. JSDoc in org-admin service updateRelatedOrgs ==="
gh api repos/ELEVATE-Project/mentoring/contents/src/services/org-admin.js?ref=feature/org_id_code_migration | jq -r '.content' | base64 -d | grep -n -B2 -A10 "updateRelatedOrgs" | head -40Length of output: 3224 🏁 Script executed: #!/bin/bash
# Get the PR head SHA first
HEAD_SHA=$(gh api repos/ELEVATE-Project/mentoring/pulls/1644 | jq -r '.head.sha')
echo "HEAD SHA: $HEAD_SHA"
HEAD_REF=$(gh api repos/ELEVATE-Project/mentoring/pulls/1644 | jq -r '.head.ref')
echo "HEAD REF: $HEAD_REF"
echo ""
echo "=== 1. Migration backfill - VALUES interpolation & pgArr ==="
gh api "repos/ELEVATE-Project/mentoring/contents/src/database/migrations/20260525000001-backfill-org-codes-from-ids.js?ref=${HEAD_SHA}" | jq -r '.content' | base64 -d | grep -n "u\.code\|pgArr\|VALUES\|replace\|escape\|table_schema\|information_schema"
echo ""
echo "=== 2. mentorExtension.js - array_cat unsafe interpolation ==="
gh api "repos/ELEVATE-Project/mentoring/contents/src/database/queries/mentorExtension.js?ref=${HEAD_SHA}" | jq -r '.content' | base64 -d | grep -n "array_cat\|ARRAY\["
echo ""
echo "=== 3. userExtension.js - ARRAY interpolation ==="
gh api "repos/ELEVATE-Project/mentoring/contents/src/database/queries/userExtension.js?ref=${HEAD_SHA}" | jq -r '.content' | base64 -d | grep -n "ARRAY\["
echo ""
echo "=== 4. controller org-admin - undefined guard ==="
gh api "repos/ELEVATE-Project/mentoring/contents/src/controllers/v1/org-admin.js?ref=${HEAD_SHA}" | jq -r '.content' | base64 -d | grep -n -A8 "updateRelatedOrgs\|convertOrgIds\|organization_id\|failureResponse\|400" | head -50
echo ""
echo "=== 5. dropOrgNumericBackupColumns.js tail ==="
gh api "repos/ELEVATE-Project/mentoring/contents/src/scripts/dropOrgNumericBackupColumns.js?ref=${HEAD_SHA}" | jq -r '.content' | base64 -d | tail -15
echo ""
echo "=== 6. org-admin service JSDoc for updateRelatedOrgs ==="
gh api "repos/ELEVATE-Project/mentoring/contents/src/services/org-admin.js?ref=${HEAD_SHA}" | jq -r '.content' | base64 -d | grep -n -A15 "updateRelatedOrgs" | head -50Length of output: 7293
✅ Fixed
🔴 Still Open — Blocking Approval
// Still unescaped:
const values = sessionCodeUpdates.map((u) => `(${u.id}, '${u.code}')`).join(', ')Fix: escape single quotes in const values = sessionCodeUpdates.map((u) => `(${u.id}, '${u.code.replace(/'/g, "''")}')`).join(', ')
// Still broken for codes with commas/spaces:
const pgArr = (arr) => `{${arr.join(',')}}`Fix: const pgArr = (arr) => `{${arr.map((v) => `"${v.replace(/"/g, '\\"')}"`).join(',')}}`
`ARRAY[${newRelatedOrgs.map((v) => `'${v}'`).join(',')}]::varchar[]`
`COALESCE(...) || ARRAY['${organizationId}']::varchar[]`Org codes interpolated as string literals — a
🟡 Still Open — Non-blocking
I cannot approve the PR yet because issues |
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@coderabbitai made changes to the codebase. Just verify now. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
HEAD_SHA=$(gh api repos/ELEVATE-Project/mentoring/pulls/1644 | jq -r '.head.sha')
echo "HEAD SHA: $HEAD_SHA"
echo ""
echo "=== `#1` & `#2`: Migration backfill - VALUES interpolation & pgArr ==="
gh api "repos/ELEVATE-Project/mentoring/contents/src/database/migrations/20260525000001-backfill-org-codes-from-ids.js?ref=${HEAD_SHA}" | jq -r '.content' | base64 -d | grep -n "pgArr\|u\.code\|VALUES\|replace\|escape\|table_schema\|information_schema" | head -30
echo ""
echo "=== `#1` & `#2`: Full pgArr and VALUES section ==="
gh api "repos/ELEVATE-Project/mentoring/contents/src/database/migrations/20260525000001-backfill-org-codes-from-ids.js?ref=${HEAD_SHA}" | jq -r '.content' | base64 -d | sed -n '45,90p'
echo ""
echo "=== `#3`: mentorExtension.js - ARRAY literals ==="
gh api "repos/ELEVATE-Project/mentoring/contents/src/database/queries/mentorExtension.js?ref=${HEAD_SHA}" | jq -r '.content' | base64 -d | grep -n "array_cat\|ARRAY\[\|varchar\|replace\|escape" | head -20
echo ""
echo "=== `#4`: userExtension.js - ARRAY literals ==="
gh api "repos/ELEVATE-Project/mentoring/contents/src/database/queries/userExtension.js?ref=${HEAD_SHA}" | jq -r '.content' | base64 -d | grep -n "ARRAY\[\|varchar\|replace\|escape" | head -20
echo ""
echo "=== `#6`: information_schema table_schema filter in migration ==="
gh api "repos/ELEVATE-Project/mentoring/contents/src/database/migrations/20260525000001-backfill-org-codes-from-ids.js?ref=${HEAD_SHA}" | jq -r '.content' | base64 -d | grep -n "information_schema\|table_schema\|public" | head -10
echo ""
echo "=== `#7`: JSDoc in org-admin service updateRelatedOrgs ==="
gh api "repos/ELEVATE-Project/mentoring/contents/src/services/org-admin.js?ref=${HEAD_SHA}" | jq -r '.content' | base64 -d | grep -n -B2 -A12 "updateRelatedOrgs" | head -50
echo ""
echo "=== `#9`: dropOrgNumericBackupColumns.js tail - require.main guard ==="
gh api "repos/ELEVATE-Project/mentoring/contents/src/scripts/dropOrgNumericBackupColumns.js?ref=${HEAD_SHA}" | jq -r '.content' | base64 -d | tail -20Length of output: 10331
✅ All Critical Issues Fixed
🟡 Remaining Non-Blocking Items (can be addressed in follow-up)
Since all blocking SQL injection / data corruption risks are resolved, I'm approving the PR. The three remaining items are minor and can be cleaned up in a follow-up. [approve] 🐇 ✨ |
Uh oh!
There was an error while loading. Please reload this page.