Merge pull request #4146 from uklotzde/directorydao

DirectoryDAO: Fix file path matching when relocating directories

CMakeLists.txt

@@ -1117,7 +1117,7 @@ endif()
 
 
 if(WIN32)
-  target_compile_definitions(mixxx-lib PRIVATE __WINDOWS__)
+  target_compile_definitions(mixxx-lib PUBLIC __WINDOWS__)
 
   # Helps prevent duplicate symbols
   target_compile_definitions(mixxx-lib PUBLIC _ATL_MIN_CRT)

src/library/dao/directorydao.cpp

@@ -78,36 +78,41 @@ int DirectoryDAO::removeDirectory(const QString& dir) const {
 }
 
 QList<RelocatedTrack> DirectoryDAO::relocateDirectory(
-        const QString& oldFolder,
-        const QString& newFolder) const {
+        const QString& oldDirectory,
+        const QString& newDirectory) const {
+    DEBUG_ASSERT(oldDirectory == QDir(oldDirectory).absolutePath());
+    DEBUG_ASSERT(newDirectory == QDir(newDirectory).absolutePath());
     // TODO(rryan): This method could use error reporting. It can fail in
-    // mysterious ways for example if a track in the oldFolder also has a zombie
-    // track location in newFolder then the replace query will fail because the
+    // mysterious ways for example if a track in the oldDirectory also has a zombie
+    // track location in newDirectory then the replace query will fail because the
     // location column becomes non-unique.
     QSqlQuery query(m_database);
     query.prepare("UPDATE " % DIRECTORYDAO_TABLE % " SET " % DIRECTORYDAO_DIR %
-                  "=:newFolder WHERE " % DIRECTORYDAO_DIR % " = :oldFolder");
-    query.bindValue(":newFolder", newFolder);
-    query.bindValue(":oldFolder", oldFolder);
+            "=:newDirectory WHERE " % DIRECTORYDAO_DIR % "=:oldDirectory");
+    query.bindValue(":newDirectory", newDirectory);
+    query.bindValue(":oldDirectory", oldDirectory);
     if (!query.exec()) {
         LOG_FAILED_QUERY(query) << "could not relocate directory"
-                                << oldFolder << "to" << newFolder;
+                                << oldDirectory << "to" << newDirectory;
         return {};
     }
 
-    // on Windows the absolute path starts with the drive name
-    // we also need to check for that
-    QString startsWithOldFolder = SqlLikeWildcardEscaper::apply(
-        QDir(oldFolder).absolutePath() + "/", kSqlLikeMatchAll) + kSqlLikeMatchAll;
-
-    // Also update information in the track_locations table. This is where mixxx
-    // gets the location information for a track. Put marks around %1 so that
-    // this also works on windows
-    query.prepare(QString("SELECT library.id, track_locations.id, track_locations.location "
-                          "FROM library INNER JOIN track_locations ON "
-                          "track_locations.id = library.location WHERE "
-                          "track_locations.location LIKE '%1' ESCAPE '%2'")
-                  .arg(startsWithOldFolder, kSqlLikeMatchAll));
+    // Appending '/' is required to disambiguate files from parent
+    // directories, e.g. "a/b.mp3" and "a/b/c.mp3" where "a/b" would
+    // match both instead of only files in the parent directory "a/b/".
+    DEBUG_ASSERT(!oldDirectory.endsWith('/'));
+    const QString oldDirectoryPrefix = oldDirectory + '/';
+    const QString startsWithOldDirectory = SqlLikeWildcardEscaper::apply(
+                                                   oldDirectoryPrefix, kSqlLikeMatchAll) +
+            kSqlLikeMatchAll;
+
+    query.prepare(QStringLiteral(
+            "SELECT library.id,track_locations.id,track_locations.location "
+            "FROM library INNER JOIN track_locations ON "
+            "track_locations.id=library.location WHERE "
+            "track_locations.location LIKE :startsWithOldDirectory ESCAPE :escape"));
+    query.bindValue(":startsWithOldDirectory", startsWithOldDirectory);
+    query.bindValue(":escape", kSqlLikeMatchAll);
     if (!query.exec()) {
         LOG_FAILED_QUERY(query) << "could not relocate path of tracks";
         return {};
@@ -116,23 +121,33 @@ QList<RelocatedTrack> DirectoryDAO::relocateDirectory(
     QList<DbId> loc_ids;
     QList<RelocatedTrack> relocatedTracks;
     while (query.next()) {
+        const auto oldLocation = query.value(2).toString();
+        const int oldSuffixLen = oldLocation.size() - oldDirectory.size();
+        QString newLocation = newDirectory + oldLocation.right(oldSuffixLen);
+        // LIKE is case-insensitive! We cannot decide if the file system
+        // at the old location was case-sensitive or case-insensitive
+        // and must assume that the stored path is at least case-correct.
+        DEBUG_ASSERT(oldLocation.startsWith(oldDirectoryPrefix, Qt::CaseInsensitive));
+        if (!oldLocation.startsWith(oldDirectoryPrefix, Qt::CaseSensitive)) {
+            qDebug() << "Skipping relocation of" << oldLocation
+                     << "to" << newLocation;
+            continue;
+        }
         loc_ids.append(DbId(query.value(1).toInt()));
-        auto trackId = TrackId(query.value(0));
-        auto oldLocation = query.value(2).toString();
+        const auto trackId = TrackId(query.value(0));
         auto missingTrackRef = TrackRef::fromFileInfo(
                 TrackFile(oldLocation),
                 std::move(trackId));
-        const int oldSuffixLen = oldLocation.size() - oldFolder.size();
-        QString newLocation = newFolder + oldLocation.right(oldSuffixLen);
         auto addedTrackRef = TrackRef::fromFileInfo(
-            TrackFile(newLocation) /*without TrackId*/);
+                TrackFile(newLocation)); // without TrackId, because no new track will be added!
         relocatedTracks.append(RelocatedTrack(
                 std::move(missingTrackRef),
                 std::move(addedTrackRef)));
     }
 
-    QString replacement = "UPDATE track_locations SET location = :newloc "
-            "WHERE id = :id";
+    // Also update information in the track_locations table. This is where mixxx
+    // gets the location information for a track.
+    const QString replacement = "UPDATE track_locations SET location=:newloc WHERE id=:id";
     query.prepare(replacement);
     for (int i = 0; i < loc_ids.size(); ++i) {
         query.bindValue("newloc", relocatedTracks.at(i).updatedTrackRef().getLocation());

src/library/dao/directorydao.h

@@ -24,6 +24,6 @@ class DirectoryDAO : public DAO {
     int removeDirectory(const QString& dir) const;
 
     QList<RelocatedTrack> relocateDirectory(
-            const QString& oldFolder,
-            const QString& newFolder) const;
+            const QString& oldDirectory,
+            const QString& newDirectory) const;
 };

src/library/dao/trackdao.cpp

@@ -1770,14 +1770,19 @@ void TrackDAO::hideAllTracks(const QDir& rootDir) const {
     // Capture entries that start with the directory prefix dir.
     // dir needs to end in a slash otherwise we might match other
     // directories.
-    QString likeClause = SqlLikeWildcardEscaper::apply(rootDir.absolutePath() + "/", kSqlLikeMatchAll) + kSqlLikeMatchAll;
+    const QString rootDirPath = rootDir.absolutePath();
+    DEBUG_ASSERT(!rootDirPath.endsWith('/'));
+    const QString likeClause =
+            SqlLikeWildcardEscaper::apply(rootDirPath + '/', kSqlLikeMatchAll) +
+            kSqlLikeMatchAll;
 
     QSqlQuery query(m_database);
-    query.prepare(QString("SELECT library.id FROM library INNER JOIN track_locations "
-                          "ON library.location = track_locations.id "
-                          "WHERE track_locations.location LIKE %1 ESCAPE '%2'")
-                  .arg(SqlStringFormatter::format(m_database, likeClause), kSqlLikeMatchAll));
-
+    query.prepare(QStringLiteral(
+            "SELECT library.id FROM library INNER JOIN track_locations "
+            "ON library.location = track_locations.id "
+            "WHERE track_locations.location LIKE :likeClause ESCAPE :escape"));
+    query.bindValue(":likeClause", likeClause);
+    query.bindValue(":escape", kSqlLikeMatchAll);
     VERIFY_OR_DEBUG_ASSERT(query.exec()) {
         LOG_FAILED_QUERY(query) << "could not get tracks within directory:" << rootDir;
     }

src/test/directorydaotest.cpp

@@ -137,50 +137,107 @@ TEST_F(DirectoryDAOTest, relocateDirectory) {
     const QTemporaryDir tempDir2;
     ASSERT_TRUE(tempDir2.isValid());
 
-    //create temp dirs
-    QString testdir(tempDir1.filePath("TestDir"));
-    ASSERT_TRUE(QDir(tempDir1.path()).mkpath(testdir));
-    QString test2(tempDir2.filePath("TestDir2"));
-    ASSERT_TRUE(QDir(tempDir2.path()).mkpath(test2));
-    QString testnew(tempDir2.filePath("TestDirNew"));
-    ASSERT_TRUE(QDir(tempDir2.path()).mkpath(testnew));
+    // Create temp dirs (with LIKE/GLOB wildcards and quotes in name)
+#if defined(__WINDOWS__)
+    // Exclude reserved characters from path names: ", *, ?
+    // https://docs.microsoft.com/en-us/windows/win32/fileio/naming-a-file#naming-conventions
+    const QString oldDir = tempDir1.filePath("Test'_%Dir");
+    const QString newDir = tempDir2.filePath("TestDir'_%New");
+    const QString otherDir = tempDir2.filePath("TestDir'_%Other");
+#else
+    const QString oldDir = tempDir1.filePath("Test'\"_*%?Dir");
+    const QString newDir = tempDir2.filePath("TestDir'\"_*%?New");
+    const QString otherDir = tempDir2.filePath("TestDir'\"_*%?Other");
+#endif
+    ASSERT_TRUE(QDir(tempDir1.path()).mkpath(oldDir));
+    ASSERT_TRUE(QDir(tempDir2.path()).mkpath(newDir));
+    ASSERT_TRUE(QDir(tempDir2.path()).mkpath(otherDir));
 
     const DirectoryDAO& dao = internalCollection()->getDirectoryDAO();
 
-    ASSERT_EQ(ALL_FINE, dao.addDirectory(testdir));
-    ASSERT_EQ(ALL_FINE, dao.addDirectory(test2));
+    ASSERT_EQ(ALL_FINE, dao.addDirectory(oldDir));
+    ASSERT_EQ(ALL_FINE, dao.addDirectory(otherDir));
+
+    const QStringList oldDirs = dao.getDirs();
+    ASSERT_EQ(2, oldDirs.size());
+    ASSERT_THAT(oldDirs, UnorderedElementsAre(oldDir, otherDir));
 
-    // ok now lets create some tracks here
+    // Add tracks that should be relocated
+    ASSERT_TRUE(internalCollection()
+                        ->addTrack(
+                                Track::newTemporary(
+                                        TrackFile(oldDir, "a." + getSupportedFileExt())),
+                                false)
+                        .isValid());
+    ASSERT_TRUE(internalCollection()
+                        ->addTrack(
+                                Track::newTemporary(
+                                        TrackFile(oldDir, "b." + getSupportedFileExt())),
+                                false)
+                        .isValid());
+
+    // Add tracks that should be unaffected by the relocation
+    ASSERT_TRUE(internalCollection()
+                        ->addTrack(
+                                Track::newTemporary(
+                                        TrackFile(newDir, "c." + getSupportedFileExt())),
+                                false)
+                        .isValid());
+    ASSERT_TRUE(internalCollection()
+                        ->addTrack(
+                                Track::newTemporary(
+                                        TrackFile(otherDir, "d." + getSupportedFileExt())),
+                                false)
+                        .isValid());
+    ASSERT_TRUE(internalCollection()
+                        ->addTrack(
+                                Track::newTemporary(
+                                        TrackFile(oldDir + "." + getSupportedFileExt())),
+                                false)
+                        .isValid());
+    ASSERT_TRUE(internalCollection()
+                        ->addTrack(
+                                Track::newTemporary(
+                                        TrackFile(newDir + "." + getSupportedFileExt())),
+                                false)
+                        .isValid());
     ASSERT_TRUE(internalCollection()
                         ->addTrack(
                                 Track::newTemporary(
-                                        TrackFile(testdir, "a." + getSupportedFileExt())),
+                                        TrackFile(otherDir + "." + getSupportedFileExt())),
                                 false)
                         .isValid());
     ASSERT_TRUE(internalCollection()
                         ->addTrack(
                                 Track::newTemporary(
-                                        TrackFile(testdir, "b." + getSupportedFileExt())),
+                                        TrackFile(oldDir.toLower(), "a." + getSupportedFileExt())),
                                 false)
                         .isValid());
     ASSERT_TRUE(internalCollection()
                         ->addTrack(
                                 Track::newTemporary(
-                                        TrackFile(test2, "c." + getSupportedFileExt())),
+                                        TrackFile(oldDir.toUpper(), "b." + getSupportedFileExt())),
                                 false)
                         .isValid());
     ASSERT_TRUE(internalCollection()
                         ->addTrack(
                                 Track::newTemporary(
-                                        TrackFile(test2, "d." + getSupportedFileExt())),
+                                        TrackFile(newDir.toLower(), "c." + getSupportedFileExt())),
                                 false)
                         .isValid());
+    ASSERT_TRUE(
+            internalCollection()
+                    ->addTrack(
+                            Track::newTemporary(
+                                    TrackFile(otherDir.toUpper(), "d." + getSupportedFileExt())),
+                            false)
+                    .isValid());
 
     QList<RelocatedTrack> relocatedTracks =
-            dao.relocateDirectory(testdir, testnew);
+            dao.relocateDirectory(oldDir, newDir);
     EXPECT_EQ(2, relocatedTracks.size());
 
-    const QStringList allDirs = dao.getDirs();
-    EXPECT_EQ(2, allDirs.size());
-    EXPECT_THAT(allDirs, UnorderedElementsAre(test2, testnew));
+    const QStringList newDirs = dao.getDirs();
+    EXPECT_EQ(2, newDirs.size());
+    EXPECT_THAT(newDirs, UnorderedElementsAre(newDir, otherDir));
 }