chore: Fix typing in ctl backup and restore #37663
Conversation
WalkthroughThe changes in this pull request involve significant modifications to the backup functionality within the application. A new class, Changes
Possibly related PRs
Suggested reviewers
Warning Rate limit exceeded@sharat87 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 8 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 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
skip-changelog
github-actions
bot
added
the
skip-changelog
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (6)
app/client/packages/rts/src/ctl/backup.test.ts (1)
234-234: Consider adding specific error message verificationThe test correctly verifies that the function throws an error for mismatched passwords. However, it would be more robust to verify the specific error message to ensure the exact error condition is being tested.
- expect(() => backup.getEncryptionPasswordFromUser()).toThrow(); + expect(() => backup.getEncryptionPasswordFromUser()).toThrow("Passwords do not match");app/client/packages/rts/src/ctl/backup.ts (5)
Line range hint
175-209: Avoid using synchronousreadlineSyncin asynchronous contextUsing
readlineSyncfor user input is synchronous and can block the event loop. Consider using an asynchronous alternative to improve performance and avoid blocking other operations.Refactor the code to use an asynchronous method for reading user input:
- const encryptionPwd1: string = readlineSync.question( - "Enter a password to encrypt the backup archive: ", - { hideEchoBack: true }, - ); + const encryptionPwd1: string = await utils.promptUser( + "Enter a password to encrypt the backup archive: ", + true, + );Ensure that
utils.promptUseris an asynchronous function that handles user input without blocking the event loop.
312-317: RenameexecuteCopyCMDto reflect its functionalityThe function
executeCopyCMDcreates a symbolic link usingln -s, but the name suggests it performs a copy operation. Rename the function to better describe its action.Apply this diff to rename the function:
-export async function executeCopyCMD(srcFolder: string, destFolder: string) { +export async function createSymbolicLink(srcFolder: string, destFolder: string) { return await utils.execCommand([ "ln", "-s", srcFolder, path.join(destFolder, "git-storage"), ]); }
113-113: Assignprocess.exitCodein thefinallyblockAssigning
process.exitCode = 1;in thecatchblock may not ensure that it's set before the process exits, due to asynchronous operations in thefinallyblock. Move the assignment to thefinallyblock to guarantee it is set correctly.Apply this diff to move
process.exitCodeassignment:} catch (err) { - process.exitCode = 1; await logger.backup_error(err.stack); // ... existing code ... } finally { + if (err) { + process.exitCode = 1; + } // ... existing code ... }
357-358: SimplifygetBackupArchiveLimitfunctionThe function can be simplified using the nullish coalescing operator for better readability.
Apply this diff to simplify the function:
export function getBackupArchiveLimit(backupArchivesLimit?: number): number { - return backupArchivesLimit || Constants.APPSMITH_DEFAULT_BACKUP_ARCHIVE_LIMIT; + return backupArchivesLimit ?? Constants.APPSMITH_DEFAULT_BACKUP_ARCHIVE_LIMIT; }
Line range hint
386-393: Improve error message clarity incheckAvailableBackupSpaceThe error message could be more precise about the required disk space. Specify the exact amount needed for better user understanding.
Apply this diff to enhance the error message:
throw new Error( - "Not enough space available at /appsmith-stacks. Please ensure availability of at least 2GB to backup successfully.", + "Insufficient disk space at /appsmith-stacks. Please ensure at least 2GB of free space to perform a successful backup.", );
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
app/client/packages/rts/src/ctl/backup.test.ts(1 hunks)app/client/packages/rts/src/ctl/backup.ts(16 hunks)
sharat87
changed the title
ctl backup
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (6)
app/client/packages/rts/src/ctl/restore.ts (6)
Line range hint
60-78: Add return type and extract magic numberThe function signature could be improved for better type safety.
-async function decryptArchive(encryptedFilePath: string, backupFilePath: string) { +const PBKDF2_ITERATIONS = 100000; +async function decryptArchive(encryptedFilePath: string, backupFilePath: string): Promise<boolean> { // ... "-iter", - "100000", + PBKDF2_ITERATIONS.toString(),
96-96: Add explicit return typeFor consistency with TypeScript best practices, add return type annotation.
-async function extractArchive(backupFilePath: string, restoreRootPath: string) { +async function extractArchive(backupFilePath: string, restoreRootPath: string): Promise<void> {
Line range hint
108-138: Improve error handling in database restorationThe catch block suppresses specific error details which could be valuable for debugging.
} catch (error) { console.warn( - "Error reading manifest file. Assuming same database name.", + `Error reading manifest file. Assuming same database name. Details: ${error instanceof Error ? error.message : 'Unknown error'}`, error ); }
139-141: Consider splitting function and enhancing securityThe function handles multiple responsibilities and sensitive data. Consider:
- Breaking it into smaller functions
- Clearing sensitive data from memory after use
Would you like me to provide a detailed refactoring suggestion?
230-230: Add error handling for git storage restorationConsider adding try-catch block to handle potential file system operation failures.
-async function restoreGitStorageArchive(restoreContentsPath: string, backupName: string) { +async function restoreGitStorageArchive(restoreContentsPath: string, backupName: string): Promise<void> { try { console.log("Restoring git-storage archive"); // ... existing code ... } catch (error) { console.error("Failed to restore git storage:", error); throw error; } }
384-386: Add input validationConsider adding null/undefined check for more robust implementation.
-function isArchiveEncrypted(backupFilePath: string) { +function isArchiveEncrypted(backupFilePath: string): boolean { + if (!backupFilePath) return false; return backupFilePath.endsWith(".enc"); }
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
app/client/packages/rts/src/ctl/backup.ts (2)
Line range hint
27-147: Consider breaking down the run function into smaller, focused functionsThe run function is handling multiple responsibilities which makes it harder to maintain and test. Consider extracting the backup, encryption, and cleanup logic into separate functions.
312-317: Add error handling for symbolic link creationThe symbolic link creation could fail due to permissions or existing links. Consider adding specific error handling.
export async function executeCopyCMD(srcFolder: string, destFolder: string) { + try { return await utils.execCommand([ "ln", "-s", srcFolder, path.join(destFolder, "git-storage"), ]); + } catch (error) { + if (error.code === 'EEXIST') { + throw new Error(`Git storage directory already exists at ${destFolder}`); + } + throw error; + } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
app/client/packages/rts/src/ctl/backup.ts(16 hunks)app/client/packages/rts/src/ctl/restore.ts(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/packages/rts/src/ctl/restore.ts
🔇 Additional comments (5)
app/client/packages/rts/src/ctl/backup.ts (5)
13-25: LGTM! Well-structured class implementation
The BackupState class is well-implemented with proper TypeScript types and initialization of all properties.
147-147: Remove process.exit() call
Using process.exit() in an async function can lead to unfinished operations. Let the process exit naturally after all operations complete.
Line range hint 151-209: Strong implementation of encryption-related functions
Good error handling and proper type annotations. The encryption password validation is thorough with appropriate retry logic.
Line range hint 321-364: Well-structured utility functions
Functions are focused, well-typed, and handle their responsibilities clearly.
Line range hint 378-392: LGTM! Proper disk space validation
The implementation correctly checks for available space with appropriate error handling.
Now that we've moved to TypeScript, this PR fixes missing type annotations in `backup.ts` and `restore.ts`. ## 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/11988138792> > Commit: 5770dd0 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=11988138792&attempt=2" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Sanity` > Spec: > <hr>Sun, 24 Nov 2024 01:02:33 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 - **New Features** - Enhanced error handling for password mismatches during backup encryption. - Improved test coverage for backup functionality, including backup paths and disk space validation. - **Bug Fixes** - Refined backup cleanup logic to ensure proper file retention and removal. - **Refactor** - Introduced a new `BackupState` class to manage backup process state. - Streamlined password handling and improved code structure for better readability. - Updated type safety for several functions in the backup and restore processes. - Simplified exit process in error handling for restore functionality. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Now that we've moved to TypeScript, this PR fixes missing type annotations in
backup.tsandrestore.ts.Automation
/test sanity
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11988138792
Commit: 5770dd0
Cypress dashboard.
Tags:
@tag.SanitySpec:
Sun, 24 Nov 2024 01:02:33 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
BackupStateclass to manage backup process state.