Allow assigning None to optional fields

Summary: Optional fields have a default value of `None` and unqualified fields cannot be set to `None`. However, after setting a value for the optional fields, there was no mechanism (except for `_fbthrift_internal_resetFieldToStandardDefault()`) to reset the optional field to its default value of `None`. This diff enables assigning `None` to optional fields.

Reviewed By: ahilger

Differential Revision: D62921027

fbshipit-source-id: 4ad169b071accc4123b87aa595842c568c5cd3de

thrift/lib/python/mutable_types.pyx

@@ -94,6 +94,13 @@ def _isset(MutableStructOrError struct):
     }
 
 
+cdef _resetFieldToStandardDefault(structOrError, field_index):
+    if isinstance(structOrError, MutableStruct):
+        (<MutableStruct>structOrError)._fbthrift_reset_field_to_standard_default(field_index)
+    else:
+        (<MutableGeneratedError>structOrError)._fbthrift_reset_field_to_standard_default(field_index)
+
+
 class _MutableStructField:
     """
     The `_MutableStructField` class is a descriptor class that is used to
@@ -102,10 +109,11 @@ class _MutableStructField:
     """
     # `_field_index` is the insertion order of the field in the
     # `MutableStructInfo` (this is not the Thrift field id)
-    __slots__ = ('_field_index',)
+    __slots__ = ('_field_index', '_is_optional')
 
-    def __init__(self, field_id):
+    def __init__(self, field_id, is_optional):
         self._field_index = field_id
+        self._is_optional = is_optional
 
     def __get__(self, obj, objtype):
         if obj is None:
@@ -120,6 +128,11 @@ class _MutableStructField:
         if obj is None:
             return
 
+        if value is None and self._is_optional:
+            # reseting optional field to default is setting it to `None`
+            _resetFieldToStandardDefault(obj, self._field_index)
+            return
+
         if isinstance(obj, MutableStruct):
             (<MutableStruct>obj)._fbthrift_set_field_value(self._field_index, value)
         else:
@@ -136,10 +149,11 @@ cdef is_container(ThriftIdlType idl_type):
 
 
 class _MutableStructCachedField:
-    __slots__ = ('_field_index')
+    __slots__ = ('_field_index', '_is_optional')
 
-    def __init__(self, field_id):
+    def __init__(self, field_id, is_optional):
         self._field_index = field_id
+        self._is_optional = is_optional
 
     def __get__(self, obj, objtype):
         if obj is None:
@@ -154,6 +168,12 @@ class _MutableStructCachedField:
         if obj is None:
             return
 
+        if value is None and self._is_optional:
+            # reseting optional field to default is setting it to `None`
+            _resetFieldToStandardDefault(obj, self._field_index)
+            obj._fbthrift_field_cache[self._field_index] = None
+            return
+
         if isinstance(obj, MutableStruct):
             (<MutableStruct>obj)._fbthrift_set_field_value(self._field_index, value)
         else:
@@ -534,25 +554,31 @@ cdef object _mutable_struct_meta_new(cls, cls_name, bases, dct):
         # if field has an adapter or is not primitive type, consider
         # as "non-primitive"
         if field_info.adapter_info is not None or not field_info.is_primitive:
-            non_primitive_types.append(
-                (field_index, field_info.py_name, field_info.idl_type, field_info.adapter_info))
+            non_primitive_types.append((field_index,
+                                        field_info.py_name,
+                                        field_info.qualifier,
+                                        field_info.idl_type,
+                                        field_info.adapter_info)) 
         else:
-            primitive_types.append((field_index, field_info.py_name, field_info.idl_type))
+            primitive_types.append((field_index,
+                                    field_info.py_name,
+                                    field_info.qualifier,
+                                    field_info.idl_type))
 
     dct["__slots__"] = slots
     klass = type.__new__(cls, cls_name, bases, dct)
 
-    for field_index, field_name, *_ in primitive_types:
+    for field_index, field_name, field_qualifier, *_ in primitive_types:
         type.__setattr__(
             klass,
             field_name,
-            _MutableStructField(field_index),
+            _MutableStructField(field_index, field_qualifier == FieldQualifier.Optional),
         )
 
