feat: Introduce backup and restore for postgres #37326
Conversation
(cherry picked from commit 588e792)
WalkthroughThe changes in this pull request enhance the backup and restore functionalities to support both MongoDB and PostgreSQL databases. The backup script has been updated to conditionally handle database types, introducing new functions for PostgreSQL exports and imports. Additionally, utility functions have been renamed and improved for better error handling. The test suite has also been modified to reflect these updates, ensuring comprehensive coverage for both database types. 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
abhvsn
changed the title
abhvsn
added
Enhancement
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11784388675. |
1 similar comment
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11784388675. |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11784652237. |
|
Deploy-Preview-URL: https://ce-37326.dp.appsmith.com |
github-actions
bot
added
DB Infrastructure Pod
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Outside diff range and nitpick comments (7)
deploy/docker/fs/opt/appsmith/utils/bin/export_db.js (1)
Line range hint
43-65: Improve error handling and application state managementThe error handling could be more specific and ensure proper application restart in all scenarios.
+ const DatabaseExportError = class extends Error { + constructor(message, cause) { + super(message); + this.cause = cause; + this.name = 'DatabaseExportError'; + } + }; + try { check_supervisord_status_cmd = '/usr/bin/supervisorctl >/dev/null 2>&1 '; shell.exec(check_supervisord_status_cmd, function (code) { if (code > 0) { - shell.echo('application is not running, starting supervisord'); + throw new DatabaseExportError('Supervisord is not running'); shell.exec('/usr/bin/supervisord'); } });deploy/docker/fs/opt/appsmith/utils/bin/utils.js (1)
Add PostgreSQL support and remove MongoDB dependencies in utils.js
The
preprocessMongoDBURIfunction is still present and PostgreSQL support is not implemented.
deploy/docker/fs/opt/appsmith/utils/bin/utils.js(Line 125)🔗 Analysis chain
Line range hint
1-201: Verify PostgreSQL support in utility functionsSeveral utility functions in this file were originally designed for MongoDB. Let's verify they work correctly with PostgreSQL.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for PostgreSQL-specific handling in the codebase rg -l 'postgres|postgresql' --type js # Check for any potential MongoDB-specific operations that might need PostgreSQL equivalents ast-grep --pattern 'MongoClient|mongoose|mongodb'Length of output: 1305
Script:
#!/bin/bash # Verify PostgreSQL support within utils.js ast-grep --pattern 'PostgreSQL|pg' deploy/docker/fs/opt/appsmith/utils/bin/utils.js # Check if MongoDB functions are still being used or referenced in utils.js ast-grep --pattern 'preprocessMongoDBURI' deploy/docker/fs/opt/appsmith/utils/bin/utils.jsLength of output: 279
deploy/docker/fs/opt/appsmith/utils/bin/backup.test.js (1)
59-67: Add test cases for edge cases in PostgreSQL connection stringWhile the basic test case looks good, consider adding tests for:
- Invalid connection strings
- Connection strings with special characters
- Different PostgreSQL connection string formats
deploy/docker/fs/opt/appsmith/utils/bin/import_db.js (2)
38-46: Avoid redundantutils.getDburl()callsPass
dbUrlas a parameter toget_table_or_collection_lento prevent multiple calls.Apply this diff:
- function get_table_or_collection_len() { + function get_table_or_collection_len(dbUrl) {Update the call at line 63:
- const collectionsLen = get_table_or_collection_len(); + const collectionsLen = get_table_or_collection_len(dbUrl);
12-16: Handle unsupported database typesAdd an
elseclause to handle cases where the database type is neither MongoDB nor PostgreSQL.Apply this diff:
} else if (dbUrl.startsWith('postgresql')) { restore_postgres_db(dbUrl); + } else { + console.error(`Unsupported database type: ${dbUrl}`); + process.exit(1); }deploy/docker/fs/opt/appsmith/utils/bin/backup.js (1)
128-133: Store database URL in a variable to avoid multiple function callsCurrently,
utils.getDburl()is called multiple times. Consider storing the result in a variable for efficiency.Apply this diff to implement the change:
+ const dbUrl = utils.getDburl(); - if (utils.getDburl().startsWith('mongodb')) { + if (dbUrl.startsWith('mongodb')) { - await executeMongoDumpCMD(destFolder, utils.getDburl()) + await executeMongoDumpCMD(destFolder, dbUrl) - } else if (utils.getDburl().startsWith('postgresql')) { + } else if (dbUrl.startsWith('postgresql')) { - await executePostgresDumpCMD(destFolder, utils.getDburl()); + await executePostgresDumpCMD(destFolder, dbUrl); }deploy/docker/fs/opt/appsmith/utils/bin/restore.js (1)
64-65: Use consistent methods for database type checksFor consistency, use
dbUrl.startsWith('postgresql')instead ofdbUrl.includes('postgresql')when checking for PostgreSQL URLs.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/CommonDBConfig.java(1 hunks)deploy/docker/fs/opt/appsmith/utils/bin/backup.js(4 hunks)deploy/docker/fs/opt/appsmith/utils/bin/backup.test.js(2 hunks)deploy/docker/fs/opt/appsmith/utils/bin/export_db.js(1 hunks)deploy/docker/fs/opt/appsmith/utils/bin/import_db.js(3 hunks)deploy/docker/fs/opt/appsmith/utils/bin/restore.js(1 hunks)deploy/docker/fs/opt/appsmith/utils/bin/utils.js(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/CommonDBConfig.java
🔇 Additional comments (4)
deploy/docker/fs/opt/appsmith/utils/bin/utils.js (1)
198-199: LGTM: Module exports are properly updated
The new functions are correctly exported.
deploy/docker/fs/opt/appsmith/utils/bin/import_db.js (2)
26-26: Use the correct database name in pg_restore command
The database name is hardcoded as appsmith. Ensure this matches the database in dbUrl or extract the database name dynamically.
Consider updating the command:
- const cmd = `pg_restore -U postgres -d appsmith --verbose --clean ${Constants.RESTORE_PATH}/${Constants.DUMP_FILE_NAME}`;
+ const dbName = utils.getDbNameFromUrl(dbUrl);
+ const cmd = `pg_restore -U postgres -d ${dbName} --verbose --clean ${Constants.RESTORE_PATH}/${Constants.DUMP_FILE_NAME}`;Ensure you have a utility function getDbNameFromUrl to extract the database name.
25-28: 🛠️ Refactor suggestion
Declare restore_postgres_db with const and pass dbUrl as a parameter
For consistency and proper scope, declare the function with const and pass dbUrl.
Apply this diff:
- restore_postgres_db = (dbUrl) => {
+ const restore_postgres_db = (dbUrl) => {Likely invalid or redundant comment.
deploy/docker/fs/opt/appsmith/utils/bin/restore.js (1)
86-87: Verify getDatabaseNameFromDBURI supports PostgreSQL URIs
Confirm that utils.getDatabaseNameFromDBURI correctly parses database names from PostgreSQL URIs, as it now handles both MongoDB and PostgreSQL.
…ith into feat/backup-restore-pg
There was a problem hiding this comment.
Actionable comments posted: 9
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
deploy/docker/fs/opt/appsmith/utils/bin/backup.js(4 hunks)deploy/docker/fs/opt/appsmith/utils/bin/import_db.js(3 hunks)deploy/docker/fs/opt/appsmith/utils/bin/restore.js(1 hunks)
🧰 Additional context used
📓 Learnings (1)
deploy/docker/fs/opt/appsmith/utils/bin/backup.js (1)
Learnt from: abhvsn
PR: appsmithorg/appsmith#37326
File: deploy/docker/fs/opt/appsmith/utils/bin/backup.js:169-171
Timestamp: 2024-11-13T09:39:34.416Z
Learning: In `backup.js`, when using `pg_dump` in the `executePostgresDumpCMD` function, the `--dbname=` option is not mandatory; the PostgreSQL URL can be passed directly.
🔇 Additional comments (4)
deploy/docker/fs/opt/appsmith/utils/bin/backup.js (3)
166-168: LGTM!
The MongoDB dump command is correctly constructed with proper parameter usage.
150-150: LGTM!
The manifest file creation correctly uses the generic database name extraction function.
265-265: LGTM!
The new PostgreSQL function is correctly exported.
deploy/docker/fs/opt/appsmith/utils/bin/restore.js (1)
70-81: LGTM! Well-structured MongoDB restore implementation
The implementation correctly handles:
- Database name mapping
- Command construction with proper flags
- Error handling for manifest file
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
deploy/docker/fs/opt/appsmith/utils/bin/backup.js(4 hunks)deploy/docker/fs/opt/appsmith/utils/bin/export_db.js(2 hunks)deploy/docker/fs/opt/appsmith/utils/bin/import_db.js(3 hunks)
🧰 Additional context used
📓 Learnings (2)
deploy/docker/fs/opt/appsmith/utils/bin/backup.js (1)
Learnt from: abhvsn
PR: appsmithorg/appsmith#37326
File: deploy/docker/fs/opt/appsmith/utils/bin/backup.js:169-171
Timestamp: 2024-11-13T09:39:34.416Z
Learning: In `backup.js`, when using `pg_dump` in the `executePostgresDumpCMD` function, the `--dbname=` option is not mandatory; the PostgreSQL URL can be passed directly.
deploy/docker/fs/opt/appsmith/utils/bin/export_db.js (2)
Learnt from: abhvsn
PR: appsmithorg/appsmith#37326
File: deploy/docker/fs/opt/appsmith/utils/bin/export_db.js:11-17
Timestamp: 2024-11-13T09:41:48.119Z
Learning: In the `export_db.js` script located at `deploy/docker/fs/opt/appsmith/utils/bin/export_db.js`, it's acceptable to use simple string-based detection (e.g., `dbUrl.startsWith('mongodb')`) for determining the database type because the database URL is restrictive, and misconfiguration will cause the server to fail to start up. Therefore, using a URL parsing library for database URL detection is unnecessary in this context.
Learnt from: abhvsn
PR: appsmithorg/appsmith#37326
File: deploy/docker/fs/opt/appsmith/utils/bin/backup.js:169-171
Timestamp: 2024-11-13T09:39:34.416Z
Learning: In `backup.js`, when using `pg_dump` in the `executePostgresDumpCMD` function, the `--dbname=` option is not mandatory; the PostgreSQL URL can be passed directly.
🔇 Additional comments (6)
deploy/docker/fs/opt/appsmith/utils/bin/import_db.js (2)
69-69:
Update function call with database URL parameter
The call to get_table_or_collection_len() needs to be updated to match the new function signature.
- const collectionsLen = get_table_or_collection_len();
+ const dbUrl = utils.getDburl();
+ const collectionsLen = get_table_or_collection_len(dbUrl);Likely invalid or redundant comment.
20-23:
Critical: Add security and error handling to MongoDB restore
The current implementation is vulnerable to command injection and lacks proper error handling.
restore_mongo_db = (dbUrl) => {
+ if (!shell.test('-f', `${Constants.RESTORE_PATH}/${Constants.DUMP_FILE_NAME}`)) {
+ throw new Error('MongoDB dump file not found');
+ }
+ const sanitizedUrl = dbUrl.replace(/[;&|`$]/g, '');
- const cmd = `mongorestore --uri='${dbUrl}' --drop --archive='${Constants.RESTORE_PATH}/${Constants.DUMP_FILE_NAME}' --gzip`;
+ const cmd = `mongorestore --uri='${sanitizedUrl}' --drop --archive='${Constants.RESTORE_PATH}/${Constants.DUMP_FILE_NAME}' --gzip`;
- shell.exec(cmd);
+ const result = shell.exec(cmd);
+ if (result.code !== 0) {
+ throw new Error(`MongoDB restore failed: ${result.stderr}`);
+ }
}Likely invalid or redundant comment.
deploy/docker/fs/opt/appsmith/utils/bin/backup.js (4)
166-168: LGTM! MongoDB dump implementation is solid
The command correctly uses mongodump with appropriate flags for URI connection, compression, and archiving.
150-150: LGTM! Manifest update correctly supports multiple databases
The change appropriately uses the generic database name extraction function.
265-265: LGTM! Module exports properly include new PostgreSQL function
The new function is correctly exported for external use.
128-134:
Add error handling for database URL validation
The database type detection needs proper validation and error handling.
Apply this diff:
const dbUrl = utils.getDburl();
// Check the DB url
+ if (!dbUrl) {
+ throw new Error('Database URL is not configured');
+ }
if (dbUrl.startsWith('mongodb')) {
await executeMongoDumpCMD(destFolder, dbUrl);
} else if (dbUrl.startsWith('postgresql')) {
await executePostgresDumpCMD(destFolder, dbUrl);
+ } else {
+ throw new Error(`Unsupported database type in URL: ${dbUrl}`);
}Likely invalid or redundant comment.
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (3)
deploy/docker/fs/opt/appsmith/utils/bin/backup.js (1)
152-152: Add version validation in manifestEnsure version is not null before creating manifest.
- const manifest_data = { "appsmithVersion": version, "dbName": utils.getDatabaseNameFromDBURI(utils.getDburl()) } + if (!version) { + throw new Error('Unable to determine Appsmith version'); + } + const manifest_data = { + "appsmithVersion": version, + "dbName": utils.getDatabaseNameFromDBURI(utils.getDburl()), + "backupType": utils.getDburl().startsWith('mongodb') ? 'mongodb' : 'postgresql' + }deploy/docker/fs/opt/appsmith/utils/bin/backup.test.js (2)
58-66: Add error handling test cases for PostgreSQL dump commandWhile the happy path is covered, consider adding test cases for error scenarios such as invalid connection strings or failed dump operations.
Example test case to add:
test('Test postgresdump CMD with invalid URI', async () => { var dest = '/dest' var invalidURI = 'invalid://uri' await expect(backup.executePostgresDumpCMD(dest, invalidURI)).rejects.toThrow() })
266-276: Add more edge cases for PostgreSQL URI parsingConsider adding test cases for:
- URIs with special characters in database name
- URIs with multiple query parameters
- URIs with non-standard ports
Example test cases to add:
test('Get DB name from PostgreSQL URI with special characters', async () => { var pg_uri = "postgresql://user:password@host:5432/postgres-db_1" var expectedDBName = 'postgres-db_1' expect(utils.getDatabaseNameFromDBURI(pg_uri)).toEqual(expectedDBName) }) test('Get DB name from PostgreSQL URI with multiple query params', async () => { var pg_uri = "postgresql://user:password@host:5432/postgres_db?sslmode=verify-full&target_session_attrs=read-write" var expectedDBName = 'postgres_db' expect(utils.getDatabaseNameFromDBURI(pg_uri)).toEqual(expectedDBName) })
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
deploy/docker/fs/opt/appsmith/utils/bin/backup.js(4 hunks)deploy/docker/fs/opt/appsmith/utils/bin/backup.test.js(2 hunks)deploy/docker/fs/opt/appsmith/utils/bin/export_db.js(2 hunks)deploy/docker/fs/opt/appsmith/utils/bin/import_db.js(3 hunks)deploy/docker/fs/opt/appsmith/utils/bin/restore.js(1 hunks)deploy/docker/fs/opt/appsmith/utils/bin/utils.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- deploy/docker/fs/opt/appsmith/utils/bin/import_db.js
- deploy/docker/fs/opt/appsmith/utils/bin/utils.js
🧰 Additional context used
📓 Learnings (3)
deploy/docker/fs/opt/appsmith/utils/bin/backup.js (1)
Learnt from: abhvsn
PR: appsmithorg/appsmith#37326
File: deploy/docker/fs/opt/appsmith/utils/bin/backup.js:169-171
Timestamp: 2024-11-13T09:39:34.416Z
Learning: In `backup.js`, when using `pg_dump` in the `executePostgresDumpCMD` function, the `--dbname=` option is not mandatory; the PostgreSQL URL can be passed directly.
deploy/docker/fs/opt/appsmith/utils/bin/backup.test.js (1)
Learnt from: abhvsn
PR: appsmithorg/appsmith#37326
File: deploy/docker/fs/opt/appsmith/utils/bin/backup.js:169-171
Timestamp: 2024-11-13T09:39:34.416Z
Learning: In `backup.js`, when using `pg_dump` in the `executePostgresDumpCMD` function, the `--dbname=` option is not mandatory; the PostgreSQL URL can be passed directly.
deploy/docker/fs/opt/appsmith/utils/bin/export_db.js (2)
Learnt from: abhvsn
PR: appsmithorg/appsmith#37326
File: deploy/docker/fs/opt/appsmith/utils/bin/export_db.js:11-17
Timestamp: 2024-11-13T09:41:48.119Z
Learning: In the `export_db.js` script located at `deploy/docker/fs/opt/appsmith/utils/bin/export_db.js`, it's acceptable to use simple string-based detection (e.g., `dbUrl.startsWith('mongodb')`) for determining the database type because the database URL is restrictive, and misconfiguration will cause the server to fail to start up. Therefore, using a URL parsing library for database URL detection is unnecessary in this context.
Learnt from: abhvsn
PR: appsmithorg/appsmith#37326
File: deploy/docker/fs/opt/appsmith/utils/bin/backup.js:169-171
Timestamp: 2024-11-13T09:39:34.416Z
Learning: In `backup.js`, when using `pg_dump` in the `executePostgresDumpCMD` function, the `--dbname=` option is not mandatory; the PostgreSQL URL can be passed directly.
🔇 Additional comments (4)
deploy/docker/fs/opt/appsmith/utils/bin/export_db.js (1)
11-18:
Security: Protect against command injection
Direct string interpolation of dbUrl in shell commands is unsafe. Implement proper sanitization.
+const sanitizeUrl = (url) => url.replace(/[$&;`|*?~<>^()!{}[\]]/g, '\\$&');
+const sanitizedDbUrl = sanitizeUrl(dbUrl);
if (dbUrl.startsWith('mongodb')) {
- cmd = `mongodump --uri='${dbUrl}' --archive='${Constants.BACKUP_PATH}/${Constants.DUMP_FILE_NAME}' --gzip`;
+ cmd = `mongodump --uri='${sanitizedDbUrl}' --archive='${Constants.BACKUP_PATH}/${Constants.DUMP_FILE_NAME}' --gzip`;
} else if (dbUrl.startsWith('postgresql')) {
- cmd = `pg_dump ${dbUrl} -n appsmith -Fc -f '${Constants.BACKUP_PATH}/${Constants.DUMP_FILE_NAME}'`;
+ cmd = `pg_dump '${sanitizedDbUrl}' -n appsmith -Fc -f '${Constants.BACKUP_PATH}/${Constants.DUMP_FILE_NAME}'`;deploy/docker/fs/opt/appsmith/utils/bin/backup.js (1)
268-268: LGTM!
The new PostgreSQL function is properly exported.
deploy/docker/fs/opt/appsmith/utils/bin/backup.test.js (1)
241-264: LGTM! MongoDB URI parsing tests are comprehensive
The tests cover various MongoDB URI formats and have been correctly updated to use the new database-agnostic function name.
deploy/docker/fs/opt/appsmith/utils/bin/restore.js (1)
72-83: MongoDB restore implementation looks good
The implementation correctly handles database name mapping and includes appropriate error handling.
| cmd = `pg_dump ${dbUrl} -n appsmith -Fc -f '${Constants.BACKUP_PATH}/${Constants.DUMP_FILE_NAME}'`; | ||
| } else { | ||
| throw new Error('Unsupported database type, only MongoDB and PostgreSQL are supported'); | ||
| } | ||
| shell.exec(cmd); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for database export commands
The command execution should handle potential failures and provide meaningful error messages.
-shell.exec(cmd);
+const result = shell.exec(cmd);
+if (result.code !== 0) {
+ throw new Error(`Database export failed: ${result.stderr}`);
+}📝 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.
| shell.exec(cmd); | |
| const result = shell.exec(cmd); | |
| if (result.code !== 0) { | |
| throw new Error(`Database export failed: ${result.stderr}`); | |
| } |
| const dbUrl = utils.getDburl(); | ||
| // Check the DB url | ||
| if (dbUrl.startsWith('mongodb')) { | ||
| await executeMongoDumpCMD(destFolder, dbUrl); | ||
| } else if (dbUrl.startsWith('postgresql')) { | ||
| await executePostgresDumpCMD(destFolder, dbUrl); | ||
| } else { | ||
| throw new Error(`Unsupported database type in URL: ${dbUrl}`); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for database URL
The database URL should be validated before checking its type.
const dbUrl = utils.getDburl();
// Check the DB url
+ if (!dbUrl) {
+ throw new Error('Database URL is not configured');
+ }
if (dbUrl.startsWith('mongodb')) {📝 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 dbUrl = utils.getDburl(); | |
| // Check the DB url | |
| if (dbUrl.startsWith('mongodb')) { | |
| await executeMongoDumpCMD(destFolder, dbUrl); | |
| } else if (dbUrl.startsWith('postgresql')) { | |
| await executePostgresDumpCMD(destFolder, dbUrl); | |
| } else { | |
| throw new Error(`Unsupported database type in URL: ${dbUrl}`); | |
| } | |
| const dbUrl = utils.getDburl(); | |
| // Check the DB url | |
| if (!dbUrl) { | |
| throw new Error('Database URL is not configured'); | |
| } | |
| if (dbUrl.startsWith('mongodb')) { | |
| await executeMongoDumpCMD(destFolder, dbUrl); | |
| } else if (dbUrl.startsWith('postgresql')) { | |
| await executePostgresDumpCMD(destFolder, dbUrl); | |
| } else { | |
| throw new Error(`Unsupported database type in URL: ${dbUrl}`); | |
| } |
| async function executeMongoDumpCMD(destFolder, dbUrl) { | ||
| return await utils.execCommand(['mongodump', `--uri=${dbUrl}`, `--archive=${destFolder}/mongodb-data.gz`, '--gzip']);// generate cmd | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for MongoDB dump
Add try-catch block to handle potential MongoDB dump failures.
async function executeMongoDumpCMD(destFolder, dbUrl) {
- return await utils.execCommand(['mongodump', `--uri=${dbUrl}`, `--archive=${destFolder}/mongodb-data.gz`, '--gzip']);
+ try {
+ return await utils.execCommand(['mongodump', `--uri=${dbUrl}`, `--archive=${destFolder}/mongodb-data.gz`, '--gzip']);
+ } catch (error) {
+ throw new Error(`MongoDB dump failed: ${error.message}`);
+ }
}📝 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.
| async function executeMongoDumpCMD(destFolder, dbUrl) { | |
| return await utils.execCommand(['mongodump', `--uri=${dbUrl}`, `--archive=${destFolder}/mongodb-data.gz`, '--gzip']);// generate cmd | |
| } | |
| async function executeMongoDumpCMD(destFolder, dbUrl) { | |
| try { | |
| return await utils.execCommand(['mongodump', `--uri=${dbUrl}`, `--archive=${destFolder}/mongodb-data.gz`, '--gzip']); | |
| } catch (error) { | |
| throw new Error(`MongoDB dump failed: ${error.message}`); | |
| } | |
| } |
| async function executePostgresDumpCMD(destFolder, dbUrl) { | ||
| return await utils.execCommand(['pg_dump', dbUrl, '-n', 'appsmith','-Fc', '-f', destFolder + '/pg-data.archive']); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance PostgreSQL dump reliability
Add error handling and schema existence check.
async function executePostgresDumpCMD(destFolder, dbUrl) {
- return await utils.execCommand(['pg_dump', dbUrl, '-n', 'appsmith','-Fc', '-f', destFolder + '/pg-data.archive']);
+ try {
+ // Check if schema exists
+ await utils.execCommand(['psql', dbUrl, '-c', "SELECT schema_name FROM information_schema.schemata WHERE schema_name = 'appsmith'"]);
+ return await utils.execCommand(['pg_dump', dbUrl, '-n', 'appsmith', '-Fc', '-f', destFolder + '/pg-data.archive']);
+ } catch (error) {
+ if (error.message.includes('schema_name')) {
+ throw new Error('Schema "appsmith" not found in database');
+ }
+ throw new Error(`PostgreSQL dump failed: ${error.message}`);
+ }
}📝 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.
| async function executePostgresDumpCMD(destFolder, dbUrl) { | |
| return await utils.execCommand(['pg_dump', dbUrl, '-n', 'appsmith','-Fc', '-f', destFolder + '/pg-data.archive']); | |
| async function executePostgresDumpCMD(destFolder, dbUrl) { | |
| try { | |
| // Check if schema exists | |
| await utils.execCommand(['psql', dbUrl, '-c', "SELECT schema_name FROM information_schema.schemata WHERE schema_name = 'appsmith'"]); | |
| return await utils.execCommand(['pg_dump', dbUrl, '-n', 'appsmith', '-Fc', '-f', destFolder + '/pg-data.archive']); | |
| } catch (error) { | |
| if (error.message.includes('schema_name')) { | |
| throw new Error('Schema "appsmith" not found in database'); | |
| } | |
| throw new Error(`PostgreSQL dump failed: ${error.message}`); | |
| } | |
| } |
| if (dbUrl.startsWith('mongodb')) { | ||
| await restore_mongo_db(restoreContentsPath, dbUrl); | ||
| } else if (dbUrl.includes('postgresql')) { | ||
| await restore_postgres_db(restoreContentsPath, dbUrl); | ||
| } else { | ||
| throw new Error('Unsupported database type, only MongoDB and PostgreSQL are supported'); | ||
| } | ||
| console.log('Restoring database completed'); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use URL parsing for database type detection
The current string-based database type detection is fragile. Consider using URL parsing for more robust type detection.
async function restoreDatabase(restoreContentsPath, dbUrl) {
console.log('Restoring database...');
- if (dbUrl.startsWith('mongodb')) {
- await restore_mongo_db(restoreContentsPath, dbUrl);
- } else if (dbUrl.includes('postgresql')) {
- await restore_postgres_db(restoreContentsPath, dbUrl);
+ const url = new URL(dbUrl);
+ const protocol = url.protocol.replace(':', '');
+ switch(protocol) {
+ case 'mongodb':
+ await restore_mongo_db(restoreContentsPath, dbUrl);
+ break;
+ case 'postgresql':
+ await restore_postgres_db(restoreContentsPath, dbUrl);
+ break;
+ default:
+ throw new Error(`Unsupported database type: ${protocol}. Only MongoDB and PostgreSQL are supported`);
}
console.log('Restoring database completed');
}📝 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.
| if (dbUrl.startsWith('mongodb')) { | |
| await restore_mongo_db(restoreContentsPath, dbUrl); | |
| } else if (dbUrl.includes('postgresql')) { | |
| await restore_postgres_db(restoreContentsPath, dbUrl); | |
| } else { | |
| throw new Error('Unsupported database type, only MongoDB and PostgreSQL are supported'); | |
| } | |
| console.log('Restoring database completed'); | |
| } | |
| async function restoreDatabase(restoreContentsPath, dbUrl) { | |
| console.log('Restoring database...'); | |
| const url = new URL(dbUrl); | |
| const protocol = url.protocol.replace(':', ''); | |
| switch(protocol) { | |
| case 'mongodb': | |
| await restore_mongo_db(restoreContentsPath, dbUrl); | |
| break; | |
| case 'postgresql': | |
| await restore_postgres_db(restoreContentsPath, dbUrl); | |
| break; | |
| default: | |
| throw new Error(`Unsupported database type: ${protocol}. Only MongoDB and PostgreSQL are supported`); | |
| } | |
| console.log('Restoring database completed'); | |
| } |
| async function restore_postgres_db(restoreContentsPath, dbUrl) { | ||
| const cmd = ['pg_restore', '--verbose', '--clean', `${restoreContentsPath}/pg-data.archive`]; | ||
| const url = new URL(dbUrl); | ||
| const isLocalhost = ['localhost', '127.0.0.1'].includes(url.hostname); | ||
| if (isLocalhost) { | ||
| let dbName; | ||
| try { | ||
| dbName = utils.getDatabaseNameFromDBURI(dbUrl); | ||
| console.log("Restoring database to " + dbName); | ||
| } catch (error) { | ||
| console.warn('Error reading manifest file. Assuming same database name as appsmith.', error); | ||
| dbName = 'appsmith'; | ||
| } | ||
| cmd.push('-d' , 'postgresql://localhost:5432/' + dbName); | ||
| // Use default user for local postgres | ||
| cmd.push('-U', 'postgres'); | ||
| } else { | ||
| cmd.push('-d', dbUrl); | ||
| } | ||
| await utils.execCommand(cmd); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve PostgreSQL restore implementation
The implementation needs improvements in credential handling and error management:
- Hardcoded 'postgres' user for local connections
- Missing error handling for pg_restore command
async function restore_postgres_db(restoreContentsPath, dbUrl) {
const cmd = ['pg_restore', '--verbose', '--clean', `${restoreContentsPath}/pg-data.archive`];
const url = new URL(dbUrl);
const isLocalhost = ['localhost', '127.0.0.1'].includes(url.hostname);
if (isLocalhost) {
let dbName;
try {
dbName = utils.getDatabaseNameFromDBURI(dbUrl);
console.log("Restoring database to " + dbName);
} catch (error) {
console.warn('Error reading manifest file. Assuming same database name as appsmith.', error);
dbName = 'appsmith';
}
cmd.push('-d' , 'postgresql://localhost:5432/' + dbName);
- cmd.push('-U', 'postgres');
+ const pgUser = process.env.POSTGRES_USER || 'postgres';
+ cmd.push('-U', pgUser);
} else {
cmd.push('-d', dbUrl);
}
- await utils.execCommand(cmd);
+ try {
+ await utils.execCommand(cmd);
+ } catch (error) {
+ throw new Error(`PostgreSQL restore failed: ${error.message}`);
+ }
}📝 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.
| async function restore_postgres_db(restoreContentsPath, dbUrl) { | |
| const cmd = ['pg_restore', '--verbose', '--clean', `${restoreContentsPath}/pg-data.archive`]; | |
| const url = new URL(dbUrl); | |
| const isLocalhost = ['localhost', '127.0.0.1'].includes(url.hostname); | |
| if (isLocalhost) { | |
| let dbName; | |
| try { | |
| dbName = utils.getDatabaseNameFromDBURI(dbUrl); | |
| console.log("Restoring database to " + dbName); | |
| } catch (error) { | |
| console.warn('Error reading manifest file. Assuming same database name as appsmith.', error); | |
| dbName = 'appsmith'; | |
| } | |
| cmd.push('-d' , 'postgresql://localhost:5432/' + dbName); | |
| // Use default user for local postgres | |
| cmd.push('-U', 'postgres'); | |
| } else { | |
| cmd.push('-d', dbUrl); | |
| } | |
| await utils.execCommand(cmd); | |
| async function restore_postgres_db(restoreContentsPath, dbUrl) { | |
| const cmd = ['pg_restore', '--verbose', '--clean', `${restoreContentsPath}/pg-data.archive`]; | |
| const url = new URL(dbUrl); | |
| const isLocalhost = ['localhost', '127.0.0.1'].includes(url.hostname); | |
| if (isLocalhost) { | |
| let dbName; | |
| try { | |
| dbName = utils.getDatabaseNameFromDBURI(dbUrl); | |
| console.log("Restoring database to " + dbName); | |
| } catch (error) { | |
| console.warn('Error reading manifest file. Assuming same database name as appsmith.', error); | |
| dbName = 'appsmith'; | |
| } | |
| cmd.push('-d' , 'postgresql://localhost:5432/' + dbName); | |
| const pgUser = process.env.POSTGRES_USER || 'postgres'; | |
| cmd.push('-U', pgUser); | |
| } else { | |
| cmd.push('-d', dbUrl); | |
| } | |
| try { | |
| await utils.execCommand(cmd); | |
| } catch (error) { | |
| throw new Error(`PostgreSQL restore failed: ${error.message}`); | |
| } | |
| } |
Description
PR to add backup and restore functionality for Postgres.
Q: Do we want to differentiate the mongo and postgres backups, to avoid the confusion in future? Also do we want to implement that right away because with that we need to change the docs immeditely.
Fixes #35369 #36947
/test Sanity
🔍 Cypress test results
Caution
🔴 🔴 🔴 Some tests have failed.
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11837815687
Commit: ecd642f
Cypress dashboard.
Tags: @tag.Sanity
Spec:
The following are new failures, please fix them before merging the PR:
- cypress/e2e/Sanity/Datasources/DatasourceForm_spec.js
List of identified flaky tests.Thu, 14 Nov 2024 13:39:52 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Release Notes
New Features
Improvements
Documentation
These updates significantly enhance the flexibility and usability of database operations within the application.