PYTHON-5306 - Fix use of public MongoClient attributes before connection #2285

NoahStapp · 2025-04-15T20:37:37Z

The public attributes of MongoClient are topology_description, nodes, address, primary, secondaries, and arbiters. Of these, only topology_description and nodes do not already perform network I/O. The rest can safely ensure connection as part of their execution without modifying their previous behavior.

NoahStapp · 2025-04-15T20:38:16Z

pymongo/asynchronous/mongo_client.py

@@ -1205,6 +1206,18 @@ def topology_description(self) -> TopologyDescription:

        .. versionadded:: 4.0
        """
+        if self._topology is None:


If the client has not connected yet, return a TopologyDescription with placeholder servers derived from the seeds passed to the client's constructor.

We should make sure the returned TopologyDescription has the same _topology_id. Otherwise it could be confusing because _topology_id will change.

Also can you give an example of what topology_description looks like in the connect=False srv case?

<TopologyDescription id: 67ffb68a6e12ca176991f521, topology_type: Unknown, servers: [<ServerDescription (('test1.test.build.10gen.cc', None), 27017) server_type: Unknown, rtt: None>]>

Please include an example of this new/old behavior in the Jira ticket when closing.

NoahStapp · 2025-04-15T20:39:08Z

pymongo/asynchronous/mongo_client.py

@@ -1218,6 +1231,8 @@ def nodes(self) -> FrozenSet[_Address]:
          to any servers, or a network partition causes it to lose connection
          to all servers.
        """
+        if self._topology is None:


As the docstring defines, we expect nodes to return an empty set if the client has not yet connected.

ShaneHarvey · 2025-04-15T20:44:16Z

pymongo/asynchronous/mongo_client.py

@@ -1205,6 +1206,18 @@ def topology_description(self) -> TopologyDescription:

        .. versionadded:: 4.0
        """
+        if self._topology is None:


We should make sure the returned TopologyDescription has the same _topology_id. Otherwise it could be confusing because _topology_id will change.

ShaneHarvey · 2025-04-15T20:53:14Z

test/asynchronous/test_client.py

+            {("unknown", None): ServerDescription(("unknown", None))},
+        )
+        await client.aconnect()
+        self.assertEqual(await client.address, None)


Can we add a tests for address and all the helper helpers without calling aconnect?

This test was mistakenly included: our test SRV URIs aren't real servers, so we can't actually connect to them.

This is actually a larger restriction: the new test needs to be run against a real SRV server, or have accurate responses mocked out, to ensure correct behavior.

For all non-SRV URIs, we immediately create a Topology, making this test on such connections useless.

We do have real SRV addresses which resolve to localhost:

>>> client = MongoClient("mongodb+srv://test1.test.build.10gen.cc", tlsInsecure=True) >>> client.topology_description <TopologyDescription id: 68000f362bab7f34cc9902c2, topology_type: ReplicaSetWithPrimary, servers: [<ServerDescription ('localhost', 27017) server_type: RSPrimary, rtt: 0.001482416999351699>]>

Ah, I forgot that SRV implies replica + SSL, thanks!

ShaneHarvey · 2025-04-15T20:53:54Z

test/asynchronous/test_client.py

+        # arbiters causes client to block until connected
+        await c.arbiters
+        self.assertIsNotNone(c._topology)
+        c = await self.async_rs_or_single_client(connect=False)


In this kind of test it's a good idea to close each client once it's no longer needed to avoid building up unclosed clients.

async_rs_or_single_client already automatically closes all clients at the end of the method.

I know but I'm saying it's better to close them explicitly to avoid building up unclosed clients.

We don't currently use that pattern in our testcases, relying on the helper client creation methods to clean up clients for us. If it's better to avoid relying on those, should we rethink our approach?

I'm saying for tests that specifically open multiple/many clients, it's good practice to add an explicit call to close().

Not trying to pushback stubbornly here, just want to keep our codebase consistent where possible since we don't currently explicitly close clients created with the helpers. Do we have a measurable improvement or benefit from explicitly closing clients that will be cleaned up by the test?

It's not strictly a performance issue. It's also about easier debugging. Leaving open clients around unnecessarily like this makes debugging harder because each client has many threads/tasks causing noise.

Ah, makes sense! I'll open a ticket to make sure we follow the same pattern elsewhere too.

ShaneHarvey · 2025-04-15T20:54:49Z

pymongo/asynchronous/mongo_client.py

@@ -1205,6 +1206,18 @@ def topology_description(self) -> TopologyDescription:

        .. versionadded:: 4.0
        """
+        if self._topology is None:


Also can you give an example of what topology_description looks like in the connect=False srv case?

ShaneHarvey · 2025-04-16T21:37:18Z

pymongo/synchronous/mongo_client.py

