fix: adds topology discovery for mongos #2557
Conversation
lib/core/uri_parser.js
Outdated
| @@ -687,6 +687,8 @@ function parseConnectionString(uri, options, callback) { | |||
| return callback(new MongoParseError('directConnection option requires exactly one host')); | |||
| } | |||
|
|
|||
| parsedOptions._directConnection = Boolean(parsedOptions.directConnection); | |||
There was a problem hiding this comment.
Open changing the naming here, we need a way to know if directConnect is defaulted to true or not.
lib/core/uri_parser.js
Outdated
| @@ -687,6 +687,8 @@ function parseConnectionString(uri, options, callback) { | |||
| return callback(new MongoParseError('directConnection option requires exactly one host')); | |||
| } | |||
|
|
|||
| parsedOptions._directConnection = Boolean(parsedOptions.directConnection); | |||
|
|
|||
There was a problem hiding this comment.
isn't the fix here to just remove the code below in the // NOTE section?
There was a problem hiding this comment.
My reasoning for not removing that code is, this issue was reported in 3.6, I thought we'd want to fix it there not issue a breaking change.
Since changing the starting topology can reasonably be considered a backwards-breaking change, existing drivers SHOULD stage implementation according to semantic versioning guidelines. Specifically, support for the directConnection URI option can be added in a minor release. In a subsequent major release, the default starting topology can be changed to Unknown. Drivers MUST document this in a prior minor release.
There was a problem hiding this comment.
It's a bug reported in 3.6 using an opt-in feature where we broke compatibility with the existing behavior, I don't think anybody is depending on the bug's behavior. We should just fix it in 3.6 now, and maintain the new behavior as-is in master
reggi
force-pushed
the
NODE-2827/toplology-sharded
branch
from
3d32864 to
50d6bd1
Compare
Co-authored-by: Eric Adum <[email protected]>
…-mongodb-native into NODE-2827/toplology-sharded
|
Had to update SDAM tests. |
| metadata: { requires: { topology: 'sharded' } }, | ||
| test: function() { | ||
| const client = this.configuration.newClient({}, { useUnifiedTopology: true }); | ||
| expect(client.s.options).to.have.property('useUnifiedTopology', true); |
There was a problem hiding this comment.
prefer not to observe private properties, I think it's satisfactory to have the check for the topology description type below.
Co-authored-by: Matt Broadstone <[email protected]>
@emadum I don't think it's useful to put in a change request that the tests are failing, because the pull will be blocked by that, I approve PR's all the time that have failing tests I think it's implied. Just a thought here, because now that the tests are passing I'm still blocked by your change request. |
That make sense, will do that going forward. |
NODE-2827