Repository: beetbox/beets
Pull request: Fix data_source not updated on re-import and refactor `reimport_metadata`
## Description

- Fixes #4726 
- Refactors preserving of flexible attributes in the importer's `reimport_metadata()` method.
- New logging message when flexible attributes are excluded from the preserving process and _actually_ set to a new value.
- Reformats all existing logging messages and streamline their looks.
- Adds a new unittest that checks if the data_source album field is _not_ reset to its original value on a reimport.

## To Do

- [X] ~Documentation.~
- [X] Changelog.
- [X] Tests.


Problem statement:
Re-imports don't update `data_source` flex attr
### Problem

When importing an album or a singleton that was previously imported already, the `data_source` flexible attribute is not updated. It stays as **initially imported**.


### Setup

* OS: ---
* Python version: ---
* beets version: 1.6.1 dev-latest
* Turning off plugins made problem go away (yes/no): no


### Proposed solution 

`data_source` needs to be excluded in `reimport_metadata()`

Hints from issue discussion:
Even album re-import does not update `data_source`.
> 

Thanks for testing and clarifying. I was actually assuming that this is also the case but I wasn't sure yet. So, many thanks, that is very helpful! :-)

I'm pretty sure the problem is located here:

https://github.com/beetbox/beets/blob/4194914e94077f673cf920d595580696b4485305/beets/importer.py#L810-L812

On a reimport this function looks up what `data_source` is in the library already and then simply re-applies it. Similar to the ID fields over at PR #4708, `data_source` would need to be excluded from this "preserving mechanism".


We should investigate older issues and the one that implemented the `reimport_metadata()` method and make sure this is not "intentional" behaviour. If it was, I could think about making it configurable. As I see it now though, it feels like a bug, data_source is an informational field and after a reimport it is just wrong. I _do_ use this field sometimes to save that "I tagged this manuall" with `beet mod data_source=MANUAL query`, but also then _if_ I do find an appropriate album in some metadata source, it makes sense that this field gets overwritten and tells the truth.

Commit message: Helper function for reimport flex attr preserving

Flexible attribute keys listed in REIMPORT_FRESH_FIELDS_* are now removed from
the original LazyConvertDict in a separate function which is used for both
album and item level fields.

Patch to review:
diff --git a/beets/importer.py b/beets/importer.py
index 96d1f17dfc..d04ff10649 100644
--- a/beets/importer.py
+++ b/beets/importer.py
@@ -50,6 +50,11 @@
 SINGLE_ARTIST_THRESH = 0.25
 PROGRESS_KEY = 'tagprogress'
 HISTORY_KEY = 'taghistory'
+# Album and item flexble attrbutes that should not be preserved on reimports.
+REIMPORT_FRESH_FIELDS_ALBUM = ['data_source']
+REIMPORT_FRESH_FIELDS_ITEM = ['data_source', 'bandcamp_album_id',
+                              'spotify_album_id', 'deezer_album_id',
+                              'beatport_album_id']
 
 # Global logger.
 log = logging.getLogger('beets')
@@ -811,21 +816,46 @@ def reimport_metadata(self, lib):
         """For reimports, preserves metadata for reimported items and
         albums.
         """
+        def _reduce_and_log(new_obj, existing_fields, overwrite_keys):
+            """Some flexible attributes should be overwritten (and not
+            preserved) on reimports which this function handles. Copies
+            existing_fields, logs and removes entries that should not be
+            preserved and returns a dict containing those fields left to
+            actually be preserved
+            """
+            noun = 'album' if self.is_album else 'item'
+            existing_fields = dict(existing_fields)
+            overwritten_fields = [k for k in existing_fields
+                                  if k in overwrite_keys
+                                  and existing_fields.get(k) != new_obj.get(k)]
+            if overwritten_fields:
+                log.debug('Not preserving {} {} flexible attributes: {}. '
+                          'Path: {}', noun, new_obj.id, overwritten_fields,
+                          displayable_path(new_obj.path))
+                for key in overwritten_fields:
+                    del existing_fields[key]
+            return existing_fields
+
         if self.is_album:
             replaced_album = self.replaced_albums.get(self.album.path)
             if replaced_album:
+                album_fields = _reduce_and_log(self.album,
+                                               replaced_album._values_flex,
+                                               REIMPORT_FRESH_FIELDS_ALBUM)
                 self.album.added = replaced_album.added
