[Device Registry] Implement CQRS pattern with dual database connections #4513
Conversation
📝 WalkthroughWalkthroughThis pull request implements a Command Query Responsibility Segregation (CQRS) pattern in the device registry module. The database connection logic now distinguishes between command and query operations by creating separate connections, functions, and tenant-specific retrieval mechanisms. Additionally, environment configurations across development, staging, and production have been updated to support distinct command and query database URIs, with enhanced error handling and logging. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant DB as Database Module
participant CMD as Command Connection
participant QRY as Query Connection
App->>DB: connectToMongoDB()
DB->>CMD: createCommandConnection(COMMAND_URI)
DB->>QRY: createQueryConnection(QUERY_URI)
CMD-->>DB: Connection status (open/error/disconnection)
QRY-->>DB: Connection status (open/error/disconnection)
DB-->>App: Returns commandDB & queryDB
sequenceDiagram
participant App as Application
participant DB as Database Module
App->>DB: getTenantDB(tenantId, modelName, schema, operationType)
alt operationType = "command"
DB->>DB: getCommandTenantDB(tenantId, modelName, schema)
else operationType = "query"
DB->>DB: getQueryTenantDB(tenantId, modelName, schema)
end
DB-->>App: Returns tenant-specific DB instance
Possibly related PRs
Poem
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
✨ Finishing Touches
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 (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## staging #4513 +/- ##
===========================================
- Coverage 11.59% 11.57% -0.02%
===========================================
Files 163 159 -4
Lines 18766 18733 -33
Branches 547 547
===========================================
- Hits 2175 2169 -6
+ Misses 16587 16560 -27
Partials 4 4 |
|
Auth-service changes in this PR available for preview here |
|
Device registry changes in this PR available for preview here |
![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
🧹 Nitpick comments (15)
src/auth-service/utils/common/generate-filter.js (1)
99-123: Consider validating the ObjectId before assigning to the filter.
While the addition oftenantSettingscorrectly merges parameters fromreq.queryandreq.params, passing potentially invalid IDs directly toObjectId()may lead to unhandled exceptions if the ID is malformed. Adding a brief check or try/catch foridwould help guarantee safer validations.src/auth-service/routes/v2/tenant-settings.routes.js (1)
51-57: Reusing the same controller method for GET /:id may cause confusion.
Although it’s possible to handle all GET requests inlist, separating single-item retrieval logic into its own controller method often improves maintainability and clarity.src/auth-service/models/TenantSettings.js (3)
21-65: The register static method is comprehensive but might blend concerns.
While the method correctly distinguishes between successful and unsuccessful creations, consider separating the duplicate-check from the creation logic for cleaner error handling and potential reusability.
105-143: modify method is straightforward but lacks concurrency safeguards.
Concurrent updates may overwrite data. If concurrency is expected, consider using a version key or optimistic locking to ensure consistent record updates.
144-185: remove method handles edge cases well but could return more details.
The method returns minimal information about the removed document. Including additional metadata (timestamps, user context, etc.) can be beneficial for audit logs or security.src/auth-service/models/User.js (1)
477-530: Remove large blocks of commented-out legacy code
Retaining commented-out code bloats the file and can cause confusion. Consider removing it altogether since version control can track old approaches.- // Old constants-based code block ... - // ...src/auth-service/controllers/tenant-settings.controller.js (1)
1-231: Consider refactoring repetitive request-handling logic
Each method (create/list/update/delete) mostly repeats the same pattern of error extraction, tenant fallback, and response formatting. You could abstract these into helper functions for clearer, succinct code while maintaining consistent responses.src/auth-service/validators/tenant-settings.validators.js (4)
7-42: Consolidate tenant validation logic
UsingoneOf()for a single array of validations is valid but somewhat redundant. Sincequery("tenant").optional()ensures the field is either absent or among the specified values, you could simplify this logic while still restricting tenant values. This is a style preference, but a singlequery("tenant")chain might suffice.
44-46: Modularize repeated validations for listing endpoints
BothlistandlistInformationshare the samevalidateTenantlogic. This is acceptable, but if new listing endpoints appear, consider grouping them into a single object for better scalability.
48-105: Avoid repetitive patterns in create validations
Validations fordefaultNetwork,defaultNetworkRole,defaultGroup, anddefaultGroupRoleshare mostly identical rules. You could introduce a helper function (e.g.,validateObjectIdField(fieldName, optional = false)) to remove duplication and centralize ObjectId logic.+function validateObjectIdField(fieldName, isOptional = false) { + const chain = body(fieldName); + return isOptional ? chain.optional() : chain.exists().withMessage(`${fieldName} is missing`); + // chain trim, isMongoId, sanitizer... +}
107-146: Mirror DRY principle in update validations
Similar to the 'create' validations,updateuses repeating patterns for each field. A single helper function for validating optional or required ObjectId fields could reduce verbosity and maintenance costs.src/device-registry/config/database.js (2)
47-62: Event handlers are well-structured
Glancing atsetupConnectionHandlers, everything is straightforward. Re-enable the commented-outlogger.infolines if you ever need detailed debugging logs.
90-117: Tenant-based separation
DefininggetCommandTenantDBandgetQueryTenantDBfor different DBs by tenant is aligned with multi-tenant best practices. Confirm each tenant’s DB actually exists in the cluster or is created dynamically.src/auth-service/config/database.js (2)
8-8: Utilize logger for environment checks
Your logger initialization is straightforward. Consider adding a log statement to confirm ifCOMMAND_URIorQUERY_URIis missing to preempt potential connection errors.
50-62: Connection handlers
Similar to the other config, your event handlers are minimal but effective. Consider logging successful connections in production if you need to diagnose issues quickly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/auth-service/config/database.js(3 hunks)src/auth-service/controllers/tenant-settings.controller.js(1 hunks)src/auth-service/models/TenantSettings.js(1 hunks)src/auth-service/models/User.js(2 hunks)src/auth-service/routes/v2/index.js(1 hunks)src/auth-service/routes/v2/tenant-settings.routes.js(1 hunks)src/auth-service/utils/common/generate-filter.js(1 hunks)src/auth-service/utils/tenant-settings.util.js(1 hunks)src/auth-service/validators/tenant-settings.validators.js(1 hunks)src/device-registry/config/database.js(2 hunks)
🧰 Additional context used
🪛 GitHub Actions: code-coverage-workflow
src/auth-service/config/database.js
[error] 3-3: MongooseError: The uri parameter to openUri() must be a string, got 'undefined'. Make sure the first parameter to mongoose.connect() or mongoose.createConnection() is a string.
src/device-registry/config/database.js
[error] 4-4: MongooseError: The uri parameter to openUri() must be a string, got 'undefined'. Make sure the first parameter to mongoose.connect() or mongoose.createConnection() is a string.
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: build-push-deploy-device-registry
- GitHub Check: build-push-deploy-auth-service
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (23)
src/auth-service/routes/v2/index.js (1)
30-30: Looks good adding the tenant-settings route.
This addition is straightforward and consistent with existing route definitions, ensuring that the new endpoints can be accessed under/tenant-settings.src/auth-service/routes/v2/tenant-settings.routes.js (5)
1-16: CORS headers and middleware look properly configured.
The headers function ensures broad accessibility, and the usage oftenantSettingsValidations.paginationis well-placed to handle pagination for subsequent routes.
19-25: GET / route is well-structured.
The route’s validation chain (list → setJWTAuth → authJWT → controller.list) seems orderly. It ensures authorized and validated requests, aligning with best practices for secure endpoints.
27-33: POST / route handles creation flow properly.
Using the create validation before authorization helps fail fast on invalid input, enhancing readability and maintainability.
35-41: PUT /:id route updates tenant settings effectively.
The route addresses updates with a well-organized chain of validations and authentication steps, mirroring the patterns in the codebase.
43-49: DELETE /:id route is consistent with the existing CRUD design.
The route’s usage oftenantSettingsController.deleteis properly sequenced after relevant validations and authentication.src/auth-service/models/TenantSettings.js (2)
13-19: Schema fields appear suitable for referencing a multi-tenant model.
Each reference (e.g., defaultNetwork, defaultGroup) is strongly typed to Mongoose ObjectIds, promoting data consistency. Ensure that referenced collections (Network, Role, Group) exist in each tenant’s database to prevent runtime resolution issues.
188-205: Mongoose model retrieval logic is pragmatic and consistent with multi-tenant usage.
UtilizinggetModelByTenantensures each tenant is managed by a unique Mongoose connection. This approach effectively avoids naming collisions.src/auth-service/models/User.js (4)
20-20: Use of the new TenantSettingsModel import looks appropriate
This addition cleanly aligns with the new tenant-specific configuration approach.
438-452: Ensure robust error-handling for tenant settings retrieval
Fetching and verifying tenant settings is handled here. The logic appears correct, but confirm that DB errors and network issues are adequately caught under broader error handling.
457-461: Dynamic defaults for network roles
ReferencingtenantSettings.defaultNetworkensures flexibility across tenants. This is a welcome improvement over hard-coded constants.
469-473: Dynamic defaults for group roles
Likewise, usingtenantSettings.defaultGroupkeeps group roles tenant-specific. This is consistent with the CQRS multi-tenant vision.src/auth-service/utils/tenant-settings.util.js (1)
1-112: Verify safety of calling.toLowerCase()on tenant
Although the code handles a default tenant, iftenantis ever missing or not a string,.toLowerCase()may throw. Consider validating or coercingtenantto a string before usage.src/auth-service/validators/tenant-settings.validators.js (3)
1-6: Use consistent naming in imports & constants
The overall imports and constant references look consistent. However, consider verifying that the@config/constantsfile has up-to-date tenant definitions and that all environment-based constants align with the new validators.
148-148: Explicitly handle “deleteTenantSetting” validations
CombiningvalidateTenantandvalidateIdParamis effective. Ensure that you also handle any additional rules for deletions (e.g., preventing deletion of default system settings) as needed in the controller or service layer.
150-157: Export is consistent and clear
Exporting both single and composite validators in an organized manner is helpful. This is a well-defined approach for scaling future validations.src/device-registry/config/database.js (4)
30-46: Separate connections for CQRS
Creating distinct connections for command/read aligns well with CQRS. Keep an eye on additional resource usage and confirm thatpoolSizeremains optimal for each connection.
66-86: Graceful error handling on connection setup
You correctly wrap connection logic in atry/catch. Verifying environment variables ensures stability. Also confirm that your global error handlers do not swallow other system exceptions.
119-160: Consistent naming for command/query model retrieval
getCommandModelByTenantandgetQueryModelByTenantnicely separate read and write concerns. Make sure to document these methods in your dev guide so future maintainers can quickly pick up the usage pattern.
162-170: Well-organized module exports
Exporting all relevant getters in one object is beneficial for discoverability. Good job keeping backward compatibility forgetTenantDBandgetModelByTenant.src/auth-service/config/database.js (3)
33-49: Establish separate command/query connections
This approach parallels the device-registry config. Verify thedbNameis correct in your environment variables so the system differentiates read/write DBs properly.
66-87: Wrap strategies in robust error handling
The try/catch helps ensure that the application logs the root cause of a failed connection. IfcommandDBorqueryDBisnull, throw a descriptive error to facilitate debugging.
119-160: Clear function naming and backward compatibility
getCommandModelByTenant/getQueryModelByTenantplus optionaloperationTypeis a clean approach to preserve old code usage. Good job ensuring minimal refactoring costs.
| async list({ skip = 0, limit = 100, filter = {} } = {}, next) { | ||
| try { | ||
| const totalCount = await this.countDocuments(filter).exec(); | ||
| const data = await this.find(filter) | ||
| .sort({ createdAt: -1 }) | ||
| .skip(skip ? parseInt(skip) : 0) | ||
| .limit(limit ? parseInt(limit) : parseInt(constants.DEFAULT_LIMIT)) | ||
| .exec(); | ||
| if (!isEmpty(data)) { | ||
| return { | ||
| success: true, | ||
| message: "successfully retrieved the tenant settings", | ||
| data, | ||
| totalCount, | ||
| status: httpStatus.OK, | ||
| }; | ||
| } else if (isEmpty(data)) { | ||
| return { | ||
| success: true, | ||
| message: "no tenant settings exist", | ||
| data: [], | ||
| totalCount, | ||
| status: httpStatus.OK, | ||
| }; | ||
| } | ||
| } catch (error) { | ||
| logger.error(`🐛🐛 Internal Server Error -- ${error.message}`); | ||
| if (error instanceof HttpError) { | ||
| return next(error); | ||
| } | ||
| return next( | ||
| new HttpError( | ||
| "Internal Server Error", | ||
| httpStatus.INTERNAL_SERVER_ERROR, | ||
| { message: error.message } | ||
| ) | ||
| ); | ||
| } | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
list method provides useful pagination, but large skip/limit usage may impact performance.
Consider implementing a more scalable pagination approach (e.g., using an index-based or cursor-based technique) for collections with large data sets to avoid resource-intensive operations.
| // Database URIs for CQRS pattern | ||
| const COMMAND_URI = constants.COMMAND_MONGO_URI || constants.MONGO_URI; | ||
| const QUERY_URI = constants.QUERY_MONGO_URI || constants.MONGO_URI; |
There was a problem hiding this comment.
Ensure environment variables are properly set
The pipeline error indicates that COMMAND_MONGO_URI or QUERY_MONGO_URI may be undefined. Double-check that the environment or constants file sets COMMAND_MONGO_URI and QUERY_MONGO_URI.
src/auth-service/config/database.js
Outdated
| const COMMAND_URI = constants.COMMAND_MONGO_URI || constants.MONGO_URI; | ||
| const QUERY_URI = constants.QUERY_MONGO_URI || constants.MONGO_URI; |
There was a problem hiding this comment.
Validate environment variables
Like the device-registry config, ensure COMMAND_MONGO_URI and QUERY_MONGO_URI are well-defined. The pipeline logs warn about undefined URIs.
src/auth-service/config/database.js
Outdated
| // Initialize connections | ||
| const { commandDB: commandMongoDB, queryDB: queryMongoDB } = connectToMongoDB(); | ||
|
|
||
| /** | ||
| * Get a tenant-specific database from command DB (for write operations) | ||
| */ | ||
| function getCommandTenantDB(tenantId, modelName, schema) { | ||
| const dbName = `${constants.DB_NAME}_command_${tenantId}`; | ||
| if (commandMongoDB) { | ||
| const db = commandMongoDB.useDb(dbName, { useCache: true }); | ||
| db.model(modelName, schema); | ||
| return db; | ||
| } | ||
| throw new Error("Command database connection not established"); | ||
| } | ||
|
|
||
| /**** | ||
| * creating a new mongoDB connection by switching tenant | ||
| * using this to create a new connection based on tenant ID | ||
| /** | ||
| * Get a tenant-specific database from query DB (for read operations) | ||
| */ | ||
| function getTenantDB(tenantId, modelName, schema) { | ||
| const dbName = `${constants.DB_NAME}_${tenantId}`; | ||
| if (mongodb) { | ||
| const db = mongodb.useDb(dbName, { useCache: true }); | ||
| function getQueryTenantDB(tenantId, modelName, schema) { | ||
| const dbName = `${constants.DB_NAME}_query_${tenantId}`; | ||
| if (queryMongoDB) { | ||
| const db = queryMongoDB.useDb(dbName, { useCache: true }); | ||
| db.model(modelName, schema); | ||
| return db; | ||
| } | ||
| throw new Error("Query database connection not established"); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Segregating tenant DB for command/query
Please confirm that each tenant’s read and write DB usage is consistent across all new controllers and utilities. This ensures data flows remain correct under CQRS.
|
Device registry changes in this PR available for preview here |
…trings are properly handled
|
Device registry changes in this PR available for preview here |
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
src/device-registry/config/environments/staging.js (1)
8-43: 💡 Verification agent🧩 Analysis chain
Verify environment variable setup in deployment pipelines
While the configuration looks correct, it's important to ensure that these new environment variables (COMMAND_MONGO_URI_STAGE and QUERY_MONGO_URI_STAGE) are properly set up in your deployment pipelines for the staging environment.
🏁 Script executed:
#!/bin/bash # Check if the new environment variables have values in current environment # Note: This will only work if the environment has these variables set echo "Checking if new environment variables are set:" if [ -z "$COMMAND_MONGO_URI_STAGE" ]; then echo "Warning: COMMAND_MONGO_URI_STAGE is not set" else echo "COMMAND_MONGO_URI_STAGE is set" fi if [ -z "$QUERY_MONGO_URI_STAGE" ]; then echo "Warning: QUERY_MONGO_URI_STAGE is not set" else echo "QUERY_MONGO_URI_STAGE is set" fi # Look for references to these new environment variables in deployment scripts echo "Looking for references to new environment variables in CI/CD scripts:" fd -e yml -e yaml -e json | xargs grep -l "COMMAND_MONGO_URI_STAGE\|QUERY_MONGO_URI_STAGE" || echo "No references found in CI/CD files"Length of output: 645
Action Required: Update Staging Pipeline to Include New Environment Variables
Our review verification indicates that the new environment variables—COMMAND_MONGO_URI_STAGEandQUERY_MONGO_URI_STAGE—are currently not set in the deployment environment and aren’t referenced in any CI/CD configuration files. Although the code insrc/device-registry/config/environments/staging.jsappears syntactically correct, the missing environment values could lead to runtime issues in staging.
- Ensure these variables are provisioned in your staging deployment pipeline.
- Update any relevant CI/CD configuration files to reference these keys.
- Verify that dependent services and scripts can handle or fallback when these values are provided.
🧰 Tools
🪛 Biome (1.9.4)
[error] 31-31: This property value named BIGQUERY_DEVICE_UPTIME_TABLE is later overwritten by an object member with the same name.
Overwritten with this value.
If an object property with the same name is defined multiple times (except when combining a getter with a setter), only the last definition makes it into the object and previous definitions are ignored.
Unsafe fix: Remove this property value named BIGQUERY_DEVICE_UPTIME_TABLE(lint/suspicious/noDuplicateObjectKeys)
🧹 Nitpick comments (5)
src/device-registry/config/environments/development.js (1)
8-42: Consider documentation for local development setupWith the introduction of these new database connection URIs, developers setting up local environments will need guidance on how to properly configure these new variables. Consider adding a note to the relevant developer documentation explaining the CQRS pattern and how to set up these connections locally.
🧰 Tools
🪛 Biome (1.9.4)
[error] 30-30: This property value named BIGQUERY_DEVICE_UPTIME_TABLE is later overwritten by an object member with the same name.
Overwritten with this value.
If an object property with the same name is defined multiple times (except when combining a getter with a setter), only the last definition makes it into the object and previous definitions are ignored.
Unsafe fix: Remove this property value named BIGQUERY_DEVICE_UPTIME_TABLE(lint/suspicious/noDuplicateObjectKeys)
src/device-registry/.env.example (1)
1-126: Consider adding documentation on migration strategyThe PR objectives mention a migration strategy that involves "initially configuring both command and query URIs to point to the same database server." It would be helpful to include a comment in the .env.example file that explains this strategy to guide developers during the transition period.
You could add a comment above these new variables explaining the migration approach, for example:
9 MONGO_URI_PROD= 10 MONGO_URI_STAGE= 11 MONGO_URI_DEV= +# CQRS Database Configuration - During initial migration, configure command and query URIs +# to point to the same database. They can be separated to different servers as needed later. 12 COMMAND_MONGO_URI_PROD= 13 COMMAND_MONGO_URI_STAGE= 14 COMMAND_MONGO_URI_DEV= 15 QUERY_MONGO_URI_PROD= 16 QUERY_MONGO_URI_STAGE= 17 QUERY_MONGO_URI_DEV=src/device-registry/config/database.js (3)
30-41: Database naming convention needs improvement.While the command connection correctly uses a specific naming convention (
${constants.DB_NAME}_command), the query connection just uses the base name without a suffix. For consistency and clarity, consider using a suffix for the query database as well.const createQueryConnection = () => mongoose.createConnection(QUERY_URI, { ...options, - dbName: `${constants.DB_NAME}`, + dbName: `${constants.DB_NAME}_query`, });
49-51: Commented log statement for successful connections.You have a commented-out logger statement for successful database connections. Either uncomment it to provide visibility into successful connections or remove it entirely.
db.on("open", () => { - // logger.info(`Connected to ${dbType} database successfully`); + logger.info(`Connected to ${dbType} database successfully`); });
90-91: Database connections are initialized at module load time.The database connections are established immediately when the module is loaded. While this ensures connections are available, it may complicate testing and could cause issues if the module is imported but not used. Consider implementing a lazy connection approach or making this more explicit.
A more flexible approach would be to export an initialization function:
-// Initialize both database connections -const { commandDB: commandMongoDB, queryDB: queryMongoDB } = connectToMongoDB(); +// Database connection variables +let commandMongoDB = null; +let queryMongoDB = null; + +/** + * Initialize database connections if they haven't been established + */ +function initializeDatabaseConnections() { + if (!commandMongoDB || !queryMongoDB) { + const connections = connectToMongoDB(); + commandMongoDB = connections.commandDB; + queryMongoDB = connections.queryDB; + } + return { commandMongoDB, queryMongoDB }; +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/device-registry/.env.example(1 hunks)src/device-registry/config/database.js(2 hunks)src/device-registry/config/environments/development.js(1 hunks)src/device-registry/config/environments/production.js(1 hunks)src/device-registry/config/environments/staging.js(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: code-coverage-workflow
src/device-registry/config/database.js
[error] 10-10: TypeError: logger.error is not a function
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build-push-deploy-device-registry
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (11)
src/device-registry/config/environments/staging.js (1)
11-12: Implementation of CQRS database connections looks goodThe addition of separate COMMAND_MONGO_URI and QUERY_MONGO_URI variables for the staging environment aligns perfectly with the Command Query Responsibility Segregation pattern described in the PR objectives. This clean separation will allow for independent scaling of read and write operations.
src/device-registry/config/environments/development.js (1)
11-12: Good implementation of CQRS pattern for development environmentThe addition of separate COMMAND_MONGO_URI and QUERY_MONGO_URI variables for the development environment is consistent with the pattern applied to other environments. This maintains a coherent approach across all deployment contexts.
src/device-registry/config/environments/production.js (2)
11-12: Production CQRS implementation looks goodThe addition of COMMAND_MONGO_URI and QUERY_MONGO_URI variables to the production environment completes the consistent implementation across all environments. This ensures that the CQRS pattern is fully supported in production.
8-42:❓ Verification inconclusive
Verify database connection resilience in production
For production environments, it's especially important to ensure that there's proper fallback handling if one of the new database connections fails. The PR objectives mention "enhanced error handling," but I'd recommend verifying how connection failures are managed.
🏁 Script executed:
#!/bin/bash # Look for error handling related to database connections echo "Checking database connection error handling:" # Search for error handling in database configuration rg -A 10 -B 3 "catch|error|exception" --glob "**/*.js" | grep -i "mongo\|database\|connection" # Look for any retry logic rg -A 5 "retry|reconnect" --glob "**/*.js" | grep -i "mongo\|database\|connection"Length of output: 73932
Action Required: Confirm Robust Fallback Handling for Production DB Connections
It appears that error handling for database connections is implemented across several modules (e.g., in
src/device-registry/config/database.jswhere error events trigger logging and process termination). However, the production configuration itself (insrc/device-registry/config/environments/production.js) doesn’t include explicit fallback or reconnection logic for a failed database connection. Please verify that the current approach—relying on error handlers that log issues and exit—is acceptable for production resilience, or if additional fallback mechanisms (like automated retries or seamless failovers) should be implemented. Consider coordinating with your process manager or orchestration system to ensure graceful recovery if a connection fails.
- Review the error handling and process termination logic in
src/device-registry/config/database.js.- Confirm that terminating the process on errors is the desired behavior in production, or update the logic to include a fallback mechanism.
🧰 Tools
🪛 Biome (1.9.4)
[error] 30-30: This property value named BIGQUERY_DEVICE_UPTIME_TABLE is later overwritten by an object member with the same name.
Overwritten with this value.
If an object property with the same name is defined multiple times (except when combining a getter with a setter), only the last definition makes it into the object and previous definitions are ignored.
Unsafe fix: Remove this property value named BIGQUERY_DEVICE_UPTIME_TABLE(lint/suspicious/noDuplicateObjectKeys)
src/device-registry/.env.example (2)
12-17: Environment variables for CQRS added properlyThe addition of the six new environment variables (COMMAND_MONGO_URI and QUERY_MONGO_URI for each environment) is well-structured and placed logically near the existing MongoDB URI variables. This provides a clear template for developers to follow when setting up their environments.
12-17:❓ Verification inconclusive
Verify other dependent services are aware of these changes
For a complete implementation of CQRS, it's important to verify that all services that interact with the device registry are aware of these changes, especially if they directly access the database.
🏁 Script executed:
#!/bin/bash # Look for imports of the configuration that might need updating echo "Looking for imports of environment configs that might need updating:" rg -l "require.*environments\/(development|production|staging)" --glob "**/*.js" | grep -v "environments" # Check for direct references to MONGO_URI that might need to be updated echo "Checking for code that might need updating to use COMMAND_MONGO_URI or QUERY_MONGO_URI:" rg "MONGO_URI(_PROD|_STAGE|_DEV)?" --glob "**/*.js" | grep -v "environments\/"Length of output: 2964
Subject: Verify CQRS Config Propagation Across Dependent Services
The device-registry (and even auth-service) now properly fall back to the new COMMAND_MONGO_URI/QUERY_MONGO_URI values. However, our grep results reveal that several other services (e.g., blog-content-manager, data-mgt, incentives, and others) continue to reference the generic MONGO_URI directly. Please confirm that these services are intended to remain on the legacy variable or that they need to be updated for full CQRS consistency. Also, note that the regex used to check environment config imports returned a parse error—this might need revisiting to avoid false negatives in your verification tests.
src/device-registry/config/database.js (5)
12-14: Good fallback pattern for database URIs.The use of fallback patterns (
||) for the URIs ensures backward compatibility, allowing for a smooth migration strategy as mentioned in the PR objectives. This approach enables initially configuring both URIs to point to the same database server, with the flexibility to separate them later.
74-77: Good error handling for unhandled promises.Setting up global error handlers for unhandled rejections and exceptions is a best practice, especially in a database connection module where asynchronous operations are common.
96-104: Command tenant DB implementation looks correct.The implementation for getting a tenant-specific command database is well-structured. It creates a unique database name with the tenant ID, uses MongoDB's
useDbwith caching for efficiency, and includes proper error handling.
140-145: Good backward compatibility implementation.The backward compatible function with an optional parameter is a clean way to maintain support for existing code while enabling the new functionality. Defaulting to "query" operations is a sensible choice.
151-161: Well-structured model retrieval with operation type parameter.The implementation of
getModelByTenantwith an optional operation type parameter provides a clean interface for client code to specify the database operation type. This maintains backward compatibility while enabling CQRS.
| const logger = log4js.getLogger( | ||
| `${constants.ENVIRONMENT} -- dbConnection-config` | ||
| ); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Logger initialization might be incorrect.
The pipeline error "TypeError: logger.error is not a function" suggests an issue with the logger setup. Ensure that log4js is configured correctly and that the environment variable needed for the logger name is properly set.
🏁 Script executed:
#!/bin/bash
# Check the constants file for ENVIRONMENT variable
grep -n "ENVIRONMENT" src/device-registry/config/constants.js
# Check if there's a log4js configuration somewhere
grep -r -n "log4js.configure" src/device-registry/Length of output: 274
Logger Configuration Issue – Action Required
The investigation shows that the log4js.configure call is only executed in the bin entry points (e.g., in src/device-registry/bin/index.js and src/device-registry/bin/test/ut_index.js), while the logger in src/device-registry/config/database.js is created independently. This means that if the module is imported in contexts where the bin files don’t run, the logger instance may lack essential methods (like error), hence triggering the error.
- Verify that the
ENVIRONMENTvariable fromsrc/device-registry/config/constants.jsis properly set. - Ensure that
log4js.configureis executed before any module (likedatabase.js) callslog4js.getLogger. Consider centralizing the configuration in an initialization script that runs for all contexts.
Please adjust the initialization flow so that the logger is correctly configured across all parts of the application.
🧰 Tools
🪛 GitHub Actions: code-coverage-workflow
[error] 10-10: TypeError: logger.error is not a function
| function getQueryTenantDB(tenantId, modelName, schema) { | ||
| // const dbName = `${constants.DB_NAME}_query_${tenantId}`; | ||
| const dbName = `${constants.DB_NAME}_${tenantId}`; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Commented code and inconsistent naming convention in query tenant DB.
There's a commented-out line that defines a database name with a _query_ suffix, but the actual implementation just uses the tenant ID without a query indication. This inconsistency should be addressed.
function getQueryTenantDB(tenantId, modelName, schema) {
- // const dbName = `${constants.DB_NAME}_query_${tenantId}`;
- const dbName = `${constants.DB_NAME}_${tenantId}`;
+ const dbName = `${constants.DB_NAME}_query_${tenantId}`;
if (queryMongoDB) {📝 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.
| function getQueryTenantDB(tenantId, modelName, schema) { | |
| // const dbName = `${constants.DB_NAME}_query_${tenantId}`; | |
| const dbName = `${constants.DB_NAME}_${tenantId}`; | |
| function getQueryTenantDB(tenantId, modelName, schema) { | |
| const dbName = `${constants.DB_NAME}_query_${tenantId}`; | |
| if (queryMongoDB) { | |
| // rest of implementation... | |
| } | |
| } |
| module.exports = { | ||
| getModelByTenant, | ||
| getTenantDB, | ||
| connectToMongoDB, | ||
| getCommandModelByTenant, | ||
| getQueryModelByTenant, | ||
| getCommandTenantDB, | ||
| getQueryTenantDB, | ||
| }; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Pipeline failure related to logger.
The pipeline is reporting "TypeError: logger.error is not a function" which suggests an issue with the logger configuration. The error seems related to line 10-10 which is within the logger initialization.
🏁 Script executed:
#!/bin/bash
# Check the logger initialization
grep -n "logger =" src/device-registry/config/*.js
# Check if log4js is correctly imported in all files
grep -n "log4js" src/device-registry/config/*.jsLength of output: 927
Logger Import Conflict: Revise Local Log4js Implementation
It appears that the local file src/device-registry/config/log4js.js is inadvertently shadowing the intended npm package since the logger is imported with require("log4js") in src/device-registry/config/database.js. The custom file does not expose a proper getLogger method, so when the code calls logger.error, it fails.
Please consider one of the following corrective actions:
- Rename the local log4js file (or update its location) to prevent naming conflicts with the npm package.
- Or update the require statement in
src/device-registry/config/database.jsto explicitly import the correct module (e.g., using a relative path likerequire("./log4js")if that is intended).
These changes should help ensure that the logger is correctly configured, resolving the pipeline failure.
| db.on("disconnected", (err) => { | ||
| logger.warn( | ||
| `${dbType} database disconnected: ${err ? err.message : "Unknown reason"}` | ||
| ); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential logging error in disconnection event handler.
The disconnection event may not provide an error object, which could cause an issue when trying to access err.message. Your code includes a fallback message, but the check should be on whether err exists, not on the presence of an error message.
db.on("disconnected", (err) => {
logger.warn(
- `${dbType} database disconnected: ${err ? err.message : "Unknown reason"}`
+ `${dbType} database disconnected: ${err && err.message ? err.message : "Unknown reason"}`
);
});📝 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.
| db.on("disconnected", (err) => { | |
| logger.warn( | |
| `${dbType} database disconnected: ${err ? err.message : "Unknown reason"}` | |
| ); | |
| }); | |
| db.on("disconnected", (err) => { | |
| logger.warn( | |
| `${dbType} database disconnected: ${err && err.message ? err.message : "Unknown reason"}` | |
| ); | |
| }); |
|
Device registry changes in this PR available for preview here |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/device-registry/config/database.js (2)
7-10:⚠️ Potential issueEnsure logger is properly configured to prevent TypeError errors.
The pipeline failure shows "TypeError: logger.error is not a function", suggesting an issue with the logger initialization. This type of error typically occurs when log4js isn't configured before getLogger is called.
Consider refactoring the logger initialization to ensure proper configuration:
const log4js = require("log4js"); + // Ensure log4js is configured before getting logger + if (!log4js.configured) { + log4js.configure({ + appenders: { console: { type: "console" } }, + categories: { default: { appenders: ["console"], level: "info" } } + }); + } const logger = log4js.getLogger( `${constants.ENVIRONMENT} -- dbConnection-config` );🧰 Tools
🪛 GitHub Actions: code-coverage-workflow
[error] 10-10: TypeError: logger.error is not a function
111-113: 🛠️ Refactor suggestionInconsistent DB naming convention for query tenant databases.
There's a commented-out line showing a consistent naming pattern (
${constants.DB_NAME}_query_${tenantId}), but the actual implementation uses a different pattern that doesn't indicate it's a query database.function getQueryTenantDB(tenantId, modelName, schema) { - // const dbName = `${constants.DB_NAME}_query_${tenantId}`; - const dbName = `${constants.DB_NAME}_${tenantId}`; + const dbName = `${constants.DB_NAME}_query_${tenantId}`; if (queryMongoDB) {This change will make it clear which databases are for queries and maintain consistent naming with the command databases.
🧹 Nitpick comments (2)
src/device-registry/config/database.js (2)
30-41: Review database naming convention for consistency.The command database uses a "_command" suffix, but the query database uses the base name without a suffix. This difference may lead to confusion and makes it harder to identify which database is which when inspecting the database server.
Consider using a consistent naming approach:
const createQueryConnection = () => mongoose.createConnection(QUERY_URI, { ...options, - dbName: `${constants.DB_NAME}`, + dbName: `${constants.DB_NAME}_query`, });
47-64: Improve connection event handling.The implementation of connection event handlers is good, but there are two minor issues:
- The successful connection log message is commented out, which means you won't have visibility into successful connections
- The Biome analyzer suggests using optional chaining for err.message access
const setupConnectionHandlers = (db, dbType) => { db.on("open", () => { - // logger.info(`Connected to ${dbType} database successfully`); + logger.info(`Connected to ${dbType} database successfully`); }); db.on("error", (err) => { logger.error(`${dbType} database connection error: ${err.message}`); }); db.on("disconnected", (err) => { logger.warn( `${dbType} database disconnected: ${ - err && err.message ? err.message : "Unknown reason" + err?.message || "Unknown reason" }` ); }); };🧰 Tools
🪛 Biome (1.9.4)
[error] 60-60: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/device-registry/config/database.js(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/device-registry/config/database.js
[error] 60-60: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🪛 GitHub Actions: code-coverage-workflow
src/device-registry/config/database.js
[error] 10-10: TypeError: logger.error is not a function
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build-push-deploy-device-registry
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (4)
src/device-registry/config/database.js (4)
12-14: Good implementation of fallback URIs for backward compatibility.The fallback chain ensures graceful degradation by first trying CQRS-specific URIs, then falling back to the legacy URI, and finally defaulting to an empty string if nothing is defined.
142-147: Well-implemented backward compatibility in getTenantDB.The function elegantly routes to the appropriate specialized function based on the operation type, defaulting to query operations for backward compatibility. This is a good pattern for maintaining API compatibility while introducing new features.
153-163: Well-implemented backward compatibility in getModelByTenant.Similar to getTenantDB, this function provides a smooth migration path for existing code while allowing new code to explicitly specify the operation type.
165-173: Comprehensive module exports including both backward-compatible and specialized functions.The exports include both the new specialized functions and the backward-compatible functions, giving clients the flexibility to choose which API to use based on their needs.
| }; | ||
|
|
||
| const mongodb = connectToMongoDB(); | ||
| // Initialize both database connections | ||
| const { commandDB: commandMongoDB, queryDB: queryMongoDB } = connectToMongoDB(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Immediate connection invocation may cause issues with module imports.
Initializing the database connections immediately when the module is loaded could cause problems in testing scenarios or when the module is imported but not yet ready to connect to the database.
Consider making the connections lazy-loaded or providing a way to initialize connections explicitly:
- // Initialize both database connections
- const { commandDB: commandMongoDB, queryDB: queryMongoDB } = connectToMongoDB();
+ // Database connection instances
+ let commandMongoDB = null;
+ let queryMongoDB = null;
+
+ // Initialize database connections explicitly
+ function initializeConnections() {
+ if (!commandMongoDB || !queryMongoDB) {
+ const connections = connectToMongoDB();
+ commandMongoDB = connections.commandDB;
+ queryMongoDB = connections.queryDB;
+ }
+ return { commandMongoDB, queryMongoDB };
+ }Then update each DB access function to call initializeConnections() if needed:
function getCommandTenantDB(tenantId, modelName, schema) {
const dbName = `${constants.DB_NAME}_command_${tenantId}`;
+ if (!commandMongoDB) {
+ initializeConnections();
+ }
if (commandMongoDB) {
// rest of function📝 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 { commandDB: commandMongoDB, queryDB: queryMongoDB } = connectToMongoDB(); | |
| // Instead of immediately initializing the connections: | |
| // const { commandDB: commandMongoDB, queryDB: queryMongoDB } = connectToMongoDB(); | |
| // Use lazy-loaded database connection instances | |
| let commandMongoDB = null; | |
| let queryMongoDB = null; | |
| // Initialize database connections explicitly | |
| function initializeConnections() { | |
| if (!commandMongoDB || !queryMongoDB) { | |
| const connections = connectToMongoDB(); | |
| commandMongoDB = connections.commandDB; | |
| queryMongoDB = connections.queryDB; | |
| } | |
| return { commandMongoDB, queryMongoDB }; | |
| } | |
| // Example usage in a DB access function: | |
| function getCommandTenantDB(tenantId, modelName, schema) { | |
| const dbName = `${constants.DB_NAME}_command_${tenantId}`; | |
| if (!commandMongoDB) { | |
| initializeConnections(); | |
| } | |
| if (commandMongoDB) { | |
| // rest of function implementation using commandMongoDB | |
| } | |
| // Possibly add handling for the case when commandMongoDB is still null | |
| } | |
| // Similar updates should be applied for functions using queryMongoDB. |
There was a problem hiding this comment.
create a Github issue for this.
There was a problem hiding this comment.
🧩 Analysis chain
@Baalmart I've created a GitHub issue for the database connection refactoring suggestion. The issue addresses the concern about immediate connection invocation potentially causing problems in testing scenarios or when the module is imported but not ready to connect to the database.
The issue proposes refactoring the connections to use a lazy-loading pattern, which will improve testability and provide more explicit control over when database connections are established. It includes the specific code changes needed and has been assigned to you.
You can find the issue at the URL provided in the GitHub CLI response above.
🏁 Script executed:
#!/bin/bash
# Create a GitHub issue for the database connection refactoring
gh issue create \
--title "[Device Registry] Refactor database connections to lazy loading pattern" \
--body "## Background
In PR #4513 (https://github.com/airqo-platform/AirQo-api/pull/4513), the following refactor suggestion was identified:
**Immediate connection invocation may cause issues with module imports.**
Initializing the database connections immediately when the module is loaded could cause problems in testing scenarios or when the module is imported but not yet ready to connect to the database.
## Proposed Changes
Refactor the database connections in \`src/device-registry/config/database.js\` to use lazy-loading pattern:
\`\`\`diff
- // Initialize both database connections
- const { commandDB: commandMongoDB, queryDB: queryMongoDB } = connectToMongoDB();
+ // Database connection instances
+ let commandMongoDB = null;
+ let queryMongoDB = null;
+
+ // Initialize database connections explicitly
+ function initializeConnections() {
+ if (!commandMongoDB || !queryMongoDB) {
+ const connections = connectToMongoDB();
+ commandMongoDB = connections.commandDB;
+ queryMongoDB = connections.queryDB;
+ }
+ return { commandMongoDB, queryMongoDB };
+ }
\`\`\`
Then update each DB access function to call initializeConnections() if needed:
\`\`\`diff
function getCommandTenantDB(tenantId, modelName, schema) {
const dbName = \`\${constants.DB_NAME}_command_\${tenantId}\`;
+ if (!commandMongoDB) {
+ initializeConnections();
+ }
if (commandMongoDB) {
// rest of function
\`\`\`
## Benefits
- Improves testability
- Avoids issues when the module is imported but not yet ready to connect to the database
- Makes database connections lazy-loaded
## Related PR
This issue was referenced in PR #4513 (https://github.com/airqo-platform/AirQo-api/pull/4513#discussion_r1977804219)" \
--assignee "Baalmart"
Length of output: 1888
@Baalmart I've successfully created a GitHub issue for the database connection refactoring suggestion. The issue has been created and assigned to you:
Issue: #4520
The issue includes:
- The background explaining why immediate connection invocation could be problematic
- The proposed code changes to implement lazy loading
- The benefits of this approach (improved testability, avoiding issues with module imports)
- A reference back to this PR discussion
This will allow you to address this refactoring in a separate PR while proceeding with the current implementation.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
Description
This pull request adds support for the Command Query Responsibility Segregation (CQRS) pattern to our microservice by enabling connections to separate command and query databases. The implementation maintains backward compatibility with our existing multi-tenant architecture while allowing explicit separation between read and write operations.
Changes Made
Testing
Affected Services
API Documentation Updated?
Additional Notes
Constants Configuration
Add the following to your constants file:
Write Operations (Command):
Read Operations (Query):
Using Backward Compatible Methods:
This implementation provides a foundation for CQRS while maintaining compatibility with existing code, allowing for a gradual migration to the new pattern.
Summary by CodeRabbit
New Features
Chores