Fix nested container conversion

Summary: Fix converter for the nested containers.

Reviewed By: aristidisp

Differential Revision: D67769030

fbshipit-source-id: ccf9b0de3986a6f7046e584b0334e6f85ce98fb6

thrift/lib/python/mutable_converter.pyx

@@ -85,7 +85,8 @@ cdef object _to_mutable_python_struct_or_union(
             **{
                 field.py_name: _to_mutable_python_field_value(
                     _get_src_struct_field_value(src_struct_or_union, field),
-                    field.type_info
+                    field.type_info,
+                    is_nested_container=False,
                 )
                 for field in mutable_struct_info.fields
             }
@@ -98,7 +99,7 @@ cdef object _to_mutable_python_struct_or_union(
             try:
                 src_union_value = _get_src_union_field_value(src_struct_or_union, field)
                 mutable_python_value = _to_mutable_python_field_value(
-                    src_union_value, field.type_info
+                    src_union_value, field.type_info, is_nested_container=False,
                 )
                 return mutable_thrift_python_cls(
                     **{field.py_name: mutable_python_value}
@@ -168,14 +169,30 @@ cdef object _get_src_union_field_value(object src_union, FieldInfo field):
     )
 
 cdef object _to_mutable_python_field_value(
-    object src_value, object mutable_type_info
+    object src_value, object mutable_type_info, bint is_nested_container
 ):
     """
     Converts `src_value` (from the src object) to a value suitable for a thrift-python
     field with the given `mutable_type_info`.
 
     Effectively, the returned value should correspond to the value that a client would
     provide at initialization time, if directly creating a thrift-python instance.
+
+    Args:
+        src_value: The source value to be converted.
+        mutable_type_info: An object providing type information for the field.
+        is_nested_container: A boolean indicating whether the current container is nested
+            within another container. Only applicable if `mutable_type_info` is a container
+            type info (MutableListTypeInfo, MutableSetTypeInfo, or MutableMapTypeInfo).
+
+    Note:
+        - Mutable thrift-python fields should be assigned with to_thrift_list(),
+        to_thrift_set(), or to_thrift_map(). These functions use the corresponding
+        _Thrift{List,Set,Map}Wrapper structs.
+        - For nested containers, only the top container should be wrapped, not
+        the nested containers. For example, for list<set<i32>>, assignment should
+        be to_thrift_list([{1, 2}, {3, 4}]), wrapping the internal container will
+        result in an error (to_thrift_list([to_thrift_set({1, 2})])).
     """
     if src_value is None:
         return None
@@ -188,26 +205,35 @@ cdef object _to_mutable_python_field_value(
             (<MutableStructTypeInfo>mutable_type_info)._mutable_struct_class, src_value
         )
     elif isinstance(mutable_type_info, MutableListTypeInfo):
-        return _ThriftListWrapper([
+        list_value = [
             _to_mutable_python_field_value(
-                elem, (<MutableListTypeInfo>mutable_type_info).val_info
+                elem,
+                (<MutableListTypeInfo>mutable_type_info).val_info,
+                is_nested_container=True,
             )
             for elem in src_value
-        ])
+        ]
+        return list_value if is_nested_container else _ThriftListWrapper(list_value)
     elif isinstance(mutable_type_info, MutableSetTypeInfo):
-        return _ThriftSetWrapper({
+        set_value = {
             _to_mutable_python_field_value(
-                elem, (<MutableSetTypeInfo>mutable_type_info).val_info
+                elem,
+                (<MutableSetTypeInfo>mutable_type_info).val_info,
+                is_nested_container=True
             )
             for elem in src_value
-        })
+        }
+        return set_value if is_nested_container else _ThriftSetWrapper(set_value)
     elif isinstance(mutable_type_info, MutableMapTypeInfo):
         mutable_map_type_info = <MutableMapTypeInfo>mutable_type_info
-        return _ThriftMapWrapper({
-            _to_mutable_python_field_value(k, mutable_map_type_info.key_info):
-            _to_mutable_python_field_value(v, mutable_map_type_info.val_info)
+        map_value = {
+            _to_mutable_python_field_value(
+                k, mutable_map_type_info.key_info, is_nested_container=True):
+            _to_mutable_python_field_value(
+                v, mutable_map_type_info.val_info, is_nested_container=True)
             for k, v in src_value.items()
-        })
+        }
+        return map_value if is_nested_container else _ThriftMapWrapper(map_value)
     elif isinstance(mutable_type_info, EnumTypeInfo):
         try:
             return (<EnumTypeInfo>mutable_type_info)._class(int(src_value))

thrift/lib/python/test/mutable_structs.py

@@ -53,6 +53,7 @@
     TestStructNested_0 as TestStructNested_0_Immutable,
     TestStructNested_1 as TestStructNested_1_Immutable,
     TestStructNested_2 as TestStructNested_2_Immutable,
+    TestStructWithNestedContainers as TestStructWithNestedContainersImmutable,
 )
 
 max_byte: int = 2**7 - 1
@@ -528,3 +529,21 @@ def test_copy_from_immutable_struct(self) -> None:
         self.assertEqual(42, s_mutable.i32_field)
         self.assertEqual(41, s_mutable.nested_1.i32_field)
         self.assertEqual(40, s_mutable.nested_1.nested_2.i32_field)
+
+    def test_conversion_nested_containers(self) -> None:
+        s = TestStructWithNestedContainersImmutable(
+            list_list_i32=[[1]],
+            list_set_i32=[{2}],
+            list_map_string_i32=[{"a": 1}],
+            list_map_string_list_i32=[{"b": [2]}],
+            list_map_string_set_i32=[{"c": {3}}],
+            map_i32_list_i32={1: [1]},
+            map_i32_set_i32={2: {2}},
+            map_i32_map_string_i32={3: {"a": 1}},
+            map_i32_map_string_list_i32={4: {"b": [2]}},
+            map_i32_map_string_set_i32={5: {"c": {3}}},
+            many_nested=[[{1: [{"a": [{2}]}]}]],
+        )
+
+        mutable_struct = s._to_mutable_python()
+        self.assertEqual(s, mutable_struct._to_python())

thrift/test/thrift-python/struct_test.py

@@ -76,7 +76,6 @@
     TestStructNested_1 as TestStructNested_1_Mutable,
     TestStructNested_2 as TestStructNested_2_Mutable,
     TestStructWithDefaultValues as TestStructWithDefaultValuesMutable,
-    TestStructWithMapOfList as TestStructWithMapOfListMutable,
     TestStructWithTypedefField as TestStructWithTypedefFieldMutable,
 )
 
