Fix class definition registrations with the same class id

When class definitions for two different classes with the same class id
but different factory ids are registered, we were throwing an error
indicating that there are duplicate registrations. However, we should
allow such cases.

Apart from the fix for that, the PR also includes a test case for
null portable serialization.

Closes #199

Author: mdumandag

hazelcast/serialization/portable/classdef.py

@@ -225,7 +225,8 @@ def build(self):
 
     def _add_field_by_type(self, field_name, field_type, version, factory_id=0, class_id=0):
         self._check()
-        self._field_defs.append(FieldDefinition(self._index, field_name, field_type, version, factory_id, class_id))
+        fd = FieldDefinition(self._index, field_name, field_type, version, factory_id, class_id)
+        self._field_defs.append(fd)
         self._index += 1
 
     def _check(self):

hazelcast/serialization/service.py

@@ -78,24 +78,40 @@ def _register_constant_serializers(self):
         self._registry.safe_register_serializer(self._registry._python_serializer)
 
     def register_class_definitions(self, class_definitions, check_error):
-        class_defs = dict()
+        factories = dict()
         for cd in class_definitions:
-            if cd in class_defs:
-                raise HazelcastSerializationError("Duplicate registration found for class-id: %s" % cd.class_id)
-            class_defs[cd.class_id] = cd
+            factory_id = cd.factory_id
+            class_defs = factories.get(factory_id, None)
+            if class_defs is None:
+                class_defs = dict()
+                factories[factory_id] = class_defs
+
+            class_id = cd.class_id
+            if class_id in class_defs:
+                raise HazelcastSerializationError("Duplicate registration found for class-id: %s" % class_id)
+            class_defs[class_id] = cd
+
         for cd in class_definitions:
-            self.register_class_definition(cd, class_defs, check_error)
+            self.register_class_definition(cd, factories, check_error)
 
-    def register_class_definition(self, cd, class_defs, check_error):
+    def register_class_definition(self, cd, factories, check_error):
         field_names = cd.get_field_names()
         for field_name in field_names:
             fd = cd.get_field(field_name)
             if fd.field_type == FieldType.PORTABLE or fd.field_type == FieldType.PORTABLE_ARRAY:
-                nested_cd = class_defs.get(fd.class_id, None)
-                if nested_cd is not None:
-                    self.register_class_definition(nested_cd, class_defs, check_error)
-                    self._portable_context.register_class_definition(nested_cd)
-                elif check_error:
+                factory_id = fd.factory_id
+                class_id = fd.class_id
+                class_defs = factories.get(factory_id, None)
+                if class_defs is not None:
+                    nested_cd = class_defs.get(class_id, None)
+                    if nested_cd is not None:
+                        self.register_class_definition(nested_cd, factories, check_error)
+                        self._portable_context.register_class_definition(nested_cd)
+                        continue
+
+                if check_error:
                     raise HazelcastSerializationError(
-                            "Could not find registered ClassDefinition for class-id: %s" % fd.class_id)
+                        "Could not find registered ClassDefinition for factory-id: %s, class-id: %s"
+                        % (factory_id, class_id))
+
         self._portable_context.register_class_definition(cd)

tests/serialization/portable_test.py

@@ -231,6 +231,51 @@ def create_portable():
 the_factory = {SerializationV1Portable.CLASS_ID: SerializationV1Portable, InnerPortable.CLASS_ID: InnerPortable}
 
 
+class MyPortable1(Portable):
+    def __init__(self, str_field=None):
+        self.str_field = str_field
+
+    def write_portable(self, writer):
+        writer.write_utf("str_field", self.str_field)
+
+    def read_portable(self, reader):
+        self.str_field = reader.read_utf("str_field")
+
+    def get_factory_id(self):
+        return 1
+
+    def get_class_id(self):
+        return 1
+
+    def __eq__(self, other):
+        return isinstance(other, MyPortable1) and self.str_field == other.str_field
+
+    def __ne__(self, other):
+        return not self.__eq__(other)
+
+
+class MyPortable2(Portable):
+    def __init__(self, int_field=0):
+        self.int_field = int_field
+
+    def write_portable(self, writer):
+        writer.write_int("int_field", self.int_field)
+
+    def read_portable(self, reader):
+        self.int_field = reader.read_int("int_field")
+
+    def get_factory_id(self):
+        return 2
+
+    def get_class_id(self):
+        return 1
+
+    def __eq__(self, other):
+        return isinstance(other, MyPortable2) and self.int_field == other.int_field
+
+    def __ne__(self, other):
+        return not self.__eq__(other)
+
 class PortableSerializationTestCase(unittest.TestCase):
     def test_encode_decode(self):
         config = _Config()
@@ -370,3 +415,61 @@ def test_nested_portable_serialization(self):
         data = ss1.to_data(p)
 
         self.assertEqual(p, ss2.to_object(data))
+
+    def test_nested_null_portable_serialization(self):
+        config = _Config()
+
+        config.portable_factories = {
+            1: {
+                1: Parent,
+                2: Child
+            }
+        }
+
+        child_class_def = ClassDefinitionBuilder(FACTORY_ID, 2).add_utf_field("name").build()
+        parent_class_def = ClassDefinitionBuilder(FACTORY_ID, 1).add_portable_field("child", child_class_def).build()
+
+        config.class_definitions = [child_class_def, parent_class_def]
+
+        ss = SerializationServiceV1(config)
+
+        p = Parent(None)
+        data = ss.to_data(p)
+
+        self.assertEqual(p, ss.to_object(data))
+
+    def test_duplicate_class_definition(self):
+        config = _Config()
+
+        class_def1 = ClassDefinitionBuilder(1, 1).add_utf_field("str_field").build()
+        class_def2 = ClassDefinitionBuilder(1, 1).add_int_field("int_field").build()
+
+        config.class_definitions = [class_def1, class_def2]
+
+        with self.assertRaises(HazelcastSerializationError):
+            SerializationServiceV1(config)
+
+    def test_classes_with_same_class_id_in_different_factories(self):
+        config = _Config()
+        config.portable_factories = {
+            1: {
+                1: MyPortable1
+            },
+            2: {
+                1: MyPortable2
+            }
+        }
+
+        class_def1 = ClassDefinitionBuilder(1, 1).add_utf_field("str_field").build()
+        class_def2 = ClassDefinitionBuilder(2, 1).add_int_field("int_field").build()
+
+        config.class_definitions = [class_def1, class_def2]
+        ss = SerializationServiceV1(config)
+
+        portable1 = MyPortable1("test")
+        data1 = ss.to_data(portable1)
+        self.assertEqual(portable1, ss.to_object(data1))
+
+        portable2 = MyPortable2(1)
+        data2 = ss.to_data(portable2)
+        self.assertEqual(portable2, ss.to_object(data2))