Merge : Synchronising groups.

src/core/Group.cpp

@@ -77,8 +77,7 @@ template <class P, class V> inline bool Group::set(P& property, const V& value)
         updateTimeinfo();
         emit modified();
         return true;
-    }
+    } else {
-    else {
         return false;
     }
 }
@@ -693,27 +692,48 @@ void Group::merge(const Group* other)
                 existingEntry->setGroup(this);
                 qDebug("Location changed for entry %s. Updating it", qPrintable(existingEntry->title()));
             }
-            resolveConflict(existingEntry, entry);
+            resolveEntryConflict(existingEntry, entry);
         }
     }
 
     // merge groups recursively
     const QList<Group*> dbChildren = other->children();
     for (Group* group : dbChildren) {
-        // groups are searched by name instead of uuid
+
-        if (findChildByName(group->name())) {
+        Group* existingGroup = rootGroup->findChildByUuid(group->uuid());
-            findChildByName(group->name())->merge(group);
+
-        } else {
+        if (!existingGroup) {
             qDebug("New group %s detected. Creating it.", qPrintable(group->name()));
-            Group* newGroup = group->clone(Entry::CloneNoFlags, true);
+            Group* newGroup = group->clone(Entry::CloneNoFlags, Group::CloneNoFlags);
             newGroup->setParent(this);
             newGroup->merge(group);
+        } else {
+            bool locationChanged = existingGroup->timeInfo().locationChanged() < group->timeInfo().locationChanged();
+            if (locationChanged && existingGroup->parent() != this) {
+                existingGroup->setParent(this);
+                qDebug("Location changed for group %s. Updating it", qPrintable(existingGroup->name()));
+            }
+            resolveGroupConflict(existingGroup, group);
+            existingGroup->merge(group);
         }
+
     }
 
     emit modified();
 }
 
+Group* Group::findChildByUuid(const Uuid& uuid)
+{
+    Q_ASSERT(!uuid.isNull());
+    for (Group* group : groupsRecursive(true)) {
+        if (group->uuid() == uuid) {
+            return group;
+        }
+    }
+
+    return nullptr;
+}
+
 Group* Group::findChildByName(const QString& name)
 {
     for (Group* group : asConst(m_children)) {
@@ -725,16 +745,21 @@ Group* Group::findChildByName(const QString& name)
     return nullptr;
 }
 
-Group* Group::clone(Entry::CloneFlags entryFlags, bool shallow) const
+Group* Group::clone(Entry::CloneFlags entryFlags, Group::CloneFlags groupFlags) const
 {
     Group* clonedGroup = new Group();
 
     clonedGroup->setUpdateTimeinfo(false);
 
-    clonedGroup->setUuid(Uuid::random());
+    if (groupFlags & Group::CloneNewUuid) {
+        clonedGroup->setUuid(Uuid::random());
+    } else {
+        clonedGroup->setUuid(this->uuid());
+    }
+
     clonedGroup->m_data = m_data;
 
-    if (!shallow) {
+    if (groupFlags & Group::CloneIncludeEntries) {
         const QList<Entry*> entryList = entries();
         for (Entry* entry : entryList) {
             Entry* clonedEntry = entry->clone(entryFlags);
@@ -743,18 +768,20 @@ Group* Group::clone(Entry::CloneFlags entryFlags, bool shallow) const
 
         const QList<Group*> childrenGroups = children();
         for (Group* groupChild : childrenGroups) {
-            Group* clonedGroupChild = groupChild->clone(entryFlags);
+            Group* clonedGroupChild = groupChild->clone(entryFlags, groupFlags);
             clonedGroupChild->setParent(clonedGroup);
         }
     }
 
     clonedGroup->setUpdateTimeinfo(true);
+    if (groupFlags & Group::CloneResetTimeInfo) {
 
-    QDateTime now = QDateTime::currentDateTimeUtc();
+        QDateTime now = QDateTime::currentDateTimeUtc();
-    clonedGroup->m_data.timeInfo.setCreationTime(now);
+        clonedGroup->m_data.timeInfo.setCreationTime(now);
-    clonedGroup->m_data.timeInfo.setLastModificationTime(now);
+        clonedGroup->m_data.timeInfo.setLastModificationTime(now);
-    clonedGroup->m_data.timeInfo.setLastAccessTime(now);
+        clonedGroup->m_data.timeInfo.setLastAccessTime(now);
-    clonedGroup->m_data.timeInfo.setLocationChanged(now);
+        clonedGroup->m_data.timeInfo.setLocationChanged(now);
+    }
 
     return clonedGroup;
 }
@@ -908,7 +935,7 @@ bool Group::resolveAutoTypeEnabled() const
     }
 }
 
-void Group::resolveConflict(Entry* existingEntry, Entry* otherEntry)
+void Group::resolveEntryConflict(Entry* existingEntry, Entry* otherEntry)
 {
     const QDateTime timeExisting = existingEntry->timeInfo().lastModificationTime();
     const QDateTime timeOther = otherEntry->timeInfo().lastModificationTime();
@@ -946,6 +973,23 @@ void Group::resolveConflict(Entry* existingEntry, Entry* otherEntry)
     }
 }
 
