Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Device Registry] Implement CQRS pattern with dual database connections #4513

Merged
merged 4 commits into from
Mar 3, 2025
+138 −19
Merged

[Device Registry] Implement CQRS pattern with dual database connections #4513

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7543642
Implement CQRS pattern with dual database connections for microservice
Baalmart Mar 2, 2025
344672f
Merge branch 'staging' into cqrs-device-registry
Baalmart Mar 3, 2025
fdf89ce
just ensuring that the environmental variable for the DB connection s…
Baalmart Mar 3, 2025
ef43a76
handling edge case where the err object might not be existing
Baalmart Mar 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/device-registry/.env.example
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,12 @@ MONGO_GCE_URI=
MONGO_URI_PROD=
MONGO_URI_STAGE=
MONGO_URI_DEV=
COMMAND_MONGO_URI_PROD=
COMMAND_MONGO_URI_STAGE=
COMMAND_MONGO_URI_DEV=
QUERY_MONGO_URI_PROD=
QUERY_MONGO_URI_STAGE=
QUERY_MONGO_URI_DEV=
MONGO_DEV=
MONGO_TEST=
MONGO_PROD=
Expand Down
145 changes: 126 additions & 19 deletions src/device-registry/config/database.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,15 @@ mongoose.set("useFindAndModify", false);
mongoose.set("useCreateIndex", true);
mongoose.set("debug", false);
const constants = require("./constants");
const URI = constants.MONGO_URI;
const log4js = require("log4js");
const logger = log4js.getLogger(
`${constants.ENVIRONMENT} -- dbConnection-config`
);
Comment on lines 8 to 10
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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 ENVIRONMENT variable from src/device-registry/config/constants.js is properly set.
  • Ensure that log4js.configure is executed before any module (like database.js) calls log4js.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


// 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 || "";