-                self.album.update(replaced_album._values_flex)
+                self.album.update(album_fields)
                 self.album.artpath = replaced_album.artpath
                 self.album.store()
                 log.debug(
-                    'Reimported album: added {0}, flexible '
-                    'attributes {1} from album {2} for {3}',
-                    self.album.added,
-                    replaced_album._values_flex.keys(),
-                    replaced_album.id,
-                    displayable_path(self.album.path)
-                )
+                    "Reimported album {} attribute: ['added'] from album {}, "
+                    "Path: {}",
+                    self.album.id, replaced_album.id,
+                    displayable_path(self.album.path))
+                log.debug(
+                    'Reimported album {} flexible attributes: {} '
+                    'from album {}, Path:{}',
+                    self.album.id, list(album_fields.keys()),
+                    replaced_album.id, displayable_path(self.album.path))
 
         for item in self.imported_items():
             dup_items = self.replaced_items[item]
@@ -833,33 +863,17 @@ def reimport_metadata(self, lib):
                 if dup_item.added and dup_item.added != item.added:
                     item.added = dup_item.added
                     log.debug(
-                        'Reimported item added {0} '
-                        'from item {1} for {2}',
-                        item.added,
-                        dup_item.id,
-                        displayable_path(item.path)
-                    )
-                # We exclude certain flexible attributes from the preserving
-                # process since they might have been fetched from MusicBrainz
-                # and been set in beets.autotag.apply_metadata().
-                # discogs_albumid might also have been set but is not a
-                # flexible attribute, thus no exclude is required.
-                if item.get('bandcamp_album_id'):
-                    dup_item.bandcamp_album_id = item.bandcamp_album_id
-                if item.get('spotify_album_id'):
-                    dup_item.spotify_album_id = item.spotify_album_id
-                if item.get('deezer_album_id'):
-                    dup_item.deezer_album_id = item.deezer_album_id
-                if item.get('beatport_album_id'):
-                    dup_item.beatport_album_id = item.beatport_album_id
-                item.update(dup_item._values_flex)
+                        "Reimported item {} attribute: ['added'] "
+                        "from item {}, Path: {}",
+                        item.id, dup_item.id, displayable_path(item.path))
+                item_fields = _reduce_and_log(item, dup_item._values_flex,
+                                              REIMPORT_FRESH_FIELDS_ITEM)
+                item.update(item_fields)
                 log.debug(
-                    'Reimported item flexible attributes {0} '
-                    'from item {1} for {2}',
-                    dup_item._values_flex.keys(),
-                    dup_item.id,
-                    displayable_path(item.path)
-                )
+                    'Reimported item {} flexible attributes: {} '
+                    'from item {}, Path: {}',
+                    item.id, list(item_fields.keys()), dup_item.id,
+                    displayable_path(item.path))
                 item.store()
 
     def remove_replaced(self, lib):