-    for field_index, field_name, idl_type, adapter_info in non_primitive_types:
-        field_descriptor = _MutableStructField(field_index)
+    for field_index, field_name, field_qualifier, idl_type, adapter_info in non_primitive_types:
+        field_descriptor = _MutableStructField(field_index, field_qualifier == FieldQualifier.Optional)
         if is_container(idl_type) or adapter_info is not None or is_cacheable_non_primitive(idl_type):
-            field_descriptor = _MutableStructCachedField(field_index)
+            field_descriptor = _MutableStructCachedField(field_index, field_qualifier == FieldQualifier.Optional)
 
         type.__setattr__(
             klass,

thrift/lib/python/test/mutable_structs.py

@@ -101,13 +101,16 @@ def _assert_field_behavior(
             overflow_value (optional): A value that, when set, should raise
                 an `OverflowError`. This is typically used for integral types.
         """
+        # If `expected_default_value` is `None`, field is optional
+        is_optional = expected_default_value is None
+
         # Check for the `expected_default_value`. The unqualified field should
         # never be `None`
-        if expected_default_value is not None:
+        if not is_optional:
             self.assertIsNotNone(getattr(struct, field_name))
             self.assertEqual(expected_default_value, getattr(struct, field_name))
             self.assertFalse(mutable_isset(struct)[field_name])
-        else:  # OPTIONAL
+        else:
             self.assertIsNone(getattr(struct, field_name))
             self.assertFalse(mutable_isset(struct)[field_name])
 
@@ -118,21 +121,28 @@ def _assert_field_behavior(
 
         # pyre-ignore[16]: internal, could be remove/replaced later
         struct._fbthrift_internal_resetFieldToStandardDefault(field_name)
-        if expected_default_value is not None:
+        if not is_optional:
             self.assertIsNotNone(getattr(struct, field_name))
             self.assertEqual(expected_default_value, getattr(struct, field_name))
             self.assertTrue(mutable_isset(struct)[field_name])
-        else:  # OPTIONAL
+        else:
             self.assertIsNone(getattr(struct, field_name))
             self.assertFalse(mutable_isset(struct)[field_name])
 
         # `del struct.field_name` raises a `AttributeError`
         with self.assertRaises(AttributeError):
             delattr(struct, field_name)
 
-        # Assigning `None` raises a `TypeError`
-        with self.assertRaises(TypeError):
+        # Assigning `None` raises a `TypeError` for non-optional fields
+        if not is_optional:
+            with self.assertRaises(TypeError):
+                setattr(struct, field_name, None)
+        else:
+            # For optional fields, assigning `None` resets the field
+            setattr(struct, field_name, value)
+            self.assertTrue(mutable_isset(struct)[field_name])
             setattr(struct, field_name, None)
+            self.assertFalse(mutable_isset(struct)[field_name])
 
         # Value with wrong type raises `TypeError`
         with self.assertRaises(TypeError):

thrift/test/thrift-python/struct_test.py

@@ -595,9 +595,6 @@ def test_to_mutable_python(self) -> None:
         self.assertIs(w_mutable, w_mutable._to_mutable_python())
 
     def test_create_and_assign_for_i32(self) -> None:
-        # This is the singular version of `test_create_and_assign_for_all_types`
-        # for the i32 type. It's more readable since it doesn't use the
-        # `verify_{qualified,optional}_helper` functions.
         s = TestStructAllThriftPrimitiveTypesMutable(unqualified_i32=11)
 
         # Check the value assigned during initialization
@@ -651,7 +648,95 @@ def test_create_and_assign_for_i32(self) -> None:
 
         # Boundary check for integral types
         with self.assertRaises(OverflowError):
-            s.unqualified_i32 = -(2**31 + 1)
+            s.unqualified_i32 = max_i32 + 1
+
+    def test_create_and_assign_for_optional_i32_and_optional_string(self) -> None:
+        s = TestStructAllThriftPrimitiveTypesMutable()
+
+        # Check optional fields are `None`
+        self.assertIsNone(s.optional_i32)
+        self.assertIsNone(s.optional_string)
+
+        # Set the values and read them back
+        s.optional_i32 = 23
+        s.optional_string = "thrift"
+
+        self.assertEqual(23, s.optional_i32)
+        self.assertEqual("thrift", s.optional_string)
+
+        # `del struct.field_name` raises a `AttributeError`
+        with self.assertRaises(AttributeError):
+            del s.optional_i32
+
+        with self.assertRaises(AttributeError):
+            del s.optional_string
+
+        # Assigning a value of the wrong type raises a `TypeError`
+        with self.assertRaisesRegex(TypeError, "is not a <class 'int'>"):
+            # pyre-ignore[8]: Intentional for test
+            s.optional_i32 = "This is not an integer"
+
+        with self.assertRaisesRegex(TypeError, "Expected type <class 'str'>"):
+            # pyre-ignore[8]: Intentional for test
+            s.optional_string = 42
+
+        self.assertEqual(23, s.optional_i32)
+        self.assertEqual("thrift", s.optional_string)
+
+        # Assigning `None` is reseting the optional field to `None`
+        s.optional_i32 = None
+        s.optional_string = None
+
+        self.assertIsNone(s.optional_i32)
+        self.assertIsNone(s.optional_string)
+
+        # Assigning a value of the wrong type raises a `TypeError` when fields
+        # are `None`
+        with self.assertRaisesRegex(TypeError, "is not a <class 'int'>"):
+            # pyre-ignore[8]: Intentional for test
+            s.optional_i32 = "This is not an integer"
+
+        with self.assertRaisesRegex(TypeError, "Expected type <class 'str'>"):
+            # pyre-ignore[8]: Intentional for test
+            s.optional_string = 42
+
+        # Boundary check for integral types
+        with self.assertRaises(OverflowError):
+            s.optional_i32 = -(2**31 + 1)
+
+    def test_create_and_assign_for_optional_container(self) -> None:
+        s = TestStructAllThriftContainerTypesMutable()
+
+        # Check optional field is `None`
+        self.assertIsNone(s.optional_list_i32)
+
+        # Set the value and read it back
+        s.optional_list_i32 = [1, 2, 3]
+        self.assertEqual([1, 2, 3], s.optional_list_i32)
+
+        # `del struct.field_name` raises a `AttributeError`
+        with self.assertRaises(AttributeError):
+            del s.optional_list_i32
+
+        # Assigning a value of the wrong type raises a `TypeError`
+        with self.assertRaisesRegex(TypeError, "is not a <class 'int'>"):
+            # pyre-ignore[8]: Intentional for test
+            s.optional_list_i32 = ["list", "with", "different", "type"]
+
+        self.assertEqual([1, 2, 3], s.optional_list_i32)
+
+        # Assigning `None` is reseting the optional field to `None`
+        s.optional_list_i32 = None
+
+        # Assigning a value of the wrong type raises a `TypeError` when fields
+        # are `None`
+        with self.assertRaisesRegex(TypeError, "is not a <class 'int'>"):
+            # pyre-ignore[8]: Intentional for test
+            s.optional_list_i32 = ["list", "with", "different", "type"]
+
+        # Boundary check for integral types
+        with self.assertRaises(OverflowError):
+            s.optional_list_i32 = [max_i32 + 1]
 
     def test_serialization_round_trip(self) -> None:
         s = TestStructAllThriftPrimitiveTypesMutable()
@@ -1363,6 +1448,7 @@ def test_call_as_deepcopy(self) -> None:
           7: map<string, i32> unqualified_map_string_i32;
 
           8: optional TestStructCopy recursive_struct;
+
           @cpp.Type{name = "folly::IOBuf"}
           9: binary unqualified_binary;
         }
@@ -1474,6 +1560,10 @@ def test_exception_deepcopy(self) -> None:
         self.assertEqual(e.unqualified_string, e_clone.unqualified_string)
         self.assertEqual(e.optional_string, e_clone.optional_string)
 
+        e_clone.optional_i32 = None
+        self.assertIsNone(e_clone.optional_i32)
+        self.assertIsNotNone(e.optional_i32)
+
         self.assertEqual(e.unqualified_list_i32, e_clone.unqualified_list_i32)
         self.assertIsNot(e.unqualified_list_i32, e_clone.unqualified_list_i32)
         self.assertEqual(e.optional_list_i32, e_clone.optional_list_i32)

thrift/test/thrift-python/struct_test.thrift

@@ -208,6 +208,7 @@ struct TestStructCopy {
   7: map<string, i32> unqualified_map_string_i32;
 
   8: optional TestStructCopy recursive_struct;
+
   @cpp.Type{name = "folly::IOBuf"}
   9: binary unqualified_binary;
 }