-
Notifications
You must be signed in to change notification settings - Fork 1.1k
PYTHON-5289 Validate ignored bits are 0 on write for bson.BinaryVector #2397
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
Changes from 4 commits
0e8bdf6
34c0ecf
8199493
093a6dc
d7e5f3b
14db97b
62865ba
70b391d
7e71c93
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -51,6 +51,9 @@ PyMongo 4.13 brings a number of changes including: | |
or the `migration guide <https://www.mongodb.com/docs/languages/python/pymongo-driver/current/reference/migration/>`_ for more information. | ||
- Fixed a bug where :class:`pymongo.write_concern.WriteConcern` repr was not eval-able | ||
when using ``w="majority"``. | ||
- Ignored bits in a BSON BinaryVector of PACKED_BIT dtype should be set to zero. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "Ignored bits" refers to the padding bits right? Should we say "padding" to avoid introducing a new term? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will update to clarify. |
||
On writes, this is enforced and is a breaking change. | ||
Reads from the database will not fail, however a warning will be triggered. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggest referring to BSON encoding vs decoding instead of writes vs reads. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok. Updated with nod to the motivation. |
||
|
||
Issues Resolved | ||
............... | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -739,7 +739,7 @@ def test_vector(self): | |
"""Tests of subtype 9""" | ||
# We start with valid cases, across the 3 dtypes implemented. | ||
# Work with a simple vector that can be interpreted as int8, float32, or ubyte | ||
list_vector = [127, 7] | ||
list_vector = [127, 8] | ||
# As INT8, vector has length 2 | ||
binary_vector = Binary.from_vector(list_vector, BinaryVectorDtype.INT8) | ||
vector = binary_vector.as_vector() | ||
|
@@ -764,18 +764,18 @@ def test_vector(self): | |
uncompressed = "" | ||
for val in list_vector: | ||
uncompressed += format(val, "08b") | ||
assert uncompressed[:-padding] == "0111111100000" | ||
assert uncompressed[:-padding] == "0111111100001" | ||
|
||
# It is worthwhile explicitly showing the values encoded to BSON | ||
padded_doc = {"padded_vec": padded_vec} | ||
assert ( | ||
encode(padded_doc) | ||
== b"\x1a\x00\x00\x00\x05padded_vec\x00\x04\x00\x00\x00\t\x10\x03\x7f\x07\x00" | ||
== b"\x1a\x00\x00\x00\x05padded_vec\x00\x04\x00\x00\x00\t\x10\x03\x7f\x08\x00" | ||
) | ||
# and dumped to json | ||
assert ( | ||
json_util.dumps(padded_doc) | ||
== '{"padded_vec": {"$binary": {"base64": "EAN/Bw==", "subType": "09"}}}' | ||
== '{"padded_vec": {"$binary": {"base64": "EAN/CA==", "subType": "09"}}}' | ||
) | ||
|
||
# FLOAT32 is also implemented | ||
|
@@ -791,8 +791,15 @@ def test_vector(self): | |
else: | ||
self.fail("Failed to raise an exception.") | ||
|
||
# Test form of Binary.from_vector(BinaryVector) | ||
# Test one must pass zeros for all ignored bits | ||
try: | ||
Binary.from_vector([255], BinaryVectorDtype.PACKED_BIT, padding=7) | ||
except Exception as exc: | ||
self.assertIsInstance(exc, ValueError) | ||
else: | ||
self.fail("Failed to raise an exception.") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This assertion should use assertRaises. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah. You got me. This looked funny when I reviewed it last week. I'll update to follow best practice. |
||
|
||
# Test form of Binary.from_vector(BinaryVector) | ||
assert padded_vec == Binary.from_vector( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we test that the warning is raised as well? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Definitely. Thanks! Added. |
||
BinaryVector(list_vector, BinaryVectorDtype.PACKED_BIT, padding) | ||
) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So in the next major version, this warning will become an error to match the behavior in
from_vector
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. We make the significant change (exception not warning) in the next major change and document in changelog and api. The ticket for that is PYTHON-5280 and has 5.0 as its fix version.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add "pymongo version 5.0" rather than "the next major version". This helps show the user the version (5.0) and that the warning is coming from pymongo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure can.