🐛 merge custom data only when necessary

keepassxreboot · Sep 14, 2019 · 09b5059 · 09b5059
1 parent 490ef29
commit 09b5059
Show file tree

Hide file tree

Showing 3 changed files with 33 additions and 12 deletions.
diff --git a/src/core/Merger.cpp b/src/core/Merger.cpp
@@ -622,11 +622,11 @@ Merger::ChangeList Merger::mergeMetadata(const MergeContext& context)
     }
 
     // Merge Custom Data if source is newer
-    const auto targetCustomDataModificationTime = sourceMetadata->customData()->getLastModified();
-    const auto sourceCustomDataModificationTime = targetMetadata->customData()->getLastModified();
+    const auto targetCustomDataModificationTime = targetMetadata->customData()->getLastModified();
+    const auto sourceCustomDataModificationTime = sourceMetadata->customData()->getLastModified();
     if (!targetMetadata->customData()->contains(CustomData::LastModified)
         || (targetCustomDataModificationTime.isValid() && sourceCustomDataModificationTime.isValid()
-            && targetCustomDataModificationTime > sourceCustomDataModificationTime)) {
+            && targetCustomDataModificationTime < sourceCustomDataModificationTime)) {
         const auto sourceCustomDataKeys = sourceMetadata->customData()->keys();
         const auto targetCustomDataKeys = targetMetadata->customData()->keys();
 
@@ -641,9 +641,18 @@ Merger::ChangeList Merger::mergeMetadata(const MergeContext& context)
 
         // Transfer new/existing keys
         for (const auto& key : sourceCustomDataKeys) {
-            auto value = sourceMetadata->customData()->value(key);
-            targetMetadata->customData()->set(key, value);
-            changes << tr("Adding custom data %1 [%2]").arg(key, value);
+            // Don't merge this meta field, it is updated automatically.
+            if (key == CustomData::LastModified) {
+                continue;
+            }
+
+            auto sourceValue = sourceMetadata->customData()->value(key);
+            auto targetValue = targetMetadata->customData()->value(key);
+            // Merge only if the values are not the same.
+            if (sourceValue != targetValue) {
+                targetMetadata->customData()->set(key, sourceValue);
+                changes << tr("Adding custom data %1 [%2]").arg(key, sourceValue);
+            }
         }
     }
 

diff --git a/tests/TestMerge.cpp b/tests/TestMerge.cpp
@@ -1159,12 +1159,12 @@ void TestMerge::testMetadata()
 {
     QSKIP("Sophisticated merging for Metadata not implemented");
     // TODO HNH: I think a merge of recycle bins would be nice since duplicating them
-    //           is not realy a good solution - the one to use as final recycle bin
+    //           is not really a good solution - the one to use as final recycle bin
     //           is determined by the merge method - if only one has a bin, this one
     //           will be used - exception is the target has no recycle bin activated
 }
 
-void TestMerge::testCustomdata()
+void TestMerge::testCustomData()
 {
     QScopedPointer<Database> dbDestination(new Database());
     QScopedPointer<Database> dbSource(createTestDatabase());
@@ -1198,10 +1198,9 @@ void TestMerge::testCustomdata()
     m_clock->advanceSecond(1);
 
     Merger merger(dbSource.data(), dbDestination.data());
-    merger.merge();
+    QStringList changes = merger.merge();
 
-    Merger merger2(dbSource2.data(), dbDestination2.data());
-    merger2.merge();
+    QVERIFY(!changes.isEmpty());
 
     // Source is newer, data should be merged
     QVERIFY(!dbDestination->metadata()->customData()->isEmpty());
@@ -1215,6 +1214,19 @@ void TestMerge::testCustomdata()
     QCOMPARE(dbDestination->metadata()->customData()->value("key3"),
              QString("newValue")); // Old value should be replaced
 
+
+    // Merging again should not do anything if the values are the same.
+    m_clock->advanceSecond(1);
+    dbSource->metadata()->customData()->set("key3", "oldValue");
+    dbSource->metadata()->customData()->set("key3", "newValue");
+    Merger merger2(dbSource.data(), dbDestination.data());
+    QStringList changes2 = merger2.merge();
+    QVERIFY(changes2.isEmpty());
+
+
+    Merger merger3(dbSource2.data(), dbDestination2.data());
+    merger3.merge();
+
     // Target is newer, no data is merged
     QVERIFY(!dbDestination2->metadata()->customData()->isEmpty());
     QVERIFY(!dbDestination2->metadata()->customData()->contains("key1"));

diff --git a/tests/TestMerge.h b/tests/TestMerge.h
@@ -59,7 +59,7 @@ private slots:
     void testMergeCustomIcons();
     void testMergeDuplicateCustomIcons();
     void testMetadata();
-    void testCustomdata();
+    void testCustomData();
     void testDeletedEntry();
     void testDeletedGroup();
     void testDeletedRevertedEntry();