+                None,
+                None,
+                None,
+                TopologySettings(),


This is missing certain settings that are publicly accessible, like TopologyDescription.heartbeat_frequency.

In the ticket I suggested making the MongoClient constructor always create the Topology instance, and then mutate it later once the SRV info is resolved. Did you consider that approach? It would avoid the problem of attempting to mock out these attributes and avoid the type-ignores since everything would always be non-None.

(It could be that this approach is a better fit, I just want to know if an alternative was considered.)

I started with the approach you suggested, but switched to the current implementation for a few reasons:

topology_description is the only public attribute that requires changes of meaningful size. The rest either already do IO or are nodes, which has a trivial workaround.

We don't currently mutate Topology instances or TopologySettings instances directly. To do so, we'd need to make more code changes and be confident that we haven't overlooked any problematic edge cases.

I foresee more potential bugs/confusion caused by introducing a new state specifically for SRV Topologies where we know that the Topology we currently have is not actually correct, and will be mutated upon SRV resolution.

This approach has fewer changes that could cause errors, especially since it isolates changes to resolving the specific issue at hand.

Thanks, could you update TopologySettings() here to be more accurate then?

TopologySettings sets defaults for each attribute, is passing no arguments to it invalid?

Like I said above, client.topology_description.heartbeat_frequency is one example of an attribute we need to set here to avoid heartbeat_frequency appearing to change from one value to another. There may be others.

For example:

>>> client = MongoClient(..., connect=False, heartbeatFrequencyMS=99999) >>> client.topology_description.heartbeat_frequency 10 >>> client.admin.command("ping") ... >>> client.topology_description.heartbeat_frequency 99.999

Sorry, I misunderstood what you meant. The example makes it clear, thanks!

Now we always initialize a TopologySettings in the constructor. If SRV is enabled, we create a new one after resolution but keep the existing topology id.

ShaneHarvey · 2025-04-16T21:45:41Z

test/asynchronous/test_client.py

@@ -849,6 +849,54 @@ async def test_init_disconnected_with_auth(self):
        with self.assertRaises(ConnectionFailure):
            await c.pymongo_test.test.find_one()

+    @async_client_context.require_replica_set


I believe we can remove require_replica_set.

Removing it causes failures on any non-replica topology: https://spruce.mongodb.com/task/mongo_python_driver_mongodb_latest_test_python3.9_auth_ssl_standalone_cov_patch_846b1fc25ced5979ade934bb27d6ffbb60f68267_68010865cc419c000710589f_25_04_17_13_56_05?execution=0&sortBy=STATUS&sortDir=ASC

I assume this is due to SRV requiring SSL + replica to function.

Oh I see. "mongodb+srv://test1.test.build.10gen.cc" returns 2 hosts so it can't connect to standalone. It can connect to Mongos though so we should run this test on both replica set and sharded.

require_no_standalone, then?

Load balancers probably dont work here so it would need to be @require_no_standalone+@require_no_load_balancer+@require_no_load_balancer

ShaneHarvey · 2025-04-16T21:51:21Z

test/asynchronous/test_client.py

+        # arbiters causes client to block until connected
+        await c.arbiters
+        self.assertIsNotNone(c._topology)
+        c = await self.async_rs_or_single_client(connect=False)


I'm saying for tests that specifically open multiple/many clients, it's good practice to add an explicit call to close().

ShaneHarvey · 2025-04-16T21:53:49Z

pymongo/asynchronous/mongo_client.py

