Skip to content

PYTHON-5172 bugfix: Add __repr__ and __eq__ to bson.binary.BinaryVector #2162

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.

Sign up for GitHub

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

Merged
merged 11 commits into from
Mar 10, 2025
Merged

PYTHON-5172 bugfix: Add __repr__ and __eq__ to bson.binary.BinaryVector #2162

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
519291c
Adds __repr__ to BinaryVector dataclass
caseyclements Feb 27, 2025
a4ce5c7
Typing
caseyclements Feb 27, 2025
8b46ecf
Updated repr form to match python convention
caseyclements Feb 28, 2025
ba43d0c
Updated repr form to match python convention and added repr tests
caseyclements Feb 28, 2025
b83bee4
Removed unused noqa rule RUF100
caseyclements Feb 28, 2025
8bd9482
Reverted BinaryVector repr to f-string following python convention
caseyclements Feb 28, 2025
389b1cf
Additional test of repr form: eval(repr(obj)) ~= obj
caseyclements Feb 28, 2025
916ec33
Remove dataclass decorator from BinaryVector. Instead, add __init__ a…
caseyclements Mar 3, 2025
023d8ca
Revert "Remove dataclass decorator from BinaryVector. Instead, add __…
caseyclements Mar 5, 2025
ceb6786
BinaryVector is no longer a dataclass. Instead, we implement __eq__
caseyclements Mar 6, 2025
f35ef2b
Typing, other: Any
caseyclements Mar 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions bson/binary.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,11 @@ def __init__(self, data: Sequence[float | int], dtype: BinaryVectorDtype, paddin
self.dtype = dtype
self.padding = padding

def __repr__(self) -> str:
return "BinaryVector(dtype={}, padding={}, data={})".format( # noqa: UP032
Copy link
Member

Choose a reason for hiding this comment

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

will repr create the string even when it's not called?

f-string vs format makes no difference. If anything f-string will be faster. __repr__ only runs when repr() is called.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

__ only runs when repr() is called.

So if one has logger.debug(vector), is it not true that str.format won't be called, but the f-string will be created?

Copy link
Member

@ShaneHarvey ShaneHarvey Feb 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Logging behavior is unrelated to this PR since we're just adding a new method. The behavior of repr(vector) is the same whether we use f-string vs format() so let's go back to using f-string.

self.dtype, self.padding, self.data
)

Copy link
Member

Choose a reason for hiding this comment

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

Hold up, the real bug here is that we're using @dataclass but also defining __init__. Why is that? The whole point of dataclass is that it provides __init__ and __repr__ automatically.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe that we had issues with the combo of {dataclass, slots, defaults}. So I've just removed the dataclass decorator.

Copy link
Member

@ShaneHarvey ShaneHarvey Mar 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Are there any other features that will break by removing @dataclass? We may be stuck with it until 5.0 if it's a breaking change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so. As mentioned in the closed PR, we've done init, and repr, and we don't want to rely on equality except via the binary representation.

Copy link
Contributor Author

@caseyclements caseyclements Mar 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The code seems happy to be a dataclass or not. I have no strong preference.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://jira.mongodb.org/browse/DRIVERS-3123

Copy link
Member

@ShaneHarvey ShaneHarvey Mar 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Can you show an example of == and > before and after this change? I'm wondering if the old code (with @dataclass) even works at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. It's easy to try. Put a breakpoint on line 83 of test_bson_binary.

# With the decorator:
vector_obs == vector_exp.as_vector()
True
vector_obs > vector_exp.as_vector()
# TypeError: '>' not supported between instances of 'BinaryVector' and 'BinaryVector'

# Without the decorator:
vector_obs == vector_exp.as_vector()
False
vector_obs > vector_exp.as_vector()
# TypeError: '>' not supported between instances of 'BinaryVector' and 'BinaryVector'

Copy link
Member

Choose a reason for hiding this comment

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

== is already broken with our current @dataclass approach:

>>> from bson.binary import *
>>> BinaryVector([1], BinaryVectorDtype.INT8) == BinaryVector([2], BinaryVectorDtype.INT8)
True

So we need to remove dataclass and implement == as well as __repr__.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Take a look. I hate subtleties with == and floats..


class Binary(bytes):
"""Representation of BSON binary data.
Expand Down
24 changes: 24 additions & 0 deletions test/test_bson.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -809,6 +809,30 @@ def test_vector(self):
dtype=BinaryVectorDtype.PACKED_BIT,
) # type: ignore[call-overload]

def test_binaryvector_repr(self):
"""Tests of repr(BinaryVector)"""
data = [127, 7]
zero = BinaryVector([], BinaryVectorDtype.INT8)
self.assertEqual(
repr(zero), "BinaryVector(dtype=BinaryVectorDtype.INT8, padding=0, data=[])"
)
one = BinaryVector(data, BinaryVectorDtype.INT8)
self.assertEqual(
repr(one), f"BinaryVector(dtype=BinaryVectorDtype.INT8, padding=0, data={data})"
)
two = BinaryVector(data, BinaryVectorDtype.FLOAT32)
self.assertEqual(
repr(two), f"BinaryVector(dtype=BinaryVectorDtype.FLOAT32, padding=0, data={data})"
)
three = BinaryVector(data, BinaryVectorDtype.FLOAT32, padding=0)
self.assertEqual(
repr(three), f"BinaryVector(dtype=BinaryVectorDtype.FLOAT32, padding=0, data={data})"
)
four = BinaryVector(data, BinaryVectorDtype.PACKED_BIT, padding=3)
self.assertEqual(
repr(four), f"BinaryVector(dtype=BinaryVectorDtype.PACKED_BIT, padding=3, data={data})"
)
Copy link
Member

Choose a reason for hiding this comment

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

Could you add asserts using assertRepr like we do in test_connection_monitoring.py?:

    def assertRepr(self, obj):
        new_obj = eval(repr(obj))
        self.assertEqual(type(new_obj), type(obj))
        self.assertEqual(repr(new_obj), repr(obj))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. That's very cool, that one can create an object using eval(repr(obj))


def test_unicode_regex(self):
"""Tests we do not get a segfault for C extension on unicode RegExs.
This had been happening.
Expand Down
Loading