feat: Introduce backup and restore for postgres #37326
Conversation
(cherry picked from commit 588e792)
WalkthroughThe changes in this pull request enhance backup functionality by integrating PostgreSQL support alongside existing MongoDB capabilities. Modifications include the addition of a new Changes
Possibly related PRs
Suggested labels
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 20 minutes and 20 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. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (8)
app/client/packages/rts/src/ctl/backup/links/ManifestLink.ts (1)
16-18: Consider adding database type to manifest metadataSince the system now supports both MongoDB and PostgreSQL, it would be helpful to include the database type in the manifest for better clarity during restore operations.
const manifestData = { appsmithVersion: version, dbName: utils.getDatabaseNameFromUrl(utils.getDburl()), + dbType: utils.getDatabaseType(utils.getDburl()), };app/client/packages/rts/src/ctl/backup/links/MongoDumpLink.ts (1)
14-18: Consider using a more specific MongoDB URL checkWhile the current implementation works, using
startsWith("mongodb")might be too permissive.Consider using a more specific check:
- if (url.startsWith("mongodb")) { + if (url.startsWith("mongodb://") || url.startsWith("mongodb+srv://")) {app/client/packages/rts/src/ctl/backup/index.ts (3)
11-18: Consider enhancing URL validation robustnessWhile the current validation works, consider using URL parsing to ensure valid database URLs.
- if (!url.startsWith("mongodb") && !url.startsWith("postgresql")) { + try { + const dbUrl = new URL(url); + const protocol = dbUrl.protocol.replace(':', ''); + if (!['mongodb', 'postgresql'].includes(protocol)) { + throw new Error('Unsupported database protocol'); + } + } catch (error) { console.error("Only MongoDB and Postgres databases are supported."); process.exitCode = 1; return; + }
Line range hint
24-29: Consider conditional link inclusion based on database typeCurrently, both MongoDB and PostgreSQL dump links are included in the chain regardless of the database type. Consider conditionally including only the relevant dump link.
- new linkClasses.MongoDumpLink(state), - new linkClasses.PostgresDumpLink(state), + ...(url.startsWith('mongodb') ? [new linkClasses.MongoDumpLink(state)] : []), + ...(url.startsWith('postgresql') ? [new linkClasses.PostgresDumpLink(state)] : []),
Line range hint
93-94: Avoid explicit process.exit() in finally blockThe explicit
process.exit()call might prevent cleanup operations from completing. Consider letting the process exit naturally.- await postBackupCleanup(); - process.exit(); + await postBackupCleanup();app/client/packages/rts/src/ctl/backup/backup.test.ts (1)
291-305: Improve test descriptions for clarityThe test descriptions could be more specific about the scenarios being tested.
-test("Get DB name from Postgres URL", async () => { +test("Should extract database name from basic PostgreSQL URL without query parameters", async () => { // ... rest of the test }); -test("Get DB name from Postgres URL with query params", async () => { +test("Should extract database name from PostgreSQL URL with SSL configuration", async () => { // ... rest of the test });app/client/packages/rts/src/ctl/restore.ts (2)
157-191: Allow configurable PostgreSQL credentials for local restoresThe
restorePostgresfunction assumes the usernamepostgresfor localhost restores. To support different local configurations, consider making the username and password configurable.
165-165: Include IPv6 localhost address for completenessTo ensure compatibility with IPv6 environments, consider including
'::1'in the list of localhost addresses.Apply this diff to update the localhost check:
const isLocalhost = ["localhost", "127.0.0.1"].includes(url.hostname); +const isLocalhost = ["localhost", "127.0.0.1", "::1"].includes(url.hostname);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (8)
app/client/packages/rts/src/ctl/backup/backup.test.ts(5 hunks)app/client/packages/rts/src/ctl/backup/index.ts(2 hunks)app/client/packages/rts/src/ctl/backup/links/ManifestLink.ts(1 hunks)app/client/packages/rts/src/ctl/backup/links/MongoDumpLink.ts(1 hunks)app/client/packages/rts/src/ctl/backup/links/PostgresDumpLink.ts(1 hunks)app/client/packages/rts/src/ctl/backup/links/index.ts(1 hunks)app/client/packages/rts/src/ctl/restore.ts(3 hunks)app/client/packages/rts/src/ctl/utils.ts(1 hunks)
🧰 Additional context used
📓 Learnings (1)
app/client/packages/rts/src/ctl/backup/links/MongoDumpLink.ts (3)
Learnt from: sharat87
PR: appsmithorg/appsmith#37829
File: app/client/packages/rts/src/ctl/backup/links/MongoDumpLink.ts:1-25
Timestamp: 2024-11-29T09:32:10.654Z
Learning: In the backup process, exceptions are caught outside of individual link methods. Therefore, adding try-catch blocks within link methods like `MongoDumpLink`'s `doBackup()` method is unnecessary.
Learnt from: sharat87
PR: appsmithorg/appsmith#37829
File: app/client/packages/rts/src/ctl/backup/links/MongoDumpLink.ts:11-15
Timestamp: 2024-11-29T09:31:54.733Z
Learning: In the backup links, exceptions are handled outside of the `doBackup()` methods, so internal methods do not need to include additional try-catch blocks.
Learnt from: sharat87
PR: appsmithorg/appsmith#37829
File: app/client/packages/rts/src/ctl/backup/links/MongoDumpLink.ts:18-25
Timestamp: 2024-11-29T09:36:52.112Z
Learning: In `app/client/packages/rts/src/ctl/backup/links/MongoDumpLink.ts`, the `destFolder` parameter in `executeMongoDumpCMD` already includes randomness, so there's no need to append a timestamp to the backup file name.
🔇 Additional comments (7)
app/client/packages/rts/src/ctl/backup/links/index.ts (1)
19-19: LGTM! Verify PostgresDumpLink implementation.
The addition of PostgresDumpLink export aligns well with the existing architecture pattern.
Let's verify the PostgresDumpLink implementation:
✅ Verification successful
Let me verify the Link interface implementation to ensure all required methods are properly handled.
PostgresDumpLink implementation matches the Link interface pattern
The implementation follows the same pattern as other Link implementations (MongoDumpLink, etc.) and correctly implements the optional Link interface methods. The doBackup method handles PostgreSQL database export similar to how MongoDumpLink handles MongoDB exports.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify PostgresDumpLink implements Link interface correctly
# and check for related documentation updates
# Check PostgresDumpLink implementation
ast-grep --pattern 'class PostgresDumpLink implements Link {
$$$
}'
# Look for documentation files that might need updates
fd -e md -e mdx . | xargs rg -l "backup|restore|postgres|postgresql"
Length of output: 1500
Script:
#!/bin/bash
# Check Link interface definition
ast-grep --pattern 'interface Link {
$$$
}'
# Check if there are any other Link implementations for reference
ast-grep --pattern 'class $_ implements Link {
$$$
}'
Length of output: 14231
app/client/packages/rts/src/ctl/backup/links/ManifestLink.ts (1)
17-17: Verify database URL parsing compatibility
The change from getDatabaseNameFromMongoURI to getDatabaseNameFromUrl looks good, as it aligns with the new PostgreSQL support.
Let's verify the implementation of the new function:
✅ Verification successful
Function implementation is safe and well-tested
The getDatabaseNameFromUrl function is properly tested for both MongoDB and PostgreSQL URLs, with test cases covering various scenarios including URLs with query parameters. The implementation correctly extracts database names from both types of database URLs by splitting on "/" and removing query parameters.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the implementation of getDatabaseNameFromUrl
# to ensure it handles both MongoDB and PostgreSQL URLs correctly
ast-grep --pattern 'function getDatabaseNameFromUrl($_) {
$$$
}'
Length of output: 430
Script:
#!/bin/bash
# Let's check the test cases to verify URL parsing for both MongoDB and PostgreSQL
rg -A 5 "getDatabaseNameFromUrl" --type ts --type test
Length of output: 83
Script:
#!/bin/bash
# Let's check test files for getDatabaseNameFromUrl
rg -A 5 "getDatabaseNameFromUrl" --type ts
# Also check for any usage patterns to understand how it's being used
rg "getDburl\(\)" --type ts -A 2
Length of output: 7343
app/client/packages/rts/src/ctl/backup/links/MongoDumpLink.ts (1)
12-13: LGTM: Clean URL retrieval implementation
The URL retrieval is cleanly separated into its own statement for better readability.
app/client/packages/rts/src/ctl/backup/links/PostgresDumpLink.ts (1)
1-33: Verify backup system integration and security
Please ensure:
- Integration with the broader backup system's error handling and logging
- Secure handling of database credentials in logs
- Proper cleanup of backup files in case of failures
Let's verify the integration:
✅ Verification successful
Based on the shell script results, I can now generate the final response:
Integration and error handling are properly implemented
The backup system shows good architectural patterns:
- Error handling is centralized in the main backup flow (
index.ts) - Secure credential handling through the
EncryptionLink - Multiple backup implementations following the
Linkinterface - Cleanup is managed by
BackupFolderLinkusing temporary directories
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for other backup implementations and common patterns
rg -A 5 "implements Link" "app/client/packages/rts/src/ctl/backup"
# Check for error handling patterns
rg "catch|throw|error" "app/client/packages/rts/src/ctl/backup"
# Look for credential handling
rg "password|secret|credential" "app/client/packages/rts/src/ctl/backup"
Length of output: 10501
app/client/packages/rts/src/ctl/backup/index.ts (1)
29-29: Verify PostgresDumpLink implementation
Ensure that the PostgresDumpLink class is properly implemented with all necessary backup functionality.
✅ Verification successful
Let me search for the PostgresDumpLink implementation specifically.
Let me check the complete implementation of PostgresDumpLink and its utility function.
Let me check for test coverage with the correct file pattern.
PostgresDumpLink implementation is properly structured
The PostgresDumpLink class is correctly implemented with:
- Proper interface implementation of the Link contract
- Conditional database export based on PostgreSQL URL check
- Appropriate pg_dump command execution with required parameters (schema, format, and output file)
- Clear console logging for operation status
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for PostgresDumpLink implementation
ast-grep --pattern 'class PostgresDumpLink {
$$$
}'
# Check for backup-related methods
rg -A 5 "doBackup|preBackup|postBackup" --type ts
Length of output: 9861
Script:
#!/bin/bash
# Search for PostgresDumpLink class implementation and its dependencies
ast-grep --pattern 'class PostgresDumpLink {
$$$
}'
# Look for PostgreSQL dump related utilities
rg -A 5 "executePostgresDumpCMD" --type ts
Length of output: 2077
Script:
#!/bin/bash
# Get the full content of PostgresDumpLink.ts
fd PostgresDumpLink.ts --exec cat {}
# Check for any test coverage
rg -A 5 "PostgresDumpLink" --type test
Length of output: 905
Script:
#!/bin/bash
# Check test coverage in .test.ts files
rg -A 5 "PostgresDumpLink" --type ts ".*\.test\.ts$"
# Check if pg_dump command is tested
rg -A 5 "pg_dump" --type ts ".*\.test\.ts$"
Length of output: 204
app/client/packages/rts/src/ctl/utils.ts (1)
252-260: Verify database URL handling across the codebase
Since this function now handles both MongoDB and PostgreSQL URLs, we should verify its usage.
✅ Verification successful
Based on the search results, I can generate the final response since I have sufficient information to verify the database URL handling across the codebase.
Function correctly handles both MongoDB and PostgreSQL URLs
The getDatabaseNameFromUrl function is used correctly across the codebase for both MongoDB and PostgreSQL URLs, as evidenced by:
- Test coverage in
backup.test.tsincludes both URL types:- MongoDB:
mongodb://appsmith:pAssW0rd!@localhost:27017/appsmith - PostgreSQL:
postgresql://user:password@host:5432/postgres_db
- MongoDB:
- All usages in production code (
ManifestLink.ts,restore.ts) handle both URL types consistently
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all usages of getDatabaseNameFromUrl
rg "getDatabaseNameFromUrl" -A 2
# Search for MongoDB and PostgreSQL connection strings to verify URL patterns
rg -i "mongodb://|postgresql://" -A 2
Length of output: 30815
app/client/packages/rts/src/ctl/backup/backup.test.ts (1)
11-11: LGTM: Import added for PostgreSQL support
app/client/packages/rts/src/ctl/backup/links/PostgresDumpLink.ts
Outdated
Show resolved
Hide resolved
app/client/packages/rts/src/ctl/backup/links/PostgresDumpLink.ts
Outdated
Show resolved
Hide resolved
app/client/packages/rts/src/ctl/backup/links/PostgresDumpLink.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
app/client/packages/rts/src/ctl/backup/backup.test.ts (1)
78-78: Remove debug console.log statementDebug statements should not be committed to production code.
- console.log(res);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
app/client/packages/rts/src/ctl/backup/backup.test.ts(5 hunks)
🔇 Additional comments (2)
app/client/packages/rts/src/ctl/backup/backup.test.ts (2)
72-72: Fix misleading variable name
The variable appsmithMongoURI contains a PostgreSQL URI, which is confusing.
- const appsmithMongoURI = "postgresql://username:password@host/appsmith";
+ const postgresURI = "postgresql://username:password@host/appsmith";291-305: LGTM! Good test coverage for PostgreSQL database name extraction
The tests comprehensively cover PostgreSQL URL parsing with and without query parameters.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
app/client/packages/rts/src/ctl/restore.ts (1)
Line range hint
139-156: LGTM with a minor suggestion for error handlingThe migration to
getDatabaseNameFromUrland the command construction look good. Consider being more specific in the error message about what went wrong with the manifest file.} catch (error) { console.warn( - "Error reading manifest file. Assuming same database name.", + "Failed to read database name from manifest file. Using default database name.", error, ); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
app/client/packages/rts/src/ctl/backup/backup.test.ts(5 hunks)app/client/packages/rts/src/ctl/backup/links/PostgresDumpLink.ts(1 hunks)app/client/packages/rts/src/ctl/restore.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/client/packages/rts/src/ctl/backup/backup.test.ts
- app/client/packages/rts/src/ctl/backup/links/PostgresDumpLink.ts
🔇 Additional comments (1)
app/client/packages/rts/src/ctl/restore.ts (1)
114-126:
Improve database type detection logic
The current string matching approach is fragile, especially for PostgreSQL URLs. Consider using URL parsing for more reliable database type detection.
- if (dbUrl.startsWith("mongodb")) {
- await restoreMongoDB(restoreContentsPath, dbUrl);
- } else if (dbUrl.includes("postgresql")) {
+ const url = new URL(dbUrl);
+ const protocol = url.protocol.replace(':', '');
+
+ if (protocol === "mongodb") {
+ await restoreMongoDB(restoreContentsPath, dbUrl);
+ } else if (["postgresql", "postgres"].includes(protocol)) {
await restorePostgres(restoreContentsPath, dbUrl);
} else {
throw new Error(
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
app/client/packages/rts/src/ctl/backup/backup.test.ts (1)
291-305: Consider adding edge case testsThe test cases cover basic PostgreSQL URL parsing well. Consider adding tests for edge cases:
- URLs with special characters in database names
- URLs with multiple query parameters
- Invalid URLs
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
app/client/packages/rts/src/ctl/backup/backup.test.ts(5 hunks)
🔇 Additional comments (3)
app/client/packages/rts/src/ctl/backup/backup.test.ts (3)
11-11: LGTM!
The import statement for PostgreSQL dump command execution is correctly added.
261-261: LGTM!
The test cases have been consistently updated to use the new getDatabaseNameFromUrl function, maintaining compatibility with both MongoDB and PostgreSQL URLs.
Also applies to: 270-270, 279-279, 287-287
70-79: Remove debug logging and verify schema flag requirement
- Remove the unnecessary
console.logstatement - Verify if the
--schema=appsmithflag is required for all PostgreSQL instances
test("Test postgres dump CMD generation", async () => {
const dest = "/dest";
const url = "postgresql://username:password@host/appsmith";
const cmd =
"pg_dump postgresql://username:password@host/appsmith --schema=appsmith --format=custom --file=/dest/pg-data.gz";
const res = await executePostgresDumpCMD(dest, url);
expect(res).toBe(cmd);
- console.log(res);
});Let's verify the schema requirement:
✅ Verification successful
Let me gather more information about the PostgreSQL schema usage.
Let me check the actual implementation of the PostgreSQL dump command.
Let me check the actual implementation of the PostgreSQL dump command to understand the schema requirement.
Let me try a different pattern to find the implementation.
Remove console.log and keep schema flag
The --schema=appsmith flag is required as the application uses a dedicated 'appsmith' schema for its PostgreSQL database, as evidenced by the database initialization scripts and schema creation checks in pg-utils.sh. Remove the debug logging statement.
test("Test postgres dump CMD generation", async () => {
const dest = "/dest";
const url = "postgresql://username:password@host/appsmith";
const cmd =
"pg_dump postgresql://username:password@host/appsmith --schema=appsmith --format=custom --file=/dest/pg-data.gz";
const res = await executePostgresDumpCMD(dest, url);
expect(res).toBe(cmd);
- console.log(res);
});🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for PostgreSQL schema configurations across the codebase
rg -l "postgresql.*schema.*appsmith"
Length of output: 92
Script:
#!/bin/bash
# Search for PostgreSQL schema-related code and configurations
ast-grep --pattern 'executePostgresDumpCMD($_, $_)'
# Look for any PostgreSQL schema configurations in the codebase
rg "schema.*appsmith" -A 5 -B 5
Length of output: 45654
Script:
#!/bin/bash
# Find the implementation of executePostgresDumpCMD
ast-grep --pattern 'executePostgresDumpCMD'
# Look for any PostgreSQL backup-related configurations
rg "pg_dump.*schema" -A 3 -B 3
Length of output: 1212
Script:
#!/bin/bash
# Get the implementation of executePostgresDumpCMD function
ast-grep --pattern 'export async function executePostgresDumpCMD($$$)'
Length of output: 72
Script:
#!/bin/bash
# Find the implementation file content
cat app/client/packages/rts/src/ctl/backup/links/PostgresDumpLink.ts
Length of output: 864
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
app/client/packages/rts/src/ctl/backup/backup.test.ts (1)
291-305: Consider adding edge case tests for PostgreSQL URL parsingThe current tests cover basic cases well. Consider adding tests for:
- URLs with non-standard ports
- URLs with multiple query parameters
- URLs with special characters in database names
Example test case:
test("Get DB name from Postgres URL with special characters and multiple params", async () => { const dbName = utils.getDatabaseNameFromUrl( "postgresql://user:pass@host:5433/my_db-name.1?sslmode=verify-full&timeout=10" ); expect(dbName).toEqual("my_db-name.1"); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
app/client/packages/rts/src/ctl/backup/backup.test.ts(5 hunks)
🔇 Additional comments (1)
app/client/packages/rts/src/ctl/backup/backup.test.ts (1)
70-79: Remove debug console.log statement
The console.log statement on line 78 should be removed as it's not needed for the test.
expect(res).toBe(cmd);
- console.log(res);
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12092119183. |
|
Deploy-Preview-URL: https://ce-37326.dp.appsmith.com |
sharat87
requested review from
mohanarpit and
AnaghHegde
and removed request for
sharat87
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
|
This PR has been closed because of inactivity. |
github-actions
bot
added
the
Platform Administration Pod
Description
PR to add backup and restore functionality for Postgres.
Added support for
backupandrestorecommands, but not toimport-dbandexport-db. They are discouraged already anyway, and we can add support to them later if it's really needed. Or actually deprecate them and remove them along with MongoDB itself.Fixes #35369
/test Sanity
🔍 Cypress test results
Warning
Tests have not run on the HEAD 552044c yet
Mon, 27 Jan 2025 12:36:15 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Release Notes
New Features
PostgresDumpLinkclass for PostgreSQL export functionality.restorePostgresfunction for restoring PostgreSQL databases.Improvements