Replace deprecated MongoDB option #116

backflip · 2021-06-16T12:57:10Z

The returnOriginal option for findOneAndUpdate is deprecated, see https://github.com/mongodb/node-mongodb-native/blob/f916843ad2002a4c829c5f126e58e8efba257525/lib/collection.js#L1720

This should get rid of warnings like [MONGODB DRIVER] DeprecationWarning: collection.findOneAndUpdate option [returnOriginal] is deprecated and will be removed in a later version.

animir · 2021-06-17T01:27:06Z

@backflip Hi, thank you for this PR.

I think, we can't replace it like that. This package is used with many different versions of node-mongodb-native. The new option was implemented just month ago mongodb/node-mongodb-native@f916843.

One option is to check mongodb-native package version and use appropriate option.

backflip · 2021-06-17T13:02:48Z

Very good point, sorry for missing that. I have added a proposal for reading the mongodb driver version. It looks like the option will be removed with v4: https://github.com/mongodb/node-mongodb-native/releases/tag/v4.0.0-beta.4

animir · 2021-06-23T04:03:51Z

@backflip This looks good. Could you also write a couple of tests to make sure it works as expected?

ainkinen · 2021-06-30T16:07:03Z

Should this maybe cache the version info into a variable to avoid doing the full getDriverVersion operation on every _upsert?

animir · 2021-07-01T07:48:14Z

@ainkinen Very good point. And it would be better if we can get MongoDB driver version from client not from filesystem.
Actually, current solution is not viable at all.

@backflip Could you try to get version from client and store it in the local class variable during class instance construction?

backflip · 2021-07-01T10:04:04Z

@animir, I did not find a way of getting the version from the driver. There was a discussion about exposing it: mongodb/node-mongodb-native#2709

backflip · 2021-07-01T10:28:56Z

Update: It seems to work via client.topology.s.options.metadata.driver.version after successful connection. Let me check this out.

backflip · 2021-07-01T12:13:57Z

@backflip This looks good. Could you also write a couple of tests to make sure it works as expected?

@animir, would you want to replace the stubbed tests or extend them?

animir · 2021-07-01T12:49:43Z

@backflip You can stub a connected Mongo Client and test it for a case when client.topology.s.options.metadata.driver.version exists and not.

backflip · 2021-07-01T13:10:01Z

lib/RateLimiterMongo.js

