glue code for CAPI custom user via callbacks #7615
Conversation
ironage
added
no-changelog
no-jira-ticket
| typedef const char* (*realm_user_get_refresh_token_cb_t)(realm_userdata_t userdata); | ||
| typedef realm_user_state_e (*realm_user_state_cb_t)(realm_userdata_t userdata); | ||
| typedef bool (*realm_user_access_token_refresh_required_cb_t)(realm_userdata_t userdata); | ||
| typedef realm_sync_manager_t* (*realm_user_get_sync_manager_cb_t)(realm_userdata_t userdata); |
There was a problem hiding this comment.
I'm not sure I understand why this is a callback into the SDK (this is probably a decision made in the previous PR, but it's confusing to me).
src/realm/object-store/c_api/app.cpp
Outdated
| m_request_log_out_cb(m_userdata); | ||
| } | ||
| } | ||
| void request_refresh_user(CompletionHandler&& callback) override |
There was a problem hiding this comment.
What is this used for and how is it different from request_access_token?
There was a problem hiding this comment.
This was intended to refresh the user's profile data / identities. But that is actually never used from the sync session. I'll remove it from the SyncUser interface.
src/realm.h
Outdated
| /** | ||
| * Create realm_sync_manager_t* instance given a valid realm sync client configuration. | ||
| * | ||
| * @return A non-null pointer if no error occurred. | ||
| */ | ||
| RLM_API realm_sync_manager_t* realm_sync_manager_create(const realm_sync_client_config_t*); | ||
|
|
||
| /** | ||
| * See SyncManager::set_sync_route() | ||
| */ | ||
| RLM_API void realm_sync_manager_set_route(const realm_sync_manager_t* session, const char* route, bool is_verified); |
There was a problem hiding this comment.
I don't think the SDK should ever be creating the sync manager or setting its route.
There was a problem hiding this comment.
The sync manager now has a separate lifetime that is managed by core's App implementation. It is no longer tied to the lifetime of any specific user. This allows a SyncUser instance to outlive its sync sessions in a removed/offline state (in the past we had issues with cyclical shared pointers that kept each other alive). Due to that design the SDK must provide a backing SyncManager instance through the SyncUser interface. That's how things currently stand.
There was a problem hiding this comment.
This is not introduced by this PR, so probably safe to punt on it, but we'll probably need to discuss these lifetimes, as the current design doesn't make a lot of sense to me. To be able to construct the sync manager from the user, we'll need the managed app configuration (where all the sync manager settings are currently configured), which would be extremely awkward to stick on the managed user. So we'll probably have to add a backreference to the managed app that owns the user, tying the lifetime of the managed (and thus the native) user and app instances together.
So the way I see it, the only case where a user might briefly outlive an app instance is when the garbage collector detects that both are eligible for collection and collects the app instance before the user one, but in that case, attempting to create a sync manager in the brief moment when an app is collected, but a user isn't would never work.
There was a problem hiding this comment.
The user should not be creating the manager; the manager should be created in advance and then the user should hold a reference to the manager. The API has always been designed around passing around SyncUsers and obtaining everything needed from that.
Pull Request Test Coverage Report for Build james.stone_534Details
💛 - Coveralls |
|
We should probably move all the C API for getting user details, such as |
Allows SDKs using the C-API to compile out core's app services implementation and create a custom SyncUser by plugging callbacks into each of SyncUser's virtual methods.
No changelog because the feature hasn't shipped yet.