@@ -87,7 +86,6 @@
     TestStructAllThriftPrimitiveTypes as TestStructAllThriftPrimitiveTypesImmutable,
     TestStructAllThriftPrimitiveTypesWithDefaultValues as TestStructAllThriftPrimitiveTypesWithDefaultValuesImmutable,
     TestStructWithDefaultValues as TestStructWithDefaultValuesImmutable,
-    TestStructWithMapOfList as TestStructWithMapOfListImmutable,
 )
 
 max_byte: int = 2**7 - 1
@@ -522,21 +520,6 @@ def test_match(self) -> None:
             case _:
                 self.fail("Expected match, got none.")
 
-    def test_conversion_map_of_list(self) -> None:
-        s = TestStructWithMapOfListImmutable(
-            str_to_test_structs={"foo": [TestStructImmutable(unqualified_string="bar")]}
-        )
-
-        # DO_BEFORE(aristidis,20250115): Fix conversion bug, code below should not throw
-        with self.assertRaisesRegex(
-            TypeError,
-            (
-                "error setting Thrift struct field 'str_to_test_structs': object of "
-                "type 'thrift.python.mutable_types._ThriftListWrapper' has no len()"
-            ),
-        ):
-            s._to_mutable_python()
-
 
 class ThriftPython_MutableStruct_Test(unittest.TestCase):
     def setUp(self) -> None:

thrift/test/thrift-python/struct_test.thrift

@@ -277,6 +277,21 @@ struct TestStructWithInvariantField {
   3: map<TestStruct, i32> unqualified_map_struct_i32;
 }
 
-struct TestStructWithMapOfList {
-  1: map<string, list<TestStruct>> str_to_test_structs;
+struct TestStructWithNestedContainers {
+  1: list<list<i32>> list_list_i32;
+  2: list<set<i32>> list_set_i32;
+  3: list<map<string, i32>> list_map_string_i32;
+  4: list<map<string, list<i32>>> list_map_string_list_i32;
+  5: list<map<string, set<i32>>> list_map_string_set_i32;
+  6: set<list<i32>> set_list_i32;
+  7: set<set<i32>> set_set_i32;
+  8: set<map<string, i32>> set_map_string_i32;
+  9: set<map<string, list<i32>>> set_map_string_list_i32;
+  10: set<map<string, set<i32>>> set_map_string_set_i32;
+  11: map<i32, list<i32>> map_i32_list_i32;
+  12: map<i32, set<i32>> map_i32_set_i32;
+  13: map<i32, map<string, i32>> map_i32_map_string_i32;
+  14: map<i32, map<string, list<i32>>> map_i32_map_string_list_i32;
+  15: map<i32, map<string, set<i32>>> map_i32_map_string_set_i32;
+  16: list<list<map<i32, list<map<string, list<set<i32>>>>>>> many_nested;
 }