From c8d341b9e0a650db9701aab499e3896c590741ae Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Sun, 30 Jun 2019 17:33:16 +0200 Subject: [PATCH 01/13] Exclude missing tracks when detecting moved tracks --- src/library/dao/trackdao.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/library/dao/trackdao.cpp b/src/library/dao/trackdao.cpp index a6a51440985..89cd972493a 100644 --- a/src/library/dao/trackdao.cpp +++ b/src/library/dao/trackdao.cpp @@ -1579,7 +1579,8 @@ bool TrackDAO::detectMovedTracks(QSet* pTracksMovedSetOld, "INNER JOIN library ON track_locations.id=library.location " "WHERE track_locations.location IN (%1) AND " "filename=:filename AND " - "ABS(duration - :duration) < 1").arg( + "ABS(duration - :duration) < 1 AND " + "fs_deleted=0").arg( SqlStringFormatter::formatList(m_database, addedTracks))); QSqlRecord queryRecord = deletedTrackQuery.record(); From db8707d6b1e181cfd3980e685caa5af9e079a486 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Sun, 30 Jun 2019 17:34:57 +0200 Subject: [PATCH 02/13] Fix indexing of query result columns --- src/library/dao/trackdao.cpp | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/library/dao/trackdao.cpp b/src/library/dao/trackdao.cpp index 89cd972493a..c5524ed5348 100644 --- a/src/library/dao/trackdao.cpp +++ b/src/library/dao/trackdao.cpp @@ -1593,7 +1593,6 @@ bool TrackDAO::detectMovedTracks(QSet* pTracksMovedSetOld, if (*pCancel) { return false; } - DbId newTrackLocationId; DbId oldTrackLocationId(deletedTrackQuery.value(idColumn)); filename = deletedTrackQuery.value(filenameColumn).toString(); duration = deletedTrackQuery.value(durationColumn).toInt(); @@ -1611,9 +1610,10 @@ bool TrackDAO::detectMovedTracks(QSet* pTracksMovedSetOld, LOG_FAILED_QUERY(newTrackQuery) << "Result size was greater than 1."; } - const int query2idColumn = newTrackQuery.record().indexOf("id"); + DbId newTrackLocationId; + const int newTrackLocationIdColumn = newTrackQuery.record().indexOf("id"); while (newTrackQuery.next()) { - newTrackLocationId = DbId(newTrackQuery.value(query2idColumn)); + newTrackLocationId = DbId(newTrackQuery.value(newTrackLocationIdColumn)); } //If we found a moved track... @@ -1639,9 +1639,9 @@ bool TrackDAO::detectMovedTracks(QSet* pTracksMovedSetOld, LOG_FAILED_QUERY(query); } - const int query3idColumn = query.record().indexOf("id"); + const int newTrackIdColumn = query.record().indexOf("id"); if (query.next()) { - TrackId newTrackId(query.value(query3idColumn)); + TrackId newTrackId(query.value(newTrackIdColumn)); query.prepare("DELETE FROM library WHERE id=:newid"); query.bindValue(":newid", newTrackLocationId.toVariant()); if (!query.exec()) { @@ -1668,9 +1668,10 @@ bool TrackDAO::detectMovedTracks(QSet* pTracksMovedSetOld, // Should not happen! LOG_FAILED_QUERY(query); } + const int oldTrackIdColumn = query.record().indexOf("id"); if (query.next()) { - TrackId oldTrackId(query.value(query3idColumn)); + TrackId oldTrackId(query.value(oldTrackIdColumn)); query.prepare("UPDATE library SET location=:newloc WHERE id=:oldid"); query.bindValue(":newloc", newTrackLocationId.toVariant()); query.bindValue(":oldid", oldTrackId.toVariant()); From 4d9857ea91dbf48892180dd8949b5cc524874167 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Sun, 30 Jun 2019 17:35:14 +0200 Subject: [PATCH 03/13] Fix wrong parameter binding --- src/library/dao/trackdao.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/library/dao/trackdao.cpp b/src/library/dao/trackdao.cpp index c5524ed5348..3dcea32f965 100644 --- a/src/library/dao/trackdao.cpp +++ b/src/library/dao/trackdao.cpp @@ -1643,7 +1643,7 @@ bool TrackDAO::detectMovedTracks(QSet* pTracksMovedSetOld, if (query.next()) { TrackId newTrackId(query.value(newTrackIdColumn)); query.prepare("DELETE FROM library WHERE id=:newid"); - query.bindValue(":newid", newTrackLocationId.toVariant()); + query.bindValue(":newid", newTrackId.toVariant()); if (!query.exec()) { // Should not happen! LOG_FAILED_QUERY(query); From d8eee8e15719cbb87d3bc1744c3c0e0337027b64 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Sun, 30 Jun 2019 17:35:42 +0200 Subject: [PATCH 04/13] Skip results on errors or inconsistencies --- src/library/dao/trackdao.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/library/dao/trackdao.cpp b/src/library/dao/trackdao.cpp index 3dcea32f965..3e49f2b241c 100644 --- a/src/library/dao/trackdao.cpp +++ b/src/library/dao/trackdao.cpp @@ -1604,10 +1604,12 @@ bool TrackDAO::detectMovedTracks(QSet* pTracksMovedSetOld, if (!newTrackQuery.exec()) { // Should not happen! LOG_FAILED_QUERY(newTrackQuery); + continue; } // WTF duplicate tracks? if (newTrackQuery.size() > 1) { LOG_FAILED_QUERY(newTrackQuery) << "Result size was greater than 1."; + continue; } DbId newTrackLocationId; From 6071d61185ae80af77ae1ab5a57d067943a273e4 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Sun, 30 Jun 2019 17:36:10 +0200 Subject: [PATCH 05/13] Reorder database operation to preserve referential integrity --- src/library/dao/trackdao.cpp | 32 +++++++++++++++----------------- 1 file changed, 15 insertions(+), 17 deletions(-) diff --git a/src/library/dao/trackdao.cpp b/src/library/dao/trackdao.cpp index 3e49f2b241c..08c5c1fb65c 100644 --- a/src/library/dao/trackdao.cpp +++ b/src/library/dao/trackdao.cpp @@ -1622,14 +1622,6 @@ bool TrackDAO::detectMovedTracks(QSet* pTracksMovedSetOld, 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 @@ -1640,7 +1632,6 @@ bool TrackDAO::detectMovedTracks(QSet* pTracksMovedSetOld, // Should not happen! LOG_FAILED_QUERY(query); } - const int newTrackIdColumn = query.record().indexOf("id"); if (query.next()) { TrackId newTrackId(query.value(newTrackIdColumn)); @@ -1650,16 +1641,16 @@ bool TrackDAO::detectMovedTracks(QSet* pTracksMovedSetOld, // Should not happen! LOG_FAILED_QUERY(query); } + // Delete the track + query.prepare("DELETE FROM library WHERE id=:newid"); + query.bindValue(":newid", newTrackId.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 - query.prepare("DELETE FROM library WHERE id=:newid"); - query.bindValue(":newid", newTrackLocationId.toVariant()); - if (!query.exec()) { - // Should not happen! - LOG_FAILED_QUERY(query); - } // Update the location foreign key for the existing row in the // library table to point to the correct row in the track_locations @@ -1671,7 +1662,6 @@ bool TrackDAO::detectMovedTracks(QSet* pTracksMovedSetOld, LOG_FAILED_QUERY(query); } const int oldTrackIdColumn = query.record().indexOf("id"); - if (query.next()) { TrackId oldTrackId(query.value(oldTrackIdColumn)); query.prepare("UPDATE library SET location=:newloc WHERE id=:oldid"); @@ -1682,6 +1672,14 @@ bool TrackDAO::detectMovedTracks(QSet* pTracksMovedSetOld, LOG_FAILED_QUERY(query); } + // 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); + } + // We collect all the old tracks that has to be updated in BaseTrackCache as well pTracksMovedSetOld->insert(oldTrackId); } From 3e079c1c1159f47ec5f133cca5004ddaf498c973 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Tue, 2 Jul 2019 09:07:25 +0200 Subject: [PATCH 06/13] Avoid redundant queries --- src/library/dao/trackdao.cpp | 222 ++++++++++++++++++----------------- 1 file changed, 117 insertions(+), 105 deletions(-) diff --git a/src/library/dao/trackdao.cpp b/src/library/dao/trackdao.cpp index 08c5c1fb65c..0ee34523276 100644 --- a/src/library/dao/trackdao.cpp +++ b/src/library/dao/trackdao.cpp @@ -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) { @@ -1545,7 +1548,7 @@ bool TrackDAO::detectMovedTracks(QSet* 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 @@ -1553,137 +1556,146 @@ bool TrackDAO::detectMovedTracks(QSet* pTracksMovedSetOld, 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 AND " - "fs_deleted=0").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( + "SELECT libray.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 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."; - continue; - } - + int newTrackLocationCount = 0; + TrackId newTrackId; DbId newTrackLocationId; - const int newTrackLocationIdColumn = newTrackQuery.record().indexOf("id"); + QString newTrackLocation; while (newTrackQuery.next()) { - newTrackLocationId = DbId(newTrackQuery.value(newTrackLocationIdColumn)); + 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"))); + newTrackLocationId = DbId(newTrackQuery.value( + newTrackQuery.record().indexOf("location_id"))); + DEBUG_ASSERT(newTrackLocationId.isValid()); + } + switch (newTrackLocationCount) { + case 0: + kLogger.info() + << "Found no substitute for missing track location" + << oldTrackLocation; + continue; + case 1: + kLogger.info() + << "Found moved track location:" + << oldTrackLocation + << "->" + << newTrackLocation; + break; + default: + kLogger.warning() + << "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; - - // 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! + TrackId oldTrackId(oldTrackQuery.value(oldTrackIdColumn)); + DbId oldTrackLocationId(oldTrackQuery.value(oldLocationIdColumn)); + + // 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); + // Last chance to skip this entry + continue; } - const int newTrackIdColumn = query.record().indexOf("id"); - if (query.next()) { - TrackId newTrackId(query.value(newTrackIdColumn)); - query.prepare("DELETE FROM library WHERE id=:newid"); - query.bindValue(":newid", newTrackId.toVariant()); - if (!query.exec()) { - // Should not happen! - LOG_FAILED_QUERY(query); - } - // Delete the track - query.prepare("DELETE FROM library WHERE id=:newid"); - query.bindValue(":newid", newTrackId.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); - } + } - // 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! + // 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. + { + QSqlQuery query(m_database); + query.prepare("DELETE FROM library WHERE id=:newid"); + query.bindValue(":newid", newTrackId.toVariant()); + VERIFY_OR_DEBUG_ASSERT(query.exec()) { LOG_FAILED_QUERY(query); } - const int oldTrackIdColumn = query.record().indexOf("id"); - if (query.next()) { - TrackId oldTrackId(query.value(oldTrackIdColumn)); - 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); - } - - // 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); - } - - // 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; } From 69bfe6b00b82741d645b374f8fc7151a1609af26 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Tue, 2 Jul 2019 23:13:42 +0200 Subject: [PATCH 07/13] Fix typo --- src/library/dao/trackdao.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/library/dao/trackdao.cpp b/src/library/dao/trackdao.cpp index 0ee34523276..d70eeb55822 100644 --- a/src/library/dao/trackdao.cpp +++ b/src/library/dao/trackdao.cpp @@ -1562,7 +1562,7 @@ bool TrackDAO::detectMovedTracks(QSet* pTracksMovedSetOld, // is sometimes truncated to nearest integer, tolerance of 1 second is used. QSqlQuery newTrackQuery(m_database); newTrackQuery.prepare(QString( - "SELECT libray.id as track_id, track_locations.id as location_id, " + "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 " From a2477f38166af0bdbdd313fc9fd830d3e5a223b5 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Tue, 2 Jul 2019 23:14:08 +0200 Subject: [PATCH 08/13] Add more consistency checks and assertions --- src/library/dao/trackdao.cpp | 50 +++++++++++++++++++++--------------- 1 file changed, 29 insertions(+), 21 deletions(-) diff --git a/src/library/dao/trackdao.cpp b/src/library/dao/trackdao.cpp index d70eeb55822..72c58338c27 100644 --- a/src/library/dao/trackdao.cpp +++ b/src/library/dao/trackdao.cpp @@ -1637,6 +1637,8 @@ bool TrackDAO::detectMovedTracks(QSet* pTracksMovedSetOld, << oldTrackLocation; continue; case 1: + DEBUG_ASSERT(newTrackId.isValid()); + DEBUG_ASSERT(newTrackLocationId.isValid()); kLogger.info() << "Found moved track location:" << oldTrackLocation @@ -1653,22 +1655,12 @@ bool TrackDAO::detectMovedTracks(QSet* pTracksMovedSetOld, } TrackId oldTrackId(oldTrackQuery.value(oldTrackIdColumn)); + DEBUG_ASSERT(oldTrackId.isValid()); DbId oldTrackLocationId(oldTrackQuery.value(oldLocationIdColumn)); + DEBUG_ASSERT(oldTrackLocationId.isValid()); - // 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); - // Last chance to skip this entry - continue; - } - } + // The queries ensure that this assertion is always true (fs_deleted=0/1)! + DEBUG_ASSERT(oldTrackId != newTrackId); // The library scanner will have added a new row to the Library // table which corresponds to the track in the new location. We need @@ -1682,13 +1674,29 @@ bool TrackDAO::detectMovedTracks(QSet* pTracksMovedSetOld, LOG_FAILED_QUERY(query); } } - // 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); + if (oldTrackLocationId != newTrackLocationId) { + // 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); + // Last chance to skip this entry + 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); + } } } From 0b2abcbbac24987477af7e873fd796b083116b16 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Tue, 2 Jul 2019 23:14:45 +0200 Subject: [PATCH 09/13] Raise log level for query errors from debug to warning --- src/library/queryutil.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/library/queryutil.h b/src/library/queryutil.h index e7ab94cdfea..fe28dba7779 100644 --- a/src/library/queryutil.h +++ b/src/library/queryutil.h @@ -5,7 +5,7 @@ #include -#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 { From d2d4359ceb34946c2629922066fa880b7bd1e987 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Wed, 3 Jul 2019 23:49:01 +0200 Subject: [PATCH 10/13] Address review comments --- src/library/dao/trackdao.cpp | 58 ++++++++++++++++++------------------ 1 file changed, 29 insertions(+), 29 deletions(-) diff --git a/src/library/dao/trackdao.cpp b/src/library/dao/trackdao.cpp index 72c58338c27..302e4da665e 100644 --- a/src/library/dao/trackdao.cpp +++ b/src/library/dao/trackdao.cpp @@ -1610,13 +1610,15 @@ bool TrackDAO::detectMovedTracks(QSet* pTracksMovedSetOld, LOG_FAILED_QUERY(newTrackQuery); continue; } + const auto newTrackIdColumn = newTrackQuery.record().indexOf("track_id"); + const auto newTrackLocationIdColumn = newTrackQuery.record().indexOf("location_id"); + const auto newTrackLocationColumn = newTrackQuery.record().indexOf("location"); int newTrackLocationCount = 0; TrackId newTrackId; DbId newTrackLocationId; QString newTrackLocation; while (newTrackQuery.next()) { - newTrackLocation = newTrackQuery.value( - newTrackQuery.record().indexOf("location")).toString(); + newTrackLocation = newTrackQuery.value(newTrackLocationColumn).toString(); VERIFY_OR_DEBUG_ASSERT(newTrackLocation != oldTrackLocation) { continue; } @@ -1624,10 +1626,8 @@ bool TrackDAO::detectMovedTracks(QSet* pTracksMovedSetOld, << "Found potential moved track location:" << newTrackLocation; ++newTrackLocationCount; - newTrackId = TrackId(newTrackQuery.value( - newTrackQuery.record().indexOf("track_id"))); - newTrackLocationId = DbId(newTrackQuery.value( - newTrackQuery.record().indexOf("location_id"))); + newTrackId = TrackId(newTrackQuery.value(newTrackIdColumn)); + newTrackLocationId = DbId(newTrackQuery.value(newTrackLocationIdColumn)); DEBUG_ASSERT(newTrackLocationId.isValid()); } switch (newTrackLocationCount) { @@ -1659,8 +1659,9 @@ bool TrackDAO::detectMovedTracks(QSet* pTracksMovedSetOld, DbId oldTrackLocationId(oldTrackQuery.value(oldLocationIdColumn)); DEBUG_ASSERT(oldTrackLocationId.isValid()); - // The queries ensure that this assertion is always true (fs_deleted=0/1)! + // 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 @@ -1672,31 +1673,30 @@ bool TrackDAO::detectMovedTracks(QSet* pTracksMovedSetOld, 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; } } - if (oldTrackLocationId != newTrackLocationId) { - // 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); - // Last chance to skip this entry - 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. + { + 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); } - // 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); - } + } + // 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); } } From 4700a5bf2bbf09ec0e483daf500452208f83e47e Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Thu, 4 Jul 2019 00:22:30 +0200 Subject: [PATCH 11/13] Resolve conflicts for multiple candidate tracks --- src/library/dao/trackdao.cpp | 63 +++++++++++++++++++++--------------- src/test/trackdao_test.cpp | 10 ++++-- 2 files changed, 44 insertions(+), 29 deletions(-) diff --git a/src/library/dao/trackdao.cpp b/src/library/dao/trackdao.cpp index 302e4da665e..b2fc420970c 100644 --- a/src/library/dao/trackdao.cpp +++ b/src/library/dao/trackdao.cpp @@ -1534,6 +1534,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 @@ -1613,46 +1629,41 @@ bool TrackDAO::detectMovedTracks(QSet* pTracksMovedSetOld, const auto newTrackIdColumn = newTrackQuery.record().indexOf("track_id"); const auto newTrackLocationIdColumn = newTrackQuery.record().indexOf("location_id"); const auto newTrackLocationColumn = newTrackQuery.record().indexOf("location"); - int newTrackLocationCount = 0; + int newTrackLocationSuffixMatch = 0; TrackId newTrackId; DbId newTrackLocationId; QString newTrackLocation; while (newTrackQuery.next()) { - newTrackLocation = newTrackQuery.value(newTrackLocationColumn).toString(); - VERIFY_OR_DEBUG_ASSERT(newTrackLocation != oldTrackLocation) { + const auto nextTrackLocation = + newTrackQuery.value(newTrackLocationColumn).toString(); + VERIFY_OR_DEBUG_ASSERT(nextTrackLocation != oldTrackLocation) { continue; } kLogger.info() << "Found potential moved track location:" - << newTrackLocation; - ++newTrackLocationCount; - newTrackId = TrackId(newTrackQuery.value(newTrackIdColumn)); - newTrackLocationId = DbId(newTrackQuery.value(newTrackLocationIdColumn)); - DEBUG_ASSERT(newTrackLocationId.isValid()); + << nextTrackLocation; + const auto nextSuffixMatch = + matchStringSuffix(nextTrackLocation, oldTrackLocation); + DEBUG_ASSERT(nextSuffixMatch >= filename.length()); + if (nextSuffixMatch > newTrackLocationSuffixMatch) { + newTrackId = TrackId(newTrackQuery.value(newTrackIdColumn)); + newTrackLocationId = DbId(newTrackQuery.value(newTrackLocationIdColumn)); + newTrackLocation = nextTrackLocation; + } } - switch (newTrackLocationCount) { - case 0: + if (newTrackLocation.isEmpty()) { kLogger.info() << "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:" - << oldTrackLocation - << "->" - << newTrackLocation; - break; - default: - kLogger.warning() - << "Found too many (" - << newTrackLocationCount - << ") potential substitutes 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()); diff --git a/src/test/trackdao_test.cpp b/src/test/trackdao_test.cpp index 2a8a0344a30..2e43ee87a13 100644 --- a/src/test/trackdao_test.cpp +++ b/src/test/trackdao_test.cpp @@ -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 @@ -45,5 +49,5 @@ TEST_F(TrackDAOTest, detectMovedTracks) { EXPECT_THAT(tracksMovedSetNew, UnorderedElementsAre(newId)); QSet trackLocations = trackDAO.getTrackLocations(); - EXPECT_THAT(trackLocations, UnorderedElementsAre(newFile)); + EXPECT_THAT(trackLocations, UnorderedElementsAre(newFile, otherFile)); } From 1bc09e8c4deb6f90285d3be3f9ff2fed3320689e Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Thu, 4 Jul 2019 10:09:07 +0200 Subject: [PATCH 12/13] Fix ranking of moved track candidates --- src/library/dao/trackdao.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/library/dao/trackdao.cpp b/src/library/dao/trackdao.cpp index b2fc420970c..04cd70d91fd 100644 --- a/src/library/dao/trackdao.cpp +++ b/src/library/dao/trackdao.cpp @@ -1645,7 +1645,8 @@ bool TrackDAO::detectMovedTracks(QSet* pTracksMovedSetOld, const auto nextSuffixMatch = matchStringSuffix(nextTrackLocation, oldTrackLocation); DEBUG_ASSERT(nextSuffixMatch >= filename.length()); - if (nextSuffixMatch > newTrackLocationSuffixMatch) { + if (newTrackLocationSuffixMatch < nextSuffixMatch) { + newTrackLocationSuffixMatch = nextSuffixMatch; newTrackId = TrackId(newTrackQuery.value(newTrackIdColumn)); newTrackLocationId = DbId(newTrackQuery.value(newTrackLocationIdColumn)); newTrackLocation = nextTrackLocation; From 726a21663b2f76a5034f3ce99f26fae31b113e7e Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Sun, 7 Jul 2019 00:26:39 +0200 Subject: [PATCH 13/13] Fix invalid warning for deleted tracks --- src/library/dao/trackdao.cpp | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/library/dao/trackdao.cpp b/src/library/dao/trackdao.cpp index 04cd70d91fd..282b05a86f7 100644 --- a/src/library/dao/trackdao.cpp +++ b/src/library/dao/trackdao.cpp @@ -1244,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();