Fix backlink check in migration.

[DominicFrei]

Necessary core changes to enable realm/realm-swift#7094 (realm/realm-swift#7060):

• Setting tables to embedded had to be moved to post migration changes.
• Objects without backlinks will get deleted.

[fealebenpae]

Hey @DominicFrei, thanks for the contribution! There are just a couple of things we need to fix: please update CHANGELOG.md with a line explaining the change (see CONTRIBUTING.md), and you need to run clang-format on your patch to make sure it's formatted according to the repository rules (see the Artifacts tab in CI for the format errors).
@jedelbo could you please review this?

[DominicFrei]

@fealebenpae Thanks for clearing that up! I've put it into draft mode since there seems to be one thing to clarify first. @tgoyne suggested a couple of tests that I implemented. According to the CI, one (already existing) test is failing though:
TEST(Group_ChangeEmbeddedness)
It checks if all objects have a parent when their table is set to embedded.
Thomas' suggestion for one of the new tests was:
Objects with zero incoming links are automatically deleted when converting to embedded
Those two tests conflict now if I understand it correctly, and I first need to understand which of the two is the desired behaviour.

[DominicFrei]

According to the original post in the forum that brought this problem up it seems like that specific user did actually have child objects being used by multiple parent objects. This did fail in the past and will still fail after this PR but the user will have a chance to fix it within the migration block from now on due to calling set_embedded in the apply_post_migration_changes instead of the apply_pre_migration_changes (see src/realm/object-store/object_store.cpp).

Maybe the user was mislead by the error message in https://github.com/realm/realm-core/blob/master/src/realm/object-store/object_store.cpp#L570 though:
"Cannot convert object type '%1' to embedded because objects have multiple incoming links."
This error is thrown whenever set_embedded returns false which can happen in multiple cases, not just if (o.get_backlink_count() != 1).

To address this issue a bit better and also incorporate the suggestions made by @jedelbo and @ironage I reworked the set_embedded function:

• Move the above mentioned error into set_embedded and only throw this particular one in case there are multiple incoming links.
• Add new throws in all other cases where we returned false for reasons other than multiple backlinks.
• Remove suggested deletion of objects and throw an error in all those cases.
• Add a check for isEmbedded == false as suggested by @ironage to skip unnecessary checks.

Notes:

• I also tried to add a unit test for the if (get_primary_key_column()) path (since I reworked the whole function anyway). The test passes but a different reason (tbd).
• The path if (embedded == m_is_embedded) is actually never hit since https://github.com/realm/realm-core/blob/master/src/realm/object-store/shared_realm.cpp#L344 evaluates the required_changes to be empty which means set_embedded is never called anyway.

[ironage]

👍

[sipersso]

According to the original post in the forum that brought this problem up it seems like that specific user did actually have child objects being used by multiple parent objects. This did fail in the past and will still fail after this PR but the user will have a chance to fix it within the migration block from now on due to calling set_embedded in the apply_post_migration_changes instead of the apply_pre_migration_changes (see src/realm/object-store/object_store.cpp).

Maybe the user was mislead by the error message in https://github.com/realm/realm-core/blob/master/src/realm/object-store/object_store.cpp#L570 though:
"Cannot convert object type '%1' to embedded because objects have multiple incoming links."
This error is thrown whenever set_embedded returns false which can happen in multiple cases, not just if (o.get_backlink_count() != 1).

To address this issue a bit better and also incorporate the suggestions made by @jedelbo and @ironage I reworked the set_embedded function:

• Move the above mentioned error into set_embedded and only throw this particular one in case there are multiple incoming links.
• Add new throws in all other cases where we returned false for reasons other than multiple backlinks.
• Remove suggested deletion of objects and throw an error in all those cases.
• Add a check for isEmbedded == false as suggested by @ironage to skip unnecessary checks.

Notes:

• I also tried to add a unit test for the if (get_primary_key_column()) path (since I reworked the whole function anyway). The test passes but a different reason (tbd).
• The path if (embedded == m_is_embedded) is actually never hit since https://github.com/realm/realm-core/blob/master/src/realm/object-store/shared_realm.cpp#L344 evaluates the required_changes to be empty which means set_embedded is never called anyway.

I am "the user" from the forums. Just to clarify, my objects that I wanted to embed did not have multiple parents even though the crash said that they did, so the error message was misleading.

But... one thing I am scared about is orphaned objects. I would not be able to use this in production without a way to ensure that I have deleted all possible orphaned objects. Crashes in the migration function are about as bad as it can get from a user perspective.

[DominicFrei]

@sipersso The changes made in https://github.com/realm/realm-core/pull/4414/files#diff-e752b38a6d72eb50c7ab147fe48749dee382b2c4153c01364db035c2843f1519R1054 lead to several different errors that will be thrown depending on the problem that happened. The error message should be accurate now.