Replace metaclass-based registry with a decorator-based one

In nova, we used a metaclass to ensure that merely defining an object as
inheriting from the base would register it for use. I don't feel that is
a responsible way for the library to behave, since just importing a module
full of objects would allow things to be registered which might enable
RPC methods that are undesired.

Further, it complicated our ability to register alternate implementations
of certain objects, and provided no end of complication to our tests.

This patch adds a decorator-based object registry that looks much the same,
but requires explicit opt-in registration to objects before they can be
deserialized over RPC and instantiated by name.

Change-Id: Ic50ea4d59df7981eb8a413b5c80ca47155864166

oslo_versionedobjects/base.py

@@ -93,31 +93,26 @@ def make_class_properties(cls):
         setattr(cls, name, property(getter, setter))
 
 
-class NovaObjectMetaclass(type):
-    """Metaclass that allows tracking of object classes."""
+class VersionedObjectRegistry(object):
+    _registry = None
 
-    # NOTE(danms): This is what controls whether object operations are
-    # remoted. If this is not None, use it to remote things over RPC.
-    indirection_api = None
-
-    def __init__(cls, names, bases, dict_):
-        if not hasattr(cls, '_obj_classes'):
-            # This means this is a base class using the metaclass. I.e.,
-            # the 'NovaObject' class.
-            cls._obj_classes = collections.defaultdict(list)
-            return
+    def __new__(cls, *args, **kwargs):
+        if not VersionedObjectRegistry._registry:
+            VersionedObjectRegistry._registry = \
+                object.__new__(cls, *args, **kwargs)
+            VersionedObjectRegistry._registry._obj_classes = \
+                collections.defaultdict(list)
+        return VersionedObjectRegistry._registry
 
+    def _register_class(self, cls):
         def _vers_tuple(obj):
             return tuple([int(x) for x in obj.VERSION.split(".")])
 
-        # Add the subclass to NovaObject._obj_classes. If the
-        # same version already exists, replace it. Otherwise,
-        # keep the list with newest version first.
         make_class_properties(cls)
         obj_name = cls.obj_name()
-        for i, obj in enumerate(cls._obj_classes[obj_name]):
+        for i, obj in enumerate(self._obj_classes[obj_name]):
             if cls.VERSION == obj.VERSION:
-                cls._obj_classes[obj_name][i] = cls
+                self._obj_classes[obj_name][i] = cls
                 # Update nova.objects with this newer class.
                 # FIXME(dhellmann): We can't store library state in
                 # the application module.
@@ -125,7 +120,7 @@ class NovaObjectMetaclass(type):
                 break
             if _vers_tuple(cls) > _vers_tuple(obj):
                 # Insert before.
-                cls._obj_classes[obj_name].insert(i, cls)
+                self._obj_classes[obj_name].insert(i, cls)
                 # FIXME(dhellmann): We can't store library state in
                 # the application module.
                 # if i == 0:
@@ -134,7 +129,7 @@ class NovaObjectMetaclass(type):
                 #     setattr(objects, obj_name, cls)
                 break
         else:
-            cls._obj_classes[obj_name].append(cls)
+            self._obj_classes[obj_name].append(cls)
             # Either this is the first time we've seen the object or it's
             # an older version than anything we'e seen. Update nova.objects
             # only if it's the first time we've seen this object name.
@@ -143,6 +138,17 @@ class NovaObjectMetaclass(type):
             # if not hasattr(objects, obj_name):
             #     setattr(objects, obj_name, cls)
 
+    @classmethod
+    def register(cls, obj_cls):
+        registry = cls()
+        registry._register_class(obj_cls)
+        return obj_cls
+
+    @classmethod
+    def obj_classes(cls):
+        registry = cls()
+        return registry._obj_classes
+
 
 # These are decorators that mark an object's method as remotable.
 # If the metaclass is configured to forward object methods to an
@@ -215,7 +221,6 @@ def remotable(fn):
     return wrapper
 
 
-@six.add_metaclass(NovaObjectMetaclass)
 class NovaObject(object):
     """Base class and object factory.
 
@@ -299,7 +304,7 @@ class NovaObject(object):
     @classmethod
     def obj_class_from_name(cls, objname, objver):
         """Returns a class from the registry based on a name and version."""
-        if objname not in cls._obj_classes:
+        if objname not in VersionedObjectRegistry.obj_classes():
             LOG.error(_LE('Unable to instantiate unregistered object type '
                           '%(objtype)s'), dict(objtype=objname))
             raise exception.UnsupportedObjectError(objtype=objname)
@@ -310,7 +315,7 @@ class NovaObject(object):
         # once below.
         compatible_match = None
 
-        for objclass in cls._obj_classes[objname]:
+        for objclass in VersionedObjectRegistry.obj_classes()[objname]:
             if objclass.VERSION == objver:
                 return objclass
             if (not compatible_match and
@@ -321,7 +326,7 @@ class NovaObject(object):
             return compatible_match
 
         # As mentioned above, latest version is always first in the list.
-        latest_ver = cls._obj_classes[objname][0].VERSION
+        latest_ver = VersionedObjectRegistry.obj_classes()[objname][0].VERSION
         raise exception.IncompatibleObjectVersion(objname=objname,
                                                   objver=objver,
                                                   supported=latest_ver)

oslo_versionedobjects/test.py

@@ -24,7 +24,6 @@ inline callbacks.
 import eventlet
 eventlet.monkey_patch(os=False)
 
-import copy
 import inspect
 import mock
 import os
@@ -43,7 +42,6 @@ import testtools
 #from nova import db
 #from nova.network import manager as network_manager
 #from nova import objects
-from oslo_versionedobjects import base as objects_base
 from oslo_versionedobjects.tests import obj_fixtures
 from oslo_versionedobjects import utils
 
@@ -177,14 +175,6 @@ class TestCase(testtools.TestCase):
         # because sqlalchemy-migrate messes with the warnings filters.
         self.useFixture(obj_fixtures.WarningsFixture())
 
-        # NOTE(danms): Make sure to reset us back to non-remote objects
-        # for each test to avoid interactions. Also, backup the object
-        # registry.
-        objects_base.NovaObject.indirection_api = None
-        self._base_test_obj_backup = copy.copy(
-            objects_base.NovaObject._obj_classes)
-        self.addCleanup(self._restore_obj_registry)
-
         # NOTE(mnaser): All calls to utils.is_neutron() are cached in
         # nova.utils._IS_NEUTRON.  We set it to None to avoid any
         # caching of that value.
@@ -196,9 +186,6 @@ class TestCase(testtools.TestCase):
         self.addCleanup(self._clear_attrs)
         self.useFixture(fixtures.EnvironmentVariable('http_proxy'))
 
-    def _restore_obj_registry(self):
-        objects_base.NovaObject._obj_classes = self._base_test_obj_backup
-
     def _clear_attrs(self):
         # Delete attributes that don't start with _ so they don't pin
         # memory around unnecessarily for the duration of the test

oslo_versionedobjects/tests/test_fields.py

@@ -331,6 +331,7 @@ class TestObject(TestField):
     def setUp(self):
         super(TestObject, self).setUp()
 
+        @obj_base.VersionedObjectRegistry.register
         class TestableObject(obj_base.NovaObject):
             fields = {
                 'uuid': fields.StringField(),

oslo_versionedobjects/tests/test_objects.py

@@ -25,7 +25,6 @@ import mock
 from oslo_context import context
 from oslo_serialization import jsonutils
 from oslo_utils import timeutils
-import six
 from testtools import matchers
 
 # from nova.conductor import rpcapi as conductor_rpcapi
@@ -41,11 +40,21 @@ from oslo_versionedobjects import utils
 LOG = logging.getLogger(__name__)
 
 
+def is_test_object(cls):
+    """Return True if class is defined in the tests.
+
+    :param cls: Class to inspect
+    """
+    return 'oslo_versionedobjects.tests' in cls.__module__
+
+
+@base.VersionedObjectRegistry.register
 class MyOwnedObject(base.NovaPersistentObject, base.NovaObject):
     VERSION = '1.0'
     fields = {'baz': fields.Field(fields.Integer())}
 
 
+@base.VersionedObjectRegistry.register
 class MyObj(base.NovaPersistentObject, base.NovaObject,
             base.NovaObjectDictCompat):
     VERSION = '1.6'
@@ -115,6 +124,7 @@ class MyObj(base.NovaPersistentObject, base.NovaObject,
             primitive['bar'] = 'old%s' % primitive['bar']
 
 
+@base.VersionedObjectRegistry.register
 class MyObjDiffVers(MyObj):
     VERSION = '1.5'
 
@@ -123,7 +133,8 @@ class MyObjDiffVers(MyObj):
         return 'MyObj'
 
 
-class MyObj2(object):
+@base.VersionedObjectRegistry.register
+class MyObj2(base.NovaObject):
     @classmethod
     def obj_name(cls):
         return 'MyObj'
@@ -138,14 +149,15 @@ class RandomMixInWithNoFields(object):
     pass
 
 
+@base.VersionedObjectRegistry.register
 class TestSubclassedObject(RandomMixInWithNoFields, MyObj):
     fields = {'new_field': fields.Field(fields.String())}
 
 
-class TestMetaclass(test.TestCase):
+class TestRegistry(test.TestCase):
     def test_obj_tracking(self):
 
-        @six.add_metaclass(base.NovaObjectMetaclass)
+        @base.VersionedObjectRegistry.register
         class NewBaseClass(object):
             VERSION = '1.0'
             fields = {}
@@ -154,28 +166,35 @@ class TestMetaclass(test.TestCase):
             def obj_name(cls):
                 return cls.__name__
 
+        @base.VersionedObjectRegistry.register
         class Fake1TestObj1(NewBaseClass):
             @classmethod
             def obj_name(cls):
                 return 'fake1'
 
+        @base.VersionedObjectRegistry.register
         class Fake1TestObj2(Fake1TestObj1):
             pass
 
+        @base.VersionedObjectRegistry.register
         class Fake1TestObj3(Fake1TestObj1):
             VERSION = '1.1'
 
+        @base.VersionedObjectRegistry.register
         class Fake2TestObj1(NewBaseClass):
             @classmethod
             def obj_name(cls):
                 return 'fake2'
 
+        @base.VersionedObjectRegistry.register
         class Fake1TestObj4(Fake1TestObj3):
             VERSION = '1.2'
 
+        @base.VersionedObjectRegistry.register
         class Fake2TestObj2(Fake2TestObj1):
             VERSION = '1.1'
 
+        @base.VersionedObjectRegistry.register
         class Fake1TestObj5(Fake1TestObj1):
             VERSION = '1.1'
 
@@ -183,18 +202,14 @@ class TestMetaclass(test.TestCase):
         # newest object.
         expected = {'fake1': [Fake1TestObj4, Fake1TestObj5, Fake1TestObj2],
                     'fake2': [Fake2TestObj2, Fake2TestObj1]}
-        self.assertEqual(expected, NewBaseClass._obj_classes)
-        # The following should work, also.
-        self.assertEqual(expected, Fake1TestObj1._obj_classes)
-        self.assertEqual(expected, Fake1TestObj2._obj_classes)
-        self.assertEqual(expected, Fake1TestObj3._obj_classes)
-        self.assertEqual(expected, Fake1TestObj4._obj_classes)
-        self.assertEqual(expected, Fake1TestObj5._obj_classes)
-        self.assertEqual(expected, Fake2TestObj1._obj_classes)
-        self.assertEqual(expected, Fake2TestObj2._obj_classes)
+        self.assertEqual(expected['fake1'],
+                         base.VersionedObjectRegistry.obj_classes()['fake1'])
+        self.assertEqual(expected['fake2'],
+                         base.VersionedObjectRegistry.obj_classes()['fake2'])
 
     def test_field_checking(self):
         def create_class(field):
+            @base.VersionedObjectRegistry.register
             class TestField(base.NovaObject):
                 VERSION = '1.5'
                 fields = {'foo': field()}
@@ -210,6 +225,7 @@ class TestMetaclass(test.TestCase):
 class TestObjToPrimitive(test.TestCase):
 
     def test_obj_to_primitive_list(self):
+        @base.VersionedObjectRegistry.register
         class MyObjElement(base.NovaObject):
             fields = {'foo': fields.IntegerField()}
 
@@ -217,6 +233,7 @@ class TestObjToPrimitive(test.TestCase):
                 super(MyObjElement, self).__init__()
                 self.foo = foo
 
+        @base.VersionedObjectRegistry.register
         class MyList(base.ObjectListBase, base.NovaObject):
             fields = {'objects': fields.ListOfObjectsField('MyObjElement')}
 
@@ -231,6 +248,7 @@ class TestObjToPrimitive(test.TestCase):
                          base.obj_to_primitive(myobj))
 
     def test_obj_to_primitive_recursive(self):
+        @base.VersionedObjectRegistry.register
         class MyList(base.ObjectListBase, base.NovaObject):
             fields = {'objects': fields.ListOfObjectsField('MyObj')}
 
@@ -241,6 +259,7 @@ class TestObjToPrimitive(test.TestCase):
                          base.obj_to_primitive(mylist))
 
     def test_obj_to_primitive_with_ip_addr(self):
+        @base.VersionedObjectRegistry.register
         class TestObject(base.NovaObject):
             fields = {'addr': fields.IPAddressField(),
                       'cidr': fields.IPNetworkField()}
@@ -253,6 +272,7 @@ class TestObjToPrimitive(test.TestCase):
 class TestObjMakeList(test.TestCase):
 
     def test_obj_make_list(self):
+        @base.VersionedObjectRegistry.register
         class MyList(base.ObjectListBase, base.NovaObject):
             pass
 
@@ -518,6 +538,7 @@ class _TestObject(object):
         self.assertEqual(obj.bar, 'loaded!')
 
     def test_load_in_base(self):
+        @base.VersionedObjectRegistry.register
         class Foo(base.NovaObject):
             fields = {'foobar': fields.Field(fields.Integer())}
         obj = Foo()
@@ -623,6 +644,7 @@ class _TestObject(object):
         self.assertRemotes()
 
     def test_changed_with_sub_object(self):
+        @base.VersionedObjectRegistry.register
         class ParentObject(base.NovaObject):
             fields = {'foo': fields.IntegerField(),
                       'bar': fields.ObjectField('MyObj'),
@@ -909,6 +931,7 @@ class TestRemoteObject(_RemoteTest, _TestObject):
 
 class TestObjectListBase(test.TestCase):
     def test_list_like_operations(self):
+        @base.VersionedObjectRegistry.register
         class MyElement(base.NovaObject):
             fields = {'foo': fields.IntegerField()}
 
@@ -934,9 +957,11 @@ class TestObjectListBase(test.TestCase):
                          [x.foo for x in objlist])
 
     def test_serialization(self):
+        @base.VersionedObjectRegistry.register
         class Foo(base.ObjectListBase, base.NovaObject):
             fields = {'objects': fields.ListOfObjectsField('Bar')}
 
+        @base.VersionedObjectRegistry.register
         class Bar(base.NovaObject):
             fields = {'foo': fields.Field(fields.String())}
 
@@ -958,7 +983,10 @@ class TestObjectListBase(test.TestCase):
 
         # Look through all object classes of this type and make sure that
         # the versions we find are covered by the parent list class
-        for item_class in base.NovaObject._obj_classes[item_obj_name]:
+        obj_classes = base.VersionedObjectRegistry.obj_classes()[item_obj_name]
+        for item_class in obj_classes:
+            if is_test_object(item_class):
+                continue
             self.assertIn(
                 item_class.VERSION,
                 list_obj_class.child_versions.values(),
@@ -968,15 +996,17 @@ class TestObjectListBase(test.TestCase):
     def test_object_version_mappings(self):
         # Find all object list classes and make sure that they at least handle
         # all the current object versions
-        for obj_classes in base.NovaObject._obj_classes.values():
+        for obj_classes in base.VersionedObjectRegistry.obj_classes().values():
             for obj_class in obj_classes:
                 if issubclass(obj_class, base.ObjectListBase):
                     self._test_object_list_version_mappings(obj_class)
 
     def test_list_changes(self):
+        @base.VersionedObjectRegistry.register
         class Foo(base.ObjectListBase, base.NovaObject):
             fields = {'objects': fields.ListOfObjectsField('Bar')}
 
+        @base.VersionedObjectRegistry.register
         class Bar(base.NovaObject):
             fields = {'foo': fields.StringField()}
 
@@ -1003,9 +1033,11 @@ class TestObjectListBase(test.TestCase):
         self.assertEqual(set(), obj.obj_what_changed())
 
     def test_obj_repr(self):
+        @base.VersionedObjectRegistry.register
         class Foo(base.ObjectListBase, base.NovaObject):
             fields = {'objects': fields.ListOfObjectsField('Bar')}
 
+        @base.VersionedObjectRegistry.register
         class Bar(base.NovaObject):
             fields = {'uuid': fields.StringField()}
 
@@ -1034,6 +1066,7 @@ class TestObjectSerializer(_BaseTestCase):
         ser._conductor = mock.Mock()
         ser._conductor.object_backport.return_value = 'backported'
 
+        @base.VersionedObjectRegistry.register
         class MyTestObj(MyObj):
             VERSION = my_version
 
@@ -1128,7 +1161,7 @@ object_data = {
     'MyOwnedObject': '1.0-fec853730bd02d54cc32771dd67f08a0',
     'TestSubclassedObject': '1.6-6c1976a36987b9832b3183a7d9163655',
 }
-
+object_data = {}
 
 object_relationships = {
     'BlockDeviceMapping': {'Instance': '1.18'},
@@ -1170,7 +1203,7 @@ class TestObjectVersions(test.TestCase):
             return None
 
     def _get_fingerprint(self, obj_name):
-        obj_class = base.NovaObject._obj_classes[obj_name][0]
+        obj_class = base.VersionedObjectRegistry.obj_classes()[obj_name][0]
         fields = obj_class.fields.items()
         fields.sort()
         methods = []
@@ -1196,7 +1229,11 @@ class TestObjectVersions(test.TestCase):
 
     def test_versions(self):
         fingerprints = {}
-        for obj_name in base.NovaObject._obj_classes:
+        for obj_name in base.VersionedObjectRegistry.obj_classes():
+            obj_classes = base.VersionedObjectRegistry._registry._obj_classes
+            obj_cls = obj_classes[obj_name][0]
+            if is_test_object(obj_cls):
+                continue
             fingerprints[obj_name] = self._get_fingerprint(obj_name)
 
         if os.getenv('GENERATE_HASHES'):
@@ -1227,7 +1264,8 @@ class TestObjectVersions(test.TestCase):
         for name, field in obj_class.fields.items():
             if isinstance(field._type, fields.Object):
                 sub_obj_name = field._type._obj_name
-                sub_obj_class = base.NovaObject._obj_classes[sub_obj_name][0]
+                obj_classes = base.VersionedObjectRegistry.obj_classes()
+                sub_obj_class = obj_classes[sub_obj_name][0]
                 self._build_tree(tree, sub_obj_class)
                 tree.setdefault(obj_name, {})
                 tree[obj_name][sub_obj_name] = sub_obj_class.VERSION
@@ -1235,8 +1273,9 @@ class TestObjectVersions(test.TestCase):
     def test_relationships(self):
         self.skip('relationship test needs to be rewritten')
         tree = {}
-        for obj_name in base.NovaObject._obj_classes.keys():
-            self._build_tree(tree, base.NovaObject._obj_classes[obj_name][0])
+        obj_classes = base.VersionedObjectRegistry.obj_classes()
+        for obj_name in base.VersionedObjectRegistry.obj_classes().keys():
+            self._build_tree(tree, obj_classes[obj_name][0])
 
         stored = set([(x, str(y)) for x, y in object_relationships.items()])
         computed = set([(x, str(y)) for x, y in tree.items()])
@@ -1259,8 +1298,8 @@ class TestObjectVersions(test.TestCase):
         # This doesn't actually test the data conversions, but it at least
         # makes sure the method doesn't blow up on something basic like
         # expecting the wrong version format.
-        for obj_name in base.NovaObject._obj_classes:
-            obj_class = base.NovaObject._obj_classes[obj_name][0]
+        for obj_name in base.VersionedObjectRegistry.obj_classes():
+            obj_class = base.VersionedObjectRegistry.obj_classes()[obj_name][0]
             version = utils.convert_version_to_tuple(obj_class.VERSION)
             for n in range(version[1]):
                 test_version = '%d.%d' % (version[0], n)
@@ -1274,8 +1313,8 @@ class TestObjectVersions(test.TestCase):
         # This doesn't actually test the data conversions, but it at least
         # makes sure the method doesn't blow up on something basic like
         # expecting the wrong version format.
-        for obj_name in base.NovaObject._obj_classes:
-            obj_class = base.NovaObject._obj_classes[obj_name][0]
+        for obj_name in base.VersionedObjectRegistry.obj_classes():
+            obj_class = base.VersionedObjectRegistry.obj_classes()[obj_name][0]
             for field, versions in obj_class.obj_relationships.items():
                 last_my_version = (0, 0)
                 last_child_version = (0, 0)