PYTHON-3636 MongoClient should perform SRV resolution lazily by sleepyStick · Pull Request #2191 · mongodb/mongo-python-driver

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 <noah@noahstapp.com>

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

test/asynchronous/test_client.py

            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


    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

            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!

doc/changelog.rst

NoahStapp · 2025-03-24T13:36:54Z

pymongo/asynchronous/mongo_client.py

    cast,
 )

+import pymongo.uri_parser_shared


Duplicate import with the from pymongo_uri_parsed_shared... below.

NoahStapp · 2025-03-24T13:41:51Z

pymongo/asynchronous/srv_resolver.py

+        if hasattr(asyncresolver, "resolve"):
+            # dnspython >= 2
+            return await asyncresolver.resolve(*args, **kwargs)  # type:ignore[return-value]
+        raise ConfigurationError("Upgrade to dnspython version >= 2.0")


This error message should explicitly inform users that they are attempting to use the async API with an old dnspython version. Telling them only to upgrade without any other information is inconsistent with the underlying reason.

Could you add a commit suggestion here?

NoahStapp · 2025-03-24T13:44:45Z

test/test_default_exports.py


    def test_pymongo_imports(self):
        import pymongo
+        from pymongo.asynchronous.uri_parser import parse_uri


This test ensures backwards compatibility with our synchronous API public exports, so there shouldn't be any asynchronous imports here.

Good catch. We need to revert all the changes to this test.

Co-authored-by: Noah Stapp <noah@noahstapp.com>

NoahStapp · 2025-03-24T18:04:31Z

pymongo/asynchronous/srv_resolver.py

+        if hasattr(asyncresolver, "resolve"):
+            # dnspython >= 2
+            return await asyncresolver.resolve(*args, **kwargs)  # type:ignore[return-value]
+        raise ConfigurationError("Upgrade to dnspython version >= 2.0")


Suggested change

raise ConfigurationError("Upgrade to dnspython version >= 2.0")

raise ConfigurationError("Upgrade to dnspython version >= 2.0 to use AsyncMongoClient with mongodb+srv:// connections.")

ShaneHarvey

LGTM once the tests pass!

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 <noah@noahstapp.com>

sleepyStick added 3 commits March 18, 2025 08:45

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 previously approved these changes Mar 21, 2025

View reviewed changes

NoahStapp requested changes Mar 24, 2025

View reviewed changes

Update doc/changelog.rst

afd82f4

Co-authored-by: Noah Stapp <noah@noahstapp.com>

ShaneHarvey dismissed their stale review via afd82f4 March 24, 2025 17:49

NoahStapp requested changes Mar 24, 2025

View reviewed changes

Address review

99a5c8a

NoahStapp requested review from ShaneHarvey March 24, 2025 19:57

ShaneHarvey previously approved these changes Mar 24, 2025

View reviewed changes

NoahStapp self-requested a review March 25, 2025 14:26

Merge branch 'master' into PYTHON-3636

21d3f58

ShaneHarvey dismissed their stale review via 21d3f58 March 25, 2025 18:37

ShaneHarvey approved these changes Mar 25, 2025

View reviewed changes

ShaneHarvey self-requested a review March 25, 2025 18:37

NoahStapp approved these changes Mar 25, 2025

View reviewed changes

ShaneHarvey merged commit eea8a37 into mongodb:master Mar 25, 2025
34 of 37 checks passed

	raise ConfigurationError("Upgrade to dnspython version >= 2.0")
	raise ConfigurationError("Upgrade to dnspython version >= 2.0 to use AsyncMongoClient with mongodb+srv:// connections.")

Conversation

sleepyStick commented Mar 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NoahStapp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ShaneHarvey Mar 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ShaneHarvey left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ShaneHarvey Mar 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ShaneHarvey left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sleepyStick commented Mar 11, 2025 •

edited

Loading

ShaneHarvey Mar 21, 2025 •

edited

Loading

ShaneHarvey Mar 24, 2025 •

edited

Loading