PYTHON-3636 MongoClient should perform SRV resolution lazily #2191

sleepyStick · 2025-03-11T00:00:55Z

https://jira.mongodb.org/browse/PYTHON-3636

NoahStapp

Some small clarity tweaks and async-specific changes, but great start!

pymongo/asynchronous/mongo_client.py

pymongo/uri_parser.py

Co-authored-by: Noah Stapp <[email protected]>

NoahStapp · 2025-03-14T17:08:18Z

pymongo/asynchronous/mongo_client.py

@@ -58,10 +59,11 @@
    cast,
 )

+import pymongo.asynchronous.uri_parser


Unneeded import since uri_parser is also imported below.

NoahStapp · 2025-03-14T17:08:41Z

pymongo/asynchronous/mongo_client.py

 from bson.codec_options import DEFAULT_CODEC_OPTIONS, CodecOptions, TypeRegistry
 from bson.timestamp import Timestamp
-from pymongo import _csot, common, helpers_shared, periodic_executor, uri_parser
-from pymongo.asynchronous import client_session, database
+from pymongo import _csot, common, helpers_shared, periodic_executor, uri_parser_shared


Unneeded import since uri_parser_shared is also imported from below.

NoahStapp · 2025-03-14T17:19:57Z

pymongo/asynchronous/srv_resolver.py

+    else:
+        from dns import asyncresolver
+
+        return await asyncresolver.resolve(*args, **kwargs)  # type:ignore[return-value]


We still need to check if resolver.resolve exists in the async case. If it doesn't, that means the version of dnspython in use is too old for async support and we need to throw an error.

NoahStapp · 2025-03-14T17:22:11Z

pymongo/asynchronous/uri_parser.py

+    return result
+
+
+def _validate_uri(


This method does no network IO, can it be moved to uri_parser_shared.py to reduce code duplication?

test/asynchronous/helpers.py

test/auth_aws/test_auth_aws.py

test/auth_oidc/test_auth_oidc.py

Co-authored-by: Noah Stapp <[email protected]>

…ver into PYTHON-3636

ShaneHarvey · 2025-03-17T22:34:43Z

pymongo/synchronous/uri_parser.py

+    }
+
+
+if __name__ == "__main__":


This part should be moved back to pymongo/uri_parser.py.

pymongo/uri_parser.py

pymongo/srv_resolver.py

pymongo/asynchronous/mongo_client.py

ShaneHarvey · 2025-03-17T23:03:15Z

pymongo/asynchronous/mongo_client.py

+        if hasattr(self, "_topology"):
+            return hash(self._topology)
+        else:
+            raise InvalidOperation("Cannot hash client until it is connected")


It seems odd to disallow comparison and hashing without "connecting" first. I wonder if there's a better way to define these without relying on SRV or TXT resolution. Like could we compare some subset of MongoClient arguments?

pymongo/asynchronous/mongo_client.py

ShaneHarvey · 2025-03-17T23:12:13Z

pymongo/asynchronous/mongo_client.py

            ]
-        ]
+        else:
+            options = ["host={self._host}", "port={self._port}"]


This is not safe. self._host is the entire URI so doing this would leak the user credentials.

oh, i see, what should I use in the repr here instead then?

What about this?:

AsyncMongoClient(host='mongodb+srv://fqdn', ...

ShaneHarvey · 2025-03-21T19:49:19Z

test/asynchronous/test_client.py

@@ -1911,28 +1911,37 @@ async def test_service_name_from_kwargs(self):
            srvServiceName="customname",
            connect=False,
        )
+        await client.aconnect()


Are these open/close calls still needed?

yes because these are srv uris

ShaneHarvey · 2025-03-21T19:50:12Z

pymongo/srv_resolver.py

-        ttl = rrset.ttl if rrset else 0
-        return nodes, ttl
+__doc__ = original_doc
+__all__ = ["maybe_decode", "_SrvResolver"]


Thanks for checking, let's remove it.

ShaneHarvey · 2025-03-21T19:52:21Z

