Migrations from TM and docs#56
Conversation
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
|
|
||
| """ | ||
|
|
||
| import sqlalchemy as sa |
| import sqlalchemy as sa | ||
| from alembic import op | ||
| from geoalchemy2 import Geometry | ||
| from sqlalchemy.dialects import postgresql |
|
|
||
| """ | ||
|
|
||
| import sqlalchemy as sa |
|
Warning Review limit reached
Next review available in: 20 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe pull request documents the deployment architecture, updates Alembic geospatial initialization, adds ChangesTask database migration chain
Estimated code review effort: 5 (Critical) | ~120 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
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: 9
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (28)
pyproject.toml-38-41 (1)
38-41: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winUse bound parameters in
alembic_task/versions/bfcf4182dcb5_.py. The rawnameis still interpolated into SQL, andhandle_special_chars()only doubles the first', so names with additional quotes can still break the statement. Replace the f-string with a parameterizedUPDATE.🤖 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 `@pyproject.toml` around lines 38 - 41, Update the migration logic in bfcf4182dcb5_ to replace the interpolated SQL f-string with a parameterized UPDATE using bound parameters for the organisation name and slug; remove reliance on handle_special_chars() for SQL escaping and ensure all values are passed through the database execution parameters.alembic_task/versions/d2e18f0f34a9_.py-21-27 (1)
21-27: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
project_favoritestable is missing a primary key.This join table has no primary key column or composite primary key constraint. Without uniqueness enforcement, duplicate favorite entries can be inserted, and ORM operations that rely on identity will be unreliable. Consider adding a composite PK on
(project_id, user_id)or a surrogate key.Additionally, both FK columns are
nullable=True, which allows rows with no valid references — likely unintended for a favorites association table.🛡️ Proposed fix: add composite primary key and make FK columns non-null
op.create_table( "project_favorites", - sa.Column("project_id", sa.Integer(), nullable=True), - sa.Column("user_id", sa.BigInteger(), nullable=True), + sa.Column("project_id", sa.Integer(), nullable=False), + sa.Column("user_id", sa.BigInteger(), nullable=False), sa.ForeignKeyConstraint(["project_id"], ["projects.id"]), sa.ForeignKeyConstraint(["user_id"], ["users.id"]), + sa.PrimaryKeyConstraint("project_id", "user_id"), )🤖 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 `@alembic_task/versions/d2e18f0f34a9_.py` around lines 21 - 27, Update the project_favorites table definition in the migration to make project_id and user_id nullable=False and enforce uniqueness with a composite primary key on both columns, using a PrimaryKeyConstraint or equivalent constraint in op.create_table.alembic_task/versions/068674f06b0f_.py-34-40 (1)
34-40: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDowngrade will fail if
expert_modecontains NULLs.The downgrade sets
expert_modeback tonullable=Falsewithout first backfilling any NULL values. Since the upgrade made the column nullable, rows with NULLexpert_modemay exist at downgrade time, causingALTER COLUMN ... SET NOT NULLto fail.🛡️ Proposed fix: backfill NULLs before re-imposing NOT NULL
def downgrade(): # ### commands auto-generated by Alembic - please adjust! ### + op.execute("UPDATE users SET expert_mode = false WHERE expert_mode IS NULL") op.alter_column( "users", "expert_mode", existing_type=sa.BOOLEAN(), nullable=False, existing_server_default=sa.text("false"), ) op.drop_column("users", "picture_url") # ### end Alembic commands ###🤖 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 `@alembic_task/versions/068674f06b0f_.py` around lines 34 - 40, Before the downgrade’s op.alter_column call for users.expert_mode, update all NULL expert_mode values to false using the migration’s database connection, then apply nullable=False. Keep the existing server default and type settings unchanged.alembic_task/versions/2cfa981c70e7_.py-21-42 (1)
21-42: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUpgrade will fail if geometry columns contain NULLs.
Setting
centroidandgeometrytonullable=Falsewithout backfilling existing NULL values will causeALTER COLUMN ... SET NOT NULLto fail if any rows have NULL geometry data. A data backfill or cleanup step should precede the nullability change.🛡️ Proposed fix: backfill or remove NULL rows before setting NOT NULL
def upgrade(): # ### commands auto-generated by Alembic - please adjust! ### + op.execute("DELETE FROM projects WHERE centroid IS NULL OR geometry IS NULL") op.alter_column( "projects", "centroid",If deleting rows is too aggressive, consider updating with default geometry values instead:
+ op.execute("UPDATE projects SET centroid = ST_SetSRID(ST_MakePoint(0, 0), 4326) WHERE centroid IS NULL") + op.execute("UPDATE projects SET geometry = ST_GeomFromEWKT('SRID=4326;MULTIPOLYGON EMPTY') WHERE geometry IS NULL")🤖 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 `@alembic_task/versions/2cfa981c70e7_.py` around lines 21 - 42, Before the nullability changes in the migration upgrade function, backfill valid geometry values or remove rows with NULL projects.centroid and projects.geometry, handling each column consistently and safely. Then retain the op.alter_column calls to set both columns nullable=False, ensuring the migration succeeds on existing data.alembic_task/versions/0a6b82b55983_.py-59-75 (1)
59-75: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPotential
TypeErrorifmessage.subjectormessage.messageis NULL.
re.search()raisesTypeErrorwhen passedNone. If any existing message has a NULLsubjectormessagefield, the migration will crash. Add a guard before regex operations.🛡️ Proposed fix: guard against NULL fields
- match = project_task_re.search(message.subject) + subject = message.subject or "" if match: project_id = match.group(1) task_id = match.group(2) - if validated_notification_re.search(message.subject): + if validated_notification_re.search(subject): message_type = 4 - elif invalidated_notification_re.search(message.subject): + elif invalidated_notification_re.search(subject): message_type = 5 - elif mention_notification_re.search(message.subject): + elif mention_notification_re.search(subject): message_type = 3 - elif welcome_re.search(message.subject): + elif welcome_re.search(subject): message_type = 1 # Look for direct messages from project managers if message_type is None and project_id is None: - match = message_all_re.search(message.message) + match = message_all_re.search(message.message or "")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@alembic_task/versions/0a6b82b55983_.py` around lines 59 - 75, Guard nullable fields before every regex operation in the migration’s message-processing logic: ensure message.subject and message.message are non-NULL strings before passing them to project_task_re, validated_notification_re, invalidated_notification_re, mention_notification_re, welcome_re, or message_all_re. Preserve the existing classification behavior for populated fields and safely skip matching when values are NULL.alembic_task/versions/0a6b82b55983_.py-80-96 (1)
80-96: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winLogic bug: messages with
message_typebut noproject_idare silently skipped.When
message_typeis set (e.g., welcome messages → type 1) butproject_idis None, the code convertsproject_idto the string"null"(line 85) and then runsselect * from projects where id = null(line 91), which always returns empty. This setsproject_existence["null"] = False, causing the update to be skipped (line 96). Messages that should have their type classified are left unmodified.🐛 Proposed fix: skip project existence check when project_id is None
if project_id not in project_existence: - project = conn.execute( - sa.text("select * from projects where id = " + str(project_id)) - ).first() - project_existence[project_id] = project is not None + if project_id == "null": + project_existence[project_id] = True + else: + project = conn.execute( + sa.text("select * from projects where id = :pid"), + {"pid": int(project_id)}, + ).first() + project_existence[project_id] = project is not None # Only process messages from projects that still exist - if project_existence[project_id]: + if project_existence.get(project_id, True):🤖 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 `@alembic_task/versions/0a6b82b55983_.py` around lines 80 - 96, Fix the classification logic in the message update block so messages with a non-null message_type and no project_id are still updated. Preserve project_id as None for the existence-check decision, skip the projects lookup and project_existence gating when project_id is absent, and only perform that check for messages with a real project_id; retain task_id/message_type fallback handling as needed.alembic_task/versions/073a96d114ab_.py-21-36 (1)
21-36: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftBackfill
workspaces.imageryListbefore dropping italembic_task/versions/073a96d114ab_.py:21-36This migration creates
workspaces_imageryand dropsworkspaces.imageryList, but it never copies existing values over. Any saved imagery settings in the old column will be lost, and the downgrade won’t restore them either. Add a data-migration step first if existing rows need to be preserved.🤖 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 `@alembic_task/versions/073a96d114ab_.py` around lines 21 - 36, Before dropping imageryList in the migration, backfill workspaces_imagery with one row per existing workspace containing the parsed imageryList data and appropriate modified metadata, handling null or empty values safely. Update the downgrade to recreate imageryList and restore each workspace’s serialized imagery settings from workspaces_imagery before removing that table.alembic_task/versions/0aaac86a48dc_.py-21-27 (1)
21-27: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMissing primary key on join table allows duplicate entries.
The
project_allowed_userstable has no primary key, and bothproject_idanduser_idare nullable. Without a primary key or unique constraint on(project_id, user_id), duplicate associations can be inserted, leading to data integrity issues. Additionally, nullable foreign key columns in a join table are unusual — a row with a nullproject_idoruser_idserves no purpose.🛡️ Proposed fix
op.create_table( "project_allowed_users", - sa.Column("project_id", sa.Integer(), nullable=True), - sa.Column("user_id", sa.BigInteger(), nullable=True), + sa.Column("project_id", sa.Integer(), nullable=False), + sa.Column("user_id", sa.BigInteger(), nullable=False), sa.ForeignKeyConstraint(["project_id"], ["projects.id"]), sa.ForeignKeyConstraint(["user_id"], ["users.id"]), + sa.PrimaryKeyConstraint("project_id", "user_id"), )🤖 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 `@alembic_task/versions/0aaac86a48dc_.py` around lines 21 - 27, Update the project_allowed_users table definition to make project_id and user_id non-nullable and enforce uniqueness for the association, preferably by adding a composite primary key on both columns (or an equivalent unique constraint). Preserve the existing foreign key relationships.alembic_task/versions/924a63857df4_.py-28-32 (1)
28-32: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUser notification preferences are lost during migration.
The upgrade drops
comments_notifications(line 28) before migrating its values. Both new columns are unconditionally set tofalse(lines 29, 31), so any users who hadcomments_notifications = truesilently lose their preference. Consider migrating existing values before dropping the column.🛡️ Proposed fix to preserve existing preferences
op.add_column( "users", sa.Column("projects_comments_notifications", sa.Boolean(), nullable=True), ) op.add_column( "users", sa.Column("tasks_comments_notifications", sa.Boolean(), nullable=True) ) - op.drop_column("users", "comments_notifications") - op.execute("UPDATE users SET projects_comments_notifications = false") + op.execute("UPDATE users SET projects_comments_notifications = comments_notifications") - op.alter_column("users", "projects_comments_notifications", nullable=False) - op.execute("UPDATE users SET tasks_comments_notifications = false") + op.execute("UPDATE users SET tasks_comments_notifications = comments_notifications") + op.alter_column("users", "projects_comments_notifications", nullable=False) + op.drop_column("users", "comments_notifications") op.alter_column("users", "tasks_comments_notifications", nullable=False)🤖 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 `@alembic_task/versions/924a63857df4_.py` around lines 28 - 32, Preserve existing notification preferences in the migration upgrade: before dropping comments_notifications, copy its values into both projects_comments_notifications and tasks_comments_notifications, then enforce non-null constraints and drop the legacy column only after the data migration completes. Update the relevant alembic upgrade function and avoid unconditional false assignments.alembic_task/versions/48c3aed123cf_.py-28-34 (1)
28-34: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDowngrade may fail if NULL values exist in
x,y, orzoom.The downgrade sets
nullable=Falseonx,y, andzoom. If any rows have NULL values in these columns (which is now possible after the upgrade), the downgrade will fail with a NOT NULL constraint violation. Consider adding a data backfill in the downgrade (e.g., setting NULL values to 0 or another default) before applying the non-nullable constraint.🤖 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 `@alembic_task/versions/48c3aed123cf_.py` around lines 28 - 34, Update downgrade() to backfill NULL values in tasks.x, tasks.y, and tasks.zoom with an appropriate default before altering those columns to nullable=False; execute the updates via Alembic’s op.execute, then retain the existing alter_column calls.alembic_task/versions/451f6bd05a19_.py-26-27 (1)
26-27: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winImplement a proper downgrade for the trigger migration.
The downgrade is
pass, meaning rolling back this migration leaves the new trigger in place. If a previous version of the trigger existed, the downgrade should restore it. If this is the first time the trigger is created, document whypassis acceptable.🤖 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 `@alembic_task/versions/451f6bd05a19_.py` around lines 26 - 27, The downgrade() function is empty, so rolling back does not remove or restore the trigger introduced by this migration. Implement downgrade() to drop the new trigger and recreate the prior trigger definition if one existed; otherwise, add a clear comment documenting why pass is intentionally acceptable.alembic_task/versions/55790eeec3bf_.py-63-71 (1)
63-71: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDowngrade may fail if NULL values exist in
notificationscolumns.Same pattern as other migrations in this chain: the downgrade restores
nullable=Falseondate,unread_count, anduser_id. If any rows have NULL values after the upgrade, the downgrade will fail with a NOT NULL constraint violation. Consider backfilling NULLs to defaults before re-applying the non-nullable constraints.🤖 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 `@alembic_task/versions/55790eeec3bf_.py` around lines 63 - 71, Update the downgrade function containing these notifications.alter_column calls to backfill NULL values in user_id, unread_count, and date with appropriate defaults before restoring nullable=False, using executable SQL or equivalent migration operations; then apply the non-nullable constraints.alembic_task/versions/f86698c827cc_.py-70-102 (1)
70-102: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPrevent silent data loss during downgrade.
The downgrade preserves only campaign names and project links, then deletes all campaign-organization relationships and campaign rows. Any
logo,url,description, or post-upgrade organization association is irreversibly lost. Add a preflight guard that rejects downgrade when unrepresentable data exists, or archive it explicitly.🤖 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 `@alembic_task/versions/f86698c827cc_.py` around lines 70 - 102, The downgrade migration silently discards campaign metadata and organization associations. In the downgrade function, add a preflight validation before mutating data that detects non-null or non-empty campaign logo, URL, description, and organization relationships that cannot be represented, then raise an explicit migration error; alternatively, archive those values before deletion and verify the archive completes.alembic_task/versions/badf8bb7d56b_.py-52-57 (1)
52-57: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winBackfill nullable timestamps before restoring
NOT NULL.Any
application_keysrow written withcreated = NULLafter the upgrade makes this downgrade fail. Apply an explicit rollback policy before tightening the constraint.Proposed fix
+ op.execute( + "UPDATE application_keys " + "SET created = CURRENT_TIMESTAMP " + "WHERE created IS NULL" + ) op.alter_column( "application_keys", "created", existing_type=postgresql.TIMESTAMP(), nullable=False, )🤖 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 `@alembic_task/versions/badf8bb7d56b_.py` around lines 52 - 57, Before the nullable=False alteration in the downgrade function, explicitly backfill all NULL values in application_keys.created using the migration’s rollback policy, then apply the NOT NULL constraint. Locate the downgrade implementation and its op.alter_column call to ensure the backfill runs first and covers every affected row.alembic_task/versions/0eee8c1abd3a_.py-80-88 (1)
80-88: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winBind the country name in this update.
country["properties"]["NAME"]is interpolated directly into SQL, so a name with an apostrophe will break the migration. Useconn.execute(sa.text(...), {"country": ..., "project_id": ...});conn = op.get_bind()is already available inupgrade().🤖 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 `@alembic_task/versions/0eee8c1abd3a_.py` around lines 80 - 88, In upgrade(), replace the string-built update_country_info SQL and op.execute call with a parameterized sa.text statement executed through the existing conn binding, passing country["properties"]["NAME"] as the country parameter and project_id as the project_id parameter.alembic_task/versions/6276c258149c_.py-74-124 (1)
74-124: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winGuard the downgrade from truncating live data.
downgrade()narrows severalpartnerscolumns. If any existing rows now exceed the old limits, the rollback can fail mid-migration. Add a preflight check or an explicit truncation policy before reducing these lengths.🤖 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 `@alembic_task/versions/6276c258149c_.py` around lines 74 - 124, Update downgrade() to protect existing partners data before narrowing columns: add a preflight query that detects values exceeding each target length and aborts with a clear error, or explicitly implement and document truncation if that is the intended policy. Apply the guard before the batch_alter_table("partners") block and cover permalink, social links, logo_url, hashtags, and name.alembic_task/versions/14340f1e0d6b_.py-107-177 (1)
107-177: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftScope the downgrade to rows created by this migration
Atalembic_task/versions/14340f1e0d6b_.py:117-177, the downgrade still looks up teams and the organisation bynameand restores roles from current membership.alembic_task/versions/e3282e2db2d7_.pydoes not enforce unique names onteamsororganisations, so this can delete the wrong rows or multiple rows, and it can overwrite later role changes for users added/removed after the migration. Persist the affected IDs/original roles inupgrade()and use those exact rows indowngrade().🤖 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 `@alembic_task/versions/14340f1e0d6b_.py` around lines 107 - 177, The downgrade currently identifies teams and organisations by non-unique names and reconstructs roles from mutable membership data. Update the migration’s upgrade() to persist the exact created organisation/team IDs, affected user IDs, and original roles, then revise downgrade() to use those persisted identifiers and restore only those rows and roles; delete only the teams, memberships, projects, and organisation created or modified by this migration, including safe handling when persisted records are absent.alembic_task/versions/14340f1e0d6b_.py-25-55 (1)
25-55: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUse
RETURNING idfor the team inserts. Theorganisationslookup is fine here, butselect id from teams order by id desc limit 1is a global latest-row query and can bind memberships/projects to the wrong team under concurrent inserts.🤖 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 `@alembic_task/versions/14340f1e0d6b_.py` around lines 25 - 55, Replace the global latest-team queries after the team inserts in the migration with INSERT statements using RETURNING id, and assign the returned scalar values directly to validator_team_id and project_manager_team_id. Update both create_validator_team and create_pm_team execution paths while leaving the organisations lookup unchanged.alembic_task/versions/b5b8370f9262_.py-32-38 (1)
32-38: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winJoin table
project_priority_areaslacks a primary key and has nullable FK columns.Without a composite primary key on (
project_id,priority_area_id), duplicate associations can be inserted. Both FK columns beingnullable=Truealso allows meaningless rows with NULL references. If this migration has already been applied, a follow-up migration is needed to add the PK and set the columns non-nullable.🛡️ Proposed fix (if migration not yet applied)
op.create_table( "project_priority_areas", - sa.Column("project_id", sa.Integer(), nullable=True), - sa.Column("priority_area_id", sa.Integer(), nullable=True), + sa.Column("project_id", sa.Integer(), nullable=False), + sa.Column("priority_area_id", sa.Integer(), nullable=False), sa.ForeignKeyConstraint(["priority_area_id"], ["priority_areas.id"]), sa.ForeignKeyConstraint(["project_id"], ["projects.id"]), + sa.PrimaryKeyConstraint("project_id", "priority_area_id"), )🤖 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 `@alembic_task/versions/b5b8370f9262_.py` around lines 32 - 38, Update the project_priority_areas table definition to make project_id and priority_area_id non-nullable and add a composite primary key covering both columns, preventing duplicate or incomplete associations. If this migration may already have been applied, create a follow-up migration to alter the existing columns and add the composite primary key.alembic_task/versions/c5121cbba124_.py-35-47 (1)
35-47: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDowngrade sets
nullable=Falsewithout handling existing NULL values.After the upgrade makes
definitionnullable, any rows with NULLdefinitionwill cause the downgrade'sALTER COLUMN ... SET NOT NULLto fail. The downgrade should backfill or delete NULL rows before enforcing the NOT NULL constraint.🛡️ Proposed fix for downgrade
def downgrade(): # ### commands auto generated by Alembic - please adjust! ### + op.execute("DELETE FROM workspaces_long_quests WHERE definition IS NULL") with op.batch_alter_table("workspaces_long_quests", schema=None) as batch_op: batch_op.alter_column("definition", existing_type=sa.VARCHAR(), nullable=False) + op.execute("DELETE FROM workspaces_imagery WHERE definition IS NULL") with op.batch_alter_table("workspaces_imagery", schema=None) as batch_op: batch_op.alter_column( "definition", existing_type=postgresql.JSON(astext_type=sa.Text()), nullable=False, )🤖 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 `@alembic_task/versions/c5121cbba124_.py` around lines 35 - 47, Update the downgrade function to handle existing NULL values in the definition columns before altering either column to nullable=False: backfill them with appropriate defaults or remove affected rows using migration-safe SQL, then retain the existing alter_column calls in the workspaces_long_quests and workspaces_imagery batch operations.alembic_task/versions/251a7638da78_.py-30-37 (1)
30-37: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDefine a safe rollback policy for messages exceeding 250 characters.
After this upgrade permits longer messages, narrowing the column can fail during rollback. Add a preflight check and an explicit archive/truncation policy rather than discovering this mid-deployment.
🤖 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 `@alembic_task/versions/251a7638da78_.py` around lines 30 - 37, Update downgrade() to preflight-check project_chat.message for values exceeding 250 characters before narrowing the column; apply an explicit, documented archive or truncation policy for those messages, then perform op.alter_column only after ensuring all values satisfy the limit.alembic_task/versions/bfcf4182dcb5_.py-26-34 (1)
26-34: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winResolve slug collisions before creating the unique constraint.
Distinct names can produce identical or empty slugs—for example, punctuation and capitalization variants. In that case the unique constraint fails and blocks deployment. Generate deterministic fallbacks such as an organization-ID suffix.
🤖 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 `@alembic_task/versions/bfcf4182dcb5_.py` around lines 26 - 34, The migration’s slug backfill can create duplicate or empty values before organisations_slug_key is added. Update the organisations iteration and slug generation to resolve collisions deterministically, using an organization-ID-based suffix (and ensuring the result is non-empty), while preserving already-unique slugs before creating the constraint.alembic_task/versions/7937dae319b5_.py-22-35 (1)
22-35: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftRemove the live third-party dependency from the migration.
Fresh deployments now depend on Nominatim availability and mutable responses. The request also has no timeout or status/schema handling, so it can hang or abort the entire migration. Commit a deterministic country mapping or perform this backfill separately before deployment.
🤖 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 `@alembic_task/versions/7937dae319b5_.py` around lines 22 - 35, Remove the Nominatim request from upgrade() and replace the live country lookup with a committed, deterministic country-name mapping used to backfill existing project records; alternatively, move the backfill into a separately runnable pre-deployment task. Ensure the migration completes without network access or mutable third-party responses.Source: Linters/SAST tools
alembic_task/versions/bfcf4182dcb5_.py-26-32 (1)
26-32: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winUpdate organizations by ID with bound parameters.
The raw query embeds organization names, while the helper escapes only the first apostrophe. Select
id, nameand bind the generated slug and ID; then removehandle_special_chars().Proposed query structure
- orgs = conn.execute(sa.text("select name from organisations;")) - for org in orgs: - name = handle_special_chars(org[0]) - query = ( - f"UPDATE organisations SET slug = '{slugify(name)}' WHERE name = '{name}';" - ) - op.execute(query) + orgs = conn.execute(sa.text("SELECT id, name FROM organisations")) + for organisation_id, name in orgs: + conn.execute( + sa.text( + "UPDATE organisations SET slug = :slug WHERE id = :organisation_id" + ), + { + "slug": slugify(name), + "organisation_id": organisation_id, + }, + )Also applies to: 45-49
🤖 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 `@alembic_task/versions/bfcf4182dcb5_.py` around lines 26 - 32, Update the migration logic that iterates over organisations to select both id and name, remove handle_special_chars(), and update each row by its ID using bound parameters for the generated slug and organization ID instead of interpolating names into raw SQL. Apply the same change to the additional affected block.Source: Linters/SAST tools
alembic_task/versions/7937dae319b5_.py-36-44 (1)
36-44: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winBind country values instead of constructing privileged SQL.
Both values can contain quotes or SQL syntax, while
handle_special_chars()escapes only the first apostrophe. Parameter binding avoids malformed queries and prevents database-controlled or external values from changing query semantics.Also applies to: 51-55
🤖 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 `@alembic_task/versions/7937dae319b5_.py` around lines 36 - 44, Replace the dynamically concatenated SQL in the migration’s `update_project` logic with a parameterized `sa.text` statement, binding `updated_country_name` and `country` as query parameters; remove reliance on `handle_special_chars()` for SQL safety and apply the same change to the other occurrence noted in lines 51–55.Source: Linters/SAST tools
alembic_task/versions/c40e1fdf6b70_.py-46-47 (1)
46-47: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winValidate and constrain permission values before dropping the legacy fields.
The mapping can return
None, which is written as'None'to an integer column. The new nullable, unconstrained columns also allow later values that make downgrade fail at.split(). Reject invalid legacy rows, then addNOT NULLandCHECKconstraints for the supported ranges before dropping the source columns.Proposed safeguards
validation_permission = d.determine_validation_permission( validation_restriction ) + if mapping_permission is None or validation_permission is None: + raise RuntimeError( + f"Project {project_id} contains unsupported permission values" + ) update_query = (After the backfill:
op.alter_column("projects", "mapping_permission", nullable=False) op.alter_column("projects", "validation_permission", nullable=False) op.create_check_constraint( "ck_projects_mapping_permission", "projects", "mapping_permission IN (0, 1)", ) op.create_check_constraint( "ck_projects_validation_permission", "projects", "validation_permission IN (0, 1, 2, 3)", )Also applies to: 59-80, 109-118
🤖 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 `@alembic_task/versions/c40e1fdf6b70_.py` around lines 46 - 47, Validate the backfilled values before dropping legacy permission columns: reject rows where the mapping returns None or produces unsupported values, and ensure invalid data cannot be written. In the migration’s permission backfill logic, then make projects.mapping_permission and projects.validation_permission non-nullable and add CHECK constraints allowing only 0–1 and 0–3 respectively, before removing the source columns; apply the same safeguards to the downgrade/relevant mirrored sections.alembic_task/versions/7bbc01082457_.py-199-260 (1)
199-260: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftParameterize all organisation queries.
Names and tags are interpolated into privileged SQL, and the manual escaping handles only one apostrophe. A crafted or simply unusual organisation name can break the migration or alter its query semantics.
Also applies to: 283-293
🤖 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 `@alembic_task/versions/7bbc01082457_.py` around lines 199 - 260, Parameterize every SQL statement in the organisation migration logic, including the queries in the shown block and the additional statements around the referenced lines. Replace string concatenation in the select, insert, update, manager lookup, and manager insertion statements with named bind parameters passed through sa.text().bindparams or execute keyword arguments; remove the manual apostrophe escaping. Preserve existing value and ID handling while ensuring organisation names, tags, and IDs are never interpolated into SQL.Source: Linters/SAST tools
alembic_task/versions/7bbc01082457_.py-181-186 (1)
181-186: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDon't gate the migration on
SELECTrowcount.org_tags.rowcountcan be-1here, socount == total_orgsnever passes andorganisation_tagis left behind. Materialize the distinct rows and uselen(...), or switch toCOUNT(*).🤖 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 `@alembic_task/versions/7bbc01082457_.py` around lines 181 - 186, The migration incorrectly relies on org_tags.rowcount, which may be -1 for SELECT queries. In the migration logic around org_tags, materialize the distinct organisation_tag results and use len(...) for total_orgs, or replace the query with a COUNT(*) query so the subsequent cleanup condition works reliably.
🟡 Minor comments (1)
alembic_task/versions/fcd9cebaa79c_.py-4-14 (1)
4-14: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocstring
Revisesdoes not match actualdown_revision.The docstring says
Revises: d77ee40254f1but the code setsdown_revision = "a8a7537985c0". This mismatch will confuse anyone tracing the migration chain. Update the docstring to reflect the actual revision.📝 Proposed fix
-Revises: d77ee40254f1 +Revises: a8a7537985c0🤖 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 `@alembic_task/versions/fcd9cebaa79c_.py` around lines 4 - 14, Update the migration module’s header docstring so its “Revises” value matches the actual down_revision identifier "a8a7537985c0", keeping it consistent with revision "fcd9cebaa79c".
🧹 Nitpick comments (7)
README.md (2)
17-17: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSpecify a language for the fenced code block.
The ASCII topology diagram uses a bare
```fence. Add a language hint (e.g.,text) to satisfy markdownlint MD040 and improve rendering in markdown viewers.🔧 Proposed fix
-``` +```text client (TDEI/Keycloak JWT)🤖 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 `@README.md` at line 17, Update the fenced ASCII topology code block in the README to use a language hint, such as ```text, on its opening fence while leaving the diagram content unchanged.Source: Linters/SAST tools
71-71: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor: "outside of pytest" → "outside pytest".
LanguageTool flags "outside of" as redundant. Trivial wording improvement.
🤖 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 `@README.md` at line 71, Replace the phrase “outside of pytest” with “outside pytest” in the startup description.Source: Linters/SAST tools
alembic_task/versions/22e7d7e0fa02_.py (1)
56-62: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove unused
idkeys frominitial_categories.The
idfield in each category dict is never used — onlynameis passed to the insert. This is dead data that could confuse future readers into thinking the ids are explicitly set.♻️ Proposed refactor
initial_categories = [ - {"id": 1, "name": "Missed Feature(s)"}, - {"id": 2, "name": "Feature Geometry"}, + "Missed Feature(s)", + "Feature Geometry", ] - for category in initial_categories: - op.execute(categories_table.insert().values(name=category["name"])) + for name in initial_categories: + op.execute(categories_table.insert().values(name=name))🤖 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 `@alembic_task/versions/22e7d7e0fa02_.py` around lines 56 - 62, Remove the unused "id" entries from each dictionary in the initial_categories collection, retaining only the "name" values consumed by the insert loop.alembic_task/versions/fcd9cebaa79c_.py (1)
22-22: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
sa.inspect()instead of deprecatedInspector.from_engine.
sa.engine.reflection.Inspector.from_engineis deprecated in SQLAlchemy 2.0. The project targets SQLAlchemy 2.0.36, so prefersa.inspect(conn)which is the supported replacement.♻️ Proposed refactor
- inspector = sa.engine.reflection.Inspector.from_engine(conn) + inspector = sa.inspect(conn)#!/bin/bash # Verify SQLAlchemy 2.0 deprecation of Inspector.from_engine rg -n "from_engine" --type=python alembic_task/ rg -n "sa.inspect" --type=python alembic_task/🤖 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 `@alembic_task/versions/fcd9cebaa79c_.py` at line 22, Replace the deprecated sa.engine.reflection.Inspector.from_engine(conn) call with sa.inspect(conn), preserving the existing inspector usage in the migration.alembic_task/versions/7d55a089b5bc_.py (1)
37-44: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winUse parameterized queries instead of string concatenation.
Lines 37 and 41–43 build SQL via string concatenation with
str(project.id). While the value originates from the database, this pattern is flagged by static analysis (S608) and is fragile. Usesa.text()with bind parameters, and standardize onconn.execute(line 44 currently usesop.execute).♻️ Proposed refactor
for project in projects: - query = "select distinct zoom from tasks where project_id = " + str(project.id) - zooms = conn.execute(sa.text(query)).fetchall() + zooms = conn.execute( + sa.text("select distinct zoom from tasks where project_id = :pid"), + {"pid": project.id}, + ).fetchall() if len(zooms) == 1 and zooms[0] == (None,): - query = "update projects set task_creation_mode = 1 where id = " + str( - project.id - ) - op.execute(sa.text(query)) + conn.execute( + sa.text("update projects set task_creation_mode = 1 where id = :pid"), + {"pid": project.id}, + )🤖 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 `@alembic_task/versions/7d55a089b5bc_.py` around lines 37 - 44, Replace the concatenated SQL in the migration’s zoom query and project update with sa.text() statements using a named bind parameter for project.id; execute both through conn.execute, including the update currently using op.execute, while preserving the existing condition and behavior.Source: Linters/SAST tools
alembic_task/versions/51ccc46f1b8a_.py (1)
21-30: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueConsider adding
ondeletecascade to the FK constraint.The FK from
project_custom_editors.project_idtoprojects.idlacks anondeleteclause. Sinceproject_idis also the primary key (one-to-one), deleting a project will fail if a matching custom editor row exists. Consider addingondelete="CASCADE"if the intent is that deleting a project should also remove its custom editor configuration.🤖 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 `@alembic_task/versions/51ccc46f1b8a_.py` around lines 21 - 30, Add ondelete="CASCADE" to the ForeignKeyConstraint in the project_custom_editors table definition so deleting a projects row automatically removes its associated custom editor configuration.alembic_task/versions/5eba4824505e_.py (1)
21-30: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRemove the redundant index on the primary key.
users_with_email.idalready has a primary-key index, soix_users_with_email_idduplicates the same access path while adding storage and write-maintenance cost. Keep the primary key and drop the explicitcreate_index/drop_indexcalls unless a database-specific requirement justifies both.🤖 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 `@alembic_task/versions/5eba4824505e_.py` around lines 21 - 30, Remove the redundant op.create_index call for users_with_email.id in the migration, and remove the corresponding op.drop_index call from its downgrade logic while retaining the primary key constraint.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b6264ded-1994-4497-8da2-9b4018de2c95
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (93)
.gitignoreREADME.mdalembic_task/env.pyalembic_task/versions/05e1ecf9953a_.pyalembic_task/versions/05f1b650ddbc_.pyalembic_task/versions/068674f06b0f_.pyalembic_task/versions/073a96d114ab_.pyalembic_task/versions/0a6b82b55983_.pyalembic_task/versions/0aaac86a48dc_.pyalembic_task/versions/0d47c10c8a66_.pyalembic_task/versions/0eeaa5aed53b_.pyalembic_task/versions/0eee8c1abd3a_.pyalembic_task/versions/14340f1e0d6b_.pyalembic_task/versions/14842761654b_.pyalembic_task/versions/1579d24928e7_.pyalembic_task/versions/22e7d7e0fa02_.pyalembic_task/versions/22e9a0e9b254_.pyalembic_task/versions/251a7638da78_.pyalembic_task/versions/279f8d753529_.pyalembic_task/versions/29097876c7e6_.pyalembic_task/versions/2cfa981c70e7_.pyalembic_task/versions/2d6a7ad4e1eb_.pyalembic_task/versions/2ee4bca188a9_.pyalembic_task/versions/30b091260689_.pyalembic_task/versions/3b8b0956b217_.pyalembic_task/versions/3ee58dee57c9_.pyalembic_task/versions/42c45e74752b_.pyalembic_task/versions/451f6bd05a19_.pyalembic_task/versions/48c3aed123cf_.pyalembic_task/versions/4b9fb4d3f349_.pyalembic_task/versions/4ed992117718_.pyalembic_task/versions/5053c01cb170_.pyalembic_task/versions/51ccc46f1b8a_.pyalembic_task/versions/55790eeec3bf_.pyalembic_task/versions/567b75e74955_.pyalembic_task/versions/5952780a577e_.pyalembic_task/versions/5eba4824505e_.pyalembic_task/versions/6276c258149c_.pyalembic_task/versions/64b682d53e23_.pyalembic_task/versions/6612e4d6524c_.pyalembic_task/versions/7435b0a865e6_.pyalembic_task/versions/772aff899389_.pyalembic_task/versions/7937dae319b5_.pyalembic_task/versions/7991a7e1b88d_.pyalembic_task/versions/7bbc01082457_.pyalembic_task/versions/7d55a089b5bc_.pyalembic_task/versions/824268a7a675_.pyalembic_task/versions/83eb4cbb0abd_.pyalembic_task/versions/84c793a951b2_.pyalembic_task/versions/86c0f6b6a176_.pyalembic_task/versions/8a6419f289aa_.pyalembic_task/versions/8aa8f8d6a0c3_.pyalembic_task/versions/8b61ac59bc57_.pyalembic_task/versions/8f51fb7f4e40_.pyalembic_task/versions/924a63857df4_.pyalembic_task/versions/9712d29e24c8_.pyalembic_task/versions/9f5b73af01db_.pyalembic_task/versions/a43b9748ceee_.pyalembic_task/versions/a60315eb2654_.pyalembic_task/versions/a7c617755721_.pyalembic_task/versions/a8a7537985c0_.pyalembic_task/versions/a9cbd2c6c213_.pyalembic_task/versions/ac55902fcc3d_.pyalembic_task/versions/ae084e0b68b2_changed_imagery_definition_type_to_json.pyalembic_task/versions/b25f088d40c2_.pyalembic_task/versions/b5b8370f9262_.pyalembic_task/versions/ba778ef9c615_.pyalembic_task/versions/badf8bb7d56b_.pyalembic_task/versions/bcb474128817_.pyalembic_task/versions/bcdb7875bd1c_.pyalembic_task/versions/bfcf4182dcb5_.pyalembic_task/versions/c40e1fdf6b70_.pyalembic_task/versions/c4670f3bb828_.pyalembic_task/versions/c5121cbba124_.pyalembic_task/versions/c5121cbba124_initial_task_schema.pyalembic_task/versions/cbc419d1740c_.pyalembic_task/versions/d2e18f0f34a9_.pyalembic_task/versions/d77ee40254f1_.pyalembic_task/versions/dc250d726600_.pyalembic_task/versions/dd99815a393c_.pyalembic_task/versions/deec8123583d_.pyalembic_task/versions/e0741bdfde1d_.pyalembic_task/versions/e3282e2db2d7_.pyalembic_task/versions/e36f1d0c4947_.pyalembic_task/versions/e8ffa33a9c18_.pyalembic_task/versions/ee46f5e8723b_.pyalembic_task/versions/ee5315dcf3e1_.pyalembic_task/versions/f26a7c36eb65_.pyalembic_task/versions/f547382df31b_.pyalembic_task/versions/f86698c827cc_.pyalembic_task/versions/fb96ef86e2f6_.pyalembic_task/versions/fcd9cebaa79c_.pypyproject.toml
💤 Files with no reviewable changes (1)
- alembic_task/versions/c5121cbba124_initial_task_schema.py
| op.create_table( | ||
| "workspaces_imagery", | ||
| sa.Column("workspace_id", sa.Integer(), nullable=False), | ||
| sa.Column("definition", sa.Unicode(), nullable=False), |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
Column type mismatch: definition should be sa.JSON(), not sa.Unicode().
The upstream model WorkspaceImagery in api/src/workspaces/schemas.py defines definition as Column(SAJson, nullable=False), but this migration creates it as sa.Unicode() (VARCHAR/TEXT). The ORM will attempt JSON serialization/deserialization on a text column, causing runtime errors and losing PostgreSQL JSON validation and query capabilities.
🔧 Proposed fix
- sa.Column("definition", sa.Unicode(), nullable=False),
+ sa.Column("definition", sa.JSON(), nullable=False),📝 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.
| sa.Column("definition", sa.Unicode(), nullable=False), | |
| sa.Column("definition", sa.JSON(), nullable=False), |
🤖 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 `@alembic_task/versions/073a96d114ab_.py` at line 24, Update the `definition`
column declaration in the migration to use `sa.JSON()` instead of
`sa.Unicode()`, matching the `WorkspaceImagery` model’s JSON type and preserving
database-level JSON behavior.
| project = conn.execute( | ||
| sa.text("select * from projects where id = " + str(project_id)) | ||
| ).first() | ||
| project_existence[project_id] = project is not None | ||
|
|
||
| # Only process messages from projects that still exist | ||
| if project_existence[project_id]: | ||
| query = ( | ||
| "update messages " | ||
| + "set message_type=" | ||
| + str(message_type) | ||
| + ", project_id=" | ||
| + str(project_id) | ||
| + ", task_id=" | ||
| + str(task_id) | ||
| + " where id = " | ||
| + str(message.id) | ||
| ) | ||
|
|
||
| op.execute(query) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
SQL injection via string concatenation in query construction.
Both the project existence check (lines 90-91) and the message update (lines 97-109) build SQL by concatenating values derived from message content. While the regex patterns constrain project_id and task_id to digits, this pattern is unsafe and becomes exploitable if the regex is ever loosened. Use parameterized queries with sa.text().
🔒 Proposed fix using parameterized queries
- if project_id not in project_existence:
- project = conn.execute(
- sa.text("select * from projects where id = " + str(project_id))
- ).first()
- project_existence[project_id] = project is not None
+ if project_id not in project_existence:
+ project = conn.execute(
+ sa.text("select * from projects where id = :pid"),
+ {"pid": int(project_id) if project_id != "null" else None},
+ ).first()
+ project_existence[project_id] = project is not None
- query = (
- "update messages "
- + "set message_type="
- + str(message_type)
- + ", project_id="
- + str(project_id)
- + ", task_id="
- + str(task_id)
- + " where id = "
- + str(message.id)
- )
-
- op.execute(query)
+ query = sa.text(
+ "update messages set message_type = :mt, project_id = :pid, "
+ "task_id = :tid where id = :id"
+ )
+ params = {
+ "mt": int(message_type) if message_type != "null" else None,
+ "pid": int(project_id) if project_id != "null" else None,
+ "tid": int(task_id) if task_id != "null" else None,
+ "id": message.id,
+ }
+ op.execute(query, params)📝 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.
| project = conn.execute( | |
| sa.text("select * from projects where id = " + str(project_id)) | |
| ).first() | |
| project_existence[project_id] = project is not None | |
| # Only process messages from projects that still exist | |
| if project_existence[project_id]: | |
| query = ( | |
| "update messages " | |
| + "set message_type=" | |
| + str(message_type) | |
| + ", project_id=" | |
| + str(project_id) | |
| + ", task_id=" | |
| + str(task_id) | |
| + " where id = " | |
| + str(message.id) | |
| ) | |
| op.execute(query) | |
| project = conn.execute( | |
| sa.text("select * from projects where id = :pid"), | |
| {"pid": int(project_id) if project_id != "null" else None}, | |
| ).first() | |
| project_existence[project_id] = project is not None | |
| # Only process messages from projects that still exist | |
| if project_existence[project_id]: | |
| query = sa.text( | |
| "update messages set message_type = :mt, project_id = :pid, " | |
| "task_id = :tid where id = :id" | |
| ) | |
| params = { | |
| "mt": int(message_type) if message_type != "null" else None, | |
| "pid": int(project_id) if project_id != "null" else None, | |
| "tid": int(task_id) if task_id != "null" else None, | |
| "id": message.id, | |
| } | |
| op.execute(query, params) |
🧰 Tools
🪛 Ruff (0.15.20)
[error] 91-91: Possible SQL injection vector through string-based query construction
(S608)
🤖 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 `@alembic_task/versions/0a6b82b55983_.py` around lines 90 - 109, Replace the
concatenated SQL in the project existence check and message update with
parameterized sa.text() statements. Bind project_id in the projects lookup, and
bind message_type, project_id, task_id, and message.id in the update executed
through op.execute, preserving the existing logic and project_existence
behavior.
Source: Linters/SAST tools
| import json | ||
| import sys | ||
|
|
||
| import shapely.wkt | ||
| import sqlalchemy as sa | ||
| from alembic import op | ||
| from shapely.geometry import shape | ||
| from sqlalchemy.dialects.postgresql import ARRAY |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Include the world data files required by this migration.
Line 42 immediately opens scripts/world/continents.json, but the PR branch's scripts directory contains only ci.sh and fix-lint.sh. This upgrade therefore fails with FileNotFoundError. Also resolve resources relative to the migration rather than the caller's working directory. (github.com)
Proposed path handling
import json
import sys
+from pathlib import Path
+WORLD_DATA_DIR = Path(__file__).resolve().parents[2] / "scripts" / "world"
+
- with open("scripts/world/continents.json") as continents_data:
+ with (WORLD_DATA_DIR / "continents.json").open(encoding="utf-8") as continents_data:
continents = json.load(continents_data)
- with open(
- "scripts/world/" + project_continent + ".json",
- "r",
- encoding="utf-8",
- ) as countries_data:
+ with (WORLD_DATA_DIR / f"{project_continent}.json").open(
+ encoding="utf-8"
+ ) as countries_data:Also applies to: 42-70
🤖 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 `@alembic_task/versions/0eee8c1abd3a_.py` around lines 9 - 16, Include the
continents and other world data files required by the migration in the
repository, and update the migration’s resource loading logic to resolve paths
relative to its own file location rather than the process working directory. In
the migration upgrade logic and any helper reading world data, use the migration
module’s directory as the base path before opening the JSON files.
| op.execute(""" | ||
| DROP TRIGGER IF EXISTS tsvectorupdate ON project_info; | ||
| CREATE TRIGGER tsvectorupdate BEFORE INSERT OR UPDATE ON project_info FOR EACH ROW EXECUTE PROCEDURE | ||
| tsvector_update_trigger(text_searchable, "pg_catalog.english", project_id_str, short_description, description) | ||
| """) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Split multiple SQL statements into separate op.execute() calls.
The CI pipeline confirms this migration fails with asyncpg: cannot insert multiple commands into a prepared statement. The DROP TRIGGER and CREATE TRIGGER must be executed as separate op.execute() calls.
🔧 Proposed fix
def upgrade():
- op.execute("""
- DROP TRIGGER IF EXISTS tsvectorupdate ON project_info;
- CREATE TRIGGER tsvectorupdate BEFORE INSERT OR UPDATE ON project_info FOR EACH ROW EXECUTE PROCEDURE
- tsvector_update_trigger(text_searchable, "pg_catalog.english", project_id_str, short_description, description)
- """)
+ op.execute("DROP TRIGGER IF EXISTS tsvectorupdate ON project_info")
+ op.execute(
+ """
+ CREATE TRIGGER tsvectorupdate BEFORE INSERT OR UPDATE ON project_info
+ FOR EACH ROW EXECUTE PROCEDURE
+ tsvector_update_trigger(text_searchable, "pg_catalog.english", project_id_str, short_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.
| op.execute(""" | |
| DROP TRIGGER IF EXISTS tsvectorupdate ON project_info; | |
| CREATE TRIGGER tsvectorupdate BEFORE INSERT OR UPDATE ON project_info FOR EACH ROW EXECUTE PROCEDURE | |
| tsvector_update_trigger(text_searchable, "pg_catalog.english", project_id_str, short_description, description) | |
| """) | |
| op.execute("DROP TRIGGER IF EXISTS tsvectorupdate ON project_info") | |
| op.execute( | |
| """ | |
| CREATE TRIGGER tsvectorupdate BEFORE INSERT OR UPDATE ON project_info | |
| FOR EACH ROW EXECUTE PROCEDURE | |
| tsvector_update_trigger(text_searchable, "pg_catalog.english", project_id_str, short_description, description) | |
| """ | |
| ) |
🧰 Tools
🪛 GitHub Actions: CI / 0_ci (3.12).txt
[error] 19-19: Alembic migration failed during upgrade: cannot insert multiple commands into a prepared statement (asyncpg PostgresSyntaxError). op.execute() contains multiple SQL commands at once: "DROP TRIGGER IF EXISTS tsvectorupdate ON project_info; CREATE TRIGGER tsvectorupdate BEFORE INSERT OR UPDATE ON project_info FOR EACH ROW EXECUTE PROCEDURE tsvector_update_trigger(...)".
🪛 GitHub Actions: CI / ci (3.12)
[error] 19-19: Alembic upgrade failed during op.execute() with PostgreSQL/asyncpg error: cannot insert multiple commands into a prepared statement. The executed SQL contains multiple statements: "DROP TRIGGER IF EXISTS tsvectorupdate ON project_info; CREATE TRIGGER tsvectorupdate ...".
🤖 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 `@alembic_task/versions/451f6bd05a19_.py` around lines 19 - 23, Split the SQL
in the migration’s trigger setup into two separate op.execute() calls: one for
DROP TRIGGER IF EXISTS and one for CREATE TRIGGER, preserving the existing
statements and trigger configuration.
Source: Pipeline failures
| update_project = ( | ||
| "update projects set country = '{\"" | ||
| + handle_special_chars(updated_country_name) | ||
| + "\"}' where country @> ARRAY['" | ||
| + handle_special_chars(country) | ||
| + "']::varchar[]" | ||
| + ";" | ||
| ) | ||
| conn.execute(sa.text(update_project)) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
Preserve every other value in the country array.
SET country = '{"…"}' replaces the entire array for projects containing the matched country. A project with multiple countries silently loses all other entries. Replace only the matching element, for example with PostgreSQL array_replace.
Proposed update
- update_project = (
- "update projects set country = '{\""
- + handle_special_chars(updated_country_name)
- + "\"}' where country @> ARRAY['"
- + handle_special_chars(country)
- + "']::varchar[]"
- + ";"
- )
- conn.execute(sa.text(update_project))
+ conn.execute(
+ sa.text(
+ """
+ UPDATE projects
+ SET country = array_replace(
+ country,
+ CAST(:old_country AS varchar),
+ CAST(:new_country AS varchar)
+ )
+ WHERE CAST(:old_country AS varchar) = ANY(country)
+ """
+ ),
+ {
+ "old_country": country,
+ "new_country": updated_country_name,
+ },
+ )📝 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.
| update_project = ( | |
| "update projects set country = '{\"" | |
| + handle_special_chars(updated_country_name) | |
| + "\"}' where country @> ARRAY['" | |
| + handle_special_chars(country) | |
| + "']::varchar[]" | |
| + ";" | |
| ) | |
| conn.execute(sa.text(update_project)) | |
| conn.execute( | |
| sa.text( | |
| """ | |
| UPDATE projects | |
| SET country = array_replace( | |
| country, | |
| CAST(:old_country AS varchar), | |
| CAST(:new_country AS varchar) | |
| ) | |
| WHERE CAST(:old_country AS varchar) = ANY(country) | |
| """ | |
| ), | |
| { | |
| "old_country": country, | |
| "new_country": updated_country_name, | |
| }, | |
| ) |
🧰 Tools
🪛 Ruff (0.15.20)
[error] 37-42: Possible SQL injection vector through string-based query construction
(S608)
🤖 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 `@alembic_task/versions/7937dae319b5_.py` around lines 36 - 44, Update the SQL
built in the migration’s update_project statement to replace only the matching
country element using PostgreSQL array_replace(country, matched_country,
updated_country), preserving all other array values. Continue escaping both
country names with handle_special_chars and retain the existing filtering
condition and execution through conn.execute(sa.text(...)).
| def downgrade(): | ||
| conn = op.get_bind() | ||
| op.add_column("projects", sa.Column("organisation_tag", sa.String(), nullable=True)) | ||
| # Remove all mappings made | ||
| org_ids = conn.execute(sa.text("select id, name from organisations")) | ||
| for org_id, org_name in org_ids: | ||
| quote_index = org_name.find("'") | ||
| if quote_index > -1: | ||
| org_name = org_name[:quote_index] + "'" + org_name[quote_index:] | ||
| conn.execute( | ||
| sa.text( | ||
| "update projects set organisation_tag='" | ||
| + str(org_name) | ||
| + "' where organisation_id=" | ||
| + str(org_id) | ||
| ) | ||
| ) | ||
| conn.execute(sa.text("delete from project_teams where team_id is not null")) | ||
| conn.execute(sa.text("delete from team_members where team_id is not null")) | ||
| conn.execute(sa.text("delete from teams where organisation_id is not null")) | ||
| conn.execute( | ||
| sa.text("delete from organisation_managers where organisation_id is not null") | ||
| ) | ||
| conn.execute( | ||
| sa.text( | ||
| "update projects set organisation_id = null where organisation_id is not null" | ||
| ) | ||
| ) | ||
| conn.execute(sa.text("delete from organisations where name is not null")) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy lift
Restrict downgrade deletion to records introduced by this migration.
The downgrade deletes all organization managers and every non-null-named organization, plus broad team data—not merely rows created by upgrade(). Rolling back would destroy pre-existing and subsequently created records and cannot restore original organization tags.
🧰 Tools
🪛 Ruff (0.15.20)
[error] 288-291: Possible SQL injection vector through string-based query construction
(S608)
🤖 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 `@alembic_task/versions/7bbc01082457_.py` around lines 277 - 305, Restrict
downgrade cleanup to rows created by this migration rather than deleting all
matching records. Update downgrade() to track or identify the organizations,
teams, memberships, mappings, and managers introduced by upgrade(), restore
affected project organization_id and organisation_tag values from pre-migration
state, and delete only those tracked rows; avoid broad conditions such as
non-null organisation_id or name.
| with op.batch_alter_table("workspace_imagery", schema=None) as batch_op: | ||
| batch_op.drop_constraint( | ||
| "workspace_imagery_workspace_id_fkey", type_="foreignkey" | ||
| ) | ||
| batch_op.create_foreign_key( | ||
| "workspace_imagery_workspace_id_fkey", | ||
| "workspaces", | ||
| ["workspace_id"], | ||
| ["id"], | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
Table name mismatch in downgrade: workspace_imagery should be workspaces_imagery.
The upgrade modifies the workspaces_imagery table (line 33), but the downgrade references workspace_imagery (singular, line 61) and its constraint workspace_imagery_workspace_id_fkey (line 63). Since the table is named workspaces_imagery (plural), the downgrade will fail at runtime, making this migration non-reversible.
🐛 Proposed fix: correct table and constraint names in downgrade
- with op.batch_alter_table("workspace_imagery", schema=None) as batch_op:
+ with op.batch_alter_table("workspaces_imagery", schema=None) as batch_op:
batch_op.drop_constraint(
- "workspace_imagery_workspace_id_fkey", type_="foreignkey"
+ "workspaces_imagery_workspace_id_fkey", type_="foreignkey"
)
batch_op.create_foreign_key(
- "workspace_imagery_workspace_id_fkey",
+ "workspaces_imagery_workspace_id_fkey",
"workspaces",
["workspace_id"],
["id"],
)📝 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.
| with op.batch_alter_table("workspace_imagery", schema=None) as batch_op: | |
| batch_op.drop_constraint( | |
| "workspace_imagery_workspace_id_fkey", type_="foreignkey" | |
| ) | |
| batch_op.create_foreign_key( | |
| "workspace_imagery_workspace_id_fkey", | |
| "workspaces", | |
| ["workspace_id"], | |
| ["id"], | |
| ) | |
| with op.batch_alter_table("workspaces_imagery", schema=None) as batch_op: | |
| batch_op.drop_constraint( | |
| "workspaces_imagery_workspace_id_fkey", type_="foreignkey" | |
| ) | |
| batch_op.create_foreign_key( | |
| "workspaces_imagery_workspace_id_fkey", | |
| "workspaces", | |
| ["workspace_id"], | |
| ["id"], | |
| ) |
🤖 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 `@alembic_task/versions/dd99815a393c_.py` around lines 61 - 70, Correct the
downgrade block to consistently use the plural table name and foreign-key
constraint: update the batch_alter_table target and both constraint references
in the downgrade function to workspaces_imagery and
workspaces_imagery_workspace_id_fkey, matching the upgrade migration.
| campaign_tags = conn.execute( | ||
| sa.text("select distinct(campaigns) from tags where campaigns is not null") | ||
| ) | ||
| total_campaigns = campaign_tags.rowcount | ||
|
|
||
| print("Total distinct campaigns in the DB: " + str(total_campaigns)) | ||
| for campaign_tag in campaign_tags: | ||
| campaign_tag = campaign_tag[0] | ||
| conn.execute("insert into campaigns (name) values ('" + campaign_tag + "')") | ||
| new_campaign_id = conn.execute( | ||
| "select id from campaigns where name ='" + campaign_tag + "'" | ||
| ).scalar() | ||
| projects = conn.execute( | ||
| "select id from projects " + " where campaign_tag='" + campaign_tag + "'" | ||
| ) | ||
| for project_id in projects: | ||
| project_id = project_id[0] | ||
| conn.execute( | ||
| sa.text( | ||
| "insert into campaign_projects (campaign_id, project_id) values (" | ||
| + str(new_campaign_id) | ||
| + "," | ||
| + str(project_id) | ||
| + ")" | ||
| ) | ||
| ) | ||
| op.drop_table("tags") | ||
| op.drop_index("ix_projects_campaign_tag", table_name="projects") | ||
| op.drop_column("projects", "campaign_tag") |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy lift
Preserve organization tags before dropping tags.
The old table contains both campaigns and organisations, but this upgrade copies only campaign values before Line 47 drops the entire table. The immediately preceding revision creates organization tables without backfilling this data. Migrate organization values and associations first, or abort when non-null organization tags exist.
🧰 Tools
🪛 Ruff (0.15.20)
[error] 29-29: Possible SQL injection vector through string-based query construction
(S608)
[error] 31-31: Possible SQL injection vector through string-based query construction
(S608)
[error] 34-34: Possible SQL injection vector through string-based query construction
(S608)
[error] 40-44: Possible SQL injection vector through string-based query construction
(S608)
🤖 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 `@alembic_task/versions/f86698c827cc_.py` around lines 21 - 49, The migration
drops the legacy tags table without preserving its organisation data. Update the
upgrade logic alongside the campaign migration to read non-null organisation
tags, create corresponding organisation records, and populate their project
associations using the new organisation tables before op.drop_table("tags");
alternatively, explicitly abort if any organisation tags exist. Reuse the
migration’s existing connection and SQLAlchemy execution patterns, ensuring
organisation values are migrated before the destructive drop.
| conn.execute("insert into campaigns (name) values ('" + campaign_tag + "')") | ||
| new_campaign_id = conn.execute( | ||
| "select id from campaigns where name ='" + campaign_tag + "'" | ||
| ).scalar() | ||
| projects = conn.execute( | ||
| "select id from projects " + " where campaign_tag='" + campaign_tag + "'" | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C2 'sqlalchemy' pyproject.toml
rg -n -C3 'conn\.execute\(' alembic_task/versions/f86698c827cc_.pyRepository: TaskarCenterAtUW/workspaces-backend
Length of output: 2745
🏁 Script executed:
#!/bin/bash
sed -n '20,40p' alembic_task/versions/f86698c827cc_.py
printf '\n---\n'
sed -n '68,92p' alembic_task/versions/f86698c827cc_.pyRepository: TaskarCenterAtUW/workspaces-backend
Length of output: 1955
🏁 Script executed:
#!/bin/bash
python3 - <<'PY'
from pathlib import Path
import re
path = Path("alembic_task/versions/f86698c827cc_.py")
text = path.read_text()
# Show all execute calls and whether they use raw strings or sa.text.
for i, line in enumerate(text.splitlines(), 1):
if "conn.execute(" in line or "sa.text(" in line:
print(f"{i}: {line}")
PYRepository: TaskarCenterAtUW/workspaces-backend
Length of output: 1030
🏁 Script executed:
#!/bin/bash
rg -n --hidden -S "RETURNING|psycopg|postgres|postgresql|sqlalchemy>=2|mysql|sqlite" pyproject.toml alembic.ini alembic_task -g '!**/__pycache__/**'Repository: TaskarCenterAtUW/workspaces-backend
Length of output: 3212
🏁 Script executed:
#!/bin/bash
sed -n '1,130p' alembic_task/versions/f86698c827cc_.pyRepository: TaskarCenterAtUW/workspaces-backend
Length of output: 3605
Parameterize all migration SQL
conn.execute("...")appears in both upgrade and downgrade, and several statements interpolatecampaign_tag/IDs directly. These raw strings won’t execute cleanly under SQLAlchemy 2, and the interpolation will break on quotes. Switch every statement in this revision tosa.text(...), {...}.
🧰 Tools
🪛 Ruff (0.15.20)
[error] 29-29: Possible SQL injection vector through string-based query construction
(S608)
[error] 31-31: Possible SQL injection vector through string-based query construction
(S608)
[error] 34-34: Possible SQL injection vector through string-based query construction
(S608)
🤖 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 `@alembic_task/versions/f86698c827cc_.py` around lines 29 - 35, Parameterize
every SQL statement in both upgrade and downgrade of the migration, replacing
raw string conn.execute calls with sa.text(...) and bound parameter
dictionaries. Update statements involving campaign_tag and IDs, including the
INSERT, campaign lookup, project query, and any remaining migration queries,
while preserving existing behavior and avoiding direct string interpolation.
Source: Linters/SAST tools
| # Add the project root directory to the Python path | ||
| sys.path.insert(0, os.path.abspath(os.path.join(os.path.dirname(__file__), ".."))) | ||
|
|
||
| import geoalchemy2.alembic_helpers # noqa: F401 |
cyrossignol
left a comment
cyrossignol
left a comment
There was a problem hiding this comment.
We don't want to bring all of the HOT tasking manager tables. Those aren't used anywhere at this point.
Ideally, we just move the workspaces tables and install those in the OSM DB so we can migrate and drop the task DB completely. I think @susrisha was already working on that.
Got it, I thought this is what you suggested at the dev call. That's fine, you can discard this then, maybe keep the docs. Or remove the tables we don't need as a final migration? I'll let y'all take over this then. |
2 participants
Copy migrations from the old Tasking Manager to this repo's alembic + add docs
Summary
python-slugifyfor migration backfills.README.md..claudeto.gitignore.