Skip to content

Commit

Permalink
Return False on type error
Browse files Browse the repository at this point in the history
Summary: Mutable containers `__contains__` should return `False` on  type errors similar to immutable thrift-python.

Reviewed By: ahilger

Differential Revision: D65205650

fbshipit-source-id: ba46c689650249f2719d758a9279dbfaa7202ca8
  • Loading branch information
yoney authored and facebook-github-bot committed Oct 30, 2024
1 parent 5226b11 commit a08deda
Show file tree
Hide file tree
Showing 4 changed files with 24 additions and 30 deletions.
15 changes: 13 additions & 2 deletions third-party/thrift/src/thrift/lib/python/mutable_containers.pyx
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,11 @@ cdef class MutableSet:
if item is None:
return False

internal_item = self._val_typeinfo.to_internal_data(item)
try:
internal_item = self._val_typeinfo.to_internal_data(item)
except (TypeError, OverflowError):
return False

return internal_item in self._set_data

def __iter__(MutableSet self):
Expand Down Expand Up @@ -536,7 +540,14 @@ cdef class MutableMap:
return default

def __contains__(self, key):
internal_key = self._key_typeinfo.to_internal_data(key)
if key is None:
return False

try:
internal_key = self._key_typeinfo.to_internal_data(key)
except (TypeError, OverflowError):
return False

return internal_key in self._map_data

def __reduce__(self):
Expand Down
13 changes: 3 additions & 10 deletions third-party/thrift/src/thrift/lib/python/test/mutable_map_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -221,18 +221,11 @@ def test_contains(self) -> None:

def test_contains_wrong_type(self) -> None:
mutable_map = MutableMap(typeinfo_string, typeinfo_i32, {})

with self.assertRaisesRegex(
TypeError, "Expected type <class 'str'>, got: <class 'int'>"
):
_ = 1 in mutable_map
self.assertNotIn(1, mutable_map)

mutable_map["A"] = 65

with self.assertRaisesRegex(
TypeError, "Expected type <class 'str'>, got: <class 'int'>"
):
_ = 999 in mutable_map
self.assertNotIn(999, mutable_map)
self.assertIn("A", mutable_map)

def test_keys(self) -> None:
mutable_map = MutableMap(typeinfo_string, typeinfo_i32, {})
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,17 +66,13 @@ def test_contains(self) -> None:

def test_contains_wrong_type(self) -> None:
mutable_set = MutableSet._from_iterable(typeinfo_i32, set(), range(10))
with self.assertRaisesRegex(
TypeError, "is not a <class 'int'>, is actually of type <class 'str'>"
):
if "Not an interger" in mutable_set:
pass
self.assertIn(1, mutable_set)
self.assertNotIn("Not an interger", mutable_set)

def test_contains_i32_overflow(self) -> None:
mutable_set = MutableSet._from_iterable(typeinfo_i32, set(), range(10))
with self.assertRaises(OverflowError):
if (2**31) in mutable_set:
pass
self.assertIn(1, mutable_set)
self.assertNotIn(2**31, mutable_set)

def test_iter(self) -> None:
mutable_set = MutableSet._from_iterable(typeinfo_i32, set(), range(10))
Expand Down
14 changes: 4 additions & 10 deletions third-party/thrift/src/thrift/test/thrift-python/struct_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -1033,10 +1033,8 @@ def test_create_and_assign_for_set(self) -> None:
self.assertNotIn("11", s.unqualified_set_string)
self.assertNotIn("12", s.unqualified_set_string)

with self.assertRaisesRegex(
TypeError, "Expected type <class 'str'>, got: <class 'int'>"
):
self.assertIn(1, s.unqualified_set_string)
# `__contains__()` returns `False` even with the wrong key type
self.assertNotIn(1, s.unqualified_set_string)

# `add()`
s.unqualified_set_string.add("4")
Expand Down Expand Up @@ -1196,12 +1194,8 @@ def test_create_and_assign_for_map(self) -> None:
self.assertNotIn("x", s.unqualified_map_string_i32)
self.assertNotIn("y", s.unqualified_map_string_i32)

# `__contains__()` is type checked
self.assertIn("a", s.unqualified_map_string_i32)
with self.assertRaisesRegex(
TypeError, "Expected type <class 'str'>, got: <class 'int'>"
):
self.assertIn(1, s.unqualified_map_string_i32)
# `__contains__()` returns `False` even with the wrong key type
self.assertNotIn(1, s.unqualified_map_string_i32)

# `__getitem__()`
self.assertEqual(1, s.unqualified_map_string_i32["a"])
Expand Down

0 comments on commit a08deda

Please sign in to comment.