From 8f96bf0f394f2a19e85400dda622c78e6188117e Mon Sep 17 00:00:00 2001
From: Thomas Adamcik <thomas@adamcik.no>
Date: Sat, 4 Apr 2015 11:33:32 +0200
Subject: [PATCH 01/13] tests: Fix some model use oddities

---
 tests/local/test_tracklist.py | 2 +-
 tests/mpd/test_translator.py  | 9 ++++-----
 2 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/tests/local/test_tracklist.py b/tests/local/test_tracklist.py
index 22d4c954..ca36ac44 100644
--- a/tests/local/test_tracklist.py
+++ b/tests/local/test_tracklist.py
@@ -103,7 +103,7 @@ class LocalTracklistProviderTest(unittest.TestCase):
 
     def test_filter_by_uri_returns_nothing_if_no_match(self):
         self.controller.playlist = Playlist(
-            tracks=[Track(uri=['z']), Track(uri=['y'])])
+            tracks=[Track(uri='z'), Track(uri='y')])
         self.assertEqual([], self.controller.filter(uri=['a']))
 
     def test_filter_by_multiple_criteria_returns_elements_matching_all(self):
diff --git a/tests/mpd/test_translator.py b/tests/mpd/test_translator.py
index bf50687d..3a9b00d8 100644
--- a/tests/mpd/test_translator.py
+++ b/tests/mpd/test_translator.py
@@ -1,6 +1,5 @@
 from __future__ import absolute_import, unicode_literals
 
-import datetime
 import unittest
 
 from mopidy.models import Album, Artist, Playlist, TlTrack, Track
@@ -20,8 +19,8 @@ class TrackMpdFormatTest(unittest.TestCase):
         composers=[Artist(name='a composer')],
         performers=[Artist(name='a performer')],
         genre='a genre',
-        date=datetime.date(1977, 1, 1),
-        disc_no='1',
+        date='1977-1-1',
+        disc_no=1,
         comment='a comment',
         length=137000,
     )
@@ -73,8 +72,8 @@ class TrackMpdFormatTest(unittest.TestCase):
         self.assertIn(('Performer', 'a performer'), result)
         self.assertIn(('Genre', 'a genre'), result)
         self.assertIn(('Track', '7/13'), result)
-        self.assertIn(('Date', datetime.date(1977, 1, 1)), result)
-        self.assertIn(('Disc', '1'), result)
+        self.assertIn(('Date', '1977-1-1'), result)
+        self.assertIn(('Disc', 1), result)
         self.assertIn(('Pos', 9), result)
         self.assertIn(('Id', 122), result)
         self.assertNotIn(('Comment', 'a comment'), result)

From 5c0430ef4a3211e63ccec2c8bb8475527efdfa50 Mon Sep 17 00:00:00 2001
From: Thomas Adamcik <thomas@adamcik.no>
Date: Sat, 4 Apr 2015 15:16:31 +0200
Subject: [PATCH 02/13] tests: Move models tests into a directory

---
 tests/{ => models}/test_models.py | 0
 1 file changed, 0 insertions(+), 0 deletions(-)
 rename tests/{ => models}/test_models.py (100%)

diff --git a/tests/test_models.py b/tests/models/test_models.py
similarity index 100%
rename from tests/test_models.py
rename to tests/models/test_models.py

From 07912e1091cb1f17a55217f6d1204aefb7beeac8 Mon Sep 17 00:00:00 2001
From: Thomas Adamcik <thomas@adamcik.no>
Date: Sat, 4 Apr 2015 15:17:31 +0200
Subject: [PATCH 03/13] models: Add fields for supporting validation of models

Feature makes use of python descriptors to hook in type checking and other
validation when fields get set.
---
 mopidy/models.py            | 121 ++++++++++++++++++++++
 tests/models/test_fields.py | 194 ++++++++++++++++++++++++++++++++++++
 2 files changed, 315 insertions(+)
 create mode 100644 tests/models/test_fields.py

diff --git a/mopidy/models.py b/mopidy/models.py
index 1ae26811..52af300d 100644
--- a/mopidy/models.py
+++ b/mopidy/models.py
@@ -2,6 +2,127 @@ from __future__ import absolute_import, unicode_literals
 
 import json
 
