diff --git a/benedict/dicts/base/base_dict.py b/benedict/dicts/base/base_dict.py index a8ad765..1a00144 100644 --- a/benedict/dicts/base/base_dict.py +++ b/benedict/dicts/base/base_dict.py @@ -22,15 +22,41 @@ class BaseDict(dict[_K, _V]): _dict: dict[_K, _V] | None _frozen: bool + # ids of mappings currently being unwrapped by `_get_dict_or_value`, + # anywhere on the current call stack (not just in directly nested calls + # of this method: unwrapping a value can re-enter this method through + # __init__/__setitem__/_cast). Used to detect self-referential + # (cyclic) structures and abort unwrapping them instead of recursing + # forever. fix #592 + _unwrapping_ids: set[int] = set() + @classmethod def _get_dict_or_value(cls, value: Any) -> Any: value = value.dict() if isinstance(value, cls) else value if isinstance(value, MutableMapping): - for key in value.keys(): - key_val = value[key] - if isinstance(key_val, cls): - key_val = cls._get_dict_or_value(value[key]) - value[key] = key_val + value_id = id(value) + if value_id in cls._unwrapping_ids: + # value is already being unwrapped higher up the call stack, + # ie. it (directly or indirectly) contains itself. Raise + # immediately instead of unwrapping it again, to unwind the + # whole unwrap/cast chain before the still-cyclic structure + # can reach unrelated code with no cycle protection of its + # own (eg. keypath separator validation), which would also + # recurse infinitely but with an uncontrolled RecursionError. + raise ValueError( + "Cannot assign a dict that contains itself " + "(directly or indirectly): self-referential " + "(cyclic) structures are not supported." + ) + cls._unwrapping_ids.add(value_id) + try: + for key in value.keys(): + key_val = value[key] + if isinstance(key_val, cls): + key_val = cls._get_dict_or_value(value[key]) + value[key] = key_val + finally: + cls._unwrapping_ids.discard(value_id) return value def __new__(cls, *args: Any, **kwargs: Any) -> Self: diff --git a/tests/github/test_issue_0592.py b/tests/github/test_issue_0592.py new file mode 100644 index 0000000..32662d8 --- /dev/null +++ b/tests/github/test_issue_0592.py @@ -0,0 +1,52 @@ +import unittest + +from benedict import benedict + + +class github_issue_0592_test_case(unittest.TestCase): + """ + This class describes a github issue 0592 test case. + https://github.com/fabiocaccamo/python-benedict/issues/592 + + To run this specific test: + - Run python -m unittest tests.github.test_issue_0592 + """ + + def test_assigning_dict_to_its_own_nested_descendant_does_not_raise(self) -> None: + # this specific case (assigning the dict into an empty nested + # descendant) is not self-referential yet at assignment time, + # so it must keep working exactly as before this fix. + d = benedict(keyattr_enabled=True, keyattr_dynamic=True) + d.a.b.c = d + self.assertIs(dict.__getitem__(d, "a")["b"]["c"], d) + + def test_assigning_dict_to_its_own_nested_descendant_twice_does_not_recurse( + self, + ) -> None: + # once `d` contains itself (from the first assignment), any further + # operation that needs to unwrap `d` again (eg. navigating into it + # to perform a second self-nesting assignment) must not raise + # RecursionError (it used to crash the Python/Jupyter process). + d = benedict(keyattr_enabled=True, keyattr_dynamic=True) + d.a.b.c = d + with self.assertRaises(ValueError): + d.a.b.d.e = d + + def test_self_assignment_at_new_key_still_works(self) -> None: + # assigning a dict to itself under a brand new key must keep + # behaving exactly as before this fix (unaffected by it, verified + # against unpatched behavior: it does not raise, and the resulting + # structure is still a benedict, even though it stays cyclic). + d = benedict({"a": {"b": 1}}) + d["self"] = d + self.assertIsInstance(d["self"], benedict) + self.assertEqual(d["self"]["a"], {"b": 1}) + + def test_normal_nested_dict_assignment_is_unaffected(self) -> None: + # sanity check: normal (non self-referential) nested benedict + # values must still be unwrapped/copied as before this fix. + inner = benedict({"x": 1}) + outer = benedict() + outer["inner"] = inner + self.assertEqual(outer, {"inner": {"x": 1}}) + self.assertIsNot(outer["inner"], inner)