Skip to content
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

Check for schema version before client reset callbacks #8125

Merged
merged 8 commits into from
Feb 10, 2023
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ x.y.z Release notes (yyyy-MM-dd)

### Fixed
* <How to hit and notice issue? what was the impact?> ([#????](https://github.com/realm/realm-swift/issues/????), since v?.?.?)
* None.
* The client reset callbacks no longer attempt to open a realm without an initialized schema ([PR #8125](https://github.com/realm/realm-swift/pull/8125))

<!-- ### Breaking Changes - ONLY INCLUDE FOR NEW MAJOR version -->

Expand Down
63 changes: 62 additions & 1 deletion Realm/ObjectServerTests/RLMObjectServerTests.mm
Original file line number Diff line number Diff line change
Expand Up @@ -27,18 +27,20 @@
#import "RLMObjectSchema_Private.hpp"
#import "RLMRealm+Sync.h"
#import "RLMRealmConfiguration_Private.h"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not required as RLMRealmConfiguration_Private.hpp pulls it in

#import "RLMRealmConfiguration_Private.hpp"
#import "RLMRealmUtil.hpp"
#import "RLMRealm_Dynamic.h"
#import "RLMRealm_Private.hpp"
#import "RLMSchema_Private.h"
#import "RLMSyncConfiguration_Private.h"
#import "RLMSyncConfiguration_Private.hpp"
#import "RLMSyncManager_Private.hpp"
#import "RLMSyncUtil_Private.h"
#import "RLMUser_Private.hpp"
#import "RLMWatchTestUtility.h"

#import <realm/object-store/shared_realm.hpp>
#import <realm/object-store/sync/sync_manager.hpp>
#import <realm/object-store/thread_safe_reference.hpp>
#import <realm/util/file.hpp>

#import <atomic>
Expand Down Expand Up @@ -1510,6 +1512,65 @@ - (void)testSetClientResetCallbacks {

}

- (void)testBeforeClientResetCallbackNotVersioned {
// Setup a realm with a versioned schema
RLMRealmConfiguration *config = [RLMRealmConfiguration defaultConfiguration];
config.fileURL = RLMTestRealmURL();
// Need to open the realm once to get a version.
@autoreleasepool {
RLMRealm *versioned = [RLMRealm realmWithConfiguration:config error:nil];
XCTAssertEqual(0U, versioned->_realm->schema_version());
}
std::shared_ptr<realm::Realm> versioned = realm::Realm::get_shared_realm(config.config);

// 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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see how this isn't strictly necessary? schema_version defaults to 0.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments removed. Or do you think the whole lines could be removed since the schema isn't set?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the default and only needs to be set when it's being disabled.


realm::BeforeClientResetWrapper beforeWrapper = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, thanks. This clears up a ton.

.block = ^(RLMRealm * _Nonnull beforeFrozen) {
XCTAssertNotEqual(RLMNotVersioned, beforeFrozen->_realm->schema_version());
[beforeExpectation fulfill];
}
};

XCTAssertNotEqual(versioned->schema_version(), RLMNotVersioned);
XCTAssertEqual(unversioned->schema_version(), RLMNotVersioned);
beforeWrapper(versioned); // one realm should invoke the block
beforeWrapper(unversioned); // while the other should not invoke the block

[self waitForExpectationsWithTimeout:5 handler:nil];
}

- (void)testAfterClientResetCallbackNotVersioned {
// 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.
std::shared_ptr<realm::Realm> unversioned = realm::Realm::get_shared_realm(configUnversioned.config);

XCTestExpectation *afterExpectation = [self expectationWithDescription:@"block called once"];
afterExpectation.inverted = true;

#pragma clang diagnostic push
#pragma clang diagnostic ignored "-Wunused-parameter"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just remove the parameter names instead of suppressing warnings

realm::AfterClientResetWrapper afterWrapper = {
.block = ^(RLMRealm * _Nonnull beforeFrozen, RLMRealm * _Nonnull after) {
[afterExpectation fulfill];
}
};
#pragma clang diagnostic pop // unused parameter warning

auto unversionedTsr = realm::ThreadSafeReference(unversioned);
XCTAssertEqual(unversioned->schema_version(), RLMNotVersioned);
afterWrapper(unversioned, std::move(unversionedTsr), false);

[self waitForExpectationsWithTimeout:1 handler:nil];
}

#pragma mark - Progress Notifications

static const NSInteger NUMBER_OF_BIG_OBJECTS = 2;
Expand Down
2 changes: 1 addition & 1 deletion Realm/RLMSyncConfiguration.h
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ typedef NS_ENUM(NSUInteger, RLMClientResetMode) {
A block type used to report before a client reset will occur.
The `beforeFrozen` is a frozen copy of the local state prior to client reset.
*/
RLM_SWIFT_SENDABLE // invoked on a backgroun thread
RLM_SWIFT_SENDABLE // invoked on a background thread
typedef void(^RLMClientResetBeforeBlock)(RLMRealm * _Nonnull beforeFrozen);

/**
Expand Down
42 changes: 19 additions & 23 deletions Realm/RLMSyncConfiguration.mm
Original file line number Diff line number Diff line change
Expand Up @@ -61,36 +61,31 @@ RLMSyncError errorKindForSyncError(SyncError error) {
return RLMSyncErrorClientSessionError;
return RLMSyncErrorClientInternalError;
}
} // anonymous namespace

struct CallbackSchema {
bool dynamic;
std::string path;
RLMSchema *customSchema;
namespace realm {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The realm namespace is for core and we shouldn't be defining things in it.


RLMSchema *getSchema(Realm& realm) {
if (dynamic) {
return [RLMSchema dynamicSchemaFromObjectStoreSchema:realm.schema()];
}
if (auto cached = RLMGetAnyCachedRealmForPath(path)) {
return cached.schema;
}
return customSchema ?: RLMSchema.sharedSchema;
RLMSchema *CallbackSchema::getSchema(Realm& realm) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see any reason for this to be a member function?

if (dynamic) {
return [RLMSchema dynamicSchemaFromObjectStoreSchema:realm.schema()];
}
};
if (auto cached = RLMGetAnyCachedRealmForPath(path)) {
return cached.schema;
}
return customSchema ?: RLMSchema.sharedSchema;
}

struct BeforeClientResetWrapper : CallbackSchema {
RLMClientResetBeforeBlock block;
void operator()(std::shared_ptr<Realm> local) {
@autoreleasepool {
void BeforeClientResetWrapper::operator ()(std::shared_ptr<realm::Realm> local) {
@autoreleasepool {
if (local->schema_version() != RLMNotVersioned) {
block([RLMRealm realmWithSharedRealm:local schema:getSchema(*local) dynamic:false]);
}
}
};
}

struct AfterClientResetWrapper : CallbackSchema {
RLMClientResetAfterBlock block;
void operator()(std::shared_ptr<Realm> local, ThreadSafeReference remote, bool) {
@autoreleasepool {
void AfterClientResetWrapper::operator()(std::shared_ptr<realm::Realm> local, realm::ThreadSafeReference remote, bool) {
@autoreleasepool {
if (local->schema_version() != RLMNotVersioned) {
RLMSchema *schema = getSchema(*local);
RLMRealm *localRealm = [RLMRealm realmWithSharedRealm:local
schema:schema
Expand All @@ -102,7 +97,8 @@ void operator()(std::shared_ptr<Realm> local, ThreadSafeReference remote, bool)
block(localRealm, remoteRealm);
}
}
};
}

} // anonymous namespace
ejm01 marked this conversation as resolved.
Show resolved Hide resolved

@interface RLMSyncConfiguration () {
Expand Down
23 changes: 23 additions & 0 deletions Realm/RLMSyncConfiguration_Private.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -20,16 +20,39 @@

#import <functional>
#import <memory>
#import <string>

namespace realm {
class Realm;
class SyncSession;
struct SyncConfig;
struct SyncError;
using SyncSessionErrorHandler = void(std::shared_ptr<SyncSession>, SyncError);
class ThreadSafeReference;
}

RLM_HEADER_AUDIT_BEGIN(nullability, sendability)

namespace realm {

struct CallbackSchema {
bool dynamic;
std::string path;
RLMSchema *customSchema;

RLMSchema *getSchema(realm::Realm& realm);
};

struct BeforeClientResetWrapper : CallbackSchema {
RLMClientResetBeforeBlock block;
void operator()(std::shared_ptr<realm::Realm> local);
};
struct AfterClientResetWrapper : CallbackSchema {
RLMClientResetAfterBlock block;
void operator()(std::shared_ptr<realm::Realm> local, realm::ThreadSafeReference remote, bool);
};
}

@interface RLMSyncConfiguration ()

- (instancetype)initWithRawConfig:(realm::SyncConfig)config path:(std::string const&)path;
Expand Down