+# TODO: split into base models, serialization and fields?
+
+
+class Field(object):
+    def __init__(self, default=None, type=None, choices=None):
+        """
+        Base field for use in :class:`ImmutableObject`. These fields are
+        responsible 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
+        the object they are attached to allow it.
+
+        Default values will be validated with the exception of :class:`None`.
+
+        :param default: default value for field
+        :param type: if set the field value must be of this type
+        :param choices: if set the field value must be one of these
+        """
+        self._name = None  # Set by FieldMeta
+        self._choices = choices
+        self._default = default
+        self._type = type
+
+        if self._default is not None:
+            self.validate(self._default)
+
+    def validate(self, value):
+        """Validate and possibly modify the field value before assignment"""
+        if self._type and not isinstance(value, self._type):
+            raise TypeError('Expected %s to be a %s, not %r' %
+                            (self._name, self._type, value))
+        if self._choices and value not in self._choices:
+            raise TypeError('Expected %s to be a one of %s, not %r' %
+                            (self._name, self._choices, value))
+        return value
+
+    def __get__(self, instance, owner):
+        if not instance:
+            return self
+        return instance.__dict__.get(self._name, self._default)
+
+    def __set__(self, instance, value):
+        if value is None:
+            value = self._default
+        value = self.validate(value)
+        if value is not None:
+            instance.__dict__[self._name] = value
+        else:
+            self.__delete__(instance)
+
+    def __delete__(self, instance):
+        instance.__dict__.pop(self._name, None)
+
+
+class String(Field):
+    def __init__(self, default=None):
+        """
+        Specialized :class:`Field` which is wired up for bytes and unicode.
+
+        :param default: default value for field
+        """
+        # TODO: normalize to unicode?
+        # TODO: only allow unicode?
+        # TODO: disallow empty strings?
+        super(String, self).__init__(type=basestring, default=default)
+
+
+class Integer(Field):
+    def __init__(self, default=None, min=None, max=None):
+        """
+        :class:`Field` for storing integer numbers.
+
+        :param default: default value for field
+        :param min: if set the field value larger or equal to this value
+        :param max: if set the field value smaller or equal to this value
+        """
+        self._min = min
+        self._max = max
+        super(Integer, self).__init__(type=(int, long), default=default)
+
+    def validate(self, value):
+        value = super(Integer, self).validate(value)
+        if self._min is not None and value < self._min:
+            raise ValueError('Expected %s to be at least %d, not %d' %
+                             (self._name, self._min, value))
+        if self._max is not None and value > self._max:
+            raise ValueError('Expected %s to be at most %d, not %d' %
+                             (self._name, self._max, value))
+        return value
+
+
+class Collection(Field):
+    def __init__(self, type, container=tuple):
+        """
+        :class:`Field` for storing collections of a given type.
+
+        :param type: all items stored in the collection must be of this type
+        :param container: the type to store the items in
+        """
+        super(Collection, self).__init__(type=type, default=container())
+
+    def validate(self, value):
+        if isinstance(value, basestring):
+            raise TypeError('Expected %s to be a collection of %s, not %r'
+                            % (self._name, self._type.__name__, value))
+        for v in value:
+            if not isinstance(v, self._type):
+                raise TypeError('Expected %s to be a collection of %s, not %r'
+                                % (self._name, self._type.__name__, value))
+        return self._default.__class__(value) or None
+
+
+class FieldOwner(type):
+    """Helper to automatically assign field names to descriptors."""
+    def __new__(cls, name, bases, attrs):
+        for key, value in attrs.items():
+            if isinstance(value, Field):
+                value._name = key
+        return super(FieldOwner, cls).__new__(cls, name, bases, attrs)
+
 
 class ImmutableObject(object):
 
