Repository: beetbox/beets

Issue:
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()`



Discussion:
JOJ0: Hi @sampsyo, I would love to get a review from you. This might be tedious to look at in the final diff. I suggest looking at it commit-wise. The actual technical change you'll find in the first one: https://github.com/beetbox/beets/pull/4735/commits/bf29e4fb65c178ce88d622bed630da01363f9321

Thanks!

If you want I can post some logging examples comparing "before" and "after". If that might help?

Patch:
diff --git a/beets/importer.py b/beets/importer.py
index 96d1f17df..601f8246f 100644
--- a/beets/importer.py
+++ b/beets/importer.py
@@ -50,6 +50,11 @@ QUEUE_SIZE = 128
 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 @@ class ImportTask(BaseImportTask):
         """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 (rather than
+            preserved) on reimports; 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 isinstance(new_obj, library.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(
+                    'Reimported {} {}. 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 {}. Preserving attribute ['added']. "
+                    "Path: {}",
+                    self.album.id, displayable_path(self.album.path))
+                log.debug(
+                    'Reimported album {}. Preserving flexible attributes {}. '
+                    'Path: {}',
+                    self.album.id, list(album_fields.keys()),
+                    displayable_path(self.album.path))
 
         for item in self.imported_items():
             dup_items = self.replaced_items[item]
@@ -833,33 +863,17 @@ class ImportTask(BaseImportTask):
                 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 {}. Preserving attribute ['added']. "
+                        "Path: {}",
+                        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 {}. Preserving flexible attributes {}. '
+                    'Path: {}',
+                    item.id, list(item_fields.keys()),
+                    displayable_path(item.path))
                 item.store()
 
     def remove_replaced(self, lib):
diff --git a/docs/changelog.rst b/docs/changelog.rst
index 397f4af2c..8d03cd1d0 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:
 


Test patch:
diff --git a/test/test_importer.py b/test/test_importer.py
index 121de53d6..7de00c65e 100644
--- a/test/test_importer.py
+++ b/test/test_importer.py
@@ -139,7 +139,8 @@ class AutotagStub:
             va=False,
             album_id='albumid' + id,
             artist_id='artistid' + id,
-            albumtype='soundtrack'
+            albumtype='soundtrack',
+            data_source='match_source'
         )
 
 
@@ -1723,6 +1724,7 @@ class ReimportTest(unittest.TestCase, ImportHelper, _common.Assertions):
         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 @@ class ReimportTest(unittest.TestCase, ImportHelper, _common.Assertions):
         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