From 41f9ecc73b66ab1a775e5609a0cc7b38b8300b8c Mon Sep 17 00:00:00 2001 From: Jonathan Matthews Date: Fri, 11 Nov 2022 23:44:44 +0200 Subject: [PATCH 1/9] Introduce new DB type: DelimeteredString --- beets/dbcore/types.py | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/beets/dbcore/types.py b/beets/dbcore/types.py index 40f6a0805..8ab3bcfa2 100644 --- a/beets/dbcore/types.py +++ b/beets/dbcore/types.py @@ -207,6 +207,29 @@ class String(Type): else: return self.model_type(value) +class DelimeteredString(String): + """A list of Unicode strings, represented in-database by a single string + containing delimiter-separated values. + """ + model_type = list + + def __init__(self, delim=','): + self.delim = delim + + def to_sql(self, model_value): + return self.delim.join([str(elem) for elem in model_value]) + + def from_sql(self, sql_value): + if sql_value is None: + return self.null() + else: + return self.parse(sql_value) + + def parse(self, string): + try: + return string.split(self.delim) + except: + return self.null class Boolean(Type): """A boolean type. @@ -231,3 +254,4 @@ FLOAT = Float() NULL_FLOAT = NullFloat() STRING = String() BOOLEAN = Boolean() +SEMICOLON_DSV = DelimeteredString(delim=';') From af65c6d70715e5b828f8a380bd8f52c416a9b087 Mon Sep 17 00:00:00 2001 From: Jonathan Matthews Date: Fri, 11 Nov 2022 23:45:59 +0200 Subject: [PATCH 2/9] Serialise albumtypes field as a semicolon-based DelimeteredString --- beets/autotag/mb.py | 2 +- beets/library.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/beets/autotag/mb.py b/beets/autotag/mb.py index 7d7932faf..f380cd033 100644 --- a/beets/autotag/mb.py +++ b/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) diff --git a/beets/library.py b/beets/library.py index c05dcda18..69cf8d349 100644 --- a/beets/library.py +++ b/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_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_DSV, 'label': types.STRING, 'mb_releasegroupid': types.STRING, 'asin': types.STRING, From 7cfc39ea27c0c9c1bc357bf32a446920957bc402 Mon Sep 17 00:00:00 2001 From: Jonathan Matthews Date: Mon, 12 Dec 2022 10:34:40 +0000 Subject: [PATCH 3/9] Realign with known-working code after review by @mkhl @mkhl was kind enough to do a drive-by review of my proposed changes, which I'll include here as the GitHub URI may bit-rot over time (it's technically [here](https://github.com/beetbox/beets/commit/bc21caa0d5665c091683f885ee5b0c59110fca74), but that commit isn't part of the `beets` repo, so may get GC'd). I've encorporated all their proposed changes, as their code is being run against an existing Beets library, whereas my changes were made as I tried to set up Beets for the first time - thus I'm inclined to trust their known-working code more than my own! This is a review starting at https://github.com/beetbox/beets/commit/bc21caa0d5665c091683f885ee5b0c59110fca74#diff-d53f73df7f26990645e7bdac865ef86a52b67bafc6fe6ad69890b510a57e2955R210 (`class DelimeteredString(String):`) > for context this is the version i'm using now: > > ```python > class DelimitedString(String): > 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) > ``` > > i think 'delimited string' is the correct term here > > the rest of the code doesn't seem to use many abbreviations, so calling the property `delimiter` seems appropriate > > i don't think a default value for the delimiter makes a lot of sense? > > the list comprehension and string conversions in `to_sql` don't seem necessary to me, see above. did you run into trouble without them? > > the `from_sql` seems to just be missing functionality from the `Type` parent and seems completely unnecessary > > `parse` shouldn't be able to fail because at that point, we've ensured that its argument is actually a string. i also added a `if not string` condition because otherwise the empty list of album types would turn into the list containing the empty string (because that's what split returns) > > if we don't define a `format` method here we print the internal python representation of the values (i.e. `['album', 'live']` or somesuch) in the `beet write` output. joining on the delimiter nicely formats the output :) > > just so i don't ping you twice unnecessarily, i think it's better to instantiate this type with `'; '` (semicolon space) as the delimiter, because that's what was used before to join the albumtypes and what we'll find in the database All these changes have been made, including the switch from `;` to `;` as the in-DB separator. --- beets/dbcore/types.py | 30 ++++++++++++++---------------- beets/library.py | 4 ++-- 2 files changed, 16 insertions(+), 18 deletions(-) diff --git a/beets/dbcore/types.py b/beets/dbcore/types.py index 8ab3bcfa2..8c8bfa3f6 100644 --- a/beets/dbcore/types.py +++ b/beets/dbcore/types.py @@ -207,29 +207,27 @@ class String(Type): else: return self.model_type(value) -class DelimeteredString(String): + +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, delim=','): - self.delim = delim + def __init__(self, delimiter): + self.delimiter = delimiter - def to_sql(self, model_value): - return self.delim.join([str(elem) for elem in model_value]) - - def from_sql(self, sql_value): - if sql_value is None: - return self.null() - else: - return self.parse(sql_value) + def format(self, value): + return self.delimiter.join(value) def parse(self, string): - try: - return string.split(self.delim) - except: - return self.null + 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. @@ -254,4 +252,4 @@ FLOAT = Float() NULL_FLOAT = NullFloat() STRING = String() BOOLEAN = Boolean() -SEMICOLON_DSV = DelimeteredString(delim=';') +SEMICOLON_SPACE_DSV = DelimitedString(delimiter='; ') diff --git a/beets/library.py b/beets/library.py index 69cf8d349..9d5219b18 100644 --- a/beets/library.py +++ b/beets/library.py @@ -504,7 +504,7 @@ class Item(LibModel): 'mb_releasetrackid': types.STRING, 'trackdisambig': types.STRING, 'albumtype': types.STRING, - 'albumtypes': types.SEMICOLON_DSV, + '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.SEMICOLON_DSV, + 'albumtypes': types.SEMICOLON_SPACE_DSV, 'label': types.STRING, 'mb_releasegroupid': types.STRING, 'asin': types.STRING, From cd52a05d3affc61bf33050a81c8f8fd919b6e761 Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Mon, 27 Feb 2023 13:29:30 +0100 Subject: [PATCH 4/9] Add fix for #4528 to changelog --- docs/changelog.rst | 3 +++ 1 file changed, 3 insertions(+) diff --git a/docs/changelog.rst b/docs/changelog.rst index 921ae7b30..f00288d20 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -145,6 +145,9 @@ Bug fixes: :bug:`4561` :bug:`4600` * Fix issue where deletion of flexible fields on an album doesn't cascade to items :bug:`4662` +* Store ``albumtypes`` multi-value field correctly in the DB and in files' + tags, stopping useless re-tagging of files on every ``beets write``. + :bug:`4528` For packagers: From 27218a94909f0244ea7c7650e7dd3a10d5c0acb1 Mon Sep 17 00:00:00 2001 From: Jonathan Matthews Date: Sun, 18 Dec 2022 12:39:20 +0000 Subject: [PATCH 5/9] Mark albumtype/s expected test failure as fixed --- test/test_ui.py | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/test/test_ui.py b/test/test_ui.py index 86c40d204..a1e02aaae 100644 --- a/test/test_ui.py +++ b/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 + 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): From 93fa19f493fb0e2354c55d3429d66e23862d1465 Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Sat, 25 Feb 2023 07:46:33 +0100 Subject: [PATCH 6/9] Fix albumtypes plugin and its tests The new database type DelimitedString does list to string and vice versa conversions itself. --- beetsplug/albumtypes.py | 2 +- test/test_albumtypes.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/beetsplug/albumtypes.py b/beetsplug/albumtypes.py index 47f8dc64e..b54e802e6 100644 --- a/beetsplug/albumtypes.py +++ b/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]: diff --git a/test/test_albumtypes.py b/test/test_albumtypes.py index 91808553d..3d329dd7b 100644 --- a/test/test_albumtypes.py +++ b/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 ) From 7be1eec762ec1e315dec734c322211eafd3ed892 Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Mon, 27 Feb 2023 13:56:35 +0100 Subject: [PATCH 7/9] Rewrite changelog entry for #4583 and include linking to manual fixing tutorial. --- docs/changelog.rst | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index f00288d20..b71bc1104 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -145,9 +145,13 @@ Bug fixes: :bug:`4561` :bug:`4600` * Fix issue where deletion of flexible fields on an album doesn't cascade to items :bug:`4662` -* Store ``albumtypes`` multi-value field correctly in the DB and in files' - tags, stopping useless re-tagging of files on every ``beets write``. - :bug:`4528` +* 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 + `_, a + sanity check and potential fix is easily possible. :bug:`4528` For packagers: From 78853cc9c2aa562ee0e7bffc3ce7b07025d1e417 Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Tue, 28 Feb 2023 08:31:30 +0100 Subject: [PATCH 8/9] Add note to albumtypes plugin docs about #4528 Add a note to the docs of the albumtypes plugin warning about issue #4528 and linking to the manual fixing description. --- docs/plugins/albumtypes.rst | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/docs/plugins/albumtypes.rst b/docs/plugins/albumtypes.rst index 7a1a08f95..bf736abca 100644 --- a/docs/plugins/albumtypes.rst +++ b/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 `_ for +a sanity check and potential fix. :bug:`4528` + Configuration ------------- From 4908e1ae096155c547995d1b719936f63ba805be Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Tue, 28 Feb 2023 09:24:28 +0100 Subject: [PATCH 9/9] Fix flake8 issues in test_ui.py that were introduced in 27218a94. --- test/test_ui.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/test_ui.py b/test/test_ui.py index a1e02aaae..d3ce4a560 100644 --- a/test/test_ui.py +++ b/test/test_ui.py @@ -709,10 +709,10 @@ class UpdateTest(_common.TestCase): 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 - correct_albumtype = correct_albumtypes[0] + # 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.albumtype = correct_albumtype album.albumtypes = correct_albumtypes album.try_sync(write=True, move=False)