Skip to content

Commit 50684b7

Browse files
authored
Use None as default values for multi-valued fields in hooks.Info (#6405)
## Fix empty list treated as non-empty field value in `_apply_metadata` Fixes #6403 Thanks to @aereaux for reporting and helpful debugging! ### Problem In `beets/autotag/__init__.py`, `_apply_metadata` skipped overwriting existing field values with `None` to avoid clearing data unintentionally. However, an empty list (`[]`) was not treated the same way — it would still overwrite existing field values, effectively clearing multi-valued fields (e.g. `genres`) when the tag source returned nothing. ### Fix Keep multi-value field values as `None` by default in `beets.autotag.hooks.Info` subclasses: ```python def __init__( ... genres: list[str] | None = None, ... ): ... # Before self.genres = genres or [] # After self.genres = genres ``` ### Test changes - Added `genres=["Rock", "Pop"]` to the test album fixture to expose the bug: album genres were not being propagated to tracks due to the empty-list issue, since empty track-level genres overwrote them. - Removed the `@pytest.mark.xfail` marker once the fix made the test pass. - Consolidated ~20 granular `ApplyTest` methods and the separate `ApplyCompilationTest` class into a single data-driven `test_autotag_items` test, reducing noise and improving coverage clarity. - Moved autotag tests into `test/autotag/` to better reflect module structure.
2 parents 86fac4b + a8b8aa9 commit 50684b7

File tree

5 files changed

+327
-484
lines changed

5 files changed

+327
-484
lines changed

beets/autotag/hooks.py

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -97,15 +97,15 @@ def __init__(
9797
self.artist = artist
9898
self.artist_credit = artist_credit
9999
self.artist_id = artist_id
100-
self.artists = artists or []
101-
self.artists_credit = artists_credit or []
102-
self.artists_ids = artists_ids or []
100+
self.artists = artists
101+
self.artists_credit = artists_credit
102+
self.artists_ids = artists_ids
103103
self.artist_sort = artist_sort
104-
self.artists_sort = artists_sort or []
104+
self.artists_sort = artists_sort
105105
self.data_source = data_source
106106
self.data_url = data_url
107107
self.genre = None
108-
self.genres = genres or []
108+
self.genres = genres
109109
self.media = media
110110
self.update(kwargs)
111111

@@ -160,7 +160,7 @@ def __init__(
160160
self.albumdisambig = albumdisambig
161161
self.albumstatus = albumstatus
162162
self.albumtype = albumtype
163-
self.albumtypes = albumtypes or []
163+
self.albumtypes = albumtypes
164164
self.asin = asin
165165
self.barcode = barcode
166166
self.catalognum = catalognum

docs/changelog.rst

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,9 @@ Bug fixes
4444
- :doc:`plugins/zero`: When the ``omit_single_disc`` option is set,
4545
``disctotal`` is zeroed alongside ``disc``.
4646
- :doc:`plugins/fetchart`: Prevent deletion of configured fallback cover art
47+
- In autotagging, initialise empty multi-valued fields with ``None`` instead of
48+
empty list, which caused beets to overwrite existing metadata with empty list
49+
values instead of leaving them unchanged. :bug:`6403`
4750

4851
For plugin developers
4952
~~~~~~~~~~~~~~~~~~~~~

test/autotag/test_autotag.py

Lines changed: 317 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,317 @@
1+
# This file is part of beets.
2+
# Copyright 2016, Adrian Sampson.
3+
#
4+
# Permission is hereby granted, free of charge, to any person obtaining
5+
# a copy of this software and associated documentation files (the
6+
# "Software"), to deal in the Software without restriction, including
7+
# without limitation the rights to use, copy, modify, merge, publish,
8+
# distribute, sublicense, and/or sell copies of the Software, and to
9+
# permit persons to whom the Software is furnished to do so, subject to
10+
# the following conditions:
11+
#
12+
# The above copyright notice and this permission notice shall be
13+
# included in all copies or substantial portions of the Software.
14+
15+
"""Tests for autotagging functionality."""
16+
17+
import operator
18+
19+
import pytest
20+
21+
from beets import autotag
22+
from beets.autotag import AlbumInfo, TrackInfo, correct_list_fields, match
23+
from beets.library import Item
24+
from beets.test.helper import BeetsTestCase
25+
26+
27+
class TestAssignment:
28+
A = "one"
29+
B = "two"
30+
C = "three"
31+
32+
@pytest.fixture(autouse=True)
33+
def config(self, config):
34+
config["match"]["track_length_grace"] = 10
35+
config["match"]["track_length_max"] = 30
36+
37+
@pytest.mark.parametrize(
38+
# 'expected' is a tuple of expected (mapping, extra_items, extra_tracks)
39+
"item_titles, track_titles, expected",
40+
[
41+
# items ordering gets corrected
42+
([A, C, B], [A, B, C], ({A: A, B: B, C: C}, [], [])),
43+
# unmatched tracks are returned as 'extra_tracks'
44+
# the first track is unmatched
45+
([B, C], [A, B, C], ({B: B, C: C}, [], [A])),
46+
# the middle track is unmatched
47+
([A, C], [A, B, C], ({A: A, C: C}, [], [B])),
48+
# the last track is unmatched
49+
([A, B], [A, B, C], ({A: A, B: B}, [], [C])),
50+
# unmatched items are returned as 'extra_items'
51+
([A, C, B], [A, C], ({A: A, C: C}, [B], [])),
52+
],
53+
)
54+
def test_assign_tracks(self, item_titles, track_titles, expected):
55+
expected_mapping, expected_extra_items, expected_extra_tracks = expected
56+
57+
items = [Item(title=title) for title in item_titles]
58+
tracks = [TrackInfo(title=title) for title in track_titles]
59+
60+
item_info_pairs, extra_items, extra_tracks = match.assign_items(
61+
items, tracks
62+
)
63+
64+
assert (
65+
{i.title: t.title for i, t in item_info_pairs},
66+
[i.title for i in extra_items],
67+
[t.title for t in extra_tracks],
68+
) == (expected_mapping, expected_extra_items, expected_extra_tracks)
69+
70+
def test_order_works_when_track_names_are_entirely_wrong(self):
71+
# A real-world test case contributed by a user.
72+
def item(i, length):
73+
return Item(
74+
artist="ben harper",
75+
album="burn to shine",
76+
title=f"ben harper - Burn to Shine {i}",
77+
track=i,
78+
length=length,
79+
)
80+
81+
items = []
82+
items.append(item(1, 241.37243007106997))
83+
items.append(item(2, 342.27781704375036))
84+
items.append(item(3, 245.95070222338137))
85+
items.append(item(4, 472.87662515485437))
86+
items.append(item(5, 279.1759535763187))
87+
items.append(item(6, 270.33333768012))
88+
items.append(item(7, 247.83435613222923))
89+
items.append(item(8, 216.54504531525072))
90+
items.append(item(9, 225.72775379800484))
91+
items.append(item(10, 317.7643606963552))
92+
items.append(item(11, 243.57001238834192))
93+
items.append(item(12, 186.45916150485752))
94+
95+
def info(index, title, length):
96+
return TrackInfo(title=title, length=length, index=index)
97+
98+
trackinfo = []
99+
trackinfo.append(info(1, "Alone", 238.893))
100+
trackinfo.append(info(2, "The Woman in You", 341.44))
101+
trackinfo.append(info(3, "Less", 245.59999999999999))
102+
trackinfo.append(info(4, "Two Hands of a Prayer", 470.49299999999999))
103+
trackinfo.append(info(5, "Please Bleed", 277.86599999999999))
104+
trackinfo.append(info(6, "Suzie Blue", 269.30599999999998))
105+
trackinfo.append(info(7, "Steal My Kisses", 245.36000000000001))
106+
trackinfo.append(info(8, "Burn to Shine", 214.90600000000001))
107+
trackinfo.append(info(9, "Show Me a Little Shame", 224.0929999999999))
108+
trackinfo.append(info(10, "Forgiven", 317.19999999999999))
109+
trackinfo.append(info(11, "Beloved One", 243.733))
110+
trackinfo.append(info(12, "In the Lord's Arms", 186.13300000000001))
111+
112+
expected = list(zip(items, trackinfo)), [], []
113+
114+
assert match.assign_items(items, trackinfo) == expected
115+
116+
117+
class ApplyTest(BeetsTestCase):
118+
def _apply(self, per_disc_numbering=False, artist_credit=False):
119+
info = self.info
120+
item_info_pairs = list(zip(self.items, info.tracks))
121+
self.config["per_disc_numbering"] = per_disc_numbering
122+
self.config["artist_credit"] = artist_credit
123+
autotag.apply_metadata(self.info, item_info_pairs)
124+
125+
def setUp(self):
126+
super().setUp()
127+
128+
self.items = [Item(), Item()]
129+
self.info = AlbumInfo(
130+
tracks=[
131+
TrackInfo(
132+
title="title",
133+
track_id="dfa939ec-118c-4d0f-84a0-60f3d1e6522c",
134+
medium=1,
135+
medium_index=1,
136+
medium_total=1,
137+
index=1,
138+
artist="trackArtist",
139+
artist_credit="trackArtistCredit",
140+
artists_credit=["trackArtistCredit"],
141+
artist_sort="trackArtistSort",
142+
artists_sort=["trackArtistSort"],
143+
),
144+
TrackInfo(
145+
title="title2",
146+
track_id="40130ed1-a27c-42fd-a328-1ebefb6caef4",
147+
medium=2,
148+
medium_index=1,
149+
index=2,
150+
medium_total=1,
151+
),
152+
],
153+
artist="albumArtist",
154+
artists=["albumArtist", "albumArtist2"],
155+
album="album",
156+
album_id="7edb51cb-77d6-4416-a23c-3a8c2994a2c7",
157+
artist_id="a6623d39-2d8e-4f70-8242-0a9553b91e50",
158+
artists_ids=None,
159+
artist_credit="albumArtistCredit",
160+
artists_credit=["albumArtistCredit1", "albumArtistCredit2"],
161+
artist_sort=None,
162+
artists_sort=["albumArtistSort", "albumArtistSort2"],
163+
albumtype="album",
164+
va=True,
165+
mediums=2,
166+
data_source="MusicBrainz",
167+
year=2013,
168+
month=12,
169+
day=18,
170+
genres=["Rock", "Pop"],
171+
)
172+
173+
common_expected = {
174+
"album": "album",
175+
"albumartist_credit": "albumArtistCredit",
176+
"albumartist_sort": "",
177+
"albumartist": "albumArtist",
178+
"albumartists": ["albumArtist", "albumArtist2"],
179+
"albumartists_credit": [
180+
"albumArtistCredit1",
181+
"albumArtistCredit2",
182+
],
183+
"albumartists_sort": ["albumArtistSort", "albumArtistSort2"],
184+
"albumtype": "album",
185+
"albumtypes": ["album"],
186+
"comp": True,
187+
"disctotal": 2,
188+
"mb_albumartistid": "a6623d39-2d8e-4f70-8242-0a9553b91e50",
189+
"mb_albumartistids": ["a6623d39-2d8e-4f70-8242-0a9553b91e50"],
190+
"mb_albumid": "7edb51cb-77d6-4416-a23c-3a8c2994a2c7",
191+
"mb_artistid": "a6623d39-2d8e-4f70-8242-0a9553b91e50",
192+
"mb_artistids": ["a6623d39-2d8e-4f70-8242-0a9553b91e50"],
193+
"tracktotal": 2,
194+
"year": 2013,
195+
"month": 12,
196+
"day": 18,
197+
"genres": ["Rock", "Pop"],
198+
}
199+
200+
self.expected_tracks = [
201+
{
202+
**common_expected,
203+
"artist": "trackArtist",
204+
"artists": ["albumArtist", "albumArtist2"],
205+
"artist_credit": "trackArtistCredit",
206+
"artist_sort": "trackArtistSort",
207+
"artists_credit": ["trackArtistCredit"],
208+
"artists_sort": ["trackArtistSort"],
209+
"disc": 1,
210+
"mb_trackid": "dfa939ec-118c-4d0f-84a0-60f3d1e6522c",
211+
"title": "title",
212+
"track": 1,
213+
},
214+
{
215+
**common_expected,
216+
"artist": "albumArtist",
217+
"artists": ["albumArtist", "albumArtist2"],
218+
"artist_credit": "albumArtistCredit",
219+
"artist_sort": "",
220+
"artists_credit": [
221+
"albumArtistCredit1",
222+
"albumArtistCredit2",
223+
],
224+
"artists_sort": ["albumArtistSort", "albumArtistSort2"],
225+
"disc": 2,
226+
"mb_trackid": "40130ed1-a27c-42fd-a328-1ebefb6caef4",
227+
"title": "title2",
228+
"track": 2,
229+
},
230+
]
231+
232+
def test_autotag_items(self):
233+
self._apply()
234+
235+
keys = self.expected_tracks[0].keys()
236+
get_values = operator.itemgetter(*keys)
237+
238+
applied_data = [
239+
dict(zip(keys, get_values(dict(i)))) for i in self.items
240+
]
241+
242+
assert applied_data == self.expected_tracks
243+
244+
def test_per_disc_numbering(self):
245+
self._apply(per_disc_numbering=True)
246+
247+
assert self.items[0].track == 1
248+
assert self.items[1].track == 1
249+
assert self.items[0].tracktotal == 1
250+
assert self.items[1].tracktotal == 1
251+
252+
def test_artist_credit_prefers_artist_over_albumartist_credit(self):
253+
self.info.tracks[0].update(artist="oldArtist", artist_credit=None)
254+
255+
self._apply(artist_credit=True)
256+
257+
assert self.items[0].artist == "oldArtist"
258+
259+
def test_artist_credit_falls_back_to_albumartist(self):
260+
self.info.artist_credit = None
261+
262+
self._apply(artist_credit=True)
263+
264+
assert self.items[1].artist == "albumArtist"
265+
266+
def test_date_only_zeroes_month_and_day(self):
267+
self.items = [Item(year=1, month=2, day=3)]
268+
self.info.update(year=2013, month=None, day=None)
269+
270+
self._apply()
271+
272+
assert self.items[0].year == 2013
273+
assert self.items[0].month == 0
274+
assert self.items[0].day == 0
275+
276+
def test_missing_date_applies_nothing(self):
277+
self.items = [Item(year=1, month=2, day=3)]
278+
self.info.update(year=None, month=None, day=None)
279+
280+
self._apply()
281+
282+
assert self.items[0].year == 1
283+
assert self.items[0].month == 2
284+
assert self.items[0].day == 3
285+
286+
287+
@pytest.mark.parametrize(
288+
"single_field,list_field",
289+
[
290+
("mb_artistid", "mb_artistids"),
291+
("mb_albumartistid", "mb_albumartistids"),
292+
("albumtype", "albumtypes"),
293+
],
294+
)
295+
@pytest.mark.parametrize(
296+
"single_value,list_value",
297+
[
298+
(None, []),
299+
(None, ["1"]),
300+
(None, ["1", "2"]),
301+
("1", []),
302+
("1", ["1"]),
303+
("1", ["1", "2"]),
304+
("1", ["2", "1"]),
305+
],
306+
)
307+
def test_correct_list_fields(
308+
single_field, list_field, single_value, list_value
309+
):
310+
"""Ensure that the first value in a list field matches the single field."""
311+
data = {single_field: single_value, list_field: list_value}
312+
item = Item(**data)
313+
314+
correct_list_fields(item)
315+
316+
single_val, list_val = item[single_field], item[list_field]
317+
assert (not single_val and not list_val) or single_val == list_val[0]

test/plugins/test_musicbrainz.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -539,7 +539,7 @@ def test_no_genres(self):
539539
config["musicbrainz"]["genres"] = False
540540
release = self._make_release()
541541
d = self.mb.album_info(release)
542-
assert d.genres == []
542+
assert d.genres is None
543543

544544
def test_ignored_media(self):
545545
config["match"]["ignored_media"] = ["IGNORED1", "IGNORED2"]

0 commit comments

Comments
 (0)