diff --git a/docs/api/models.rst b/docs/api/models.rst index d2d8ec0a..d25e7f34 100644 --- a/docs/api/models.rst +++ b/docs/api/models.rst @@ -77,8 +77,29 @@ Data model helpers .. autoclass:: mopidy.models.ImmutableObject :members: -.. autoclass:: mopidy.models.Field +.. autoclass:: mopidy.models.ValidatedImmutableObject + :members: replace + +Data model (de)serialization +---------------------------- + +.. autofunction:: mopidy.models.model_json_decoder .. autoclass:: mopidy.models.ModelJSONEncoder -.. autofunction:: mopidy.models.model_json_decoder +Data model field types +---------------------- + +.. autoclass:: mopidy.models.fields.Field + +.. autoclass:: mopidy.models.fields.String + +.. autoclass:: mopidy.models.fields.Identifier + +.. autoclass:: mopidy.models.fields.URI + +.. autoclass:: mopidy.models.fields.Date + +.. autoclass:: mopidy.models.fields.Integer + +.. autoclass:: mopidy.models.fields.Collection 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..01a03a75 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 @@ -72,20 +73,41 @@ class String(Field): class Date(String): + """ + :class:`Field` for storing ISO 8601 dates as a string. + + Supported formats are ``YYYY-MM-DD``, ``YYYY-MM`` and ``YYYY``, currently + not validated. + + :param default: default value for field + """ pass # TODO: make this check for YYYY-MM-DD, YYYY-MM, YYYY using strptime. class Identifier(String): + """ + :class:`Field` for storing ASCII values such as GUIDs or other identifiers. + + Values will be interned. + + :param default: default value for field + """ def validate(self, value): return intern(str(super(Identifier, self).validate(value))) class URI(Identifier): + """ + :class`Field` for storing URIs + + Values will be interned, currently not validated. + + :param default: default value for field + """ pass # TODO: validate URIs? class Integer(Field): - """ :class:`Field` for storing integer numbers. @@ -111,7 +133,6 @@ class Integer(Field): class Collection(Field): - """ :class:`Field` for storing collections of a given type. diff --git a/mopidy/models/immutable.py b/mopidy/models/immutable.py index 8e282c91..98cd8b5b 100644 --- a/mopidy/models/immutable.py +++ b/mopidy/models/immutable.py @@ -1,7 +1,6 @@ from __future__ import absolute_import, unicode_literals import copy -import inspect import itertools import weakref @@ -9,74 +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 key, value in attrs.items(): - 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()) - - for ancestor in [b for base in bases for b in inspect.getmro(base)]: - if '__weakref__' in getattr(ancestor, '__slots__', []): - break - else: - attrs['__slots__'].append('__weakref__') - - 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 = [] @@ -92,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__): @@ -111,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 @@ -137,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/mopidy/models/serialize.py b/mopidy/models/serialize.py index 1c438efb..5002a8f7 100644 --- a/mopidy/models/serialize.py +++ b/mopidy/models/serialize.py @@ -2,7 +2,9 @@ from __future__ import absolute_import, unicode_literals import json -from mopidy.models.immutable import ImmutableObject +from mopidy.models import immutable + +_MODELS = ['Ref', 'Artist', 'Album', 'Track', 'TlTrack', 'Playlist'] class ModelJSONEncoder(json.JSONEncoder): @@ -19,7 +21,7 @@ class ModelJSONEncoder(json.JSONEncoder): """ def default(self, obj): - if isinstance(obj, ImmutableObject): + if isinstance(obj, immutable.ImmutableObject): return obj.serialize() return json.JSONEncoder.default(self, obj) @@ -38,8 +40,8 @@ def model_json_decoder(dct): """ if '__model__' in dct: - models = {c.__name__: c for c in ImmutableObject.__subclasses__()} + from mopidy import models model_name = dct.pop('__model__') - if model_name in models: - return models[model_name](**dct) + if model_name in _MODELS: + return getattr(models, model_name)(**dct) return dct diff --git a/tests/models/test_fields.py b/tests/models/test_fields.py index 6ef10f18..bf842fd5 100644 --- a/tests/models/test_fields.py +++ b/tests/models/test_fields.py @@ -3,15 +3,14 @@ from __future__ import absolute_import, unicode_literals import unittest from mopidy.models.fields import * # noqa: F403 -from mopidy.models.immutable import ImmutableObjectMeta def create_instance(field): """Create an instance of a dummy class for testing fields.""" class Dummy(object): - __metaclass__ = ImmutableObjectMeta attr = field + attr._name = 'attr' return Dummy() 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)) diff --git a/tests/models/test_models.py b/tests/models/test_models.py index f0f5ff6c..5108411a 100644 --- a/tests/models/test_models.py +++ b/tests/models/test_models.py @@ -19,6 +19,7 @@ class InheritanceTest(unittest.TestCase): pass def test_sub_class_can_have_its_own_slots(self): + # Needed for things like SpotifyTrack in mopidy-spotify 1.x class Foo(Track): __slots__ = ('_foo',) @@ -26,6 +27,17 @@ class InheritanceTest(unittest.TestCase): f = Foo() f._foo = 123 + def test_sub_class_can_be_initialized(self): + # Fails with following error if fields are not handled across classes. + # TypeError: __init__() got an unexpected keyword argument "type" + # Essentially this is testing that sub-classes take parent _fields into + # account. + + class Foo(Ref): + pass + + Foo.directory() + class CachingTest(unittest.TestCase): @@ -1156,9 +1168,3 @@ class SearchResultTest(unittest.TestCase): self.assertDictEqual( {'__model__': 'SearchResult', 'uri': 'uri'}, SearchResult(uri='uri').serialize()) - - def test_to_json_and_back(self): - result1 = SearchResult(uri='uri') - serialized = json.dumps(result1, cls=ModelJSONEncoder) - result2 = json.loads(serialized, object_hook=model_json_decoder) - self.assertEqual(result1, result2)