Fix GetCommitIds local deletion case resulting in orphaning of directory entries

sync/engine/syncer_unittest.cc

 // Copyright 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 //
 // Syncer unit tests. Unfortunately a lot of these tests
 // are outdated and need to be reworked and updated.
 
 #include <stddef.h>
 #include <stdint.h>
 
@@ skipping 44 matching lines @@
 #include "sync/test/engine/mock_nudge_handler.h"
 #include "sync/test/engine/test_directory_setter_upper.h"
 #include "sync/test/engine/test_id_factory.h"
 #include "sync/test/engine/test_syncable_utils.h"
 #include "sync/test/fake_encryptor.h"
 #include "sync/test/fake_sync_encryption_handler.h"
 #include "sync/test/sessions/mock_debug_info_getter.h"
 #include "sync/util/cryptographer.h"
 #include "sync/util/extensions_activity.h"
 #include "sync/util/time.h"
+#include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 using base::TimeDelta;
 
 using std::count;
 using std::map;
 using std::multimap;
 using std::set;
 using std::string;
 using std::vector;
@@ skipping 2727 matching lines @@
     existing.PutIsUnsynced(true);
     EXPECT_TRUE(existing.GetId().ServerKnows());
 
     newfolder.PutIsDel(true);
     existing.PutIsDel(true);
   }
   EXPECT_TRUE(SyncShareNudge());
   EXPECT_EQ(0, GetCommitCounters(BOOKMARKS).num_commits_conflict);
 }
 
+// Test conflict resolution when deleting a hierarchy of nodes within a folder
+// and running into a conflict in one of subfolders.
+TEST_F(SyncerTest, DeletingFolderWithConflictInSubfolder) {
+  int64_t top_handle, nested_handle, leaf_handle;
+  {
+    WriteTransaction trans(FROM_HERE, UNITTEST, directory());
+    MutableEntry top_entry(&trans, CREATE, BOOKMARKS, trans.root_id(), "top");
+    ASSERT_TRUE(top_entry.good());
+    top_entry.PutIsDir(true);
+    top_entry.PutSpecifics(DefaultBookmarkSpecifics());
+    top_entry.PutIsUnsynced(true);
+    top_handle = top_entry.GetMetahandle();
+
+    MutableEntry nested_entry(&trans, CREATE, BOOKMARKS, top_entry.GetId(),
+                              "nested");
+    ASSERT_TRUE(nested_entry.good());
+    nested_entry.PutIsDir(true);
+    nested_entry.PutSpecifics(DefaultBookmarkSpecifics());
+    nested_entry.PutIsUnsynced(true);
+    nested_handle = nested_entry.GetMetahandle();
+
+    MutableEntry leaf_entry(&trans, CREATE, BOOKMARKS, nested_entry.GetId(),
+                            "leaf");
+    ASSERT_TRUE(leaf_entry.good());
+    leaf_entry.PutSpecifics(DefaultBookmarkSpecifics());
+    leaf_entry.PutIsUnsynced(true);
+    leaf_handle = leaf_entry.GetMetahandle();
+  }
+  EXPECT_TRUE(SyncShareNudge());
+
+  // Delete all 3 entries and also add unapplied update to the middle one.
+  {
+    WriteTransaction trans(FROM_HERE, UNITTEST, directory());
+    MutableEntry leaf_entry(&trans, GET_BY_HANDLE, leaf_handle);
+    ASSERT_TRUE(leaf_entry.good());
+    EXPECT_TRUE(leaf_entry.GetId().ServerKnows());
+    leaf_entry.PutIsUnsynced(true);
+    leaf_entry.PutIsDel(true);
+
+    MutableEntry nested_entry(&trans, GET_BY_HANDLE, nested_handle);
+    ASSERT_TRUE(nested_entry.good());
+    EXPECT_TRUE(nested_entry.GetId().ServerKnows());
+    nested_entry.PutIsUnsynced(true);
+    nested_entry.PutIsDel(true);
+
+    sync_pb::EntitySpecifics specifics;
+    specifics.mutable_bookmark()->set_url("http://demo/");
+    specifics.mutable_bookmark()->set_favicon("PNG");
+    nested_entry.PutServerSpecifics(specifics);
+    // This will put the entry into conflict.
+    nested_entry.PutIsUnappliedUpdate(true);
+    nested_entry.PutServerVersion(nested_entry.GetBaseVersion() + 1);
+
+    MutableEntry top_entry(&trans, GET_BY_HANDLE, top_handle);
+    ASSERT_TRUE(top_entry.good());
+    EXPECT_TRUE(top_entry.GetId().ServerKnows());
+    top_entry.PutIsUnsynced(true);
+    top_entry.PutIsDel(true);
+  }
+  EXPECT_TRUE(SyncShareNudge());
+
+  // Verify that the top folder hasn't been committed. Doing so would
+  // orphan the nested folder.

[Nicolas Zea, 2015/12/31 02:12:03]

Is there any test that verifies that children of conflicting folders aren't
committed?

[stanisc, 2016/01/04 22:00:48]

I think there are some tests that indirectly verify that already because I
remember running into the commit IDs filtering code before. But since it wasn't
clear which tests verify that I decided it would be a good idea to add a new one
- CommittingItemsWithConflictInParentFolder.

+  syncable::Id top_id;
+  {
+    WriteTransaction trans(FROM_HERE, UNITTEST, directory());
+    MutableEntry top_entry(&trans, GET_BY_HANDLE, top_handle);
+    ASSERT_TRUE(top_entry.good());
+    top_id = top_entry.GetId();
+
+    EXPECT_TRUE(top_entry.GetIsUnsynced());
+    EXPECT_TRUE(top_entry.GetIsDel());
+  }
+
+  EXPECT_THAT(mock_server_->committed_ids(),
+              testing::Not(testing::Contains(top_id)));
+}
+
 // Test conflict resolution when handling an update for an item with specified
 // Parent ID and having an implicit (unset) Parent ID in the update.
 TEST_F(SyncerTest, ConflictWithImplicitParent) {
   // Make sure PREFERENCES root exists so that we can get its parent ID.
   mock_server_->AddUpdateSpecifics(1, 0, "Folder", 10, 10, true, 1,
                                    DefaultPreferencesSpecifics());
   mock_server_->SetLastUpdateServerTag(ModelTypeToRootTag(PREFERENCES));
   EXPECT_TRUE(SyncShareNudge());
 
   Id pref_root_id;
@@ skipping 2826 matching lines @@
     EXPECT_EQ("xyz", final_monitor_records["xyz"].extension_id);
     EXPECT_EQ(2049U, final_monitor_records["ABC"].bookmark_write_count);
     EXPECT_EQ(4U,    final_monitor_records["xyz"].bookmark_write_count);
   } else {
     EXPECT_TRUE(final_monitor_records.empty())
         << "Should not restore records after successful bookmark commit.";
   }
 }
 
 }  // namespace syncer