-
Notifications
You must be signed in to change notification settings - Fork 168
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Do not allow migrations with asymmetric tables #5603
Do not allow migrations with asymmetric tables #5603
Conversation
Can you rebase this onto latest master to fix all the object-store-tests? |
02a0584
to
774161d
Compare
@@ -423,9 +423,12 @@ static inline realm_class_info_t to_capi(const ObjectSchema& o) | |||
info.num_properties = o.persisted_properties.size(); | |||
info.num_computed_properties = o.computed_properties.size(); | |||
info.key = o.table_key.value; | |||
if (o.is_embedded) { | |||
if (o.table_type == ObjectSchema::TableType::Embedded) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this just be a switch statement?
o << "TopLevelAsymmetric"; | ||
break; | ||
default: | ||
o << "Unknown table type"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this isn't just going to be REALM_UNREACHABLE, do we want to output the value as an integer here for debugging purposes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point
using IsAsymmetric = util::TaggedBool<class IsAsymmetricTag>; | ||
/// The type of tables supported by a realm. | ||
/// Note: Enumeration value assignments must be kept in sync with <realm/table.hpp>. | ||
enum class TableType : uint8_t { TopLevel = 0, Embedded = 0x1, TopLevelAsymmetric = 0x2 }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we add a static_assert in the implementation file that verifies the values match? Is the reason we don't want to just use the actual Table::Type
enum here that we don't want to pull in table.hpp as an include in this file?
also, from a naming perspective. should this really be ObjectType rather than TableType at the ObjectStore layer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a strong opinion on the name (as a fact I wasn't happy with it), but I think ObjectType makes more sense.
The reason is that since some SDKs interface with OS directly, I find it a bit weird that they would need to know about table.hpp and Table::Type when creating schemas (I want everything doing with schemas to be contained in object_schema.hpp).
b01d99a
to
f05db09
Compare
What, How & Why?
This PR fixes and improves several things around asymmetric sync.
Fixes RCORE-1120.
(Commits are logically separated to make review easier)
☑️ ToDos
[ ] C-API, if public C++ API changed.