From b480311d66d8135b53a8e692c1c12786733175fe Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Mon, 4 May 2015 23:41:11 +0200 Subject: [PATCH] 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 | 21 ++-- mopidy/models/fields.py | 7 +- mopidy/models/immutable.py | 206 +++++++++++++++++++++++------------- tests/models/test_legacy.py | 164 ++++++++++++++++++++++++++++ 4 files changed, 312 insertions(+), 86 deletions(-) create mode 100644 tests/models/test_legacy.py diff --git a/mopidy/models/__init__.py b/mopidy/models/__init__.py index e4e8528a..231a472a 100644 --- a/mopidy/models/__init__.py +++ b/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 diff --git a/mopidy/models/fields.py b/mopidy/models/fields.py index 23154df5..bd0ba9f9 100644 --- a/mopidy/models/fields.py +++ b/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 - responsible for type checking and other data sanitation in our models. + Base field for use in + :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 diff --git a/mopidy/models/immutable.py b/mopidy/models/immutable.py index 0b6ef2f7..98cd8b5b 100644 --- a/mopidy/models/immutable.py +++ b/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 - safety in our models. + constructor. - 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. + This version of this class has been retained to avoid breaking any clients + relying on it's behavior. Internally in Mopidy we now use + :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"' % - key) - super(ImmutableObject, self).__setattr__(key, value) + '__init__() got an unexpected keyword argument "%s"' % key) + self._set_field(key, value) def __setattr__(self, name, value): - if name in self.__slots__: - return super(ImmutableObject, self).__setattr__(name, value) - raise AttributeError('Object is immutable.') + if name.startswith('_'): + object.__setattr__(self, name, value) + else: + raise AttributeError('Object is immutable.') def __delattr__(self, name): - if name in self.__slots__: - return super(ImmutableObject, self).__delattr__(name) - raise AttributeError('Object is immutable.') + if name.startswith('_'): + object.__delattr__(self, name) + 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(): - if hasattr(self, key): - yield field, getattr(self, key) + return self.__dict__.iteritems() def __repr__(self): kwarg_pairs = [] @@ -88,12 +71,10 @@ class ImmutableObject(object): } def __hash__(self): - if not hasattr(self, '_hash'): - hash_sum = 0 - for key, value in self._items(): - hash_sum += hash(key) + hash(value) - super(ImmutableObject, self).__setattr__('_hash', hash_sum) - return self._hash + hash_sum = 0 + for key, value in self._items(): + hash_sum += hash(key) + hash(value) + return hash_sum 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) - for key, value in kwargs.items(): - if key not in self._fields: - raise TypeError( - 'copy() got an unexpected keyword argument "%s"' % key) - super(ImmutableObject, other).__setattr__(key, value) - super(ImmutableObject, other).__delattr__('_hash') + other = super(ValidatedImmutableObject, self).replace(**kwargs) + if hasattr(self, '_hash'): + object.__delattr__(other, '_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 diff --git a/tests/models/test_legacy.py b/tests/models/test_legacy.py new file mode 100644 index 00000000..d837d738 --- /dev/null +++ b/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))