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.

Sign up for GitHub

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

Jump to bottom

library/dao/playlistdao: Increase robustness by using prepared statements #2900

Merged
merged 1 commit into from
Jun 23, 2020
Merged

library/dao/playlistdao: Increase robustness by using prepared statements #2900

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
146 changes: 84 additions & 62 deletions src/library/dao/playlistdao.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -397,24 +397,18 @@ void PlaylistDAO::removeHiddenTracks(const int playlistId) {
ScopedTransaction transaction(m_database);
// This query deletes all tracks marked as deleted and all
// phantom track_ids with no match in the library table
QString queryString =
QStringLiteral(
"SELECT position FROM PlaylistTracks "
"WHERE PlaylistTracks.id NOT IN ("
"SELECT PlaylistTracks.id FROM PlaylistTracks "
"INNER JOIN library ON library.id = PlaylistTracks.track_id "
"WHERE PlaylistTracks.playlist_id = %1 "
"AND library.mixxx_deleted = 0 ) "
"AND PlaylistTracks.playlist_id = %1")
.arg(playlistId);

QSqlQuery query(m_database);
if (!query.prepare(queryString)) {
LOG_FAILED_QUERY(query);
return;
}

query.prepare(QStringLiteral(
Copy link
Contributor

@uklotzde uklotzde Jun 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MUCH (25 ms vs. 1000 ms) faster: Oh, but that doesn't cover orphaned tracks. Ignore it!

SELECT PlaylistTracks.position FROM PlaylistTracks
INNER JOIN library ON library.id=PlaylistTracks.track_id
AND library.mixxx_deleted=0
AND PlaylistTracks.playlist_id=:id

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Works as is. No need to change it.

"SELECT p1.position FROM PlaylistTracks AS p1"
"WHERE p1.id NOT IN ("
"SELECT p2.id FROM PlaylistTracks AS p2"
"INNER JOIN library ON library.id = p2.track_id "
"WHERE p2.playlist_id = p1.playlist_id "
"AND library.mixxx_deleted = 0 ) "
"AND p1.playlist_id=:id"));
query.bindValue(":id", playlistId);
query.setForwardOnly(true);

if (!query.exec()) {
LOG_FAILED_QUERY(query);
return;
Expand Down Expand Up @@ -502,7 +496,7 @@ void PlaylistDAO::removeTracksFromPlaylistInner(int playlistId, int position) {
// Delete the track from the playlist.
query.prepare(QStringLiteral(
"DELETE FROM PlaylistTracks "
"WHERE playlist_id=:id AND position= :position"));
"WHERE playlist_id=:id AND position=:position"));
query.bindValue(":id", playlistId);
query.bindValue(":position", position);

Expand All @@ -511,12 +505,13 @@ void PlaylistDAO::removeTracksFromPlaylistInner(int playlistId, int position) {
return;
}

QString queryString =
QStringLiteral(
"UPDATE PlaylistTracks SET position=position-1 "
"WHERE position>=%1 AND playlist_id=%2")
.arg(position, playlistId);
if (!query.exec(queryString)) {
query.prepare(QStringLiteral(
"UPDATE PlaylistTracks SET position=position-1 "
"WHERE position>=:position AND playlist_id=:id"));
query.bindValue(":id", playlistId);
query.bindValue(":position", position);

if (!query.exec()) {
LOG_FAILED_QUERY(query);
}

Expand All @@ -537,14 +532,14 @@ bool PlaylistDAO::insertTrackIntoPlaylist(TrackId trackId, const int playlistId,
}

// Move all the tracks in the playlist up by one
QString queryString =
QStringLiteral(
"UPDATE PlaylistTracks SET position=position+1 "
"WHERE position>=%1 AND playlist_id=%2")
.arg(position, playlistId);

QSqlQuery query(m_database);
if (!query.exec(queryString)) {
query.prepare(QStringLiteral(
"UPDATE PlaylistTracks SET position=position+1 "
"WHERE position>=:position AND playlist_id=:id"));
query.bindValue(":id", playlistId);
query.bindValue(":position", position);

if (!query.exec()) {
LOG_FAILED_QUERY(query);
return false;
}
Expand Down Expand Up @@ -599,9 +594,10 @@ int PlaylistDAO::insertTracksIntoPlaylist(const QList<TrackId>& trackIds,
// TODO(XXX) We could do this in one query before the for loop.
query.prepare(QStringLiteral(
"UPDATE PlaylistTracks SET position=position+1 "
"WHERE position>=%1 AND "
"playlist_id=%2")
.arg(insertPositon, playlistId));
"WHERE position>=:position AND "
"playlist_id=:id"));
query.bindValue(":id", playlistId);
query.bindValue(":position", insertPositon);

if (!query.exec()) {
LOG_FAILED_QUERY(query);
Expand Down Expand Up @@ -819,36 +815,47 @@ void PlaylistDAO::moveTrack(const int playlistId, const int oldPosition, const i
// 2) Decrement position where pos > source AND pos <= dest
// 3) Set position=dest where pos=-1 -- Move that track from dummy pos to final destination

QString queryString;

// Move moved track to dummy position -1
queryString = QStringLiteral(
query.prepare(QStringLiteral(
"UPDATE PlaylistTracks SET position=-1 "
"WHERE position=%1 AND "
"playlist_id=%2")
.arg(oldPosition, playlistId);
query.exec(queryString);
"WHERE position=:position AND "
"playlist_id=:id"));
query.bindValue(":position", oldPosition);
query.bindValue(":id", playlistId);
if (!query.exec()) {
LOG_FAILED_QUERY(query);
}

if (newPosition < oldPosition) {
queryString = QStringLiteral(
query.prepare(QStringLiteral(
"UPDATE PlaylistTracks SET position=position+1 "
"WHERE position >= %1 AND position < %2 AND "
"playlist_id=%3")
.arg(newPosition, oldPosition, playlistId);
"WHERE position >= :new_position AND position < :old_position AND "
"playlist_id=:id"));
query.bindValue(":new_position", newPosition);
query.bindValue(":old_position", oldPosition);
query.bindValue(":id", playlistId);
} else {
queryString = QStringLiteral(
query.prepare(QStringLiteral(
"UPDATE PlaylistTracks SET position=position-1 "
"WHERE position>%1 AND position<=%2 AND "
"playlist_id=%3")
.arg(oldPosition, newPosition, playlistId);
"WHERE position > :old_position AND position <= :new_position AND "
"playlist_id=:id"));
query.bindValue(":new_position", newPosition);
query.bindValue(":old_position", oldPosition);
query.bindValue(":id", playlistId);
}
if (!query.exec()) {
LOG_FAILED_QUERY(query);
}
query.exec(queryString);

query.exec(QStringLiteral(
"UPDATE PlaylistTracks SET position = %1 "
query.prepare(QStringLiteral(
"UPDATE PlaylistTracks SET position=:new_position "
"WHERE position=-1 AND "
"playlist_id=%2")
.arg(newPosition, playlistId));
"playlist_id=:id"));
query.bindValue(":new_position", newPosition);
query.bindValue(":id", playlistId);
if (!query.exec()) {
LOG_FAILED_QUERY(query);
}

transaction.commit();

Expand Down Expand Up @@ -883,7 +890,6 @@ void PlaylistDAO::shuffleTracks(const int playlistId,
const QList<int>& positions,
const QHash<int, TrackId>& allIds) {
ScopedTransaction transaction(m_database);
QSqlQuery query(m_database);

#if QT_VERSION < QT_VERSION_CHECK(5, 10, 0)
// Seed the randomness generator
Expand Down Expand Up @@ -1044,15 +1050,31 @@ void PlaylistDAO::shuffleTracks(const int playlistId,
newPositions.indexOf(trackBPosition));
#endif

QString swapQuery = QStringLiteral(
"UPDATE PlaylistTracks SET position=%1 "
"WHERE position=%2 AND playlist_id=%3");
query.exec(swapQuery.arg(-1, trackAPosition, playlistId));
query.exec(swapQuery.arg(trackAPosition, trackBPosition, playlistId));
query.exec(swapQuery.arg(trackBPosition, -1, playlistId));
QSqlQuery query(m_database);
query.prepare(QStringLiteral(
"UPDATE PlaylistTracks SET position=:new_position "
"WHERE position=:old_position AND playlist_id=:id"));

if (query.lastError().isValid())
qDebug() << query.lastError();
query.bindValue(":new_position", -1);
query.bindValue(":old_position", trackAPosition);
query.bindValue(":id", playlistId);
if (!query.exec()) {
LOG_FAILED_QUERY(query);
}

query.bindValue(":new_position", trackAPosition);
query.bindValue(":old_position", trackBPosition);
query.bindValue(":id", playlistId);
if (!query.exec()) {
LOG_FAILED_QUERY(query);
}

query.bindValue(":new_position", trackBPosition);
query.bindValue(":old_position", -1);
query.bindValue(":id", playlistId);
if (!query.exec()) {
LOG_FAILED_QUERY(query);
}
}

transaction.commit();
Expand Down