Conversation
Trim the _truncate_metadata comments and number/label the handshake prose tests (backpressure no. 9, delimiter no. 10, index no. 11).
Reapply the 512-byte handshake limit after append_metadata, guard the check/update/record sequence with a lock for thread-safe clients, and document the reserved '|' delimiter on DriverInfo.
Use _create_lock() so the metadata lock is registered with pymongo.lock and reset after a fork, avoiding a deadlock in the child process.
Trim wrapper version content before dropping name/version segments so driver identity is preserved, and recreate the platform field when a platform append follows truncation that removed it.
Revert the 'Equal versions do not collapse' prose test case to the specification and shorten the truncation comment.
Only record drivers that remain representable in the truncated metadata, so __appended_drivers cannot grow without bound and the dedup membership check stays fast. Add a regression test.
Use the name delimiter count before/after the update to decide whether an appended pair survived truncation, instead of a name/version branch that always recorded platform-only (empty name/version) drivers.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Several required index-correspondence prose cases are missing or do not exercise their stated behavior.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Updates handshake metadata to preserve one-to-one correspondence between driver names and versions.
Changes:
- Rejects reserved delimiters in
DriverInfo. - Adds aligned metadata appending, deduplication, locking, and truncation.
- Expands synchronous, asynchronous, and handshake tests.
File summaries
| File | Description |
|---|---|
pymongo/driver_info.py |
Validates metadata delimiters. |
pymongo/pool_options.py |
Implements aligned, thread-safe metadata updates. |
test/asynchronous/test_client.py |
Tests async metadata alignment and truncation. |
test/asynchronous/test_client_metadata.py |
Adds async handshake prose tests. |
test/test_client.py |
Adds generated synchronous coverage. |
test/test_client_metadata.py |
Adds generated synchronous prose tests. |
test/mockupdb/test_handshake.py |
Updates expected handshake metadata. |
Review details
Suppressed comments (1)
test/asynchronous/test_client_metadata.py:286
- This does not test a wrapper matching the driver's own identity because the appended version is
None. Append PyMongo's base name and version together and expect both entries, otherwise the required whole-identity case remains uncovered.
("Wrapper matching the driver's own identity", [("PyMongo", None)], "|PyMongo", "|"),
- Files reviewed: 7/7 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Add 'Gap in middle (name)' and 'All names absent' cases and drop the non-None name assertion so empty name segments are verified to stay index-aligned.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Number index correspondence as prose test 10 and delimiter rejection as prose test 11, and mirror the specifications PR mongodb#1975 case table (order and content, resolving <driver-name>/<driver-version> at runtime).
Prefix each client metadata prose test method with its prose test number and full specification title (Test 1, 2, 9, 10, 11) and order them by prose test number.
Exercise the DriverInfo delimiter ValueError for every field, the platform-recreation path, and add truncation/bounded-retention coverage so the new pool_options and driver_info lines are covered without mockupdb.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements the DRIVERS-3251 handshake metadata update (PR #1975): PyMongo's
driver.nameanddriver.versionare now pipe-delimited lists with a 1:1 index correspondence. Each appended name segment (|c,|async, or a wrapped driver) has a matching version entry, so the number of|innamealways equals the number of|inversion.Changes in this PR
|delimiter inDriverInfofields at construction time.|cand|asyncname suffixes so name/version stay index-aligned._update_metadatato always append the delimiter for name and version, deduplicate appended drivers by wholeDriverInfoobject, and track appended drivers._truncate_metadatato keep name/version index-aligned when metadata is truncated to the 512-byte limit.Test Plan
just lintandjust typing— both pass.just test test/asynchronous/test_client.py::AsyncClientUnitTest::test_metadata test/test_client.py::ClientUnitTest::test_metadata— 2 passed.test/asynchronous/test_client_metadata.pyandtest/test_client_metadata.py(async+sync) — 28 passed.test/mockupdb/test_handshake.py— 7 passed.test_client.py(async+sync) — 240 passed.name.count("|") == version.count("|")after truncation for sync/async × huge name/version.Checklist
Checklist for Author
Checklist for Reviewer