Je/fix backlink removal #6638
Je/fix backlink removal #6638
Conversation
There was a problem hiding this comment.
With this we have different behavior in debug mode ... first I thought it was a bug, but seeing it all over, I can see it is intentional. I think it should be added to the description in the changelog, that the new non-crashing behavior is only in release mode.
I like the introduction of for_all().
CHANGELOG.md
Outdated
| @@ -6,7 +6,7 @@ | |||
|
|
|||
| ### Fixed | |||
| * <How do the end-user experience this issue? what was the impact?> ([#????](https://github.com/realm/realm-core/issues/????), since v?.?.?) | |||
| * None. | |||
| * The application could crash when removing backlinks. We will no longer assert that the backlink is found, and thus not cause a crash. ([#6585](https://github.com/realm/realm-core/issues/6585), v6.0.0) | |||
There was a problem hiding this comment.
I'd suggest being more specific, like: we could crash when removing backlinks in cases where forward links did not have a corresponding backlink due to corruption. We now silently ignore this inconsistency allowing the app to continue" or similar.
8000| @@ -2036,7 +2036,11 @@ bool Obj::remove_backlink(ColKey col_key, ObjLink old_link, CascadeState& state) | |||
| if (old_link && old_link.get_obj_key()) { | |||
| REALM_ASSERT(m_table->valid_column(col_key)); | |||
| ObjKey old_key = old_link.get_obj_key(); | |||
| auto target_obj = m_table->get_parent_group()->get_object(old_link); | |||
| auto target_obj = m_table->get_parent_group()->try_get_object(old_link); | |||
| REALM_ASSERT_DEBUG(target_obj); | |||
There was a problem hiding this comment.
Should this assert still be here? It would seem to change behavior of remove_backlink() dependent upon debug mode or not?
| @@ -101,14 +101,19 @@ void ArrayBacklink::add(size_t ndx, ObjKey key) | |||
| bool ArrayBacklink::remove(size_t ndx, ObjKey key) | |||
| { | |||
| uint64_t value = Array::get(ndx); | |||
| REALM_ASSERT(value != 0); | |||
| REALM_ASSERT_DEBUG(value != 0); | |||
There was a problem hiding this comment.
Should this still be here?
| if (backlink_ndx != last_ndx) | ||
| backlink_list.set(backlink_ndx, backlink_list.get(last_ndx)); | ||
| backlink_list.truncate(last_ndx); // Throws | ||
| REALM_ASSERT_DEBUG(backlink_ndx != not_found); |
There was a problem hiding this comment.
Same comment as elsewhere
977785f to
e1b3767
Compare
What, How & Why?
Fixes #6585
This PR also includes BPlusTree::for_all which simplifies code in several places.
☑️ ToDos