From 961c616802ba7d5c4a13ac2814672091b969593c Mon Sep 17 00:00:00 2001 From: Daniel Tabacaru Date: Mon, 16 Jan 2023 13:22:38 +0100 Subject: [PATCH 1/4] Include context about what object caused the exception in OT --- src/realm/sync/transform.cpp | 197 ++++++++++++++++++++++------------- src/realm/sync/transform.hpp | 5 - 2 files changed, 125 insertions(+), 77 deletions(-) diff --git a/src/realm/sync/transform.cpp b/src/realm/sync/transform.cpp index 8ac67259022..66f244f40cd 100644 --- a/src/realm/sync/transform.cpp +++ b/src/realm/sync/transform.cpp @@ -52,9 +52,6 @@ namespace { #endif #endif -#define REALM_MERGE_ASSERT(condition) \ - (REALM_LIKELY(condition) ? static_cast(0) : throw sync::TransformError{"Assertion failed: " #condition}) - } // unnamed namespace using namespace realm; @@ -895,6 +892,25 @@ void TransformerImpl::MinorSide::prepend(InputIterator begin, InputIterator end) namespace { +REALM_NORETURN void throw_bad_merge(std::string msg) +{ + throw sync::TransformError{std::move(msg)}; +} + +template +REALM_NORETURN void bad_merge(const char* msg, Params&&... params) +{ + throw_bad_merge(util::format(msg, std::forward(params)...)); +} + +REALM_NORETURN void bad_merge(_impl::TransformerImpl::Side& side, Instruction::ObjectInstruction instr, + const std::string& msg) +{ + StringData table_name = side.get_string(instr.table); + auto pk = format_pk(side.m_changeset->get_key(instr.object)); + bad_merge("%1. Instruction target: %2[%3]", msg, table_name, pk); +} + template struct Merge; template @@ -975,7 +991,7 @@ struct MergeUtils { return left.data.uuid == right.data.uuid; } - REALM_MERGE_ASSERT(false && "Invalid payload type in instruction"); + bad_merge("Invalid payload type in instruction"); } bool same_path_element(const Instruction::Path::Element& left, @@ -1210,7 +1226,7 @@ struct MergeUtils { REALM_ASSERT(mpark::holds_alternative(left.path.back())); size_t index = left.path.size() - 1; if (!mpark::holds_alternative(right.path[index])) { - throw TransformError{"Inconsistent paths"}; + bad_merge("Inconsistent paths"); } return mpark::get(right.path[index]); } @@ -1389,44 +1405,35 @@ DEFINE_MERGE(Instruction::AddTable, Instruction::AddTable) StringData left_pk_name = left_side.get_string(left_spec->pk_field); StringData right_pk_name = right_side.get_string(right_spec->pk_field); if (left_pk_name != right_pk_name) { - std::stringstream ss; - ss << "Schema mismatch: '" << left_name << "' has primary key '" << left_pk_name - << "' on one side, but primary key '" << right_pk_name << "' on the other."; - throw SchemaMismatchError(ss.str()); + bad_merge( + "Schema mismatch: '%1' has primary key '%2' on one side, but primary key '%2' on the other.", + left_name, left_pk_name, right_pk_name); } if (left_spec->pk_type != right_spec->pk_type) { - std::stringstream ss; - ss << "Schema mismatch: '" << left_name << "' has primary key '" << left_pk_name - << "', which is of type " << get_type_name(left_spec->pk_type) << " on one side and type " - << get_type_name(right_spec->pk_type) << " on the other."; - throw SchemaMismatchError(ss.str()); + bad_merge("Schema mismatch: '%1' has primary key '%2', which is of type %3 on one side and type " + "%4 on the other.", + left_name, left_pk_name, get_type_name(left_spec->pk_type), + get_type_name(right_spec->pk_type)); } if (left_spec->pk_nullable != right_spec->pk_nullable) { - std::stringstream ss; - ss << "Schema mismatch: '" << left_name << "' has primary key '" << left_pk_name - << "', which is nullable on one side, but not the other."; - throw SchemaMismatchError(ss.str()); + bad_merge("Schema mismatch: '%1' has primary key '%2', which is nullable on one side, but not " + "the other.", + left_name, left_pk_name); } if (left_spec->is_asymmetric != right_spec->is_asymmetric) { - std::stringstream ss; - ss << "Schema mismatch: '" << left_name << "' is asymmetric on one side, but not on the other."; - throw SchemaMismatchError(ss.str()); + bad_merge("Schema mismatch: '%1' is asymmetric on one side, but not on the other.", left_name); } } else { - std::stringstream ss; - ss << "Schema mismatch: '" << left_name << "' has a primary key on one side, but not on the other."; - throw SchemaMismatchError(ss.str()); + bad_merge("Schema mismatch: '%1' has a primary key on one side, but not on the other.", left_name); } } else if (mpark::get_if(&left.type)) { if (!mpark::get_if(&right.type)) { - std::stringstream ss; - ss << "Schema mismatch: '" << left_name << "' is an embedded table on one side, but not the other."; - throw SchemaMismatchError(ss.str()); + bad_merge("Schema mismatch: '%1' is an embedded table on one side, but not the other.", left_name); } } @@ -1601,15 +1608,19 @@ DEFINE_MERGE(Instruction::Update, Instruction::Update) if (same_path(left, right)) { bool left_is_default = false; bool right_is_default = false; - REALM_MERGE_ASSERT(left.is_array_update() == right.is_array_update()); + if (!(left.is_array_update() == right.is_array_update())) { + bad_merge(left_side, left, "Merge error: left.is_array_update() == right.is_array_update()"); + } if (!left.is_array_update()) { - REALM_MERGE_ASSERT(!right.is_array_update()); + if (right.is_array_update()) { + bad_merge(right_side, right, "Merge error: !right.is_array_update()"); + } left_is_default = left.is_default; right_is_default = right.is_default; } - else { - REALM_MERGE_ASSERT(left.prior_size == right.prior_size); + else if (!(left.prior_size == right.prior_size)) { + bad_merge(left_side, left, "Merge error: left.prior_size == right.prior_size"); } if (left.value.type != right.value.type) { @@ -1661,7 +1672,10 @@ DEFINE_MERGE(Instruction::AddInteger, Instruction::Update) // RESOLUTION: If the Add was later than the Set, add its value to // the payload of the Set instruction. Otherwise, discard it. - REALM_MERGE_ASSERT(right.value.type == Instruction::Payload::Type::Int || right.value.is_null()); + if (!(right.value.type == Instruction::Payload::Type::Int || right.value.is_null())) { + bad_merge(right_side, right, + "Merge error: right.value.type == Instruction::Payload::Type::Int || right.value.is_null()"); + } bool right_is_default = !right.is_array_update() && right.is_default; @@ -1698,9 +1712,15 @@ DEFINE_MERGE(Instruction::ArrayInsert, Instruction::Update) { if (same_container(left, right)) { REALM_ASSERT(right.is_array_update()); - REALM_MERGE_ASSERT(left.prior_size == right.prior_size); - REALM_MERGE_ASSERT(left.index() <= left.prior_size); - REALM_MERGE_ASSERT(right.index() < right.prior_size); + if (!(left.prior_size == right.prior_size)) { + bad_merge(left_side, left, "Merge error: left.prior_size == right.prior_size"); + } + if (!(left.index() <= left.prior_size)) { + bad_merge(left_side, left, "Merge error: left.index() <= left.prior_size"); + } + if (!(right.index() < right.prior_size)) { + bad_merge(right_side, right, "Merge error: right.index() < right.prior_size"); + } right.prior_size += 1; if (right.index() >= left.index()) { right.index() += 1; // ---> @@ -1713,8 +1733,12 @@ DEFINE_MERGE(Instruction::ArrayMove, Instruction::Update) if (same_container(left, right)) { REALM_ASSERT(right.is_array_update()); - REALM_MERGE_ASSERT(left.index() < left.prior_size); - REALM_MERGE_ASSERT(right.index() < right.prior_size); + if (!(left.index() < left.prior_size)) { + bad_merge(left_side, left, "Merge error: left.index() < left.prior_size"); + } + if (!(right.index() < right.prior_size)) { + bad_merge(right_side, right, "Merge error: right.index() < right.prior_size"); + } // FIXME: This marks both sides as dirty, even when they are unmodified. merge_get_vs_move(right.index(), left.index(), left.ndx_2); @@ -1725,9 +1749,15 @@ DEFINE_MERGE(Instruction::ArrayErase, Instruction::Update) { if (same_container(left, right)) { REALM_ASSERT(right.is_array_update()); - REALM_MERGE_ASSERT(left.prior_size == right.prior_size); - REALM_MERGE_ASSERT(left.index() < left.prior_size); - REALM_MERGE_ASSERT(right.index() < right.prior_size); + if (!(left.prior_size == right.prior_size)) { + bad_merge(left_side, left, "Merge error: left.prior_size == right.prior_size"); + } + if (!(left.index() < left.prior_size)) { + bad_merge(left_side, left, "Merge error: left.index() < left.prior_size"); + } + if (!(right.index() < right.prior_size)) { + bad_merge(right_side, right, "Merge error: right.index() < right.prior_size"); + } // CONFLICT: Update of a removed element. // @@ -1775,18 +1805,14 @@ DEFINE_MERGE(Instruction::AddColumn, Instruction::AddColumn) if (same_column(left, right)) { StringData left_name = left_side.get_string(left.field); if (left.type != right.type) { - std::stringstream ss; - ss << "Schema mismatch: Property '" << left_name << "' in class '" << left_side.get_string(left.table) - << "' is of type " << get_type_name(left.type) << " on one side and type " << get_type_name(right.type) - << " on the other."; - throw SchemaMismatchError(ss.str()); + bad_merge( + "Schema mismatch: Property '%1' in class '%2' is of type %3 on one side and type %4 on the other.", + left_name, left_side.get_string(left.table), get_type_name(left.type), get_type_name(right.type)); } if (left.nullable != right.nullable) { - std::stringstream ss; - ss << "Schema mismatch: Property '" << left_name << "' in class '" << left_side.get_string(left.table) - << "' is nullable on one side and not on the other."; - throw SchemaMismatchError(ss.str()); + bad_merge("Schema mismatch: Property '%1' in class '%2' is nullable on one side and not on the other.", + left_name, left_side.get_string(left.table)); } if (left.collection_type != right.collection_type) { @@ -1807,20 +1833,17 @@ DEFINE_MERGE(Instruction::AddColumn, Instruction::AddColumn) std::stringstream ss; const char* left_type = collection_type_name(left.collection_type); const char* right_type = collection_type_name(right.collection_type); - ss << "Schema mismatch: Property '" << left_name << "' in class '" << left_side.get_string(left.table) - << "' is a " << left_type << " on one side, and a " << right_type << " on the other."; - throw SchemaMismatchError(ss.str()); + bad_merge("Schema mismatch: Property '%1' in class '%2' is a %3 on one side, and a %4 on the other.", + left_name, left_side.get_string(left.table), left_type, right_type); } if (left.type == Instruction::Payload::Type::Link) { StringData left_target = left_side.get_string(left.link_target_table); StringData right_target = right_side.get_string(right.link_target_table); if (left_target != right_target) { - std::stringstream ss; - ss << "Schema mismatch: Link property '" << left_name << "' in class '" - << left_side.get_string(left.table) << "' points to class '" << left_target - << "' on one side and to '" << right_target << "' on the other."; - throw SchemaMismatchError(ss.str()); + bad_merge("Schema mismatch: Link property '%1' in class '%2' points to class '%3' on one side and to " + "'%4' on the other.", + left_name, left_side.get_string(left.table), left_target, right_target); } } @@ -1880,7 +1903,9 @@ DEFINE_NESTED_MERGE(Instruction::ArrayInsert) DEFINE_MERGE(Instruction::ArrayInsert, Instruction::ArrayInsert) { if (same_container(left, right)) { - REALM_MERGE_ASSERT(left.prior_size == right.prior_size); + if (!(left.prior_size == right.prior_size)) { + bad_merge(right_side, right, "Exception: left.prior_size == right.prior_size"); + } left.prior_size++; right.prior_size++; @@ -1943,9 +1968,15 @@ DEFINE_MERGE(Instruction::ArrayMove, Instruction::ArrayInsert) DEFINE_MERGE(Instruction::ArrayErase, Instruction::ArrayInsert) { if (same_container(left, right)) { - REALM_MERGE_ASSERT(left.prior_size == right.prior_size); - REALM_MERGE_ASSERT(left.index() < left.prior_size); - REALM_MERGE_ASSERT(right.index() <= right.prior_size); + if (!(left.prior_size == right.prior_size)) { + bad_merge(left_side, left, "Merge error: left.prior_size == right.prior_size"); + } + if (!(left.index() < left.prior_size)) { + bad_merge(left_side, left, "Merge error: left.index() < left.prior_size"); + } + if (!(right.index() <= right.prior_size)) { + bad_merge(left_side, left, "Merge error: right.index() <= right.prior_size"); + } left.prior_size++; right.prior_size--; @@ -1977,11 +2008,21 @@ DEFINE_NESTED_MERGE(Instruction::ArrayMove) DEFINE_MERGE(Instruction::ArrayMove, Instruction::ArrayMove) { if (same_container(left, right)) { - REALM_MERGE_ASSERT(left.prior_size == right.prior_size); - REALM_MERGE_ASSERT(left.index() < left.prior_size); - REALM_MERGE_ASSERT(right.index() < right.prior_size); - REALM_MERGE_ASSERT(left.ndx_2 < left.prior_size); - REALM_MERGE_ASSERT(right.ndx_2 < right.prior_size); + if (!(left.prior_size == right.prior_size)) { + bad_merge(left_side, left, "Merge error: left.prior_size == right.prior_size"); + } + if (!(left.index() < left.prior_size)) { + bad_merge(left_side, left, "Merge error: left.index() < left.prior_size"); + } + if (!(right.index() < right.prior_size)) { + bad_merge(right_side, right, "Merge error: right.index() < right.prior_size"); + } + if (!(left.ndx_2 < left.prior_size)) { + bad_merge(left_side, left, "Merge error: left.ndx_2 < left.prior_size"); + } + if (!(right.ndx_2 < right.prior_size)) { + bad_merge(right_side, right, "Merge error: right.ndx_2 < right.prior_size"); + } if (left.index() < right.index()) { right.index() -= 1; // <--- @@ -2062,9 +2103,15 @@ DEFINE_MERGE(Instruction::ArrayMove, Instruction::ArrayMove) DEFINE_MERGE(Instruction::ArrayErase, Instruction::ArrayMove) { if (same_container(left, right)) { - REALM_MERGE_ASSERT(left.prior_size == right.prior_size); - REALM_MERGE_ASSERT(left.index() < left.prior_size); - REALM_MERGE_ASSERT(right.index() < right.prior_size); + if (!(left.prior_size == right.prior_size)) { + bad_merge(left_side, left, "Merge error: left.prior_size == right.prior_size"); + } + if (!(left.index() < left.prior_size)) { + bad_merge(left_side, left, "Merge error: left.index() < left.prior_size"); + } + if (!(right.index() < right.prior_size)) { + bad_merge(right_side, right, "Merge error: right.index() < right.prior_size"); + } right.prior_size -= 1; @@ -2129,9 +2176,15 @@ DEFINE_NESTED_MERGE(Instruction::ArrayErase) DEFINE_MERGE(Instruction::ArrayErase, Instruction::ArrayErase) { if (same_container(left, right)) { - REALM_MERGE_ASSERT(left.prior_size == right.prior_size); - REALM_MERGE_ASSERT(left.index() < left.prior_size); - REALM_MERGE_ASSERT(right.index() < right.prior_size); + if (!(left.prior_size == right.prior_size)) { + bad_merge(left_side, left, "Merge error: left.prior_size == right.prior_size"); + } + if (!(left.index() < left.prior_size)) { + bad_merge(left_side, left, "Merge error: left.index() < left.prior_size"); + } + if (!(right.index() < right.prior_size)) { + bad_merge(right_side, right, "Merge error: right.index() < right.prior_size"); + } left.prior_size -= 1; right.prior_size -= 1; diff --git a/src/realm/sync/transform.hpp b/src/realm/sync/transform.hpp index d9cb4d72a0c..f772e390d36 100644 --- a/src/realm/sync/transform.hpp +++ b/src/realm/sync/transform.hpp @@ -296,11 +296,6 @@ class TransformError : public std::runtime_error { } }; -class SchemaMismatchError : public TransformError { -public: - using TransformError::TransformError; -}; - inline Transformer::RemoteChangeset::RemoteChangeset(version_type rv, version_type lv, ChunkedBinaryData d, timestamp_type ot, file_ident_type fi) : remote_version(rv) From 0503b7279bddeabf522304feff4057bb36c0c71a Mon Sep 17 00:00:00 2001 From: Daniel Tabacaru Date: Mon, 16 Jan 2023 13:32:02 +0100 Subject: [PATCH 2/4] changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index d9bcb6e0b40..dfdb55bae7d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,7 @@ ### Enhancements * `SyncSession::pause()` and `SyncSession::resume()` allow users to suspend a Realm's sync session until it is explicitly resumed in ([#6183](https://github.com/realm/realm-core/pull/6183)). Previously `SyncSession::log_out()` and `SyncSession::close()` could be resumed under a number of circumstances where `SyncSession::revive_if_needed()` were called (like when freezing a realm) - fixes ([#6085](https://github.com/realm/realm-core/issues/6085)) +* Include context about what object caused the merge exception in OT ([#6204](https://github.com/realm/realm-core/issues/6204)) ### Fixed * ([#????](https://github.com/realm/realm-core/issues/????), since v?.?.?) From 01b9ac7a457c8902f76e82045306012e546ec2f9 Mon Sep 17 00:00:00 2001 From: Daniel Tabacaru Date: Mon, 16 Jan 2023 13:47:03 +0100 Subject: [PATCH 3/4] Fix wrong string placeholder --- src/realm/sync/transform.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/realm/sync/transform.cpp b/src/realm/sync/transform.cpp index 66f244f40cd..2bb222d85e8 100644 --- a/src/realm/sync/transform.cpp +++ b/src/realm/sync/transform.cpp @@ -1406,7 +1406,7 @@ DEFINE_MERGE(Instruction::AddTable, Instruction::AddTable) StringData right_pk_name = right_side.get_string(right_spec->pk_field); if (left_pk_name != right_pk_name) { bad_merge( - "Schema mismatch: '%1' has primary key '%2' on one side, but primary key '%2' on the other.", + "Schema mismatch: '%1' has primary key '%2' on one side, but primary key '%3' on the other.", left_name, left_pk_name, right_pk_name); } From e3ee3ed3e1204e386f989b13b3220cf43bc49e44 Mon Sep 17 00:00:00 2001 From: Daniel Tabacaru Date: Sat, 21 Jan 2023 11:18:40 +0100 Subject: [PATCH 4/4] Add information about the field and path causing the exception --- src/realm/sync/transform.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/realm/sync/transform.cpp b/src/realm/sync/transform.cpp index 2bb222d85e8..20f6bb0f000 100644 --- a/src/realm/sync/transform.cpp +++ b/src/realm/sync/transform.cpp @@ -903,12 +903,12 @@ REALM_NORETURN void bad_merge(const char* msg, Params&&... params) throw_bad_merge(util::format(msg, std::forward(params)...)); } -REALM_NORETURN void bad_merge(_impl::TransformerImpl::Side& side, Instruction::ObjectInstruction instr, +REALM_NORETURN void bad_merge(_impl::TransformerImpl::Side& side, Instruction::PathInstruction instr, const std::string& msg) { - StringData table_name = side.get_string(instr.table); - auto pk = format_pk(side.m_changeset->get_key(instr.object)); - bad_merge("%1. Instruction target: %2[%3]", msg, table_name, pk); + std::stringstream ss; + side.m_changeset->print_path(ss, instr.table, instr.object, instr.field, &instr.path); + bad_merge("%1 (instruction target: %2)", msg, ss.str()); } template