Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SQLite: Replace LIKE with SUBSTR for file path matching #4162

Merged
merged 5 commits into from
Aug 7, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -862,7 +862,6 @@ add_library(mixxx-lib STATIC EXCLUDE_FROM_ALL
src/util/db/dbid.cpp
src/util/db/fwdsqlquery.cpp
src/util/db/fwdsqlqueryselectresult.cpp
src/util/db/sqllikewildcardescaper.cpp
src/util/db/sqlite.cpp
src/util/db/sqlqueryfinisher.cpp
src/util/db/sqlstringformatter.cpp
Expand Down
21 changes: 3 additions & 18 deletions src/library/dao/directorydao.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,6 @@

#include "library/queryutil.h"
#include "util/db/fwdsqlquery.h"
#include "util/db/sqllikewildcardescaper.h"
#include "util/db/sqllikewildcards.h"
#include "util/logger.h"

namespace {
Expand Down Expand Up @@ -167,17 +165,12 @@ QList<RelocatedTrack> DirectoryDAO::relocateDirectory(
// 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);
"INSTR(track_locations.location,:oldDirectoryPrefix)=1"));
query.bindValue(":oldDirectoryPrefix", oldDirectoryPrefix);
if (!query.exec()) {
LOG_FAILED_QUERY(query) << "could not relocate path of tracks";
return {};
Expand All @@ -189,15 +182,7 @@ QList<RelocatedTrack> DirectoryDAO::relocateDirectory(
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;
}
DEBUG_ASSERT(oldLocation.startsWith(oldDirectoryPrefix));
loc_ids.append(DbId(query.value(1).toInt()));
const auto trackId = TrackId(query.value(0));
auto missingTrackRef = TrackRef::fromFilePath(
Expand Down
49 changes: 22 additions & 27 deletions src/library/dao/trackdao.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,6 @@
#include "util/datetime.h"
#include "util/db/fwdsqlquery.h"
#include "util/db/sqlite.h"
#include "util/db/sqllikewildcardescaper.h"
#include "util/db/sqllikewildcards.h"
#include "util/db/sqlstringformatter.h"
#include "util/db/sqltransaction.h"
#include "util/fileinfo.h"
Expand Down Expand Up @@ -70,6 +68,14 @@ QString joinTrackIdList(const QSet<TrackId>& trackIds) {
return trackIdList.join(QChar(','));
}

QString locationPathPrefixFromRootDir(const QDir& rootDir) {
// 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(!mixxx::FileInfo(rootDir).location().endsWith('/'));
return mixxx::FileInfo(rootDir).location() + '/';
}

} // anonymous namespace

TrackDAO::TrackDAO(CueDAO& cueDao,
Expand Down Expand Up @@ -934,21 +940,17 @@ void TrackDAO::afterUnhidingTracks(
}

QList<TrackRef> TrackDAO::getAllTrackRefs(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.
const QString dirPath = rootDir.absolutePath();
QString likeClause = SqlLikeWildcardEscaper::apply(dirPath + "/", kSqlLikeMatchAll) + kSqlLikeMatchAll;

const QString locationPathPrefix = locationPathPrefixFromRootDir(rootDir);
QSqlQuery query(m_database);
query.prepare(QString("SELECT library.id, track_locations.location "
"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,track_locations.location "
"FROM library INNER JOIN track_locations "
"ON library.location=track_locations.id "
"WHERE "
"INSTR(track_locations.location,:locationPathPrefix)=1"));
query.bindValue(":locationPathPrefix", locationPathPrefix);
VERIFY_OR_DEBUG_ASSERT(query.exec()) {
LOG_FAILED_QUERY(query) << "could not get tracks within directory:" << dirPath;
LOG_FAILED_QUERY(query) << "could not get tracks within directory:" << locationPathPrefix;
}

QList<TrackRef> trackRefs;
Expand Down Expand Up @@ -1952,22 +1954,15 @@ bool TrackDAO::detectMovedTracks(
}

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.
const QString rootDirPath = rootDir.absolutePath();
DEBUG_ASSERT(!rootDirPath.endsWith('/'));
const QString likeClause =
SqlLikeWildcardEscaper::apply(rootDirPath + '/', kSqlLikeMatchAll) +
kSqlLikeMatchAll;

const QString locationPathPrefix = locationPathPrefixFromRootDir(rootDir);
QSqlQuery query(m_database);
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);
"WHERE "
"INSTR(track_locations.location,:locationPathPrefix)=1"
"locationPathPrefix"));
query.bindValue(":locationPathPrefix", locationPathPrefix);
VERIFY_OR_DEBUG_ASSERT(query.exec()) {
LOG_FAILED_QUERY(query) << "could not get tracks within directory:" << rootDir;
}
Expand Down
4 changes: 4 additions & 0 deletions src/library/trackset/crate/cratestorage.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -503,6 +503,10 @@ CrateSummarySelectResult CrateStorage::selectCratesWithTrackCount(

CrateTrackSelectResult CrateStorage::selectTracksSortedByCrateNameLike(
const QString& crateNameLike) const {
// TODO: Do SQL LIKE wildcards in crateNameLike need to be escaped?
// Previously we used SqlLikeWildcardEscaper in the past for this
// purpose. This utility class has become obsolete but could be
// restored from the 2.3 branch if ever needed again.
FwdSqlQuery query(m_database,
QStringLiteral("SELECT %1,%2 FROM %3 "
"JOIN %4 ON %5 = %6 "
Expand Down
12 changes: 7 additions & 5 deletions src/test/directorydaotest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -180,17 +180,19 @@ TEST_F(DirectoryDAOTest, relocateDirectory) {
const QTemporaryDir tempDir2;
ASSERT_TRUE(tempDir2.isValid());

// Create temp dirs (with LIKE/GLOB wildcards and quotes in name)
// Create temp dirs (with LIKE/GLOB wildcards, Unicode characters, and quotes in name).
// The different directories only differ by the character 'e' with decorations, namely
// 'é' and 'ë'.
#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 oldDirPath = tempDir1.filePath("Test'_%Dir");
const QString newDirPath = tempDir2.filePath("TestDir'_%New");
const QString otherDirPath = tempDir2.filePath("TestDir'_%Other");
const QString newDirPath = tempDir2.filePath("Tést'_%Dir");
const QString otherDirPath = tempDir2.filePath("Tëst'_%Dir");
#else
const QString oldDirPath = tempDir1.filePath("Test'\"_*%?Dir");
const QString newDirPath = tempDir2.filePath("TestDir'\"_*%?New");
const QString otherDirPath = tempDir2.filePath("TestDir'\"_*%?Other");
const QString newDirPath = tempDir2.filePath("Tést'\"_*%?Dir");
const QString otherDirPath = tempDir2.filePath("Tëst'\"_*%?Dir");
#endif
ASSERT_TRUE(QDir{}.mkpath(oldDirPath));
ASSERT_TRUE(QDir{}.mkpath(newDirPath));
Expand Down
6 changes: 0 additions & 6 deletions src/test/queryutiltest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@

#include "library/queryutil.h"
#include "test/mixxxdbtest.h"
#include "util/db/sqllikewildcardescaper.h"

class QueryUtilTest : public MixxxDbTest {};

Expand All @@ -13,8 +12,3 @@ TEST_F(QueryUtilTest, FieldEscaperEscapesQuotes) {
EXPECT_STREQ(qPrintable(QString("'foobar''s'")),
qPrintable(fieldEscaper.escapeString("foobar's")));
}

TEST_F(QueryUtilTest, SqlLikeWildcardEscaperEscapesForLike) {
EXPECT_STREQ(qPrintable(QString("xx44xx4%yy4_yy")),
qPrintable(SqlLikeWildcardEscaper::apply("xx4xx%yy_yy", '4')));
}
24 changes: 0 additions & 24 deletions src/util/db/sqllikewildcardescaper.cpp

This file was deleted.

17 changes: 0 additions & 17 deletions src/util/db/sqllikewildcardescaper.h

This file was deleted.