chore: Port ctl code to Typescript
#37606
Conversation
WalkthroughThe changes in this pull request primarily involve transitioning from CommonJS to ES module syntax across various files in the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
the
skip-changelog
sharat87
requested review from
riodeuno,
mohanarpit,
nidhi-nair and
trishaanand
| execCommand: jest.fn().mockImplementation(async (a) => a.join(" ")), | ||
| })); | ||
|
|
||
| import * as backup from "./backup"; |
There was a problem hiding this comment.
Do we do * imports on the client side code base? Should we change this to particular func exports instead?
There was a problem hiding this comment.
We do, actually. They're not nearly as bad as in Java. In Java, a * import pulls in tons of things into the namespace. But here, this * import only brings one name into the namespace.
Shot from our EE repo:
| @@ -77,7 +77,7 @@ async function sendBackupErrorToAdmins(err, backupTimestamp) { | |||
| user: mailUser, | |||
| pass: mailPass, | |||
| }, | |||
| }); | |||
| } as any); | |||
There was a problem hiding this comment.
Doesn't any surpass the type checks?
There was a problem hiding this comment.
Yes, they do. That's the intention here. There's one other place I had to do this.
Why? One, I don't want to change the files too much, or Git won't treat it as a rename, which I really really want. So I want to fix this in another PR.
Two, the type definitions for this library don't think host should be provided. Which is very weird, but the code clearly has been working. I have to check if the types are incorrect, or is our usage has always been incorrect.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (44)
app/client/packages/rts/src/ctl/version.ts (2)
Line range hint
3-16: Add TypeScript type annotationsSince this is a TypeScript conversion, we should leverage TypeScript's type system for better type safety.
-export async function exec() { +export async function exec(): Promise<void> { - let version = null; + let version: string | null = null; try { version = await utils.getCurrentAppsmithVersion(); - } catch (err) { + } catch (err: unknown) { console.error("Error fetching current Appsmith version", err); process.exit(1); }
Line range hint
3-16: Consolidate error handling logicThe error handling paths can be simplified to reduce code duplication.
export async function exec(): Promise<void> { let version: string | null = null; try { version = await utils.getCurrentAppsmithVersion(); + if (!version) { + throw new Error("Could not find the current Appsmith version"); + } + console.log(version); } catch (err: unknown) { - console.error("Error fetching current Appsmith version", err); + console.error("Failed to get Appsmith version:", err instanceof Error ? err.message : String(err)); process.exit(1); } - if (version) { - console.log(version); - } else { - console.error("Error: could not find the current Appsmith version"); - process.exit(1); - } }app/client/packages/rts/src/ctl/mongo_shell_utils.ts (3)
Line range hint
5-14: Improve error handling and type safety in exec functionThe function needs TypeScript return type and better error handling.
- export async function exec() { + export async function exec(): Promise<void> { + if (!command_args[0]) { + console.error("Query expression is required"); + process.exit(1); + } + let errorCode = 0; try { await execMongoEval(command_args[0], process.env.APPSMITH_DB_URL); } catch (err) { errorCode = 1; - console.error("Error evaluating the mongo query", err); + console.error("Error evaluating the mongo query:", err instanceof Error ? err.message : String(err)); } finally { process.exit(errorCode); } }
Line range hint
16-26: Add type safety and input validation for MongoDB shell executionThe function needs parameter types and input validation to prevent injection vulnerabilities.
- async function execMongoEval(queryExpression, appsmithMongoURI) { + async function execMongoEval(queryExpression: string, appsmithMongoURI?: string): Promise<string> { + if (!appsmithMongoURI) { + throw new Error("MongoDB URI is required"); + } + queryExpression = queryExpression.trim(); if (command_args.includes("--pretty")) { queryExpression += ".pretty()"; } + + // Validate query expression to prevent injection + if (queryExpression.includes(";") || queryExpression.includes("&&")) { + throw new Error("Invalid characters in query expression"); + } + return await utils.execCommand([ "mongosh", appsmithMongoURI, `--eval=${queryExpression}`, ]); }
Line range hint
1-26: Consider restructuring for better testabilityThe current implementation with direct process access and MongoDB shell execution makes unit testing challenging.
Consider:
- Dependency injection for MongoDB client
- Separating command line handling from core logic
- Making process.exit configurable for testing
app/client/packages/rts/src/ctl/logger.ts (3)
1-2: Add type information for the Constants importSince this is TypeScript, we should leverage type information for better maintainability.
-import * as Constants from "./constants"; +import type * as Constants from "./constants";
Line range hint
4-15: Several improvements needed for type safety and maintainabilityBoth functions need TypeScript types and could benefit from some refactoring.
- Add type annotations:
-export async function backup_error(err) { +export async function backup_error(err: Error | string) { -export async function backup_info(msg) { +export async function backup_info(msg: string) {
- Extract common directory creation logic:
async function ensureLogDirectory() { try { await fsPromises.access(Constants.APPSMITHCTL_LOG_PATH); } catch { await fsPromises.mkdir(Constants.APPSMITHCTL_LOG_PATH, { recursive: true }); } }
- Use path.join for safe path concatenation:
+import path from 'path'; -Constants.APPSMITHCTL_LOG_PATH + "/backup.log" +path.join(Constants.APPSMITHCTL_LOG_PATH, "backup.log")Also applies to: 17-28
Line range hint
4-28: Enhance error handling for file operationsThe current error handling could be more robust, especially for file operations.
Consider wrapping the file operations in try-catch blocks:
export async function backup_error(err: Error | string) { console.error(err); try { await ensureLogDirectory(); await fsPromises.appendFile( path.join(Constants.APPSMITHCTL_LOG_PATH, "backup.log"), `${new Date().toISOString()} [ ERROR ] ${err}\n`, ); } catch (error) { console.error('Failed to write to log file:', error); // Optionally: throw new Error('Failed to write to log file: ' + error.message); } }app/client/packages/rts/src/ctl/export_db.ts (4)
1-3: Consider adding type annotations for imported modulesWhile the ES module imports are correct, adding explicit type annotations would enhance type safety and documentation.
-import fsPromises from "fs/promises"; +import type { promises as fsPromises } from "fs";
Line range hint
5-15: Add error handling and type annotationsThe function should handle potential errors from filesystem operations and mongodump command execution.
-export async function exportDatabase() { +export async function exportDatabase(): Promise<void> { console.log("export_database ...."); const dbUrl = utils.getDburl(); + if (!dbUrl) { + throw new Error("Database URL is not configured"); + } try { await fsPromises.mkdir(Constants.BACKUP_PATH, { recursive: true }); await utils.execCommand([ "mongodump", "--uri=" + dbUrl, `--archive=${Constants.BACKUP_PATH}/${Constants.DUMP_FILE_NAME}`, "--gzip", ]); console.log("export_database done"); + } catch (error) { + console.error("Failed to export database:", error); + throw error; + } }
Line range hint
18-47: Improve error handling and add type safetyThe function has good error handling but could be improved in a few areas:
- Using process.exit in finally block can lead to unexpected behavior
- Missing type annotations
- Consider using a logging library instead of console.log
-export async function run() { +export async function run(): Promise<number> { let errorCode = 0; await utils.ensureSupervisorIsRunning(); try { // ... existing code ... } catch (err) { - console.log(err); + console.error('Export failed:', err); errorCode = 1; } finally { await utils.start(["backend", "rts"]); - process.exit(errorCode); + return errorCode; } }
TypeScript features could be better leveraged in this code
The code could benefit from stronger TypeScript patterns:
- Add explicit return type annotation for
exportDatabase()function- Type the error in catch block instead of using implicit
err: any- Add interface for command array arguments in
utils.execCommand()- Consider using
constwith type assertion fordbUrlvariable🔗 Analysis chain
Line range hint
1-47: Verify TypeScript migration completenessWhile the code has been converted to TypeScript, there are opportunities to leverage more TypeScript features for better type safety.
Let me analyze the TypeScript features usage with a different approach to provide a more accurate assessment.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any remaining .js files in the directory fd -e js . app/client/packages/rts/src/ctl/ # Check for any 'any' types or missing type annotations ast-grep --pattern 'any' app/client/packages/rts/src/ctl/export_db.tsLength of output: 115
Script:
#!/bin/bash # Check for type annotations in function parameters and return types ast-grep --pattern 'function $NAME($PARAMS): $RETURN { $$$}' # Check for variable declarations with explicit type annotations ast-grep --pattern 'const $NAME: $TYPE = $$$' ast-grep --pattern 'let $NAME: $TYPE = $$$' # Check imports to see if they have type annotations ast-grep --pattern 'import { $$$_type $$$} from $$$' # Look for potential error types ast-grep --pattern 'catch (err) { $$$ }'Length of output: 105953
app/client/packages/rts/package.json (1)
Line range hint
45-45: Verify TypeScript version specificationThe specified TypeScript version
^5.5.4appears to be incorrect as TypeScript's latest stable version is in the 5.3.x range.Consider updating to a valid version:
- "typescript": "^5.5.4" + "typescript": "^5.3.3"app/client/packages/rts/src/ctl/check_replica_set.ts (2)
7-8: Add environment variable validationDirect use of process.env without validation could lead to runtime errors.
+ if (!process.env.APPSMITH_DB_URL) { + throw new Error("APPSMITH_DB_URL environment variable is required"); + } const client = new MongoClient( preprocessMongoDBURI(process.env.APPSMITH_DB_URL),
Line range hint
27-55: Improve resource cleanup and error handlingThe changeStream cleanup could be more robust, and the setTimeout approach is somewhat hacky.
Consider this more robust implementation:
async function checkReplicaSet(client: MongoClient) { await client.connect(); - return await new Promise<boolean>((resolve) => { + return await new Promise<boolean>((resolve, reject) => { + let changeStream: ChangeStream; + const cleanup = () => { + if (changeStream) { + try { + changeStream.close(); + } catch (err) { + console.error('Error closing change stream:', err); + } + } + }; + try { - const changeStream = client + changeStream = client .db() .collection("user") .watch() .on("change", (change) => console.log(change)) .on("error", (err) => { + cleanup(); if ( err instanceof MongoServerError && err.toString() == "MongoServerError: The $changeStream stage is only supported on replica sets" ) { console.log("Replica set not enabled"); } else { console.error("Error even from changeStream", err); } resolve(false); }); // Use AbortController instead of setTimeout for cleaner timeout handling + const controller = new AbortController(); + const timeoutId = setTimeout(() => { + cleanup(); + resolve(true); + controller.abort(); + }, 1000); + + // Cleanup on abort + controller.signal.addEventListener('abort', () => { + clearTimeout(timeoutId); + }); - setTimeout(() => { - resolve(true); - changeStream.close(); - }, 1000); } catch (err) { + cleanup(); console.error("Error thrown when checking replicaset", err); resolve(false); } }); }app/client/packages/rts/src/ctl/index.ts (2)
23-23: Add error handling for config loadingConsider adding try-catch block for config loading and make the path configurable via environment variable.
-config({ path: APPLICATION_CONFIG_PATH }); +try { + config({ path: process.env.CONFIG_PATH || APPLICATION_CONFIG_PATH }); +} catch (error) { + console.warn(`Failed to load config from ${APPLICATION_CONFIG_PATH}: ${error.message}`); +}
Line range hint
37-61: Standardize error handling across all commandsOnly the import-db command has proper error handling. Consider wrapping all command executions in try-catch blocks for consistent error handling.
} else if (["backup"].includes(command)) { - backup.run(); + try { + backup.run(); + console.log("Backup completed successfully"); + } catch (error) { + console.error("Backup failed:", error.message); + process.exit(1); + }app/client/packages/rts/src/ctl/import_db.ts (2)
Line range hint
32-44: Consider using a more specific type and improving error handlingWhile the
stringtype annotation is correct, we could make it more specific and improve error handling.Consider these improvements:
- let shellCmdResult: string; + let shellCmdResult: string = ''; try { shellCmdResult = await utils.execCommandReturningOutput([ "mongo", process.env.APPSMITH_DB_URL, "--quiet", "--eval", "db.getCollectionNames().length", ]); } catch (error) { - console.error("Failed to execute mongo command:", error); + console.error("Failed to execute mongo command:", error instanceof Error ? error.message : String(error)); throw error; }
MongoDB connection string is passed directly to shell command without sanitization
The code directly uses
process.env.APPSMITH_DB_URLin a shell command without any sanitization, which could potentially expose sensitive information in logs or allow command injection. The codebase shows no evidence of URL sanitization utilities or connection string masking.
app/client/packages/rts/src/ctl/import_db.ts:34: Add sanitization before using the connection string in shell commands- Consider using a MongoDB client library instead of shell commands for better security
🔗 Analysis chain
Line range hint
34-40: Verify MongoDB connection string handlingLet's ensure the connection string is properly sanitized before being used in commands or logs.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for potential connection string logging rg -g '!*.{lock,md,json}' "process\.env\.APPSMITH_DB_URL.*console\." # Check for connection string sanitization utilities ast-grep --pattern 'function $_($$sanitize$$) { $$$ }'Length of output: 127
Script:
#!/bin/bash # Check for any MongoDB connection string handling or sanitization rg -g '!*.{lock,md,json}' "APPSMITH_DB_URL" -B 2 -A 2 # Check for any utility functions that might handle connection strings rg -g '!*.{lock,md,json}' "sanitize.*URL|hideURL|maskURL|redact.*URL" -i # Look for MongoDB related utility functions ast-grep --pattern 'function $_($$) { $$$ mongo $$$ }'Length of output: 33018
Script:
#!/bin/bash # Check for any URL sanitization or masking utilities in the codebase ast-grep --pattern 'function $_($$) { $$$ $url $$$ }' # Look for any logging related to MongoDB connection rg -g '!*.{lock,md,json}' "console\.(log|debug|info|error).*mongo.*url" -i # Check for any environment variable sanitization rg -g '!*.{lock,md,json}' "sanitize.*env|mask.*env|redact.*env" -iLength of output: 130845
app/client/packages/rts/build.js (1)
Line range hint
1-90: Consider renaming this file tobuild.tsSince this build script is now handling TypeScript files and the project is moving towards TypeScript, consider converting this file to TypeScript as well. This would provide better type safety for the build configuration.
app/client/packages/rts/src/ctl/mailer.ts (3)
Line range hint
1-13: Consider adding type safety for environment variablesThe environment variables should be properly typed to prevent runtime issues.
-const mailEnabled = process.env.APPSMITH_MAIL_ENABLED; -const mailFrom = process.env.APPSMITH_MAIL_FROM; +const mailEnabled = process.env.APPSMITH_MAIL_ENABLED === 'true'; +const mailFrom = process.env.APPSMITH_MAIL_FROM || '';
Line range hint
44-47: Use path.join for file pathsString concatenation for file paths can be problematic across different operating systems.
- const lastBackupPath = Constants.BACKUP_PATH + "/" + lastBackupfile; + const lastBackupPath = path.join(Constants.BACKUP_PATH, lastBackupfile);Don't forget to add the import:
import path from 'path';
Line range hint
14-89: Consider extracting configuration validationThe multiple environment variable checks could be moved to a separate validation function for better maintainability.
function validateEmailConfig(): void { if (!mailEnabled || !mailFrom || !mailHost || !mailPort || !mailUser || !mailPass) { throw new Error("Email provider not configured properly"); } if (!mailTo) { throw new Error("Admin email(s) not configured"); } }app/client/packages/rts/src/ctl/utils.ts (6)
Line range hint
41-67: Consider performance and readability improvementsThe current implementation has some areas for improvement:
- Using synchronous
readFileSynccan block the event loop- Using
for...inwith arrays is not recommendedConsider this refactoring:
export function getDburl() { let dbUrl = ""; + const dbEnvUrl = process.env.APPSMITH_DB_URL || process.env.APPSMITH_MONGO_DB_URI; + if (dbEnvUrl && dbEnvUrl !== "undefined") { + return dbEnvUrl.trim(); + } + try { - const env_array = fs + const envContent = fs .readFileSync(Constants.ENV_PATH, "utf8") - .toString() - .split("\n"); - for (const i in env_array) { - if ( - env_array[i].startsWith("APPSMITH_MONGODB_URI") || - env_array[i].startsWith("APPSMITH_DB_URL") - ) { - dbUrl = env_array[i].toString().split("=")[1].trim(); - break; - } - } + const match = envContent.match(/^(APPSMITH_MONGODB_URI|APPSMITH_DB_URL)=(.*)$/m); + if (match) { + return match[2].trim(); + } } catch (err) { console.error("Error reading the environment file:", err); } - const dbEnvUrl = - process.env.APPSMITH_DB_URL || process.env.APPSMITH_MONGO_DB_URI; - if (dbEnvUrl && dbEnvUrl !== "undefined") { - dbUrl = dbEnvUrl.trim(); - } return dbUrl; }
Line range hint
69-100: Add proper TypeScript types for options parameterThe function implementation is solid, but the options parameter needs proper typing.
Consider adding proper types:
-export function execCommand(cmd: string[], options?) { +export function execCommand(cmd: string[], options?: childProcess.SpawnOptions) {
Line range hint
101-134: Add proper typing and consider using Buffer for better performanceThe function needs proper TypeScript types and could be optimized.
Consider these improvements:
-export function execCommandReturningOutput(cmd, options?) { +export function execCommandReturningOutput( + cmd: string[], + options?: childProcess.SpawnOptions +): Promise<string> { return new Promise<string>((resolve, reject) => { const p = childProcess.spawn(cmd[0], cmd.slice(1), options); p.stdin.end(); - const outChunks = [], - errChunks = []; + const outChunks: Buffer[] = []; + const errChunks: Buffer[] = []; - p.stdout.setEncoding("utf8"); p.stdout.on("data", (data) => { - outChunks.push(data.toString()); + outChunks.push(Buffer.from(data)); }); - p.stderr.setEncoding("utf8"); p.stderr.on("data", (data) => { - errChunks.push(data.toString()); + errChunks.push(Buffer.from(data)); }); p.on("close", (code) => { const output = ( - outChunks.join("").trim() + + Buffer.concat(outChunks).toString().trim() + "\n" + - errChunks.join("").trim() + Buffer.concat(errChunks).toString().trim() ).trim();
Line range hint
174-204: Add type annotation for clarityThe function is well-implemented with good documentation and logic.
Consider adding the type annotation:
-export function preprocessMongoDBURI(uri /* string */) { +export function preprocessMongoDBURI(uri: string) {
Line range hint
206-236: Consider reducing code duplication with execCommandThe function shares significant code with
execCommand.Consider extracting common logic:
+function createSpawnPromise( + cmd: string[], + options?: childProcess.SpawnOptions, + silent = false +): Promise<void> { + return new Promise<void>((resolve, reject) => { + let isPromiseDone = false; + + const p = childProcess.spawn(cmd[0], cmd.slice(1), { + ...options, + stdio: silent ? "ignore" : "inherit", + }); + + p.on("close", (code) => { + if (isPromiseDone) return; + isPromiseDone = true; + code === 0 ? resolve() : reject(); + }); + + p.on("error", (err) => { + if (isPromiseDone) return; + isPromiseDone = true; + reject(err); + }); + }); +} + -export function execCommand(cmd: string[], options?) { +export function execCommand(cmd: string[], options?: childProcess.SpawnOptions) { + return createSpawnPromise(cmd, options); +} + -export function execCommandSilent(cmd: string[], options?) { +export function execCommandSilent(cmd: string[], options?: childProcess.SpawnOptions) { + return createSpawnPromise(cmd, options, true); +}
TypeScript strict mode improvements needed
The package's tsconfig.json lacks strict mode settings. Several functions are missing parameter type annotations:
stop(apps)andstart(apps)should typeappsas string arrayupdateLastBackupErrorMailSentInMilliSec(ts)should typetsas numbergetDatabaseNameFromMongoURI(uri)should typeurias stringEnable strict mode and add type annotations to improve type safety.
🔗 Analysis chain
Line range hint
1-240: Verify TypeScript strict mode compatibilityThe TypeScript port looks good overall, but we should verify strict mode compatibility.
Let me verify the TypeScript configuration for the specific package.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if strict TypeScript checks are enabled in the config rg '"strict": true' -l --type json # Look for any // @ts-ignore or // @ts-nocheck comments that might be hiding issues rg '// @ts-(ignore|nocheck)' --type tsLength of output: 2200
Script:
#!/bin/bash # Check TypeScript config for the RTS package fd tsconfig.json app/client/packages/rts/ # Check for any type-related comments or potential type issues in the utils file rg -A 2 'any|Function|object' app/client/packages/rts/src/ctl/utils.tsLength of output: 154
Script:
#!/bin/bash # Check the content of RTS package's tsconfig.json cat app/client/packages/rts/tsconfig.json # Check function parameters that might need typing ast-grep --pattern 'function $NAME($PARAMS) { $$$ }' app/client/packages/rts/src/ctl/utils.tsLength of output: 2367
app/client/packages/rts/src/ctl/backup.ts (6)
1-9: Add TypeScript type definitions for imported modulesConsider adding type definitions for better type safety:
+import type { Stats } from 'fs'; +import type { ExecException } from 'child_process';
Line range hint
13-117: Consider breaking down therunfunction into smaller, focused functionsThe function handles multiple responsibilities including:
- Backup space verification
- Database export
- Git storage archival
- Encryption
- Error handling
Consider extracting these into separate functions for better maintainability.
Example structure:
async function run() { const context = await initializeBackupContext(); try { await verifyBackupPrerequisites(); await performBackup(context); await handleEncryption(context); } catch (err: unknown) { await handleBackupError(err); } finally { await cleanup(context); } }
Line range hint
119-137: Add type annotations and consider moving encryption parameters to constants-export async function encryptBackupArchive(archivePath, encryptionPassword) { +export async function encryptBackupArchive(archivePath: string, encryptionPassword: string): Promise<string> {Consider moving the encryption parameters to constants:
const ENCRYPTION_ITERATIONS = 100000; const ENCRYPTION_ALGORITHM = 'aes-256-cbc';
Line range hint
211-250: Enhance error handling for external commandsAdd specific error handling for mongodump and tar commands:
export async function executeMongoDumpCMD(destFolder: string, appsmithMongoURI: string): Promise<void> { try { await utils.execCommand([/*...*/]); } catch (err) { throw new Error(`MongoDB backup failed: ${err instanceof Error ? err.message : String(err)}`); } }
Line range hint
261-304: Add strict TypeScript types for utility functions-export function getGitRoot(gitRoot?) { +export function getGitRoot(gitRoot?: string): string { -export function getBackupArchiveLimit(backupArchivesLimit?) { +export function getBackupArchiveLimit(backupArchivesLimit?: number): number { -export async function removeOldBackups(backupFiles, backupArchivesLimit) { +export async function removeOldBackups(backupFiles: string[], backupArchivesLimit: number): Promise<string[]> {
309-320: Add types and move magic numbers to constants-export async function getAvailableBackupSpaceInBytes(path) { +export async function getAvailableBackupSpaceInBytes(path: string): Promise<number> { +const REQUIRED_BACKUP_SPACE_GB = 2; +const GB_TO_BYTES = 1024 * 1024 * 1024; export function checkAvailableBackupSpace(availSpaceInBytes: number): void { if (availSpaceInBytes < REQUIRED_BACKUP_SPACE_GB * GB_TO_BYTES) { throw new Error( - "Not enough space available at /appsmith-stacks. Please ensure availability of at least 2GB to backup successfully." + `Not enough space available at /appsmith-stacks. Please ensure availability of at least ${REQUIRED_BACKUP_SPACE_GB}GB to backup successfully.` ); } }app/client/packages/rts/src/ctl/backup.test.ts (5)
1-4: Add type safety to mock implementationThe mock implementation could benefit from explicit typing for better type safety.
-jest.mock("./utils", () => ({ +jest.mock<typeof import("./utils")>("./utils", () => ({ ...jest.requireActual("./utils"), - execCommand: jest.fn().mockImplementation(async (a) => a.join(" ")), + execCommand: jest.fn().mockImplementation(async (args: string[]) => args.join(" ")), }));
26-42: Consider adding edge case testsWhile the current tests cover the basic scenarios, consider adding tests for:
- Maximum safe integer value
- Edge case when available space equals minimum required space
Line range hint
133-190: Remove debug console.log statementsMultiple console.log statements should be removed from test cases as they add noise to test output.
Line range hint
194-211: Improve mock implementation for password promptsConsider using a more deterministic approach for mocking password prompts:
- readlineSync.question = jest.fn().mockImplementation((a) => { + readlineSync.question = jest.fn().mockImplementation((prompt: string) => { - if (a == "Enter the above password again: ") { + if (prompt === "Enter the above password again: ") { return "pass"; } return password; });
Line range hint
1-269: Overall: TypeScript conversion looks goodThe conversion to TypeScript is well-executed. Consider:
- Adding type annotations to test variables
- Using TypeScript's
expecttypes from@jest/globals- Implementing proper error types for error cases
app/client/packages/rts/src/ctl/restore.ts (4)
1-6: Consider using named imports for better tree-shakingInstead of namespace imports with *, consider using named imports for
utilsandConstantsto enable better tree-shaking.-import * as utils from "./utils"; -import * as Constants from "./constants"; +import { getCurrentAppsmithVersion, listLocalBackupFiles, execCommand, execCommandSilent, getDburl, ensureSupervisorIsRunning, stop, start } from "./utils"; +import { BACKUP_PATH } from "./constants";
223-223: Fix typo in function nameThe function name has a typo: "Compatability" should be "Compatibility".
-async function checkRestoreVersionCompatability(restoreContentsPath) { +async function checkRestoreVersionCompatibility(restoreContentsPath) {
282-282: Add return type annotationSince this is TypeScript, add a return type annotation to the exported function.
-export async function run() { +export async function run(): Promise<void> {
Line range hint
1-282: Add TypeScript type definitionsThe TypeScript migration is incomplete without proper type definitions. Consider:
- Add parameter type annotations
- Define interfaces for manifest.json structure
- Add proper error types for try-catch blocks
Example types to add:
interface ManifestJson { appsmithVersion: string; dbName?: string; } interface RestoreOptions { backupPath: string; overwriteEncryptionKeys: boolean; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
app/client/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (17)
app/client/packages/rts/build.js(1 hunks)app/client/packages/rts/package.json(1 hunks)app/client/packages/rts/src/ctl/backup.test.ts(11 hunks)app/client/packages/rts/src/ctl/backup.ts(8 hunks)app/client/packages/rts/src/ctl/check_replica_set.ts(2 hunks)app/client/packages/rts/src/ctl/constants.js(0 hunks)app/client/packages/rts/src/ctl/constants.ts(1 hunks)app/client/packages/rts/src/ctl/export_db.ts(2 hunks)app/client/packages/rts/src/ctl/import_db.ts(3 hunks)app/client/packages/rts/src/ctl/index.ts(4 hunks)app/client/packages/rts/src/ctl/logger.ts(2 hunks)app/client/packages/rts/src/ctl/mailer.ts(3 hunks)app/client/packages/rts/src/ctl/mongo_shell_utils.ts(1 hunks)app/client/packages/rts/src/ctl/restore.ts(4 hunks)app/client/packages/rts/src/ctl/utils.test.ts(1 hunks)app/client/packages/rts/src/ctl/utils.ts(10 hunks)app/client/packages/rts/src/ctl/version.ts(1 hunks)
💤 Files with no reviewable changes (1)
- app/client/packages/rts/src/ctl/constants.js
✅ Files skipped from review due to trivial changes (2)
- app/client/packages/rts/src/ctl/constants.ts
- app/client/packages/rts/src/ctl/utils.test.ts
🔇 Additional comments (11)
app/client/packages/rts/src/ctl/version.ts (1)
1-1: Verify utils module TypeScript conversion
Ensure that the imported utils module is also properly typed.
✅ Verification successful
Let me check the utils.ts file to verify its type definitions.
Utils module is properly typed in TypeScript
The utils module has been properly converted to TypeScript with the getCurrentAppsmithVersion function correctly defined as an async function that implicitly returns a string (from JSON parsing). The function is being used consistently across multiple TypeScript files in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if utils.ts exists and has proper type definitions
rg -t ts "getCurrentAppsmithVersion" app/client/packages/rts/src/ctl/
Length of output: 574
Script:
#!/bin/bash
# Check the type definitions in utils.ts
ast-grep --pattern 'export async function getCurrentAppsmithVersion() {
$$$
}'
Length of output: 520
app/client/packages/rts/src/ctl/logger.ts (1)
2-2: Verify the APPSMITHCTL_LOG_PATH constant
Let's ensure the constant is properly defined in the constants file.
✅ Verification successful
The previous search found the constant but let's verify its definition and usage in context by looking at the full constants file.
Constant APPSMITHCTL_LOG_PATH is properly defined
The constant is correctly defined in the constants file with the expected path value /appsmith-stacks/logs/appsmithctl.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if APPSMITHCTL_LOG_PATH is defined in constants.ts
ast-grep --pattern 'export const APPSMITHCTL_LOG_PATH = $_'
Length of output: 142074
Script:
#!/bin/bash
# Let's check the constants file directly
cat app/client/packages/rts/src/ctl/constants.ts
Length of output: 694
app/client/packages/rts/package.json (1)
39-40: LGTM: Type definitions added correctly
The added type definitions match their runtime dependencies and use appropriate version ranges.
app/client/packages/rts/src/ctl/check_replica_set.ts (1)
1-4: LGTM! Clean import statements
The transition to ES6 imports with explicit type imports from MongoDB is well done.
app/client/packages/rts/src/ctl/index.ts (2)
3-12: LGTM! Clean transition to ES modules
The conversion from CommonJS to ES modules is well-structured with proper use of TypeScript imports.
62-63: 🛠️ Refactor suggestion
Add argument validation before command execution
Consider adding validation for required arguments before executing commands and show help when arguments are missing.
Let's check if the commands require arguments:
app/client/packages/rts/src/ctl/import_db.ts (1)
1-4: LGTM: Clean ES module imports
The transition from CommonJS to ES module syntax is properly implemented.
app/client/packages/rts/build.js (1)
47-47: Verify the TypeScript entry point file exists
The entry point has been updated to use TypeScript. Let's verify the file exists and was properly converted.
✅ Verification successful
TypeScript entry point exists and is properly structured
The TypeScript entry point file src/ctl/index.ts exists and contains the necessary command-line interface logic. The file has been properly converted from JavaScript to TypeScript with appropriate imports and type-safe code structure.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the TypeScript entry point exists and contains exports
# Expected: The file exists and exports are properly defined
# Check if the file exists
fd "index.ts" "src/ctl"
# Check the exports in the TypeScript file
ast-grep --pattern 'export $$$' "src/ctl/index.ts"
Length of output: 231
Script:
#!/bin/bash
# Let's first check the repository structure to locate the correct path
fd "index.ts" "app/client/packages/rts"
# Also check for the JavaScript version to understand the context
fd "index.js" "app/client/packages/rts"
# Let's also check the content of the build.js file to understand the relative paths
cat "app/client/packages/rts/build.js"
Length of output: 2634
Script:
#!/bin/bash
# Let's verify the content of the TypeScript file
cat "app/client/packages/rts/src/ctl/index.ts"
# Also check if there are any exports
ast-grep --pattern 'export $$$' "app/client/packages/rts/src/ctl/index.ts"
Length of output: 2241
app/client/packages/rts/src/ctl/utils.ts (1)
1-5: LGTM! Modern import syntax with proper typing
The imports follow Node.js best practices, using the node: protocol for built-ins and proper ES module syntax.
app/client/packages/rts/src/ctl/backup.test.ts (1)
17-17: LGTM: Timestamp regex pattern is correct
The regex pattern correctly validates ISO timestamp format with dashes.
app/client/packages/rts/src/ctl/restore.ts (1)
46-46: LGTM! Type-safe array access
The array access is properly guarded by index validation.
| import * as utils from "./utils"; | ||
|
|
||
| const command_args = process.argv.slice(3); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add type safety and validation for command line arguments
The command arguments handling needs type safety and validation to prevent runtime errors.
- import * as utils from "./utils";
+ import * as utils from "./utils";
+
+ type CommandArgs = string[];
- const command_args = process.argv.slice(3);
+ const command_args: CommandArgs = process.argv.slice(3);
+ if (command_args.length === 0) {
+ throw new Error("No command arguments provided");
+ }📝 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.
| import * as utils from "./utils"; | |
| const command_args = process.argv.slice(3); | |
| import * as utils from "./utils"; | |
| type CommandArgs = string[]; | |
| const command_args: CommandArgs = process.argv.slice(3); | |
| if (command_args.length === 0) { | |
| throw new Error("No command arguments provided"); | |
| } |
| useUnifiedTopology: true, | ||
| }, | ||
| } as any, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove 'any' type assertion
The any type assertion defeats TypeScript's type safety. Use proper MongoDB option types.
- } as any,
+ } as MongoClientOptions,Don't forget to import MongoClientOptions:
-import { MongoClient, MongoServerError } from "mongodb";
+import { MongoClient, MongoClientOptions, MongoServerError } from "mongodb";Committable suggestion skipped: line range outside the PR's diff.
| @@ -41,7 +42,7 @@ async function run(forceOption) { | |||
| console.error("Failed to execute mongo command:", error); | |||
| throw error; | |||
| } | |||
| const collectionsLen = parseInt(shellCmdResult.stdout.toString().trimEnd()); | |||
| const collectionsLen = parseInt(shellCmdResult.trimEnd()); | |||
There was a problem hiding this comment.
Add parseInt radix and NaN validation
The current implementation could be more robust with proper number parsing.
Apply this improvement:
- const collectionsLen = parseInt(shellCmdResult.trimEnd());
+ const collectionsLen = parseInt(shellCmdResult.trimEnd(), 10);
+ if (isNaN(collectionsLen)) {
+ throw new Error('Failed to parse collection count from database');
+ }📝 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 collectionsLen = parseInt(shellCmdResult.trimEnd()); | |
| const collectionsLen = parseInt(shellCmdResult.trimEnd(), 10); | |
| if (isNaN(collectionsLen)) { | |
| throw new Error('Failed to parse collection count from database'); | |
| } |
| @@ -11,7 +11,7 @@ const mailUser = process.env.APPSMITH_MAIL_USERNAME; | |||
| const mailPass = process.env.APPSMITH_MAIL_PASSWORD; | |||
| const mailTo = process.env.APPSMITH_ADMIN_EMAILS; | |||
|
|
|||
| async function sendBackupErrorToAdmins(err, backupTimestamp) { | |||
| export async function sendBackupErrorToAdmins(err, backupTimestamp) { | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Function signature needs type annotations
Add TypeScript types to the function parameters for better type safety.
-export async function sendBackupErrorToAdmins(err, backupTimestamp) {
+export async function sendBackupErrorToAdmins(err: Error, backupTimestamp: string): Promise<void> {📝 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.
| export async function sendBackupErrorToAdmins(err, backupTimestamp) { | |
| export async function sendBackupErrorToAdmins(err: Error, backupTimestamp: string): Promise<void> { |
| }, | ||
| }); | ||
| } as any); | ||
|
|
There was a problem hiding this comment.
Avoid type assertion to 'any'
Using as any bypasses TypeScript's type checking. Consider using proper types from @types/nodemailer.
- } as any);
+ } as nodemailer.TransportOptions);Committable suggestion skipped: line range outside the PR's diff.
| export function getDatabaseNameFromMongoURI(uri) { | ||
| const uriParts = uri.split("/"); | ||
| return uriParts[uriParts.length - 1].split("?")[0]; | ||
| } |
There was a problem hiding this comment.
Use ConnectionString class for robust URI parsing
The current implementation is fragile and could break with malformed URIs.
Consider using the already imported ConnectionString class:
export function getDatabaseNameFromMongoURI(uri: string) {
- const uriParts = uri.split("/");
- return uriParts[uriParts.length - 1].split("?")[0];
+ try {
+ const cs = new ConnectionString(uri);
+ return cs.pathname?.substring(1) || "";
+ } catch (error) {
+ console.error("Invalid MongoDB URI:", error);
+ return "";
+ }
}📝 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.
| export function getDatabaseNameFromMongoURI(uri) { | |
| const uriParts = uri.split("/"); | |
| return uriParts[uriParts.length - 1].split("?")[0]; | |
| } | |
| export function getDatabaseNameFromMongoURI(uri: string) { | |
| try { | |
| const cs = new ConnectionString(uri); | |
| return cs.pathname?.substring(1) || ""; | |
| } catch (error) { | |
| console.error("Invalid MongoDB URI:", error); | |
| return ""; | |
| } | |
| } |
| test("Test mongodump CMD generation", async () => { | ||
| const dest = "/dest"; | ||
| const appsmithMongoURI = "mongodb://username:password@host/appsmith"; | ||
| const cmd = |
There was a problem hiding this comment.
Remove sensitive information from test cases
The test contains a MongoDB connection string with credentials in plain text. Consider using placeholder values or environment variables.
- const appsmithMongoURI = "mongodb://username:password@host/appsmith";
+ const appsmithMongoURI = "mongodb://[username]:[password]@host/appsmith";Committable suggestion skipped: line range outside the PR's diff.
| const mongodb_uri = | ||
| "mongodb+srv://admin:password@test.cluster.mongodb.net/my_db_name?retryWrites=true&minPoolSize=1&maxPoolSize=10&maxIdleTimeMS=900000&authSource=admin"; | ||
| var expectedDBName = "my_db_name"; | ||
| const expectedDBName = "my_db_name"; | ||
| const dbName = utils.getDatabaseNameFromMongoURI(mongodb_uri); | ||
| expect(dbName).toEqual(expectedDBName); | ||
| }); | ||
|
|
||
| test("Get DB name from Mongo URI 2", async () => { | ||
| var mongodb_uri = | ||
| const mongodb_uri = | ||
| "mongodb+srv://admin:password@test.cluster.mongodb.net/test123?retryWrites=true&minPoolSize=1&maxPoolSize=10&maxIdleTimeMS=900000&authSource=admin"; | ||
| var expectedDBName = "test123"; | ||
| const expectedDBName = "test123"; | ||
| const dbName = utils.getDatabaseNameFromMongoURI(mongodb_uri); | ||
| expect(dbName).toEqual(expectedDBName); | ||
| }); | ||
|
|
||
| test("Get DB name from Mongo URI 3", async () => { | ||
| var mongodb_uri = | ||
| const mongodb_uri = | ||
| "mongodb+srv://admin:password@test.cluster.mongodb.net/test123"; | ||
| var expectedDBName = "test123"; | ||
| const expectedDBName = "test123"; | ||
| const dbName = utils.getDatabaseNameFromMongoURI(mongodb_uri); | ||
| expect(dbName).toEqual(expectedDBName); | ||
| }); | ||
|
|
||
| test("Get DB name from Mongo URI 4", async () => { | ||
| var mongodb_uri = "mongodb://appsmith:pAssW0rd!@localhost:27017/appsmith"; | ||
| var expectedDBName = "appsmith"; | ||
| const mongodb_uri = "mongodb://appsmith:pAssW0rd!@localhost:27017/appsmith"; | ||
| const expectedDBName = "appsmith"; | ||
| const dbName = utils.getDatabaseNameFromMongoURI(mongodb_uri); | ||
| expect(dbName).toEqual(expectedDBName); | ||
| }); |
There was a problem hiding this comment.
Remove sensitive information from MongoDB URI tests
Multiple test cases contain actual passwords in MongoDB URIs. Replace them with placeholder values.
- const mongodb_uri = "mongodb://appsmith:pAssW0rd!@localhost:27017/appsmith";
+ const mongodb_uri = "mongodb://[username]:[password]@localhost:27017/appsmith";Committable suggestion skipped: line range outside the PR's diff.
|
Deferring CodeRabbit suggestions to the next PR so I don't change the files too much. I want Git to treat the changes as file renames instead of deletions+additions. |
This PR converts the Javscript code of the `ctl` module, into Typescript, and in the process already fixing two small bugs that went undetected because... of lack of good type checking. The linting exceptions are still there and will be removed in the next PR. Didn't want to change anymore than necessary or Git will detect these changes as "new files" instead of as "renames". ## Automation /test sanity ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/11949059369> > Commit: e156dac > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=11949059369&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Sanity` > Spec: > <hr>Thu, 21 Nov 2024 08:45:48 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes - **New Features** - Introduced TypeScript support by updating entry points and adding type definitions for `nodemailer` and `readline-sync`. - Enhanced logging and error handling functionalities across various modules. - **Improvements** - Transitioned from CommonJS to ES module syntax for better compatibility and maintainability. - Improved clarity and structure in command handling and test files. - **Bug Fixes** - Corrected regex patterns and variable declarations in tests to enhance reliability. - **Chores** - Updated dependencies and refined module exports for better organization. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This PR converts the Javscript code of the
ctlmodule, into Typescript, and in the process already fixing two small bugs that went undetected because... of lack of good type checking.The linting exceptions are still there and will be removed in the next PR. Didn't want to change anymore than necessary or Git will detect these changes as "new files" instead of as "renames".
Automation
/test sanity
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11949059369
Commit: e156dac
Cypress dashboard.
Tags:
@tag.SanitySpec:
Thu, 21 Nov 2024 08:45:48 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Release Notes
New Features
nodemailerandreadline-sync.Improvements
Bug Fixes
Chores