Skip to content

PYTHON-5344 and PYTHON-5403 Allow Instantiated MongoClients to Send Client Metadata On-Demand #2358

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.

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 20 commits into from
Jun 24, 2025

Conversation

sleepyStick
Copy link
Contributor

@sleepyStick sleepyStick commented Jun 2, 2025

https://jira.mongodb.org/browse/PYTHON-5344
https://jira.mongodb.org/browse/PYTHON-5403 (ticket for async tests)

Notes:

  • I used mockupDB to capture the metadata that the client sends in the handshake
  • i put the tests in the normal tests folder (as opposed to the test/mockupdb) to utilize synchro for the async -> sync tests.

@sleepyStick sleepyStick marked this pull request as ready for review June 18, 2025 00:18
@sleepyStick sleepyStick changed the title PYTHON-5344 Allow Instantiated MongoClients to Send Client Metadata On-Demand PYTHON-5344 and PYTHON5403 Allow Instantiated MongoClients to Send Client Metadata On-Demand Jun 18, 2025
@sleepyStick sleepyStick changed the title PYTHON-5344 and PYTHON5403 Allow Instantiated MongoClients to Send Client Metadata On-Demand PYTHON-5344 and PYTHON-5403 Allow Instantiated MongoClients to Send Client Metadata On-Demand Jun 18, 2025
@sleepyStick sleepyStick requested a review from blink1073 June 18, 2025 16:33
blink1073
blink1073 previously approved these changes Jun 18, 2025
Copy link
Member

@blink1073 blink1073 left a comment

Choose a reason for hiding this comment

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

LGTM!

@sleepyStick sleepyStick requested a review from NoahStapp June 23, 2025 18:19
@@ -1040,6 +1041,23 @@ async def target() -> bool:
self._kill_cursors_executor = executor
self._opened = False

def append_metadata(self, driver_info: DriverInfo) -> None:
"""
Appends the given metadata to existing driver metadata.
Copy link
Member

Choose a reason for hiding this comment

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

Could you update this docstring to follow our standard format (params, versionadded, etc...)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made my best attempt at it, but lmk if I was off / am still missing something / have it incorrectly formatted >.<


import pytest

from pymongo import AsyncMongoClient, MongoClient
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
from pymongo import AsyncMongoClient, MongoClient
from pymongo import AsyncMongoClient

if "ismaster" in r:
# then this is a handshake request
self.handshake_req = r
return r.reply(OpMsgReply(minWireVersion=0, maxWireVersion=13))
Copy link
Contributor

Choose a reason for hiding this comment

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

Is maxWireVersion=13 specified by the spec or by MockupDB?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think its needed so that MockupDB would work(?) without specification, the maxWireVersion is 6, and PyMongo raises a configuration error: pymongo.errors.ConfigurationError: Server at localhost:56617 reports wire version 0, but this version of PyMongo requires at least 7 (MongoDB 4.0).

# send initial metadata
name, version, platform, metadata = await self.send_ping_and_get_metadata(client, True)
# wait for connection to become idle
await asyncio.sleep(0.005)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we refactor this into an async_wait_until with some predicate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The spec specifically stated to wait 5ms

self.assertEqual(metadata, new_metadata)

async def test_append_metadata(self):
client = AsyncMongoClient(
Copy link
Contributor

Choose a reason for hiding this comment

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

All of these clients should be created with one of our PyMongoTestCase helpers, not raw AsyncMongoClient().

@@ -925,7 +931,6 @@ async def run_entity_operation(self, spec):
)
else:
arguments = {}

Copy link
Contributor

Choose a reason for hiding this comment

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

Whitespace removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops accident!

def _update_metadata(self, driver: DriverInfo):
"""Updates the client's metadata"""
if driver.name:
self.__metadata["driver"]["name"] = "{}|{}".format(
Copy link
Member

Choose a reason for hiding this comment

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

Could we make this method atomic? As in, first create a copy of __metadata, then mutate, then update? That way the driver handshake will never serialize half of the metadata.

ShaneHarvey
ShaneHarvey previously approved these changes Jun 23, 2025
Copy link
Member

@ShaneHarvey ShaneHarvey left a comment

Choose a reason for hiding this comment

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

API changes LGTM. Could you also add the changelog entry?

Copy link
Contributor

@NoahStapp NoahStapp left a comment

Choose a reason for hiding this comment

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

LGTM! Great work!

@sleepyStick sleepyStick merged commit 65f7c54 into mongodb:master Jun 24, 2025
77 of 79 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants