models: Add ValidatedImmutableObject and "revert" ImmutableObject

Testing with extension that use custom models it was discovered that the
changes to have type safe models were a bit to invasive to be suitable for a
minor release. This change fixes this by bringing back ImmutableObjects in
their old form, and moving the shinny new features to ValidatedImmutableObject.

A subset of the tests for ImmutableObjects have been resurrected to have some
confidence in this working the way we think it should.

mopidy/models/__init__.py

@@ -1,15 +1,16 @@
 from __future__ import absolute_import, unicode_literals
 
 from mopidy.models import fields
-from mopidy.models.immutable import ImmutableObject
+from mopidy.models.immutable import ImmutableObject, ValidatedImmutableObject
 from mopidy.models.serialize import ModelJSONEncoder, model_json_decoder
 
 __all__ = [
     'ImmutableObject', 'Ref', 'Image', 'Artist', 'Album', 'track', 'TlTrack',
-    'Playlist', 'SearchResult', 'model_json_decoder', 'ModelJSONEncoder']
+    'Playlist', 'SearchResult', 'model_json_decoder', 'ModelJSONEncoder',
+    'ValidatedImmutableObject']
 
 
-class Ref(ImmutableObject):
+class Ref(ValidatedImmutableObject):
 
     """
     Model to represent URI references with a human friendly name and type
@@ -81,7 +82,7 @@ class Ref(ImmutableObject):
         return cls(**kwargs)
 
 
-class Image(ImmutableObject):
+class Image(ValidatedImmutableObject):
 
     """
     :param string uri: URI of the image
@@ -99,7 +100,7 @@ class Image(ImmutableObject):
     height = fields.Integer(min=0)
 
 
-class Artist(ImmutableObject):
+class Artist(ValidatedImmutableObject):
 
     """
     :param uri: artist URI
@@ -120,7 +121,7 @@ class Artist(ImmutableObject):
     musicbrainz_id = fields.Identifier()
 
 
-class Album(ImmutableObject):
+class Album(ValidatedImmutableObject):
 
     """
     :param uri: album URI
@@ -169,7 +170,7 @@ class Album(ImmutableObject):
     # actual usage of this field with more than one image.
 
 
-class Track(ImmutableObject):
+class Track(ValidatedImmutableObject):
 
     """
     :param uri: track URI
@@ -253,7 +254,7 @@ class Track(ImmutableObject):
     last_modified = fields.Integer(min=0)
 
 
-class TlTrack(ImmutableObject):
+class TlTrack(ValidatedImmutableObject):
 
     """
     A tracklist track. Wraps a regular track and it's tracklist ID.
@@ -292,7 +293,7 @@ class TlTrack(ImmutableObject):
         return iter([self.tlid, self.track])
 
 
-class Playlist(ImmutableObject):
+class Playlist(ValidatedImmutableObject):
 
     """
     :param uri: playlist URI
@@ -329,7 +330,7 @@ class Playlist(ImmutableObject):
         return len(self.tracks)
 
 
-class SearchResult(ImmutableObject):
+class SearchResult(ValidatedImmutableObject):
 
     """
     :param uri: search result URI

mopidy/models/fields.py

@@ -4,8 +4,9 @@ from __future__ import absolute_import, unicode_literals
 class Field(object):
 
     """
-    Base field for use in :class:`ImmutableObject`. These fields are
+    Base field for use in
-    responsible for type checking and other data sanitation in our models.
+    :class:`~mopidy.models.immutable.ValidatedImmutableObject`. These fields
+    are responsible for type checking and other data sanitation in our models.
 
     For simplicity fields use the Python descriptor protocol to store the
     values in the instance dictionary. Also note that fields are mutable if
@@ -19,7 +20,7 @@ class Field(object):
     """
 
     def __init__(self, default=None, type=None, choices=None):
-        self._name = None  # Set by ImmutableObjectMeta
+        self._name = None  # Set by ValidatedImmutableObjectMeta
         self._choices = choices
         self._default = default
         self._type = type

mopidy/models/immutable.py

@@ -8,71 +8,54 @@ from mopidy.models.fields import Field
 from mopidy.utils import deprecation
 
 
-class ImmutableObjectMeta(type):
-
-    """Helper to automatically assign field names to descriptors."""
-
-    def __new__(cls, name, bases, attrs):
-        fields = {}
-        for base in bases:  # Copy parent fields over to our state
-            fields.update(getattr(base, '_fields', {}))
-        for key, value in attrs.items():  # Add our own fields
-            if isinstance(value, Field):
-                fields[key] = '_' + key
-                value._name = key
-
-        attrs['_fields'] = fields
-        attrs['_instances'] = weakref.WeakValueDictionary()
-        attrs['__slots__'] = list(attrs.get('__slots__', []))
-        attrs['__slots__'].extend(['_hash'] + fields.values())
-
-        return super(ImmutableObjectMeta, cls).__new__(cls, name, bases, attrs)
-
-    def __call__(cls, *args, **kwargs):  # noqa: N805
-        instance = super(ImmutableObjectMeta, cls).__call__(*args, **kwargs)
-        return cls._instances.setdefault(weakref.ref(instance), instance)
-
-
 class ImmutableObject(object):
-
     """
     Superclass for immutable objects whose fields can only be modified via the
-    constructor. Fields should be :class:`Field` instances to ensure type
+    constructor.
-    safety in our models.
 
-    Note that since these models can not be changed, we heavily memoize them
+    This version of this class has been retained to avoid breaking any clients
-    to save memory. So constructing a class with the same arguments twice will
+    relying on it's behavior. Internally in Mopidy we now use
-    give you the same instance twice.
+    :class:`ValidatedImmutableObject` for type safety and it's much smaller
+    memory footprint.
 
     :param kwargs: kwargs to set as fields on the object
     :type kwargs: any
     """
 
-    __metaclass__ = ImmutableObjectMeta
+    # Any sub-classes that don't set slots won't be effected by the base using
+    # slots as they will still get an instance dict.
     __slots__ = ['__weakref__']
 
     def __init__(self, *args, **kwargs):
         for key, value in kwargs.items():
-            if key not in self._fields:
+            if not self._is_valid_field(key):
                 raise TypeError(
-                    '__init__() got an unexpected keyword argument "%s"' %
+                    '__init__() got an unexpected keyword argument "%s"' % key)
-                    key)
+            self._set_field(key, value)
-            super(ImmutableObject, self).__setattr__(key, value)
 
     def __setattr__(self, name, value):
-        if name in self.__slots__:
+        if name.startswith('_'):
-            return super(ImmutableObject, self).__setattr__(name, value)
+            object.__setattr__(self, name, value)
-        raise AttributeError('Object is immutable.')
+        else:
+            raise AttributeError('Object is immutable.')
 
     def __delattr__(self, name):
-        if name in self.__slots__:
+        if name.startswith('_'):
-            return super(ImmutableObject, self).__delattr__(name)
+            object.__delattr__(self, name)
-        raise AttributeError('Object is immutable.')
+        else:
+            raise AttributeError('Object is immutable.')
+
+    def _is_valid_field(self, name):
+        return hasattr(self, name) and not callable(getattr(self, name))
+
+    def _set_field(self, name, value):
+        if value == getattr(self.__class__, name):
+            self.__dict__.pop(name, None)
+        else:
+            self.__dict__[name] = value
 
     def _items(self):
-        for field, key in self._fields.items():
+        return self.__dict__.iteritems()
-            if hasattr(self, key):
-                yield field, getattr(self, key)
 
     def __repr__(self):
         kwarg_pairs = []
@@ -88,12 +71,10 @@ class ImmutableObject(object):
         }
 
     def __hash__(self):
-        if not hasattr(self, '_hash'):
+        hash_sum = 0
-            hash_sum = 0
+        for key, value in self._items():
-            for key, value in self._items():
+            hash_sum += hash(key) + hash(value)
-                hash_sum += hash(key) + hash(value)
+        return hash_sum
-            super(ImmutableObject, self).__setattr__('_hash', hash_sum)
-        return self._hash
 
     def __eq__(self, other):
         if not isinstance(other, self.__class__):
@@ -107,11 +88,108 @@ class ImmutableObject(object):
     def copy(self, **values):
         """
         .. deprecated:: 1.1
-            Use :meth:`replace` instead. Note that we no longer return copies.
+            Use :meth:`replace` instead.
         """
         deprecation.warn('model.immutable.copy')
         return self.replace(**values)
 
+    def replace(self, **kwargs):
+        """
+        Replace the fields in the model and return a new instance
+
+        Examples::
+
+            # Returns a track with a new name
+            Track(name='foo').replace(name='bar')
+            # Return an album with a new number of tracks
+            Album(num_tracks=2).replace(num_tracks=5)
+
+        :param kwargs: kwargs to set as fields on the object
+        :type kwargs: any
+        :rtype: instance of the model with replaced fields
+        """
+        other = copy.copy(self)
+        for key, value in kwargs.items():
+            if not self._is_valid_field(key):
+                raise TypeError(
+                    'copy() got an unexpected keyword argument "%s"' % key)
+            other._set_field(key, value)
+        return other
+
+    def serialize(self):
+        data = {}
+        data['__model__'] = self.__class__.__name__
+        for key, value in self._items():
+            if isinstance(value, (set, frozenset, list, tuple)):
+                value = [
+                    v.serialize() if isinstance(v, ImmutableObject) else v
+                    for v in value]
+            elif isinstance(value, ImmutableObject):
+                value = value.serialize()
+            if not (isinstance(value, list) and len(value) == 0):
+                data[key] = value
+        return data
+
+
+class _ValidatedImmutableObjectMeta(type):
+
+    """Helper that initializes fields, slots and memoizes instance creation."""
+
+    def __new__(cls, name, bases, attrs):
+        fields = {}
+
+        for base in bases:  # Copy parent fields over to our state
+            fields.update(getattr(base, '_fields', {}))
+
+        for key, value in attrs.items():  # Add our own fields
+            if isinstance(value, Field):
+                fields[key] = '_' + key
+                value._name = key
+
+        attrs['_fields'] = fields
+        attrs['_instances'] = weakref.WeakValueDictionary()
+        attrs['__slots__'] = list(attrs.get('__slots__', [])) + fields.values()
+
+        return super(_ValidatedImmutableObjectMeta, cls).__new__(
+            cls, name, bases, attrs)
+
+    def __call__(cls, *args, **kwargs):  # noqa: N805
+        instance = super(_ValidatedImmutableObjectMeta, cls).__call__(
+            *args, **kwargs)
+        return cls._instances.setdefault(weakref.ref(instance), instance)
+
+
+class ValidatedImmutableObject(ImmutableObject):
+    """
+    Superclass for immutable objects whose fields can only be modified via the
+    constructor. Fields should be :class:`Field` instances to ensure type
+    safety in our models.
+
+    Note that since these models can not be changed, we heavily memoize them
+    to save memory. So constructing a class with the same arguments twice will
+    give you the same instance twice.
+    """
+
+    __metaclass__ = _ValidatedImmutableObjectMeta
+    __slots__ = ['_hash']
+
+    def __hash__(self):
+        if not hasattr(self, '_hash'):
+            hash_sum = super(ValidatedImmutableObject, self).__hash__()
+            object.__setattr__(self, '_hash', hash_sum)
+        return self._hash
+
+    def _is_valid_field(self, name):
+        return name in self._fields
+
+    def _set_field(self, name, value):
+        object.__setattr__(self, name, value)
+
+    def _items(self):
+        for field, key in self._fields.items():
+            if hasattr(self, key):
+                yield field, getattr(self, key)
+
     def replace(self, **kwargs):
         """
         Replace the fields in the model and return a new instance
@@ -133,25 +211,7 @@ class ImmutableObject(object):
         """
         if not kwargs:
             return self
-        other = copy.copy(self)
+        other = super(ValidatedImmutableObject, self).replace(**kwargs)
-        for key, value in kwargs.items():
+        if hasattr(self, '_hash'):
-            if key not in self._fields:
+            object.__delattr__(other, '_hash')
-                raise TypeError(
-                    'copy() got an unexpected keyword argument "%s"' % key)
-            super(ImmutableObject, other).__setattr__(key, value)
-        super(ImmutableObject, other).__delattr__('_hash')
         return self._instances.setdefault(weakref.ref(other), other)
-
-    def serialize(self):
-        data = {}
-        data['__model__'] = self.__class__.__name__
-        for key, value in self._items():
-            if isinstance(value, (set, frozenset, list, tuple)):
-                value = [
-                    v.serialize() if isinstance(v, ImmutableObject) else v
-                    for v in value]
-            elif isinstance(value, ImmutableObject):
-                value = value.serialize()
-            if not (isinstance(value, list) and len(value) == 0):
-                data[key] = value
-        return data

tests/models/test_legacy.py

@@ -0,0 +1,164 @@
+from __future__ import absolute_import, unicode_literals
+
+import unittest
+
+from mopidy.models import ImmutableObject
+
+
+class Model(ImmutableObject):
+    uri = None
+    name = None
+    models = frozenset()
+
+    def __init__(self, *args, **kwargs):
+        self.__dict__['models'] = frozenset(kwargs.pop('models', None) or [])
+        super(Model, self).__init__(self, *args, **kwargs)
+
+
+class SubModel(ImmutableObject):
+    uri = None
+    name = None
+
+
+class GenericCopyTest(unittest.TestCase):
+    def compare(self, orig, other):
+        self.assertEqual(orig, other)
+        self.assertNotEqual(id(orig), id(other))
+
+    def test_copying_model(self):
+        model = Model()
+        self.compare(model, model.replace())
+
+    def test_copying_model_with_basic_values(self):
+        model = Model(name='foo', uri='bar')
+        other = model.replace(name='baz')
+        self.assertEqual('baz', other.name)
+        self.assertEqual('bar', other.uri)
+
+    def test_copying_model_with_missing_values(self):
+        model = Model(uri='bar')
+        other = model.replace(name='baz')
+        self.assertEqual('baz', other.name)
+        self.assertEqual('bar', other.uri)
+
+    def test_copying_model_with_private_internal_value(self):
+        model = Model(models=[SubModel(name=123)])
+        other = model.replace(models=[SubModel(name=345)])
+        self.assertIn(SubModel(name=345), other.models)
+
+    def test_copying_model_with_invalid_key(self):
+        with self.assertRaises(TypeError):
+            Model().replace(invalid_key=True)
+
+    def test_copying_model_to_remove(self):
+        model = Model(name='foo').replace(name=None)
+        self.assertEqual(model, Model())
+
+
+class ModelTest(unittest.TestCase):
+    def test_uri(self):
+        uri = 'an_uri'
+        model = Model(uri=uri)
+        self.assertEqual(model.uri, uri)
+        with self.assertRaises(AttributeError):
+            model.uri = None
+
+    def test_name(self):
+        name = 'a name'
+        model = Model(name=name)
+        self.assertEqual(model.name, name)
+        with self.assertRaises(AttributeError):
+            model.name = None
+
+    def test_submodels(self):
+        models = [SubModel(name=123), SubModel(name=456)]
+        model = Model(models=models)
+        self.assertEqual(set(model.models), set(models))
+        with self.assertRaises(AttributeError):
+            model.models = None
+
+    def test_models_none(self):
+        self.assertEqual(set(), Model(models=None).models)
+
+    def test_invalid_kwarg(self):
+        with self.assertRaises(TypeError):
+            Model(foo='baz')
+
+    def test_repr_without_models(self):
+        self.assertEqual(
+            "Model(name=u'name', uri=u'uri')",
+            repr(Model(uri='uri', name='name')))
+
+    def test_repr_with_models(self):
+        self.assertEqual(
+            "Model(models=[SubModel(name=123)], name=u'name', uri=u'uri')",
+            repr(Model(uri='uri', name='name', models=[SubModel(name=123)])))
+
+    def test_serialize_without_models(self):
+        self.assertDictEqual(
+            {'__model__': 'Model', 'uri': 'uri', 'name': 'name'},
+            Model(uri='uri', name='name').serialize())
+
+    def test_serialize_with_models(self):
+        submodel = SubModel(name=123)
+        self.assertDictEqual(
+            {'__model__': 'Model', 'uri': 'uri', 'name': 'name',
+                'models': [submodel.serialize()]},
+            Model(uri='uri', name='name', models=[submodel]).serialize())
+
+    def test_eq_uri(self):
+        model1 = Model(uri='uri1')
+        model2 = Model(uri='uri1')
+        self.assertEqual(model1, model2)
+        self.assertEqual(hash(model1), hash(model2))
+
+    def test_eq_name(self):
+        model1 = Model(name='name1')
+        model2 = Model(name='name1')
+        self.assertEqual(model1, model2)
+        self.assertEqual(hash(model1), hash(model2))
+
+    def test_eq_models(self):
+        models = [SubModel()]
+        model1 = Model(models=models)
+        model2 = Model(models=models)
+        self.assertEqual(model1, model2)
+        self.assertEqual(hash(model1), hash(model2))
+
+    def test_eq_models_order(self):
+        submodel1 = SubModel(name='name1')
+        submodel2 = SubModel(name='name2')
+        model1 = Model(models=[submodel1, submodel2])
+        model2 = Model(models=[submodel2, submodel1])
+        self.assertEqual(model1, model2)
+        self.assertEqual(hash(model1), hash(model2))
+
+    def test_eq_none(self):
+        self.assertNotEqual(Model(), None)
+
+    def test_eq_other(self):
+        self.assertNotEqual(Model(), 'other')
+
+    def test_ne_uri(self):
+        model1 = Model(uri='uri1')
+        model2 = Model(uri='uri2')
+        self.assertNotEqual(model1, model2)
+        self.assertNotEqual(hash(model1), hash(model2))
+
+    def test_ne_name(self):
+        model1 = Model(name='name1')
+        model2 = Model(name='name2')
+        self.assertNotEqual(model1, model2)
+        self.assertNotEqual(hash(model1), hash(model2))
+
+    def test_ne_models(self):
+        model1 = Model(models=[SubModel(name='name1')])
+        model2 = Model(models=[SubModel(name='name2')])
+        self.assertNotEqual(model1, model2)
+        self.assertNotEqual(hash(model1), hash(model2))
+
+    def test_ignores_values_with_default_value_none(self):
+        model1 = Model(name='name1')
+        model2 = Model(name='name1', uri=None)
+        self.assertEqual(model1, model2)
+        self.assertEqual(hash(model1), hash(model2))