diff --git a/CMakeLists.txt b/CMakeLists.txt index e67b1f4b9d8..599ff9aa0ce 100644 --- a/CMakeLists.txt +++ b/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) diff --git a/src/library/dao/directorydao.cpp b/src/library/dao/directorydao.cpp index 8ba7acae2b2..9525f2aa8ab 100644 --- a/src/library/dao/directorydao.cpp +++ b/src/library/dao/directorydao.cpp @@ -78,36 +78,41 @@ int DirectoryDAO::removeDirectory(const QString& dir) const { } QList 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 DirectoryDAO::relocateDirectory( QList loc_ids; QList 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()); diff --git a/src/library/dao/directorydao.h b/src/library/dao/directorydao.h index 5478cd699d4..907a5a9f061 100644 --- a/src/library/dao/directorydao.h +++ b/src/library/dao/directorydao.h @@ -24,6 +24,6 @@ class DirectoryDAO : public DAO { int removeDirectory(const QString& dir) const; QList relocateDirectory( - const QString& oldFolder, - const QString& newFolder) const; + const QString& oldDirectory, + const QString& newDirectory) const; }; diff --git a/src/library/dao/trackdao.cpp b/src/library/dao/trackdao.cpp index 5dab5700fe2..1e82fbccc6b 100644 --- a/src/library/dao/trackdao.cpp +++ b/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; } diff --git a/src/test/directorydaotest.cpp b/src/test/directorydaotest.cpp index 400929cfbe5..cef96b49a5a 100644 --- a/src/test/directorydaotest.cpp +++ b/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 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)); }