diff --git a/src/libs/database/impl/objects/Artist.cpp b/src/libs/database/impl/objects/Artist.cpp index 3b0053ec..f74971d0 100644 --- a/src/libs/database/impl/objects/Artist.cpp +++ b/src/libs/database/impl/objects/Artist.cpp @@ -334,6 +334,31 @@ AND NOT EXISTS ( return utils::fetchQuerySingleResult(session.getDboSession()->query("SELECT 1 FROM artist").where("id = ?").bind(id)) == 1; } + RangeResults Artist::findWithMBIDNameVariants(Session& session, ArtistId& lastRetrievedArtist, std::optional range) + { + session.checkReadTransaction(); + + auto query{ session.getDboSession()->query>(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(query, range) }; + + if (!results.results.empty()) + lastRetrievedArtist = results.results.back()->getId(); + + return results; + } + void Artist::updatePreferredArtwork(Session& session, ArtistId artistId, ArtworkId artworkId) { session.checkWriteTransaction(); diff --git a/src/libs/database/impl/objects/Track.cpp b/src/libs/database/impl/objects/Track.cpp index 119f9644..d42dcf48 100644 --- a/src/libs/database/impl/objects/Track.cpp +++ b/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; diff --git a/src/libs/database/impl/objects/TrackArtistLink.cpp b/src/libs/database/impl/objects/TrackArtistLink.cpp index 90a373ff..f0bfd591 100644 --- a/src/libs/database/impl/objects/TrackArtistLink.cpp +++ b/src/libs/database/impl/objects/TrackArtistLink.cpp @@ -50,10 +50,22 @@ 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"); - query.where("t.release_id = ?").bind(params.release); + + 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; @@ -112,11 +124,8 @@ namespace lms::db void TrackArtistLink::find(Session& session, const FindParameters& parameters, const std::function& func) { - const auto query{ createQuery(session, parameters) }; - - utils::forEachQueryResult(query, [&](const TrackArtistLink::pointer& link) { - func(link); - }); + auto query{ createQuery(session, parameters) }; + utils::forEachQueryRangeResult(query, parameters.range, func); } core::EnumSet TrackArtistLink::findUsedTypes(Session& session, ArtistId artistId) diff --git a/src/libs/database/include/database/Types.hpp b/src/libs/database/include/database/Types.hpp index b5845738..ac0350f0 100644 --- a/src/libs/database/include/database/Types.hpp +++ b/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, diff --git a/src/libs/database/include/database/objects/Artist.hpp b/src/libs/database/include/database/objects/Artist.hpp index 6310e283..7cae8afd 100644 --- a/src/libs/database/include/database/objects/Artist.hpp +++ b/src/libs/database/include/database/objects/Artist.hpp @@ -136,6 +136,7 @@ namespace lms::db static RangeResults findIds(Session& session, const FindParameters& params); static RangeResults findOrphanIds(Session& session, std::optional range = std::nullopt); // No track related static bool exists(Session& session, ArtistId id); + static RangeResults findWithMBIDNameVariants(Session& session, ArtistId& lastRetrievedArtist, std::optional range = std::nullopt); // Updates static void updatePreferredArtwork(Session& session, ArtistId artistId, ArtworkId artworkId); diff --git a/src/libs/database/include/database/objects/TrackArtistLink.hpp b/src/libs/database/include/database/objects/TrackArtistLink.hpp index 1f9e6117..03527f64 100644 --- a/src/libs/database/include/database/objects/TrackArtistLink.hpp +++ b/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) { @@ -77,6 +78,11 @@ namespace lms::db track = _track; return *this; } + FindParameters& setSortMethod(TrackArtistLinkSortMethod _method) + { + sortMethod = _method; + return *this; + } }; TrackArtistLink() = default; diff --git a/src/libs/database/test/Artist.cpp b/src/libs/database/test/Artist.cpp index 69abbfd0..0e443db8 100644 --- a/src/libs/database/test/Artist.cpp +++ b/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 diff --git a/src/libs/database/test/TrackArtistLink.cpp b/src/libs/database/test/TrackArtistLink.cpp index d8e512a2..e827cc31 100644 --- a/src/libs/database/test/TrackArtistLink.cpp +++ b/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(track1.get(), artist.get(), TrackArtistLinkType::Artist, false) }; + link1.modify()->setArtistName("MyArtistOldName"); + track1.get().modify()->setOriginalDate(core::PartialDateTime{ 1990, 1 }); + } + + { + auto link2{ session.create(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 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 \ No newline at end of file diff --git a/src/libs/services/scanner/impl/helpers/ArtistHelpers.cpp b/src/libs/services/scanner/impl/helpers/ArtistHelpers.cpp index a380d9a3..43d47ace 100644 --- a/src/libs/services/scanner/impl/helpers/ArtistHelpers.cpp +++ b/src/libs/services/scanner/impl/helpers/ArtistHelpers.cpp @@ -39,55 +39,25 @@ namespace lms::scanner::helpers return artist; } - - std::string optionalMBIDAsString(const std::optional& uuid) - { - return uuid ? std::string{ uuid->getAsString() } : ""; - } - - 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) - { - updateArtistIfNeeded(artist, artistInfo); - } - else + if (!artist) { 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); } diff --git a/src/libs/services/scanner/impl/scanners/artistinfo/ArtistInfoFileScanner.cpp b/src/libs/services/scanner/impl/scanners/artistinfo/ArtistInfoFileScanner.cpp index 5ed7c9c9..7e333441 100644 --- a/src/libs/services/scanner/impl/scanners/artistinfo/ArtistInfoFileScanner.cpp +++ b/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(_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()); diff --git a/src/libs/services/scanner/impl/steps/ScanStepArtistReconciliation.cpp b/src/libs/services/scanner/impl/steps/ScanStepArtistReconciliation.cpp index b98997a8..f7df576c 100644 --- a/src/libs/services/scanner/impl/steps/ScanStepArtistReconciliation.cpp +++ b/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 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; + + 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 }; diff --git a/src/libs/services/scanner/impl/steps/ScanStepArtistReconciliation.hpp b/src/libs/services/scanner/impl/steps/ScanStepArtistReconciliation.hpp index 6121dcb4..7e4e1e95 100644 --- a/src/libs/services/scanner/impl/steps/ScanStepArtistReconciliation.hpp +++ b/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);