const options = {
useCreateIndex: true,
useNewUrlParser: true,
Expand All @@ -22,45 +25,149 @@ const options = {
connectTimeoutMS: 1200000,
socketTimeoutMS: 600000,
serverSelectionTimeoutMS: 3600000,
dbName: constants.DB_NAME,
};

const connect = () => mongoose.createConnection(URI, options);
// Create separate connection functions for command and query databases
const createCommandConnection = () =>
mongoose.createConnection(COMMAND_URI, {
...options,
dbName: `${constants.DB_NAME}_command`,
});

const createQueryConnection = () =>
mongoose.createConnection(QUERY_URI, {
...options,
dbName: `${constants.DB_NAME}`,
});

// Store database connections
let commandDB = null;
let queryDB = null;

// Helper function to set up connection event handlers
const setupConnectionHandlers = (db, dbType) => {
db.on("open", () => {
// 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"
}`
);
});
};

const connectToMongoDB = () => {
try {
const db = connect();
db.on("open", () => {});
db.on("error", (err) => {});
// Establish command database connection
commandDB = createCommandConnection();
setupConnectionHandlers(commandDB, "command");

db.on("disconnection", (err) => {});
// Establish query database connection
queryDB = createQueryConnection();
setupConnectionHandlers(queryDB, "query");

process.on("unlimitedRejection", (reason, p) => {
console.log("Unhandled Rejection at: Promise", p, "reason:", reason);
// Set up global error handlers
process.on("unhandledRejection", (reason, p) => {
logger.error("Unhandled Rejection at: Promise", p, "reason:", reason);
});

process.on("uncaughtException", (err) => {
console.error("There was an uncaught error", err);
logger.error("There was an uncaught error", err);
});

return db;
} catch (error) {}
return { commandDB, queryDB };
} catch (error) {
logger.error(`Database connection error: ${error.message}`);
throw error;
}
};

const mongodb = connectToMongoDB();
// Initialize both database connections
const { commandDB: commandMongoDB, queryDB: queryMongoDB } = connectToMongoDB();
Copy link
Contributor

@coderabbitai coderabbitai bot Mar 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

create a Github issue for this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 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!


/**
* Get a tenant-specific command database (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");
}

function getTenantDB(tenantId, modelName, schema) {
/**
* Get a tenant-specific query database (for read operations)
*/
function getQueryTenantDB(tenantId, modelName, schema) {
// const dbName = `${constants.DB_NAME}_query_${tenantId}`;
const dbName = `${constants.DB_NAME}_${tenantId}`;
Comment on lines +111 to 113
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
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...
}
}

if (mongodb) {
const db = mongodb.useDb(dbName, { useCache: true });
if (queryMongoDB) {
const db = queryMongoDB.useDb(dbName, { useCache: true });
db.model(modelName, schema);
return db;
}
throw new Error("Query database connection not established");
}

/**
* Get a command model (for write operations) for a specific tenant
*/
function getCommandModelByTenant(tenantId, modelName, schema) {
const tenantDb = getCommandTenantDB(tenantId, modelName, schema);
return tenantDb.model(modelName);
}

function getModelByTenant(tenantId, modelName, schema) {
const tenantDb = getTenantDB(tenantId, modelName, schema);
/**
* Get a query model (for read operations) for a specific tenant
*/
function getQueryModelByTenant(tenantId, modelName, schema) {
const tenantDb = getQueryTenantDB(tenantId, modelName, schema);
return tenantDb.model(modelName);
}

module.exports = { getModelByTenant, getTenantDB, connectToMongoDB };
/**
* Backward compatible function to get tenant database
* Now accepts an optional operationType parameter
*/
function getTenantDB(tenantId, modelName, schema, operationType = "query") {
if (operationType === "command") {
return getCommandTenantDB(tenantId, modelName, schema);
}
return getQueryTenantDB(tenantId, modelName, schema);
}

/**
* Backward compatible function to get tenant model
* Now accepts an optional operationType parameter
*/
function getModelByTenant(
tenantId,
modelName,
schema,
operationType = "query"
) {
if (operationType === "command") {
return getCommandModelByTenant(tenantId, modelName, schema);
}
return getQueryModelByTenant(tenantId, modelName, schema);
}

module.exports = {
getModelByTenant,
getTenantDB,
connectToMongoDB,
getCommandModelByTenant,
getQueryModelByTenant,
getCommandTenantDB,
getQueryTenantDB,
};
Comment on lines +165 to +173
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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/*.js

Length 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.js to explicitly import the correct module (e.g., using a relative path like require("./log4js") if that is intended).

These changes should help ensure that the logger is correctly configured, resolving the pipeline failure.

2 changes: 2 additions & 0 deletions src/device-registry/config/environments/development.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ const logger = log4js.getLogger(`${this.ENVIRONMENT} -- constants-config`);
const devConfig = {
DEFAULT_COHORT: process.env.DEV_DEFAULT_COHORT,
MONGO_URI: process.env.MONGO_URI_DEV,
COMMAND_MONGO_URI: process.env.COMMAND_MONGO_URI_DEV,
QUERY_MONGO_URI: process.env.QUERY_MONGO_URI_DEV,
DB_NAME: process.env.MONGO_DEV,
REDIS_SERVER: process.env.REDIS_SERVER_DEV,
REDIS_PORT: process.env.REDIS_PORT,
Expand Down
2 changes: 2 additions & 0 deletions src/device-registry/config/environments/production.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ const logger = log4js.getLogger(`${this.ENVIRONMENT} -- constants-config`);
const prodConfig = {
DEFAULT_COHORT: process.env.PROD_DEFAULT_COHORT,
MONGO_URI: process.env.MONGO_URI_PROD,
COMMAND_MONGO_URI: process.env.COMMAND_MONGO_URI_PROD,
QUERY_MONGO_URI: process.env.QUERY_MONGO_URI_PROD,
DB_NAME: process.env.MONGO_PROD,
REDIS_SERVER: process.env.REDIS_SERVER,
REDIS_PORT: process.env.REDIS_PORT,
Expand Down
2 changes: 2 additions & 0 deletions src/device-registry/config/environments/staging.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ const logger = log4js.getLogger(`${this.ENVIRONMENT} -- constants-config`);
const stageConfig = {
DEFAULT_COHORT: process.env.STAGE_DEFAULT_COHORT,
MONGO_URI: process.env.MONGO_URI_STAGE,
COMMAND_MONGO_URI: process.env.COMMAND_MONGO_URI_STAGE,
QUERY_MONGO_URI: process.env.QUERY_MONGO_URI_STAGE,
DB_NAME: process.env.MONGO_STAGE,
REDIS_SERVER: process.env.REDIS_SERVER,
REDIS_PORT: process.env.REDIS_PORT,
Expand Down
Loading