pymongo/uri_parser.py

-        print(exc)  # noqa: T201
-    sys.exit(0)
+from pymongo.synchronous.uri_parser import *  # noqa: F403
+from pymongo.synchronous.uri_parser import __doc__ as original_doc


By __doc__ I mean a module level docstring like:

"""Tools to parse and validate a MongoDB URI."""

ShaneHarvey · 2025-03-21T19:56:21Z

pymongo/asynchronous/mongo_client.py

        return NotImplemented

    def __ne__(self, other: Any) -> bool:
        return not self == other

    def __hash__(self) -> int:
-        return hash(self._topology)
+        if self._topology is None:


We still need to address hashing and equality. It's broken to change the hash of an object since it means that a single client could be added twice to a dictionary or set. The hash (and eq) methods need to use a single deterministic approach:

set = {} c = AsyncMongoClient('mongodb+srv://...') set.add(c) await c.aconnect() set.add(c) assert len(set) == 1

This means we need to use the eq_props() approach in all cases.

ShaneHarvey · 2025-03-21T21:55:40Z

pymongo/asynchronous/mongo_client.py

@@ -750,6 +755,9 @@ def __init__(
            port = self.PORT
        if not isinstance(port, int):
            raise TypeError(f"port must be an instance of int, not {type(port)}")
+        self._host = host
+        self._port = port
+        self._topology: Optional[Topology] = None


Adding assert self._topology is not None so many times feels a bit off since it adds runtime overhead for a static typing issue. What if we do this instead?:

self._topology: Topology = None # type: ignore[assignment]

ShaneHarvey · 2025-03-21T21:58:06Z

pymongo/asynchronous/mongo_client.py

-                self._kill_cursors_executor.join(),  # type: ignore[func-returns-value]
-                return_exceptions=True,
-            )
+        if self._topology is not None:


Rather than indenting this code (which causes code churn) how about we do this?:

if self._topology is not None: return session_ids = self._topology.pop_all_sessions() ...

ShaneHarvey · 2025-03-21T22:04:07Z

test/asynchronous/test_dns.py

-        )
+        with self.assertRaisesRegex(ConfigurationError, "Invalid URI host: mongodb is not"):
+            client = self.simple_client("mongodb+srv://mongodb")
+            await client.aconnect()


This behavior change is important to call out in the changelog and jira ticket.

ShaneHarvey · 2025-03-21T22:05:48Z

pymongo/asynchronous/mongo_client.py

-                "%s:%d" % (host, port) if port is not None else host
-                for host, port in self._topology_settings.seeds
+        if self._topology is None:
+            options = self._resolve_srv_info["seeds"]


Do we have tests for this? If not could we add some? I'd like to see the behavior difference in repr().

ShaneHarvey

LGTM! Nice work!

sleepyStick and others added 25 commits March 7, 2025 12:57

WIP (not cleaned up)

ead780a

this might be broken? unsure....

79c09ea

Merge branch 'mongodb:master' into PYTHON-3636

3afd732

keep parse_uri as is and have it call two different functions instead

7d771cb

cleanup

0f64689

some refactoring to reduce code duplication

ed50141

fix typing

ed25867

remove copied doc string

8d48f44

move init_background to only be called upon client connection

1a3efed

only define topology after uri resolution

d94743b

okay turns out it was *too* lazy HAHA

ad20606

cleanup

dfa0639

more cleanup

57edcbc

more cleanup

58a58a0

fix fork tests

35a41e9

fix typing

d343311

determine is_srv differently

d03c78f

fix test

e1d091f

fix encrypter

8efd549

undoing unintended changes

4c06dec

bringing back a previously deleted test

511fcc4

undoing unintended changes

40509a1

some refactoring

97e0778

Merge branch 'master' into PYTHON-3636

1de56d4

fix typing

2653a56

sleepyStick marked this pull request as ready for review March 13, 2025 16:23

sleepyStick requested review from NoahStapp and ShaneHarvey March 13, 2025 16:23