+void Group::resolveGroupConflict(Group* existingGroup, Group* otherGroup)
+{
+    const QDateTime timeExisting = existingGroup->timeInfo().lastModificationTime();
+    const QDateTime timeOther = otherGroup->timeInfo().lastModificationTime();
+
+    // only if the other group newer, update the existing one.
+    if (timeExisting < timeOther) {
+        qDebug("Updating group %s.", qPrintable(existingGroup->name()));
+        existingGroup->setName(otherGroup->name());
+        existingGroup->setNotes(otherGroup->notes());
+        existingGroup->setIcon(otherGroup->iconNumber());
+        existingGroup->setIcon(otherGroup->iconUuid());
+        existingGroup->setExpiryTime(otherGroup->timeInfo().expiryTime());
+    }
+
+}
+
 QStringList Group::locate(QString locateTerm, QString currentPath)
 {
     Q_ASSERT(!locateTerm.isNull());

src/core/Group.h

@@ -37,6 +37,14 @@ public:
     enum TriState { Inherit, Enable, Disable };
     enum MergeMode { ModeInherit, KeepBoth, KeepNewer, KeepExisting };
 
+    enum CloneFlag {
+        CloneNoFlags        = 0,
+        CloneNewUuid        = 1,  // generate a random uuid for the clone
+        CloneResetTimeInfo  = 2,  // set all TimeInfo attributes to the current time
+        CloneIncludeEntries = 4,  // clone the group entries
+    };
+    Q_DECLARE_FLAGS(CloneFlags, CloneFlag)
+
     struct GroupData
     {
         QString name;
@@ -80,6 +88,7 @@ public:
     static const int RecycleBinIconNumber;
 
     Group* findChildByName(const QString& name);
+    Group* findChildByUuid(const Uuid& uuid);
     Entry* findEntry(QString entryId);
     Entry* findEntryByUuid(const Uuid& uuid);
     Entry* findEntryByPath(QString entryPath, QString basePath = QString(""));
@@ -126,7 +135,8 @@ public:
      * new group into another database.
      */
     Group* clone(Entry::CloneFlags entryFlags = Entry::CloneNewUuid | Entry::CloneResetTimeInfo,
-                 bool shallow = false) const;
+                 CloneFlags groupFlags = static_cast<CloneFlags>(Group::CloneNewUuid | Group::CloneResetTimeInfo |
+                                                                 Group::CloneIncludeEntries)) const;
     void copyDataFrom(const Group* other);
     void merge(const Group* other);
     QString print(bool recursive = false, int depth = 0);
@@ -160,7 +170,8 @@ private:
     void removeEntry(Entry* entry);
     void setParent(Database* db);
     void markOlderEntry(Entry* entry);
-    void resolveConflict(Entry* existingEntry, Entry* otherEntry);
+    void resolveEntryConflict(Entry* existingEntry, Entry* otherEntry);
+    void resolveGroupConflict(Group* existingGroup, Group* otherGroup);
 
     void recSetDatabase(Database* db);
     void cleanupParent();
@@ -183,4 +194,6 @@ private:
     friend void Entry::setGroup(Group* group);
 };
 
+Q_DECLARE_OPERATORS_FOR_FLAGS(Group::CloneFlags)
+
 #endif // KEEPASSX_GROUP_H

tests/TestMerge.cpp

@@ -64,7 +64,7 @@ void TestMerge::testMergeNoChanges()
     Database* dbDestination = createTestDatabase();
 
     Database* dbSource = new Database();
-    dbSource->setRootGroup(dbDestination->rootGroup()->clone(Entry::CloneNoFlags));
+    dbSource->setRootGroup(dbDestination->rootGroup()->clone(Entry::CloneNoFlags, Group::CloneIncludeEntries));
 
     QCOMPARE(dbDestination->rootGroup()->entriesRecursive().size(), 2);
     QCOMPARE(dbSource->rootGroup()->entriesRecursive().size(), 2);
@@ -92,7 +92,7 @@ void TestMerge::testResolveConflictNewer()
     Database* dbDestination = createTestDatabase();
 
     Database* dbSource = new Database();
-    dbSource->setRootGroup(dbDestination->rootGroup()->clone(Entry::CloneNoFlags));
+    dbSource->setRootGroup(dbDestination->rootGroup()->clone(Entry::CloneNoFlags, Group::CloneIncludeEntries));
 
     // sanity check
     Group* group1 = dbSource->rootGroup()->findChildByName("group1");
@@ -141,7 +141,7 @@ void TestMerge::testResolveConflictOlder()
     Database* dbDestination = createTestDatabase();
 
     Database* dbSource = new Database();
-    dbSource->setRootGroup(dbDestination->rootGroup()->clone(Entry::CloneNoFlags));
+    dbSource->setRootGroup(dbDestination->rootGroup()->clone(Entry::CloneNoFlags, Group::CloneIncludeEntries));
 
     // sanity check
     Group* group1 = dbSource->rootGroup()->findChildByName("group1");
@@ -197,7 +197,7 @@ void TestMerge::testResolveConflictKeepBoth()
     Database* dbDestination = createTestDatabase();
 
     Database* dbSource = new Database();
-    dbSource->setRootGroup(dbDestination->rootGroup()->clone(Entry::CloneIncludeHistory));
+    dbSource->setRootGroup(dbDestination->rootGroup()->clone(Entry::CloneIncludeHistory, Group::CloneIncludeEntries));
 
     // sanity check
     QCOMPARE(dbDestination->rootGroup()->children().at(0)->entries().size(), 2);
@@ -236,7 +236,7 @@ void TestMerge::testMoveEntry()
     Database* dbDestination = createTestDatabase();
 
     Database* dbSource = new Database();
-    dbSource->setRootGroup(dbDestination->rootGroup()->clone(Entry::CloneNoFlags));
+    dbSource->setRootGroup(dbDestination->rootGroup()->clone(Entry::CloneNoFlags, Group::CloneIncludeEntries));
 
     Entry* entry1 = dbSource->rootGroup()->findEntry("entry1");
     QVERIFY(entry1 != nullptr);
@@ -270,7 +270,7 @@ void TestMerge::testMoveEntryPreserveChanges()
     Database* dbDestination = createTestDatabase();
 
     Database* dbSource = new Database();
-    dbSource->setRootGroup(dbDestination->rootGroup()->clone(Entry::CloneNoFlags));
+    dbSource->setRootGroup(dbDestination->rootGroup()->clone(Entry::CloneNoFlags, Group::CloneIncludeEntries));
 
     Entry* entry1 = dbSource->rootGroup()->findEntry("entry1");
     QVERIFY(entry1 != nullptr);
