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.
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.
There was a problem hiding this comment.
🟡 Changes recommended
Truncation can discard an entire wrapper name instead of retaining a size-compliant truncated value.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 1
- Review effort level: Balanced
When the trailing version entry is empty, shrink the oversized wrapper name instead of dropping the whole name/version pair, so a driver with a large name and no version keeps a truncated name rather than collapsing to the base entry.
There was a problem hiding this comment.
🟡 Changes recommended
Driver truncation incorrectly treats byte overflow as a character count, discarding valid multibyte metadata.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
pymongo/pool_options.py:257
overflowmeasures encoded bytes, whereas this slice removes characters. An oversized multibyte name such as an emoji-only wrapper is reduced to an empty segment (PyMongo|) even though many complete characters fit under 512 bytes, contradicting the intended retention of a truncated wrapper name. Apply the limit to UTF-8 bytes without splitting a code point.
elif len(n_parts) > 1 and n_parts[-1]:
n_parts[-1] = n_parts[-1][:-overflow]
driver["name"] = "|".join(n_parts)
- Files reviewed: 7/7 changed files
- Comments generated: 1
- Review effort level: Balanced
The overflow is a BSON byte count, so trim wrapper version, name, and platform by UTF-8 bytes and decode a valid prefix instead of slicing Unicode code points.
There was a problem hiding this comment.
🔵 Needs a closer look
Deduplication incorrectly treats equivalent empty-string and unset fields as distinct metadata.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
pymongo/pool_options.py:406
- Empty strings are defined as unset when comparing driver metadata, but namedtuple membership distinguishes
""fromNone. For example, after appendingDriverInfo("library", None, "platform"), appendingDriverInfo("library", "", "platform")currently adds a duplicate name/version segment instead of being a no-op. Normalize fields before the deduplication comparison.
- Files reviewed: 7/7 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Normalize DriverInfo fields before the dedup comparison so None and '' are treated as the same unset value, matching the spec and avoiding duplicate name/version segments.
There was a problem hiding this comment.
🟢 Approval recommended
The implementation preserves metadata alignment, bounds truncation correctly, and includes comprehensive mirrored coverage.
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Implements the DRIVERS-3251 handshake metadata update: PyMongo's
driver.nameanddriver.versionare now pipe-delimited lists with a 1:1 index correspondence. Each name segment (|c,|async, or a framework) 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
Checklist
Checklist for Author
Checklist for Reviewer