Allow multiple app instances with the same ID but different base URLs #6480
Conversation
|
Realm welcomes all contributions! The only requirement we have is that, like many other projects, we need to have a Contributor License Agreement (CLA) in place before we can accept any external code. Our own CLA is a modified version of the Apache Software Foundation’s CLA. Our records show that CLA has not been signed by @kmorkos. Please submit your CLA electronically using our Google form so we can accept your submissions. After signing the CLA you can recheck this PR with a |
|
@cla-bot check |
|
The cla-bot has been summoned, and re-checked this pull request! |
CHANGELOG.md
Outdated
| @@ -1,8 +1,7 @@ | |||
| # NEXT RELEASE | |||
|
|
|||
| ### Enhancements | |||
| * <New feature description> (PR [#????](https://github.com/realm/realm-core/pull/????)) | |||
| * None. | |||
| * Allow multiple app instances with the same ID but different base URLs ([#6480](https://github.com/realm/realm-core/pull/6480)) | |||
There was a problem hiding this comment.
I'd add something here about supporting multiple app instances via get_shared_app() and get_cached_app() specifically - the changelog is used by the SDKs to know when/what to change when they're adopting a version of core and to communicate to users what they'll need to change.
There was a problem hiding this comment.
Good call, will update 👍🏼
test/object-store/sync/app.cpp
Outdated
|
|
||
| auto config2 = base_config; | ||
| config2.app_id = "app1"; | ||
| config2.base_url = "https://realm.mongodb.com"; |
There was a problem hiding this comment.
Add a comment that this is supposed to equal the default_base_url value
| auto app2_2 = App::get_cached_app(config3.app_id, config3.base_url); | ||
| auto app2_3 = App::get_shared_app(config4, sync_config); | ||
| auto app2_4 = App::get_cached_app(config3.app_id); | ||
|
|
There was a problem hiding this comment.
So we verify getting the cached app when there are two different URLs, let's add one more:
auto app2_5 = App::get_cached_app(config4.app_id, config4.base_url);
And then check that app2_3 == app2_5
Also auto app2_6 = App::get_cached_app(config4.app_id, "https://some.different.url");
And then check that app2_6 == nullptr
There was a problem hiding this comment.
So we verify getting the cached app when there are two different URLs, let's add one more:
auto app2_5 = App::get_cached_app(config4.app_id, config4.base_url);
And then check thatapp2_3 == app2_5
This is the same scenario as CHECK(app1_1 == app1_3); above
Also auto app2_6 = App::get_cached_app(config4.app_id, "https://some.different.url");
And then check that app2_6 == nullptr
I'll add this one 👍🏼
src/realm/object-store/sync/app.cpp
Outdated
| return it->second; | ||
| const auto& apps_by_url = it->second; | ||
|
|
||
| auto app_it = base_url.has_value() ? apps_by_url.find(*base_url) : apps_by_url.begin(); |
There was a problem hiding this comment.
You don't need to call has_value() - since it's already evaluating to a bool, you can do this:
auto app_it = base_url ? apps_by_url.find(*base_url) : apps_by_url.begin();
Looks like that one is no longer used either according to my github search, I can remove it here as well |
What, How & Why?
Updates
s_apps_cacheto be a multi-level map keyed off the base URL in addition to app ID. To avoid making this a breaking change,get_cached_appnow optionally accepts the base URL as a second parameter. If it is not provided then the behavior is non-deterministic when there's multiple app configs with the same app ID but different base URLs (but still no worse than the current behavior).Also worth noting that the changes to
get_cached_appare not exposed in the C api since that method is being removed as per #5644 (comment)☑️ ToDos