From 6ff1feb2b48c4e425afc8e5b777619f88ec59200 Mon Sep 17 00:00:00 2001 From: Kaitlin Mahar Date: Tue, 26 May 2020 22:30:15 -0400 Subject: [PATCH 1/8] Add DNS seedlist test cases --- .../tests/direct-connection-false.json | 14 ++++++++++++++ .../tests/direct-connection-true.json | 7 +++++++ 2 files changed, 21 insertions(+) create mode 100644 Tests/Specs/initial-dns-seedlist-discovery/tests/direct-connection-false.json create mode 100644 Tests/Specs/initial-dns-seedlist-discovery/tests/direct-connection-true.json diff --git a/Tests/Specs/initial-dns-seedlist-discovery/tests/direct-connection-false.json b/Tests/Specs/initial-dns-seedlist-discovery/tests/direct-connection-false.json new file mode 100644 index 000000000..01560d5ac --- /dev/null +++ b/Tests/Specs/initial-dns-seedlist-discovery/tests/direct-connection-false.json @@ -0,0 +1,14 @@ +{ + "uri": "mongodb+srv://test3.test.build.10gen.cc/?directConnection=false", + "seeds": [ + "localhost.test.build.10gen.cc:27017" + ], + "hosts": [ + "localhost:27017", + "localhost:27018", + "localhost:27019" + ], + "options": { + "ssl": true + } +} diff --git a/Tests/Specs/initial-dns-seedlist-discovery/tests/direct-connection-true.json b/Tests/Specs/initial-dns-seedlist-discovery/tests/direct-connection-true.json new file mode 100644 index 000000000..ace670010 --- /dev/null +++ b/Tests/Specs/initial-dns-seedlist-discovery/tests/direct-connection-true.json @@ -0,0 +1,7 @@ +{ + "uri": "mongodb+srv://test3.test.build.10gen.cc/?directConnection=true", + "seeds": [], + "hosts": [], + "error": true, + "comment": "Should fail because directConnection=true is incompatible with SRV URIs." +} From a2c46872a89165b1153b83f735d7facf61b2dccf Mon Sep 17 00:00:00 2001 From: Kaitlin Mahar Date: Wed, 27 May 2020 13:10:42 -0400 Subject: [PATCH 2/8] set directConnection to false if not specified --- Sources/MongoSwift/ConnectionString.swift | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/Sources/MongoSwift/ConnectionString.swift b/Sources/MongoSwift/ConnectionString.swift index 8caa8a8f4..cb80a816f 100644 --- a/Sources/MongoSwift/ConnectionString.swift +++ b/Sources/MongoSwift/ConnectionString.swift @@ -33,6 +33,12 @@ internal class ConnectionString { mongoc_uri_set_option_as_bool(self._uri, MONGOC_URI_RETRYREADS, rr) } + // Per SDAM spec: If the ``directConnection`` option is not specified, newly developed drivers MUST behave as + // if it was specified with the false value. + if !self.hasOption("directConnection") { + mongoc_uri_set_option_as_bool(self._uri, MONGOC_URI_DIRECTCONNECTION, false) + } + if let credential = options?.credential { try self.setMongoCredential(credential) } @@ -195,6 +201,10 @@ internal class ConnectionString { return hosts } + private func hasOption(_ option: String) -> Bool { + mongoc_uri_has_option(self._uri, option) + } + /// Executes the provided closure using a pointer to the underlying `mongoc_uri_t`. internal func withMongocURI(_ body: (OpaquePointer) throws -> T) rethrows -> T { try body(self._uri) From f3636a04c4260218adc409c0b7900d471b0af1f7 Mon Sep 17 00:00:00 2001 From: Kaitlin Mahar Date: Wed, 27 May 2020 20:57:39 -0400 Subject: [PATCH 3/8] update SDAM test --- Tests/BSONTests/DocumentTests.swift | 2 - .../SDAMMonitoringTests.swift | 62 ++++++++----------- 2 files changed, 27 insertions(+), 37 deletions(-) diff --git a/Tests/BSONTests/DocumentTests.swift b/Tests/BSONTests/DocumentTests.swift index 7ab4c78c5..4d6fdbd34 100644 --- a/Tests/BSONTests/DocumentTests.swift +++ b/Tests/BSONTests/DocumentTests.swift @@ -374,8 +374,6 @@ final class DocumentTests: MongoSwiftTestCase { // save a reference to original bson_t so we can verify it doesn't change let pointer = doc.pointerAddress - print("pointer is: \(pointer)") - // overwrite int32 with int32 doc["int32"] = .int32(15) expect(doc["int32"]).to(equal(.int32(15))) diff --git a/Tests/MongoSwiftSyncTests/SDAMMonitoringTests.swift b/Tests/MongoSwiftSyncTests/SDAMMonitoringTests.swift index 064c6be77..9d490ca6f 100644 --- a/Tests/MongoSwiftSyncTests/SDAMMonitoringTests.swift +++ b/Tests/MongoSwiftSyncTests/SDAMMonitoringTests.swift @@ -16,10 +16,6 @@ final class SDAMTests: MongoSwiftTestCase { expect(desc.passives).to(haveCount(0)) } - func checkUnknownServerType(_ desc: ServerDescription) { - expect(desc.type).to(equal(ServerDescription.ServerType.unknown)) - } - // Basic test based on the "standalone" spec test for SDAM monitoring: // swiftlint:disable line_length // https://github.com/mongodb/specifications/blob/master/source/server-discovery-and-monitoring/tests/monitoring/standalone.json @@ -51,52 +47,48 @@ final class SDAMTests: MongoSwiftTestCase { } let hostAddress = try ServerAddress(host) - // check event count and that events are of the expected types - expect(receivedEvents.count).to(beGreaterThanOrEqualTo(5)) + expect(receivedEvents.count).to(equal(4)) expect(receivedEvents[0].topologyOpeningValue).toNot(beNil()) - expect(receivedEvents[1].topologyDescriptionChangedValue).toNot(beNil()) - expect(receivedEvents[2].serverOpeningValue).toNot(beNil()) - expect(receivedEvents[3].serverDescriptionChangedValue).toNot(beNil()) - expect(receivedEvents[4].topologyDescriptionChangedValue).toNot(beNil()) + expect(receivedEvents[1].serverOpeningValue).toNot(beNil()) + expect(receivedEvents[2].serverDescriptionChangedValue).toNot(beNil()) + expect(receivedEvents[3].topologyDescriptionChangedValue).toNot(beNil()) - // verify that data in ServerDescription and TopologyDescription looks reasonable let event0 = receivedEvents[0].topologyOpeningValue! - expect(event0.topologyID).toNot(beNil()) - let event1 = receivedEvents[1].topologyDescriptionChangedValue! + let event1 = receivedEvents[1].serverOpeningValue! expect(event1.topologyID).to(equal(event0.topologyID)) - expect(event1.previousDescription.type).to(equal(TopologyDescription.TopologyType.unknown)) - expect(event1.newDescription.type).to(equal(TopologyDescription.TopologyType.single)) - // This is a bit of a deviation from the SDAM spec tests linked above. However, this is how mongoc responds so - // there is no other way to get around this. - expect(event1.newDescription.servers).to(beEmpty()) + expect(event1.serverAddress).to(equal(hostAddress)) - let event2 = receivedEvents[2].serverOpeningValue! + let event2 = receivedEvents[2].serverDescriptionChangedValue! expect(event2.topologyID).to(equal(event1.topologyID)) - expect(event2.serverAddress).to(equal(hostAddress)) - let event3 = receivedEvents[3].serverDescriptionChangedValue! - expect(event3.topologyID).to(equal(event2.topologyID)) - let prevServer = event3.previousDescription - expect(prevServer.address).to(equal(hostAddress)) - self.checkEmptyLists(prevServer) - self.checkUnknownServerType(prevServer) + let prevServer = event2.previousDescription + let newServer = event2.newDescription - let newServer = event3.newDescription + expect(prevServer.address).to(equal(hostAddress)) expect(newServer.address).to(equal(hostAddress)) + + self.checkEmptyLists(prevServer) self.checkEmptyLists(newServer) - expect(newServer.type).to(equal(ServerDescription.ServerType.standalone)) - let event4 = receivedEvents[4].topologyDescriptionChangedValue! - expect(event4.topologyID).to(equal(event3.topologyID)) - let prevTopology = event4.previousDescription - expect(prevTopology.type).to(equal(TopologyDescription.TopologyType.single)) + expect(prevServer.type).to(equal(.unknown)) + expect(newServer.type).to(equal(.standalone)) + + let event3 = receivedEvents[3].topologyDescriptionChangedValue! + expect(event3.topologyID).to(equal(event2.topologyID)) + + let prevTopology = event3.previousDescription + let newTopology = event3.newDescription + + expect(prevTopology.type).to(equal(.unknown)) + expect(newTopology.type).to(equal(.single)) + expect(prevTopology.servers).to(beEmpty()) + expect(newTopology.servers).to(haveCount(1)) - let newTopology = event4.newDescription - expect(newTopology.type).to(equal(TopologyDescription.TopologyType.single)) expect(newTopology.servers[0].address).to(equal(hostAddress)) - expect(newTopology.servers[0].type).to(equal(ServerDescription.ServerType.standalone)) + expect(newTopology.servers[0].type).to(equal(.standalone)) + self.checkEmptyLists(newTopology.servers[0]) } } From 1d6da0e7b9e6c00c2a5124b064ae9451c070a3d5 Mon Sep 17 00:00:00 2001 From: Kaitlin Mahar Date: Fri, 29 May 2020 13:12:30 -0400 Subject: [PATCH 4/8] add test with discovery behavior --- ...MMonitoringTests.swift => SDAMTests.swift} | 35 +++++++++++++++++++ 1 file changed, 35 insertions(+) rename Tests/MongoSwiftSyncTests/{SDAMMonitoringTests.swift => SDAMTests.swift} (79%) diff --git a/Tests/MongoSwiftSyncTests/SDAMMonitoringTests.swift b/Tests/MongoSwiftSyncTests/SDAMTests.swift similarity index 79% rename from Tests/MongoSwiftSyncTests/SDAMMonitoringTests.swift rename to Tests/MongoSwiftSyncTests/SDAMTests.swift index 9d490ca6f..76d94c63d 100644 --- a/Tests/MongoSwiftSyncTests/SDAMMonitoringTests.swift +++ b/Tests/MongoSwiftSyncTests/SDAMTests.swift @@ -91,6 +91,41 @@ final class SDAMTests: MongoSwiftTestCase { self.checkEmptyLists(newTopology.servers[0]) } + + func testInitialReplicaSetDiscovery() throws { + guard MongoSwiftTestCase.topologyType == .replicaSetWithPrimary else { + print(unsupportedTopologyMessage(testName: self.name)) + return + } + + let hostURIs = Self.getConnectionStringPerHost() + // separately connect to each host and verify we are able to perform a write, + // meaning that the primary was successfully discovered + for uri in hostURIs { + let client = try MongoClient.makeTestClient(uri) + try withTestNamespace(client: client) { _, collection in + expect(try collection.insertOne(["x": 1])).toNot(throwError()) + } + } + + // separately connect to each host with directConnection=true and verify 2/3 write + // attempts fail as the primary will not be discovered + var failures = 0 + for var uri in hostURIs { + uri += "&directConnection=true" + let client = try MongoClient.makeTestClient(uri) + do { + _ = try withTestNamespace(client: client) { _, collection in + try collection.insertOne(["x": 1]) + } + } catch { + expect(error).to(beAnInstanceOf(CommandError.self)) + failures += 1 + } + } + + expect(failures).to(equal(2), description: "Write should fail when directly connected to secondaries") + } } /// SDAM monitoring event handler that behaves similarly to the `TestCommandMonitor` From 431ac3ef77881a6e8467e78f5efd21f0d5a9422c Mon Sep 17 00:00:00 2001 From: Kaitlin Mahar Date: Fri, 29 May 2020 16:13:57 -0400 Subject: [PATCH 5/8] add client option and more tests --- Sources/MongoSwift/ConnectionString.swift | 4 ++- Sources/MongoSwift/MongoClient.swift | 8 ++++++ Tests/MongoSwiftSyncTests/SDAMTests.swift | 35 ++++++++++++++++------- 3 files changed, 36 insertions(+), 11 deletions(-) diff --git a/Sources/MongoSwift/ConnectionString.swift b/Sources/MongoSwift/ConnectionString.swift index cb80a816f..36400d455 100644 --- a/Sources/MongoSwift/ConnectionString.swift +++ b/Sources/MongoSwift/ConnectionString.swift @@ -35,7 +35,9 @@ internal class ConnectionString { // Per SDAM spec: If the ``directConnection`` option is not specified, newly developed drivers MUST behave as // if it was specified with the false value. - if !self.hasOption("directConnection") { + if let dc = options?.directConnection { + mongoc_uri_set_option_as_bool(self._uri, MONGOC_URI_DIRECTCONNECTION, dc) + } else if !self.hasOption("directConnection") { mongoc_uri_set_option_as_bool(self._uri, MONGOC_URI_DIRECTCONNECTION, false) } diff --git a/Sources/MongoSwift/MongoClient.swift b/Sources/MongoSwift/MongoClient.swift index a7e4a7b25..2848ea5d2 100644 --- a/Sources/MongoSwift/MongoClient.swift +++ b/Sources/MongoSwift/MongoClient.swift @@ -16,6 +16,12 @@ public struct MongoClientOptions: CodingStrategyProvider { /// databases or collections that derive from it. public var dateCodingStrategy: DateCodingStrategy? + /// When true, the client will connect directly to a single host. When false, the client will attempt to + /// automatically discover all replica set members if a replica set name is provided. + /// It is an error to set this option to `true` when used with a mongodb+srv connection string or when multiple + /// hosts are specified in the connection string. + public var directConnection: Bool? + /// The maximum number of connections that may be associated with a connection pool created by this client at a /// given time. This includes in-use and available connections. Defaults to 100. public var maxPoolSize: Int? @@ -83,6 +89,7 @@ public struct MongoClientOptions: CodingStrategyProvider { credential: MongoCredential? = nil, dataCodingStrategy: DataCodingStrategy? = nil, dateCodingStrategy: DateCodingStrategy? = nil, + directConnection: Bool? = nil, maxPoolSize: Int? = nil, readConcern: ReadConcern? = nil, readPreference: ReadPreference? = nil, @@ -101,6 +108,7 @@ public struct MongoClientOptions: CodingStrategyProvider { self.credential = credential self.dataCodingStrategy = dataCodingStrategy self.dateCodingStrategy = dateCodingStrategy + self.directConnection = directConnection self.maxPoolSize = maxPoolSize self.readConcern = readConcern self.readPreference = readPreference diff --git a/Tests/MongoSwiftSyncTests/SDAMTests.swift b/Tests/MongoSwiftSyncTests/SDAMTests.swift index 76d94c63d..89f3a7781 100644 --- a/Tests/MongoSwiftSyncTests/SDAMTests.swift +++ b/Tests/MongoSwiftSyncTests/SDAMTests.swift @@ -99,21 +99,33 @@ final class SDAMTests: MongoSwiftTestCase { } let hostURIs = Self.getConnectionStringPerHost() - // separately connect to each host and verify we are able to perform a write, - // meaning that the primary was successfully discovered - for uri in hostURIs { - let client = try MongoClient.makeTestClient(uri) + + let optsFalse = MongoClientOptions(directConnection: false) + let optsTrue = MongoClientOptions(directConnection: true) + + // We should succeed in discovering the primary in all of these cases: + let testClientsShouldSucceed = try + hostURIs.map { try MongoClient.makeTestClient($0) } + // option unspecified + hostURIs.map { try MongoClient.makeTestClient("\($0)&directConnection=false") } + // false in URI + hostURIs.map { try MongoClient.makeTestClient($0, options: optsFalse) } // false in options struct + + // separately connect to each host and verify we are able to perform a write, meaning + // that the primary is successfully discovered no matter which host we start with + for client in testClientsShouldSucceed { try withTestNamespace(client: client) { _, collection in expect(try collection.insertOne(["x": 1])).toNot(throwError()) } } - // separately connect to each host with directConnection=true and verify 2/3 write - // attempts fail as the primary will not be discovered + let testClientsShouldMostlyFail = try + hostURIs.map { try MongoClient.makeTestClient("\($0)&directConnection=true") } + // true in URI + hostURIs.map { try MongoClient.makeTestClient($0, options: optsTrue) } // true in options struct + + // 4 of 6 attempts to perform writes should fail assuming these are 3-node replica sets, since in 2 cases we + // will directly connect to the primary, and in the other 4 we will directly connect to a secondary. + var failures = 0 - for var uri in hostURIs { - uri += "&directConnection=true" - let client = try MongoClient.makeTestClient(uri) + for client in testClientsShouldMostlyFail { do { _ = try withTestNamespace(client: client) { _, collection in try collection.insertOne(["x": 1]) @@ -124,7 +136,10 @@ final class SDAMTests: MongoSwiftTestCase { } } - expect(failures).to(equal(2), description: "Write should fail when directly connected to secondaries") + expect(failures).to( + equal(4), + description: "Writes should fail when connecting to secondaries with directConnection=true" + ) } } From e2063b3dbaa956a9a494c67d2b787b2b58576295 Mon Sep 17 00:00:00 2001 From: Kaitlin Mahar Date: Fri, 29 May 2020 16:19:50 -0400 Subject: [PATCH 6/8] Update LinuxMain.swift --- Tests/LinuxMain.swift | 1 + 1 file changed, 1 insertion(+) diff --git a/Tests/LinuxMain.swift b/Tests/LinuxMain.swift index fb9d3bb9b..2281341d2 100644 --- a/Tests/LinuxMain.swift +++ b/Tests/LinuxMain.swift @@ -319,6 +319,7 @@ extension RetryableWritesTests { extension SDAMTests { static var allTests = [ ("testMonitoring", testMonitoring), + ("testInitialReplicaSetDiscovery", testInitialReplicaSetDiscovery), ] } From 666db9520c58aa65df4000bbed2514dbf3efd049 Mon Sep 17 00:00:00 2001 From: Kaitlin Mahar Date: Fri, 29 May 2020 17:53:47 -0400 Subject: [PATCH 7/8] update error type --- Tests/MongoSwiftSyncTests/SDAMTests.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Tests/MongoSwiftSyncTests/SDAMTests.swift b/Tests/MongoSwiftSyncTests/SDAMTests.swift index 89f3a7781..f67e8d78a 100644 --- a/Tests/MongoSwiftSyncTests/SDAMTests.swift +++ b/Tests/MongoSwiftSyncTests/SDAMTests.swift @@ -131,7 +131,7 @@ final class SDAMTests: MongoSwiftTestCase { try collection.insertOne(["x": 1]) } } catch { - expect(error).to(beAnInstanceOf(CommandError.self)) + expect(error).to(beAnInstanceOf(MongoError.CommandError.self)) failures += 1 } } From 1e4cfaa2e1488623f0d154308422f1162658d163 Mon Sep 17 00:00:00 2001 From: Kaitlin Mahar Date: Fri, 29 May 2020 17:54:12 -0400 Subject: [PATCH 8/8] add default behavior --- Sources/MongoSwift/MongoClient.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Sources/MongoSwift/MongoClient.swift b/Sources/MongoSwift/MongoClient.swift index 2848ea5d2..096446e60 100644 --- a/Sources/MongoSwift/MongoClient.swift +++ b/Sources/MongoSwift/MongoClient.swift @@ -17,7 +17,7 @@ public struct MongoClientOptions: CodingStrategyProvider { public var dateCodingStrategy: DateCodingStrategy? /// When true, the client will connect directly to a single host. When false, the client will attempt to - /// automatically discover all replica set members if a replica set name is provided. + /// automatically discover all replica set members if a replica set name is provided. Defaults to false. /// It is an error to set this option to `true` when used with a mongodb+srv connection string or when multiple /// hosts are specified in the connection string. public var directConnection: Bool?