@@ -307,11 +307,12 @@ void TestMerge::testCreateNewGroups()
     Database* dbDestination = createTestDatabase();
 
     Database* dbSource = new Database();
-    dbSource->setRootGroup(dbDestination->rootGroup()->clone(Entry::CloneNoFlags));
+    dbSource->setRootGroup(dbDestination->rootGroup()->clone(Entry::CloneNoFlags, Group::CloneIncludeEntries));
 
     QTest::qSleep(1);
     Group* group3 = new Group();
     group3->setName("group3");
+    group3->setUuid(Uuid::random());
     group3->setParent(dbSource->rootGroup());
 
     dbDestination->merge(dbSource);
@@ -329,11 +330,12 @@ void TestMerge::testMoveEntryIntoNewGroup()
     Database* dbDestination = createTestDatabase();
 
     Database* dbSource = new Database();
-    dbSource->setRootGroup(dbDestination->rootGroup()->clone(Entry::CloneNoFlags));
+    dbSource->setRootGroup(dbDestination->rootGroup()->clone(Entry::CloneNoFlags, Group::CloneIncludeEntries));
 
     QTest::qSleep(1);
     Group* group3 = new Group();
     group3->setName("group3");
+    group3->setUuid(Uuid::random());
     group3->setParent(dbSource->rootGroup());
 
     Entry* entry1 = dbSource->rootGroup()->findEntry("entry1");
@@ -365,10 +367,11 @@ void TestMerge::testUpdateEntryDifferentLocation()
     Database* dbDestination = createTestDatabase();
 
     Database* dbSource = new Database();
-    dbSource->setRootGroup(dbDestination->rootGroup()->clone(Entry::CloneNoFlags));
+    dbSource->setRootGroup(dbDestination->rootGroup()->clone(Entry::CloneNoFlags, Group::CloneIncludeEntries));
 
     Group* group3 = new Group();
     group3->setName("group3");
+    group3->setUuid(Uuid::random());
     group3->setParent(dbDestination->rootGroup());
 
     Entry* entry1 = dbDestination->rootGroup()->findEntry("entry1");
@@ -399,6 +402,84 @@ void TestMerge::testUpdateEntryDifferentLocation()
     delete dbSource;
 }
 
+/**
+ * Groups should be updated using the uuids.
+ */
+void TestMerge::testUpdateGroup()
+{
+    Database* dbDestination = createTestDatabase();
+
+    Database* dbSource = new Database();
+    dbSource->setRootGroup(dbDestination->rootGroup()->clone(Entry::CloneNoFlags, Group::CloneIncludeEntries));
+
+    QTest::qSleep(1);
+
+    Group* group2 = dbSource->rootGroup()->findChildByName("group2");
+    group2->setName("group2 renamed");
+    group2->setNotes("updated notes");
+    Uuid customIconId = Uuid::random();
+    QImage customIcon;
+    dbSource->metadata()->addCustomIcon(customIconId, customIcon);
+    group2->setIcon(customIconId);
+
+    Entry* entry1 = dbSource->rootGroup()->findEntry("entry1");
+    QVERIFY(entry1 != nullptr);
+    entry1->setGroup(group2);
+    entry1->setTitle("entry1 renamed");
+    Uuid uuidBeforeSyncing = entry1->uuid();
+
+    dbDestination->merge(dbSource);
+
+    QCOMPARE(dbDestination->rootGroup()->entriesRecursive().size(), 2);
+
+    entry1 = dbDestination->rootGroup()->findEntry("entry1 renamed");
+    QVERIFY(entry1 != nullptr);
+    QVERIFY(entry1->group() != nullptr);
+    QCOMPARE(entry1->group()->name(), QString("group2 renamed"));
+    QCOMPARE(uuidBeforeSyncing, entry1->uuid());
+
+    group2 = dbDestination->rootGroup()->findChildByName("group2 renamed");
+    QCOMPARE(group2->notes(), QString("updated notes"));
+    QCOMPARE(group2->iconUuid(), customIconId);
+
+    delete dbDestination;
+    delete dbSource;
+}
+
+void TestMerge::testUpdateGroupLocation()
+{
+    Database* dbDestination = createTestDatabase();
+    Group* group3 = new Group();
+    Uuid group3Uuid = Uuid::random();
+    group3->setUuid(group3Uuid);
+    group3->setName("group3");
+    group3->setParent(dbDestination->rootGroup()->findChildByName("group1"));
+
+    Database* dbSource = new Database();
+    dbSource->setRootGroup(dbDestination->rootGroup()->clone(Entry::CloneNoFlags, Group::CloneIncludeEntries));
+
+    // Sanity check
+    group3 = dbSource->rootGroup()->findChildByUuid(group3Uuid);
+    QVERIFY(group3 != nullptr);
+
+    QTest::qSleep(1);
+
+    group3->setParent(dbSource->rootGroup()->findChildByName("group2"));
+
+    dbDestination->merge(dbSource);
+    group3 = dbDestination->rootGroup()->findChildByUuid(group3Uuid);
+    QVERIFY(group3 != nullptr);
+    QCOMPARE(group3->parent(), dbDestination->rootGroup()->findChildByName("group2"));
+
+    dbDestination->merge(dbSource);
+    group3 = dbDestination->rootGroup()->findChildByUuid(group3Uuid);
+    QVERIFY(group3 != nullptr);
+    QCOMPARE(group3->parent(), dbDestination->rootGroup()->findChildByName("group2"));
+
+    delete dbDestination;
+    delete dbSource;
+}
+
 /**
  * The first merge should create new entries, the
  * second should only sync them, since they have
@@ -447,14 +528,54 @@ void TestMerge::testMergeCustomIcons()
     delete dbSource;
 }
 
+/**
+ * If the group is updated in the source database, and the
+ * destination database after, the group should remain the
+ * same.
+ */
+void TestMerge::testResolveGroupConflictOlder()
+{
+    Database* dbDestination = createTestDatabase();
+
+    Database* dbSource = new Database();
+    dbSource->setRootGroup(dbDestination->rootGroup()->clone(Entry::CloneNoFlags, Group::CloneIncludeEntries));
+
+    // sanity check
+    Group* group1 = dbSource->rootGroup()->findChildByName("group1");
+    QVERIFY(group1 != nullptr);
+
+    // Make sure the two changes have a different timestamp.
+    QTest::qSleep(1);
+    group1->setName("group1 updated in source");
+
+    // Make sure the two changes have a different timestamp.
+    QTest::qSleep(1);
+
+    group1 = dbDestination->rootGroup()->findChildByName("group1");
+    group1->setName("group1 updated in destination");
+
+    dbDestination->merge(dbSource);
+
+    // sanity check
+    group1 = dbDestination->rootGroup()->findChildByName("group1 updated in destination");
+    QVERIFY(group1 != nullptr);
+
+    delete dbDestination;
+    delete dbSource;
+}
+
+
 Database* TestMerge::createTestDatabase()
 {
     Database* db = new Database();
 
     Group* group1 = new Group();
     group1->setName("group1");
+    group1->setUuid(Uuid::random());
+
     Group* group2 = new Group();
     group2->setName("group2");
+    group2->setUuid(Uuid::random());
 
     Entry* entry1 = new Entry();
     Entry* entry2 = new Entry();

tests/TestMerge.h

@@ -31,12 +31,15 @@ private slots:
     void testMergeNoChanges();
     void testResolveConflictNewer();
     void testResolveConflictOlder();
+    void testResolveGroupConflictOlder();
     void testResolveConflictKeepBoth();
     void testMoveEntry();
     void testMoveEntryPreserveChanges();
     void testMoveEntryIntoNewGroup();
     void testCreateNewGroups();
     void testUpdateEntryDifferentLocation();
+    void testUpdateGroup();
+    void testUpdateGroupLocation();
     void testMergeAndSync();
     void testMergeCustomIcons();