Do not delete the genres tag when musicbrainz.genres is False.

[djl]

Description

Fixes #6403

This commit also introduces a way to explicitly exclude fields from being updated when mbsync runs.

To Do

• Documentation. (If you've added a new command-line flag, for example, find the appropriate page under docs/ to describe it.)
• Changelog. (Add an entry to docs/changelog.rst to the bottom of one of the lists near the top of the document.)
• Tests. (Very much encouraged but not strictly required.)

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 70.42%. Comparing base (fd586ef) to head (d451c62).
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6513      +/-   ##
==========================================
+ Coverage   70.40%   70.42%   +0.02%     
==========================================
  Files         148      148              
  Lines       18806    18819      +13     
  Branches     3067     3071       +4     
==========================================
+ Hits        13240    13253      +13     
  Misses       4916     4916              
  Partials      650      650              

Files with missing lines	Coverage Δ
beetsplug/mbsync.py	84.94% <100.00%> (+2.44%)	⬆️

🚀 New features to boost your workflow:

• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[snejus]

Given that the bug seems to stem from the musicbrainz plugin, I expected the fix to be there. Using excluded fields in this seems to be a workaround, rather than a solution. For example, if I do not use mbsync but rather reimport music with beet import -LI, presumably we will still have the same issue?

[djl]

Ah I didn't think about re-importing. That will still show the same issue. I'll move the fix over to the musicbrainz plugin.

[snejus]

I had a look at the source code: it doesn't seem that genres field even gets set when musicbrainz.genres is False. In this case pre-existing genres should not be overwritten.

Are you able to reproduce this issue when reimporting?

[djl]

So I think this happens because the genres field on the data returned by musicbrainz plugin is None, which results in the existing genres being also set to None. This would mean it's not an issue with the musicbrainz plugin directly but would happen with any field that is different from existing data.

Here I updated the musicbrainz plugin to set info.country = None:

$ bq mbsync -p Këkht Aräkh pale
Këkht Aräkh - Pale Swordsman - Intro
  country: XW ->
  genres:
    - black metal
Këkht Aräkh - Pale Swordsman - Thorns
  country: XW ->
  genres:
    - black metal
Këkht Aräkh - Pale Swordsman - Night Descends
  country: XW ->
  genres:
    - black metal
Këkht Aräkh - Pale Swordsman - In the Garden
  country: XW ->
  genres:
    - black metal
Këkht Aräkh - Pale Swordsman - Amor
  country: XW ->
  genres:
    - black metal
Këkht Aräkh - Pale Swordsman - Nocturne
  country: XW ->
  genres:
    - black metal
Këkht Aräkh - Pale Swordsman - Amid the Stars
  country: XW ->
  genres:
    - black metal
Këkht Aräkh - Pale Swordsman - Lily
  country: XW ->
  genres:
    - black metal
Këkht Aräkh - Pale Swordsman - Crystal
  country: XW ->
  genres:
    - black metal
Këkht Aräkh - Pale Swordsman - Swordsman
  country: XW ->
  genres:
    - black metal

If this is an issue beyond just the musicbrainz plugin, my immediate thought for a solution would be to have a way for a metadata plugin to say "I have nothing for this field, use any existing data". Something other than None, which could mean that, but also mean "this should be set to null".

And yes, this does happen on re-importing.

[snejus]

What is your overwrite_null configuration?

[snejus]

Given the configuration you shared in #6403 (comment), it's expected that country is overwritten by None. However, genres is not listed in your configuration, so it should not be overwritten. Can you try to mbsync an album that currently has some genres and forcibly set them to None from inside the musicbrainz plugin?

[djl]

There is an issue with master at the moment, related to the relative path changes:

$ beet mbsync -p run the jewels
Migrating path for 295291 items...
Traceback (most recent call last):
  File "<frozen runpy>", line 198, in _run_module_as_main
  File "<frozen runpy>", line 88, in _run_code
  File "/home/djl/prj/beets/beets/__main__.py", line 24, in <module>
    main(sys.argv[1:])
    ~~~~^^^^^^^^^^^^^^
  File "/home/djl/prj/beets/beets/ui/__init__.py", line 1013, in main
    _raw_main(args)
    ~~~~~~~~~^^^^^^
  File "/home/djl/prj/beets/beets/ui/__init__.py", line 988, in _raw_main
    subcommands, lib = _setup(options, lib)
                       ~~~~~~^^^^^^^^^^^^^^
  File "/home/djl/prj/beets/beets/ui/__init__.py", line 825, in _setup
    lib = _open_library(config)
  File "/home/djl/prj/beets/beets/ui/__init__.py", line 885, in _open_library
    lib = library.Library(
        dbpath,
    ...<2 lines>...
        get_replacements(),
    )
  File "/home/djl/prj/beets/beets/library/library.py", line 47, in __init__
    super().__init__(path, timeout=timeout)
    ~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/djl/prj/beets/beets/dbcore/db.py", line 1161, in __init__
    self._migrate()
    ~~~~~~~~~~~~~^^
  File "/home/djl/prj/beets/beets/dbcore/db.py", line 1395, in _migrate
    migration.migrate_model(
    ~~~~~~~~~~~~~~~~~~~~~~~^
        model_cls, self.db_tables[model_cls._table]["columns"]
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    )
    ^
  File "/home/djl/prj/beets/beets/dbcore/db.py", line 1085, in migrate_model
    self._migrate_data(model_cls, *args, **kwargs)
    ~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/djl/prj/beets/beets/library/migrations.py", line 265, in _migrate_data
    self._migrate_field(model_cls, field)
    ~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^
  File "/home/djl/prj/beets/beets/library/migrations.py", line 252, in _migrate_field
    (normalize_path_for_db(r[field]), r["id"])
     ~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^
  File "/home/djl/prj/beets/beets/dbcore/pathutils.py", line 42, in normalize_path_for_db
    return _to_db_path(os.path.relpath(path, music_dir))
                       ~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^
  File "<frozen posixpath>", line 533, in relpath
  File "<frozen genericpath>", line 191, in _check_arg_types
TypeError: Can't mix strings and bytes in path components

I'll try to find some time later today to debug this unless someone beats me to it.

[snejus]

I've seen this before locally, but this only happened because I have manually rewritten my paths in the database as TEXT. Could this be the case on your end?

[djl]

That was it. There were a handful of paths (and artpaths) that were TEXT.

So explicitly setting info.genres = None in musicbrainz.py still results in genres being wiped:

$ beet mbsync -p "run the jewels 4"
Run the Jewels - Run the Jewels 4 - Yankee and the Brave (ep. 4)
  genres:
    - hip-hop
Run the Jewels feat. Greg Nice and DJ Premier - Run the Jewels 4 - Ooh La La
  genres:
    - hip-hop
Run the Jewels feat. 2 Chainz - Run the Jewels 4 - Out of Sight
  genres:
    - hip-hop
Run the Jewels - Run the Jewels 4 - Holy Calamafuck
  genres:
    - hip-hop
Run the Jewels - Run the Jewels 4 - Goonies Vs. E.T.
  genres:
    - hip-hop
Run the Jewels - Run the Jewels 4 - Walking in the Snow
  genres:
    - hip-hop
Run the Jewels featuring Pharrell Williams & Zack de la Rocha - Run the Jewels 4 - Ju$t
  genres:
    - hip-hop
Run the Jewels - Run the Jewels 4 - Never Look Back
  genres:
    - hip-hop
Run the Jewels - Run the Jewels 4 - The Ground Below
  genres:
    - hip-hop
Run the Jewels feat. Mavis Staples & Josh Homme - Run the Jewels 4 - Pulling the Pin
  genres:
    - hip-hop
Run the Jewels - Run the Jewels 4 - A Few Words for the Firing Squad (Radiation)
  genres:
    - hip-hop

[snejus]

That's so weird. I set my configuration to

musicbrainz:
  genres: false

and

$ beet ls supermode 'tell me why' -f '$genres'
electronic; house
$ beet mbsync supermode 'tell me why' -p
57594 | 2006 / Tell Me Why: Supermode, Axwell & Steve Angello - Tell Me Why
  albumartists:
    - Supermode, Axwell & Steve Angello
  albumartists_credit:
    - Supermode, Axwell & Steve Angello
  albumartists_sort:
    - Supermode, Axwell & Angello, Steve
  artists:
    - Supermode, Axwell & Steve Angello
  artists_credit:
    - Supermode, Axwell & Steve Angello
  artists_sort:
    - Supermode, Axwell & Angello, Steve
  country: SE -> XW

[snejus]

Can you double-check the data source for those items?

[djl]

Data source seems fine to me:

$ beet ls -f '$data_source' "run the jewels 4"
MusicBrainz
MusicBrainz
MusicBrainz
MusicBrainz
MusicBrainz
MusicBrainz
MusicBrainz
MusicBrainz
MusicBrainz
MusicBrainz
MusicBrainz

[djl]

Aha! So it's import.from_scratch: true that's causing this issue.

I think if I remove the genre specific code from this pull request and just leave the excluded_fields part, this will solve my problem.

[snejus]

Ah that makes sense. I don't think any of the changes in this PR are required if you remove from_scratch from your configuration.

[djl]

Won't that result in some metadata being left around after re-importing? Or does from_scratch only apply to finding a match, not writing the data?

[snejus]

from_scratch is the same as adding all fields to overwrite_null - it will remove preexisting values from every field that is not populated by the metadata source.

[djl]

That looks like what I need then. Thanks!