diff --git a/tests/models/test_fields.py b/tests/models/test_fields.py
new file mode 100644
index 00000000..f2b55f01
--- /dev/null
+++ b/tests/models/test_fields.py
@@ -0,0 +1,194 @@
+from __future__ import absolute_import, unicode_literals
+
+import unittest
+
+from mopidy.models import *  # noqa: F403
+
+
+def create_instance(field):
+    """Create an instance of a dummy class for testing fields."""
+
+    class Dummy(object):
+        __metaclass__ = FieldOwner
+        attr = field
+
+    return Dummy()
+
+
+class FieldDescriptorTest(unittest.TestCase):
+    def test_raw_field_accesible_through_class(self):
+        field = Field()
+        instance = create_instance(field)
+        self.assertEqual(field, instance.__class__.attr)
+
+    def test_field_knows_its_name(self):
+        instance = create_instance(Field())
+        self.assertEqual('attr', instance.__class__.attr._name)
+
+    def test_field_has_none_as_default(self):
+        instance = create_instance(Field())
+        self.assertIsNone(instance.attr)
+
+    def test_field_does_not_store_default_in_dict(self):
+        instance = create_instance(Field())
+        self.assertNotIn('attr', instance.__dict__)
+
+    def test_field_assigment_and_retrival(self):
+        instance = create_instance(Field())
+        instance.attr = 1234
+        self.assertEqual(1234, instance.attr)
+        self.assertEqual(1234, instance.__dict__['attr'])
+
+    def test_field_can_be_reassigned(self):
+        instance = create_instance(Field())
+        instance.attr = 1234
+        instance.attr = 5678
+        self.assertEqual(5678, instance.attr)
+
+    def test_field_can_be_deleted(self):
+        instance = create_instance(Field())
+        instance.attr = 1234
+        del instance.attr
+        self.assertEqual(None, instance.attr)
+        self.assertNotIn('attr', instance.__dict__)
+
+    def test_field_can_be_set_to_none(self):
+        instance = create_instance(Field())
+        instance.attr = 1234
+        instance.attr = None
+        self.assertEqual(None, instance.attr)
+        self.assertNotIn('attr', instance.__dict__)
+
+
+class FieldTest(unittest.TestCase):
+    def test_default_handling(self):
+        instance = create_instance(Field(default=1234))
+        self.assertEqual(1234, instance.attr)
+
+    def test_type_checking(self):
+        instance = create_instance(Field(type=set))
+        instance.attr = set()
+
+        with self.assertRaises(TypeError):
+            instance.attr = 1234
+
+    def test_choices_checking(self):
+        instance = create_instance(Field(choices=(1, 2, 3)))
+        instance.attr = 1
+
+        with self.assertRaises(TypeError):
+            instance.attr = 4
+
+    def test_default_respects_type_check(self):
+        with self.assertRaises(TypeError):
+            create_instance(Field(type=int, default='123'))
+
+    def test_default_respects_choices_check(self):
+        with self.assertRaises(TypeError):
+            create_instance(Field(choices=(1, 2, 3), default=5))
+
+
+class StringTest(unittest.TestCase):
+    def test_default_handling(self):
+        instance = create_instance(String(default='abc'))
+        self.assertEqual('abc', instance.attr)
+
+    def test_str_allowed(self):
+        instance = create_instance(String())
+        instance.attr = str('abc')
+        self.assertEqual(b'abc', instance.attr)
+
+    def test_unicode_allowed(self):
+        instance = create_instance(String())
+        instance.attr = unicode('abc')
+        self.assertEqual(u'abc', instance.attr)
+
+    def test_other_disallowed(self):
+        instance = create_instance(String())
+        with self.assertRaises(TypeError):
+            instance.attr = 1234
+
+    def test_empty_string(self):
+        instance = create_instance(String())
+        instance.attr = ''
+        self.assertEqual('', instance.attr)
+
+
+class IntegerTest(unittest.TestCase):
+    def test_default_handling(self):
+        instance = create_instance(Integer(default=1234))
+        self.assertEqual(1234, instance.attr)
+
+    def test_int_allowed(self):
+        instance = create_instance(Integer())
+        instance.attr = int(123)
+        self.assertEqual(123, instance.attr)
+
+    def test_long_allowed(self):
+        instance = create_instance(Integer())
+        instance.attr = long(123)
+        self.assertEqual(123, instance.attr)
+
+    def test_float_disallowed(self):
+        instance = create_instance(Integer())
+        with self.assertRaises(TypeError):
+            instance.attr = 123.0
+
+    def test_numeric_string_disallowed(self):
+        instance = create_instance(Integer())
+        with self.assertRaises(TypeError):
+            instance.attr = '123'
+
+    def test_other_disallowed(self):
+        instance = create_instance(String())
+        with self.assertRaises(TypeError):
+            instance.attr = tuple()
+
+    def test_min_validation(self):
+        instance = create_instance(Integer(min=0))
+        instance.attr = 0
+        self.assertEqual(0, instance.attr)
+
+        with self.assertRaises(ValueError):
+            instance.attr = -1
+
+    def test_max_validation(self):
+        instance = create_instance(Integer(max=10))
+        instance.attr = 10
+        self.assertEqual(10, instance.attr)
+
+        with self.assertRaises(ValueError):
+            instance.attr = 11
+
+
+class CollectionTest(unittest.TestCase):
+    def test_container_instance_is_default(self):
+        instance = create_instance(Collection(type=int, container=frozenset))
+        self.assertEqual(frozenset(), instance.attr)
+
+    def test_empty_collection(self):
+        instance = create_instance(Collection(type=int, container=frozenset))
+        instance.attr = []
+        self.assertEqual(frozenset(), instance.attr)
+        self.assertNotIn('attr', instance.__dict__)
+
+    def test_collection_gets_stored_in_container(self):
+        instance = create_instance(Collection(type=int, container=frozenset))
+        instance.attr = [1, 2, 3]
+        self.assertEqual(frozenset([1, 2, 3]), instance.attr)
+        self.assertEqual(frozenset([1, 2, 3]), instance.__dict__['attr'])
+
+    def test_collection_with_wrong_type(self):
+        instance = create_instance(Collection(type=int, container=frozenset))
+        with self.assertRaises(TypeError):
+            instance.attr = [1, '2', 3]
+
+    def test_collection_with_string(self):
+        instance = create_instance(Collection(type=int, container=frozenset))
+        with self.assertRaises(TypeError):
+            instance.attr = '123'
+
+    def test_strings_should_not_be_considered_a_collection(self):
+        instance = create_instance(Collection(type=str, container=tuple))
+        with self.assertRaises(TypeError):
+            instance.attr = b'123'

From 4faf4de7aada18d92c90366228dfdbe865f6aa76 Mon Sep 17 00:00:00 2001
From: Thomas Adamcik <thomas@adamcik.no>
Date: Sat, 4 Apr 2015 15:18:56 +0200
Subject: [PATCH 04/13] models: Convert all models to using fields.

---
 mopidy/models.py | 125 ++++++++++++++++++++---------------------------
 1 file changed, 52 insertions(+), 73 deletions(-)

diff --git a/mopidy/models.py b/mopidy/models.py
index 52af300d..817b5ae3 100644
--- a/mopidy/models.py
+++ b/mopidy/models.py
@@ -128,12 +128,15 @@ class ImmutableObject(object):
 
     """
     Superclass for immutable objects whose fields can only be modified via the
-    constructor.
+    constructor. Fields should be :class:`Field` instances to ensure type
+    safety in our models.
 
     :param kwargs: kwargs to set as fields on the object
     :type kwargs: any
     """
 
+    __metaclass__ = FieldOwner
+
     def __init__(self, *args, **kwargs):
         for key, value in kwargs.items():
             if not hasattr(self, key) or callable(getattr(self, key)):
@@ -142,7 +145,7 @@ class ImmutableObject(object):
                     key)
             if value == getattr(self, key):
                 continue  # Don't explicitly set default values
