RCORE-2015 Add collection type to Clear instruction (#7590)

danieltabacaru · jedelbo · web-flow · commit 2e6477c595a2 · 2024-04-16T17:32:26.000+03:00
* Add collection type to Clear instruction

* Bump sync protocol version for collections in mixed

* Changes after code review

---------

Co-authored-by: Jørgen Edelbo &lt;jorgen.edelbo@mongodb.com&gt;
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -7,6 +7,11 @@
 ### Compatibility
 * Fileformat: Generates files with format v24. Reads and automatically upgrade from fileformat v10. If you want to upgrade from an earlier file format version you will have to use RealmCore v13.x.y or earlier.
 
+-----------
+
+### Internals
+* `prior_size` field in Clear instruction is being repurposed as `collection_type` (no protocol changes required)
+
 ----------------------------------------------
 
 # 14.5.1 Release notes
diff --git a/src/realm/sync/changeset.cpp b/src/realm/sync/changeset.cpp
@@ -431,7 +431,7 @@ void Changeset::Reflector::operator()(const Instruction::AddColumn& p) const
     if (p.type == Instruction::Payload::Type::Link) {
         m_tracer.field("target_table", p.link_target_table);
     }
-    if (p.collection_type == Instruction::AddColumn::CollectionType::Dictionary) {
+    if (p.collection_type == Instruction::CollectionType::Dictionary) {
         m_tracer.field("key_type", p.key_type);
     }
 }
@@ -505,7 +505,7 @@ void Changeset::Printer::field(StringData n, Instruction::Payload::Type type)
     print_field(n, get_type_name(type));
 }
 
-void Changeset::Printer::field(StringData n, Instruction::AddColumn::CollectionType type)
+void Changeset::Printer::field(StringData n, Instruction::CollectionType type)
 {
     print_field(n, get_collection_type(type));
 }
diff --git a/src/realm/sync/changeset.hpp b/src/realm/sync/changeset.hpp
@@ -348,7 +348,7 @@ struct Changeset::Reflector {
                           util::Optional<InternString> field, const Instruction::Path* path) = 0;
         virtual void field(StringData, InternString) = 0;
         virtual void field(StringData, Instruction::Payload::Type) = 0;
-        virtual void field(StringData, Instruction::AddColumn::CollectionType) = 0;
+        virtual void field(StringData, Instruction::CollectionType) = 0;
         virtual void field(StringData, const Instruction::PrimaryKey&) = 0;
         virtual void field(StringData, const Instruction::Payload&) = 0;
         virtual void field(StringData, const Instruction::Path&) = 0;
@@ -393,7 +393,7 @@ struct Changeset::Printer : Changeset::Reflector::Tracer {
               const Instruction::Path* path) final;
     void field(StringData, InternString) final;
     void field(StringData, Instruction::Payload::Type) final;
-    void field(StringData, Instruction::AddColumn::CollectionType) final;
+    void field(StringData, Instruction::CollectionType) final;
     void field(StringData, const Instruction::PrimaryKey&) final;
     void field(StringData, const Instruction::Payload&) final;
     void field(StringData, const Instruction::Path&) final;
diff --git a/src/realm/sync/changeset_encoder.cpp b/src/realm/sync/changeset_encoder.cpp
@@ -121,7 +121,7 @@ void ChangesetEncoder::append_value(Instruction::Payload::Type type)
     append_value(int64_t(type));
 }
 
-void ChangesetEncoder::append_value(Instruction::AddColumn::CollectionType type)
+void ChangesetEncoder::append_value(Instruction::CollectionType type)
 {
     append_value(uint8_t(type));
 }
@@ -190,7 +190,7 @@ void ChangesetEncoder::operator()(const Instruction::AddInteger& instr)
 
 void ChangesetEncoder::operator()(const Instruction::AddColumn& instr)
 {
-    bool is_dictionary = (instr.collection_type == Instruction::AddColumn::CollectionType::Dictionary);
+    bool is_dictionary = (instr.collection_type == Instruction::CollectionType::Dictionary);
     // Mixed columns are always nullable.
     REALM_ASSERT(instr.type != Instruction::Payload::Type::Null || instr.nullable || is_dictionary);
     append(Instruction::Type::AddColumn, instr.table, instr.field, instr.type, instr.nullable, instr.collection_type);
@@ -225,8 +225,7 @@ void ChangesetEncoder::operator()(const Instruction::ArrayErase& instr)
 
 void ChangesetEncoder::operator()(const Instruction::Clear& instr)
 {
-    uint32_t prior_size = 0; // Ignored
-    append_path_instr(Instruction::Type::Clear, instr, prior_size);
+    append_path_instr(Instruction::Type::Clear, instr, instr.collection_type);
 }
 
 void ChangesetEncoder::operator()(const Instruction::SetInsert& instr)
diff --git a/src/realm/sync/changeset_encoder.hpp b/src/realm/sync/changeset_encoder.hpp
@@ -49,7 +49,7 @@ struct ChangesetEncoder {
     void append_value(const Instruction::Payload::Link&);
     void append_value(Instruction::Payload::Type);
     void append_value(util::Optional<Instruction::Payload::Type>);
-    void append_value(Instruction::AddColumn::CollectionType);
+    void append_value(Instruction::CollectionType);
     void append_value(const Instruction::Path&);
     void append_value(DataType);
     void append_value(bool);
diff --git a/src/realm/sync/changeset_parser.cpp b/src/realm/sync/changeset_parser.cpp
@@ -54,7 +54,7 @@ struct State {
 
     util::Optional<Instruction::Payload::Type> read_optional_payload_type();
     Instruction::Payload::Type read_payload_type();
-    Instruction::AddColumn::CollectionType read_collection_type();
+    Instruction::CollectionType read_collection_type();
     Instruction::Payload read_payload();
     Instruction::Payload::Link read_link();
     Instruction::PrimaryKey read_object_key();
@@ -179,10 +179,10 @@ Instruction::Payload::Type State::read_payload_type()
     parser_error("Unsupported data type");
 }
 
-Instruction::AddColumn::CollectionType State::read_collection_type()
+Instruction::CollectionType State::read_collection_type()
 {
-    using CollectionType = Instruction::AddColumn::CollectionType;
-    auto type = Instruction::AddColumn::CollectionType(read_int<uint8_t>());
+    using CollectionType = Instruction::CollectionType;
+    auto type = Instruction::CollectionType(read_int<uint8_t>());
     // Validate the type.
     switch (type) {
         case CollectionType::Single:
@@ -435,7 +435,7 @@ void State::parse_one()
             if (instr.type == Instruction::Payload::Type::Link) {
                 instr.link_target_table = read_intern_string();
             }
-            if (instr.collection_type == Instruction::AddColumn::CollectionType::Dictionary) {
+            if (instr.collection_type == Instruction::CollectionType::Dictionary) {
                 instr.key_type = read_payload_type();
             }
             else {
@@ -486,8 +486,8 @@ void State::parse_one()
         case Instruction::Type::Clear: {
             Instruction::Clear instr;
             read_path_instr(instr);
-            uint32_t prior_size = read_int<uint32_t>();
-            static_cast<void>(prior_size); // Ignored
+            instr.collection_type = read_collection_type();
+            // TODO: Add validation for collection_type once BAAS-29262 is done
             m_handler(instr);
             return;
         }
diff --git a/src/realm/sync/instruction_applier.cpp b/src/realm/sync/instruction_applier.cpp
@@ -550,7 +550,7 @@ void InstructionApplier::operator()(const Instruction::AddInteger& instr)
 void InstructionApplier::operator()(const Instruction::AddColumn& instr)
 {
     using Type = Instruction::Payload::Type;
-    using CollectionType = Instruction::AddColumn::CollectionType;
+    using CollectionType = Instruction::CollectionType;
 
     // Temporarily swap out the last object key so it doesn't get included in error messages
     TemporarySwapOut<decltype(m_last_object_key)> last_object_key_guard(m_last_object_key);
diff --git a/src/realm/sync/instruction_replication.cpp b/src/realm/sync/instruction_replication.cpp
@@ -328,7 +328,7 @@ void SyncReplication::insert_column(const Table* table, ColKey col_key, DataType
                                     Table* target_table)
 {
     Replication::insert_column(table, col_key, type, name, target_table);
-    using CollectionType = Instruction::AddColumn::CollectionType;
+    using CollectionType = Instruction::CollectionType;
 
     if (select_table(*table)) {
         Instruction::AddColumn instr;
@@ -570,6 +570,7 @@ void SyncReplication::list_clear(const CollectionBase& view)
     if (select_collection(view)) {
         Instruction::Clear instr;
         populate_path_instr(instr, view);
+        instr.collection_type = Instruction::CollectionType::List;
         emit(instr);
     }
 }
@@ -605,6 +606,7 @@ void SyncReplication::set_clear(const CollectionBase& set)
     if (select_collection(set)) {
         Instruction::Clear instr;
         populate_path_instr(instr, set);
+        instr.collection_type = Instruction::CollectionType::Set;
         emit(instr);
     }
 }
@@ -663,6 +665,7 @@ void SyncReplication::dictionary_clear(const CollectionBase& dict)
     if (select_collection(dict)) {
         Instruction::Clear instr;
         populate_path_instr(instr, dict);
+        instr.collection_type = Instruction::CollectionType::Dictionary;
         emit(instr);
     }
 }
diff --git a/src/realm/sync/instructions.hpp b/src/realm/sync/instructions.hpp
@@ -415,6 +415,10 @@ struct Payload {
     }
 };
 
+// This is backwards compatible with previous boolean type where 0
+// indicated simple type and 1 indicated list.
+enum class CollectionType : uint8_t { Single, List, Dictionary, Set };
+
 /// All instructions are TableInstructions.
 struct TableInstruction {
     InternString table;
@@ -502,10 +506,6 @@ struct EraseTable : TableInstruction {
 struct AddColumn : TableInstruction {
     using TableInstruction::TableInstruction;
 
-    // This is backwards compatible with previous boolean type where 0
-    // indicated simple type and 1 indicated list.
-    enum class CollectionType : uint8_t { Single, List, Dictionary, Set };
-
     InternString field;
 
     // `Type::Null` for Mixed columns. Mixed columns are always nullable.
@@ -632,6 +632,7 @@ struct ArrayErase : PathInstruction {
 
 struct Clear : PathInstruction {
     using PathInstruction::PathInstruction;
+    CollectionType collection_type;
 
     bool operator==(const Clear& rhs) const noexcept
     {
@@ -674,6 +675,7 @@ struct Instruction {
     using Payload = instr::Payload;
     using Path = instr::Path;
     using Vector = std::vector<Instruction>;
+    using CollectionType = instr::CollectionType;
 
     // CAUTION: Any change to the enum values for the instruction types is a protocol-breaking
     // change!
@@ -828,9 +830,9 @@ inline const char* get_type_name(Instruction::Payload::Type type)
     return "(unknown)";
 }
 
-inline const char* get_collection_type(Instruction::AddColumn::CollectionType type)
+inline const char* get_collection_type(Instruction::CollectionType type)
 {
-    using Type = Instruction::AddColumn::CollectionType;
+    using Type = Instruction::CollectionType;
     switch (type) {
         case Type::Single:
             return "Single";
diff --git a/src/realm/sync/protocol.hpp b/src/realm/sync/protocol.hpp
@@ -57,14 +57,16 @@ namespace sync {
 //      Server replaces 'downloadable_bytes' (which was always zero prior this version)
 //      with an estimated progress value (double from 0.0 to 1.0) for flx sessions
 //
+//   13 Support for syncing collections (lists and dictionaries) in Mixed columns
+//
 //  XX Changes:
 //     - TBD
 //
 constexpr int get_current_protocol_version() noexcept
 {
     // Also update the current protocol version test in flx_sync.cpp when
     // updating this value
-    return 12;
+    return 13;
 }
 
 constexpr std::string_view get_pbs_websocket_protocol_prefix() noexcept
diff --git a/src/realm/sync/transform.cpp b/src/realm/sync/transform.cpp
@@ -341,7 +341,7 @@ struct MergeTracer {
             m_fields.emplace(n, get_type_name(type));
         }
 
-        void field(StringData n, Instruction::AddColumn::CollectionType type) override
+        void field(StringData n, Instruction::CollectionType type) override
         {
             m_fields.emplace(n, get_collection_type(type));
         }
@@ -421,7 +421,7 @@ struct MergeTracer {
             diff_field(n, get_type_name(type));
         }
 
-        void field(StringData n, Instruction::AddColumn::CollectionType type) override
+        void field(StringData n, Instruction::CollectionType type) override
         {
             diff_field(n, get_collection_type(type));
         }
@@ -1751,15 +1751,15 @@ DEFINE_MERGE(Instruction::AddColumn, Instruction::AddColumn)
         }
 
         if (left.collection_type != right.collection_type) {
-            auto collection_type_name = [](Instruction::AddColumn::CollectionType type) -> const char* {
+            auto collection_type_name = [](Instruction::CollectionType type) -> const char* {
                 switch (type) {
-                    case Instruction::AddColumn::CollectionType::Single:
+                    case Instruction::CollectionType::Single:
                         return "single value";
-                    case Instruction::AddColumn::CollectionType::List:
+                    case Instruction::CollectionType::List:
                         return "list";
-                    case Instruction::AddColumn::CollectionType::Dictionary:
+                    case Instruction::CollectionType::Dictionary:
                         return "dictionary";
-                    case Instruction::AddColumn::CollectionType::Set:
+                    case Instruction::CollectionType::Set:
                         return "set";
                 }
                 REALM_TERMINATE("");
diff --git a/test/object-store/sync/flx_sync.cpp b/test/object-store/sync/flx_sync.cpp
@@ -2581,7 +2581,7 @@ TEST_CASE("flx: writes work without waiting for sync", "[sync][flx][baas]") {
 TEST_CASE("flx: verify websocket protocol number and prefixes", "[sync][protocol]") {
     // Update the expected value whenever the protocol version is updated - this ensures
     // that the current protocol version does not change unexpectedly.
-    REQUIRE(12 == sync::get_current_protocol_version());
+    REQUIRE(13 == sync::get_current_protocol_version());
     // This was updated in Protocol V8 to use '#' instead of '/' to support the Web SDK
     REQUIRE("com.mongodb.realm-sync#" == sync::get_pbs_websocket_protocol_prefix());
     REQUIRE("com.mongodb.realm-query-sync#" == sync::get_flx_websocket_protocol_prefix());
diff --git a/test/test_changeset_encoding.cpp b/test/test_changeset_encoding.cpp
@@ -3,11 +3,13 @@
 #include <realm/sync/changeset.hpp>
 #include <realm/sync/changeset_encoder.hpp>
 #include <realm/sync/changeset_parser.hpp>
+#include <realm/sync/instructions.hpp>
 #include <realm/sync/noinst/integer_codec.hpp>
 
 using namespace realm;
 using namespace realm::sync::instr;
 using realm::sync::Changeset;
+using realm::sync::Instruction;
 
 namespace {
 Changeset encode_then_parse(const Changeset& changeset)
@@ -75,7 +77,7 @@ TEST(ChangesetEncoding_AddColumn)
     instr.table = changeset.intern_string("Foo");
     instr.field = changeset.intern_string("foo");
     instr.type = Payload::Type::Link;
-    instr.collection_type = AddColumn::CollectionType::List;
+    instr.collection_type = Instruction::CollectionType::List;
     instr.nullable = false;
     instr.link_target_table = changeset.intern_string("Bar");
     instr.key_type = Payload::Type::Null;
@@ -270,6 +272,7 @@ TEST(ChangesetEncoding_Clear)
     instr.path.push_back(234);
     instr.path.push_back(changeset.intern_string("lol"));
     instr.path.push_back(5);
+    instr.collection_type = Instruction::CollectionType::List;
     changeset.push_back(instr);
 
     auto parsed = encode_then_parse(changeset);

Original file line number	Diff line number	Diff line change
`@@ -431,7 +431,7 @@ void Changeset::Reflector::operator()(const Instruction::AddColumn& p) const`
`431`	`431`	`if (p.type == Instruction::Payload::Type::Link) {`
`432`	`432`	`m_tracer.field("target_table", p.link_target_table);`
`433`	`433`	`}`
`434`		`- if (p.collection_type == Instruction::AddColumn::CollectionType::Dictionary) {`
	`434`	`+ if (p.collection_type == Instruction::CollectionType::Dictionary) {`
`435`	`435`	`m_tracer.field("key_type", p.key_type);`
`436`	`436`	`}`
`437`	`437`	`}`
`@@ -505,7 +505,7 @@ void Changeset::Printer::field(StringData n, Instruction::Payload::Type type)`
`505`	`505`	`print_field(n, get_type_name(type));`
`506`	`506`	`}`
`507`	`507`
`508`		`-void Changeset::Printer::field(StringData n, Instruction::AddColumn::CollectionType type)`
	`508`	`+void Changeset::Printer::field(StringData n, Instruction::CollectionType type)`
`509`	`509`	`{`
`510`	`510`	`print_field(n, get_collection_type(type));`
`511`	`511`	`}`
Original file line number	Diff line number	Diff line change
`@@ -121,7 +121,7 @@ void ChangesetEncoder::append_value(Instruction::Payload::Type type)`
`121`	`121`	`append_value(int64_t(type));`
`122`	`122`	`}`
`123`	`123`
`124`		`-void ChangesetEncoder::append_value(Instruction::AddColumn::CollectionType type)`
	`124`	`+void ChangesetEncoder::append_value(Instruction::CollectionType type)`
`125`	`125`	`{`
`126`	`126`	`append_value(uint8_t(type));`
`127`	`127`	`}`
`@@ -190,7 +190,7 @@ void ChangesetEncoder::operator()(const Instruction::AddInteger& instr)`
`190`	`190`
`191`	`191`	`void ChangesetEncoder::operator()(const Instruction::AddColumn& instr)`
`192`	`192`	`{`
`193`		`- bool is_dictionary = (instr.collection_type == Instruction::AddColumn::CollectionType::Dictionary);`
	`193`	`+ bool is_dictionary = (instr.collection_type == Instruction::CollectionType::Dictionary);`
`194`	`194`	`// Mixed columns are always nullable.`
`195`	`195`	`REALM_ASSERT(instr.type != Instruction::Payload::Type::Null \|\| instr.nullable \|\| is_dictionary);`
`196`	`196`	`append(Instruction::Type::AddColumn, instr.table, instr.field, instr.type, instr.nullable, instr.collection_type);`
`@@ -225,8 +225,7 @@ void ChangesetEncoder::operator()(const Instruction::ArrayErase& instr)`
`225`	`225`
`226`	`226`	`void ChangesetEncoder::operator()(const Instruction::Clear& instr)`
`227`	`227`	`{`
`228`		`- uint32_t prior_size = 0; // Ignored`
`229`		`- append_path_instr(Instruction::Type::Clear, instr, prior_size);`
	`228`	`+ append_path_instr(Instruction::Type::Clear, instr, instr.collection_type);`
`230`	`229`	`}`
`231`	`230`
`232`	`231`	`void ChangesetEncoder::operator()(const Instruction::SetInsert& instr)`
Original file line number	Diff line number	Diff line change
`@@ -550,7 +550,7 @@ void InstructionApplier::operator()(const Instruction::AddInteger& instr)`
`550`	`550`	`void InstructionApplier::operator()(const Instruction::AddColumn& instr)`
`551`	`551`	`{`
`552`	`552`	`using Type = Instruction::Payload::Type;`
`553`		`- using CollectionType = Instruction::AddColumn::CollectionType;`
	`553`	`+ using CollectionType = Instruction::CollectionType;`
`554`	`554`
`555`	`555`	`// Temporarily swap out the last object key so it doesn't get included in error messages`
`556`	`556`	`TemporarySwapOut<decltype(m_last_object_key)> last_object_key_guard(m_last_object_key);`
Original file line number	Diff line number	Diff line change
`@@ -328,7 +328,7 @@ void SyncReplication::insert_column(const Table* table, ColKey col_key, DataType`
`328`	`328`	`Table* target_table)`
`329`	`329`	`{`
`330`	`330`	`Replication::insert_column(table, col_key, type, name, target_table);`
`331`		`- using CollectionType = Instruction::AddColumn::CollectionType;`
	`331`	`+ using CollectionType = Instruction::CollectionType;`
`332`	`332`
`333`	`333`	`if (select_table(*table)) {`
`334`	`334`	`Instruction::AddColumn instr;`
`@@ -570,6 +570,7 @@ void SyncReplication::list_clear(const CollectionBase& view)`
`570`	`570`	`if (select_collection(view)) {`
`571`	`571`	`Instruction::Clear instr;`
`572`	`572`	`populate_path_instr(instr, view);`
	`573`	`+ instr.collection_type = Instruction::CollectionType::List;`
`573`	`574`	`emit(instr);`
`574`	`575`	`}`
`575`	`576`	`}`
`@@ -605,6 +606,7 @@ void SyncReplication::set_clear(const CollectionBase& set)`
`605`	`606`	`if (select_collection(set)) {`
`606`	`607`	`Instruction::Clear instr;`
`607`	`608`	`populate_path_instr(instr, set);`
	`609`	`+ instr.collection_type = Instruction::CollectionType::Set;`
`608`	`610`	`emit(instr);`
`609`	`611`	`}`
`610`	`612`	`}`
`@@ -663,6 +665,7 @@ void SyncReplication::dictionary_clear(const CollectionBase& dict)`
`663`	`665`	`if (select_collection(dict)) {`
`664`	`666`	`Instruction::Clear instr;`
`665`	`667`	`populate_path_instr(instr, dict);`
	`668`	`+ instr.collection_type = Instruction::CollectionType::Dictionary;`
`666`	`669`	`emit(instr);`
`667`	`670`	`}`
`668`	`671`	`}`