[qtcontacts-sqlite-extensions] Add unit tests for TwoWayContactSyncAd…

…apter

This commit adds unit tests for TwoWayContactSyncAdapter to ensure
we don't regress functionality in the future.
It also fixes two bugs found in the sync enablers during testing:
 - local since timestamp could regress in some circumstances
 - remote modifications of locally deleted contacts could be
   handled incorrectly in some cases

src/engine/contactsengine.cpp

@@ -1138,8 +1138,20 @@ bool ContactsEngine::fetchSyncContacts(const QString &syncTarget, const QDateTim
                                        QContactManager::Error *error)
 {
     Q_ASSERT(error);
+    // This function is deprecated and exists for backward compatibility only!
+    qWarning() << Q_FUNC_INFO << "DEPRECATED: use the overload which takes a maxTimestamp argument instead!";
+    QDateTime maxTimestamp;
+    return fetchSyncContacts(syncTarget, lastSync, exportedIds, syncContacts, addedContacts, deletedContactIds, &maxTimestamp, error);
+}
+
+bool ContactsEngine::fetchSyncContacts(const QString &syncTarget, const QDateTime &lastSync, const QList<QContactId> &exportedIds,
+                                       QList<QContact> *syncContacts, QList<QContact> *addedContacts, QList<QContactId> *deletedContactIds,
+                                       QDateTime *maxTimestamp, QContactManager::Error *error)
+{
+    Q_ASSERT(maxTimestamp);
+    Q_ASSERT(error);
 
-    *error = writer()->fetchSyncContacts(syncTarget, lastSync, exportedIds, syncContacts, addedContacts, deletedContactIds);
+    *error = writer()->fetchSyncContacts(syncTarget, lastSync, exportedIds, syncContacts, addedContacts, deletedContactIds, maxTimestamp);
     return (*error == QContactManager::NoError);
 }
 

src/engine/contactsengine.h

@@ -136,6 +136,9 @@ class ContactsEngine : public QtContactsSqliteExtensions::ContactManagerEngine
     bool fetchSyncContacts(const QString &syncTarget, const QDateTime &lastSync, const QList<QContactId> &exportedIds,
                            QList<QContact> *syncContacts, QList<QContact> *addedContacts, QList<QContactId> *deletedContactIds,
                            QContactManager::Error *error);
+    bool fetchSyncContacts(const QString &syncTarget, const QDateTime &lastSync, const QList<QContactId> &exportedIds,
+                           QList<QContact> *syncContacts, QList<QContact> *addedContacts, QList<QContactId> *deletedContactIds,
+                           QDateTime *maxTimestamp, QContactManager::Error *error);
 
     bool storeSyncContacts(const QString &syncTarget, ConflictResolutionPolicy conflictPolicy,
                            const QList<QPair<QContact, QContact> > &remoteChanges, QContactManager::Error *error);

src/engine/contactwriter.cpp

@@ -59,6 +59,26 @@
 
 #include <QtDebug>
 
+namespace {
+    void updateMaxSyncTimestamp(const QList<QContact> *contacts, QDateTime *prevMaxSyncTimestamp)
+    {
+        Q_ASSERT(prevMaxSyncTimestamp);
+
+        if (!contacts) {
+            return;
+        }
+
+        for (int i = 0; i < contacts->size(); ++i) {
+            const QContactTimestamp &ts(contacts->at(i).detail<QContactTimestamp>());
+            if (ts.lastModified().isValid() && (ts.lastModified() > *prevMaxSyncTimestamp || !prevMaxSyncTimestamp->isValid())) {
+                *prevMaxSyncTimestamp = ts.lastModified();
+            } else if (ts.created().isValid() && (ts.created() > *prevMaxSyncTimestamp || !prevMaxSyncTimestamp->isValid())) {
+                *prevMaxSyncTimestamp = ts.created();
+            }
+        }
+    }
+}
+
 using namespace Conversion;
 
 static const QString aggregateSyncTarget(QString::fromLatin1("aggregate"));
