feat: Deprecate top-level write concern option keys #2624
Conversation
HanaPearlman
force-pushed
the
NODE-1722/3.6/deprecate-wc-options
branch
2 times, most recently
from
15cd47d to
a5362ff
Compare
HanaPearlman
changed the title
HanaPearlman
force-pushed
the
NODE-1722/3.6/deprecate-wc-options
branch
from
2ef376f to
0818bd8
Compare
HanaPearlman
force-pushed
the
NODE-1722/3.6/deprecate-wc-options
branch
2 times, most recently
from
19bbd0c to
0b2c24d
Compare
nbbeeken
left a comment
nbbeeken
left a comment
There was a problem hiding this comment.
looking mostly good!
As the for the test output sometime it turns into a WALL of deprecations 😆
I'd take a look at test/tools/runner/config.js:225 specifically the writeConcernMax method and see if changing that to the correct format will clean most of it up.
lib/gridfs/grid_store.js
Outdated
| * @param {(number|string)} [options.w] DEPRECATED: The write concern. Use writeConcern instead. | ||
| * @param {number} [options.wtimeout] DEPRECATED: The write concern timeout. Use writeConcern instead. | ||
| * @param {boolean} [options.j=false] DEPRECATED: Specify a journal write concern. Use writeConcern instead. | ||
| * @param {boolean} [options.fsync=false] DEPRECATED: Specify a file sync write concern. Use writeConcern instead. |
There was a problem hiding this comment.
https://docs.mongodb.com/manual/reference/write-concern/index.html
Fsync isn't an option anymore, its equivalent to journal, maybe this is a good opportunity to write a message: "since version x use journal instead"
| // Execute the operations | ||
| batch.execute(self.configuration.writeConcernMax(), function(err, result) { | ||
| batch.execute(self.configuration.writeConcernMax().writeConcern, function(err, result) { | ||
| expect(err).to.exist; |
There was a problem hiding this comment.
writeConcernMax was changed to return a writeConcern formatted the new way-- writeConcern: {w:1, ...}. Bulk execute takes an actual WriteConcern object as its first parameter (this was changed in master), so we have to un-wrap the writeConcernMax result here.
| return { w: 1 }; | ||
| return { writeConcern: { w: 1 } }; | ||
| } | ||
|
|
There was a problem hiding this comment.
Per @nbbeeken 's suggestion. The benefit of making this change should be about half as many warnings during testing.
HanaPearlman
force-pushed
the
NODE-1722/3.6/deprecate-wc-options
branch
from
5595c02 to
6977ed7
Compare
emadum
left a comment
emadum
left a comment
There was a problem hiding this comment.
LGTM 👍
One minor issue, Db.prototype.dropDatabase doesn't have writeConcern documented as an option but I think it does support it. While I don't think it needs to hold up this PR, we should probably go through and audit the Collection/Db objects to make sure all commands that support writeConcern have it listed in the jsdoc.
HanaPearlman
force-pushed
the
NODE-1722/3.6/deprecate-wc-options
branch
from
6977ed7 to
6cdceae
Compare
|
Hi everyone, it seems that the new format for writeConcern options is not used everywhere in the driver (I think at least in ./lib/utils.js#mergeOptionsAndWriteConcern). So even when we pass the new format, we still get the depreciation message over and over. |
4 participants
This PR is mostly documentation changes to indicate that keys
w,j,wtimeoutandfsyncare deprecated.Code changes include:
WriteConcern.fromOptionsnow warns when the options contain the top-level keys and not thewriteConcernkey.writeConcernkey, so that warnings are not logged later. Options passed directly to the client are also moved under thewriteConcernkey, but only afterWriteConcern.fromOptionsis called, ensuring at least one warning is logged.options.writeConcern, if it exists, when getting the write concern.Tests now spit out a ton of warns because the vast majority of tests which pass write concern options to client/operations use the top level keys.