When an artist is referred to using several names, pick first the one set in the artist info file, and fallback on the most recent release, ref #731

src/libs/database/impl/objects/Artist.cpp

@@ -334,6 +334,31 @@ AND NOT EXISTS (
         return utils::fetchQuerySingleResult(session.getDboSession()->query<int>("SELECT 1 FROM artist").where("id = ?").bind(id)) == 1;
     }
 
+    RangeResults<Artist::pointer> Artist::findWithMBIDNameVariants(Session& session, ArtistId& lastRetrievedArtist, std::optional<Range> range)
+    {
+        session.checkReadTransaction();
+
+        auto query{ session.getDboSession()->query<Wt::Dbo::ptr<Artist>>(R"(
+        SELECT a FROM artist a 
+        WHERE a.id IN (
+            SELECT t_a_l.artist_id 
+            FROM track_artist_link t_a_l 
+            WHERE t_a_l.artist_mbid_matched = 1 
+            GROUP BY t_a_l.artist_id 
+            HAVING COUNT(DISTINCT t_a_l.artist_name) > 1
+        )
+        AND a.id > ?
+    )")
+                        .bind(lastRetrievedArtist) };
+
+        auto results{ utils::execRangeQuery<Artist::pointer>(query, range) };
+
+        if (!results.results.empty())
+            lastRetrievedArtist = results.results.back()->getId();
+
+        return results;
+    }
+
     void Artist::updatePreferredArtwork(Session& session, ArtistId artistId, ArtworkId artworkId)
     {
         session.checkWriteTransaction();

src/libs/database/impl/objects/Track.cpp

@@ -162,6 +162,7 @@ namespace lms::db
                 query.where("t.track_number = ?").bind(*params.trackNumber);
 
             if (params.sortMethod == TrackSortMethod::DateDescAndRelease
+                || params.sortMethod == TrackSortMethod::OriginalDateDescAndRelease
                 || params.sortMethod == TrackSortMethod::Release)
             {
                 query.join("medium m ON t.medium_id = m.id");
@@ -223,6 +224,9 @@ namespace lms::db
             case TrackSortMethod::DateDescAndRelease:
                 query.orderBy("t.date DESC,t.release_id,m.position,t.track_number");
                 break;
+            case TrackSortMethod::OriginalDateDescAndRelease:
+                query.orderBy("COALESCE(t.original_date, t.date) DESC,t.release_id,m.position,t.track_number");
+                break;
             case TrackSortMethod::Release:
                 query.orderBy("m.position,t.track_number");
                 break;

src/libs/database/impl/objects/TrackArtistLink.cpp

@@ -50,12 +50,24 @@ namespace lms::db
             if (params.artist.isValid())
                 query.where("t_a_l.artist_id = ?").bind(params.artist);
 
-            if (params.release.isValid())
+            if (params.release.isValid()
+                || params.sortMethod == TrackArtistLinkSortMethod::OriginalDateDesc)
             {
                 query.join("track t ON t.id = t_a_l.track_id");
+
+                if (params.release.isValid())
                     query.where("t.release_id = ?").bind(params.release);
             }
 
+            switch (params.sortMethod)
+            {
+            case TrackArtistLinkSortMethod::None:
+                break;
+            case TrackArtistLinkSortMethod::OriginalDateDesc:
+                query.orderBy("COALESCE(t.original_date, t.date) DESC");
+                break;
+            }
+
             return query;
         }
     } // namespace
@@ -112,11 +124,8 @@ namespace lms::db
 
     void TrackArtistLink::find(Session& session, const FindParameters& parameters, const std::function<void(const TrackArtistLink::pointer&)>& func)
     {
-        const auto query{ createQuery(session, parameters) };
+        auto query{ createQuery(session, parameters) };
-
+        utils::forEachQueryRangeResult(query, parameters.range, func);
-        utils::forEachQueryResult(query, [&](const TrackArtistLink::pointer& link) {
-            func(link);
-        });
     }
 
     core::EnumSet<TrackArtistLinkType> TrackArtistLink::findUsedTypes(Session& session, ArtistId artistId)

src/libs/database/include/database/Types.hpp

@@ -193,6 +193,12 @@ namespace lms::db
         Name,
     };
 
