feat: add connection class #180
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes in this pull request involve updates to the Changes
Possibly related PRs
Suggested reviewers
Warning Rate limit exceeded@Behzad-rabiei has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 18 minutes and 8 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (1)
src/service/connetion.ts (1)
21-35: Add timeout handling to disconnect methodSimilar to connect, the disconnect method should handle timeouts and use proper logging.
Here's a suggested improvement:
- public async disconnect(): Promise<void> { + public async disconnect(timeoutMs: number = 5000): Promise<void> { try { if (this.mongoConnection !== null) { - await mongoose.disconnect(); + await Promise.race([ + mongoose.disconnect(), + new Promise((_, reject) => + setTimeout(() => reject(new Error('Disconnect timeout')), timeoutMs) + ), + ]); this.mongoConnection = null; - console.log('Disconnected from MongoDB'); + // TODO: Replace with proper logger + console.log('[MongoDB] Disconnected successfully'); } else { - console.warn('No active MongoDB connection to disconnect.'); + // TODO: Replace with proper logger + console.warn('[MongoDB] No active connection to disconnect'); } } catch (error: unknown) { const errorMessage = error instanceof Error ? error.message : 'Unknown error'; - console.error(`Failed to disconnect from MongoDB: ${errorMessage}`); + // TODO: Replace with proper logger + console.error(`[MongoDB] Disconnect failed: ${errorMessage}`); throw new Error(`Failed to disconnect from MongoDB: ${errorMessage}`); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
package.json(1 hunks)src/service/connetion.ts(1 hunks)src/service/databaseManager.ts(0 hunks)src/service/index.ts(1 hunks)
💤 Files with no reviewable changes (1)
- src/service/databaseManager.ts
✅ Files skipped from review due to trivial changes (2)
- package.json
- src/service/index.ts
src/service/connetion.ts
Outdated
| export default class MongoConnectionManager { | ||
| private mongoConnection: Connection | null = null; | ||
|
|
||
| public async connect(url: string): Promise<void> { | ||
| try { | ||
| if (this.mongoConnection !== null) { | ||
| throw new Error('MongoDB connection already exists'); | ||
| } | ||
| await mongoose.connect(url); | ||
| this.mongoConnection = mongoose.connection; | ||
| console.log('Connected to MongoDB!'); | ||
| } catch (error: unknown) { | ||
| const errorMessage = error instanceof Error ? error.message : 'Unknown error'; | ||
| console.error(`Failed to connect to MongoDB: ${errorMessage}`); | ||
| throw new Error(`Failed to connect to MongoDB: ${errorMessage}`); | ||
| } | ||
| } | ||
|
|
||
| public async disconnect(): Promise<void> { | ||
| try { | ||
| if (this.mongoConnection !== null) { | ||
| await mongoose.disconnect(); | ||
| this.mongoConnection = null; | ||
| console.log('Disconnected from MongoDB'); | ||
| } else { | ||
| console.warn('No active MongoDB connection to disconnect.'); | ||
| } | ||
| } catch (error: unknown) { | ||
| const errorMessage = error instanceof Error ? error.message : 'Unknown error'; | ||
| console.error(`Failed to disconnect from MongoDB: ${errorMessage}`); | ||
| throw new Error(`Failed to disconnect from MongoDB: ${errorMessage}`); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Ensures that there is an active MongoDB connection. | ||
| * Throws an error if no connection is active. | ||
| */ | ||
| public ensureConnected(): void { | ||
| if (this.mongoConnection !== null) { | ||
| throw new Error('No active MongoDB connection. Please connect before performing database operations.'); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Gets the current MongoDB connection. | ||
| * @returns The Mongoose Connection object. | ||
| */ | ||
| public getConnection(): Connection | null { | ||
| this.ensureConnected(); | ||
| return this.mongoConnection; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add connection event handlers
The class should listen to important Mongoose connection events to handle disconnections and errors gracefully.
Add this method and call it in the connect method:
private setupConnectionHandlers(): void {
if (!this.mongoConnection) return;
this.mongoConnection.on('disconnected', () => {
// TODO: Replace with proper logger
console.warn('[MongoDB] Connection lost');
// Implement reconnection logic if needed
});
this.mongoConnection.on('error', (error) => {
// TODO: Replace with proper logger
console.error('[MongoDB] Connection error:', error);
});
}Call it after establishing the connection:
this.mongoConnection = mongoose.connection;
+ this.setupConnectionHandlers();
// TODO: Replace with proper logger| public getConnection(): Connection | null { | ||
| this.ensureConnected(); | ||
| return this.mongoConnection; | ||
| } |
There was a problem hiding this comment.
Fix return type and behavior mismatch in getConnection
The method declares it can return null but after ensureConnected(), it should never return null.
Here's the fix:
- public getConnection(): Connection | null {
+ public getConnection(): Connection {
this.ensureConnected();
- return this.mongoConnection;
+ // After ensureConnected(), this.mongoConnection is guaranteed to be non-null
+ return this.mongoConnection!;
}📝 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.
| public getConnection(): Connection | null { | |
| this.ensureConnected(); | |
| return this.mongoConnection; | |
| } | |
| public getConnection(): Connection { | |
| this.ensureConnected(); | |
| // After ensureConnected(), this.mongoConnection is guaranteed to be non-null | |
| return this.mongoConnection!; | |
| } |
src/service/connetion.ts
Outdated
| public ensureConnected(): void { | ||
| if (this.mongoConnection !== null) { | ||
| throw new Error('No active MongoDB connection. Please connect before performing database operations.'); | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix critical logic error in ensureConnected method
The condition is reversed! It currently throws an error when a connection exists, which is the opposite of what it should do.
Here's the fix:
public ensureConnected(): void {
- if (this.mongoConnection !== null) {
+ if (this.mongoConnection === null) {
throw new Error('No active MongoDB connection. Please connect before performing database operations.');
}
}📝 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.
| public ensureConnected(): void { | |
| if (this.mongoConnection !== null) { | |
| throw new Error('No active MongoDB connection. Please connect before performing database operations.'); | |
| } | |
| } | |
| public ensureConnected(): void { | |
| if (this.mongoConnection === null) { | |
| throw new Error('No active MongoDB connection. Please connect before performing database operations.'); | |
| } | |
| } |
src/service/connetion.ts
Outdated
| import mongoose, { type Connection } from 'mongoose'; | ||
|
|
||
| export default class MongoConnectionManager { | ||
| private mongoConnection: Connection | null = null; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider implementing singleton pattern
Since database connections should be managed as singletons to prevent multiple connections, consider implementing the singleton pattern for this class.
Here's a suggested implementation:
import mongoose, { type Connection } from 'mongoose';
export default class MongoConnectionManager {
+ private static instance: MongoConnectionManager | null = null;
private mongoConnection: Connection | null = null;
+
+ private constructor() {}
+
+ public static getInstance(): MongoConnectionManager {
+ if (!MongoConnectionManager.instance) {
+ MongoConnectionManager.instance = new MongoConnectionManager();
+ }
+ return MongoConnectionManager.instance;
+ }📝 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.
| import mongoose, { type Connection } from 'mongoose'; | |
| export default class MongoConnectionManager { | |
| private mongoConnection: Connection | null = null; | |
| import mongoose, { type Connection } from 'mongoose'; | |
| export default class MongoConnectionManager { | |
| private static instance: MongoConnectionManager | null = null; | |
| private mongoConnection: Connection | null = null; | |
| private constructor() {} | |
| public static getInstance(): MongoConnectionManager { | |
| if (!MongoConnectionManager.instance) { | |
| MongoConnectionManager.instance = new MongoConnectionManager(); | |
| } | |
| return MongoConnectionManager.instance; | |
| } |
src/service/connetion.ts
Outdated
| import mongoose, { type Connection } from 'mongoose'; | ||
|
|
||
| export default class MongoConnectionManager { | ||
| private mongoConnection: Connection | null = null; | ||
|
|
||
| public async connect(url: string): Promise<void> { | ||
| try { | ||
| if (this.mongoConnection !== null) { | ||
| throw new Error('MongoDB connection already exists'); | ||
| } | ||
| await mongoose.connect(url); | ||
| this.mongoConnection = mongoose.connection; | ||
| console.log('Connected to MongoDB!'); | ||
| } catch (error: unknown) { | ||
| const errorMessage = error instanceof Error ? error.message : 'Unknown error'; | ||
| console.error(`Failed to connect to MongoDB: ${errorMessage}`); | ||
| throw new Error(`Failed to connect to MongoDB: ${errorMessage}`); | ||
| } | ||
| } | ||
|
|
||
| public async disconnect(): Promise<void> { | ||
| try { | ||
| if (this.mongoConnection !== null) { | ||
| await mongoose.disconnect(); | ||
| this.mongoConnection = null; | ||
| console.log('Disconnected from MongoDB'); | ||
| } else { | ||
| console.warn('No active MongoDB connection to disconnect.'); | ||
| } | ||
| } catch (error: unknown) { | ||
| const errorMessage = error instanceof Error ? error.message : 'Unknown error'; | ||
| console.error(`Failed to disconnect from MongoDB: ${errorMessage}`); | ||
| throw new Error(`Failed to disconnect from MongoDB: ${errorMessage}`); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Ensures that there is an active MongoDB connection. | ||
| * Throws an error if no connection is active. | ||
| */ | ||
| public ensureConnected(): void { | ||
| if (this.mongoConnection !== null) { | ||
| throw new Error('No active MongoDB connection. Please connect before performing database operations.'); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Gets the current MongoDB connection. | ||
| * @returns The Mongoose Connection object. | ||
| */ | ||
| public getConnection(): Connection | null { | ||
| this.ensureConnected(); | ||
| return this.mongoConnection; | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Fix filename typo and update affected imports
The filename typo "connetion.ts" needs to be corrected to "connection.ts", and the following changes are required:
- Rename
src/service/connetion.tstosrc/service/connection.ts - Update import in
src/service/index.tsfromimport Connetion from './connetion'toimport Connection from './connection' - Update export in
src/service/index.tsfromConnetiontoConnection
🔗 Analysis chain
Fix typo in filename: "connetion.ts" should be "connection.ts"
The filename contains a typo which should be corrected to maintain consistency and prevent confusion.
Let's verify if this typo affects any imports in other files:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for imports referencing this file
rg -l "from ['\"].*connetion['\"]"
Length of output: 61
Script:
#!/bin/bash
# Check the content of the file that imports connetion.ts
cat src/service/index.ts
Length of output: 151
| public async connect(url: string): Promise<void> { | ||
| try { | ||
| if (this.mongoConnection !== null) { | ||
| throw new Error('MongoDB connection already exists'); | ||
| } | ||
| await mongoose.connect(url); | ||
| this.mongoConnection = mongoose.connection; | ||
| console.log('Connected to MongoDB!'); | ||
| } catch (error: unknown) { | ||
| const errorMessage = error instanceof Error ? error.message : 'Unknown error'; | ||
| console.error(`Failed to connect to MongoDB: ${errorMessage}`); | ||
| throw new Error(`Failed to connect to MongoDB: ${errorMessage}`); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance connection handling with options, timeout, and proper logging
The connection method needs improvements for production readiness:
- Missing Mongoose connection options
- No connection timeout
- No retry mechanism
- Direct console.log usage
Here's a suggested implementation:
- public async connect(url: string): Promise<void> {
+ public async connect(
+ url: string,
+ options: mongoose.ConnectOptions = {},
+ timeoutMs: number = 5000
+ ): Promise<void> {
try {
if (this.mongoConnection !== null) {
throw new Error('MongoDB connection already exists');
}
- await mongoose.connect(url);
+ await Promise.race([
+ mongoose.connect(url, {
+ ...options,
+ serverSelectionTimeoutMS: timeoutMs,
+ connectTimeoutMS: timeoutMs,
+ }),
+ new Promise((_, reject) =>
+ setTimeout(() => reject(new Error('Connection timeout')), timeoutMs)
+ ),
+ ]);
this.mongoConnection = mongoose.connection;
- console.log('Connected to MongoDB!');
+ // TODO: Replace with proper logger
+ console.log('[MongoDB] Connected successfully');
} catch (error: unknown) {
const errorMessage = error instanceof Error ? error.message : 'Unknown error';
- console.error(`Failed to connect to MongoDB: ${errorMessage}`);
+ // TODO: Replace with proper logger
+ console.error(`[MongoDB] Connection failed: ${errorMessage}`);
throw new Error(`Failed to connect to MongoDB: ${errorMessage}`);
}
}📝 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.
| public async connect(url: string): Promise<void> { | |
| try { | |
| if (this.mongoConnection !== null) { | |
| throw new Error('MongoDB connection already exists'); | |
| } | |
| await mongoose.connect(url); | |
| this.mongoConnection = mongoose.connection; | |
| console.log('Connected to MongoDB!'); | |
| } catch (error: unknown) { | |
| const errorMessage = error instanceof Error ? error.message : 'Unknown error'; | |
| console.error(`Failed to connect to MongoDB: ${errorMessage}`); | |
| throw new Error(`Failed to connect to MongoDB: ${errorMessage}`); | |
| } | |
| } | |
| public async connect( | |
| url: string, | |
| options: mongoose.ConnectOptions = {}, | |
| timeoutMs: number = 5000 | |
| ): Promise<void> { | |
| try { | |
| if (this.mongoConnection !== null) { | |
| throw new Error('MongoDB connection already exists'); | |
| } | |
| await Promise.race([ | |
| mongoose.connect(url, { | |
| ...options, | |
| serverSelectionTimeoutMS: timeoutMs, | |
| connectTimeoutMS: timeoutMs, | |
| }), | |
| new Promise((_, reject) => | |
| setTimeout(() => reject(new Error('Connection timeout')), timeoutMs) | |
| ), | |
| ]); | |
| this.mongoConnection = mongoose.connection; | |
| // TODO: Replace with proper logger | |
| console.log('[MongoDB] Connected successfully'); | |
| } catch (error: unknown) { | |
| const errorMessage = error instanceof Error ? error.message : 'Unknown error'; | |
| // TODO: Replace with proper logger | |
| console.error(`[MongoDB] Connection failed: ${errorMessage}`); | |
| throw new Error(`Failed to connect to MongoDB: ${errorMessage}`); | |
| } | |
| } |
Summary by CodeRabbit
New Features
MongoConnectionManagerclass for improved MongoDB connection management.Bug Fixes
DatabaseManagerclass, streamlining database operations.Documentation
Connectionentity alongsideDatabaseManager.Chores