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 9 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
216 changes: 119 additions & 97 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 @@ -1545,143 +1548,162 @@ 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");
int newTrackLocationCount = 0;
TrackId newTrackId;
DbId newTrackLocationId;
QString newTrackLocation;
while (newTrackQuery.next()) {
newTrackLocationId = DbId(newTrackQuery.value(query2idColumn));
newTrackLocation = newTrackQuery.value(
newTrackQuery.record().indexOf("location")).toString();
VERIFY_OR_DEBUG_ASSERT(newTrackLocation != oldTrackLocation) {
continue;
}
kLogger.info()
<< "Found potential moved track location:"
<< newTrackLocation;
++newTrackLocationCount;
newTrackId = TrackId(newTrackQuery.value(
newTrackQuery.record().indexOf("track_id")));
Copy link
Member

Choose a reason for hiding this comment

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

Can we move the indexOf() part to the top?
I think it is const among all queries.

newTrackLocationId = DbId(newTrackQuery.value(
newTrackQuery.record().indexOf("location_id")));
DEBUG_ASSERT(newTrackLocationId.isValid());
}
switch (newTrackLocationCount) {
case 0:
kLogger.info()
uklotzde marked this conversation as resolved.
Show resolved Hide resolved
<< "Found no substitute for missing track location"
<< oldTrackLocation;
continue;
case 1:
DEBUG_ASSERT(newTrackId.isValid());
DEBUG_ASSERT(newTrackLocationId.isValid());
kLogger.info()
<< "Found moved track location:"
Copy link
Member

Choose a reason for hiding this comment

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

Does this potentially join two tracks?
If we have a duplicate track and remove one?
Do we need some extra handling in this case?

<< oldTrackLocation
<< "->"
<< newTrackLocation;
break;
default:
kLogger.warning()
Copy link
Member

Choose a reason for hiding this comment

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

This is the case we have tree duplicates and one disappears, or if we have duplicates and one is moved.
It would be nice to detect the move. In this case use the just added track.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't understand this comment. All the new have just been added. If multiple of them are candidates for one missing track, we cannot make a decision.

Copy link
Member

Choose a reason for hiding this comment

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

Is this a possible case:

  • User has the same track in two folders.
  • He moved to a new library location.
  • He expects that the tracks are still OK

What will happen? I have not tested it yet.

I assume currently both tracks remain missing after the move and pointing to the old location. I think it is better to assign both duplicates to the first of the two moved tracks than break a playlist.

<< "Found too many ("
<< newTrackLocationCount
<< ") potential substitutes for missing track location"
<< oldTrackLocation;
continue;
}

//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);
}
TrackId oldTrackId(oldTrackQuery.value(oldTrackIdColumn));
DEBUG_ASSERT(oldTrackId.isValid());
DbId oldTrackLocationId(oldTrackQuery.value(oldLocationIdColumn));
DEBUG_ASSERT(oldTrackLocationId.isValid());

// 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);
}
// The queries ensure that this assertion is always true (fs_deleted=0/1)!
DEBUG_ASSERT(oldTrackId != newTrackId);

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);
}
// Delete the track
// 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);
}

}
if (oldTrackLocationId != newTrackLocationId) {
uklotzde marked this conversation as resolved.
Show resolved Hide resolved
// 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!
LOG_FAILED_QUERY(query);
}

if (query.next()) {
TrackId oldTrackId(query.value(query3idColumn));
{
QSqlQuery query(m_database);
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!
VERIFY_OR_DEBUG_ASSERT(query.exec()) {
LOG_FAILED_QUERY(query);
// Last chance to skip this entry
Copy link
Member

Choose a reason for hiding this comment

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

Really? We have already deleted the new track above.

continue;
}
}
// 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 as well
pTracksMovedSetOld->insert(oldTrackId);
}
}

// 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