@@ -282,7 +302,7 @@ static const char *addedSyncContactIds =
         "\n AND Relationships.type = 'Aggregates'";
 
 static const char *deletedSyncContactIds =
-        "\n SELECT contactId, syncTarget"
+        "\n SELECT contactId, syncTarget, deleted"
         "\n FROM DeletedContacts"
         "\n WHERE deleted > :lastSync";
 
@@ -1821,7 +1841,8 @@ QVariant detailLinkedUris(const QContactDetail &detail)
 
 #ifdef QTCONTACTS_SQLITE_PERFORM_AGGREGATION
 QContactManager::Error ContactWriter::fetchSyncContacts(const QString &syncTarget, const QDateTime &lastSync, const QList<QContactId> &exportedIds,
-                                                        QList<QContact> *syncContacts, QList<QContact> *addedContacts, QList<QContactId> *deletedContactIds)
+                                                        QList<QContact> *syncContacts, QList<QContact> *addedContacts, QList<QContactId> *deletedContactIds,
+                                                        QDateTime *maxTimestamp)
 {
     // Although this is a read operation, it's probably best to make it a transaction
     QMutexLocker locker(ContactsDatabase::accessMutex());
@@ -1837,7 +1858,7 @@ QContactManager::Error ContactWriter::fetchSyncContacts(const QString &syncTarge
         return QContactManager::UnspecifiedError;
     }
 
-    QContactManager::Error error = syncFetch(syncTarget, lastSync, exportedDbIds, syncContacts, addedContacts, deletedContactIds);
+    QContactManager::Error error = syncFetch(syncTarget, lastSync, exportedDbIds, syncContacts, addedContacts, deletedContactIds, maxTimestamp);
     if (error != QContactManager::NoError) {
         rollbackTransaction();
         return error;
@@ -3532,12 +3553,14 @@ struct ConstituentDetails {
 };
 
 QContactManager::Error ContactWriter::syncFetch(const QString &syncTarget, const QDateTime &lastSync, const QSet<quint32> &exportedIds,
-                                                QList<QContact> *syncContacts, QList<QContact> *addedContacts, QList<QContactId> *deletedContactIds)
+                                                QList<QContact> *syncContacts, QList<QContact> *addedContacts, QList<QContactId> *deletedContactIds,
+                                                QDateTime *maxTimestamp)
 {
     static const DetailList unpromotedDetailTypes(getUnpromotedDetailTypes());
     static const DetailList absolutelyUnpromotedDetailTypes(getAbsolutelyUnpromotedDetailTypes());
 
     const QDateTime since(lastSync.isValid() ? lastSync : epochDateTime());
+    *maxTimestamp = since; // fall back to current sync timestamp if no data found.
 
     if (syncContacts || addedContacts) {
         QSet<quint32> aggregateIds;
@@ -3776,6 +3799,11 @@ QContactManager::Error ContactWriter::syncFetch(const QString &syncTarget, const
         }
     }
 
+    // determine the max created/modified/deleted timestamp from the data.
+    // this timestamp should be used as the syncTimestamp during the next sync.
+    updateMaxSyncTimestamp(syncContacts, maxTimestamp);
+    updateMaxSyncTimestamp(addedContacts, maxTimestamp);
+
     if (deletedContactIds) {
         m_deletedSyncContactIds.bindValue(":lastSync", since);
         if (!m_deletedSyncContactIds.exec()) {
@@ -3786,10 +3814,14 @@ QContactManager::Error ContactWriter::syncFetch(const QString &syncTarget, const
         while (m_deletedSyncContactIds.next()) {
             const quint32 dbId(m_deletedSyncContactIds.value(0).toUInt());
             const QString st(m_deletedSyncContactIds.value(1).toString());
+            const QDateTime deleted(m_deletedSyncContactIds.value(2).toDateTime());
 
             // If this contact was from this source, or was exported to this source, report the deletion
             if (st == syncTarget || exportedIds.contains(dbId)) {
                 deletedContactIds->append(ContactId::apiId(dbId));
+                if (deleted > *maxTimestamp) {
+                    *maxTimestamp = deleted;
+                }
             }
         }
         m_deletedSyncContactIds.finish();
@@ -4062,14 +4094,20 @@ QContactManager::Error ContactWriter::syncUpdate(const QString &syncTarget,
 
             QList<QContact> readList;
             QContactManager::Error readError = m_reader->readContacts(QLatin1String("syncUpdate"), &readList, affectedContactIds.toList(), hint);
-            if (readError != QContactManager::NoError || readList.size() != affectedContactIds.size()) {
+            if ((readError != QContactManager::NoError && readError != QContactManager::DoesNotExistError) || readList.size() != affectedContactIds.size()) {
+                // note that if a contact was deleted locally and modified remotely,
+                // the remote-modification may reference a contact which no longer exists locally.
                 QTCONTACTS_SQLITE_WARNING(QString::fromLatin1("Failed to read contacts for sync update"));
                 return QContactManager::UnspecifiedError;
             }
 
             QMap<quint32, QContact> modifiedContacts;
             foreach (const QContact &contact, readList) {
-                modifiedContacts.insert(ContactId::databaseId(contact.id()), contact);
+                // the contact might be empty if it was removed locally and then modified remotely.
+                // TODO: support PreserveRemoteChanges semantics.
+                if (contact != QContact()) {
+                    modifiedContacts.insert(ContactId::databaseId(contact.id()), contact);
+                }
             }
 
             // Create updated versions of the affected contacts

src/engine/contactwriter.h

@@ -101,7 +101,8 @@ class ContactWriter
 
 #ifdef QTCONTACTS_SQLITE_PERFORM_AGGREGATION
     QContactManager::Error fetchSyncContacts(const QString &syncTarget, const QDateTime &lastSync, const QList<QContactId> &exportedIds,
-                                             QList<QContact> *syncContacts, QList<QContact> *addedContacts, QList<QContactId> *deletedContactIds);
+                                             QList<QContact> *syncContacts, QList<QContact> *addedContacts, QList<QContactId> *deletedContactIds,
+                                             QDateTime *maxTimestamp);
 
     QContactManager::Error updateSyncContacts(const QString &syncTarget,
                                               QtContactsSqliteExtensions::ContactManagerEngine::ConflictResolutionPolicy conflictPolicy,
@@ -137,7 +138,8 @@ class ContactWriter
     QContactManager::Error recordAffectedSyncTargets(const QVariantList &ids);
 
     QContactManager::Error syncFetch(const QString &syncTarget, const QDateTime &lastSync, const QSet<quint32> &exportedIds,
-                                     QList<QContact> *syncContacts, QList<QContact> *addedContacts, QList<QContactId> *deletedContactIds);
+                                     QList<QContact> *syncContacts, QList<QContact> *addedContacts, QList<QContactId> *deletedContactIds,
+                                     QDateTime *maxTimestamp);
 
     QContactManager::Error syncUpdate(const QString &syncTarget,
                                       QtContactsSqliteExtensions::ContactManagerEngine::ConflictResolutionPolicy conflictPolicy,

src/extensions/contactmanagerengine.h

@@ -66,9 +66,12 @@ class Q_DECL_EXPORT ContactManagerEngine
     void setMergePresenceChanges(bool b) { m_mergePresenceChanges = b; }
 
 #ifdef QTCONTACTS_SQLITE_PERFORM_AGGREGATION
+    virtual bool Q_DECL_DEPRECATED fetchSyncContacts(const QString &syncTarget, const QDateTime &lastSync, const QList<QContactId> &exportedIds,
+                                   QList<QContact> *syncContacts, QList<QContact> *addedContacts, QList<QContactId> *deletedContactIds,
+                                   QContactManager::Error *error) = 0; // DEPRECATED
     virtual bool fetchSyncContacts(const QString &syncTarget, const QDateTime &lastSync, const QList<QContactId> &exportedIds,
                                    QList<QContact> *syncContacts, QList<QContact> *addedContacts, QList<QContactId> *deletedContactIds,
-                                   QContactManager::Error *error) = 0;
+                                   QDateTime *maxTimestamp, QContactManager::Error *error) = 0;
 
     virtual bool storeSyncContacts(const QString &syncTarget, ConflictResolutionPolicy conflictPolicy,
                                    const QList<QPair<QContact, QContact> > &remoteChanges, QContactManager::Error *error) = 0;

src/extensions/twowaycontactsyncadapter.h

@@ -117,9 +117,9 @@ class TwoWayContactSyncAdapter
     int exactDetailMatchExistsInList(const QContactDetail &det, const QList<QContactDetail> &list) const;
     void removeIgnorableDetailsFromList(QList<QContactDetail> *dets) const;
     int exactContactMatchExistsInList(const QContact &c, const QList<QContact> &list) const;
-    void dumpContact(const QContact &c) const; // debugging.
 
 protected:
+    void dumpContact(const QContact &c) const; // debugging.
     TwoWayContactSyncAdapterPrivate *d;
 };
 }

src/extensions/twowaycontactsyncadapter_impl.h

@@ -63,6 +63,7 @@ namespace QtContactsSqliteExtensions {
             QString m_oobScope;
             QDateTime m_localSince;
             QDateTime m_remoteSince;
+            QDateTime m_newLocalSince;
             QDateTime m_newRemoteSince;
             QList<QContactId> m_exportedIds;
             QList<QContact> m_prevRemote;
@@ -310,16 +311,19 @@ bool TwoWayContactSyncAdapter::determineLocalChanges(QDateTime *localSince,
     // Note: if localSince isn't valid, then we're doing a clean sync, and we can
     // to pass in zero-pointers for modified/deleted list (or at least, ignore them)
     // (otherwise enabling/disabling/re-enabling an account could cause local deletions
-    //  to be pushed upstream, which would be very bad (tm)).
+    // to be pushed upstream, which would be very bad (tm)).
+    // The backend will populate the newLocalSince timestamp appropriately.
     QContactManager::Error error;
     QList<QContactId> locallyDeletedIds;
     bool cleanSync = d->m_stateData[accountId].m_localSince.isValid() ? false : true;
+
     if (!d->m_engine->fetchSyncContacts(d->m_syncTarget,
                                         d->m_stateData[accountId].m_localSince,
                                         d->m_stateData[accountId].m_exportedIds,
                                         locallyModified,
                                         locallyAdded,
                                         &locallyDeletedIds,
+                                        &d->m_stateData[accountId].m_newLocalSince,
                                         &error)) {
         qWarning() << Q_FUNC_INFO << "error - couldn't fetch locally modified sync contacts!";
         d->clear(accountId);
@@ -524,17 +528,9 @@ bool TwoWayContactSyncAdapter::storeSyncStateData(const QString &accountId)
     writeExportedIds << d->m_stateData[accountId].m_exportedIds;
     values.insert(QStringLiteral("exportedIds"), QVariant(cdata));
 
-    // finally, determine new local since timestamp and store both of them to oob.
-    // NOTE: the newLocalSince timestamp may miss updates due to qtcontacts-sqlite setting modified timestamp. TODO!
-    QDateTime newLocalSince = maxModificationTimestamp(d->m_stateData[accountId].m_mutatedPrevRemote);
-    if (!newLocalSince.isValid()) {
-        // if no changes have ever occurred locally, we can use the same timestamp we
-        // will use for the remote since, as that timestamp is from a valid time prior
-        // to when we retrieved local changes during this sync run.
-        newLocalSince = d->m_stateData[accountId].m_newRemoteSince;
-    }
+    // finally, store the new local and remote since timestamps to the OOB db
     values.insert(QStringLiteral("remoteSince"), QVariant(d->m_stateData[accountId].m_newRemoteSince));
-    values.insert(QStringLiteral("localSince"), QVariant(newLocalSince));
+    values.insert(QStringLiteral("localSince"), QVariant(d->m_stateData[accountId].m_newLocalSince));
 
     // perform the store operation to the oob db.
     if (!d->m_engine->storeOOB(d->m_stateData[accountId].m_oobScope, values)) {