SyncSource API: support and use Synthesis DB_DataMerged/Replace/Conflict result codes (BMC #22783)

Returning 207 = DB_DataMerged when an item replaced an existing one
wasn't correct (the data wasn't really merged) and also wasn't handled
as intended by the Synthesis engine. Now a backend can properly report what it did:
- data fully replaced
- data was merged
- data needs to be merged by engine

The last option is used by EDS and CalDAV when an add<->add conflict
is detected by the backends. In that case the Synthesis engine will
read the local item, merge it with the incoming item and write back
the result.

At the moment the tests assume that the more recent item wins
completely. But our field list config specifies a merge=lines mode for
some fields, which results in concatenating these fields in the merged
item. Thus the tests currently fail. Need to decide which behavior is
desired.

src/backends/akonadi/akonadisyncsource.cpp

@@ -198,7 +198,7 @@ TrackingSyncSource::InsertItemResult AkonadiSyncSource::insertItem(const std::st
         ItemCreateJob *createJob = new ItemCreateJob(item, m_collection);
         if (!createJob->exec()) {
             throwError(string("storing new item ") + luid);
-            return InsertItemResult("", "", false);
+            return InsertItemResult("", "", ITEM_OKAY);
         }
         item = createJob->item();
     } else {
@@ -215,7 +215,7 @@ TrackingSyncSource::InsertItemResult AkonadiSyncSource::insertItem(const std::st
         // TODO: check that the item has not been updated in the meantime
         if (!modifyJob->exec()) {
             throwError(string("updating item ") + luid);
-            return InsertItemResult("", "", false);
+            return InsertItemResult("", "", ITEM_OKAY);
         }
         item = modifyJob->item();
     }
@@ -225,7 +225,7 @@ TrackingSyncSource::InsertItemResult AkonadiSyncSource::insertItem(const std::st
     // above will take care of this
     return InsertItemResult(QByteArray::number(item.id()).constData(),
                             QByteArray::number(item.revision()).constData(),
-                            false);
+                            ITEM_OKAY);
 }
 
 void AkonadiSyncSource::removeItem(const string &luid)

src/backends/evolution/EvolutionCalendarSource.cpp

