Revert "Revert "THRIFT-4002: Make generated exception classes immutable by default""

This reverts commit 1234ddf8a5c98d5d700c82e087f04725170ad581.

7 files changed, 59 insertions, 13 deletions

diff --git a/compiler/cpp/src/thrift/generate/t_py_generator.cc b/compiler/cpp/src/thrift/generate/t_py_generator.cc
index 982bca145..e93bbe17d 100644
--- a/compiler/cpp/src/thrift/generate/t_py_generator.cc
+++ b/compiler/cpp/src/thrift/generate/t_py_generator.cc
@@ -65,6 +65,7 @@ public:
     coding_ = "";
     gen_dynbaseclass_ = "";
     gen_dynbaseclass_exc_ = "";
+    gen_dynbaseclass_frozen_exc_ = "";
     gen_dynbaseclass_frozen_ = "";
     import_dynbase_ = "";
     package_prefix_ = "";
@@ -94,8 +95,11 @@ public:
         if( gen_dynbaseclass_exc_.empty()) {
           gen_dynbaseclass_exc_ = "TExceptionBase";
         }
+        if( gen_dynbaseclass_frozen_exc_.empty()) {
+          gen_dynbaseclass_frozen_exc_ = "TFrozenExceptionBase";
+        }
         if( import_dynbase_.empty()) {
-          import_dynbase_ = "from thrift.protocol.TBase import TBase, TFrozenBase, TExceptionBase, TTransport\n";
+          import_dynbase_ = "from thrift.protocol.TBase import TBase, TFrozenBase, TExceptionBase, TFrozenExceptionBase, TTransport\n";
         }
       } else if( iter->first.compare("dynbase") == 0) {
         gen_dynbase_ = true;
@@ -104,6 +108,8 @@ public:
         gen_dynbaseclass_frozen_ = (iter->second);
       } else if( iter->first.compare("dynexc") == 0) {
         gen_dynbaseclass_exc_ = (iter->second);
+      } else if( iter->first.compare("dynfrozenexc") == 0) {
+        gen_dynbaseclass_frozen_exc_ = (iter->second);
       } else if( iter->first.compare("dynimport") == 0) {
         gen_dynbase_ = true;
         import_dynbase_ = (iter->second);
@@ -269,7 +275,16 @@ public:
   }
 
   static bool is_immutable(t_type* ttype) {
-    return ttype->annotations_.find("python.immutable") != ttype->annotations_.end();
+    std::map<std::string, std::string>::iterator it = ttype->annotations_.find("python.immutable");
+
+    if (it == ttype->annotations_.end()) {
+      // Exceptions are immutable by default.
+      return ttype->is_xception();
+    } else if (it->second == "false") {
+      return false;
+    } else {
+      return true;
+    }
   }
 
 private:
@@ -288,6 +303,7 @@ private:
   std::string gen_dynbaseclass_;
   std::string gen_dynbaseclass_frozen_;
   std::string gen_dynbaseclass_exc_;
+  std::string gen_dynbaseclass_frozen_exc_;
 
   std::string import_dynbase_;
 
