feat: add postgres data backup to appsmith ctl #37837
Conversation
WalkthroughThe changes introduce a new file 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (4)
app/client/packages/rts/src/ctl/export_db_pg.ts (2)
24-28: Validate service states before and after operationsAdd checks to ensure services are actually stopped/started as expected.
try { console.log("stop backend & rts application before export database"); await utils.stop(["backend", "rts"]); + await utils.validateServicesStopped(["backend", "rts"]); await exportDatabase(); console.log("start backend & rts application after export database");
30-37: Consider moving the encryption notice to a separate functionThe warning about encryption variables is important but should be extracted for better maintainability.
+ function printEncryptionNotice() { + console.log("\x1b[0;33m++++++++++++++++++++ NOTE ++++++++++++++++++++"); + console.log(); + console.log( + "Please remember to also copy APPSMITH_ENCRYPTION_SALT and APPSMITH_ENCRYPTION_PASSWORD variables from the docker.env file to the target instance where you intend to import this database dump.", + ); + console.log(); + console.log("++++++++++++++++++++++++++++++++++++++++++++++\x1b[0m"); + console.log(); + }app/client/packages/rts/src/ctl/move-to-postgres.mjs (2)
131-134: Use theMINIMUM_MONGO_CHANGESETconstant instead of hardcoding the stringFor better maintainability and to avoid duplication, replace the hardcoded string with the
MINIMUM_MONGO_CHANGESETconstant.Apply this diff to utilize the constant:
async function isMongoDataMigratedToStableVersion(mongoDb) { const doc = await mongoDb.collection(MONGO_MIGRATION_COLLECTION).findOne({ - changeId: "add_empty_policyMap_for_null_entries", + changeId: MINIMUM_MONGO_CHANGESET, state: "EXECUTED", }); return doc !== null; }
60-61: Ensure strict equality checks when comparing tonullUsing strict equality (
===) improves code clarity and avoids unexpected type coercion.Consider updating the condition:
- if (doc.policyMap == null) { + if (doc.policyMap === null) {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
app/client/packages/rts/src/ctl/export_db_pg.ts(1 hunks)app/client/packages/rts/src/ctl/index.ts(2 hunks)app/client/packages/rts/src/ctl/move-to-postgres.mjs(3 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/packages/rts/src/ctl/move-to-postgres.mjs
[error] 111-111: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (4)
app/client/packages/rts/src/ctl/export_db_pg.ts (1)
1-4: Verify the architectural approach for Postgres migration
The file name suggests PostgreSQL functionality, but the implementation appears to be focused on MongoDB export. This might indicate an intermediate step in a larger migration process.
✅ Verification successful
The architectural approach is correct for database migration
The implementation follows a proper migration pattern where MongoDB data is first exported to JSONL format before PostgreSQL import. The file name export_db_pg.ts is appropriate as it's part of the PostgreSQL migration tooling, handling the export phase of the process.
- The code correctly stops the backend and RTS applications before export
- Includes proper error handling and application restart
- Provides important notes about encryption variables for the target instance
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for related PostgreSQL import functionality
rg -l "postgres|pg_" --type ts
Length of output: 1245
Script:
#!/bin/bash
# Check the content of export_db_pg.ts to understand the implementation
cat app/client/packages/rts/src/ctl/export_db_pg.ts
# Check the content of move-to-postgres.js to understand the migration flow
fd -e js -e ts "move-to-postgres" --exec cat {}
Length of output: 1492
app/client/packages/rts/src/ctl/index.ts (2)
7-7: LGTM: Import statement follows existing pattern
The new import follows the established pattern of the file.
38-41: Verify the integration with export_db_pg module
Let's verify that the imported module exists and has the required run method.
✅ Verification successful
Integration with export_db_pg module is properly implemented
The export_db_pg module exists at the correct location and exports an async run() function with proper implementation for database export functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the export_db_pg.ts file exists and contains the run method
fd -e ts -e js "export_db_pg" app/client/packages/rts/src/ctl/ --exec ast-grep --pattern 'export async function run() {
$$$
}'
Length of output: 2430
app/client/packages/rts/src/ctl/move-to-postgres.mjs (1)
52-73: Code logic and structure are solid
The implementation correctly exports non-archived MongoDB documents to JSONL files, efficiently handling field transformations and file operations.
| export async function run() { | ||
| let errorCode = 0; | ||
|
|
||
| await utils.ensureSupervisorIsRunning(); | ||
|
|
||
| try { | ||
| console.log("stop backend & rts application before export database"); | ||
| await utils.stop(["backend", "rts"]); | ||
| await exportDatabase(); | ||
| console.log("start backend & rts application after export database"); | ||
| console.log(); | ||
| console.log("\x1b[0;33m++++++++++++++++++++ NOTE ++++++++++++++++++++"); | ||
| console.log(); | ||
| console.log( | ||
| "Please remember to also copy APPSMITH_ENCRYPTION_SALT and APPSMITH_ENCRYPTION_PASSWORD variables from the docker.env file to the target instance where you intend to import this database dump.", | ||
| ); | ||
| console.log(); | ||
| console.log("++++++++++++++++++++++++++++++++++++++++++++++\x1b[0m"); | ||
| console.log(); | ||
| } catch (err) { | ||
| console.log(err); | ||
| errorCode = 1; | ||
| } finally { | ||
| await utils.start(["backend", "rts"]); | ||
| process.exit(errorCode); | ||
| } | ||
| } |
There was a problem hiding this comment.
Add signal handlers and improve process management
The function should handle interruption signals and ensure proper cleanup.
export async function run() {
let errorCode = 0;
+ const cleanup = async () => {
+ try {
+ await utils.start(["backend", "rts"]);
+ } finally {
+ process.exit(errorCode);
+ }
+ };
+
+ process.on('SIGINT', cleanup);
+ process.on('SIGTERM', cleanup);
await utils.ensureSupervisorIsRunning();Committable suggestion skipped: line range outside the PR's diff.
| } else if (["export_db_pg"].includes(command)) { | ||
| console.log("Exporting data dump for Postgres migration"); | ||
| export_db_pg.run(); | ||
| console.log("Exporting data dump for Postgres migration done"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and command aliases
The implementation needs two improvements:
- Add error handling similar to the import-db command
- Consider adding command aliases for consistency with other commands
Consider this implementation:
-} else if (["export_db_pg"].includes(command)) {
+} else if (["export-db-pg", "export_db_pg", "expg"].includes(command)) {
console.log("Exporting data dump for Postgres migration");
- export_db_pg.run();
- console.log("Exporting data dump for Postgres migration done");
+ try {
+ export_db_pg.run();
+ console.log("Exporting data dump for Postgres migration done");
+ } catch (error) {
+ console.error("Failed to export database for Postgres:", error.message);
+ process.exit(1);
+ }📝 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.
| } else if (["export_db_pg"].includes(command)) { | |
| console.log("Exporting data dump for Postgres migration"); | |
| export_db_pg.run(); | |
| console.log("Exporting data dump for Postgres migration done"); | |
| } else if (["export-db-pg", "export_db_pg", "expg"].includes(command)) { | |
| console.log("Exporting data dump for Postgres migration"); | |
| try { | |
| export_db_pg.run(); | |
| console.log("Exporting data dump for Postgres migration done"); | |
| } catch (error) { | |
| console.error("Failed to export database for Postgres:", error.message); | |
| process.exit(1); | |
| } |
There was a problem hiding this comment.
@AnaghHegde Let's add the try_catch block for error handling.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| } | ||
| delete obj._class; // Remove the _class field | ||
| delete obj._class; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using the delete operator for better performance
Using the delete operator can negatively impact performance. Instead, consider setting the property to undefined.
Apply this diff to replace the delete operation:
} else if (key === "_class") {
const type = mapClassToType(obj._class);
if (type) {
obj.type = type;
}
- delete obj._class;
+ obj._class = undefined;
} else if (typeof obj[key] === "object" && obj[key] !== null) {📝 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.
| delete obj._class; | |
| obj._class = undefined; |
🧰 Tools
🪛 Biome (1.9.4)
[error] 111-111: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
| @@ -34,6 +35,10 @@ if (["export-db", "export_db", "ex"].includes(command)) { | |||
| console.log("Exporting database"); | |||
| export_db.run(); | |||
| console.log("Export database done"); | |||
| } else if (["export_db_pg"].includes(command)) { | |||
There was a problem hiding this comment.
Does it make sense to make the command more explicit e.g export-mongo-to-pg?
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (2)
app/client/packages/rts/src/ctl/move-to-postgres.mjs (2)
Line range hint
2-15: Update JSDoc to match current implementationThe function documentation needs to be updated to reflect the current implementation. The parameters
mongoDumpFileandisBaselineModeare no longer part of the main function signature.Apply this diff to fix the documentation:
/** - * Moves data from MongoDB to JSONL files, with optional baseline mode filtering. + * Exports MongoDB data to JSONL files, with optional baseline mode filtering. * - * This script connects to a MongoDB instance, optionally restores from a dump file, - * and exports data from all collections to JSONL files. In baseline mode, specific - * filters are applied to exclude certain data. + * This script connects to a MongoDB instance and exports data from all collections + * to JSONL files. In baseline mode, specific filters are applied to exclude certain data. * - * @param {string} mongoDbUrl - The URL of the MongoDB. - * @param {string} mongoDumpFile - The path to the MongoDB dump file. - * @param {boolean} isBaselineMode - Flag indicating whether the script is running in baseline mode. - * @returns {Promise<void>} - A promise that resolves when the data migration is complete. + * @param {string} dbUrl - MongoDB connection URL + * @returns {Promise<void>} - A promise that resolves when the export is complete. */
140-146: Improve cleanup processThe cleanup process should handle potential errors and ensure resources are properly released.
Consider wrapping the cleanup in a try-catch block:
- await mongoClient.close(); - mongoServer?.kill(); - console.log("done"); - process.exit(0); + try { + await mongoClient.close(); + if (mongoServer) { + mongoServer.kill(); + await new Promise((resolve) => mongoServer.on('exit', resolve)); + } + console.log("done"); + process.exit(0); + } catch (error) { + console.error("Error during cleanup:", error); + process.exit(1); + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
app/client/packages/rts/src/ctl/index.ts(2 hunks)app/client/packages/rts/src/ctl/move-to-postgres.mjs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/packages/rts/src/ctl/index.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
app/client/packages/rts/src/ctl/move-to-postgres.mjs (1)
12-27: Improve argument parsing robustnessThe current argument parsing has redundant checks and could be simplified.
- for (let i = 2; i < process.argv.length; ++i) { - const arg = process.argv[i]; + const args = process.argv.slice(2); + for (const arg of args) { if (arg.startsWith("--mongodb-url=") && !mongoDbUrl) { mongoDbUrl = extractValueFromArg(arg); } else if (arg.startsWith("--mongodb-dump=") && !mongoDumpFile) {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
app/client/packages/rts/src/ctl/export_db_pg.ts(1 hunks)app/client/packages/rts/src/ctl/move-to-postgres.mjs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/packages/rts/src/ctl/export_db_pg.ts
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/packages/rts/src/ctl/move-to-postgres.mjs
[error] 164-164: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (4)
app/client/packages/rts/src/ctl/move-to-postgres.mjs (4)
164-164: Replace delete operator with undefined assignment
Using the delete operator can impact performance.
- delete obj._class;
+ obj._class = undefined;🧰 Tools
🪛 Biome (1.9.4)
[error] 164-164: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
8-8: 🛠️ Refactor suggestion
Make export directory configurable
The hardcoded export path could cause issues in different environments.
- const EXPORT_ROOT = "/appsmith-stacks/mongo-data";
+ const EXPORT_ROOT = process.env.EXPORT_ROOT || "/appsmith-stacks/mongo-data";Likely invalid or redundant comment.
190-205: 🛠️ Refactor suggestion
Remove redundant command-line parsing
The command-line parsing at the end of the file is redundant as it's already handled in the main function.
-// Parse command line arguments
-const args = process.argv.slice(2);
-let mongoUrl;
-
-for (const arg of args) {
- if (arg.startsWith('--mongodb-url=')) {
- mongoUrl = arg.split('=')[1];
- }
-}
-
-if (!mongoUrl) {
- console.error('Usage: node move-to-postgres.mjs --mongodb-url=<url>');
- process.exit(1);
-}
-
-writeDataFromMongoToJsonlFiles(mongoUrl).catch(console.error);
+// Execute if this is the main module
+if (require.main === module) {
+ writeDataFromMongoToJsonlFiles().catch(console.error);
+}Likely invalid or redundant comment.
38-54:
Add error handling and readiness check for MongoDB server
The MongoDB server spawn lacks error handling and readiness check.
mongoServer = spawn(
"mongod",
["--bind_ip_all", "--dbpath", "/tmp/db-tmp", "--port", "27500"],
{
stdio: "inherit",
},
);
+
+ mongoServer.on('error', (err) => {
+ console.error('Failed to start MongoDB server:', err);
+ process.exit(1);
+ });
+
+ // Wait for MongoDB to start
+ await new Promise((resolve) => {
+ mongoServer.stdout.on('data', (data) => {
+ if (data.toString().includes('Waiting for connections')) {
+ resolve();
+ }
+ });
+ });Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
app/client/packages/rts/src/ctl/move-to-postgres.mjs (2)
5-7: Consider moving constants to a configuration fileThese constants should be configurable through environment variables or a config file for better maintainability and flexibility across different environments.
-const EXPORT_ROOT = "/appsmith-stacks/mongo-data"; -const MINIMUM_MONGO_CHANGESET = "add_empty_policyMap_for_null_entries"; -const MONGO_MIGRATION_COLLECTION = "mongockChangeLog"; +const config = { + EXPORT_ROOT: process.env.EXPORT_ROOT || "/appsmith-stacks/mongo-data", + MINIMUM_MONGO_CHANGESET: process.env.MINIMUM_MONGO_CHANGESET || "add_empty_policyMap_for_null_entries", + MONGO_MIGRATION_COLLECTION: process.env.MONGO_MIGRATION_COLLECTION || "mongockChangeLog" +};
24-39: Consider using a command-line argument parsing libraryManual argument parsing is error-prone and lacks features like help text and validation. Consider using
yargsorcommander.Example with yargs:
import yargs from 'yargs'; import { hideBin } from 'yargs/helpers'; const argv = yargs(hideBin(process.argv)) .option('mongodb-url', { type: 'string', description: 'MongoDB connection URL' }) .option('baseline', { type: 'boolean', description: 'Run in baseline mode' }) .demandOption(['mongodb-url']) .help() .argv;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
app/client/packages/rts/src/ctl/export_db_pg.ts(1 hunks)app/client/packages/rts/src/ctl/move-to-postgres.mjs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/packages/rts/src/ctl/export_db_pg.ts
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/packages/rts/src/ctl/move-to-postgres.mjs
[error] 208-208: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (4)
app/client/packages/rts/src/ctl/move-to-postgres.mjs (4)
208-208: Replace delete operator with undefined assignment
Using the delete operator can impact performance.
- delete obj._class;
+ obj._class = undefined;🧰 Tools
🪛 Biome (1.9.4)
[error] 208-208: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
244-257: 🛠️ Refactor suggestion
Remove duplicate argument processing logic
This argument processing logic duplicates the functionality in the main function. Consider consolidating into a single implementation.
-// Parse command line arguments
-const args = process.argv.slice(2);
-let mongoUrl;
-
-for (const arg of args) {
- if (arg.startsWith('--mongodb-url=')) {
- mongoUrl = arg.split('=')[1];
- }
-}
-
-if (!mongoUrl) {
- console.error('Usage: node move-to-postgres.mjs --mongodb-url=<url>');
- process.exit(1);
-}
-
-writeDataFromMongoToJsonlFiles(mongoUrl).catch(console.error);
+writeDataFromMongoToJsonlFiles().catch(console.error);Likely invalid or redundant comment.
50-65:
Add error handling for MongoDB server operations
The MongoDB server spawn and restore operations lack proper error handling and health checks.
+ let isServerReady = false;
+ mongoServer.on('error', (error) => {
+ console.error('Failed to start MongoDB server:', error);
+ process.exit(1);
+ });
+
+ mongoServer.on('exit', (code) => {
+ if (!isServerReady) {
+ console.error(`MongoDB server exited with code ${code}`);
+ process.exit(1);
+ }
+ });
+
+ // Wait for MongoDB to start
+ await new Promise((resolve) => {
+ setTimeout(() => {
+ isServerReady = true;
+ resolve();
+ }, 1000);
+ });Likely invalid or redundant comment.
106-132: 🛠️ Refactor suggestion
Optimize data export with batch processing and proper error handling
The current implementation may have issues with large datasets and resource management.
for await (const collectionName of sortedCollectionNames) {
console.log("Collection:", collectionName);
if (isBaselineMode && collectionName.startsWith("mongock")) {
continue;
}
let outFile = null;
+ try {
+ const batchSize = 1000;
+ let batch = [];
outFile = fs.openSync(EXPORT_ROOT + "/" + collectionName + ".jsonl", "w");
for await (const doc of mongoDb
.collection(collectionName)
- .find(filters[collectionName])) {
+ .find(filters[collectionName])
+ .batchSize(batchSize)) {
if (isArchivedObject(doc)) {
continue;
}
transformFields(doc);
if (doc.policyMap == null) {
doc.policyMap = {};
}
- fs.writeSync(outFile, toJsonSortedKeys(doc) + "\n");
+ batch.push(doc);
+ if (batch.length >= batchSize) {
+ fs.writeSync(outFile, batch.map(toJsonSortedKeys).join('\n') + '\n');
+ batch = [];
+ }
}
- if (outFile != null) {
- fs.closeSync(outFile);
+ if (batch.length > 0) {
+ fs.writeSync(outFile, batch.map(toJsonSortedKeys).join('\n') + '\n');
}
+ } catch (error) {
+ console.error(`Error processing collection ${collectionName}:`, error);
+ } finally {
+ if (outFile != null) {
+ fs.closeSync(outFile);
+ }
+ }
}Likely invalid or redundant comment.
There was a problem hiding this comment.
@AnaghHegde have we tested the https://github.com/appsmithorg/appsmith/blob/release/scripts/regen-baseline.sh with these changes?
|
@abhvsn Why do you think baseline regen script will be affected? We are adding the command for backup of mongo data to stacks directory. |
|
@AnaghHegde that's because you have modified the move-to-postgres.mjs file to call |
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
Description
Automation
/ok-to-test tags="@tag.Sanity"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12134415999
Commit: 7684d1e
Cypress dashboard.
Tags:
@tag.SanitySpec:
Tue, 03 Dec 2024 07:33:19 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes