Skip to content

PYTHON-6172 C/pure-Python decoders disagree on short UUID payloads - #3123

Merged
blink1073 merged 11 commits into
mongodb:mainfrom
blink1073:PYTHON-6172
Oct 14, 2026
Merged

blink1073 merged 11 commits into
mongodb:mainfrom
blink1073:PYTHON-6172

Conversation

@blink1073

Copy link
Copy Markdown
Member

PYTHON-6172

Changes in this PR

  • Removes the C decoder's 16-byte length requirement for UUID binary subtypes, matching the pure-Python decoder: such payloads decode to Binary under UNSPECIFIED, and raise InvalidBSON only when UUID conversion is requested.
  • Adds a regression test backed by a checked-in payload and a changelog entry.

Test Plan

The regression test decodes a checked-in payload and synthetic short payloads of both UUID subtypes, under the default representation and each UUID representation. python -m unittest test.test_bson.TestBSON passes (70 tests).

Checklist

Checklist for Author

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

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 requested a lite review from Copilot October 14, 2026 19:31

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

Enforce exact UUID payload length during conversion and cover the overlong case.

0 open findings

What changed in this PR

Fixes inconsistent C and pure-Python decoding of short UUID binary payloads.

Changes:

  • Relaxes the C decoder’s unconditional 16-byte check.
  • Adds regression tests and changelog documentation.
  • Requires exact-length validation during UUID conversion to reject overlong payloads.
File Summary
test/​test_bson.py Adds short UUID payload regression coverage.
doc/​changelog.rst Documents PYTHON-6172.
bson/​_cbsonmodule.c Aligns short-payload handling, but overlong Java legacy UUID payloads still require validation.

🧠 Review effort: Lite


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

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

Add coverage for short OLD_UUID_SUBTYPE payloads with UuidRepresentation.STANDARD.

0 open findings

🧠 Review effort: Lite

@codecov

codecov Bot commented Oct 14, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

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

Overlong subtype-3 UUID payloads are not yet rejected during conversion.

1 open finding

🧠 Review effort: Lite

Comment thread test/test_bson.py

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

No unresolved issues block approval.

1 open finding

🧠 Review effort: Lite

@blink1073
blink1073 marked this pull request as ready for review October 14, 2026 21:33
@blink1073
blink1073 requested a review from a team as a code owner October 14, 2026 21:33
@blink1073
blink1073 requested a review from sleepyStick October 14, 2026 21:33
@blink1073
blink1073 merged commit 4fb6cc8 into mongodb:main Oct 14, 2026
59 of 60 checks passed
@blink1073
blink1073 deleted the PYTHON-6172 branch October 14, 2026 21:46
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.

3 participants