Mongo package upgrade

[piyush226811]

No description provided.

[ankurlucknowi]

Summary of Recommendations:

1. Proper error handling is missing for some asynchronous operations which could lead to unhandled rejections or exceptions.
2. Some deprecated methods are being used, which may lead to unexpected behavior or lack of compatibility with newer versions of libraries.
3. Potential use of undefined variables or properties that can lead to runtime errors.
4. Using consistent coding practices and inlining changes to reduce redundancy and improve clarity.

Code Review Comments:

File Name with Path: ./Makefile - Around Line Number 4 - [Category: Suggestions]

Review Comment: Ensure that transitioning from mongo to mongosh is backward-compatible by checking if the required version of mongosh is installed on the deployment environment.

4c4
- mongo sails-mongo --eval 'db.dropDatabase()'
+ mongosh sails-mongo --eval 'db.dropDatabase()'

File Name with Path: ./config/connection.js - Around Line Number 74 - [Category: Major]

Review Comment: The collection.aggregate method currently lacks error handling when converting to an array. This may result in unhandled promise rejections.

74c75
- return collection.aggregate(aggregate).toArray(function(err, results) {
+ return collection.aggregate(aggregate).toArray(function(err, results) {
+ if(err) return cb(err);

File Name with Path: ./lib/adapter.js - Around Line Number 136 - [Category: Major]

Review Comment: Variables that are destructured or accessed directly should be verified for existence to avoid potential runtime errors.

- var clients = _.pluck(_connections, 'client');
+ var clients = _connections.map(conn => conn.client).filter(Boolean);

File Name with Path: ./lib/adapter.js - Around Line Number 171 - [Category: Blocker]

Review Comment: Changing asynchronous callback patterns from synchronous to asynchronous needs proper error handling. Current handling could potentially lead to a situation where errors are not caught.

- connectionObject.connection.db.listCollections(collectionName)
+ connectionObject.connection.db.listCollections({ name: collectionName }).toArray(function(err, collections) {
+ if(err) return cb(err);

File Name with Path: ./lib/utils.js - Around Line Number 69 - [Category: Suggestions]

Review Comment: exports.rewriteIds should handle edge cases for handling null values, ensuring the function doesn't break when unexpected types are passed.

69c69
- exports.rewriteIds = function rewriteIds(models, schema) {
+ exports.rewriteIds = function rewriteIds(models, schema) {
+ if (!models || !Array.isArray(models)) {
+    return [];

File Name with Path: ./lib/document.js - Around Line Number 72 - [Category: Suggestions]

Review Comment: Use a consistent approach for creating ObjectId from string to avoid confusion and possible erroneous implementations in future updates.

- values._id = new ObjectId.createFromHexString(values.id);
+ values._id = new ObjectId(values.id);

File Name with Path: ./lib/utils.js - Around Line Number 145 - [Category: Suggestions]

Review Comment: Deprecated usage of URL parsing from Node.js should be updated to use the URL interface for consistency and accuracy.

145c145
- var obj = url.parse(config.url);
+ var obj = new URL(config.url);

These comments will help ensure the new codebase maintains stability, efficiency, and security without introducing new errors or deprecating important functionalities.

[ankurlucknowi]

Summary of Recommendations:
The code diff primarily includes updates for the MongoDB driver from legacy methods to the newer methods available in MongoDB 4.x. They refactor practices for better structure and maintainability but introduce a couple of potential issues. Specifically, there's room for improvement in error handling, ensuring backward compatibility for connection methods, and a couple of potential security concerns around authentication.

Code Review Comments:

File Name with Path: [Unknown] - Around Line Number 78 - [Category: Major]
Review Comment: While the use of toArray ensures that the query results are converted into an array, it is critical to handle any errors that may arise due to querying large datasets, which can lead to performance issues.
Code to Improve:

  collection.find(where, queryOptions).project(query.select).toArray(function(err, docs) {

Suggested Fix:
Consider using cursor iteration with a batchSize to limit memory consumption and improve performance for large result sets.

File Name with Path: [Unknown] - Around Line Number 169 - [Category: Blocker]
Review Comment: The switch from mongo to mongosh requires that the new MongoDB Shell (mongosh) be available in the runtime environment. This change could break setups where mongosh is not installed.
Code to Improve:

+	mongosh sails-mongo --eval 'db.dropDatabase()'

Suggested Fix:
To ensure backward compatibility, check for the availability of mongosh and fall back to mongo if it's not available. Possibly provide a script to install mongosh.

File Name with Path: [Unknown] - Around Line Number 604 - [Category: Blocker]
Review Comment: Changing from explicit connection option handling to a connection string without accompanying information on credential management introduces a potential security risk if user and password are not sanitized.
Code to Improve:

+  let connectionString = this.config.url;
+  if (!connectionString) {
+    connectionString = 'mongodb://';
+    
+    if (this.config.user && this.config.password) {
+      connectionString += `${encodeURIComponent(this.config.user)}:${encodeURIComponent(this.config.password)}@`;
+    }

Suggested Fix:
Ensure secure handling of credentials, preferably using environment variables or secure storage solutions instead of hardcoding sensitive data in the source.

File Name with Path: [Unknown] - Around Line Number 80 - [Category: Suggestions]
Review Comment: The migration to use the MongoDB 4.x functions is beneficial but could benefit from an abstraction to handle common database operations, improving code reuse and maintainability.
Suggested Fix:
Consider creating helper functions or classes to abstract database operations (e.g., connect, disconnect, error handling). This can be especially useful given the repetitive nature of certain operations like inserts, queries, and updates across different methods.

[ankurlucknowi]

Summary of Recommendations:
The code diff showcases the migration from mongodb version 2.x to 4.x, with numerous improvements in using updated methods and restructuring connection settings. However, some areas need refinement around error handling, security enhancements (especially regarding connections), and backward compatibility for certain deprecated options. Opportunities for abstraction and code reusability were identified, and an attempt to improve handling of edge cases in data processing was made, balancing between maintaining existing functionality and embracing new features.

Code Review Comments:

File Name with Path: IntegrationTestFile.js - Around Line Number 7 - [Category: Major]
Review Comment: The switch to mongosh for MongoDB shell operations is a positive change. However, consider including logic to verify configuration to determine if mongosh is available in the environment where tests will run.
Code to Improve:

+	mongosh sails-mongo --eval 'db.dropDatabase()'

Review Comment: Ensure compatibility in environments lacking mongosh.
Suggested Fix:

+	command -v mongosh >/dev/null 2>&1 && mongosh sails-mongo --eval 'db.dropDatabase()' || mongo sails-mongo --eval 'db.dropDatabase()'

File Name with Path: ConnectionFile.js - Around Line Number 91 - [Category: Blocker]
Review Comment: In the updated close logic for client connections, ensure defensive programming by checking if the client instance is already closed or is in a corrupt state before attempting to close.
Code to Improve:

+        if(client === undefined) { return onClosed(); }
+        client.close(onClosed);

Suggested Fix:

+        if (client === undefined || !client.isConnected()) { return onClosed(); }
+        client.close(onClosed);

File Name with Path: DatabaseConfig.js - Around Line Number 86 - [Category: Suggestions]
Review Comment: Abstract connection string building logic for reuse across possible database connection initializations, enhancing maintainability and readability.
Code to Improve:

+  let connectionString;
+  // logic to build connection string...

Suggested Fix:

function buildConnectionString(config) {
  let connectionString = 'mongodb://';
  // Build the connection string using config details
  return connectionString;
}
// Usage example
+ connectionString = buildConnectionString(this.config);

File Name with Path: CollectionHandlers.js - Around Line Number 128 - [Category: Blocker]
Review Comment: Handling of database stream (dbStream) changes to cursor should include considerations for error states and interruptions more defensively.
Code to Improve:

+  cursor.on('data', function(item) {
+    cursor.pause();
+    // processing...
+  });
+  cursor.on('error', function(err) {

Suggested Fix:

+  try {
+    cursor.on('data', function(item) {
+      cursor.pause();
+      // processing...
+    });
+    cursor.on('error', function(err) {
+      stream.end(err);
+    });
+  } catch (err) {
+    stream.end(err);
+  }

File Name with Path: CollectionOperations.js - Around Line Number 269 - [Category: Major]
Review Comment: The switch to deleteMany is efficient. However, ensure rollback or compensation logic is present in cases where there might be partial failures (especially within multi-document transactions in future extensions).
Code to Improve:

+  collection.deleteMany(query.criteria.where, function(err, result) {

Suggested Fix:

// Ensure robustness, especially if part of a larger transaction
+  transaction.start();
+  try {
+    collection.deleteMany(query.criteria.where, function(err, result) {
+      if(err) throw err;
+      transaction.commit();
+      // Processing result...
+    });
+  } catch (err) {
+    transaction.rollback();
+    return cb(err);
+  }

File Name with Path: UtilityFunctions.js - Around Line Number 69 - [Category: Suggestions]
Review Comment: Optimize the rewriteIds function to handle and log potential anomalies in input structures.
Code to Improve:

+  if (!models || !Array.isArray(models)) {
+    return [];
+  }

Suggested Fix:

+  if (!models || !Array.isArray(models)) {
+    console.warn('Expected an array, received:', models);
+    return [];
+  }

These comments aim to ensure that the code is robust, maintainable, secure, and compatible with various MongoDB configurations.