RCORE-2015 Merge rules for Clear instruction (#7597)

* Merge rules for Clear instructions using the collection type

* Unit tests

* Changes after code review

* More changes after code review

Author: danieltabacaru

src/realm/sync/changeset_parser.cpp

@@ -487,7 +487,6 @@ void State::parse_one()
             Instruction::Clear instr;
             read_path_instr(instr);
             instr.collection_type = read_collection_type();
-            // TODO: Add validation for collection_type once BAAS-29262 is done
             m_handler(instr);
             return;
         }

src/realm/sync/instruction_applier.cpp

@@ -877,87 +877,121 @@ void InstructionApplier::operator()(const Instruction::ArrayErase& instr)
 
 void InstructionApplier::operator()(const Instruction::Clear& instr)
 {
+    // For collections and nested collections in Mixed, applying a Clear instruction
+    // implicitly sets the collection type (of the clear instruction) too.
     struct ClearResolver : public PathResolver {
         ClearResolver(InstructionApplier* applier, const Instruction::Clear& instr)
             : PathResolver(applier, instr, "Clear")
         {
+            switch (instr.collection_type) {
+                case Instruction::CollectionType::Single:
+                    break;
+                case Instruction::CollectionType::List:
+                    m_collection_type = CollectionType::List;
+                    break;
+                case Instruction::CollectionType::Dictionary:
+                    m_collection_type = CollectionType::Dictionary;
+                    break;
+                case Instruction::CollectionType::Set:
+                    m_collection_type = CollectionType::Set;
+                    break;
+            }
         }
         void on_list(LstBase& list) override
         {
+            // list property
+            if (m_collection_type && *m_collection_type != CollectionType::List) {
+                m_applier->bad_transaction_log("Clear: Not a List");
+            }
             list.clear();
         }
         Status on_list_index(LstBase& list, uint32_t index) override
         {
+            REALM_ASSERT(m_collection_type);
             REALM_ASSERT(dynamic_cast<Lst<Mixed>*>(&list));
             auto& mixed_list = static_cast<Lst<Mixed>&>(list);
             if (index >= mixed_list.size()) {
                 m_applier->bad_transaction_log("Clear: Index out of bounds (%1 > %2)", index,
                                                mixed_list.size()); // Throws
-                return Status::DidNotResolve;
             }
             auto val = mixed_list.get(index);
             if (val.is_type(type_Dictionary)) {
                 Dictionary d(mixed_list, mixed_list.get_key(index));
                 d.clear();
-                return Status::Pending;
             }
-            if (val.is_type(type_List)) {
+            else if (val.is_type(type_List)) {
                 Lst<Mixed> l(mixed_list, mixed_list.get_key(index));
                 l.clear();
-                return Status::Pending;
             }
-            m_applier->bad_transaction_log("Clear: Item (%1) at index %2 is not a collection", val.get_type(),
-                                           index); // Throws
-            return Status::DidNotResolve;
+            else if (val.is_type(type_Set)) {
+                m_applier->bad_transaction_log("Clear: Item at index %1 is a Set",
+                                               index); // Throws
+            }
+            mixed_list.set_collection(size_t(index), *m_collection_type);
+            return Status::Pending;
         }
         void on_dictionary(Dictionary& dict) override
         {
+            // dictionary property
+            if (m_collection_type && *m_collection_type != CollectionType::Dictionary) {
+                m_applier->bad_transaction_log("Clear: Not a Dictionary");
+            }
             dict.clear();
         }
         Status on_dictionary_key(Dictionary& dict, Mixed key) override
         {
+            REALM_ASSERT(m_collection_type);
             auto val = dict.get(key);
             if (val.is_type(type_Dictionary)) {
                 Dictionary d(dict, dict.build_index(key));
                 d.clear();
-                return Status::Pending;
             }
-            if (val.is_type(type_List)) {
+            else if (val.is_type(type_List)) {
                 Lst<Mixed> l(dict, dict.build_index(key));
                 l.clear();
-                return Status::Pending;
             }
-            m_applier->bad_transaction_log("Clear: Item (%1) at key '%2' is not a collection", val.get_type(),
-                                           key); // Throws
-            return Status::DidNotResolve;
+            else if (val.is_type(type_Set)) {
+                m_applier->bad_transaction_log("Clear: Item at key '%1' is a Set",
+                                               key); // Throws
+            }
+            dict.insert_collection(key.get_string(), *m_collection_type);
+            return Status::Pending;
         }
         void on_set(SetBase& set) override
         {
+            // set property
+            if (m_collection_type && *m_collection_type != CollectionType::Set) {
+                m_applier->bad_transaction_log("Clear: Not a Set");
+            }
             set.clear();
         }
         void on_property(Obj& obj, ColKey col_key) override
         {
             if (col_key.get_type() == col_type_Mixed) {
+                REALM_ASSERT(m_collection_type);
                 auto val = obj.get<Mixed>(col_key);
                 if (val.is_type(type_Dictionary)) {
                     Dictionary dict(obj, col_key);
                     dict.clear();
-                    return;
                 }
                 else if (val.is_type(type_List)) {
                     Lst<Mixed> list(obj, col_key);
                     list.clear();
-                    return;
                 }
                 else if (val.is_type(type_Set)) {
-                    Set<Mixed> set(obj, col_key);
-                    set.clear();
-                    return;
+                    m_applier->bad_transaction_log("Clear: Mixed property is a Set"); // Throws
                 }
+                obj.set_collection(col_key, *m_collection_type);
+                return;
             }
 
             PathResolver::on_property(obj, col_key);
         }
+
+    private:
+        // The server may not send the type for collection properties (non-Mixed)
+        // since the clients don't send it either before v13.
+        std::optional<CollectionType> m_collection_type;
     };
     ClearResolver(this, instr).resolve();
 }

src/realm/sync/transform.cpp

@@ -1466,7 +1466,7 @@ DEFINE_MERGE_NOOP(Instruction::SetInsert, Instruction::EraseObject);
 DEFINE_MERGE_NOOP(Instruction::SetErase, Instruction::EraseObject);
 
 
-/// Set rules
+/// Update rules
 
 DEFINE_NESTED_MERGE(Instruction::Update)
 {
@@ -1695,14 +1695,33 @@ DEFINE_MERGE(Instruction::ArrayErase, Instruction::Update)
 DEFINE_MERGE(Instruction::Clear, Instruction::Update)
 {
     using Type = Instruction::Payload::Type;
+    using CollectionType = Instruction::CollectionType;
 
     // The two instructions are at the same level of nesting.
     if (same_path(left, right)) {
-        // TODO: We could make it so a Clear instruction does not win against setting a property or
-        // collection item to a different collection.
-        if (right.value.type != Type::List && right.value.type != Type::Dictionary) {
+        REALM_ASSERT(right.value.type != Type::Set);
+        // If both sides are setting/operating on the same type, let them both pass through.
+        // It is important that the instruction application rules reflect this.
+        // If it is not two lists or dictionaries, then the normal last-writer-wins rules will take effect below.
+        if (left.collection_type == CollectionType::List && right.value.type == Type::List) {
+            return;
+        }
+        if (left.collection_type == CollectionType::Dictionary && right.value.type == Type::Dictionary) {
+            return;
+        }
+
+        // CONFLICT: Clear and Update of the same element.
+        //
+        // RESOLUTION: Discard the instruction with the lower timestamp. This has the
+        // effect of preserving insertions that came after the clear (if it has the
+        // higher timestamp), or preserve additional updates (and potential insertions)
+        // that came after the update.
+        if (left_side.timestamp() < right_side.timestamp()) {
             left_side.discard();
         }
+        else {
+            right_side.discard();
+        }
     }
 }
 
@@ -1720,13 +1739,20 @@ DEFINE_NESTED_MERGE(Instruction::AddInteger)
     }
 }
 
+DEFINE_MERGE(Instruction::Clear, Instruction::AddInteger)
+{
+    // The two instructions are at the same level of nesting.
+    if (same_path(left, right)) {
+        right_side.discard();
+    }
+}
+
 DEFINE_MERGE_NOOP(Instruction::AddInteger, Instruction::AddInteger);
 DEFINE_MERGE_NOOP(Instruction::AddColumn, Instruction::AddInteger);
 DEFINE_MERGE_NOOP(Instruction::EraseColumn, Instruction::AddInteger);
 DEFINE_MERGE_NOOP(Instruction::ArrayInsert, Instruction::AddInteger);
 DEFINE_MERGE_NOOP(Instruction::ArrayMove, Instruction::AddInteger);
 DEFINE_MERGE_NOOP(Instruction::ArrayErase, Instruction::AddInteger);
-DEFINE_MERGE_NOOP(Instruction::Clear, Instruction::AddInteger);
 DEFINE_MERGE_NOOP(Instruction::SetInsert, Instruction::AddInteger);
 DEFINE_MERGE_NOOP(Instruction::SetErase, Instruction::AddInteger);