-            self.__dict__[key] = value
+            super(ImmutableObject, self).__setattr__(key, value)
 
     def __setattr__(self, name, value):
         if name.startswith('_'):
@@ -192,7 +195,7 @@ class ImmutableObject(object):
         :type values: dict
         :rtype: new instance of the model being copied
         """
-        data = {}
+        data = {}  # TODO: do we need public key handling now?
         for key in self.__dict__.keys():
             public_key = key.lstrip('_')
             value = values.pop(public_key, self.__dict__[key])
@@ -207,7 +210,7 @@ class ImmutableObject(object):
         return self.__class__(**data)
 
     def serialize(self):
-        data = {}
+        data = {}  # TODO: do we need public key handling now?
         data['__model__'] = self.__class__.__name__
         for key in self.__dict__.keys():
             public_key = key.lstrip('_')
@@ -282,14 +285,10 @@ class Ref(ImmutableObject):
     """
 
     #: The object URI. Read-only.
-    uri = None
+    uri = String()
 
     #: The object name. Read-only.
-    name = None
-
-    #: The object type, e.g. "artist", "album", "track", "playlist",
-    #: "directory". Read-only.
-    type = None
+    name = String()
 
     #: Constant used for comparison with the :attr:`type` field.
     ALBUM = 'album'
@@ -306,6 +305,10 @@ class Ref(ImmutableObject):
     #: Constant used for comparison with the :attr:`type` field.
     TRACK = 'track'
 
+    #: The object type, e.g. "artist", "album", "track", "playlist",
+    #: "directory". Read-only.
+    type = Field(choices=(ALBUM, ARTIST, DIRECTORY, PLAYLIST, TRACK))
+
     @classmethod
     def album(cls, **kwargs):
         """Create a :class:`Ref` with ``type`` :attr:`ALBUM`."""
@@ -346,13 +349,13 @@ class Image(ImmutableObject):
     """
 
     #: The image URI. Read-only.
-    uri = None
+    uri = String()
 
     #: Optional width of the image or :class:`None`. Read-only.
-    width = None
+    width = Integer(min=0)
 
     #: Optional height of the image or :class:`None`. Read-only.
-    height = None
+    height = Integer(min=0)
 
 
 class Artist(ImmutableObject):
@@ -367,13 +370,13 @@ class Artist(ImmutableObject):
     """
 
     #: The artist URI. Read-only.
-    uri = None
+    uri = String()
 
     #: The artist name. Read-only.
-    name = None
+    name = String()
 
     #: The MusicBrainz ID of the artist. Read-only.
-    musicbrainz_id = None
+    musicbrainz_id = String()
 
 
 class Album(ImmutableObject):
@@ -398,37 +401,32 @@ class Album(ImmutableObject):
     """
 
     #: The album URI. Read-only.
-    uri = None
+    uri = String()
 
     #: The album name. Read-only.
-    name = None
+    name = String()
 
     #: A set of album artists. Read-only.
-    artists = frozenset()
+    artists = Collection(type=Artist, container=frozenset)
 
     #: The number of tracks in the album. Read-only.
-    num_tracks = None
+    num_tracks = Integer(min=0)
 
     #: The number of discs in the album. Read-only.
-    num_discs = None
+    num_discs = Integer(min=0)
 
     #: The album release date. Read-only.
-    date = None
+    date = String()  # TODO: add date type
 
     #: The MusicBrainz ID of the album. Read-only.
-    musicbrainz_id = None
+    musicbrainz_id = String()
 
     #: The album image URIs. Read-only.
-    images = frozenset()
+    images = Collection(type=basestring, container=frozenset)
     # XXX If we want to keep the order of images we shouldn't use frozenset()
     # as it doesn't preserve order. I'm deferring this issue until we got
     # actual usage of this field with more than one image.
 
-    def __init__(self, *args, **kwargs):
-        self.__dict__['artists'] = frozenset(kwargs.pop('artists', None) or [])
-        self.__dict__['images'] = frozenset(kwargs.pop('images', None) or [])
-        super(Album, self).__init__(*args, **kwargs)
-
 
 class Track(ImmutableObject):
 
@@ -466,61 +464,52 @@ class Track(ImmutableObject):
     """
 
     #: The track URI. Read-only.
-    uri = None
+    uri = String()
 
     #: The track name. Read-only.
-    name = None
+    name = String()
 
     #: A set of track artists. Read-only.
-    artists = frozenset()
+    artists = Collection(type=Artist, container=frozenset)
 
     #: The track :class:`Album`. Read-only.
-    album = None
+    album = Field(type=Album)
 
     #: A set of track composers. Read-only.
-    composers = frozenset()
+    composers = Collection(type=Artist, container=frozenset)
 
     #: A set of track performers`. Read-only.
-    performers = frozenset()
+    performers = Collection(type=Artist, container=frozenset)
 
     #: The track genre. Read-only.
-    genre = None
+    genre = String()
 
     #: The track number in the album. Read-only.
-    track_no = None
+    track_no = Integer(min=0)
 
     #: The disc number in the album. Read-only.
-    disc_no = None
+    disc_no = Integer(min=0)
 
     #: The track release date. Read-only.
-    date = None
+    date = String()  # TODO: add date type
 
     #: The track length in milliseconds. Read-only.
-    length = None
+    length = Integer(min=0)
 
     #: The track's bitrate in kbit/s. Read-only.