diff --git a/docs/changelog.rst b/docs/changelog.rst
index 397f4af2c8..8d03cd1d0d 100644
--- a/docs/changelog.rst
+++ b/docs/changelog.rst
@@ -171,6 +171,9 @@ Bug fixes:
   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`
+* Fix updating "data_source" on re-imports and improve logging when flexible
+  attributes are being re-imported.
+  :bug:`4726`
 
 For packagers:
 
diff --git a/test/test_importer.py b/test/test_importer.py
index 121de53d64..7de00c65ea 100644
--- a/test/test_importer.py
+++ b/test/test_importer.py
@@ -139,7 +139,8 @@ def _make_album_match(self, artist, album, tracks, distance=0, missing=0):
             va=False,
             album_id='albumid' + id,
             artist_id='artistid' + id,
-            albumtype='soundtrack'
+            albumtype='soundtrack',
+            data_source='match_source'
         )
 
 
@@ -1723,6 +1724,7 @@ def setUp(self):
         album = self.add_album_fixture()
         album.added = 4242.0
         album.foo = 'bar'  # Some flexible attribute.
+        album.data_source = 'original_source'
         album.store()
         item = album.items().get()
         item.baz = 'qux'
@@ -1804,6 +1806,12 @@ def test_reimported_item_preserves_art(self):
         if new_artpath != old_artpath:
             self.assertNotExists(old_artpath)
 
+    def test_reimported_album_not_preserves_flexattr(self):
+        self._setup_session()
+        self.assertEqual(self._album().data_source, 'original_source')
+        self.importer.run()
+        self.assertEqual(self._album().data_source, 'match_source')
+
 
 class ImportPretendTest(_common.TestCase, ImportHelper):
     """ Test the pretend commandline option


Code review comments:
diff_hunk: @@ -811,55 +811,74 @@ def reimport_metadata(self, lib):
         """For reimports, preserves metadata for reimported items and
         albums.
         """
+        # Album and item flex attrs that should not be preserved on reimports.
+        dont_preserve_album_flex = ['data_source']
+        dont_preserve_item_flex = ['data_source', 'bandcamp_album_id',
+                                   'spotify_album_id', 'deezer_album_id',
+                                   'beatport_album_id']
         if self.is_album:
             replaced_album = self.replaced_albums.get(self.album.path)
             if replaced_album:
+                preserved_album_attrs = dict(replaced_album._values_flex)
+
+                for attr in dont_preserve_album_flex:
+                    if not self.album.get(attr):
+                        continue
+                    new_val = self.album.get(attr)
+                    old_val = replaced_album.get(attr)
+                    if new_val != old_val:
+                        preserved_album_attrs.pop(attr, None)
+                        log.debug(
+                            'Not reimported album {} flexible attribute: {}. '
+                            'New value: {}, Path: {}',
+                            self.album.id, attr, new_val,
+                            displayable_path(self.album.path))
path: beets/importer.py
text: @author:
Awesome! Thanks for this great suggestion. Have a look at https://github.com/beetbox/beets/pull/4735/commits/8bac74480c6d0ffd57175808f0e52f187ce9342e

As often in IT, the hardest part was "naming things". I'm open for suggestions if you think some of my var or docstring names sound clunky.

@author:
It's funny that you used the term "preserving" - I'm having the exact same thought since I started to refactor this thing. I kind of found the term "reimported" in the log messages kind of confusing from the beginning when I first debugged my way into reimport_metadata() a couple of months ago.

What do you think about to change all existing log messages to use that term instead of the currently used "Reimported...."? 

@author:
A comparision of possible ways to log this:

1) Original version plus logging not-preserved values:

```
Setting album flexible attribute data_source: match_source for 1, /tmp/tmpu5nwti8q/libdir/singletons
Sending event: database_change
Sending event: database_change
Reimported album: added 4242.0, flexible attributes ['foo'] from album 1 for /tmp/tmpu5nwti8q/libdir/singletons
Reimported item added 4747.0 from item 1 for /tmp/tmpu5nwti8q/libdir/singletons/tïtle 0.mp3
Reimported item flexible attributes ['baz'] from item 1 for /tmp/tmpu5nwti8q/libdir/singletons/tïtle 0.mp3
```

2) Keeping the term "Reimported" and stating the old and the new album/item ID's.

```
Not reimported album 1 flexible attribute: data_source. New value: match_source, Path: /tmp/tmpddrw2quc/libdir/singletons
Sending event: database_change
Sending event: database_change
Reimported album 1 attribute: ['added'] from album 1, Path: /tmp/tmpddrw2quc/libdir/singletons
Reimported album 1 flexible attributes: ['foo'] from album 1, Path:/tmp/tmpddrw2quc/libdir/singletons
Reimported item 1 attribute: ['added'] from item 1, Path: /tmp/tmpddrw2quc/libdir/singletons/tïtle 0.mp3
Reimported item 1 flexible attributes: ['baz'] from item 1, Path: /tmp/tmpddrw2quc/libdir/singletons/tïtle 0.mp3
```

3) Use the term "Preserved/Preserving"

```
Not preserving album 1 flexible attributes: ['data_source']. Path: /tmp/tmpda4c_4c3/libdir/singletons
Sending event: database_change
Sending event: database_change
Preserved album 1 attribute: ['added'], Path: /tmp/tmpda4c_4c3/libdir/singletons
Preserved album 1 flexible attributes: ['foo'], Path:/tmp/tmpda4c_4c3/libdir/singletons
Preserved item 1 attribute: ['added'], Path: /tmp/tmpda4c_4c3/libdir/singletons/tïtle 0.mp3
Preserved item 1 flexible attributes: ['baz'], Path: /tmp/tmpda4c_4c3/libdir/singletons/tïtle 0.mp3
```

I removed the "from album/item <ID>" in that last proposal. I am assuming that never ever on a reimport the ID of the reimported albums/items will change. Do you have any doubts in that assumption?

I also removed the "new value" int that last proposal as it was in your listcomp idea. Probably that is enough and less is more in the logging. The list of keys can become annoyingly long in that messages, if for example AcousticBrainz/Xtractor fields are present.

@user1:
Yeah, 100% agree that the current log messages are not well thought-out. They are mainly there because they are better than not logging anything at all.

FWIW, I think it would be nice to include the word "reimport" _somewhere_ in the messages, so people know that this basic behavior has been invoked:
https://beets.readthedocs.io/en/stable/reference/cli.html#reimporting

But yeah, "preserving" is much better wording for what we're doing to the metadata during a reimport.

I think you're right that the ID will never change on a reimport! I am 60% confident in that assertion. :smiley: