More permissive grouping of multi disc albums (each disc can now be s…

…plit on sub directories) when musicbrainz ids are not set, fixes #481

src/libs/database/impl/Release.cpp

@@ -41,6 +41,9 @@ namespace lms::db
         template<typename ResultType>
         Wt::Dbo::Query<ResultType> createQuery(Session& session, std::string_view itemToSelect, const Release::FindParameters& params)
         {
+            assert(params.keywords.empty() || params.name.empty());
+            assert(!params.directory.isValid() || !params.parentDirectory.isValid());
+
             auto query{ session.getDboSession()->query<ResultType>("SELECT " + std::string{ itemToSelect } + " from release r") };
 
             if (params.sortMethod == ReleaseSortMethod::ArtistNameThenName
@@ -54,11 +57,18 @@ namespace lms::db
                 || params.artist.isValid()
                 || params.clusters.size() == 1
                 || params.mediaLibrary.isValid()
-                || params.directory.isValid())
+                || params.directory.isValid()
+                || params.parentDirectory.isValid())
             {
                 query.join("track t ON t.release_id = r.id");
             }
 
+            if (params.parentDirectory.isValid())
+            {
+                query.join("directory d ON t.directory_id = d.id");
+                query.where("d.parent_directory_id = ?").bind(params.parentDirectory);
+            }
+
             if (params.mediaLibrary.isValid())
                 query.where("t.media_library_id = ?").bind(params.mediaLibrary);
 
@@ -82,6 +92,9 @@ namespace lms::db
                 query.where("COALESCE(CAST(SUBSTR(t.date, 1, 4) AS INTEGER), t.year) <= ?").bind(params.dateRange->end);
             }
 
+            if (!params.name.empty())
+                query.where("r.name = ?").bind(params.name);
+
             for (std::string_view keyword : params.keywords)
                 query.where("r.name LIKE ? ESCAPE '" ESCAPE_CHAR_STR "'").bind("%" + utils::escapeLikeKeyword(keyword) + "%");
 
@@ -287,13 +300,6 @@ namespace lms::db
         return session.getDboSession()->add(std::unique_ptr<Release>{ new Release{ name, MBID } });
     }
 
-    std::vector<Release::pointer> Release::find(Session& session, const std::string& name, const std::filesystem::path& releaseDirectory)
-    {
-        session.checkReadTransaction();
-
-        return utils::fetchQueryResults<Release::pointer>(session.getDboSession()->query<Wt::Dbo::ptr<Release>>("SELECT DISTINCT r from release r").join("track t ON t.release_id = r.id").where("r.name = ?").bind(std::string(name, 0, _maxNameLength)).where("t.absolute_file_path LIKE ? ESCAPE '" ESCAPE_CHAR_STR "'").bind(utils::escapeLikeKeyword(releaseDirectory.string()) + "%"));
-    }
-
     Release::pointer Release::find(Session& session, const core::UUID& mbid)
     {
         session.checkReadTransaction();

src/libs/database/impl/Session.cpp

@@ -189,6 +189,7 @@ namespace lms::db
         _session.execute("CREATE INDEX IF NOT EXISTS cluster_type_name_idx ON cluster_type(name)");
 
         _session.execute("CREATE INDEX IF NOT EXISTS directory_id_idx ON directory(id)");
+        _session.execute("CREATE INDEX IF NOT EXISTS directory_parent_directory_idx ON directory(parent_directory_id)");
         _session.execute("CREATE INDEX IF NOT EXISTS directory_path_idx ON directory(absolute_path)");
         _session.execute("CREATE INDEX IF NOT EXISTS directory_media_library_idx ON directory(media_library_id)");
 
@@ -207,34 +208,35 @@ namespace lms::db
         _session.execute("CREATE INDEX IF NOT EXISTS listen_track_user_backend_idx ON listen(track_id,user_id,backend)");
         _session.execute("CREATE INDEX IF NOT EXISTS listen_user_track_backend_date_time_idx ON listen(user_id,track_id,backend,date_time)");
 
+        _session.execute("CREATE INDEX IF NOT EXISTS media_library_id_idx ON media_library(id)");
+
         _session.execute("CREATE INDEX IF NOT EXISTS rated_artist_user_artist_idx ON rated_artist(user_id,artist_id)");
         _session.execute("CREATE INDEX IF NOT EXISTS rated_release_user_release_idx ON rated_release(user_id,release_id)");
         _session.execute("CREATE INDEX IF NOT EXISTS rated_track_user_track_idx ON rated_track(user_id,track_id)");
 
         _session.execute("CREATE INDEX IF NOT EXISTS release_id_idx ON release(id)");
+        _session.execute("CREATE INDEX IF NOT EXISTS release_mbid_idx ON release(mbid)");
         _session.execute("CREATE INDEX IF NOT EXISTS release_name_idx ON release(name)");
         _session.execute("CREATE INDEX IF NOT EXISTS release_name_nocase_idx ON release(name COLLATE NOCASE)");
-        _session.execute("CREATE INDEX IF NOT EXISTS release_mbid_idx ON release(mbid)");
         _session.execute("CREATE INDEX IF NOT EXISTS release_type_name_idx ON release_type(name)");
 
         _session.execute("CREATE INDEX IF NOT EXISTS track_id_idx ON track(id)");
         _session.execute("CREATE INDEX IF NOT EXISTS track_absolute_path_idx ON track(absolute_file_path)");
+        _session.execute("CREATE INDEX IF NOT EXISTS track_date_idx ON track(date)");
         _session.execute("CREATE INDEX IF NOT EXISTS track_directory_release_idx ON track(directory_id, release_id);");
+        _session.execute("CREATE INDEX IF NOT EXISTS track_file_last_write_idx ON track(file_last_write)");
+        _session.execute("CREATE INDEX IF NOT EXISTS track_media_library_idx ON track(media_library_id)");
+        _session.execute("CREATE INDEX IF NOT EXISTS track_media_library_release_idx ON track(media_library_id, release_id)");
+        _session.execute("CREATE INDEX IF NOT EXISTS track_mbid_idx ON track(mbid)");
         _session.execute("CREATE INDEX IF NOT EXISTS track_name_idx ON track(name)");
         _session.execute("CREATE INDEX IF NOT EXISTS track_name_nocase_idx ON track(name COLLATE NOCASE)");
-        _session.execute("CREATE INDEX IF NOT EXISTS track_mbid_idx ON track(mbid)");
+        _session.execute("CREATE INDEX IF NOT EXISTS track_original_date_idx ON track(original_date)");
+        _session.execute("CREATE INDEX IF NOT EXISTS track_original_year_idx ON track(original_year)");
         _session.execute("CREATE INDEX IF NOT EXISTS track_recording_mbid_idx ON track(recording_mbid)");
         _session.execute("CREATE INDEX IF NOT EXISTS track_release_idx ON track(release_id)");
         _session.execute("CREATE INDEX IF NOT EXISTS track_release_file_last_write_idx ON track(release_id, file_last_write)");
         _session.execute("CREATE INDEX IF NOT EXISTS track_release_year_idx ON track(release_id, year)");
-        _session.execute("CREATE INDEX IF NOT EXISTS track_file_last_write_idx ON track(file_last_write)");
-        _session.execute("CREATE INDEX IF NOT EXISTS track_date_idx ON track(date)");
         _session.execute("CREATE INDEX IF NOT EXISTS track_year_idx ON track(year)");
-        _session.execute("CREATE INDEX IF NOT EXISTS track_original_date_idx ON track(original_date)");
-        _session.execute("CREATE INDEX IF NOT EXISTS track_original_year_idx ON track(original_year)");
-
-        _session.execute("CREATE INDEX IF NOT EXISTS track_media_library_idx ON track(media_library_id)");
-        _session.execute("CREATE INDEX IF NOT EXISTS track_media_library_release_idx ON track(media_library_id, release_id)");
 
         _session.execute("CREATE INDEX IF NOT EXISTS tracklist_name_idx ON tracklist(name)");
         _session.execute("CREATE INDEX IF NOT EXISTS tracklist_user_idx ON tracklist(user_id)");

src/libs/database/include/database/Directory.hpp

@@ -109,6 +109,7 @@ namespace lms::db
         const std::filesystem::path& getAbsolutePath() const { return _absolutePath; }
         std::string_view getName() const { return _name; }
         ObjectPtr<Directory> getParentDirectory() const { return _parent; }
+        DirectoryId getParentDirectoryId() const { return _parent.id(); }
         ObjectPtr<MediaLibrary> getMediaLibrary() const { return _mediaLibrary; }
 
         // setters

src/libs/database/include/database/IdType.hpp

@@ -22,6 +22,7 @@
 #include <Wt/Dbo/ptr.h>
 #include <cassert>
 #include <functional>
+#include <string>
 
 namespace lms::db
 {
@@ -32,7 +33,7 @@ namespace lms::db
 
         IdType() = default;
         IdType(ValueType id)
-            : _id{ id } { assert(isValid()); }
+            : _id{ id } { }
 
         bool isValid() const { return _id != Wt::Dbo::dbo_default_traits::invalidId(); }
         std::string toString() const

src/libs/database/include/database/Release.hpp

@@ -114,7 +114,8 @@ namespace lms::db
         struct FindParameters
         {
             std::vector<ClusterId> clusters;        // if non empty, releases that belong to these clusters
-            std::vector<std::string_view> keywords; // if non empty, name must match all of these keywords
+            std::vector<std::string_view> keywords; // if non empty, name must match all of these keywords (cannot be set with keywords)
+            std::string name; // must match this name (cannot be set with keywords)
             ReleaseSortMethod sortMethod{ ReleaseSortMethod::None };
             std::optional<Range> range;
             Wt::WDateTime writtenAfter;
@@ -126,7 +127,8 @@ namespace lms::db
             core::EnumSet<TrackArtistLinkType> excludedTrackArtistLinkTypes; //    but not for these link types
             std::string releaseType;                                         // If set, albums that has this release type
             MediaLibraryId mediaLibrary;                                     // If set, releases that has at least a track in this library
-            DirectoryId directory;                                           // if set, tracks in this directory
+            DirectoryId directory;                                           // if set, releases in this directory (cannot be set with parent directory)
+            DirectoryId parentDirectory;                                     // if set, releases in this parent directory (cannot be set with directory)
 
             FindParameters& setClusters(std::span<const ClusterId> _clusters)
             {
@@ -138,6 +140,11 @@ namespace lms::db
                 keywords = _keywords;
                 return *this;
             }
+            FindParameters& setName(std::string_view _name)
+            {
+                name = _name;
+                return *this;
+            }
             FindParameters& setSortMethod(ReleaseSortMethod _sortMethod)
             {
                 sortMethod = _sortMethod;
@@ -186,6 +193,11 @@ namespace lms::db
                 directory = _directory;
                 return *this;
             }
+            FindParameters& setParentDirectory(DirectoryId _parentDirectory)
+            {
+                parentDirectory = _parentDirectory;
+                return *this;
+            }
         };
 
         Release() = default;
@@ -194,7 +206,6 @@ namespace lms::db
         static std::size_t getCount(Session& session);
         static bool exists(Session& session, ReleaseId id);
         static pointer find(Session& session, const core::UUID& MBID);
-        static std::vector<pointer> find(Session& session, const std::string& name, const std::filesystem::path& releaseDirectory);
         static pointer find(Session& session, ReleaseId id);
         static void find(Session& session, ReleaseId& lastRetrievedRelease, std::size_t count, const std::function<void(const Release::pointer&)>& func, MediaLibraryId library = {});
         static RangeResults<pointer> find(Session& session, const FindParameters& parameters);

src/libs/database/test/Release.cpp

@@ -252,39 +252,6 @@ namespace lms::db::tests
         }
     }
 
-    TEST_F(DatabaseFixture, Release_findByNameAndPath)
-    {
-        ScopedRelease release1{ session, "MyRelease" };
-        ScopedRelease release2{ session, "MyRelease" };
-        ScopedTrack track1{ session };
-        ScopedTrack track2{ session };
-
-        {
-            auto transaction{ session.createWriteTransaction() };
-
-            track1.get().modify()->setRelease(release1.get());
-            track1.get().modify()->setAbsoluteFilePath("/tmp/foo/foo.mp3");
-
-            track2.get().modify()->setRelease(release2.get());
-            track2.get().modify()->setAbsoluteFilePath("/tmp/bar/bar.mp3");
-        }
-
-        {
-            auto transaction{ session.createReadTransaction() };
-            {
-                const auto releases{ Release::find(session, "MyRelease", "/tmp/foo") };
-                ASSERT_EQ(releases.size(), 1);
-                EXPECT_EQ(releases.front()->getId(), release1.getId());
-            }
-
-            {
-                const auto releases{ Release::find(session, "MyRelease", "/tmp/bar") };
-                ASSERT_EQ(releases.size(), 1);
-                EXPECT_EQ(releases.front()->getId(), release2.getId());
-            }
-        }
-    }
-
     TEST_F(DatabaseFixture, MulitpleReleaseSearchByName)
     {
         ScopedRelease release1{ session, "MyRelease" };

src/libs/services/scanner/impl/ScanStepScanFiles.cpp

@@ -244,43 +244,74 @@ namespace lms::scanner
             }
         }
 
-        Release::pointer getOrCreateRelease(Session& session, const metadata::Release& releaseInfo, const std::filesystem::path& expectedReleaseDirectory)
+        Release::pointer getOrCreateRelease(Session& session, std::optional<std::size_t> mediumPosition, const metadata::Release& releaseInfo, const Directory::pointer& currentDirectory)
         {
             Release::pointer release;
 
-            // First try to get by MBID
+            // First try to get by MBID: fastest, safest
             if (releaseInfo.mbid)
             {
                 release = Release::find(session, *releaseInfo.mbid);
                 if (!release)
                     release = session.create<Release>(releaseInfo.name, releaseInfo.mbid);
-
-                updateReleaseIfNeeded(session, release, releaseInfo);
+            }
+            else if (releaseInfo.name.empty())
+            {
+                // No release name (only mbid) -> nothing to de
                 return release;
             }
 
-            // Fall back on release name (collisions may occur), if and only if it is in the current directory
-            if (!releaseInfo.name.empty())
+            // Fall back on release name (collisions may occur)
+            // First try in the current directory
+            if (!release)
             {
-                for (const Release::pointer& sameNamedRelease : Release::find(session, releaseInfo.name, expectedReleaseDirectory))
-                {
-                    // do not fallback on properly tagged releases
-                    if (sameNamedRelease->getMBID())
-                        continue;
-
-                    release = sameNamedRelease;
-                    break;
-                }
-
-                // No release found with the same name and without MBID -> creating
-                if (!release)
-                    release = session.create<Release>(releaseInfo.name);
+                Release::FindParameters params;
+                params.setDirectory(currentDirectory->getId());
+                params.setName(releaseInfo.name);
+                Release::find(session, params, [&](const Release::pointer& candidateRelease) {
+                    // Already found a candidate
+                    if (release)
+                        return;
+
+                    // Do not fallback on properly tagged releases
+                    if (candidateRelease->getMBID().has_value())
+                        return;
+
+                    // TODO: add more criterias?
+                    release = candidateRelease;
+                });
+            }
 
-                updateReleaseIfNeeded(session, release, releaseInfo);
-                return release;
+            // second try in another sibling directory (case for Album/DiscX)
+            const DirectoryId parentDirectoryId{ currentDirectory->getParentDirectoryId() };
+            if (!release && mediumPosition && parentDirectoryId.isValid())
+            {
+                Release::FindParameters params;
+                params.setParentDirectory(parentDirectoryId);
+                params.setName(releaseInfo.name);
+                Release::find(session, params, [&](const Release::pointer& candidateRelease) {
+                    // Already found a candidate
+                    if (release)
+                        return;
+
+                    // Do not fallback on properly tagged releases
+                    if (candidateRelease->getMBID().has_value())
+                        return;
+
+                    // Fallback only if the disc number of the current track is not the same
+                    const std::vector<DiscInfo> discs{ candidateRelease->getDiscs() };
+                    if (discs.empty() || std::any_of(discs.begin(), discs.end(), [&](const DiscInfo& discInfo) { return discInfo.position == *mediumPosition; }))
+                        return;
+
+                    release = candidateRelease;
+                });
             }
 
-            return Release::pointer{};
+            if (!release)
+                release = session.create<Release>(releaseInfo.name);
+
+            updateReleaseIfNeeded(session, release, releaseInfo);
+            return release;
         }
 
         std::vector<Cluster::pointer> getOrCreateClusters(Session& session, const metadata::Track& track)
@@ -656,7 +687,8 @@ namespace lms::scanner
 
         MediaLibrary::pointer mediaLibrary{ MediaLibrary::find(dbSession, libraryInfo.id) }; // may be null if settings are updated in // => next scan will correct this
         track.modify()->setMediaLibrary(mediaLibrary);
-        track.modify()->setDirectory(getOrCreateDirectory(dbSession, file.parent_path(), mediaLibrary));
+        Directory::pointer directory{ getOrCreateDirectory(dbSession, file.parent_path(), mediaLibrary) };
+        track.modify()->setDirectory(directory);
 
         track.modify()->clearArtistLinks();
         // Do not fallback on artists with the same name but having a MBID for artist and releaseArtists, as it may be corrected by properly tagging files
@@ -697,7 +729,7 @@ namespace lms::scanner
 
         track.modify()->setScanVersion(_settings.scanVersion);
         if (trackMetadata->medium && trackMetadata->medium->release)
-            track.modify()->setRelease(getOrCreateRelease(dbSession, *trackMetadata->medium->release, file.parent_path()));
+            track.modify()->setRelease(getOrCreateRelease(dbSession, trackMetadata->medium->position, *trackMetadata->medium->release, directory));
         else
             track.modify()->setRelease({});
         track.modify()->setTotalTrack(trackMetadata->medium ? trackMetadata->medium->trackCount : std::nullopt);