OpenWF/SpaceNinjaServer
12
6
Fork 15
Code Issues 45 Pull Requests 6 Actions Packages Projects Releases Activity

fix: abort startup if not connected to MongoDB server #665

Merged
Sainan merged 1 commits from no-mongo-no-service into main 2024-12-30 16:41:47 -08:00
Conversation 3 Commits 1 Files Changed 2 +25 -37
2 changed files with 25 additions and 37 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

16
src/index.ts
View File

@ -7,14 +7,16 @@ import https from "https";
import fs from "node:fs"; import fs from "node:fs";
import { app } from "./app"; import { app } from "./app";
import { config, validateConfig } from "./services/configService"; import { config, validateConfig } from "./services/configService";
import { connectDatabase } from "@/src/services/mongoService";
import { registerLogFileCreationListener } from "@/src/utils/logger"; import { registerLogFileCreationListener } from "@/src/utils/logger";
import mongoose from "mongoose";
registerLogFileCreationListener(); registerLogFileCreationListener();
validateConfig(); validateConfig();
void (async (): Promise<void> => { mongoose
await connectDatabase(); .connect(config.mongodbUrl)
.then(() => {
logger.info("Connected to MongoDB");
const httpPort = config.httpPort || 80; const httpPort = config.httpPort || 80;
const httpsPort = config.httpsPort || 443; const httpsPort = config.httpsPort || 443;
coderabbitai[bot] commented 2024-12-29 19:16:15 -08:00 (Migrated from github.com)
Review
Copy Link

🛠️ Refactor suggestion

Add MongoDB connection options for better reliability.

The current connection setup lacks important options that could improve reliability and error handling. Consider adding connection options to handle timeouts and connection issues gracefully.

Apply this diff:

 mongoose
-    .connect(config.mongodbUrl)
+    .connect(config.mongodbUrl, {
+        connectTimeoutMS: 5000,
+        socketTimeoutMS: 45000,
+        serverSelectionTimeoutMS: 5000,
+        heartbeatFrequencyMS: 10000,
+    })
📝 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.

mongoose
    .connect(config.mongodbUrl, {
        connectTimeoutMS: 5000,
        socketTimeoutMS: 45000,
        serverSelectionTimeoutMS: 5000,
        heartbeatFrequencyMS: 10000,
    })
    .then(() => {
        logger.info("Connected to MongoDB");
_:hammer_and_wrench: Refactor suggestion_ **Add MongoDB connection options for better reliability.** The current connection setup lacks important options that could improve reliability and error handling. Consider adding connection options to handle timeouts and connection issues gracefully. Apply this diff: ```diff mongoose - .connect(config.mongodbUrl) + .connect(config.mongodbUrl, { + connectTimeoutMS: 5000, + socketTimeoutMS: 45000, + serverSelectionTimeoutMS: 5000, + heartbeatFrequencyMS: 10000, + }) ``` <!-- suggestion_start --> <details> <summary>📝 Committable suggestion</summary> > ‼️ **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. `````suggestion mongoose .connect(config.mongodbUrl, { connectTimeoutMS: 5000, socketTimeoutMS: 45000, serverSelectionTimeoutMS: 5000, heartbeatFrequencyMS: 10000, }) .then(() => { logger.info("Connected to MongoDB"); ````` </details> <!-- suggestion_end --> <!-- This is an auto-generated comment by CodeRabbit -->
@ -33,4 +35,10 @@ void (async (): Promise<void> => {
); );
coderabbitai[bot] commented 2024-12-29 19:16:15 -08:00 (Migrated from github.com)
Review
Copy Link

🛠️ Refactor suggestion

Add error handling for certificate loading.

The certificate loading could fail but isn't wrapped in a try-catch block. This could lead to an unhandled exception.

Apply this diff:

-        const options = {
-            key: fs.readFileSync("static/certs/key.pem"),
-            cert: fs.readFileSync("static/certs/cert.pem")
-        };
+        let options;
+        try {
+            options = {
+                key: fs.readFileSync("static/certs/key.pem"),
+                cert: fs.readFileSync("static/certs/cert.pem")
+            };
+        } catch (error) {
+            logger.error("Failed to load SSL certificates");
+            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.

        let options;
        try {
            options = {
                key: fs.readFileSync("static/certs/key.pem"),
                cert: fs.readFileSync("static/certs/cert.pem")
            };
        } catch (error) {
            logger.error("Failed to load SSL certificates");
            process.exit(1);
        }
_:hammer_and_wrench: Refactor suggestion_ **Add error handling for certificate loading.** The certificate loading could fail but isn't wrapped in a try-catch block. This could lead to an unhandled exception. Apply this diff: ```diff - const options = { - key: fs.readFileSync("static/certs/key.pem"), - cert: fs.readFileSync("static/certs/cert.pem") - }; + let options; + try { + options = { + key: fs.readFileSync("static/certs/key.pem"), + cert: fs.readFileSync("static/certs/cert.pem") + }; + } catch (error) { + logger.error("Failed to load SSL certificates"); + process.exit(1); + } ``` <!-- suggestion_start --> <details> <summary>📝 Committable suggestion</summary> > ‼️ **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. `````suggestion let options; try { options = { key: fs.readFileSync("static/certs/key.pem"), cert: fs.readFileSync("static/certs/cert.pem") }; } catch (error) { logger.error("Failed to load SSL certificates"); process.exit(1); } ````` </details> <!-- suggestion_end --> <!-- This is an auto-generated comment by CodeRabbit -->
}); });
}); });
})(); })
.catch(error => {
if (error instanceof Error) {
logger.error(`Error connecting to MongoDB server: ${error.message}`);
}
process.exit(1);
});

20
src/services/mongoService.ts
View File

@ -1,20 +0,0 @@
import { logger } from "@/src/utils/logger";
import { config } from "@/src/services/configService";
import mongoose from "mongoose";
const url = config.mongodbUrl;
if (url === undefined) {
throw new Error("MONGODB_URL not found. Make sure you have a .env file in the root of the project!");
}
export const connectDatabase = async (): Promise<void> => {
try {
await mongoose.connect(`${url}`);
logger.info("Connected to MongoDB");
} catch (error: unknown) {
if (error instanceof Error) {
logger.error(`Error connecting to MongoDB ${error.message}`);
}
}
};