-    bitrate = None
+    bitrate = Integer(min=0)
 
     #: The track comment. Read-only.
-    comment = None
+    comment = String()
 
     #: The MusicBrainz ID of the track. Read-only.
-    musicbrainz_id = None
+    musicbrainz_id = String()
 
     #: Integer representing when the track was last modified. Exact meaning
     #: depends on source of track. For local files this is the modification
     #: time in milliseconds since Unix epoch. For other backends it could be an
     #: equivalent timestamp or simply a version counter.
-    last_modified = None
-
-    def __init__(self, *args, **kwargs):
-        def get(key):
-            return frozenset(kwargs.pop(key, None) or [])
-
-        self.__dict__['artists'] = get('artists')
-        self.__dict__['composers'] = get('composers')
-        self.__dict__['performers'] = get('performers')
-        super(Track, self).__init__(*args, **kwargs)
+    last_modified = Integer(min=0)
 
 
 class TlTrack(ImmutableObject):
@@ -546,10 +535,10 @@ class TlTrack(ImmutableObject):
     """
 
     #: The tracklist ID. Read-only.
-    tlid = None
+    tlid = Integer(min=0)
 
     #: The track. Read-only.
-    track = None
+    track = Field(type=Track)
 
     def __init__(self, *args, **kwargs):
         if len(args) == 2 and len(kwargs) == 0:
@@ -577,23 +566,19 @@ class Playlist(ImmutableObject):
     """
 
     #: The playlist URI. Read-only.
-    uri = None
+    uri = String()
 
     #: The playlist name. Read-only.
-    name = None
+    name = String()
 
     #: The playlist's tracks. Read-only.
-    tracks = tuple()
+    tracks = Collection(type=Track, container=tuple)
 
     #: The playlist modification time in milliseconds since Unix epoch.
     #: Read-only.
     #:
     #: Integer, or :class:`None` if unknown.
-    last_modified = None
-
-    def __init__(self, *args, **kwargs):
-        self.__dict__['tracks'] = tuple(kwargs.pop('tracks', None) or [])
-        super(Playlist, self).__init__(*args, **kwargs)
+    last_modified = Integer(min=0)
 
     # TODO: def insert(self, pos, track): ... ?
 
@@ -617,19 +602,13 @@ class SearchResult(ImmutableObject):
     """
 
     # The search result URI. Read-only.
-    uri = None
+    uri = String()
 
     # The tracks matching the search query. Read-only.
-    tracks = tuple()
+    tracks = Collection(type=Track, container=tuple)
 
     # The artists matching the search query. Read-only.
-    artists = tuple()
+    artists = Collection(type=Artist, container=tuple)
 
     # The albums matching the search query. Read-only.
-    albums = tuple()
-
-    def __init__(self, *args, **kwargs):
-        self.__dict__['tracks'] = tuple(kwargs.pop('tracks', None) or [])
-        self.__dict__['artists'] = tuple(kwargs.pop('artists', None) or [])
-        self.__dict__['albums'] = tuple(kwargs.pop('albums', None) or [])
-        super(SearchResult, self).__init__(*args, **kwargs)
+    albums = Collection(type=Album, container=tuple)

From 73415ce60ffe65b038bb396e51d98972d2b85d4b Mon Sep 17 00:00:00 2001
From: Thomas Adamcik <thomas@adamcik.no>
Date: Sat, 4 Apr 2015 15:21:38 +0200
Subject: [PATCH 05/13] models: Make sure del on attributes does not work

---
 mopidy/models.py            | 3 +++
 tests/models/test_models.py | 6 ++++++
 2 files changed, 9 insertions(+)

diff --git a/mopidy/models.py b/mopidy/models.py
index 817b5ae3..8f81b578 100644
--- a/mopidy/models.py
+++ b/mopidy/models.py
@@ -152,6 +152,9 @@ class ImmutableObject(object):
             return super(ImmutableObject, self).__setattr__(name, value)
         raise AttributeError('Object is immutable.')
 
+    def __delattr__(self, name):
+        raise AttributeError('Object is immutable.')
+
     def __repr__(self):
         kwarg_pairs = []
         for (key, value) in sorted(self.__dict__.items()):
diff --git a/tests/models/test_models.py b/tests/models/test_models.py
index e9a8f439..77383a6e 100644
--- a/tests/models/test_models.py
+++ b/tests/models/test_models.py
@@ -74,6 +74,12 @@ class RefTest(unittest.TestCase):
         with self.assertRaises(AttributeError):
             ref.name = None
 
+    # TODO: add these for the more of the models?
+    def test_del_name(self):
+        ref = Ref(name='foo')
+        with self.assertRaises(AttributeError):
+            del ref.name
+
     def test_invalid_kwarg(self):
         with self.assertRaises(TypeError):
             Ref(foo='baz')

From 0a2dff5a6a61301690902b48be523508df463dc6 Mon Sep 17 00:00:00 2001
From: Thomas Adamcik <thomas@adamcik.no>
Date: Sat, 4 Apr 2015 15:24:44 +0200
Subject: [PATCH 06/13] docs: Add model validation to changelog

---
 docs/changelog.rst | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/docs/changelog.rst b/docs/changelog.rst
index 605a30fe..b8f4f530 100644
--- a/docs/changelog.rst
+++ b/docs/changelog.rst
@@ -4,7 +4,6 @@ Changelog
 
 This changelog is used to track all major changes to Mopidy.
 
-
 v1.1.0 (UNRELEASED)
 ===================
 
@@ -14,6 +13,12 @@ Core API
 - Calling :meth:`mopidy.core.library.LibraryController.search`` with ``kwargs``
   as the query is no longer supported (PR: :issue:`1090`)
 
+Models
+------
+
+- Added type checks and other sanity checks to model construction and
+  serialization. (Fixes: :issue:`865`)
+
 Internal changes
 ----------------
 

From c8693a0591d41402a389fae7d9f63b3bf4dae3fc Mon Sep 17 00:00:00 2001
From: Thomas Adamcik <thomas@adamcik.no>
Date: Sat, 4 Apr 2015 15:31:25 +0200
Subject: [PATCH 07/13] models: Simplify copy and serialize methods

We don't need to worry about internal vs external naming when doing things via
Fields.
---
 mopidy/models.py | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/mopidy/models.py b/mopidy/models.py
index 8f81b578..5cea5723 100644
--- a/mopidy/models.py
+++ b/mopidy/models.py
@@ -198,26 +198,22 @@ class ImmutableObject(object):
         :type values: dict
         :rtype: new instance of the model being copied
         """
