feat(cpp): add functions related to user management, and initial high-level client - #3733
feat(cpp): add functions related to user management, and initial high-level client#3733slbotbm wants to merge 11 commits into
Conversation
github-actions
Bot
added
S-waiting-on-review
| }) | ||
| } | ||
|
|
||
| pub fn update_user( |
There was a problem hiding this comment.
Rust SDK allows updating username and status independently (Option params), but this forces both. Consider has_username / has_status flags like the existing has_permissions pattern
| * @return Information about the authenticated session. | ||
| * @throws IggyException if authentication fails. | ||
| */ | ||
| LoginInfo Login(std::string username, std::string password) const; |
There was a problem hiding this comment.
Login() returning ffi::LoginInfo leaks the FFI's has_access_token bool-flag workaround into the high-level API. Since this is meant to be the primary API, a dedicated type with std::optional for the token would be cleaner, and avoids a breaking change later.
There was a problem hiding this comment.
tbh this is currently a workaround for cxx's limitations. I eventually plan to convert all structs defined in rust to cpp structs/enums and call them from rust. That'll give the users a better experience as compared to now. I'll remember this, and implement this when I make that change.
| * @throws IggyException if the connection or automatic authentication | ||
| * fails. | ||
| */ | ||
| void Connect() const; |
There was a problem hiding this comment.
Nit: These methods are const but clearly mutate connection state (it only compiles because the state lives behind client_). Marking them non-const would better communicate the semantics, since const usually implies no observable state change and often thread-safety.
Applies to:
Connect()Disconnect()Shutdown()Login()Logout()
| /// Creates a new client connection and returns a raw pointer to the underlying [`Client`]. | ||
| /// | ||
| /// # Ownership | ||
| /// | ||
| /// The returned `*mut Client` is owned by the caller (the C++ side). The caller is responsible | ||
| /// for calling [`delete_connection`] exactly once to release the resources. Failing to do so | ||
| /// leaks the underlying tokio runtime resources and the open network connection. | ||
| /// |
There was a problem hiding this comment.
This and iggy.hpp:389-395 says destroying the client releases the connection. IggyClient::connect spawns a heartbeat holding its own Arc to the transport, exiting only on ClientShutdown, so the socket, the server session and the ping loop all survive. Same reason :488 ("no effect when already connected") is false and the :501-505 reuse cycle leaks a task per cycle. Worth filing upstream: store the JoinHandle discarded at clients/client.rs:250 and abort it in a Drop for IggyClient.
| * @note HTTP is stateless; connecting initializes heartbeat processing but | ||
| * does not open a persistent transport connection. | ||
| * @throws IggyException if the connection or automatic authentication | ||
| * fails. |
There was a problem hiding this comment.
This promises Connect() throws on failure. With the documented default (unlimited retries) tcp_client.rs:424-442 retries forever and block_on parks the caller indefinitely. It can only throw if WithReconnectionMaxRetries(n) was called.
| if config.has_tls_enabled { | ||
| builder = builder.with_tls_enabled(config.tls_enabled); | ||
| if config.tls_enabled { | ||
| if !config.tls_domain.is_empty() { | ||
| builder = builder.with_tls_domain(config.tls_domain); | ||
| } | ||
| if !config.tls_ca_file.is_empty() { | ||
| builder = builder.with_tls_ca_file(config.tls_ca_file); | ||
| } | ||
| if config.has_tls_validate_certificate { | ||
| builder = builder.with_tls_validate_certificate(config.tls_validate_certificate); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
has_tls_enabled: Builder().WithTlsDomain(d).WithTlsCaFile(p).Build() without WithTlsEnabled(true) silently drops all three TLS settings and connects in plaintext. The gate carries no information either, since TcpClientConfig::default() is already tls_enabled: false. Suggest deleting the field, calling with_tls_enabled unconditionally, and erroring when TLS material is set with TLS off. - set_reconnection_max_retries (src/lib.rs:348): dead. "Never set" and WithoutReconnectionLimit() are indistinguishable, and {set_=false, has_=true, value=n} is representable and silently ignored. has_reconnection_max_retries alone expresses Option<u32>.
3 participants
Which issue does this PR address?
Closes #
Relates to #2100
Rationale
Works towards completion of cpp client.
What changed?
Adds the following:
and their tests.
Adds high level client that will function as primary API with documentation. The diff looks big since a lot of the public api was changed and aligned to PascalCase. I have not rewritten the tests using the high level client since I would like to do that at once instead of a gradual approach.
Local Execution
AI Usage
If AI tools were used, please answer: