From 00c151bc7ffa015d69e73a116123b053abef187a Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Thu, 17 Jun 2021 14:21:59 +0200 Subject: [PATCH 1/7] Track: Warn when backdating source synchronization time stamps --- src/track/trackrecord.cpp | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/track/trackrecord.cpp b/src/track/trackrecord.cpp index 6d09d3724a6..9fc7964d4ce 100644 --- a/src/track/trackrecord.cpp +++ b/src/track/trackrecord.cpp @@ -118,6 +118,14 @@ bool TrackRecord::updateSourceSynchronizedAt( if (getSourceSynchronizedAt() == sourceSynchronizedAt) { return false; // unchanged } + if (getSourceSynchronizedAt().isValid() && + getSourceSynchronizedAt() >= sourceSynchronizedAt) { + kLogger.warning() + << "Backdating source synchronization time from" + << getSourceSynchronizedAt() + << "to" + << sourceSynchronizedAt; + } setSourceSynchronizedAt(sourceSynchronizedAt); m_headerParsed = sourceSynchronizedAt.isValid(); DEBUG_ASSERT(isSourceSynchronized()); From 32b5f7ef4f4a3a15db590591b1c086b8800f078d Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Thu, 17 Jun 2021 14:23:01 +0200 Subject: [PATCH 2/7] DB: Fix 0/NULL issues when reading source_synchronized_ms --- src/library/dao/trackdao.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/library/dao/trackdao.cpp b/src/library/dao/trackdao.cpp index 43e89462ba9..da07fb7c943 100644 --- a/src/library/dao/trackdao.cpp +++ b/src/library/dao/trackdao.cpp @@ -1209,7 +1209,10 @@ bool setTrackHeaderParsed(const QSqlRecord& record, const int column, TrackPoint bool setTrackSourceSynchronizedAt(const QSqlRecord& record, const int column, TrackPointer pTrack) { QDateTime sourceSynchronizedAt; const QVariant value = record.value(column); - if (value.isValid() && value.canConvert()) { + // Observation (Qt 5.15): QVariant::isValid() may return true even + // if the column value is NULL and then converts to 0 (zero). This + // is NOT desired and therefore we MUST USE QVariant::isNull() instead! + if (!value.isNull() && value.canConvert()) { const quint64 msecsSinceEpoch = qvariant_cast(value); sourceSynchronizedAt.setTimeSpec(Qt::UTC); sourceSynchronizedAt.setMSecsSinceEpoch(msecsSinceEpoch); From 27b75e406e8298ac48a3fdf9a63a8eca70d21444 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Thu, 17 Jun 2021 15:04:08 +0200 Subject: [PATCH 3/7] Track source sync: Ignore bogus file mopdification time stamps --- src/sources/metadatasourcetaglib.cpp | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/sources/metadatasourcetaglib.cpp b/src/sources/metadatasourcetaglib.cpp index 85e09f49a15..6684e820efa 100644 --- a/src/sources/metadatasourcetaglib.cpp +++ b/src/sources/metadatasourcetaglib.cpp @@ -76,7 +76,14 @@ class AiffFile : public TagLib::RIFF::AIFF::File { }; inline QDateTime getSourceSynchronizedAt(const QFileInfo& fileInfo) { - return fileInfo.lastModified().toUTC(); + const QDateTime lastModifiedUtc = fileInfo.lastModified().toUTC(); + // Ignore bogus values like 1970-01-01T00:00:00.000 UTC + // that are obviously incorrect and don't provide any + // information. + if (lastModifiedUtc.toMSecsSinceEpoch() == 0) { + return QDateTime{}; + } + return lastModifiedUtc; } } // anonymous namespace From 64312dc0c37554dcfc37d4d27f416f8663216573 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Thu, 17 Jun 2021 15:04:45 +0200 Subject: [PATCH 4/7] DB v38: Fix 0/NULL issue after upgrade to schema version 37 --- res/schema.xml | 8 ++++++++ src/database/mixxxdb.cpp | 2 +- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/res/schema.xml b/res/schema.xml index 6c086ee9799..421b70ccdd9 100644 --- a/res/schema.xml +++ b/res/schema.xml @@ -566,4 +566,12 @@ reapplying those migrations. ALTER TABLE library ADD COLUMN source_synchronized_ms INTEGER DEFAULT NULL; + + + Fix 0/NULL issue after upgrade to schema version 37. + + + UPDATE library SET source_synchronized_ms=NULL WHERE source_synchronized_ms=0; + + diff --git a/src/database/mixxxdb.cpp b/src/database/mixxxdb.cpp index f2b38a425b8..34d6b66b98f 100644 --- a/src/database/mixxxdb.cpp +++ b/src/database/mixxxdb.cpp @@ -12,7 +12,7 @@ const QString MixxxDb::kDefaultSchemaFile(":/schema.xml"); //static -const int MixxxDb::kRequiredSchemaVersion = 37; +const int MixxxDb::kRequiredSchemaVersion = 38; namespace { From 2e2b50a41a635ac4ede0c362d4a8370ee9efae37 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Thu, 17 Jun 2021 15:17:36 +0200 Subject: [PATCH 5/7] Fix condition ...even though the edge is already handled before and will never occur. --- src/track/trackrecord.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/track/trackrecord.cpp b/src/track/trackrecord.cpp index 9fc7964d4ce..d8e55f355ff 100644 --- a/src/track/trackrecord.cpp +++ b/src/track/trackrecord.cpp @@ -119,7 +119,7 @@ bool TrackRecord::updateSourceSynchronizedAt( return false; // unchanged } if (getSourceSynchronizedAt().isValid() && - getSourceSynchronizedAt() >= sourceSynchronizedAt) { + getSourceSynchronizedAt() > sourceSynchronizedAt) { kLogger.warning() << "Backdating source synchronization time from" << getSourceSynchronizedAt() From 8d3fbc3e2f92c3e17560155ea26a55ab80d09cbd Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Thu, 17 Jun 2021 19:51:27 +0200 Subject: [PATCH 6/7] Add debug assertion to document assumptions about QVariant --- src/library/dao/trackdao.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/library/dao/trackdao.cpp b/src/library/dao/trackdao.cpp index da07fb7c943..99344e42a42 100644 --- a/src/library/dao/trackdao.cpp +++ b/src/library/dao/trackdao.cpp @@ -1213,6 +1213,7 @@ bool setTrackSourceSynchronizedAt(const QSqlRecord& record, const int column, Tr // if the column value is NULL and then converts to 0 (zero). This // is NOT desired and therefore we MUST USE QVariant::isNull() instead! if (!value.isNull() && value.canConvert()) { + DEBUG_ASSERT(value.isValid()); const quint64 msecsSinceEpoch = qvariant_cast(value); sourceSynchronizedAt.setTimeSpec(Qt::UTC); sourceSynchronizedAt.setMSecsSinceEpoch(msecsSinceEpoch); From 1b21ee2b6fe6386f097bc08f2dc6658ba6c49c09 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Thu, 17 Jun 2021 19:56:39 +0200 Subject: [PATCH 7/7] Fix undefined behavior of toMSecsSinceEpoch() if QDateTime is not valid https://doc.qt.io/qt-5/qdatetime.html#toMSecsSinceEpoch --- src/library/export/engineprimeexportjob.cpp | 8 ++++++-- src/sources/metadatasourcetaglib.cpp | 4 +++- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/src/library/export/engineprimeexportjob.cpp b/src/library/export/engineprimeexportjob.cpp index a37b79f3839..112f8936387 100644 --- a/src/library/export/engineprimeexportjob.cpp +++ b/src/library/export/engineprimeexportjob.cpp @@ -151,8 +151,12 @@ void exportMetadata(djinterop::database* pDatabase, snapshot.comment = pTrack->getComment().toStdString(); snapshot.composer = pTrack->getComposer().toStdString(); snapshot.key = toDjinteropKey(pTrack->getKey()); - int64_t lastModifiedMillisSinceEpoch = - pTrack->getFileInfo().lastModified().toMSecsSinceEpoch(); + int64_t lastModifiedMillisSinceEpoch = 0; + const QDateTime fileLastModified = pTrack->getFileInfo().lastModified(); + if (fileLastModified.isValid()) { + // Only defined if valid + lastModifiedMillisSinceEpoch = fileLastModified.toMSecsSinceEpoch(); + } std::chrono::system_clock::time_point lastModifiedAt{ std::chrono::milliseconds{lastModifiedMillisSinceEpoch}}; snapshot.last_modified_at = lastModifiedAt; diff --git a/src/sources/metadatasourcetaglib.cpp b/src/sources/metadatasourcetaglib.cpp index 6684e820efa..9893623e299 100644 --- a/src/sources/metadatasourcetaglib.cpp +++ b/src/sources/metadatasourcetaglib.cpp @@ -80,7 +80,9 @@ inline QDateTime getSourceSynchronizedAt(const QFileInfo& fileInfo) { // Ignore bogus values like 1970-01-01T00:00:00.000 UTC // that are obviously incorrect and don't provide any // information. - if (lastModifiedUtc.toMSecsSinceEpoch() == 0) { + if (lastModifiedUtc.isValid() && + // Only defined if valid + lastModifiedUtc.toMSecsSinceEpoch() == 0) { return QDateTime{}; } return lastModifiedUtc;