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.

Sign up for GitHub

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

RCORE-2201 Add an implementation for the realm_app_config_get_sync_client_config() function in the Core CAPI #7891

Merged
merged 9 commits into from
Jul 17, 2024
Merged

RCORE-2201 Add an implementation for the realm_app_config_get_sync_client_config() function in the Core CAPI #7891

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
193f2f3
Added back realm_app_config_set_sync_client_config() CAPI function
Jul 16, 2024
7233c68
Updated changelog and fixed compilation error
Jul 16, 2024
88d1012
Merge branch 'master' of github.com:realm/realm-core into mwb/fix-get…
Jul 16, 2024
0d641fc
Added setter functions for the sync_client_config in app_config
Jul 16, 2024
3916d42
updated changelog
Jul 16, 2024
449179f
Fixed compilation error
Jul 17, 2024
bc7ff1b
Use different realm_sync_client_config_t based on REALM_APP_SERVICES …
Jul 17, 2024
b68b707
Added comments around use of realm_sync_client_config_t when using RE…
Jul 17, 2024
170b31c
Use static_cast<> instead of reinterpret_cast<>
Jul 17, 2024
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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
* On Windows devices Device Sync will additionally look up SSL certificates in the Windows Trusted Root Certification Authorities certificate store when establishing a connection. (PR [#7882](https://github.com/realm/realm-core/pull/7882))
* Updated the return type of `LogCategory::get_category_names()` from `std::vector<const char*>` to `std::vector<std::string_view>`. ([PR #7879](https://github.com/realm/realm-core/pull/7879))
* Added `realm_get_persisted_schema_version` for reading the version of the schema currently stored locally. (PR [#7873](https://github.com/realm/realm-core/pull/7873))
* Added `realm_app_config_get_sync_client_config()` function to the C_API to get the sync_client_config value in `realm_app_config_t` if REALM_APP_SERVICES is enabled. If REALM_APP_SERVICES is not available, `realm_sync_client_config_new()` is available to create a new `sync_client_config_t` to use when initializing the sync manager. ([PR #7891](https://github.com/realm/realm-core/pull/7891))

### Fixed
* <How do the end-user experience this issue? what was the impact?> ([#????](https://github.com/realm/realm-core/issues/????), since v?.?.?)
Expand Down
17 changes: 14 additions & 3 deletions src/realm.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3023,8 +3023,6 @@ RLM_API void realm_app_config_set_metadata_mode(realm_app_config_t*,
RLM_API void realm_app_config_set_metadata_encryption_key(realm_app_config_t*, const uint8_t[64]) RLM_API_NOEXCEPT;
RLM_API void realm_app_config_set_security_access_group(realm_app_config_t*, const char*) RLM_API_NOEXCEPT;

RLM_API realm_sync_client_config_t* realm_app_config_get_sync_client_config(realm_app_config_t*) RLM_API_NOEXCEPT;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't fully understand why this getter couldn't just be implemented. The issue I see is that with only a setter available, it becomes impossible to just set one property of the sync_client_config because realm_app_config_set_sync_client_config will overwrite everything. This means that this API will only allow a use like this:

  1. make a new sync_client_config
  2. set all the properties
  3. use realm_app_config_set_sync_client_config to apply the changes
  4. any further changes to the sync_client_config instance are not actually going to change the config that the app uses, and you'd have to do step 3 again

Whereas if we support a getter that returns a pointer to the already existing instance AppConfig::sync_client_config then users of this API can just get that pointer and modify it directly with the various setters, and never need to allocate memory on their side of things.

Copy link
Member

Choose a reason for hiding this comment

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

The problem is that in the non-appservices scenario users allocate and free realm_sync_client_config_t, which requires that it inherit from WrapC. Owning and non-owning pointers need to be different types with how we've structured the C API. We could add a second type, but a simpler variation on that would be to add getters and setters for the SyncConfig fields that just take an AppConfig.

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 have been trying to add a second type that also uses the existing functions, but I don't think I've been able to get it just right without having to add separate functions for the second type. I tried creating a realm_sync_client_config that inherits from realm::SyncClientConfig and used by all the functions; and a realm_sync_client_config_wrapc that inherits from WrapC and realm_sync_client_config, which is returned by the sync_client_config_new() function....
I would be happy to just add setters for the SyncConfig fields (we usually don't have getters on most of the struct fields).

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, that is an unfortunate conflict that muddies the API. Whatever seems easiest is fine.
If the SDKs use the realm_app_config_set_sync_client_config function as it is now, then I suppose they have created a new config instance that they manage the lifetime of anyways, which means that if they want to update a single property, they could set it on their instance, and then update the entire thing again using the setter. So not the end of the world.

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 left the realm_app_config_set_sync_client_config() function in the code and also added individual C_API setter functions for the SyncClientConfig parameters in the app config to give the SDK a choice as to what they want to use. I could also easily add a realm_app_config_get_sync_client_config() function that returns a copy of the sync_client_config, but I thought that might be confusing/misleading...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ironage - since the realm_sync_client_config_new() is not needed if REALM_APP_SERVICES is enabled, I just updated the realm_sync_client_config_t to be one of the two different types depending on the REALM_APP_SERVICES value:

  • If it is set, realm_app_config_get_sync_client_config() is now available to get the sync client config from app config.
  • If it is not set, realm_sync_client_config_new() is available to create the sync client config to use with the realm_sync_manager_create(), which is also only available if REALM_APP_SERVICES is not set.


/**
* Get an existing @a realm_app_credentials_t and return it's json representation
* Note: the caller must delete the pointer to the string via realm_release
Expand Down Expand Up @@ -3746,8 +3744,21 @@ typedef void (*realm_async_open_task_completion_func_t)(realm_userdata_t userdat
// callback runs.
typedef void (*realm_async_open_task_init_subscription_func_t)(realm_thread_safe_reference_t* realm,
realm_userdata_t userdata);

#if REALM_APP_SERVICES
// If using App Services, the realm_sync_client_config_t instance is part of the
// realm_app_config_t structure and this function returns a pointer to that
// member property. The realm_sync_client_config_t reference returned by this
// function should not be freed using realm_release.
RLM_API realm_sync_client_config_t* realm_app_config_get_sync_client_config(realm_app_config_t*) RLM_API_NOEXCEPT;
Copy link
Contributor

Choose a reason for hiding this comment

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

We should add a comment here that the returned pointer is owned by core and that it shouldn't be released and maybe some other remarks on lifecycle.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I forgot to complete my comment....
It has been updated

#else
// If not using App Services, the realm_app_config_t structure is not defined, and
// the real_sync_client_config_t structure returned by this function is meant to be
// used with realm_sync_manager_create() to create a separate Sync Manager instance.
// The realm_sync_client_config_t instance returned by this function will need to be
// manually freed using realm_release.
RLM_API realm_sync_client_config_t* realm_sync_client_config_new(void) RLM_API_NOEXCEPT;
#endif // REALM_APP_SERVICES

RLM_API void realm_sync_client_config_set_reconnect_mode(realm_sync_client_config_t*,
realm_sync_client_reconnect_mode_e) RLM_API_NOEXCEPT;
RLM_API void realm_sync_client_config_set_multiplex_sessions(realm_sync_client_config_t*, bool) RLM_API_NOEXCEPT;
Expand Down
5 changes: 5 additions & 0 deletions src/realm/object-store/c_api/app.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -279,6 +279,11 @@ RLM_API const char* realm_app_credentials_serialize_as_json(realm_app_credential
});
}

RLM_API realm_sync_client_config_t* realm_app_config_get_sync_client_config(realm_app_config_t* app_config) noexcept
{
return static_cast<realm_sync_client_config_t*>(&app_config->sync_client_config);
}

RLM_API realm_app_t* realm_app_create(const realm_app_config_t* app_config)
{
return wrap_err([&] {
Expand Down
2 changes: 2 additions & 0 deletions src/realm/object-store/c_api/sync.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,10 +128,12 @@ static Query add_ordering_to_realm_query(Query realm_query, const DescriptorOrde
return realm_query;
}

#if !REALM_APP_SERVICES
RLM_API realm_sync_client_config_t* realm_sync_client_config_new(void) noexcept
{
return new realm_sync_client_config_t;
}
#endif // !REALM_APP_SERVICES

RLM_API void realm_sync_client_config_set_reconnect_mode(realm_sync_client_config_t* config,
realm_sync_client_reconnect_mode_e mode) noexcept
Expand Down
12 changes: 12 additions & 0 deletions src/realm/object-store/c_api/types.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -617,9 +617,21 @@ struct realm_http_transport : realm::c_api::WrapC, std::shared_ptr<realm::app::G
}
};

#if REALM_APP_SERVICES
// This class doesn't support realm_release() since it is only meant to be used
// as a CAPI-compatible reference to the SyncClientConfig member variable that
// is part of AppConfig.
// To avoid data misalignment or other conflicts with the original SyncClientConfig,
// do not add any additional functions or member variables to this class.
struct realm_sync_client_config final : realm::SyncClientConfig {
using SyncClientConfig::SyncClientConfig;
};
#else
// This class must be freed using realm_release()
struct realm_sync_client_config : realm::c_api::WrapC, realm::SyncClientConfig {
using SyncClientConfig::SyncClientConfig;
};
#endif // REALM_APP_SERVICES

struct realm_sync_config : realm::c_api::WrapC, realm::SyncConfig {
using SyncConfig::SyncConfig;
Expand Down
97 changes: 65 additions & 32 deletions test/object-store/c_api/c_api.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
#include "../util/test_path.hpp"
#include "../util/unit_test_transport.hpp"

#include <realm/object-store/c_api/types.hpp>
#include <realm/object-store/sync/app_utils.hpp>
#include <realm/object-store/sync/sync_user.hpp>
#include <realm/sync/client_base.hpp>
Expand Down Expand Up @@ -537,36 +538,51 @@ TEST_CASE("C API (non-database)", "[c_api]") {

#if REALM_ENABLE_SYNC
SECTION("sync_client_config_t") {
#if REALM_APP_SERVICES
// Make a dummy app config and make sure the sync_client_config is updated
const uint64_t request_timeout = 2500;
auto transport = std::make_shared<UnitTestTransport>(request_timeout);
auto http_transport = realm_http_transport(transport);
auto app_config = cptr(realm_app_config_new("app_id_123", &http_transport));
auto sync_client_config = realm_app_config_get_sync_client_config(app_config.get());
#else
auto test_sync_client_config = cptr(realm_sync_client_config_new());
realm_sync_client_config_set_reconnect_mode(test_sync_client_config.get(),
RLM_SYNC_CLIENT_RECONNECT_MODE_TESTING);
CHECK(test_sync_client_config->reconnect_mode ==
auto sync_client_config = test_sync_client_config.get();
#endif // REALM_APP_SERVICES

realm_sync_client_config_set_reconnect_mode(sync_client_config, RLM_SYNC_CLIENT_RECONNECT_MODE_TESTING);
CHECK(sync_client_config->reconnect_mode ==
static_cast<ReconnectMode>(RLM_SYNC_CLIENT_RECONNECT_MODE_TESTING));
realm_sync_client_config_set_multiplex_sessions(test_sync_client_config.get(), true);
CHECK(test_sync_client_config->multiplex_sessions);
realm_sync_client_config_set_multiplex_sessions(test_sync_client_config.get(), false);
CHECK_FALSE(test_sync_client_config->multiplex_sessions);
realm_sync_client_config_set_user_agent_binding_info(test_sync_client_config.get(), "some user agent stg");
CHECK(test_sync_client_config->user_agent_binding_info == "some user agent stg");
realm_sync_client_config_set_user_agent_application_info(test_sync_client_config.get(), "some application");
CHECK(test_sync_client_config->user_agent_application_info == "some application");
realm_sync_client_config_set_connect_timeout(test_sync_client_config.get(), 666);
CHECK(test_sync_client_config->timeouts.connect_timeout == 666);
realm_sync_client_config_set_connection_linger_time(test_sync_client_config.get(), 999);
CHECK(test_sync_client_config->timeouts.connection_linger_time == 999);
realm_sync_client_config_set_ping_keepalive_period(test_sync_client_config.get(), 555);
CHECK(test_sync_client_config->timeouts.ping_keepalive_period == 555);
realm_sync_client_config_set_pong_keepalive_timeout(test_sync_client_config.get(), 100000);
CHECK(test_sync_client_config->timeouts.pong_keepalive_timeout == 100000);
realm_sync_client_config_set_fast_reconnect_limit(test_sync_client_config.get(), 1099);
CHECK(test_sync_client_config->timeouts.fast_reconnect_limit == 1099);
realm_sync_client_config_set_resumption_delay_interval(test_sync_client_config.get(), 1024);
CHECK(test_sync_client_config->timeouts.reconnect_backoff_info.resumption_delay_interval.count() == 1024);
realm_sync_client_config_set_max_resumption_delay_interval(test_sync_client_config.get(), 600024);
CHECK(test_sync_client_config->timeouts.reconnect_backoff_info.max_resumption_delay_interval.count() ==
600024);
realm_sync_client_config_set_resumption_delay_backoff_multiplier(test_sync_client_config.get(), 1010);
CHECK(test_sync_client_config->timeouts.reconnect_backoff_info.resumption_delay_backoff_multiplier == 1010);
realm_sync_client_config_set_multiplex_sessions(sync_client_config, true);
CHECK(sync_client_config->multiplex_sessions);
realm_sync_client_config_set_multiplex_sessions(sync_client_config, false);
realm_sync_client_config_set_user_agent_binding_info(sync_client_config, "some user agent stg");
realm_sync_client_config_set_user_agent_application_info(sync_client_config, "some application");
realm_sync_client_config_set_connect_timeout(sync_client_config, 666);
realm_sync_client_config_set_connection_linger_time(sync_client_config, 999);
realm_sync_client_config_set_ping_keepalive_period(sync_client_config, 555);
realm_sync_client_config_set_pong_keepalive_timeout(sync_client_config, 100000);
realm_sync_client_config_set_fast_reconnect_limit(sync_client_config, 1099);
realm_sync_client_config_set_resumption_delay_interval(sync_client_config, 1024);
realm_sync_client_config_set_max_resumption_delay_interval(sync_client_config, 600024);
realm_sync_client_config_set_resumption_delay_backoff_multiplier(sync_client_config, 1010);
auto verify_sync_client_config = [](SyncClientConfig* config) {
CHECK_FALSE(config->multiplex_sessions);
CHECK(config->user_agent_binding_info == "some user agent stg");
CHECK(config->user_agent_application_info == "some application");
CHECK(config->timeouts.connect_timeout == 666);
CHECK(config->timeouts.connection_linger_time == 999);
CHECK(config->timeouts.ping_keepalive_period == 555);
CHECK(config->timeouts.pong_keepalive_timeout == 100000);
CHECK(config->timeouts.fast_reconnect_limit == 1099);
CHECK(config->timeouts.reconnect_backoff_info.resumption_delay_interval.count() == 1024);
CHECK(config->timeouts.reconnect_backoff_info.max_resumption_delay_interval.count() == 600024);
CHECK(config->timeouts.reconnect_backoff_info.resumption_delay_backoff_multiplier == 1010);
};
verify_sync_client_config(sync_client_config);
#if REALM_APP_SERVICES
verify_sync_client_config(&app_config->sync_client_config);
#endif // REALM_APP_SERVICES
}

#if !REALM_APP_SERVICES
Expand Down Expand Up @@ -824,6 +840,11 @@ TEST_CASE("C API (non-database)", "[c_api]") {
realm_app_config_set_metadata_mode(app_config.get(), RLM_SYNC_CLIENT_METADATA_MODE_DISABLED);
realm_app_config_set_security_access_group(app_config.get(), "");

auto sync_client_config = realm_app_config_get_sync_client_config(app_config.get());
realm_sync_client_config_set_connect_timeout(sync_client_config, 9876543); // some bogus value
// Make sure app_config's sync_client_config has the value we set
CHECK(app_config->sync_client_config.timeouts.connect_timeout == 9876543);

auto test_app = cptr(realm_app_create(app_config.get()));
realm_user_t* sync_user;
auto user_data_free = [](realm_userdata_t) {};
Expand Down Expand Up @@ -5996,9 +6017,15 @@ TEST_CASE("C API - binding callback thread observer", "[sync][c_api]") {
};

{
auto config = cptr(realm_sync_client_config_new());
#if REALM_APP_SERVICES
struct realm_sync_client_config config_struct {};
auto config = &config_struct;
#else
auto config_ptr = cptr(realm_sync_client_config_new());
auto config = config_ptr.get();
#endif // REALM_APP_SERVICES
realm_sync_client_config_set_default_binding_thread_observer(
config.get(), bcto_on_thread_create, bcto_on_thread_destroy, bcto_on_thread_error,
config, bcto_on_thread_create, bcto_on_thread_destroy, bcto_on_thread_error,
static_cast<realm_userdata_t>(&bcto_user_data), bcto_free_userdata);
REQUIRE(config->default_socket_provider_thread_observer);
auto observer_ptr =
Expand Down Expand Up @@ -6029,8 +6056,14 @@ TEST_CASE("C API - binding callback thread observer", "[sync][c_api]") {
REQUIRE(bcto_user_data.bcto_deleted == true);

{
auto config = cptr(realm_sync_client_config_new());
realm_sync_client_config_set_default_binding_thread_observer(config.get(), nullptr, nullptr, nullptr, nullptr,
#if REALM_APP_SERVICES
struct realm_sync_client_config config_struct {};
auto config = &config_struct;
#else
auto config_ptr = cptr(realm_sync_client_config_new());
auto config = config_ptr.get();
#endif // REALM_APP_SERVICES
realm_sync_client_config_set_default_binding_thread_observer(config, nullptr, nullptr, nullptr, nullptr,
nullptr);
auto no_handle_error_ptr =
static_cast<CBindingThreadObserver*>(config->default_socket_provider_thread_observer.get());
Expand Down
Loading