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

Jump to bottom

Improvement/bb 632 bump mongo db driver 6 1 #2596

Open
wants to merge 4 commits into
base: improvement/BB-615-node-22-upgrade
Choose a base branch
from
Open

Improvement/bb 632 bump mongo db driver 6 1 #2596

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
8328f56
Bump mongodb driver to latest version 6.10
KillianG Nov 21, 2024
bb35a15
Update mongodb to use promises instead of callbacks
KillianG Nov 21, 2024
ee13447
Fix bad usage of sinon yields, using resolves instead
KillianG Nov 21, 2024
03ae19a
Refactor LocationStatusManager tests to use resolves instead of yield…
KillianG Nov 21, 2024
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
21 changes: 11 additions & 10 deletions extensions/notification/configManager/MongoConfigManager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,18 +97,13 @@ class MongoConfigManager extends BaseConfigManager {
*/
_setupMongoClient(cb) {
const mongoUrl = constructConnectionString(this._mongoConfig);
MongoClient.connect(mongoUrl, {
const client = new MongoClient(mongoUrl, {
replicaSet: this._mongoConfig.replicaSet,
useNewUrlParser: true,
},
(err, client) => {
if (err) {
this._logger.error('Could not connect to MongoDB', {
method: 'MongoConfigManager._setupMongoClient',
error: err.message,
});
return cb(err);
}
readPreference: this._mongoConfig.readPreference,
});

return client.connect().then(client => {
this._logger.debug('Connected to MongoDB', {
method: 'MongoConfigManager._setupMongoClient',
});
Expand All @@ -133,6 +128,12 @@ class MongoConfigManager extends BaseConfigManager {
} catch (error) {
return cb(error);
}
}).catch(err => {
this._logger.error('Could not connect to MongoDB', {
method: 'MongoConfigManager._setupMongoClient',
error: err.message,
});
return cb(err);
});
}

Expand Down
4 changes: 3 additions & 1 deletion extensions/oplogPopulator/OplogPopulator.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,11 +80,13 @@ class OplogPopulator {
*/
async _setupMongoClient() {
try {
const client = await MongoClient.connect(this._mongoUrl, {
let client = new MongoClient(this._mongoUrl, {
replicaSet: this._replicaSet,
useNewUrlParser: true,
useUnifiedTopology: true,
readPreference: this._mongoConfig.readPreference,
});
client = await client.connect();
// connect to metadata DB
this._mongoClient = client.db(this._database, {
ignoreUndefined: true,
Expand Down
18 changes: 9 additions & 9 deletions extensions/utils/LocationStatusStream.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,18 +63,12 @@
*/
_setupMongoClient(cb) {
const mongoUrl = constructConnectionString(this._mongoConfig);
MongoClient.connect(mongoUrl, {
const client = new MongoClient(mongoUrl, {
replicaSet: this._mongoConfig.replicaSet,
useNewUrlParser: true,
useUnifiedTopology: true,
}, (err, client) => {
if (err) {
this._log.error('Could not connect to MongoDB', {
method: 'ServiceStatusManager._setupMongoClient',
error: err.message,
});
return cb(err);
}
});
return client.connect().then(client => {
// connect to metadata DB
this._mongoClient = client.db(this._mongoConfig.database, {
ignoreUndefined: true,
Expand All @@ -96,6 +90,12 @@
return cb();
});
return undefined;
}).catch(err => {
this._log.error('Could not connect to MongoDB', {

Check warning on line 94 in extensions/utils/LocationStatusStream.js

View check run for this annotation

Codecov / codecov/patch/Backbeat

extensions/utils/LocationStatusStream.js#L94 

Added line #L94 was not covered by tests
method: 'ServiceStatusManager._setupMongoClient',
error: err.message,
});
return cb(err);

Check warning on line 98 in extensions/utils/LocationStatusStream.js

View check run for this annotation

Codecov / codecov/patch/Backbeat

extensions/utils/LocationStatusStream.js#L98 

Added line #L98 was not covered by tests
});
}

Expand Down
21 changes: 10 additions & 11 deletions lib/api/BackbeatAPI.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1335,21 +1335,14 @@ class BackbeatAPI {
});
return cb();
} else {
// To be removed once the mongodb drivers are bumped
// BB-585
const mongoUrl = constructConnectionString(mongoConfig);
return MongoClient.connect(mongoUrl, {
const client = new MongoClient(mongoUrl, {
replicaSet: mongoConfig.replicaSet,
useNewUrlParser: true,
useUnifiedTopology: true,
}, (err, client) => {
if (err) {
this._logger.error('Could not connect to MongoDB', {
method: 'BackbeatAPI._setupMongoClient',
error: err.message,
});
return cb(err);
}
});

return client.connect().then(client => {
// connect to metadata DB
this._mongoClient = client.db(mongoConfig.database, {
ignoreUndefined: true,
Expand All @@ -1358,6 +1351,12 @@ class BackbeatAPI {
method: 'BackbeatAPI._setupMongoClient',
});
return cb();
}).catch(err => {
this._logger.error('Could not connect to MongoDB', {
method: 'BackbeatAPI._setupMongoClient',
error: err.message,
});
return cb(err);
});
}
}
Expand Down
62 changes: 33 additions & 29 deletions lib/util/LocationStatusManager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,19 +68,23 @@
* @return {undefined}
*/
_initCollection(cb) {
this._mongoClient.createCollection(locationStatusCollection, err => {
// in case the collection already exists, we ignore the error
if (err && err.codeName !== 'NamespaceExists') {
this._logger.error('Could not create mongo collection', {
method: 'LocationStatusManager._initCollection',
collection: locationStatusCollection,
error: err.message,
});
return cb(err);
}
this._locationStatusColl = this._mongoClient.collection(locationStatusCollection);
return cb();
});
return this._mongoClient.createCollection(locationStatusCollection)
.then(() => {
this._locationStatusColl = this._mongoClient.collection(locationStatusCollection);
return cb();
})
.catch(err => {
if (err.codeName !== 'NamespaceExists') {
this._logger.error('Could not create mongo collection', {

Check warning on line 78 in lib/util/LocationStatusManager.js

View check run for this annotation

Codecov / codecov/patch/Backbeat

lib/util/LocationStatusManager.js#L78 

Added line #L78 was not covered by tests
method: 'LocationStatusManager._initCollection',
collection: locationStatusCollection,
error: err.message,
});
return cb(err);

Check warning on line 83 in lib/util/LocationStatusManager.js

View check run for this annotation

Codecov / codecov/patch/Backbeat

lib/util/LocationStatusManager.js#L83 

Added line #L83 was not covered by tests
}
this._locationStatusColl = this._mongoClient.collection(locationStatusCollection);
return cb();
});
}

/**
Expand All @@ -89,16 +93,17 @@
* @returns {undefined}}
*/
_listCollectionDocuments(cb) {
this._locationStatusColl.find({}, (err, cursor) => {
if (err) {
this._locationStatusColl.find({})
.toArray()
.then(docs => cb(null, docs))
.catch(err => {
this._logger.error('Could not list documents', {
method: 'LocationStatusManager._listCollectionDocuments',
collection: locationStatusCollection,
error: err.message,
});
}
return cursor.toArray(cb);
});
cb(err);

Check warning on line 105 in lib/util/LocationStatusManager.js

View check run for this annotation

Codecov / codecov/patch/Backbeat

lib/util/LocationStatusManager.js#L105 

Added line #L105 was not covered by tests
});
}

/**
Expand Down Expand Up @@ -139,15 +144,14 @@
_id: {
$in: invalidLocations
}
}, err => {
if (err) {
this._logger.error('Could not delete invalid locations', {
method: 'LocationStatusManager._deleteInvalidLocations',
error: err.message,
});
return cb(err);
}
return cb(null, locations);
})
.then(() => cb(null, locations))
.catch(err => {
this._logger.error('Could not delete invalid locations', {

Check warning on line 150 in lib/util/LocationStatusManager.js

View check run for this annotation

Codecov / codecov/patch/Backbeat

lib/util/LocationStatusManager.js#L150 

Added line #L150 was not covered by tests
method: 'LocationStatusManager._deleteInvalidLocations',
error: err.message,
});
cb(err);

Check warning on line 154 in lib/util/LocationStatusManager.js

View check run for this annotation

Codecov / codecov/patch/Backbeat

lib/util/LocationStatusManager.js#L154 

Added line #L154 was not covered by tests
});
}

Expand All @@ -166,10 +170,10 @@
async.eachLimit(newLocations, 10, (location, next) => {
const locationConfig = new LocationStatus(this._supportedServices);
this._locationStatusStore[location] = locationConfig;
this._locationStatusColl.insert({
this._locationStatusColl.insertOne({
_id: location,
value: locationConfig.getValue(),
}, next);
}).finally(next);
}, err => {
if (err) {
this._logger.error('Could not add new locations', {
Expand Down
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@
"ioredis": "^4.9.5",
"joi": "^17.6.0",
"minimatch": "^3.0.4",
"mongodb": "^3.1.13",
"mongodb": "^6.10.0",
"node-forge": "^0.7.6",
"node-rdkafka": "^2.12.0",
"node-rdkafka-prometheus": "^1.0.0",
Expand Down
3 changes: 2 additions & 1 deletion tests/config.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@
},
"mongo": {
"logName": "s3-recordlog",
"replicaSetHosts": "localhost:27018"
"replicaSetHosts": "localhost:27018",
"readPreference": "primary"
},
"probeServer": {
"bindAddress": "localhost",
Expand Down
12 changes: 5 additions & 7 deletions tests/functional/ingestion/IngestionReader.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,16 +112,14 @@ describe('ingestion reader tests with mock', function fD() {
const topic = testConfig.extensions.ingestion.topic;
async.waterfall([
next => {
MongoClient.connect(mongoUrl, {}, (err, client) => {
if (err) {
next(err);
}
const client = new MongoClient(mongoUrl, {});
client.connect().then(client => {
this.client = client;
this.db = this.client.db('metadata', {
ignoreUndefined: true,
});
next();
});
}).catch(next);
},
next => kafkaAdminClient.createTopic({
topic,
Expand Down Expand Up @@ -153,10 +151,10 @@ describe('ingestion reader tests with mock', function fD() {
consumer.subscribe([testConfig.extensions.ingestion.topic]);
setTimeout(next, 2000);
},
next => this.db.createCollection('PENSIEVE', err => {
next => this.db.createCollection('PENSIEVE').catch(err => {
assert.ifError(err);
return next();
}),
}).then(next),
next => {
this.m = this.db.collection('PENSIEVE');
this.m.insertOne(dummyPensieveCredentials, {});
Expand Down
6 changes: 3 additions & 3 deletions tests/unit/api/BackbeatAPI.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,7 @@ describe('BackbeatAPI', () => {
let debugSpy;

beforeEach(() => {
mongoClientStub = sinon.stub(MongoClient, 'connect');
mongoClientStub = sinon.stub(MongoClient.prototype, 'connect');
infoSpy = sinon.spy(fakeLogger, 'info');
errorSpy = sinon.spy(fakeLogger, 'error');
debugSpy = sinon.spy(fakeLogger, 'debug');
Expand All @@ -242,7 +242,7 @@ describe('BackbeatAPI', () => {

it('should connect to MongoDB when configuration is present', done => {
const mockDb = { db: sinon.stub().returns({}) };
mongoClientStub.yields(null, mockDb);
mongoClientStub.resolves(mockDb);

bbapi._setupMongoClient(err => {
assert.ifError(err);
Expand All @@ -256,7 +256,7 @@ describe('BackbeatAPI', () => {

it('should log an error when MongoDB connection fails', done => {
const mockError = new Error('Connection failed');
mongoClientStub.yields(mockError);
mongoClientStub.rejects(mockError);

bbapi._setupMongoClient(err => {
assert.strictEqual(err, mockError);
Expand Down
14 changes: 7 additions & 7 deletions tests/unit/lib/util/LocationStatusManager.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,13 +249,13 @@ describe('LocationStatusManager', () => {
].forEach(params => {
it(params.case, done => {
lsm._mongoClient = {
createCollection: sinon.stub().yields(),
createCollection: sinon.stub().resolves(),
collection: () => ({
find: sinon.stub().yields(null, {
toArray: sinon.stub().yields(null, params.mongoLocations),
find: sinon.stub().returns({
toArray: sinon.stub().resolves(params.mongoLocations),
}),
insert: sinon.stub().yields(),
deleteMany: sinon.stub().yields(),
insertOne: sinon.stub().resolves(),
deleteMany: sinon.stub().resolves(),
}),
};
lsm._setupLocationStatusStore(err => {
Expand Down Expand Up @@ -337,7 +337,7 @@ describe('LocationStatusManager', () => {
describe('_addNewLocations', () => {
it('should add new locations', done => {
lsm._locationStatusColl = {
insert: sinon.stub().yields(),
insertOne: sinon.stub().resolves(),
};
lsm._addNewLocations([], err => {
assert.ifError(err);
Expand All @@ -361,7 +361,7 @@ describe('LocationStatusManager', () => {
}
},
];
const deleteManyStub = sinon.stub().yields();
const deleteManyStub = sinon.stub().resolves();
lsm._locationStatusColl = {
deleteMany: deleteManyStub,
};
Expand Down
7 changes: 3 additions & 4 deletions tests/unit/notification/configManager/MongoConfigManager.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ describe('MongoConfigManager ::', () => {
collection: getCollectionStub,
command: mongoCommandStub,
});
const mongoConnectStub = sinon.stub(MongoClient, 'connect').callsArgWith(2, null, {
const mongoConnectStub = sinon.stub(MongoClient.prototype, 'connect').resolves({
db: getDbStub,
});
manager._setupMongoClient(err => {
Expand All @@ -127,8 +127,7 @@ describe('MongoConfigManager ::', () => {

it('should fail when mongo client setup fails', () => {
const manager = new MongoConfigManager(params);
sinon.stub(MongoClient, 'connect').callsArgWith(2,
errors.InternalError, null);
sinon.stub(MongoClient.prototype, 'connect').rejects(errors.InternalError);
manager._setupMongoClient(err => {
assert.deepEqual(err, errors.InternalError);
});
Expand All @@ -137,7 +136,7 @@ describe('MongoConfigManager ::', () => {
it('should fail when when getting the metadata db', () => {
const manager = new MongoConfigManager(params);
const getDbStub = sinon.stub().throws(errors.InternalError);
sinon.stub(MongoClient, 'connect').callsArgWith(2, null, {
sinon.stub(MongoClient.prototype, 'connect').resolves({
db: getDbStub,
});
manager._setupMongoClient(err => {
Expand Down
12 changes: 1 addition & 11 deletions tests/unit/oplogPopulator/oplogPopulator.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,21 +277,11 @@ describe('OplogPopulator', () => {
collection: collectionStub,
command: mongoCommandStub,
});
const mongoConnectStub = sinon.stub(MongoClient, 'connect').resolves({
sinon.stub(MongoClient.prototype, 'connect').resolves({
db: dbStub,
});
await oplogPopulator._setupMongoClient()
.then(() => {
const mongoUrl = 'mongodb://user:password@localhost:27017,localhost:27018,' +
'localhost:27019/?w=majority&readPreference=primary&replicaSet=rs0';
assert(mongoConnectStub.calledOnceWith(
mongoUrl,
{
replicaSet: 'rs0',
useNewUrlParser: true,
useUnifiedTopology: true,
}
));
assert(dbStub.calledOnceWith('metadata', { ignoreUndefined: true }));
assert(collectionStub.calledOnceWith('__metastore'));
assert(mongoCommandStub.calledOnceWith({
Expand Down
Loading
Loading