@@ -767,6 +768,7 @@ def __init__(
        self._timeout: float | None = None
        self._topology_settings: TopologySettings = None  # type: ignore[assignment]
        self._event_listeners: _EventListeners | None = None
+        self._initial_topology_id: Optional[ObjectId] = None  # type: ignore[assignment]


Could we remove the type ignore here?

ShaneHarvey · 2025-04-17T21:02:39Z

pymongo/asynchronous/mongo_client.py

@@ -1205,6 +1206,18 @@ def topology_description(self) -> TopologyDescription:

        .. versionadded:: 4.0
        """
+        if self._topology is None:


Please include an example of this new/old behavior in the Jira ticket when closing.

ShaneHarvey · 2025-04-17T21:03:21Z

pymongo/synchronous/mongo_client.py

+                None,
+                None,
+                None,
+                TopologySettings(),


Thanks, could you update TopologySettings() here to be more accurate then?

ShaneHarvey

Let's add a changelog entry and call it a day!

ShaneHarvey · 2025-04-21T17:51:55Z

doc/changelog.rst

@@ -10,6 +10,7 @@ Version 4.12.1 is a bug fix release.
 - Fixed a bug that could raise ``UnboundLocalError`` when creating asynchronous connections over SSL.
 - Fixed a bug causing SRV hostname validation to fail when resolver and resolved hostnames are identical with three domain levels.
 - Fixed a bug that caused direct use of ``pymongo.uri_parser`` to raise an ``AttributeError``.
+- Fixed a bug that could cause public ``pymongo.MongoClient`` and ``pymongo.AsyncMongoClient`` attributes to raise errors when accessed before a connection was established.


Let's describe it a bit more specifically, something like:

Fixed a bug where clients created with connect=False and a "mongodb+srv://" connection string could cause public ``pymongo.MongoClient`` and ``pymongo.AsyncMongoClient`` attributes (topology_description, nodes, address, primary, secondaries, arbiters) to incorrectly return a Database, leading to type errors such as: "NotImplementedError: Database objects do not implement truth value testing or bool()".

ShaneHarvey · 2025-04-21T19:43:01Z

doc/changelog.rst

+- Fixed a bug where clients created with connect=False and a "mongodb+srv://" connection string
+  could cause public ``pymongo.MongoClient`` and ``pymongo.AsyncMongoClient`` attributes (topology_description,
+  nodes, address, primary, secondaries, arbiters) to incorrectly return a Database, leading to type
+  errors such as: "NotImplementedError: Database objects do not implement truth value testing or bool().


Missing trailing " for "NotImplementedError.

PYTHON-5306 - Fix use of public MongoClient attributes before connection

aa86a3a

NoahStapp requested review from ShaneHarvey and sleepyStick April 15, 2025 20:37

NoahStapp commented Apr 15, 2025

View reviewed changes

ShaneHarvey requested changes Apr 15, 2025

View reviewed changes

Address review

c185c95

NoahStapp requested a review from ShaneHarvey April 16, 2025 14:20

NoahStapp added 3 commits April 16, 2025 10:20

Merge branch 'master' into PYTHON-5306

763e78c

Fix topology_description

66b7e8d

Fix SRV test

5f7f84a

ShaneHarvey requested changes Apr 16, 2025

View reviewed changes

Remove type: ignore

ed3dea5

NoahStapp requested a review from ShaneHarvey April 17, 2025 14:19

ShaneHarvey requested changes Apr 17, 2025

View reviewed changes

Address review

99802be

NoahStapp requested a review from ShaneHarvey April 18, 2025 14:33

Create TopologySettings always

14bb119

ShaneHarvey previously approved these changes Apr 18, 2025

View reviewed changes

Update changelog

c1e6684

NoahStapp dismissed ShaneHarvey’s stale review via c1e6684 April 21, 2025 12:43

NoahStapp requested a review from ShaneHarvey April 21, 2025 12:43

Merge branch 'master' into PYTHON-5306

2a63a3b

sleepyStick previously approved these changes Apr 21, 2025

View reviewed changes

ShaneHarvey reviewed Apr 21, 2025

View reviewed changes

Update changelog

835e560

NoahStapp dismissed sleepyStick’s stale review via 835e560 April 21, 2025 18:25

NoahStapp requested a review from ShaneHarvey April 21, 2025 18:26

ShaneHarvey requested changes Apr 21, 2025

View reviewed changes

Trailing quote

e5e6dc8

NoahStapp requested a review from ShaneHarvey April 21, 2025 19:47

ShaneHarvey approved these changes Apr 21, 2025

View reviewed changes

NoahStapp merged commit 412d000 into mongodb:master Apr 21, 2025
29 of 30 checks passed

PYTHON-5306 - Fix use of public MongoClient attributes before connection #2285

PYTHON-5306 - Fix use of public MongoClient attributes before connection #2285

Conversation

NoahStapp commented Apr 15, 2025 • edited Loading

NoahStapp Apr 15, 2025 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NoahStapp Apr 16, 2025 • edited Loading

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

NoahStapp Apr 16, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NoahStapp Apr 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NoahStapp Apr 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NoahStapp Apr 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NoahStapp Apr 18, 2025 • edited Loading

Choose a reason for hiding this comment

ShaneHarvey Apr 18, 2025 • edited Loading

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

NoahStapp Apr 17, 2025 • edited Loading

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

Choose a reason for hiding this comment

ShaneHarvey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NoahStapp commented Apr 15, 2025 •

edited

Loading

NoahStapp Apr 15, 2025 •

edited

Loading

NoahStapp Apr 16, 2025 •

edited

Loading

NoahStapp Apr 16, 2025 •

edited

Loading

NoahStapp Apr 17, 2025 •

edited

Loading

NoahStapp Apr 17, 2025 •

edited

Loading

NoahStapp Apr 17, 2025 •

edited

Loading

NoahStapp Apr 18, 2025 •

edited

Loading

ShaneHarvey Apr 18, 2025 •

edited

Loading

NoahStapp Apr 17, 2025 •

edited

Loading