Check for schema version before client reset callbacks #8125
Conversation
| @@ -27,18 +27,20 @@ | |||
| #import "RLMObjectSchema_Private.hpp" | |||
| #import "RLMRealm+Sync.h" | |||
| #import "RLMRealmConfiguration_Private.h" | |||
There was a problem hiding this comment.
not required as RLMRealmConfiguration_Private.hpp pulls it in
| afterExpectation.inverted = true; | ||
|
|
||
| #pragma clang diagnostic push | ||
| #pragma clang diagnostic ignored "-Wunused-parameter" |
There was a problem hiding this comment.
just remove the parameter names instead of suppressing warnings
Co-authored-by: Lee Maguire <[email protected]>
|
|
||
| // Create a config that's not versioned. | ||
| RLMRealmConfiguration *configUnversioned = [RLMRealmConfiguration defaultConfiguration]; | ||
| configUnversioned.configRef.schema_version = RLMNotVersioned; // Not strictly necessary. Already has no version because the realm's never been opened. |
There was a problem hiding this comment.
I don't see how this isn't strictly necessary? schema_version defaults to 0.
There was a problem hiding this comment.
I think once the realm is opened it by RLMRealmWithConfiguration the default is applied. But since this config is only ever used to make a sharedRealm I think 1527 isn't needed for the assert on 1541 to pass. I could remove the comment, but either way would like to leave line 1527 in.
There was a problem hiding this comment.
schemaVersion is set to 0 in RLMRealmConfiguration's initializer. The schema version is not used for anything though because there's no schema set, so this test will pass with the schema version set to any value.
There was a problem hiding this comment.
Comments removed. Or do you think the whole lines could be removed since the schema isn't set?
There was a problem hiding this comment.
I think it's maybe misleading to have it, since it's not actually being used? Not really a big deal either way.
| std::shared_ptr<realm::Realm> unversioned = realm::Realm::get_shared_realm(configUnversioned.config); | ||
|
|
||
| XCTestExpectation *beforeExpectation = [self expectationWithDescription:@"block called once"]; | ||
| beforeExpectation.assertForOverFulfill = true; // asserts that fulfill isn't called more than once. |
There was a problem hiding this comment.
This is the default and only needs to be set when it's being disabled.
| XCTestExpectation *beforeExpectation = [self expectationWithDescription:@"block called once"]; | ||
| beforeExpectation.assertForOverFulfill = true; // asserts that fulfill isn't called more than once. | ||
|
|
||
| realm::BeforeClientResetWrapper beforeWrapper = { |
There was a problem hiding this comment.
You can avoid exposing these for testing by obtaining them via RLMSyncConfiguration:
RLMSyncConfiguration *syncConfig = [[RLMSyncConfiguration alloc] initWithRawConfig:{} path:""];
syncConfig.beforeClientReset = ^(RLMRealm *beforeFrozen) {
XCTAssertNotEqual(RLMNotVersioned, beforeFrozen->_realm->schema_version());
[beforeExpectation fulfill];
};
auto& beforeWrapper = syncConfig.rawConfiguration.notify_before_client_reset;
This makes the test a bit less coupled to the implementation details and avoids having to export the wrapper types.
There was a problem hiding this comment.
Wow, thanks. This clears up a ton.
Realm/RLMSyncConfiguration.mm
Outdated
| bool dynamic; | ||
| std::string path; | ||
| RLMSchema *customSchema; | ||
| namespace realm { |
There was a problem hiding this comment.
The realm namespace is for core and we shouldn't be defining things in it.
Realm/RLMSyncConfiguration.mm
Outdated
| return cached.schema; | ||
| } | ||
| return customSchema ?: RLMSchema.sharedSchema; | ||
| RLMSchema *CallbackSchema::getSchema(Realm& realm) { |
There was a problem hiding this comment.
I don't see any reason for this to be a member function?
|
Are the xcode cloud swiftlint and docs errors "real"? |
It's possible for a realm with no schema version to experience a client reset. If this happens, the callback wrappers shouldn't be called. Otherwise a crash can occur at set_schema_subset in realmWithSharedRealm. From internal ticket 40981 and #8126