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

Fix detection of moved tracks #2197

Merged
merged 13 commits into from
Jul 7, 2019
Merged

Fix detection of moved tracks #2197

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
c8d341b
Exclude missing tracks when detecting moved tracks
uklotzde Jun 30, 2019
db8707d
Fix indexing of query result columns
uklotzde Jun 30, 2019
4d9857e
Fix wrong parameter binding
uklotzde Jun 30, 2019
d8eee8e
Skip results on errors or inconsistencies
uklotzde Jun 30, 2019
6071d61
Reorder database operation to preserve referential integrity
uklotzde Jun 30, 2019
3e079c1
Avoid redundant queries
uklotzde Jul 2, 2019
69bfe6b
Fix typo
uklotzde Jul 2, 2019
a2477f3
Add more consistency checks and assertions
uklotzde Jul 2, 2019
0b2abcb
Raise log level for query errors from debug to warning
uklotzde Jul 2, 2019
d2d4359
Address review comments
uklotzde Jul 3, 2019
4700a5b
Resolve conflicts for multiple candidate tracks
uklotzde Jul 3, 2019
1bc09e8
Fix ranking of moved track candidates
uklotzde Jul 4, 2019
726a216
Fix invalid warning for deleted tracks
uklotzde Jul 6, 2019
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
250 changes: 144 additions & 106 deletions src/library/dao/trackdao.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,12 +33,15 @@
#include "track/tracknumbers.h"
#include "util/assert.h"
#include "util/file.h"
#include "util/logger.h"
#include "util/timer.h"
#include "util/math.h"


namespace {

mixxx::Logger kLogger("TrackDAO");

enum { UndefinedRecordIndex = -2 };

void markTrackLocationsAsDeleted(QSqlDatabase database, const QString& directory) {
Expand Down Expand Up @@ -1241,11 +1244,15 @@ TrackPointer TrackDAO::getTrackFromDB(TrackId trackId) const {
"INNER JOIN track_locations ON library.location = track_locations.id "
"WHERE library.id = %2").arg(columnsStr, trackId.toString()));

if (!query.exec() || !query.next()) {
if (!query.exec()) {
LOG_FAILED_QUERY(query)
<< QString("getTrack(%1)").arg(trackId.toString());
return TrackPointer();
}
if (!query.next()) {
qDebug() << "Track with id =" << trackId << "not found";
return TrackPointer();
}

QSqlRecord queryRecord = query.record();
int recordCount = queryRecord.count();
Expand Down Expand Up @@ -1531,6 +1538,22 @@ void TrackDAO::markUnverifiedTracksAsDeleted() {
}
}

namespace {
// Computed the longest match from the right of both strings
int matchStringSuffix(const QString& str1, const QString& str2) {
int matchLength = 0;
int minLength = math_min(str1.length(), str2.length());
while (matchLength < minLength) {
if (str1[str1.length() - matchLength - 1] != str2[str2.length() - matchLength - 1]) {
// first mismatch
break;
}
++matchLength;
}
return matchLength;
}
}

// Look for moved files. Look for files that have been marked as
// "deleted on disk" and see if another "file" with the same name and
// files size exists in the track_locations table. That means the file has
Expand All @@ -1545,143 +1568,158 @@ bool TrackDAO::detectMovedTracks(QSet<TrackId>* pTracksMovedSetOld,
// When it's called from libraryscanner.cpp, there already is a transaction
// started!

if (!addedTracks.size()) {
if (addedTracks.isEmpty()) {
// We have no moved track.
// We can only guarantee for new tracks that the user has not
// edited metadata, which we have to preserve
// TODO(xxx) resolve old duplicates
return true;
}

QSqlQuery deletedTrackQuery(m_database);
QSqlQuery newTrackQuery(m_database);
QSqlQuery query(m_database);
QString filename;
// rather use duration then filesize as an indicator of changes. The filesize
// can change by adding more ID3v2 tags
int duration = -1;

// Query tracks, where we need a successor for
deletedTrackQuery.prepare("SELECT track_locations.id, filename, duration FROM track_locations "
"INNER JOIN library ON track_locations.id=library.location "
"WHERE fs_deleted=1");

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

// Query possible successors
// NOTE: Successors are identified by filename and duration (in seconds).
// Since duration is stored as double-precision floating-point and since it
// is sometimes truncated to nearest integer, tolerance of 1 second is used.
newTrackQuery.prepare(
QString("SELECT track_locations.id FROM track_locations "
"INNER JOIN library ON track_locations.id=library.location "
"WHERE track_locations.location IN (%1) AND "
"filename=:filename AND "
"ABS(duration - :duration) < 1").arg(
SqlStringFormatter::formatList(m_database, addedTracks)));

QSqlRecord queryRecord = deletedTrackQuery.record();
const int idColumn = queryRecord.indexOf("id");
const int filenameColumn = queryRecord.indexOf("filename");
const int durationColumn = queryRecord.indexOf("duration");
QSqlQuery newTrackQuery(m_database);
newTrackQuery.prepare(QString(
Copy link
Member

Choose a reason for hiding this comment

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

We need to decide if this shall only detect newly added tracks or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, we don't. At least not now. This is how the function was designed and I will not modify it.

Copy link
Member

Choose a reason for hiding this comment

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

OK.

"SELECT library.id as track_id, track_locations.id as location_id, "
"track_locations.location "
"FROM library INNER JOIN track_locations ON library.location=track_locations.id "
"WHERE track_locations.location IN (%1) AND "
"filename=:filename AND "
"ABS(duration - :duration) < 1 AND "
"fs_deleted=0").arg(
SqlStringFormatter::formatList(m_database, addedTracks)));

// Query tracks, where we need a successor for
QSqlQuery oldTrackQuery(m_database);
oldTrackQuery.prepare(
"SELECT library.id as track_id, track_locations.id as location_id, "
"track_locations.location, filename, duration "
"FROM library INNER JOIN track_locations ON library.location=track_locations.id "
"WHERE fs_deleted=1");
VERIFY_OR_DEBUG_ASSERT(oldTrackQuery.exec()) {
LOG_FAILED_QUERY(oldTrackQuery);
return false;
}
QSqlRecord oldTrackQueryRecord = oldTrackQuery.record();
const int oldTrackIdColumn = oldTrackQueryRecord.indexOf("track_id");
const int oldLocationIdColumn = oldTrackQueryRecord.indexOf("location_id");
const int oldLocationColumn = oldTrackQueryRecord.indexOf("location");
const int filenameColumn = oldTrackQueryRecord.indexOf("filename");
const int durationColumn = oldTrackQueryRecord.indexOf("duration");

// For each track that's been "deleted" on disk...
while (deletedTrackQuery.next()) {
while (oldTrackQuery.next()) {
if (*pCancel) {
return false;
}
DbId newTrackLocationId;
DbId oldTrackLocationId(deletedTrackQuery.value(idColumn));
filename = deletedTrackQuery.value(filenameColumn).toString();
duration = deletedTrackQuery.value(durationColumn).toInt();
QString oldTrackLocation = oldTrackQuery.value(oldLocationColumn).toString();
QString filename = oldTrackQuery.value(filenameColumn).toString();
// rather use duration then filesize as an indicator of changes. The filesize
// can change by adding more ID3v2 tags
const int duration = oldTrackQuery.value(durationColumn).toInt();

qDebug() << "TrackDAO::detectMovedTracks looking for a successor of" << filename << duration;
kLogger.info()
<< "Looking for substitute of missing track location"
<< oldTrackLocation;

newTrackQuery.bindValue(":filename", filename);
newTrackQuery.bindValue(":duration", duration);
if (!newTrackQuery.exec()) {
// Should not happen!
VERIFY_OR_DEBUG_ASSERT(newTrackQuery.exec()) {
LOG_FAILED_QUERY(newTrackQuery);
continue;
}
// WTF duplicate tracks?
if (newTrackQuery.size() > 1) {
LOG_FAILED_QUERY(newTrackQuery) << "Result size was greater than 1.";
}

const int query2idColumn = newTrackQuery.record().indexOf("id");
const auto newTrackIdColumn = newTrackQuery.record().indexOf("track_id");
const auto newTrackLocationIdColumn = newTrackQuery.record().indexOf("location_id");
const auto newTrackLocationColumn = newTrackQuery.record().indexOf("location");
int newTrackLocationSuffixMatch = 0;
TrackId newTrackId;
DbId newTrackLocationId;
QString newTrackLocation;
while (newTrackQuery.next()) {
newTrackLocationId = DbId(newTrackQuery.value(query2idColumn));
}

//If we found a moved track...
if (newTrackLocationId.isValid()) {
qDebug() << "Found moved track!" << filename;

// Remove old row from track_locations table
query.prepare("DELETE FROM track_locations WHERE id=:id");
query.bindValue(":id", oldTrackLocationId.toVariant());
if (!query.exec()) {
// Should not happen!
LOG_FAILED_QUERY(query);
}

// The library scanner will have added a new row to the Library
// table which corresponds to the track in the new location. We need
// to remove that so we don't end up with two rows in the library
// table for the same track.
query.prepare("SELECT id FROM library WHERE location=:location");
query.bindValue(":location", newTrackLocationId.toVariant());
if (!query.exec()) {
// Should not happen!
LOG_FAILED_QUERY(query);
const auto nextTrackLocation =
newTrackQuery.value(newTrackLocationColumn).toString();
VERIFY_OR_DEBUG_ASSERT(nextTrackLocation != oldTrackLocation) {
continue;
}

const int query3idColumn = query.record().indexOf("id");
if (query.next()) {
TrackId newTrackId(query.value(query3idColumn));
query.prepare("DELETE FROM library WHERE id=:newid");
query.bindValue(":newid", newTrackLocationId.toVariant());
if (!query.exec()) {
// Should not happen!
LOG_FAILED_QUERY(query);
}
// We collect all the new tracks the where added to BaseTrackCache as well
pTracksMovedSetNew->insert(newTrackId);
kLogger.info()
<< "Found potential moved track location:"
<< nextTrackLocation;
const auto nextSuffixMatch =
matchStringSuffix(nextTrackLocation, oldTrackLocation);
DEBUG_ASSERT(nextSuffixMatch >= filename.length());
if (newTrackLocationSuffixMatch < nextSuffixMatch) {
newTrackLocationSuffixMatch = nextSuffixMatch;
newTrackId = TrackId(newTrackQuery.value(newTrackIdColumn));
newTrackLocationId = DbId(newTrackQuery.value(newTrackLocationIdColumn));
newTrackLocation = nextTrackLocation;
}
// Delete the track
}
if (newTrackLocation.isEmpty()) {
kLogger.info()
uklotzde marked this conversation as resolved.
Show resolved Hide resolved
<< "Found no substitute for missing track location"
<< oldTrackLocation;
continue;
}
DEBUG_ASSERT(newTrackId.isValid());
DEBUG_ASSERT(newTrackLocationId.isValid());
kLogger.info()
<< "Found moved track location:"
<< oldTrackLocation
<< "->"
<< newTrackLocation;

TrackId oldTrackId(oldTrackQuery.value(oldTrackIdColumn));
DEBUG_ASSERT(oldTrackId.isValid());
DbId oldTrackLocationId(oldTrackQuery.value(oldLocationIdColumn));
DEBUG_ASSERT(oldTrackLocationId.isValid());

// The queries ensure that the following assertions are always true (fs_deleted=0/1)!
DEBUG_ASSERT(oldTrackId != newTrackId);
DEBUG_ASSERT(oldTrackLocationId != newTrackLocationId);

// The library scanner will have added a new row to the Library
// table which corresponds to the track in the new location. We need
// to remove that so we don't end up with two rows in the library
// table for the same track.
{
Copy link
Member

Choose a reason for hiding this comment

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

What is if we found a duplicate and the track was already part of playlists?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The newly added track can safely be deleted, because it is not referenced in any playlist. The track that was marked as missing will become visible again after referencing the new, existing location.

Copy link
Member

Choose a reason for hiding this comment

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

But if we delete it here and something goes wrong below, the track is lost until the directory hash changes.

QSqlQuery query(m_database);
query.prepare("DELETE FROM library WHERE id=:newid");
query.bindValue(":newid", newTrackLocationId.toVariant());
if (!query.exec()) {
// Should not happen!
query.bindValue(":newid", newTrackId.toVariant());
VERIFY_OR_DEBUG_ASSERT(query.exec()) {
LOG_FAILED_QUERY(query);
// Last chance to skip this entry, i.e. nothing has been
// deleted or updated yet!
continue;
}

// Update the location foreign key for the existing row in the
// library table to point to the correct row in the track_locations
// table.
query.prepare("SELECT id FROM library WHERE location=:location");
query.bindValue(":location", oldTrackLocationId.toVariant());
if (!query.exec()) {
// Should not happen!
}
// Update the location foreign key for the existing row in the
// library table to point to the correct row in the track_locations
// table.
{
QSqlQuery query(m_database);
query.prepare("UPDATE library SET location=:newloc WHERE id=:oldid");
query.bindValue(":newloc", newTrackLocationId.toVariant());
query.bindValue(":oldid", oldTrackId.toVariant());
VERIFY_OR_DEBUG_ASSERT(query.exec()) {
LOG_FAILED_QUERY(query);
}

if (query.next()) {
TrackId oldTrackId(query.value(query3idColumn));
query.prepare("UPDATE library SET location=:newloc WHERE id=:oldid");
query.bindValue(":newloc", newTrackLocationId.toVariant());
query.bindValue(":oldid", oldTrackId.toVariant());
if (!query.exec()) {
// Should not happen!
LOG_FAILED_QUERY(query);
}

// We collect all the old tracks that has to be updated in BaseTrackCache as well
pTracksMovedSetOld->insert(oldTrackId);
}
// Remove old, orphaned row from track_locations table
{
QSqlQuery query(m_database);
query.prepare("DELETE FROM track_locations WHERE id=:id");
query.bindValue(":id", oldTrackLocationId.toVariant());
VERIFY_OR_DEBUG_ASSERT(query.exec()) {
LOG_FAILED_QUERY(query);
}
}

// We collect all the old tracks that has to be updated in BaseTrackCache
pTracksMovedSetOld->insert(oldTrackId);
// We collect collect all the new tracks the where added and deleted to BaseTrackCache
pTracksMovedSetNew->insert(newTrackId);
}
return true;
}
Expand Down
2 changes: 1 addition & 1 deletion src/library/queryutil.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
#include <QtSql>


#define LOG_FAILED_QUERY(query) qDebug() << __FILE__ << __LINE__ << "FAILED QUERY [" \
#define LOG_FAILED_QUERY(query) qWarning() << __FILE__ << __LINE__ << "FAILED QUERY [" \
<< (query).executedQuery() << "]" << (query).lastError()

class ScopedTransaction {
Expand Down
10 changes: 7 additions & 3 deletions src/test/trackdao_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,19 +14,23 @@ TEST_F(TrackDAOTest, detectMovedTracks) {

QString filename("file.mp3");

QString oldFile(QDir::tempPath() + "/old/" + filename);
QString newFile(QDir::tempPath() + "/new/" + filename);
QString oldFile(QDir::tempPath() + "/old/dir1" + filename);
QString newFile(QDir::tempPath() + "/new/dir1" + filename);
QString otherFile(QDir::tempPath() + "/new" + filename);

TrackPointer pOldTrack = Track::newTemporary(oldFile);
TrackPointer pNewTrack = Track::newTemporary(newFile);
TrackPointer pOtherTrack = Track::newTemporary(otherFile);

// Arbitrary duration
pOldTrack->setDuration(135);
pNewTrack->setDuration(135.7);
pOtherTrack->setDuration(135.7);

trackDAO.addTracksPrepare();
TrackId oldId = trackDAO.addTracksAddTrack(pOldTrack, false);
TrackId newId = trackDAO.addTracksAddTrack(pNewTrack, false);
trackDAO.addTracksAddTrack(pOtherTrack, false);
trackDAO.addTracksFinish(false);

// Mark as missing
Expand All @@ -45,5 +49,5 @@ TEST_F(TrackDAOTest, detectMovedTracks) {
EXPECT_THAT(tracksMovedSetNew, UnorderedElementsAre(newId));

QSet<QString> trackLocations = trackDAO.getTrackLocations();
EXPECT_THAT(trackLocations, UnorderedElementsAre(newFile));
EXPECT_THAT(trackLocations, UnorderedElementsAre(newFile, otherFile));
}