@@ -235,7 +257,7 @@ class RateLimiterMongo extends RateLimiterStoreAbstract {
    const where = Object.assign({ key: rlKey }, docAttrs);

    return this._collection.deleteOne(where)
-      .then(res => res.result.n > 0);
+      .then(res => res.deletedCount > 0);


I discovered this while testing with the real clients: v4 does not return result anymore. Seems to be in line with https://docs.mongodb.com/manual/reference/method/db.collection.deleteOne/

I discovered this while testing with the real clients: v4 does not return result anymore. Seems to be in line with https://docs.mongodb.com/manual/reference/method/db.collection.deleteOne/

Should it be selected depending on driver version?

It worked fine with 3.6.9, but let me do some digging.

It looks like this was updated in https://github.com/mongodb/node-mongodb-native/pull/2651/files#diff-06ad5e5a8b0f21c69f986da5545842d224dd5a79c054081bc35d714e4bdc6077. However, it just mentions that Our CRUD result types have been out-of-sync with the official drivers CRUD specification for some time. I could not find any old specs where result was part of the response.

@backflip deletedCount is supported at least from MongoDB Server 3.2 http://mongodb.github.io/node-mongodb-native/2.2/api/Collection.html#~deleteWriteOpResult It is safe to use it

@backflip Did it brake tests, where delete function used? I see 3 mocks with result.n mocked for those tests.

@backflip never mind above question. I see you already took care of that 👍

backflip · 2021-07-01T13:10:30Z

Should this maybe cache the version info into a variable to avoid doing the full getDriverVersion operation on every _upsert?

Updated accordingly. Thanks for the input, @ainkinen!

backflip · 2021-07-01T13:15:16Z

@backflip You can stub a connected Mongo Client and test it for a case when client.topology.s.options.metadata.driver.version exists and not.

Should I remove the non-stubbed tests or keep them? They helped me discover that the delete method was failing in v4 (see comment above).

ainkinen · 2021-07-01T13:42:12Z

Should this maybe cache the version info into a variable to avoid doing the full getDriverVersion operation on every _upsert?

Updated accordingly. Thanks for the input, @ainkinen!

The path operations and reading the module were the ones that caught my eye earlier. I think the current solution is much better in general. 👍

animir · 2021-07-01T13:47:34Z

@backflip There is no infrastructure for real MongoDB connection in test environment. I understand, tests with real database can help in some cases, but it also brings new level of complexity. Let's keep stabbed tests for this PR.

backflip · 2021-07-01T14:13:51Z

@backflip There is no infrastructure for real MongoDB connection in test environment. I understand, tests with real database can help in some cases, but it also brings new level of complexity. Let's keep stabbed tests for this PR.

Fair enough. 👍

lib/RateLimiterMongo.js

backflip · 2021-07-06T08:55:31Z

test/RateLimiterMongo.test.js

+
+  it('consume 1 point (driver v3)', (done) => {
+    const testKey = 'consume1v3';
+    sinon.stub(mongoClient, 'topology').value({ s: { options: { metadata: { driver: { version: '3.6' } } } } });


@animir, does this make sense as a mock? I'm checking the resulting upsertOptions below.

@backflip yes, this looks good

Replace deprecated MongoDB option

e9b2b69

Set MongoDB upsert options based on driver version

b99d664

backflip added 4 commits July 1, 2021 15:01

Optimize MongoDB driver version detection

11948b6

Update MongoDB delete method

c6ac0c8

Add tests with MongoDB client

111553c

Move MongoDB driver detection

b0ac15f

backflip commented Jul 1, 2021

View reviewed changes

animir reviewed Jul 2, 2021

View reviewed changes

lib/RateLimiterMongo.js Show resolved Hide resolved

backflip added 2 commits July 6, 2021 10:53

Mock MongoDB driver version test

f1ea0be

Remove MongoDB dev dependencies

a8a969b

backflip commented Jul 6, 2021

View reviewed changes

animir merged commit dd1ebed into animir:master Jul 10, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace deprecated MongoDB option #116

Replace deprecated MongoDB option #116

backflip commented Jun 16, 2021

animir commented Jun 17, 2021

backflip commented Jun 17, 2021

animir commented Jun 23, 2021

ainkinen commented Jun 30, 2021

animir commented Jul 1, 2021

backflip commented Jul 1, 2021

backflip commented Jul 1, 2021 •

edited

Loading

backflip commented Jul 1, 2021

animir commented Jul 1, 2021

backflip Jul 1, 2021

animir Jul 1, 2021

backflip Jul 1, 2021

backflip Jul 6, 2021

animir Jul 10, 2021

animir Jul 10, 2021

animir Jul 10, 2021

backflip commented Jul 1, 2021

backflip commented Jul 1, 2021

ainkinen commented Jul 1, 2021

animir commented Jul 1, 2021

backflip commented Jul 1, 2021

backflip Jul 6, 2021

animir Jul 10, 2021

Replace deprecated MongoDB option #116

Replace deprecated MongoDB option #116

Conversation

backflip commented Jun 16, 2021

animir commented Jun 17, 2021

backflip commented Jun 17, 2021

animir commented Jun 23, 2021

ainkinen commented Jun 30, 2021

animir commented Jul 1, 2021

backflip commented Jul 1, 2021

backflip commented Jul 1, 2021 • edited Loading

backflip commented Jul 1, 2021

animir commented Jul 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

backflip commented Jul 1, 2021

backflip commented Jul 1, 2021

ainkinen commented Jul 1, 2021

animir commented Jul 1, 2021

backflip commented Jul 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

backflip commented Jul 1, 2021 •

edited

Loading