@@ -742,7 +758,11 @@ void t_py_generator::generate_py_struct_definition(ostream& out,
   out << endl << endl << "class " << tstruct->get_name();
   if (is_exception) {
     if (gen_dynamic_) {
-      out << "(" << gen_dynbaseclass_exc_ << ")";
+      if (is_immutable(tstruct)) {
+        out << "(" << gen_dynbaseclass_frozen_exc_ << ")";
+      } else {
+        out << "(" << gen_dynbaseclass_exc_ << ")";
+      }
     } else {
       out << "(TException)";
     }
@@ -2774,6 +2794,7 @@ THRIFT_REGISTER_GENERATOR(
     "    dynbase=CLS      Derive generated classes from class CLS instead of TBase.\n"
     "    dynfrozen=CLS    Derive generated immutable classes from class CLS instead of TFrozenBase.\n"
     "    dynexc=CLS       Derive generated exceptions from CLS instead of TExceptionBase.\n"
+    "    dynfrozenexc=CLS Derive generated immutable exceptions from CLS instead of TFrozenExceptionBase.\n"
     "    dynimport='from foo.bar import CLS'\n"
     "                     Add an import line to generated code to find the dynbase class.\n"
     "    package_prefix='top.package.'\n"
diff --git a/lib/py/src/protocol/TBase.py b/lib/py/src/protocol/TBase.py
index 9ae1b1182..6c6ef18e8 100644
--- a/lib/py/src/protocol/TBase.py
+++ b/lib/py/src/protocol/TBase.py
@@ -80,3 +80,7 @@ class TFrozenBase(TBase):
                                       [self.__class__, self.thrift_spec])
         else:
             return iprot.readStruct(cls, cls.thrift_spec, True)
+
+
+class TFrozenExceptionBase(TFrozenBase, TExceptionBase):
+    pass
diff --git a/lib/py/src/protocol/TProtocol.py b/lib/py/src/protocol/TProtocol.py
index 3456e8f0e..339a2839d 100644
--- a/lib/py/src/protocol/TProtocol.py
+++ b/lib/py/src/protocol/TProtocol.py
@@ -303,8 +303,14 @@ class TProtocolBase(object):
 
     def readContainerStruct(self, spec):
         (obj_class, obj_spec) = spec
-        obj = obj_class()
-        obj.read(self)
+
+        # If obj_class.read is a classmethod (e.g. in frozen structs),
+        # call it as such.
+        if getattr(obj_class.read, '__self__', None) is obj_class:
+            obj = obj_class.read(self)
+        else:
+            obj = obj_class()
+            obj.read(self)
         return obj
 
     def readContainerMap(self, spec):
diff --git a/test/DebugProtoTest.thrift b/test/DebugProtoTest.thrift
index de47ea717..1ab0f6aea 100644
--- a/test/DebugProtoTest.thrift
+++ b/test/DebugProtoTest.thrift
@@ -241,6 +241,10 @@ exception ExceptionWithAMap {
   2: map<string, string> map_field;
 }
 
+exception MutableException {
+  1: string msg;
+} (python.immutable = "false")
+
 service ServiceForExceptionWithAMap {
   void methodThatThrowsAnException() throws (1: ExceptionWithAMap xwamap);
 }
diff --git a/test/py.tornado/test_suite.py b/test/py.tornado/test_suite.py
index 447fde61b..0ee0a9b85 100755
--- a/test/py.tornado/test_suite.py
+++ b/test/py.tornado/test_suite.py
@@ -82,10 +82,7 @@ class TestHandler(object):
 
     def testException(self, s):
         if s == 'Xception':
-            x = Xception()
-            x.errorCode = 1001
-            x.message = s
-            raise x
+            raise Xception(1001, s)
         elif s == 'throw_undeclared':
             raise ValueError('testing undeclared exception')
 
diff --git a/test/py.twisted/test_suite.py b/test/py.twisted/test_suite.py
index 02eb7f14f..6e044939b 100755
--- a/test/py.twisted/test_suite.py
+++ b/test/py.twisted/test_suite.py
@@ -76,10 +76,7 @@ class TestHandler:
 
     def testException(self, s):
         if s == 'Xception':
-            x = Xception()
-            x.errorCode = 1001
-            x.message = s
-            raise x
+            raise Xception(1001, s)
         elif s == "throw_undeclared":
             raise ValueError("foo")
 
diff --git a/test/py/TestFrozen.py b/test/py/TestFrozen.py
index 6d2595cf2..ce7425f88 100755
--- a/test/py/TestFrozen.py
+++ b/test/py/TestFrozen.py
@@ -19,7 +19,9 @@
 # under the License.
 #
 
+from DebugProtoTest import Srv
 from DebugProtoTest.ttypes import CompactProtoTestStruct, Empty, Wrapper
+from DebugProtoTest.ttypes import ExceptionWithAMap, MutableException
 from thrift.Thrift import TFrozenDict
 from thrift.transport import TTransport
 from thrift.protocol import TBinaryProtocol, TCompactProtocol
@@ -94,6 +96,21 @@ class TestFrozenBase(unittest.TestCase):
         x2 = self._roundtrip(x, Wrapper)
         self.assertEqual(x2.foo, Empty())
 
+    def test_frozen_exception(self):
+        exc = ExceptionWithAMap(blah='foo')
+        with self.assertRaises(TypeError):
+            exc.blah = 'bar'
+        mutexc = MutableException(msg='foo')
+        mutexc.msg = 'bar'
+        self.assertEqual(mutexc.msg, 'bar')
+
+    def test_frozen_exception_serialization(self):
+        result = Srv.declaredExceptionMethod_result(
+            xwamap=ExceptionWithAMap(blah="error"))
+        deserialized = self._roundtrip(
+            result, Srv.declaredExceptionMethod_result())
+        self.assertEqual(result, deserialized)
+
 
 class TestFrozen(TestFrozenBase):
     def protocol(self, trans):