NoahStapp requested changes Mar 13, 2025

View reviewed changes

Update pymongo/asynchronous/mongo_client.py

4c23ee0

Co-authored-by: Noah Stapp <[email protected]>

sleepyStick requested a review from NoahStapp March 14, 2025 16:27

NoahStapp requested changes Mar 14, 2025

View reviewed changes

sleepyStick and others added 6 commits March 14, 2025 10:31

Update test/auth_aws/test_auth_aws.py

2a8b1b2

Co-authored-by: Noah Stapp <[email protected]>

Update test/asynchronous/helpers.py

c82cf50

Co-authored-by: Noah Stapp <[email protected]>

Update test/auth_oidc/test_auth_oidc.py

9256808

Co-authored-by: Noah Stapp <[email protected]>

address comments

c6d2ceb

Merge branch 'PYTHON-3636' of github.com:sleepyStick/mongo-python-dri…

d8d2c26

…ver into PYTHON-3636

undo import change in helpers

8927a27

sleepyStick requested a review from NoahStapp March 14, 2025 18:49

ShaneHarvey requested changes Mar 17, 2025

View reviewed changes

sleepyStick added 8 commits March 17, 2025 16:39

change client eq and hash

76a68b2

address comments part 1

b60eb60

address comment ish - remove first

0b6d303

re-order call to super's init

0ca6afd

Merge branch 'main' into PYTHON-3636

259d36b

update link to use https based on prev commit on main

d616135

fix typing

379dfb6

oops fix typing pt2

5466484

sleepyStick requested a review from ShaneHarvey March 19, 2025 22:50

ShaneHarvey requested changes Mar 21, 2025

View reviewed changes

sleepyStick added 2 commits March 21, 2025 13:08

address comments

2900718

fix patch string

63676b6

sleepyStick requested a review from ShaneHarvey March 21, 2025 21:47

ShaneHarvey requested changes Mar 21, 2025

View reviewed changes

sleepyStick added 4 commits March 21, 2025 15:14

address comments pt1

cd9bd92

Merge branch 'main' into PYTHON-3636

f33d091

add test for repr and change changelog

a7c090d

fix test

93bc3c9

ShaneHarvey approved these changes Mar 21, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PYTHON-3636 MongoClient should perform SRV resolution lazily #2191

PYTHON-3636 MongoClient should perform SRV resolution lazily #2191

sleepyStick commented Mar 11, 2025 •

edited

Loading

NoahStapp left a comment

NoahStapp Mar 14, 2025

NoahStapp Mar 14, 2025

NoahStapp Mar 14, 2025

NoahStapp Mar 14, 2025

ShaneHarvey Mar 17, 2025

ShaneHarvey Mar 17, 2025 •

edited

Loading

ShaneHarvey Mar 17, 2025

sleepyStick Mar 17, 2025

ShaneHarvey Mar 21, 2025

ShaneHarvey Mar 21, 2025

sleepyStick Mar 21, 2025

ShaneHarvey Mar 21, 2025

ShaneHarvey Mar 21, 2025

ShaneHarvey Mar 21, 2025

ShaneHarvey Mar 21, 2025

ShaneHarvey Mar 21, 2025

ShaneHarvey Mar 21, 2025

ShaneHarvey Mar 21, 2025 •

edited

Loading

ShaneHarvey left a comment

PYTHON-3636 MongoClient should perform SRV resolution lazily #2191

Are you sure you want to change the base?

PYTHON-3636 MongoClient should perform SRV resolution lazily #2191

Conversation

sleepyStick commented Mar 11, 2025 • edited Loading

NoahStapp left a 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 Mar 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

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 Mar 21, 2025 • edited Loading

Choose a reason for hiding this comment

ShaneHarvey left a comment

Choose a reason for hiding this comment

sleepyStick commented Mar 11, 2025 •

edited

Loading

ShaneHarvey Mar 17, 2025 •

edited

Loading

ShaneHarvey Mar 21, 2025 •

edited

Loading