+    enum class TrackArtistLinkSortMethod
+    {
+        None,
+        OriginalDateDesc,
+    };
+
     enum class TrackEmbeddedImageSortMethod
     {
         None,
@@ -220,6 +226,7 @@ namespace lms::db
         AbsoluteFilePath,
         Name,
         DateDescAndRelease,
+        OriginalDateDescAndRelease,
         Release,   // order by disc/track number
         TrackList, // order by asc order in tracklist
         TrackNumber,

src/libs/database/include/database/objects/Artist.hpp

@@ -136,6 +136,7 @@ namespace lms::db
         static RangeResults<ArtistId> findIds(Session& session, const FindParameters& params);
         static RangeResults<ArtistId> findOrphanIds(Session& session, std::optional<Range> range = std::nullopt); // No track related
         static bool exists(Session& session, ArtistId id);
+        static RangeResults<pointer> findWithMBIDNameVariants(Session& session, ArtistId& lastRetrievedArtist, std::optional<Range> range = std::nullopt);
 
         // Updates
         static void updatePreferredArtwork(Session& session, ArtistId artistId, ArtworkId artworkId);

src/libs/database/include/database/objects/TrackArtistLink.hpp

@@ -51,6 +51,7 @@ namespace lms::db
             ArtistId artist;                             // if set, links involved with this artist
             ReleaseId release;                           // if set, artists involved in this release
             TrackId track;                               // if set, artists involved in this track
+            TrackArtistLinkSortMethod sortMethod{ TrackArtistLinkSortMethod::None };
 
             FindParameters& setRange(std::optional<Range> _range)
             {
@@ -77,6 +78,11 @@ namespace lms::db
                 track = _track;
                 return *this;
             }
+            FindParameters& setSortMethod(TrackArtistLinkSortMethod _method)
+            {
+                sortMethod = _method;
+                return *this;
+            }
         };
 
         TrackArtistLink() = default;

src/libs/database/test/Artist.cpp

@@ -934,4 +934,43 @@ namespace lms::db::tests
             EXPECT_EQ(artist->getPreferredArtwork(), Artwork::pointer{});
         }
     }
+
+    TEST_F(DatabaseFixture, Artist_findWithMBIDNameVariants)
+    {
+        ScopedArtist artistA{ session, "ArtistA" };
+        ScopedArtist artistB{ session, "ArtistB" };
+
+        ScopedTrack trackA1{ session };
+        ScopedTrack trackA2{ session };
+        ScopedTrack trackB1{ session };
+
+        {
+            auto transaction{ session.createWriteTransaction() };
+
+            {
+                auto link{ TrackArtistLink::create(session, trackA1.get(), artistA.get(), TrackArtistLinkType::Artist, true) };
+                link.modify()->setArtistName("ArtistA");
+            }
+            {
+                auto link{ TrackArtistLink::create(session, trackA2.get(), artistA.get(), TrackArtistLinkType::Artist, true) };
+                link.modify()->setArtistName("AlternateArtistA");
+            }
+
+            {
+                auto link{ TrackArtistLink::create(session, trackB1.get(), artistB.get(), TrackArtistLinkType::Artist, true) };
+                link.modify()->setArtistName("ArtistB");
+            }
+        }
+
+        {
+            auto transaction{ session.createReadTransaction() };
+
+            ArtistId lastRetrievedArtist;
+            const auto results{ Artist::findWithMBIDNameVariants(session, lastRetrievedArtist) };
+
+            ASSERT_EQ(results.results.size(), 1);
+            EXPECT_EQ(results.results[0]->getId(), artistA.getId());
+            EXPECT_EQ(lastRetrievedArtist, artistA.getId());
+        }
+    }
 } // namespace lms::db::tests

src/libs/database/test/TrackArtistLink.cpp

@@ -18,8 +18,8 @@
  */
 
 #include "Common.hpp"
-#include "database/Types.hpp"
 
+#include "database/Types.hpp"
 #include "database/objects/TrackArtistLink.hpp"
 
 namespace lms::db::tests
@@ -183,4 +183,43 @@ namespace lms::db::tests
             }
         }
     }
+
+    TEST_F(DatabaseFixture, TrackArtistLink_findWithOriginalDateDesc)
+    {
+        ScopedArtist artist{ session, "MyArtist" };
+        ScopedTrack track1{ session };
+        ScopedTrack track2{ session };
+
+        {
+            auto transaction{ session.createWriteTransaction() };
+
+            {
+                auto link1{ session.create<TrackArtistLink>(track1.get(), artist.get(), TrackArtistLinkType::Artist, false) };
+                link1.modify()->setArtistName("MyArtistOldName");
+                track1.get().modify()->setOriginalDate(core::PartialDateTime{ 1990, 1 });
+            }
+
+            {
+                auto link2{ session.create<TrackArtistLink>(track2.get(), artist.get(), TrackArtistLinkType::Artist, false) };
+                link2.modify()->setArtistName("MyArtistNewName");
+                track2.get().modify()->setOriginalDate(core::PartialDateTime{ 1995, 1 });
+            }
+        }
+
+        {
+            auto transaction{ session.createReadTransaction() };
+
+            TrackArtistLink::FindParameters params;
+            params.setSortMethod(TrackArtistLinkSortMethod::OriginalDateDesc);
+            params.setArtist(artist->getId());
+
+            std::vector<TrackArtistLink::pointer> links;
+            TrackArtistLink::find(session, params, [&](const TrackArtistLink::pointer& link) {
+                links.push_back(link);
+            });
+            ASSERT_EQ(links.size(), 2);
+            EXPECT_EQ(links[0]->getArtistName(), "MyArtistNewName");
+            EXPECT_EQ(links[1]->getArtistName(), "MyArtistOldName");
+        }
+    }
 } // namespace lms::db::tests

src/libs/services/scanner/impl/helpers/ArtistHelpers.cpp

@@ -39,55 +39,25 @@ namespace lms::scanner::helpers
 
             return artist;
         }
-
-        std::string optionalMBIDAsString(const std::optional<core::UUID>& uuid)
-        {
-            return uuid ? std::string{ uuid->getAsString() } : "<no MBID>";
-        }
-
-        void updateArtistIfNeeded(db::Artist::pointer artist, const Artist& artistInfo)
-        {
-            // MBID may be set
-            if (artist->getMBID() != artistInfo.mbid)
-                artist.modify()->setMBID(artistInfo.mbid);
-
-            // Name may have been updated
-            if (artist->getName() != artistInfo.name)
-            {
-                LMS_LOG(DBUPDATER, DEBUG, "Artist [" << optionalMBIDAsString(artist->getMBID()) << "], updated name from '" << artist->getName() << "' to '" << artistInfo.name << "'");
-                artist.modify()->setName(artistInfo.name);
-            }
-
-            // Sortname may have been updated
-            // As the sort name is quite often not filled in, we update it only if already set (for now?)
-            if (artistInfo.sortName && *artistInfo.sortName != artist->getSortName())
-            {
-                LMS_LOG(DBUPDATER, DEBUG, "Artist [" << optionalMBIDAsString(artist->getMBID()) << "], updated sort name from '" << artist->getSortName() << "' to '" << *artistInfo.sortName << "'");
-                artist.modify()->setSortName(*artistInfo.sortName);
-            }
-        }
-
     } // namespace
 
     db::Artist::pointer getOrCreateArtistByMBID(db::Session& session, const Artist& artistInfo, AllowFallbackOnMBIDEntry allowFallbackOnMBIDEntries)
     {
         assert(artistInfo.mbid.has_value());
+
         db::Artist::pointer artist{ db::Artist::find(session, *artistInfo.mbid) };
-        if (artist)
+        if (!artist)
-        {
-            updateArtistIfNeeded(artist, artistInfo);
-        }
-        else
         {
             if (allowFallbackOnMBIDEntries.value())
             {
                 // an artist with the same name may already exist, let's recycle it
                 for (const db::Artist::pointer& artistWithSameName : db::Artist::find(session, artistInfo.name))
                 {
+                    assert(artistWithSameName->getMBID() != artistInfo.mbid);
                     if (!artistWithSameName->hasMBID())
                     {
                         artist = artistWithSameName;
-                        updateArtistIfNeeded(artist, artistInfo);
+                        artist.modify()->setMBID(artistInfo.mbid);
                         break;
                     }
                 }
@@ -102,6 +72,8 @@ namespace lms::scanner::helpers
 
     db::Artist::pointer getOrCreateArtistByName(db::Session& session, const Artist& artistInfo, AllowFallbackOnMBIDEntry allowFallbackOnMBIDEntries)
     {
+        assert(artistInfo.mbid == std::nullopt);
+
         db::Artist::pointer artist;
 
         // Here we can have only one artist with no MBID, others all have mbids
@@ -112,10 +84,7 @@ namespace lms::scanner::helpers
         if (!allowFallbackOnMBIDEntries.value() || artistCountWithMBID > 1)
         {
             if (itArtistWithoutMBID != std::end(artistsWithSameName))
-            {
                 artist = *itArtistWithoutMBID;
-                updateArtistIfNeeded(artist, artistInfo);
-            }
             else
                 artist = createArtist(session, artistInfo);
         }
@@ -124,15 +93,9 @@ namespace lms::scanner::helpers
             const auto itArtistWithMBID{ std::find_if(std::begin(artistsWithSameName), std::end(artistsWithSameName), [](const db::Artist::pointer& artist) { return artist->hasMBID(); }) };
 
             if (itArtistWithMBID != std::end(artistsWithSameName))
-            {
                 artist = *itArtistWithMBID;
-                // not updating artist here: consider metadata quality is less good
-            }
             else if (itArtistWithoutMBID != std::end(artistsWithSameName))
-            {
                 artist = *itArtistWithoutMBID;
-                updateArtistIfNeeded(artist, artistInfo);
-            }
             else
                 artist = createArtist(session, artistInfo);
         }

src/libs/services/scanner/impl/scanners/artistinfo/ArtistInfoFileScanner.cpp

@@ -125,7 +125,21 @@ namespace lms::scanner
             artistInfo.modify()->setDirectory(utils::getOrCreateDirectory(dbSession, getFilePath().parent_path(), mediaLibrary));
 
             const Artist artistMetadata{ _parsedArtistInfo->mbid, _parsedArtistInfo->name, _parsedArtistInfo->sortName.empty() ? std::nullopt : std::make_optional<std::string>(_parsedArtistInfo->sortName) };
+
             db::Artist::pointer artist{ helpers::getOrCreateArtist(dbSession, artistMetadata, helpers::AllowFallbackOnMBIDEntry{ getScannerSettings().allowArtistMBIDFallback }) };
+            // Artist info is the highest priority source for artist name/sort name, so update it as needed
+            if (artist->getName() != artistMetadata.name)
+            {
+                LMS_LOG(DBUPDATER, DEBUG, "Updated artist name from '" << artist->getName() << "' to '" << artistMetadata.name << "' using artist info file");
+                artist.modify()->setName(artistMetadata.name);
+            }
+
+            if (artist->getSortName() != artistMetadata.sortName)
+            {
+                LMS_LOG(DBUPDATER, DEBUG, "Updated artist sort name from '" << artist->getSortName() << "' to '" << (artistMetadata.sortName ? *artistMetadata.sortName : "") << "' using artist info file");
+                artist.modify()->setSortName(artistMetadata.sortName ? *artistMetadata.sortName : "");
+            }
+
             artistInfo.modify()->setArtist(artist);
             artistInfo.modify()->setMBIDMatched(_parsedArtistInfo->mbid.has_value() && _parsedArtistInfo->mbid == artist->getMBID());
 

src/libs/services/scanner/impl/steps/ScanStepArtistReconciliation.cpp

@@ -83,14 +83,17 @@ namespace lms::scanner
 
     void ScanStepArtistReconciliation::process(ScanContext& context)
     {
-        // Reconcile artist links
+        // Reconcile artist name differences when MBID was used to match
+        updateArtistPreferredName(context);
+
+        // Reconcile artist links when MBID not used to match
         {
             // Order is important
             updateLinksForArtistNameNoLongerMatch(context);
             updateLinksWithArtistNameAmbiguity(context);
         }
 
-        // Reconcile artist info
+        // Reconcile artist info when MBID not used to match
         {
             // Order is important
             updateArtistInfoForArtistNameNoLongerMatch(context);
@@ -98,6 +101,97 @@ namespace lms::scanner
         }
     }
 
+    void ScanStepArtistReconciliation::updateArtistPreferredName(ScanContext& context)
+    {
+        static constexpr std::size_t batchSize{ 50 };
+
+        db::Session& session{ _db.getTLSSession() };
+
+        // List artists that have different names when mbid matched.
+        // Possible reasons:
+        // - artist name changed over time (ex: Rhapsody then Rhapsody of Fire), legit use case
+        // - user renamed the artist
+        // Name to pick in order of priority:
+        // - name specified in artist info (if present)
+        // - name as referenced in the latest release
+
+        struct ArtistToUpdate
+        {
+            db::Artist::pointer artist;
+            std::string newName;
+            std::string newSortName;
+        };
+        std::vector<ArtistToUpdate> artistsToUpdate;
+        auto updateArtists{ [&] {
+            auto transaction{ session.createWriteTransaction() };
+
+            for (auto& artistToUpdate : artistsToUpdate)
+            {
+                artistToUpdate.artist.modify()->setName(artistToUpdate.newName);
+                artistToUpdate.artist.modify()->setSortName(artistToUpdate.newSortName);
+            }
+        } };
+
+        db::ArtistId lastRetrievedArtist;
+        while (!_abortScan)
+        {
+            {
+                auto transaction{ session.createReadTransaction() };
+
+                const auto artists{ db::Artist::findWithMBIDNameVariants(session, lastRetrievedArtist, db::Range{ .offset = 0, .size = batchSize }) };
+                if (artists.results.empty())
+                    break;
+
+                for (const db::Artist::pointer& artist : artists.results)
+                {
+                    bool hasArtistInfo{};
+                    db::ArtistInfo::find(session, artist->getId(), db::Range{ .offset = 0, .size = 1 }, [&](const db::ArtistInfo::pointer&) {
+                        hasArtistInfo = true;
+                    });
+
+                    // Scanning artist info should have updated the name of the artist
+                    if (hasArtistInfo)
+                        continue;
+
+                    std::optional<ArtistToUpdate> artistToUpdate;
+
+                    db::TrackArtistLink::FindParameters params;
+                    params.setArtist(artist->getId());
+                    params.setSortMethod(db::TrackArtistLinkSortMethod::OriginalDateDesc);
+                    params.setRange(db::Range{ .offset = 0, .size = 1 });
+                    db::TrackArtistLink::find(session, params, [&](const db::TrackArtistLink::pointer& link) {
+                        if (link->getArtistName() != artist->getName())
+                        {
+                            artistToUpdate.emplace();
+                            artistToUpdate->artist = artist;
+                            artistToUpdate->newName = link->getArtistName();
+                            artistToUpdate->newSortName = link->getArtistSortName();
+                        }
+                    });
+
+                    if (artistToUpdate)
+                    {
+                        LMS_LOG(DBUPDATER, DEBUG, "Updating artist " << artist << " name to '" << artistToUpdate->newName << "' using most recent release reference");
+                        artistsToUpdate.emplace_back(std::move(*artistToUpdate));
+                    }
+                }
+
+                if (!artists.moreResults)
+                    break;
+            }
+
+            if (artistsToUpdate.size() > batchSize)
+            {
+                updateArtists();
+                artistsToUpdate.clear();
+            }
+        }
+
+        updateArtists();
+
+        _progressCallback(context.currentStepStats);
+    }
+
     void ScanStepArtistReconciliation::updateArtistInfoForArtistNameNoLongerMatch(ScanContext& context)
     {
         static constexpr std::size_t batchSize{ 50 };

src/libs/services/scanner/impl/steps/ScanStepArtistReconciliation.hpp

@@ -34,6 +34,8 @@ namespace lms::scanner
         bool needProcess(const ScanContext& context) const override;
         void process(ScanContext& context) override;
 
+        void updateArtistPreferredName(ScanContext& context);
+
         void updateLinksForArtistNameNoLongerMatch(ScanContext& context);
         void updateLinksWithArtistNameAmbiguity(ScanContext& context);
         void updateArtistInfoForArtistNameNoLongerMatch(ScanContext& context);