Merge pull request #4582 from jpluscplusm/jcm_fix_albumtypes

Store `albumtypes` multi-value field consistently in-DB & in-tag, preventing continual file re-tagging

beets/autotag/mb.py

@@ -466,7 +466,7 @@ def album_info(release: Dict) -> beets.autotag.hooks.AlbumInfo:
         if release['release-group']['secondary-type-list']:
             for sec_type in release['release-group']['secondary-type-list']:
                 albumtypes.append(sec_type.lower())
-    info.albumtypes = '; '.join(albumtypes)
+    info.albumtypes = albumtypes
 
     # Release events.
     info.country, release_date = _preferred_release_event(release)

beets/dbcore/types.py

@@ -208,6 +208,27 @@ class String(Type):
             return self.model_type(value)
 
 
+class DelimitedString(String):
+    """A list of Unicode strings, represented in-database by a single string
+    containing delimiter-separated values.
+    """
+    model_type = list
+
+    def __init__(self, delimiter):
+        self.delimiter = delimiter
+
+    def format(self, value):
+        return self.delimiter.join(value)
+
+    def parse(self, string):
+        if not string:
+            return []
+        return string.split(self.delimiter)
+
+    def to_sql(self, model_value):
+        return self.delimiter.join(model_value)
+
+
 class Boolean(Type):
     """A boolean type.
     """
@@ -231,3 +252,4 @@ FLOAT = Float()
 NULL_FLOAT = NullFloat()
 STRING = String()
 BOOLEAN = Boolean()
+SEMICOLON_SPACE_DSV = DelimitedString(delimiter='; ')

beets/library.py

@@ -504,7 +504,7 @@ class Item(LibModel):
         'mb_releasetrackid': types.STRING,
         'trackdisambig': types.STRING,
         'albumtype': types.STRING,
-        'albumtypes': types.STRING,
+        'albumtypes': types.SEMICOLON_SPACE_DSV,
         'label': types.STRING,
         'acoustid_fingerprint': types.STRING,
         'acoustid_id': types.STRING,
@@ -1064,7 +1064,7 @@ class Album(LibModel):
         'mb_albumid': types.STRING,
         'mb_albumartistid': types.STRING,
         'albumtype': types.STRING,
-        'albumtypes': types.STRING,
+        'albumtypes': types.SEMICOLON_SPACE_DSV,
         'label': types.STRING,
         'mb_releasegroupid': types.STRING,
         'asin': types.STRING,

beetsplug/albumtypes.py

@@ -55,7 +55,7 @@ class AlbumTypesPlugin(BeetsPlugin):
             bracket_r = ''
 
         res = ''
-        albumtypes = item.albumtypes.split('; ')
+        albumtypes = item.albumtypes
         is_va = item.mb_albumartistid == VARIOUS_ARTISTS_ID
         for type in types:
             if type[0] in albumtypes and type[1]:

docs/changelog.rst

@@ -147,6 +147,13 @@ Bug fixes:
   :bug:`4561` :bug:`4600`
 * Fix issue where deletion of flexible fields on an album doesn't cascade to items
   :bug:`4662`
+* Fix issue where ``beet write`` continuosly retags the ``albumtypes`` metadata
+  field in files. Additionally broken data could have been added to the library
+  when the tag was read from file back into the library using ``beet update``.
+  It is required for all users to **check if such broken data is present in the
+  library**. Following the instructions `described here
+  <https://github.com/beetbox/beets/pull/4582#issuecomment-1445023493>`_, a
+  sanity check and potential fix is easily possible. :bug:`4528`
 
 For packagers:
 

docs/plugins/albumtypes.rst

@@ -11,6 +11,11 @@ you can use in your path formats or elsewhere.
 
 .. _MusicBrainz documentation: https://musicbrainz.org/doc/Release_Group/Type
 
+A bug introduced in beets 1.6.0 could have possibly imported broken data into
+the ``albumtypes`` library field. Please follow the instructions `described
+here <https://github.com/beetbox/beets/pull/4582#issuecomment-1445023493>`_ for
+a sanity check and potential fix. :bug:`4528`
+
 Configuration
 -------------
 

test/test_albumtypes.py

@@ -106,6 +106,6 @@ class AlbumTypesPluginTest(unittest.TestCase, TestHelper):
 
     def _create_album(self, album_types: [str], artist_id: str = 0):
         return self.add_album(
-            albumtypes='; '.join(album_types),
+            albumtypes=album_types,
             mb_albumartistid=artist_id
         )

test/test_ui.py

@@ -701,27 +701,30 @@ class UpdateTest(_common.TestCase):
         item = self.lib.items().get()
         self.assertEqual(item.title, 'full')
 
-    @unittest.expectedFailure
     def test_multivalued_albumtype_roundtrip(self):
         # https://github.com/beetbox/beets/issues/4528
 
         # albumtypes is empty for our test fixtures, so populate it first
         album = self.album
-        # setting albumtypes does not set albumtype currently...
-        # FIXME: When actually fixing the issue 4528, consider whether this
-        # should be set to "album" or ["album"]
-        album.albumtype = "album"
-        album.albumtypes = "album"
+        correct_albumtypes = ["album", "live"]
+
+        # Setting albumtypes does not set albumtype, currently.
+        # Using x[0] mirrors https://github.com/beetbox/mediafile/blob/057432ad53b3b84385e5582f69f44dc00d0a725d/mediafile.py#L1928  # noqa: E501
+        correct_albumtype = correct_albumtypes[0]
+
+        album.albumtype = correct_albumtype
+        album.albumtypes = correct_albumtypes
         album.try_sync(write=True, move=False)
 
         album.load()
-        albumtype_before = album.albumtype
-        self.assertEqual(albumtype_before, "album")
+        self.assertEqual(album.albumtype,  correct_albumtype)
+        self.assertEqual(album.albumtypes, correct_albumtypes)
 
         self._update()
 
         album.load()
-        self.assertEqual(albumtype_before, album.albumtype)
+        self.assertEqual(album.albumtype,  correct_albumtype)
+        self.assertEqual(album.albumtypes, correct_albumtypes)
 
 
 class PrintTest(_common.TestCase):