Skip to content

PYTHON-6040 Preserve version-to-name mapping in Client Metadata - #3054

Open
blink1073 wants to merge 16 commits into
mongodb:mainfrom
blink1073:PYTHON-6040
Open

blink1073 wants to merge 16 commits into
mongodb:mainfrom
blink1073:PYTHON-6040

Conversation

@blink1073

@blink1073 blink1073 commented Sep 15, 2026

Copy link
Copy Markdown
Member

Implements the DRIVERS-3251 handshake metadata update: PyMongo's driver.name and driver.version are 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 | in name always equals the number of | in version.

Changes in this PR

  • Reject the | delimiter in DriverInfo fields at construction time.
  • Append empty version entries for the |c and |async name suffixes so name/version stay index-aligned.
  • Rewrite _update_metadata to always append the delimiter for name and version, deduplicate appended drivers by whole DriverInfo object, and track appended drivers.
  • Fix _truncate_metadata to keep name/version index-aligned when metadata is truncated to the 512-byte limit.
  • Update the metadata, dedup, and handshake tests.

Test Plan

  • Standard evergreen tasks

Checklist

Checklist for Author

  • Did you update the changelog (if necessary)? N/A
  • Is there test coverage?
  • Is any followup work tracked in a JIRA ticket? If so, add link(s). N/A

Checklist for Reviewer

  • Does the title of the PR reference a JIRA Ticket?
  • Do you fully understand the implementation? (Would you be comfortable explaining how this code works to someone else?)
  • Is all relevant documentation (README or docstring) updated?

@blink1073 blink1073 closed this Sep 15, 2026
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.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 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.

Comment thread test/asynchronous/test_client_metadata.py Outdated
Comment thread test/asynchronous/test_client_metadata.py Outdated
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

codecov Bot commented Sep 15, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 88.67925% with 6 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
pymongo/pool_options.py 88.23% 3 Missing and 3 partials ⚠️

📢 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.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 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

Comment thread pymongo/pool_options.py
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.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 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

  • overflow measures 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

Comment thread pymongo/pool_options.py
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.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 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 "" from None. For example, after appending DriverInfo("library", None, "platform"), appending DriverInfo("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.
@blink1073
blink1073 requested a balanced review from Copilot September 15, 2026 17:57

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 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

@blink1073
blink1073 marked this pull request as ready for review September 15, 2026 18:21
@blink1073
blink1073 requested a review from a team as a code owner September 15, 2026 18:21
@blink1073
blink1073 requested review from NoahStapp and sleepyStick and removed request for sleepyStick September 15, 2026 18:21
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.

2 participants