fix(api): add v2.3.2 options.typing migration #1341
Conversation
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests
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: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
api/src/migration/migrations/1756483008605-v-2-2-12.migration.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
api/src/migration/migrations/1756483008605-v-2-2-12.migration.ts (5)
api/src/user/schemas/role.schema.ts (1)
RoleModel(57-60)api/src/user/schemas/user.schema.ts (1)
UserModel(129-132)api/src/migration/types.ts (1)
MigrationServices(39-43)api/src/chat/schemas/block.schema.ts (1)
BlockModel(187-190)api/src/logger/logger.service.ts (1)
error(32-34)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: API-Tests
🔇 Additional comments (2)
api/src/migration/migrations/1756483008605-v-2-2-12.migration.ts (2)
57-71: No action required: CommonJS export style is consistentMatches existing migrations and the root tsconfig.json (
module: "commonjs").
43-49: Harden v-2-2-12 migration conversion and update timestamps.
- Replace
$toInt: '$options.typing'with
$convert: { input: '$options.typing', to: 'int', onError: 0, onNull: 0 }.- Add
updatedAt: '$$NOW'in the$set.- Confirm legacy boolean/string mappings (default to 0 if uncertain).
- Verify MongoDB ≥ 4.2 for pipeline updates and
$convertsupport.
| const getAdminUser = async () => { | ||
| const RoleModel = mongoose.model<Role>(Role.name, roleSchema); | ||
| const UserModel = mongoose.model<User>(User.name, userSchema); | ||
|
|
||
| const adminRole = await RoleModel.findOne({ name: 'admin' }); | ||
| const user = await UserModel.findOne({ roles: [adminRole!._id] }).sort({ | ||
| createdAt: 'asc', | ||
| }); | ||
|
|
||
| return user!; | ||
| }; |
There was a problem hiding this comment.
If kept, fix admin lookup: unsafe non-null and array-equality query.
- Non-null assertion on
adminRole!can throw at runtime. { roles: [adminRole!._id] }matches only exact array equality, not “contains”.
Use containment and guard:
- const adminRole = await RoleModel.findOne({ name: 'admin' });
- const user = await UserModel.findOne({ roles: [adminRole!._id] }).sort({
- createdAt: 'asc',
- });
-
- return user!;
+ const adminRole = await RoleModel.findOne({ name: 'admin' }).lean();
+ if (!adminRole) return null;
+ return UserModel.findOne({ roles: { $in: [adminRole._id] } })
+ .sort({ createdAt: 1 })
+ .lean();📝 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.
| const getAdminUser = async () => { | |
| const RoleModel = mongoose.model<Role>(Role.name, roleSchema); | |
| const UserModel = mongoose.model<User>(User.name, userSchema); | |
| const adminRole = await RoleModel.findOne({ name: 'admin' }); | |
| const user = await UserModel.findOne({ roles: [adminRole!._id] }).sort({ | |
| createdAt: 'asc', | |
| }); | |
| return user!; | |
| }; | |
| const getAdminUser = async () => { | |
| const RoleModel = mongoose.model<Role>(Role.name, roleSchema); | |
| const UserModel = mongoose.model<User>(User.name, userSchema); | |
| const adminRole = await RoleModel.findOne({ name: 'admin' }).lean(); | |
| if (!adminRole) return null; | |
| return UserModel.findOne({ roles: { $in: [adminRole._id] } }) | |
| .sort({ createdAt: 1 }) | |
| .lean(); | |
| }; |
🤖 Prompt for AI Agents
In api/src/migration/migrations/1756483008605-v-2-2-12.migration.ts around lines
20-30, the code unsafely uses a non-null assertion on adminRole and queries
roles with an array equality which only matches exact arrays; instead, guard
against a missing adminRole (e.g., if (!adminRole) throw new Error('admin role
not found') or return null) and change the user lookup to use a containment
match such as await UserModel.findOne({ roles: adminRole._id }).sort({
createdAt: 'asc' }) or explicitly { roles: { $in: [adminRole._id] } }) so you
remove the non-null assertion and correctly find users that have the admin role.
| }; | ||
|
|
||
| const migrateBlockOptionsTyping = async (services: MigrationServices) => { | ||
| const BlockModel = mongoose.model<Block>(Block.name, blockSchema).collection; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prevent Mongoose OverwriteModelError by reusing compiled models.
Repeated mongoose.model(name, schema) may recompile an existing model.
- const BlockModel = mongoose.model<Block>(Block.name, blockSchema).collection;
+ const BlockModel =
+ (mongoose.models[Block.name] as mongoose.Model<Block> | undefined)?.collection ??
+ mongoose.model<Block>(Block.name, blockSchema).collection;If you keep getAdminUser, apply the same pattern for Role/User.
📝 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.
| const BlockModel = mongoose.model<Block>(Block.name, blockSchema).collection; | |
| const BlockModel = | |
| (mongoose.models[Block.name] as mongoose.Model<Block> | undefined)?.collection ?? | |
| mongoose.model<Block>(Block.name, blockSchema).collection; |
🤖 Prompt for AI Agents
In api/src/migration/migrations/1756483008605-v-2-2-12.migration.ts around line
33, calling mongoose.model<Block>(Block.name, blockSchema).collection will throw
OverwriteModelError if the model is already compiled; change to reuse existing
compiled models by checking mongoose.models[Block.name] and using
mongoose.model(Block.name) if present, otherwise compile with the schema, and
then access .collection; apply the same guard/pattern for Role and User (and any
getAdminUser helper) so you never call mongoose.model(name, schema)
unconditionally for a model that may already exist.
| const adminUser = await getAdminUser(); | ||
|
|
||
| if (!adminUser) { | ||
| services.logger.warn('Unable to process block, no admin user found'); | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unnecessary admin-user gating; it can silently skip the migration.
This migration doesn’t depend on a user context. If no admin exists, all updates are skipped, leaving data invalid.
Apply within-range removal:
- const adminUser = await getAdminUser();
-
- if (!adminUser) {
- services.logger.warn('Unable to process block, no admin user found');
- return;
- }Also delete the now-unused helper and imports:
- import roleSchema, { Role } from '@/user/schemas/role.schema';
- import userSchema, { User } from '@/user/schemas/user.schema';
@@
-/**
- * @returns The admin user or null
- */
-const getAdminUser = async () => {
- const RoleModel = mongoose.model<Role>(Role.name, roleSchema);
- const UserModel = mongoose.model<User>(User.name, userSchema);
-
- const adminRole = await RoleModel.findOne({ name: 'admin' });
- const user = await UserModel.findOne({ roles: [adminRole!._id] }).sort({
- createdAt: 'asc',
- });
-
- return user!;
-};📝 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.
| const adminUser = await getAdminUser(); | |
| if (!adminUser) { | |
| services.logger.warn('Unable to process block, no admin user found'); | |
| return; | |
| } |
🤖 Prompt for AI Agents
In api/src/migration/migrations/1756483008605-v-2-2-12.migration.ts around lines
35 to 41, remove the admin-user gating that returns early if getAdminUser() is
falsy so the migration always runs; delete the getAdminUser() call and the
surrounding if-block, apply any necessary in-range edits so updates proceed
regardless of admin existence, and also remove the now-unused helper function
and its imports from this file and any related index exports to avoid dead
code/import errors.
| } catch (error) { | ||
| services.logger.error(`Failed to update typing : ${error.message}`); | ||
|
|
||
| throw error instanceof Error ? error : new Error(error); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Type-safe error handling; avoid unknown/stringly errors and preserve stack.
Accessing error.message without narrowing breaks under useUnknownInCatchVariables; new Error(error) with non-string is unsafe.
- } catch (error) {
- services.logger.error(`Failed to update typing : ${error.message}`);
-
- throw error instanceof Error ? error : new Error(error);
- }
+ } catch (error: unknown) {
+ const err = error instanceof Error ? error : new Error(String(error));
+ services.logger.error('Failed to update typing', err);
+ throw err;
+ }
@@
- } catch (error) {
- services.logger.error(`Migration failed : ${error.message}`);
- throw error instanceof Error ? error : new Error(error);
- }
+ } catch (error: unknown) {
+ const err = error instanceof Error ? error : new Error(String(error));
+ services.logger.error('Migration failed', err);
+ throw err;
+ }Also applies to: 63-66
🤖 Prompt for AI Agents
In api/src/migration/migrations/1756483008605-v-2-2-12.migration.ts around lines
50-54 and similarly 63-66, the catch blocks assume `error` has a `.message` and
do `new Error(error)` which is unsafe under `useUnknownInCatchVariables` and can
drop stack info; update both catches to treat the caught value as `unknown`,
narrow with `if (error instanceof Error)` to log `error.message` (or
`error.stack` for richer context) and rethrow the original Error to preserve
stack, otherwise convert the unknown to a string via `String(error)` for logging
and throw `new Error(String(error))`.
| } | ||
|
|
||
| try { | ||
| await BlockModel.updateMany({ 'options.typing': { $exists: true } }, [ |
There was a problem hiding this comment.
Quick question: what if a single documents contains an invalid options.typing field and thus cannot be parsed to int ?
There was a problem hiding this comment.
This is a good edge case, but we are not supposed to face this case except if the data is updated not from the UI.
This migration main motivation is to resolve block.options.typing values previously set it accidentally with a string values (UI is fixed to not introduce new values with not expected type).
yassinedorbozgithub
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
api/src/migration/migrations/1756483008605-v-2-3-2.migration.ts (4)
50-54: Harden error logging and rethrow.Avoid accessing error.message on unknown; log the error object and ensure the rethrow handles non‑Error values.
- } catch (error) { - services.logger.error(`Failed to update typing : ${error.message}`); - - throw error instanceof Error ? error : new Error(error); - } + } catch (err) { + services.logger.error('Failed to update typing', err); + throw err instanceof Error ? err : new Error(String(err)); + }
63-66: Same logging/throwing issue in up().- } catch (error) { - services.logger.error(`Migration failed : ${error.message}`); - throw error instanceof Error ? error : new Error(error); + } catch (err) { + services.logger.error('Migration failed', err); + throw err instanceof Error ? err : new Error(String(err)); }
42-49: Optional: emit counters for visibility.Log matched/modified counts from updateMany to know the migration effect.
If your LoggerService supports info/log, add after update:
+ // Optional: visibility + // services.logger.info?.(`Block options.typing normalized: matched=${res.matchedCount}, modified=${res.modifiedCount}`);
68-70: Down is a no‑op—confirm this is acceptable.Data type changes aren’t reversible here. If that’s expected, consider adding a comment explaining why.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
api/src/migration/migrations/1756483008605-v-2-3-2.migration.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
api/src/migration/migrations/1756483008605-v-2-3-2.migration.ts (5)
api/src/user/schemas/role.schema.ts (1)
RoleModel(57-60)api/src/user/schemas/user.schema.ts (1)
UserModel(129-132)api/src/migration/types.ts (1)
MigrationServices(39-43)api/src/chat/schemas/block.schema.ts (1)
BlockModel(187-190)api/src/logger/logger.service.ts (1)
error(32-34)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: API-Tests
- GitHub Check: Frontend Tests
🔇 Additional comments (3)
api/src/migration/migrations/1756483008605-v-2-3-2.migration.ts (3)
42-49: Confirm MongoDB version supports pipeline updates.
collection.updateMany([...pipeline])requires MongoDB 4.2+. Ensure target environments meet this.
57-71: Confirm migration export style matches your migration runner (CommonJS vs ESM).Verified: the migration file api/src/migration/migrations/1756483008605-v-2-3-2.migration.ts (and other files in api/src/migration/migrations — 1735836154221-v-2-2-0, 1748492346868-v-2-2-9, 1754055187170-v-2-2-11) use ESM-style imports but export via CommonJS
module.exports. Ensure your migration runner supports CommonJS exports with TypeScript/ESM imports; if the runner requires ESM, convert these migrations to useexport default(or adjust the runner).
42-49: Make conversion tolerant: use $convert with onError/onNull and exclude nulls$t oInt can throw on unconvertible values and abort the update; use $convert and tighten the filter to skip nulls.
File: api/src/migration/migrations/1756483008605-v-2-3-2.migration.ts (lines 42–49)
- await BlockModel.updateMany({ 'options.typing': { $exists: true } }, [ - { - $set: { - 'options.typing': { $toInt: '$options.typing' }, - }, - }, - ]); + await BlockModel.updateMany( + { 'options.typing': { $exists: true, $ne: null } }, + [ + { + $set: { + 'options.typing': { + $convert: { + input: '$options.typing', + to: 'int', + onError: 0, + onNull: 0, + }, + }, + }, + }, + ] + );Confirm desired default when conversion fails (0 used above).
| import roleSchema, { Role } from '@/user/schemas/role.schema'; | ||
| import userSchema, { User } from '@/user/schemas/user.schema'; |
There was a problem hiding this comment.
Remove unused admin lookup and hard dependency (migration can silently no‑op).
The admin user is fetched but never used. Worse, the migration returns early if no admin exists, leaving data un-migrated. Drop this code to ensure the migration always runs.
-import roleSchema, { Role } from '@/user/schemas/role.schema';
-import userSchema, { User } from '@/user/schemas/user.schema';
+// Admin lookup removed: not needed for this migration
-/**
- * @returns The admin user or null
- */
-const getAdminUser = async () => {
- const RoleModel = mongoose.model<Role>(Role.name, roleSchema);
- const UserModel = mongoose.model<User>(User.name, userSchema);
-
- const adminRole = await RoleModel.findOne({ name: 'admin' });
- const user = await UserModel.findOne({ roles: [adminRole!._id] }).sort({
- createdAt: 'asc',
- });
-
- return user!;
-};
-
- const adminUser = await getAdminUser();
-
- if (!adminUser) {
- services.logger.warn('Unable to process block, no admin user found');
- return;
- }Also applies to: 17-31, 35-41
🤖 Prompt for AI Agents
In api/src/migration/migrations/1756483008605-v-2-3-2.migration.ts around lines
12-13 (and affecting 17-31, 35-41), remove the unused admin lookup and the
early-return that makes the migration a hard no-op when no admin exists; delete
the code that queries for the admin user and any conditional that returns if
admin is falsy so the migration logic always runs, and remove now-unused imports
(roleSchema/Role and userSchema/User) or adjust imports/typing accordingly;
ensure the remaining migration code executes unconditionally and still handles
the case where no admin-related data exists without throwing.
Motivation
The main motivation of this PR is to add a migration to transform the block options.typing to be a number.
Fixes #1340
Type of change:
Checklist:
Summary by CodeRabbit