-        data = {}  # TODO: do we need public key handling now?
-        for key in self.__dict__.keys():
-            public_key = key.lstrip('_')
-            value = values.pop(public_key, self.__dict__[key])
-            data[public_key] = value
+        data = {}
+        for key, value in self.__dict__.items():
+            data[key] = value
         for key in values.keys():
             if hasattr(self, key):
-                value = values.pop(key)
-                data[key] = value
+                data[key] = values.pop(key)
         if values:
+            args = ', '.join(values)
             raise TypeError(
-                'copy() got an unexpected keyword argument "%s"' % key)
+                'copy() got an unexpected keyword argument "%s"' % args)
         return self.__class__(**data)
 
     def serialize(self):
-        data = {}  # TODO: do we need public key handling now?
+        data = {}
         data['__model__'] = self.__class__.__name__
-        for key in self.__dict__.keys():
-            public_key = key.lstrip('_')
-            value = self.__dict__[key]
+        for key, value in self.__dict__.items():
             if isinstance(value, (set, frozenset, list, tuple)):
                 value = [
                     v.serialize() if isinstance(v, ImmutableObject) else v
@@ -225,7 +221,7 @@ class ImmutableObject(object):
             elif isinstance(value, ImmutableObject):
                 value = value.serialize()
             if not (isinstance(value, list) and len(value) == 0):
-                data[public_key] = value
+                data[key] = value
         return data
 
 

From c375d772dd1170cc3f9334c70772cc2c8b7d88d7 Mon Sep 17 00:00:00 2001
From: Thomas Adamcik <thomas@adamcik.no>
Date: Sat, 4 Apr 2015 15:43:49 +0200
Subject: [PATCH 08/13] models: Store field keys in models

---
 mopidy/models.py | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/mopidy/models.py b/mopidy/models.py
index 5cea5723..0da4a51f 100644
--- a/mopidy/models.py
+++ b/mopidy/models.py
@@ -118,9 +118,12 @@ class Collection(Field):
 class FieldOwner(type):
     """Helper to automatically assign field names to descriptors."""
     def __new__(cls, name, bases, attrs):
+        attrs['_fields'] = []
         for key, value in attrs.items():
             if isinstance(value, Field):
+                attrs['_fields'].append(key)
                 value._name = key
+        attrs['_fields'].sort()
         return super(FieldOwner, cls).__new__(cls, name, bases, attrs)
 
 
@@ -139,7 +142,7 @@ class ImmutableObject(object):
 
     def __init__(self, *args, **kwargs):
         for key, value in kwargs.items():
-            if not hasattr(self, key) or callable(getattr(self, key)):
+            if key not in self._fields:
                 raise TypeError(
                     '__init__() got an unexpected keyword argument "%s"' %
                     key)

From 7eda0160ca3895e3f6781c5fb6eea16e0c8301b0 Mon Sep 17 00:00:00 2001
From: Thomas Adamcik <thomas@adamcik.no>
Date: Sat, 4 Apr 2015 15:44:24 +0200
Subject: [PATCH 09/13] models: Internal attrs are no longer needed

---
 mopidy/models.py | 2 --
 1 file changed, 2 deletions(-)

diff --git a/mopidy/models.py b/mopidy/models.py
index 0da4a51f..9a22b846 100644
--- a/mopidy/models.py
+++ b/mopidy/models.py
@@ -151,8 +151,6 @@ class ImmutableObject(object):
             super(ImmutableObject, self).__setattr__(key, value)
 
     def __setattr__(self, name, value):
-        if name.startswith('_'):
-            return super(ImmutableObject, self).__setattr__(name, value)
         raise AttributeError('Object is immutable.')
 
     def __delattr__(self, name):

From 2d03cd7290b95f0df97f2aa46dd4ceaf22d39211 Mon Sep 17 00:00:00 2001
From: Thomas Adamcik <thomas@adamcik.no>
Date: Sat, 4 Apr 2015 16:22:59 +0200
Subject: [PATCH 10/13] models: Make fields handle unsetting defaults in
 __dict__

---
 mopidy/models.py            | 12 +++++-------
 tests/models/test_fields.py |  8 ++++++++
 2 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/mopidy/models.py b/mopidy/models.py
index 9a22b846..cf11b4d2 100644
--- a/mopidy/models.py
+++ b/mopidy/models.py
@@ -45,13 +45,13 @@ class Field(object):
         return instance.__dict__.get(self._name, self._default)
 
     def __set__(self, instance, value):
-        if value is None:
-            value = self._default
-        value = self.validate(value)
         if value is not None:
-            instance.__dict__[self._name] = value
-        else:
+            value = self.validate(value)
+
+        if value is None or value == self._default:
             self.__delete__(instance)
+        else:
+            instance.__dict__[self._name] = value
 
     def __delete__(self, instance):
         instance.__dict__.pop(self._name, None)
@@ -146,8 +146,6 @@ class ImmutableObject(object):
                 raise TypeError(
                     '__init__() got an unexpected keyword argument "%s"' %
                     key)
-            if value == getattr(self, key):
-                continue  # Don't explicitly set default values
             super(ImmutableObject, self).__setattr__(key, value)
 
     def __setattr__(self, name, value):
diff --git a/tests/models/test_fields.py b/tests/models/test_fields.py
index f2b55f01..864373a9 100644
--- a/tests/models/test_fields.py
+++ b/tests/models/test_fields.py
@@ -59,6 +59,14 @@ class FieldDescriptorTest(unittest.TestCase):
         self.assertEqual(None, instance.attr)
         self.assertNotIn('attr', instance.__dict__)
 
+    def test_field_can_be_set_default(self):
+        default = object()
+        instance = create_instance(Field(default=default))
+        instance.attr = 1234
+        instance.attr = default
+        self.assertEqual(default, instance.attr)
+        self.assertNotIn('attr', instance.__dict__)
+
 
 class FieldTest(unittest.TestCase):
     def test_default_handling(self):

From f131ba4879f384487541afa792f3042da15ec980 Mon Sep 17 00:00:00 2001
From: Thomas Adamcik <thomas@adamcik.no>
Date: Sat, 4 Apr 2015 16:28:32 +0200
Subject: [PATCH 11/13] models: Update copy to only validate new values.

---
 mopidy/models.py | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/mopidy/models.py b/mopidy/models.py
index cf11b4d2..a0194b1c 100644
--- a/mopidy/models.py
+++ b/mopidy/models.py
@@ -21,7 +21,7 @@ class Field(object):
         :param type: if set the field value must be of this type
         :param choices: if set the field value must be one of these
         """
-        self._name = None  # Set by FieldMeta
+        self._name = None  # Set by FieldOwner
         self._choices = choices
         self._default = default
         self._type = type
@@ -197,17 +197,14 @@ class ImmutableObject(object):
         :type values: dict
         :rtype: new instance of the model being copied
         """
-        data = {}
-        for key, value in self.__dict__.items():
-            data[key] = value
-        for key in values.keys():
-            if hasattr(self, key):
-                data[key] = values.pop(key)
-        if values:
-            args = ', '.join(values)
-            raise TypeError(
-                'copy() got an unexpected keyword argument "%s"' % args)
-        return self.__class__(**data)
+        other = self.__class__()
+        other.__dict__.update(self.__dict__.copy())
+        for key, value in values.items():
+            if key not in self._fields:
+                raise TypeError(
+                    'copy() got an unexpected keyword argument "%s"' % key)
+            super(ImmutableObject, other).__setattr__(key, value)
+        return other
 
     def serialize(self):
         data = {}

From 86042132761e96062708ab283075b9f47c06b656 Mon Sep 17 00:00:00 2001
From: Thomas Adamcik <thomas@adamcik.no>
Date: Sat, 4 Apr 2015 16:55:15 +0200
Subject: [PATCH 12/13] models: Remove __dict__.copy() that did not do anything

---
 mopidy/models.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mopidy/models.py b/mopidy/models.py
index a0194b1c..eb6f4a58 100644
--- a/mopidy/models.py
+++ b/mopidy/models.py
@@ -198,7 +198,7 @@ class ImmutableObject(object):
         :rtype: new instance of the model being copied
         """
         other = self.__class__()
-        other.__dict__.update(self.__dict__.copy())
+        other.__dict__.update(self.__dict__)
         for key, value in values.items():
             if key not in self._fields:
                 raise TypeError(

From 9b442e15637f2b0605ef6ed38b7f812cc7381855 Mon Sep 17 00:00:00 2001
From: Thomas Adamcik <thomas@adamcik.no>
Date: Mon, 6 Apr 2015 23:27:46 +0200
Subject: [PATCH 13/13] review: Address review comments

---
 docs/api/models.rst          |  2 +
 mopidy/models.py             | 72 ++++++++++++++++++++----------------
 tests/models/test_fields.py  |  9 ++++-
 tests/mpd/test_translator.py |  4 +-
 4 files changed, 52 insertions(+), 35 deletions(-)

diff --git a/docs/api/models.rst b/docs/api/models.rst
index 23a08002..7192c284 100644
--- a/docs/api/models.rst
+++ b/docs/api/models.rst
@@ -76,6 +76,8 @@ Data model helpers
 
 .. autoclass:: mopidy.models.ImmutableObject
 
+.. autoclass:: mopidy.models.Field
+
 .. autoclass:: mopidy.models.ModelJSONEncoder
 
 .. autofunction:: mopidy.models.model_json_decoder
diff --git a/mopidy/models.py b/mopidy/models.py
index eb6f4a58..45961f30 100644
--- a/mopidy/models.py
+++ b/mopidy/models.py
@@ -6,21 +6,23 @@ import json
 
 
 class Field(object):
+
+    """
+    Base field for use in :class:`ImmutableObject`. 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
+    the object they are attached to allow it.
+
+    Default values will be validated with the exception of :class:`None`.
+
+    :param default: default value for field
+    :param type: if set the field value must be of this type
+    :param choices: if set the field value must be one of these
+    """
+
     def __init__(self, default=None, type=None, choices=None):
-        """
-        Base field for use in :class:`ImmutableObject`. These fields are
-        responsible 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
-        the object they are attached to allow it.
-
-        Default values will be validated with the exception of :class:`None`.
-
-        :param default: default value for field
-        :param type: if set the field value must be of this type
-        :param choices: if set the field value must be one of these
-        """
         self._name = None  # Set by FieldOwner
         self._choices = choices
         self._default = default
@@ -58,12 +60,14 @@ class Field(object):
 
 
 class String(Field):
-    def __init__(self, default=None):
-        """
-        Specialized :class:`Field` which is wired up for bytes and unicode.
 
-        :param default: default value for field
-        """
+    """
+    Specialized :class:`Field` which is wired up for bytes and unicode.
+
+    :param default: default value for field
+    """
+
+    def __init__(self, default=None):
         # TODO: normalize to unicode?
         # TODO: only allow unicode?
         # TODO: disallow empty strings?
@@ -71,14 +75,16 @@ class String(Field):
 
 
 class Integer(Field):
-    def __init__(self, default=None, min=None, max=None):
-        """
-        :class:`Field` for storing integer numbers.
 
-        :param default: default value for field
-        :param min: if set the field value larger or equal to this value
-        :param max: if set the field value smaller or equal to this value
+    """
+    :class:`Field` for storing integer numbers.
+
+    :param default: default value for field
+    :param min: field value must be larger or equal to this value when set
+    :param max: field value must be smaller or equal to this value when set
         """
+
+    def __init__(self, default=None, min=None, max=None):
         self._min = min
         self._max = max
         super(Integer, self).__init__(type=(int, long), default=default)
@@ -95,13 +101,15 @@ class Integer(Field):
 
 
 class Collection(Field):
-    def __init__(self, type, container=tuple):
-        """
-        :class:`Field` for storing collections of a given type.
 
-        :param type: all items stored in the collection must be of this type
-        :param container: the type to store the items in
-        """
+    """
+    :class:`Field` for storing collections of a given type.
+
+    :param type: all items stored in the collection must be of this type
+    :param container: the type to store the items in
+    """
+
+    def __init__(self, type, container=tuple):
         super(Collection, self).__init__(type=type, default=container())
 
     def validate(self, value):
@@ -116,7 +124,9 @@ class Collection(Field):
 
 
 class FieldOwner(type):
+
     """Helper to automatically assign field names to descriptors."""
+
     def __new__(cls, name, bases, attrs):
         attrs['_fields'] = []
         for key, value in attrs.items():
diff --git a/tests/models/test_fields.py b/tests/models/test_fields.py
index 864373a9..5347b83c 100644
--- a/tests/models/test_fields.py
+++ b/tests/models/test_fields.py
@@ -101,14 +101,19 @@ class StringTest(unittest.TestCase):
         instance = create_instance(String(default='abc'))
         self.assertEqual('abc', instance.attr)
 
-    def test_str_allowed(self):
+    def test_native_str_allowed(self):
         instance = create_instance(String())
         instance.attr = str('abc')
+        self.assertEqual('abc', instance.attr)
+
+    def test_bytes_allowed(self):
+        instance = create_instance(String())
+        instance.attr = b'abc'
         self.assertEqual(b'abc', instance.attr)
 
     def test_unicode_allowed(self):
         instance = create_instance(String())
-        instance.attr = unicode('abc')
+        instance.attr = u'abc'
         self.assertEqual(u'abc', instance.attr)
 
     def test_other_disallowed(self):
diff --git a/tests/mpd/test_translator.py b/tests/mpd/test_translator.py
index 3a9b00d8..4e1baf0e 100644
--- a/tests/mpd/test_translator.py
+++ b/tests/mpd/test_translator.py
@@ -19,7 +19,7 @@ class TrackMpdFormatTest(unittest.TestCase):
         composers=[Artist(name='a composer')],
         performers=[Artist(name='a performer')],
         genre='a genre',
-        date='1977-1-1',
+        date='1977-01-01',
         disc_no=1,
         comment='a comment',
         length=137000,
@@ -72,7 +72,7 @@ class TrackMpdFormatTest(unittest.TestCase):
         self.assertIn(('Performer', 'a performer'), result)
         self.assertIn(('Genre', 'a genre'), result)
         self.assertIn(('Track', '7/13'), result)
-        self.assertIn(('Date', '1977-1-1'), result)
+        self.assertIn(('Date', '1977-01-01'), result)
         self.assertIn(('Disc', 1), result)
         self.assertIn(('Pos', 9), result)
         self.assertIn(('Id', 122), result)