@@ -274,7 +274,7 @@ void EvolutionCalendarSource::readItem(const string &luid, std::string &item, bo
 EvolutionCalendarSource::InsertItemResult EvolutionCalendarSource::insertItem(const string &luid, const std::string &item, bool raw)
 {
     bool update = !luid.empty();
-    bool merged = false;
+    InsertItemResultState state = ITEM_OKAY;
     bool detached = false;
     string newluid = luid;
     string data = item;
@@ -390,7 +390,7 @@ EvolutionCalendarSource::InsertItemResult EvolutionCalendarSource::insertItem(co
         // which should never happen.
         newluid = id.getLUID();
         if (m_allLUIDs.find(newluid) != m_allLUIDs.end()) {
-            merged = true;
+            state = ITEM_NEEDS_MERGE;
         } else {
             // if this is a detached recurrence, then we
             // must use e_cal_modify_object() below if
@@ -438,7 +438,7 @@ EvolutionCalendarSource::InsertItemResult EvolutionCalendarSource::insertItem(co
         }
     }
 
-    if (update || merged || detached) {
+    if (update || state != ITEM_NEEDS_MERGE || detached) {
         ItemID id(newluid);
         bool isParent = id.m_rid.empty();
 
@@ -526,7 +526,7 @@ EvolutionCalendarSource::InsertItemResult EvolutionCalendarSource::insertItem(co
         modTime = getItemModTime(newid);
     }
 
-    return InsertItemResult(newluid, modTime, merged);
+    return InsertItemResult(newluid, modTime, state);
 }
 
 EvolutionCalendarSource::ICalComps_t EvolutionCalendarSource::removeEvents(const string &uid, bool returnOnlyChildren)

src/backends/evolution/EvolutionContactSource.cpp

@@ -330,7 +330,7 @@ EvolutionContactSource::insertItem(const string &uid, const std::string &item, b
                 throwError("no UID for contact");
             }
             string newrev = getRevision(newuid);
-            return InsertItemResult(newuid, newrev, false);
+            return InsertItemResult(newuid, newrev, ITEM_OKAY);
         } else {
             throwError(uid.empty() ?
                        "storing new contact" :
@@ -341,7 +341,7 @@ EvolutionContactSource::insertItem(const string &uid, const std::string &item, b
         throwError(string("failure parsing vcard " ) + item);
     }
     // not reached!
-    return InsertItemResult("", "", false);
+    return InsertItemResult("", "", ITEM_OKAY);
 }
 
 void EvolutionContactSource::removeItem(const string &uid)

src/backends/evolution/EvolutionMemoSource.cpp

@@ -122,7 +122,7 @@ EvolutionCalendarSource::InsertItemResult EvolutionMemoSource::insertItem(const
     }
     
     bool update = !luid.empty();
-    bool merged = false;
+    InsertItemResultState state = ITEM_OKAY;
     string newluid = luid;
     string modTime;
 
@@ -170,28 +170,31 @@ EvolutionCalendarSource::InsertItemResult EvolutionMemoSource::insertItem(const
     }
 
     GError *gerror = NULL;
-    char *uid = NULL;
-
     if (!update) {
-        if(!e_cal_create_object(m_calendar, subcomp, &uid, &gerror)) {
+        const char *uid = NULL;
+
+        if(!e_cal_create_object(m_calendar, subcomp, (char **)&uid, &gerror)) {
             if (gerror->domain == E_CALENDAR_ERROR &&
                 gerror->code == E_CALENDAR_STATUS_OBJECT_ID_ALREADY_EXISTS) {
                 // Deal with error due to adding already existing item.
                 // Should never happen for plain text journal entries because
                 // they have no embedded ID, but who knows...
-                merged = true;
+                state = ITEM_NEEDS_MERGE;
+                uid = icalcomponent_get_uid(subcomp);
+                if (!uid) {
+                    throwError("storing new memo item, no UID set", gerror);
+                }
                 g_clear_error(&gerror);
             } else {
                 throwError( "storing new memo item", gerror );
             }
-        } else {
-            ItemID id(uid, "");
-            newluid = id.getLUID();
+        }
+        ItemID id(uid, "");
+        newluid = id.getLUID();
+        if (state != ITEM_NEEDS_MERGE) {
             modTime = getItemModTime(id);
         }
-    }
-
-    if (update || merged) {
+    } else {
         ItemID id(newluid);
 
         // ensure that the component has the right UID
@@ -207,7 +210,7 @@ EvolutionCalendarSource::InsertItemResult EvolutionMemoSource::insertItem(const
         modTime = getItemModTime(newid);
     }
 
-    return InsertItemResult(newluid, modTime, merged);
+    return InsertItemResult(newluid, modTime, state);
 }
 
 bool EvolutionMemoSource::isNativeType(const char *type)

src/backends/file/FileSyncSource.cpp

@@ -233,7 +233,7 @@ TrackingSyncSource::InsertItemResult FileSyncSource::insertItem(const string &ui
 
     return InsertItemResult(newuid,
                             getATimeString(filename),
-                            false /* true if adding item was turned into update */);
+                            ITEM_OKAY);
 }
 
 

src/backends/webdav/CalDAVSource.cpp

@@ -433,8 +433,9 @@ SubSyncSource::SubItemResult CalDAVSource::insertSubItem(const std::string &luid
             Event &event = loadItem(*it->second);
             event.m_etag = res.m_revision;
             if (event.m_subids.find(subid) != event.m_subids.end()) {
-                // was already in that item but caller didn't seem to know
-                subres.m_merged = true;
+                // was already in that item but caller didn't seem to know,
+                // and now we replaced the data on the CalDAV server
+                subres.m_state = ITEM_REPLACED;
             } else {
                 // add to merged item
                 event.m_subids.insert(subid);                
@@ -548,14 +549,11 @@ SubSyncSource::SubItemResult CalDAVSource::insertSubItem(const std::string &luid
             }
         }
         if (davLUID != luid) {
-            // caller didn't know final UID: if found, the tell him that
-            // we merged the item for him, if not, then don't complain about
-            // it not being found (like we do when the item should exist
-            // but doesn't)
+            // caller didn't know final UID: if found, then tell him to
+            // merge the data and try again
             if (removeme) {
-                subres.m_merged = true;
-                icalcomponent_remove_component(event.m_calendar, removeme);
-                icalcomponent_free(removeme);
+                subres.m_state = ITEM_NEEDS_MERGE;
+                goto done;
             } else {
                 event.m_subids.insert(subid);
             }
@@ -586,7 +584,7 @@ SubSyncSource::SubItemResult CalDAVSource::insertSubItem(const std::string &luid
         try {
             SE_LOG_DEBUG(this, NULL, "updating VEVENT");
             InsertItemResult res = insertItem(event.m_DAVluid, data, true);
-            if (res.m_merged ||
+            if (res.m_state != ITEM_OKAY ||
                 res.m_luid != event.m_DAVluid) {
                 // should not merge with anything, if so, our cache was invalid
                 SE_THROW("CalDAV item not updated as expected");
@@ -654,7 +652,7 @@ SubSyncSource::SubItemResult CalDAVSource::insertSubItem(const std::string &luid
                 eptr<char> icalstr(ical_strdup(icalcomponent_as_ical_string(event.m_calendar)));
                 std::string data = icalstr.get();
                 InsertItemResult res = insertItem(event.m_DAVluid, data, true);
-                if (res.m_merged ||
+                if (res.m_state != ITEM_OKAY ||
                     res.m_luid != event.m_DAVluid) {
                     // should not merge with anything, if so, our cache was invalid
                     SE_THROW("CalDAV item not updated as expected");
@@ -667,6 +665,7 @@ SubSyncSource::SubItemResult CalDAVSource::insertSubItem(const std::string &luid
         }
     }
 
+ done:
     return subres;
 }
 
@@ -850,7 +849,7 @@ std::string CalDAVSource::removeSubItem(const string &davLUID, const std::string
         } else {
             res = insertItem(davLUID, icalstr.get(), true);
         }
-        if (res.m_merged ||
+        if (res.m_state != ITEM_OKAY ||
             res.m_luid != davLUID) {
             SE_THROW("unexpected result of removing sub event");
         }

src/backends/webdav/WebDAVSource.cpp

@@ -1020,7 +1020,7 @@ TrackingSyncSource::InsertItemResult WebDAVSource::insertItem(const string &uid,
 {
     std::string new_uid;
     std::string rev;
-    bool update = false;  /* true if adding item was turned into update */
+    InsertItemResultState state = ITEM_OKAY;
 
     Timespec deadline = createDeadline(); // no resending if left empty
     m_session->startOperation("PUT", deadline);
@@ -1087,7 +1087,7 @@ TrackingSyncSource::InsertItemResult WebDAVSource::insertItem(const string &uid,
             SE_LOG_DEBUG(NULL, NULL, "new item mapped to %s", real_luid.c_str());
             new_uid = real_luid;
             // TODO: find a better way of detecting unexpected updates.
-            // update = true;
+            // state = ...
         } else if (!rev.empty()) {
             // Yahoo Contacts returns an etag, but no href. For items
             // that were really created as requested, that's okay. But
@@ -1114,7 +1114,7 @@ TrackingSyncSource::InsertItemResult WebDAVSource::insertItem(const string &uid,
                              new_uid.c_str(),
                              revisions.begin()->first.c_str());
                 new_uid = revisions.begin()->first;
-                update = true;
+                state = ITEM_REPLACED;
             }
         }
     } else {
@@ -1182,7 +1182,7 @@ TrackingSyncSource::InsertItemResult WebDAVSource::insertItem(const string &uid,
         }
     }
 
-    return InsertItemResult(new_uid, rev, update);
+    return InsertItemResult(new_uid, rev, state);
 }
 
 std::string WebDAVSource::ETag2Rev(const std::string &etag)

src/backends/xmlrpc/XMLRPCSyncSource.cpp

@@ -141,7 +141,7 @@ TrackingSyncSource::InsertItemResult XMLRPCSyncSource::insertItem(const string &
 
     return InsertItemResult((*it).first,
                             xmlrpc_c::value_string((*it).second),
-                            false);
+                            ITEM_OKAY);
 }
 
 

src/syncevo/MapSyncSource.cpp

@@ -288,12 +288,15 @@ SyncSourceRaw::InsertItemResult MapSyncSource::insertItem(const std::string &lui
 {
     StringPair ids = splitLUID(luid);
     SubSyncSource::SubItemResult res = m_sub->insertSubItem(ids.first, ids.second, item);
-    SubRevisionEntry &entry = m_revisions[res.m_mainid];
-    entry.m_uid = res.m_uid;
-    entry.m_revision = res.m_revision;
-    entry.m_subids.insert(res.m_subid);
+    // anything changed?
+    if (res.m_state != ITEM_NEEDS_MERGE) {
+        SubRevisionEntry &entry = m_revisions[res.m_mainid];
+        entry.m_uid = res.m_uid;
+        entry.m_revision = res.m_revision;
+        entry.m_subids.insert(res.m_subid);
+    }
     return SyncSourceRaw::InsertItemResult(createLUID(res.m_mainid, res.m_subid),
-                                           res.m_revision, res.m_merged);
+                                           res.m_revision, res.m_state);
 }
 
 void MapSyncSource::readItem(const std::string &luid, std::string &item)

src/syncevo/MapSyncSource.h

@@ -67,7 +67,7 @@ class SubSyncSource : virtual public SyncSourceBase
     class SubItemResult {
     public:
         SubItemResult() :
-            m_merged(false)
+            m_state(ITEM_OKAY)
         {}
         
         /**
@@ -79,25 +79,25 @@ class SubSyncSource : virtual public SyncSourceBase
          * @param uid       an arbitrary string, stored, but not used by MapSyncSource;
          *                  used in the CalDAV backend to associate mainid (= resource path)
          *                  with UID (= part of the item content, but with special semantic)
-         * @param merged    set this to true if an existing sub item was updated instead of adding it
+         * @param state     report about what was done with the data
          */
         SubItemResult(const string &mainid,
                       const string &subid,
                       const string &revision,
                       const string &uid,
-                      bool merged) :
+                      InsertItemResultState state) :
             m_mainid(mainid),
             m_subid(subid),
             m_revision(revision),
             m_uid(uid),
-            m_merged(merged)
+            m_state(state)
         {}
 
         string m_mainid;
         string m_subid;
         string m_revision;
         string m_uid;
-        bool m_merged;
+        InsertItemResultState m_state;
     };
 
     SubSyncSource() : m_parent(NULL) {}

src/syncevo/SyncSource.cpp

@@ -668,8 +668,18 @@ sysync::TSyError SyncSourceSerialize::insertItemAsKey(sysync::KeyH aItemKey, sys
         InsertItemResult inserted =
             insertItem(!aID ? "" : aID->item, data.get());
         newID->item = StrAlloc(inserted.m_luid.c_str());
-        if (inserted.m_merged) {
+        switch (inserted.m_state) {
+        case ITEM_OKAY:
+            break;
+        case ITEM_REPLACED:
+            res = sysync::DB_DataReplaced;
+            break;
+        case ITEM_MERGED:
             res = sysync::DB_DataMerged;
+            break;
+        case ITEM_NEEDS_MERGE:
+            res = sysync::DB_Conflict;
+            break;
         }
     }
 

src/syncevo/SyncSource.h

@@ -1420,6 +1420,53 @@ class SyncSourceDelete : virtual public SyncSourceBase {
     sysync::TSyError deleteItemSynthesis(sysync::cItemID aID);
 };
 
+enum InsertItemResultState {
+    /**
+     * item added or updated as requested
+     */
+    ITEM_OKAY,
+
+    /**
+     * When a backend is asked to add an item and recognizes
+     * that the item matches an already existing item, it may
+     * replace that item instead of creating a duplicate. In this
+     * case it must return ITEM_REPLACED and set the luid/revision
+     * of that updated item.
+     *
+     * This can happen when such an item was added concurrently to
+     * the running sync or, more likely, was reported as new by
+     * the backend and the engine failed to find the match because
+     * it doesn't know about some special semantic, like iCalendar
+     * 2.0 UID).
+     *
+     * Note that depending on the age of the items, the older data
+     * will replace the more recent one when always using item
+     * replacement.
+     */
+    ITEM_REPLACED,
+
+    /**
+     * Same as ITEM_REPLACED, except that the backend did some
+     * modifications to the data that was sent to it before
+     * storing it, like merging it with the existing item. The
+     * engine will treat the updated item as modified and send
+     * back the update to the peer as soon as possible. In server
+     * mode that will be in the same sync session, in a client in
+     * the next session (client cannot send changes after having
+     * received data from the server).
+     */
+    ITEM_MERGED,
+
+    /**
+     * As before, a match against an existing item was detected.
+     * By returning this state and the luid of the matched item
+     * (revision not needed) the engine is instructed to do the
+     * necessary data comparison and merging itself. Useful when a
+     * backend can't do the necessary merging itself.
+     */
+    ITEM_NEEDS_MERGE
+};
+
 /**
  * an interface for reading and writing items in the internal
  * format; see SyncSourceSerialize for an explanation
@@ -1429,26 +1476,26 @@ class SyncSourceRaw : virtual public SyncSourceBase {
     class InsertItemResult {
     public:
         InsertItemResult() :
-            m_merged(false)
+            m_state(ITEM_OKAY)
             {}
         
         /**
          * @param luid      the LUID after the operation; during an update the LUID must
          *                  not be changed, so return the original one here
          * @param revision  the revision string after the operation; leave empty if not used
-         * @param merged    set this to true if an existing item was updated instead of adding it
+         * @param state     report about what was done with the data
          */
         InsertItemResult(const string &luid,
                          const string &revision,
-                         bool merged) :
+                         InsertItemResultState state) :
         m_luid(luid),
             m_revision(revision),
-            m_merged(merged)
+            m_state(state)
             {}
 
         string m_luid;
         string m_revision;
-        bool m_merged;
+        InsertItemResultState m_state;
     };
 
     /** same as SyncSourceSerialize::insertItem(), but with internal format */

src/syncevo/TrackingSyncSource.cpp

@@ -140,14 +140,18 @@ std::string TrackingSyncSource::endSync(bool success)
 TrackingSyncSource::InsertItemResult TrackingSyncSource::insertItem(const std::string &luid, const std::string &item)
 {
     InsertItemResult res = insertItem(luid, item, false);
-    updateRevision(*m_trackingNode, luid, res.m_luid, res.m_revision);
+    if (res.m_state != ITEM_NEEDS_MERGE) {
+        updateRevision(*m_trackingNode, luid, res.m_luid, res.m_revision);
+    }
     return res;
 }
 
 TrackingSyncSource::InsertItemResult TrackingSyncSource::insertItemRaw(const std::string &luid, const std::string &item)
 {
     InsertItemResult res = insertItem(luid, item, true);
-    updateRevision(*m_trackingNode, luid, res.m_luid, res.m_revision);
+    if (res.m_state != ITEM_NEEDS_MERGE) {
+        updateRevision(*m_trackingNode, luid, res.m_luid, res.m_revision);
+    }
     return res;
 }
 

test/ClientTest.cpp

@@ -329,6 +329,16 @@ std::string LocalTests::insert(CreateSource createSource, const std::string &dat
     SOURCE_ASSERT_NO_FAILURE(source.get(), res = source->insertItemRaw("", mangled));
     CPPUNIT_ASSERT(!res.m_luid.empty());
 
+    bool updated = false;
+    if (res.m_state == ITEM_NEEDS_MERGE) {
+        // conflict detected, overwrite existing item as done in the past
+        std::string luid = res.m_luid;
+        SOURCE_ASSERT_NO_FAILURE(source.get(), res = source->insertItemRaw(luid, mangled));
+        CPPUNIT_ASSERT_EQUAL(luid, res.m_luid);
+        CPPUNIT_ASSERT(res.m_state == ITEM_OKAY);
+        updated = true;
+    }
+
     // delete source again
     CPPUNIT_ASSERT_NO_THROW(source.reset());
 
@@ -337,7 +347,7 @@ std::string LocalTests::insert(CreateSource createSource, const std::string &dat
         // - a new item was added
         // - the item was matched against an existing one
         CPPUNIT_ASSERT_NO_THROW(source.reset(createSource()));
-        CPPUNIT_ASSERT_EQUAL(numItems + (res.m_merged ? 0 : 1),
+        CPPUNIT_ASSERT_EQUAL(numItems + ((res.m_state == ITEM_REPLACED || res.m_state == ITEM_MERGED || updated) ? 0 : 1),
                              countItems(source.get()));
         CPPUNIT_ASSERT(countNewItems(source.get()) == 0);
         CPPUNIT_ASSERT(countUpdatedItems(source.get()) == 0);
@@ -394,8 +404,11 @@ void LocalTests::update(CreateSource createSource, const std::string &data, bool
     SOURCE_ASSERT_NO_FAILURE(source.get(), it = source->getAllItems().begin());
     CPPUNIT_ASSERT(it != source->getAllItems().end());
     string luid = *it;
-    SOURCE_ASSERT_NO_FAILURE(source.get(), source->insertItemRaw(luid, config.m_mangleItem(data, true)));
+    SyncSourceRaw::InsertItemResult res;
+    SOURCE_ASSERT_NO_FAILURE(source.get(), res = source->insertItemRaw(luid, config.m_mangleItem(data, true)));
     CPPUNIT_ASSERT_NO_THROW(source.reset());
+    CPPUNIT_ASSERT_EQUAL(luid, res.m_luid);
+    CPPUNIT_ASSERT_EQUAL(ITEM_OKAY, res.m_state);
 
     if (!check) {
         return;