Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 12 commits into from
Apr 21, 2025
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
aa86a3a
PYTHON-5306 - Fix use of public MongoClient attributes before connection
NoahStapp Apr 15, 2025
c185c95
Address review
NoahStapp Apr 16, 2025
763e78c
Merge branch 'master' into PYTHON-5306
NoahStapp Apr 16, 2025
66b7e8d
Fix topology_description
NoahStapp Apr 16, 2025
5f7f84a
Fix SRV test
NoahStapp Apr 16, 2025
ed3dea5
Remove type: ignore
NoahStapp Apr 17, 2025
99802be
Address review
NoahStapp Apr 18, 2025
14bb119
Create TopologySettings always
NoahStapp Apr 18, 2025
c1e6684
Update changelog
NoahStapp Apr 21, 2025
2a63a3b
Merge branch 'master' into PYTHON-5306
NoahStapp Apr 21, 2025
835e560
Update changelog
NoahStapp Apr 21, 2025
e5e6dc8
Trailing quote
NoahStapp Apr 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 31 additions & 8 deletions pymongo/asynchronous/mongo_client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@
)
from pymongo.read_preferences import ReadPreference, _ServerMode
from pymongo.results import ClientBulkWriteResult
from pymongo.server_description import ServerDescription
from pymongo.server_selectors import writable_server_selector
from pymongo.server_type import SERVER_TYPE
from pymongo.topology_description import TOPOLOGY_TYPE, TopologyDescription
Expand Down Expand Up @@ -779,7 +780,7 @@ def __init__(
keyword_opts["document_class"] = doc_class
self._resolve_srv_info: dict[str, Any] = {"keyword_opts": keyword_opts}

seeds = set()
self._seeds = set()
is_srv = False
username = None
password = None
Expand All @@ -804,18 +805,18 @@ def __init__(
srv_max_hosts=srv_max_hosts,
)
is_srv = entity.startswith(SRV_SCHEME)
seeds.update(res["nodelist"])
self._seeds.update(res["nodelist"])
username = res["username"] or username
password = res["password"] or password
dbase = res["database"] or dbase
opts = res["options"]
fqdn = res["fqdn"]
else:
seeds.update(split_hosts(entity, self._port))
if not seeds:
self._seeds.update(split_hosts(entity, self._port))
if not self._seeds:
raise ConfigurationError("need to specify at least one host")

for hostname in [node[0] for node in seeds]:
for hostname in [node[0] for node in self._seeds]:
if _detect_external_db(hostname):
break

Expand All @@ -838,7 +839,7 @@ def __init__(
srv_service_name = opts.get("srvServiceName", common.SRV_SERVICE_NAME)

srv_max_hosts = srv_max_hosts or opts.get("srvmaxhosts")
opts = self._normalize_and_validate_options(opts, seeds)
opts = self._normalize_and_validate_options(opts, self._seeds)

# Username and password passed as kwargs override user info in URI.
username = opts.get("username", username)
Expand All @@ -857,7 +858,7 @@ def __init__(
"username": username,
"password": password,
"dbase": dbase,
"seeds": seeds,
"seeds": self._seeds,
"fqdn": fqdn,
"srv_service_name": srv_service_name,
"pool_class": pool_class,
Expand All @@ -874,7 +875,7 @@ def __init__(
)

if not is_srv:
self._init_based_on_options(seeds, srv_max_hosts, srv_service_name)
self._init_based_on_options(self._seeds, srv_max_hosts, srv_service_name)

self._opened = False
self._closed = False
Expand Down Expand Up @@ -1205,6 +1206,18 @@ def topology_description(self) -> TopologyDescription:

.. versionadded:: 4.0
"""
if self._topology is None:
Copy link
Contributor Author

@NoahStapp NoahStapp Apr 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

servers = {
(host, self._port): ServerDescription((host, self._port)) for host in self._seeds
}
return TopologyDescription(
TOPOLOGY_TYPE.Unknown,
servers,
None,
None,
None,
TopologySettings(),
)
return self._topology.description

@property
Expand All @@ -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:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return frozenset()
description = self._topology.description
return frozenset(s.address for s in description.known_servers)

Expand Down Expand Up @@ -1576,6 +1591,8 @@ async def address(self) -> Optional[tuple[str, int]]:

.. versionadded:: 3.0
"""
if self._topology is None:
await self._get_topology()
topology_type = self._topology._description.topology_type
if (
topology_type == TOPOLOGY_TYPE.Sharded
Expand All @@ -1598,6 +1615,8 @@ async def primary(self) -> Optional[tuple[str, int]]:
.. versionadded:: 3.0
AsyncMongoClient gained this property in version 3.0.
"""
if self._topology is None:
await self._get_topology()
return await self._topology.get_primary() # type: ignore[return-value]

@property
Expand All @@ -1611,6 +1630,8 @@ async def secondaries(self) -> set[_Address]:
.. versionadded:: 3.0
AsyncMongoClient gained this property in version 3.0.
"""
if self._topology is None:
await self._get_topology()
return await self._topology.get_secondaries()

@property
Expand All @@ -1621,6 +1642,8 @@ async def arbiters(self) -> set[_Address]:
connected to a replica set, there are no arbiters, or this client was
created without the `replicaSet` option.
"""
if self._topology is None:
await self._get_topology()
return await self._topology.get_arbiters()

@property
Expand Down
39 changes: 31 additions & 8 deletions pymongo/synchronous/mongo_client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@
)
from pymongo.read_preferences import ReadPreference, _ServerMode
from pymongo.results import ClientBulkWriteResult
from pymongo.server_description import ServerDescription
from pymongo.server_selectors import writable_server_selector
from pymongo.server_type import SERVER_TYPE
from pymongo.synchronous import client_session, database, uri_parser
Expand Down Expand Up @@ -777,7 +778,7 @@ def __init__(
keyword_opts["document_class"] = doc_class
self._resolve_srv_info: dict[str, Any] = {"keyword_opts": keyword_opts}

seeds = set()
self._seeds = set()
is_srv = False
username = None
password = None
Expand All @@ -802,18 +803,18 @@ def __init__(
srv_max_hosts=srv_max_hosts,
)
is_srv = entity.startswith(SRV_SCHEME)
seeds.update(res["nodelist"])
self._seeds.update(res["nodelist"])
username = res["username"] or username
password = res["password"] or password
dbase = res["database"] or dbase
opts = res["options"]
fqdn = res["fqdn"]
else:
seeds.update(split_hosts(entity, self._port))
if not seeds:
self._seeds.update(split_hosts(entity, self._port))
if not self._seeds:
raise ConfigurationError("need to specify at least one host")

for hostname in [node[0] for node in seeds]:
for hostname in [node[0] for node in self._seeds]:
if _detect_external_db(hostname):
break

Expand All @@ -836,7 +837,7 @@ def __init__(
srv_service_name = opts.get("srvServiceName", common.SRV_SERVICE_NAME)

srv_max_hosts = srv_max_hosts or opts.get("srvmaxhosts")
opts = self._normalize_and_validate_options(opts, seeds)
opts = self._normalize_and_validate_options(opts, self._seeds)

# Username and password passed as kwargs override user info in URI.
username = opts.get("username", username)
Expand All @@ -855,7 +856,7 @@ def __init__(
"username": username,
"password": password,
"dbase": dbase,
"seeds": seeds,
"seeds": self._seeds,
"fqdn": fqdn,
"srv_service_name": srv_service_name,
"pool_class": pool_class,
Expand All @@ -872,7 +873,7 @@ def __init__(
)

if not is_srv:
self._init_based_on_options(seeds, srv_max_hosts, srv_service_name)
self._init_based_on_options(self._seeds, srv_max_hosts, srv_service_name)

self._opened = False
self._closed = False
Expand Down Expand Up @@ -1203,6 +1204,18 @@ def topology_description(self) -> TopologyDescription:

.. versionadded:: 4.0
"""
if self._topology is None:
servers = {
(host, self._port): ServerDescription((host, self._port)) for host in self._seeds
}
return TopologyDescription(
TOPOLOGY_TYPE.Unknown,
servers,
None,
None,
None,
TopologySettings(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Contributor Author

@NoahStapp NoahStapp Apr 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. 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.
  2. 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.
  3. 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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@NoahStapp NoahStapp Apr 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@ShaneHarvey ShaneHarvey Apr 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

)
return self._topology.description

@property
Expand All @@ -1216,6 +1229,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:
return frozenset()
description = self._topology.description
return frozenset(s.address for s in description.known_servers)

Expand Down Expand Up @@ -1570,6 +1585,8 @@ def address(self) -> Optional[tuple[str, int]]:

.. versionadded:: 3.0
"""
if self._topology is None:
self._get_topology()
topology_type = self._topology._description.topology_type
if (
topology_type == TOPOLOGY_TYPE.Sharded
Expand All @@ -1592,6 +1609,8 @@ def primary(self) -> Optional[tuple[str, int]]:
.. versionadded:: 3.0
MongoClient gained this property in version 3.0.
"""
if self._topology is None:
self._get_topology()
return self._topology.get_primary() # type: ignore[return-value]

@property
Expand All @@ -1605,6 +1624,8 @@ def secondaries(self) -> set[_Address]:
.. versionadded:: 3.0
MongoClient gained this property in version 3.0.
"""
if self._topology is None:
self._get_topology()
return self._topology.get_secondaries()

@property
Expand All @@ -1615,6 +1636,8 @@ def arbiters(self) -> set[_Address]:
connected to a replica set, there are no arbiters, or this client was
created without the `replicaSet` option.
"""
if self._topology is None:
self._get_topology()
return self._topology.get_arbiters()

@property
Expand Down
36 changes: 36 additions & 0 deletions test/asynchronous/test_client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -816,6 +816,30 @@ async def test_constants(self):
async def test_init_disconnected(self):
host, port = await async_client_context.host, await async_client_context.port
c = await self.async_rs_or_single_client(connect=False)
# nodes returns an empty set if not connected
self.assertEqual(c.nodes, frozenset())
# topology_description returns the initial seed description if not connected
topology_description = c.topology_description
self.assertEqual(topology_description.topology_type, TOPOLOGY_TYPE.Unknown)
self.assertEqual(
topology_description.server_descriptions(),
{(host, port): ServerDescription((host, port))},
)
# address causes client to block until connected
self.assertIsNotNone(await c.address)
c = await self.async_rs_or_single_client(connect=False)
# primary causes client to block until connected
await c.primary
self.assertIsNotNone(c._topology)
c = await self.async_rs_or_single_client(connect=False)
# secondaries causes client to block until connected
await c.secondaries
self.assertIsNotNone(c._topology)
c = await self.async_rs_or_single_client(connect=False)
# arbiters causes client to block until connected
await c.arbiters
self.assertIsNotNone(c._topology)
c = await self.async_rs_or_single_client(connect=False)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@NoahStapp NoahStapp Apr 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@NoahStapp NoahStapp Apr 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@NoahStapp NoahStapp Apr 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

# is_primary causes client to block until connected
self.assertIsInstance(await c.is_primary, bool)
c = await self.async_rs_or_single_client(connect=False)
Expand Down Expand Up @@ -2170,6 +2194,18 @@ async def test_uuid_queries(self):
self.assertEqual(2, len(docs))
await coll.drop()

async def test_unconnected_client_properties_with_srv(self):
client = self.simple_client("mongodb+srv://test1.test.build.10gen.cc/", connect=False)
self.assertEqual(client.nodes, frozenset())
topology_description = client.topology_description
self.assertEqual(topology_description.topology_type, TOPOLOGY_TYPE.Unknown)
self.assertEqual(
topology_description.server_descriptions(),
{("unknown", None): ServerDescription(("unknown", None))},
)
await client.aconnect()
self.assertEqual(await client.address, None)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@NoahStapp NoahStapp Apr 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>]>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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



class TestExhaustCursor(AsyncIntegrationTest):
"""Test that clients properly handle errors from exhaust cursors."""
Expand Down
36 changes: 36 additions & 0 deletions test/test_client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -791,6 +791,30 @@ def test_constants(self):
def test_init_disconnected(self):
host, port = client_context.host, client_context.port
c = self.rs_or_single_client(connect=False)
# nodes returns an empty set if not connected
self.assertEqual(c.nodes, frozenset())
# topology_description returns the initial seed description if not connected
topology_description = c.topology_description
self.assertEqual(topology_description.topology_type, TOPOLOGY_TYPE.Unknown)
self.assertEqual(
topology_description.server_descriptions(),
{(host, port): ServerDescription((host, port))},
)
# address causes client to block until connected
self.assertIsNotNone(c.address)
c = self.rs_or_single_client(connect=False)
# primary causes client to block until connected
c.primary
self.assertIsNotNone(c._topology)
c = self.rs_or_single_client(connect=False)
# secondaries causes client to block until connected
c.secondaries
self.assertIsNotNone(c._topology)
c = self.rs_or_single_client(connect=False)
# arbiters causes client to block until connected
c.arbiters
self.assertIsNotNone(c._topology)
c = self.rs_or_single_client(connect=False)
# is_primary causes client to block until connected
self.assertIsInstance(c.is_primary, bool)
c = self.rs_or_single_client(connect=False)
Expand Down Expand Up @@ -2127,6 +2151,18 @@ def test_uuid_queries(self):
self.assertEqual(2, len(docs))
coll.drop()

def test_unconnected_client_properties_with_srv(self):
client = self.simple_client("mongodb+srv://test1.test.build.10gen.cc/", connect=False)
self.assertEqual(client.nodes, frozenset())
topology_description = client.topology_description
self.assertEqual(topology_description.topology_type, TOPOLOGY_TYPE.Unknown)
self.assertEqual(
topology_description.server_descriptions(),
{("unknown", None): ServerDescription(("unknown", None))},
)
client._connect()
self.assertEqual(client.address, None)


class TestExhaustCursor(IntegrationTest):
"""Test that clients properly handle errors from exhaust cursors."""
Expand Down
Loading